Summary: | [Qt] DRT Support for setCustomPolicyDelegate | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Robert Hogan <robert> | ||||||||||
Component: | Tools / Tests | Assignee: | Robert Hogan <robert> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | laszlo.gombos, robert | ||||||||||
Priority: | P2 | Keywords: | Qt | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Attachments: |
|
Description
Robert Hogan
2010-05-23 06:04:46 PDT
Created attachment 56821 [details]
Patch
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)) : ""); > > + 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 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";
Created attachment 59184 [details]
Patch
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
(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 Created attachment 59290 [details]
Patch
Created attachment 59292 [details]
Patch
(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 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.
Committed r61953: <http://trac.webkit.org/changeset/61953> |