Bug 39564 – [Qt] DRT Support for setCustomPolicyDelegate

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Robert Hogan]

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

[Robert Hogan]

Created attachment 56821 [details]
Patch

[Laszlo Gombos]

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)) : "");

[Robert Hogan]

> > +    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.

[Simon Hausmann]

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";

[Robert Hogan]

Created attachment 59184 [details]
Patch

[Kenneth Rohde Christiansen]

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

[Robert Hogan]

(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

[Robert Hogan]

Created attachment 59290 [details]
Patch

[Robert Hogan]

Created attachment 59292 [details]
Patch

[Robert Hogan]

(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!

[Kenneth Rohde Christiansen]

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.

[Robert Hogan]

Committed r61953: <http://trac.webkit.org/changeset/61953>