Bug 145505 - Add WKTR support for "should open external URLs"
Summary: Add WKTR support for "should open external URLs"
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-31 15:45 PDT by Brady Eidson
Modified: 2022-03-01 02:47 PST (History)
3 users (show)

See Also:


Attachments
Patch v1 (30.63 KB, patch)
2015-05-31 21:32 PDT, Brady Eidson
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2015-05-31 15:45:36 PDT
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
Comment 1 Brady Eidson 2015-05-31 21:32:38 PDT
Created attachment 253988 [details]
Patch v1
Comment 2 Darin Adler 2015-06-01 12:06:16 PDT
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.
Comment 3 Brady Eidson 2015-06-01 12:39:17 PDT
(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.
Comment 4 Brady Eidson 2015-06-01 12:57:56 PDT
http://trac.webkit.org/changeset/185072