WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
Patch v2
(135.13 KB, patch)
2015-06-01 17:15 PDT
,
Brady Eidson
no flags
Details
Formatted Diff
Diff
Patch v3 - More build fixing
(139.99 KB, patch)
2015-06-02 10:08 PDT
,
Brady Eidson
no flags
Details
Formatted Diff
Diff
Patch v4 - More correct build fixing
(140.03 KB, patch)
2015-06-02 10:44 PDT
,
Brady Eidson
achristensen
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
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
http://trac.webkit.org/changeset/185111
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.
Top of Page
Format For Printing
XML
Clone This Bug