Bug 145505 – Add WKTR support for "should open external URLs"

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brady Eidson]

Add WKTR support for "should open external URLs"

This includes:
  -Adding C-spi, as WKTR still uses that
  -Adding a "dump policy delegate" call to WKTR
  -Adding basic tests for the "should open external URLs" support that already exists in the tree

[Brady Eidson]

Created attachment 253988 [details]
Patch v1

[Darin Adler]

Comment on attachment 253988 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=253988&action=review

> Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleNavigationActionPrivate.h:35
> +WK_EXPORT bool WKBundleNavigationActionGetShouldOpenExternalURLs(WKBundleNavigationActionRef);

Do we really want to add this just so we can log it? Seems like overkill to me. Isn’t there some other way to verify that the flag is set that is more in line with the feature actually works?

> Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleNavigationAction.h:60
> +    bool shouldOpenExternalURLs() const { return m_shouldOpenExternalURLs; }
>  private:

Strange formatting here. There was a blank line before private before.

[Brady Eidson]

(In reply to comment #2)
> Comment on attachment 253988 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253988&action=review
> 
> > Source/WebKit2/WebProcess/InjectedBundle/API/c/WKBundleNavigationActionPrivate.h:35
> > +WK_EXPORT bool WKBundleNavigationActionGetShouldOpenExternalURLs(WKBundleNavigationActionRef);
> 
> Do we really want to add this just so we can log it? Seems like overkill to
> me. Isn’t there some other way to verify that the flag is set that is more
> in line with the feature actually works?

There's no internal way to verify the feature besides inspecting what is passed to the navigation policy delegate.

That said, we didn't really want to expose this feature to the C-SPI at all.

Once WKTR moves to Modern WebKit (https://bugs.webkit.org/show_bug.cgi?id=143743) this can go away (as the API is exposed much more cleanly there)

> > Source/WebKit2/WebProcess/InjectedBundle/InjectedBundleNavigationAction.h:60
> > +    bool shouldOpenExternalURLs() const { return m_shouldOpenExternalURLs; }
> >  private:
> 
> Strange formatting here. There was a blank line before private before.

Doh! Thanks.

[Brady Eidson]

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