Bug 39564 - [Qt] DRT Support for setCustomPolicyDelegate
Summary: [Qt] DRT Support for setCustomPolicyDelegate
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Robert Hogan
URL:
Keywords: Qt
Depends on:
Blocks:
 
Reported: 2010-05-23 06:04 PDT by Robert Hogan
Modified: 2010-06-26 03:52 PDT (History)
2 users (show)

See Also:


Attachments
Patch (12.77 KB, patch)
2010-05-23 06:28 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (12.51 KB, patch)
2010-06-19 04:55 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (12.51 KB, patch)
2010-06-21 14:32 PDT, Robert Hogan
no flags Details | Formatted Diff | Diff
Patch (12.61 KB, patch)
2010-06-21 14:34 PDT, Robert Hogan
kenneth: review+
kenneth: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Hogan 2010-05-23 06:04:46 PDT
Unskips:

LayoutTests/fast/loader/policy-delegate-action-hit-test-zoomed.html
LayoutTests/fast/loader/onload-policy-ignore-for-frame.html
LayoutTests/fast/loader/reload-policy-delegate.html
LayoutTests/fast/loader/javascript-url-hierarchical-execution.html
Comment 1 Robert Hogan 2010-05-23 06:28:00 PDT
Created attachment 56821 [details]
Patch
Comment 2 Laszlo Gombos 2010-06-03 17:03:45 PDT
Comment on attachment 56821 [details]
Patch

> diff --git a/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp b/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp
> index 00359c0e2c824bcdc198e7a449c85a7d2cdee2d8..510ccef99f15ab738ef5807334a5b2be53d9e244 100644
> --- a/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp
> +++ b/WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp

[..]

>  
>  FrameLoaderClientQt::FrameLoaderClientQt()
>      : m_frame(0)
> @@ -1035,6 +1087,33 @@ void FrameLoaderClientQt::dispatchDecidePolicyForNavigationAction(FramePolicyFun
>      Q_ASSERT(m_webFrame);
>      QNetworkRequest r(request.toNetworkRequest(m_webFrame));
>      QWebPage*page = m_webFrame->page();
> +    PolicyAction result;
> +
> +    if (policyDelegateEnabled) {

The processing below seems to be needed only for DRT. I think this section of the code should be guarded with "if (dumpFrameLoaderCallbacks)" or some other similar mechanism.

> +
> +        RefPtr<Node> node;
> +        for (const Event* event = action.event(); event; event = event->underlyingEvent()) {
> +            if (event->isMouseEvent()) {
> +                const MouseEvent* mouseEvent =
> +                    static_cast<const MouseEvent*>(event);
> +                node = QWebFramePrivate::core(m_webFrame)->eventHandler()->hitTestResultAtPoint(
> +                    mouseEvent->absoluteLocation(), false).innerNonSharedNode();
> +                break;
> +            }
> +        }
> +
> +        printf("Policy delegate: attempt to load %s with navigation type '%s'%s\n",
> +               qPrintable(drtDescriptionSuitableForTestResult(request.url())), navigationTypeToString(action.type()),
> +               (node) ? qPrintable(" originating from " + drtDescriptionSuitableForTestResult(node, 0)) : "");
Comment 3 Robert Hogan 2010-06-04 12:00:14 PDT
> > +    if (policyDelegateEnabled) {
> 
> The processing below seems to be needed only for DRT. I think this section of the code should be guarded with "if (dumpFrameLoaderCallbacks)" or some other similar mechanism.

Everything inside policyDelegateEnabled is DRT specific anyway - I don't think anything else but DRT would have a use case for policyDelegateEnabled.
Comment 4 Simon Hausmann 2010-06-16 13:30:32 PDT
Comment on attachment 56821 [details]
Patch

WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:165
 +  static const char* linkClickedString = "link clicked";
I suggest to leave out these variables. They will be read-write pointers to read-only memory, causing unnecessary relocations.

WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:174
 +  static const char* navigationTypeToString(NavigationType type)
Instead I suggest to return the strings inline here, like return "form submitted";
Comment 5 Robert Hogan 2010-06-19 04:55:25 PDT
Created attachment 59184 [details]
Patch
Comment 6 Kenneth Rohde Christiansen 2010-06-21 13:55:03 PDT
Comment on attachment 59184 [details]
Patch

WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1110
 +  
hmm, should this printf always be printing out things when policyDelegateEnabled is true?

WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1100
 +                      static_cast<const MouseEvent*>(event);
why not just put that on one line

WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1095
 +  
why a new line here
Comment 7 Robert Hogan 2010-06-21 14:29:39 PDT
(In reply to comment #6)
> (From update of attachment 59184 [details])
> WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1110
>  +  
> hmm, should this printf always be printing out things when policyDelegateEnabled is true?
> 

Laszlo asked the same question and I think the answer is yes - policyDelegateEnabled is a dumpCallbacks type variable set exclusively by DRT.

> WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1100
>  +                      static_cast<const MouseEvent*>(event);
> why not just put that on one line
> 
> WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1095
>  +  
> why a new line here
Comment 8 Robert Hogan 2010-06-21 14:32:14 PDT
Created attachment 59290 [details]
Patch
Comment 9 Robert Hogan 2010-06-21 14:34:04 PDT
Created attachment 59292 [details]
Patch
Comment 10 Robert Hogan 2010-06-21 14:35:14 PDT
(In reply to comment #6)
> WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1100
>  +                      static_cast<const MouseEvent*>(event);
> why not just put that on one line
> 
> WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1095
>  +  
> why a new line here

updated patch to address these two!
Comment 11 Kenneth Rohde Christiansen 2010-06-25 15:13:43 PDT
Comment on attachment 59292 [details]
Patch

WebKit/qt/WebCoreSupport/FrameLoaderClientQt.cpp:1094
 +      if (policyDelegateEnabled) {
I still want a comment here explaining that this is only used by the DRT. You can commit if you fix that.
Comment 12 Robert Hogan 2010-06-26 03:52:58 PDT
Committed r61953: <http://trac.webkit.org/changeset/61953>