Bug 183907 – Expose SchemeRegistry::registerAsCanDisplayOnlyIfCanRequest() as WebKit SPI

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

[Daniel Bates]

Expose SchemeRegistry::registerAsCanDisplayOnlyIfCanRequest() as WebKit SPI.

[Radar WebKit Bug Importer]

<rdar://problem/38759127>

[Daniel Bates]

Created attachment 336293 [details]
Patch and unit tests

[Daniel Bates]

For Apple employees, the motivation for this patch is to fix <rdar://problem/33967486>.

[Alex Christensen]

Comment on attachment 336293 [details]
Patch and unit tests

I think we should have this per-WKWebView instead of per-WKContext.  We should also introduce a PageSchemeRegistry so we stop putting things in the wrong place.  See https://bugs.webkit.org/show_bug.cgi?id=164535
It is certainly a lot easier to just expose the scheme registry as-is, but eventually we should do the right thing.

[Alex Christensen]

Comment on attachment 336293 [details]
Patch and unit tests

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/SchemeRegistry.mm:168
> +        [WKBrowsingContextController registerSchemeForCustomProtocol:echoScheme];

Would it be possible for you to write an API test using WKURLSchemeHandler instead of WKBrowsingContextController?  We are trying to remove the latter.

[Daniel Bates]

(In reply to Alex Christensen from comment #4)
> Comment on attachment 336293 [details]
> Patch and unit tests
> 
> I think we should have this per-WKWebView instead of per-WKContext. 



Unless we implemented a concept of scheme registry inheritance then putting this functionality on WKWebView will require that a developer apply their scheme registry setting by hand each time a web page opens a new window (and a new WKWebView is created). We already have a mechanism to do this inheritance for us, WKProcessPool (WKContext in the deprecated C SPI).

> We should also introduce a PageSchemeRegistry so we stop putting things in the
> wrong place.  See https://bugs.webkit.org/show_bug.cgi?id=164535
> It is certainly a lot easier to just expose the scheme registry as-is, but
> eventually we should do the right thing.

We should look to group all the scheme registry logic together. This may turn out to look like PageSchemeRegistry. Regardless, I do not see it necessary to do such a reorganization and fix bug #164535 before fixing this bug given the time sensitiveness of <rdar://problem/33967486>.

[Daniel Bates]

(In reply to Alex Christensen from comment #5)
> Comment on attachment 336293 [details]
> Patch and unit tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336293&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/SchemeRegistry.mm:168
> > +        [WKBrowsingContextController registerSchemeForCustomProtocol:echoScheme];
> 
> Would it be possible for you to write an API test using WKURLSchemeHandler
> instead of WKBrowsingContextController?  We are trying to remove the latter.

As I wrote in the second paragraph of the change log entry in file Tools/ChangeLog of the attached patch (attachment #336293 [details]), I explicitly wrote the tests using WKBrowsingContextController so as to share code between the modern Objective-C tests and C SPI tests.

[Alex Christensen]

Comment on attachment 336293 [details]
Patch and unit tests

r=me
The transition to PageSchemeRegistry will be slow and gradual, but we should start it at some point.
Having properties per-WKProcessPool makes it impossible for us to have two WKWebViews with different settings in the same WKProcessPool.

[EWS Watchlist]

Comment on attachment 336293 [details]
Patch and unit tests

Attachment 336293 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7065844

New failing tests:
http/tests/preload/download_resources.html

[EWS Watchlist]

Created attachment 336307 [details]
Archive of layout-test-results from ews205 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews205  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Daniel Bates]

(In reply to Build Bot from comment #9)
> Comment on attachment 336293 [details]
> Patch and unit tests
> 
> Attachment 336293 [details] did not pass win-ews (win):
> Output: http://webkit-queues.webkit.org/results/7065844
> 
> New failing tests:
> http/tests/preload/download_resources.html

This is unrelated and the Win EWS is green as of the time of writing.

[Daniel Bates]

Comment on attachment 336293 [details]
Patch and unit tests

Clearing flags on attachment: 336293

Committed r229869: <https://trac.webkit.org/changeset/229869>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.