RESOLVED FIXED 145280
WebKit policy delegate should suggest if a navigation should be allowed to open URLs externally
https://bugs.webkit.org/show_bug.cgi?id=145280
Summary WebKit policy delegate should suggest if a navigation should be allowed to op...
Brady Eidson
Reported 2015-05-21 13:18:34 PDT
WebKit policy delegate should indicate ongoing user navigations. This includes carrying the "was this load the result of a user gesture?" carrying forward over redirects and subsequent main frame navigations. <rdar://problem/21025301>
Attachments
Patch v1 (132.09 KB, patch)
2015-06-01 15:30 PDT, Brady Eidson
beidson: review-
Patch v2 (135.13 KB, patch)
2015-06-01 17:15 PDT, Brady Eidson
no flags
Patch v3 - More build fixing (139.99 KB, patch)
2015-06-02 10:08 PDT, Brady Eidson
no flags
Patch v4 - More correct build fixing (140.03 KB, patch)
2015-06-02 10:44 PDT, Brady Eidson
achristensen: review+
Brady Eidson
Comment 1 2015-05-27 15:37:32 PDT
Title change: WebKit policy delegate should suggest if a navigation should be allowed to open URLs externally
Brady Eidson
Comment 2 2015-05-27 15:41:44 PDT
Two further patches: 1 - Add a "should open URLs externally" flag to DocumentLoader. The flag gets there from FrameLoadRequest. The flag is propagated out to WebKit navigation delegate API. In step 1, the flag will only ever be set from the WKWebView loadRequest: API 2 - Propagate that flag from one DocumentLoader to its descendant DocumentLoader under certain circumstances. Also, WebCore will sometimes decide to set the flag on a DocumentLoader by itself when a navigation meets certain criteria (Such as the navigation is in the main frame, and is the result of a user gesture)
Brady Eidson
Comment 3 2015-05-27 15:45:05 PDT
Step 1 mentioned above tracked in https://bugs.webkit.org/show_bug.cgi?id=145417
Brady Eidson
Comment 4 2015-06-01 15:30:47 PDT
Created attachment 254018 [details] Patch v1
Brady Eidson
Comment 5 2015-06-01 16:10:16 PDT
Comment on attachment 254018 [details] Patch v1 Nevermind on the review for now - Obviously the tests aren't in good shape yet.
Alex Christensen
Comment 6 2015-06-01 16:29:56 PDT
Comment on attachment 254018 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=254018&action=review This is missing a change to Source/WebKit/ios/WebView/WebPDFViewPlaceholder.mm and this patch causes lots of crashes. You're probably dereferencing a null pointer somewhere. > Source/WebCore/loader/FrameLoader.cpp:1326 > RefPtr<DocumentLoader> loader = m_client.createDocumentLoader(request, defaultSubstituteDataForURL(request.url())); Does createDocumentLoader always succeed? > Source/WebCore/page/DOMWindow.cpp:2139 > + Frame* activeFrame = activeWindow.frame(); > + if (!activeFrame) > + return nullptr; This isn't used. Is it helpful to return early if there is no active frame?
Brady Eidson
Comment 7 2015-06-01 16:55:11 PDT
(In reply to comment #6) > Comment on attachment 254018 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=254018&action=review > > This is missing a change to > Source/WebKit/ios/WebView/WebPDFViewPlaceholder.mm and this patch causes > lots of crashes. You're probably dereferencing a null pointer somewhere. That's why I pulled it from review :) > > > Source/WebCore/loader/FrameLoader.cpp:1326 > > RefPtr<DocumentLoader> loader = m_client.createDocumentLoader(request, defaultSubstituteDataForURL(request.url())); > > Does createDocumentLoader always succeed? Yup.
Brady Eidson
Comment 8 2015-06-01 17:15:03 PDT
Created attachment 254030 [details] Patch v2 This is for EWS and, if it passes, for review. But not yet for landing, as I haven't updated the ChangeLogs yet.
Brady Eidson
Comment 9 2015-06-01 21:31:15 PDT
Messed up the iOS build fix, and now that windows EWS is finally helping, will fix that one too. Rest of the patch is still reviewable at this point.
Brady Eidson
Comment 10 2015-06-02 10:08:03 PDT
Created attachment 254070 [details] Patch v3 - More build fixing
Brady Eidson
Comment 11 2015-06-02 10:44:58 PDT
Created attachment 254073 [details] Patch v4 - More correct build fixing
Alex Christensen
Comment 12 2015-06-02 11:31:14 PDT
r=me. Media and font test failures are unrelated.
Brady Eidson
Comment 13 2015-06-02 11:32:05 PDT
The test failures on mac/mac-wk2 are unrelated (Those tester bots are in a non-green status right now, anyways) I'll fix up the ChangeLogs then land.
Brady Eidson
Comment 14 2015-06-02 11:43:54 PDT
Geoffrey Garen
Comment 15 2015-06-02 11:58:45 PDT
Comment on attachment 254073 [details] Patch v4 - More correct build fixing Other interesting test cases: (1) Do a top-level (Safari-like) navigation that sets the flag to true, then another that sets it to false. Flag should be false. (2) Do a top-level (Safari-like) navigation that sets the flag to false, then another that sets it to true. Flag should be true. (3) Do a top-level (Safari-like) navigation that sets the flag to false, then a programmatic click event on a link. Flag should be false.
Brady Eidson
Comment 16 2015-06-02 12:13:26 PDT
(In reply to comment #15) > Comment on attachment 254073 [details] > Patch v4 - More correct build fixing > > Other interesting test cases: > > (1) Do a top-level (Safari-like) navigation that sets the flag to true, then > another that sets it to false. Flag should be false. > > (2) Do a top-level (Safari-like) navigation that sets the flag to false, > then another that sets it to true. Flag should be true. > > (3) Do a top-level (Safari-like) navigation that sets the flag to false, > then a programmatic click event on a link. Flag should be false. Great suggestions. Following up in https://bugs.webkit.org/show_bug.cgi?id=145558
Note You need to log in before you can comment on or make changes to this bug.