Bug 145280 – WebKit policy delegate should suggest if a navigation should be allowed to open URLs externally

• Component: WebKit API
• Status: RESOLVED FIXED

[Brady Eidson]

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>

[Brady Eidson]

Title change:
WebKit policy delegate should suggest if a navigation should be allowed to open URLs externally

[Brady Eidson]

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]

Step 1 mentioned above tracked in https://bugs.webkit.org/show_bug.cgi?id=145417

[Brady Eidson]

Created attachment 254018 [details]
Patch v1

[Brady Eidson]

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 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]

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

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]

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]

Created attachment 254070 [details]
Patch v3 - More build fixing

[Brady Eidson]

Created attachment 254073 [details]
Patch v4 - More correct build fixing

[Alex Christensen]

r=me.  Media and font test failures are unrelated.

[Brady Eidson]

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]

http://trac.webkit.org/changeset/185111

[Geoffrey Garen]

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]

(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