Bug 183907

Summary: Expose SchemeRegistry::registerAsCanDisplayOnlyIfCanRequest() as WebKit SPI
Product: WebKit Reporter: Daniel Bates <dbates>
Component: WebKit2Assignee: Daniel Bates <dbates>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, aestes, beidson, ews-watchlist, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch and unit tests
none
Archive of layout-test-results from ews205 for win-future none

Description Daniel Bates 2018-03-22 11:21:08 PDT
Expose SchemeRegistry::registerAsCanDisplayOnlyIfCanRequest() as WebKit SPI.
Comment 1 Radar WebKit Bug Importer 2018-03-22 11:21:54 PDT
<rdar://problem/38759127>
Comment 2 Daniel Bates 2018-03-22 11:40:01 PDT
Created attachment 336293 [details]
Patch and unit tests
Comment 3 Daniel Bates 2018-03-22 11:55:35 PDT
For Apple employees, the motivation for this patch is to fix <rdar://problem/33967486>.
Comment 4 Alex Christensen 2018-03-22 12:02:30 PDT
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.
Comment 5 Alex Christensen 2018-03-22 12:07:39 PDT
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.
Comment 6 Daniel Bates 2018-03-22 12:24:51 PDT
(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>.
Comment 7 Daniel Bates 2018-03-22 12:32:52 PDT
(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.
Comment 8 Alex Christensen 2018-03-22 12:53:44 PDT
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.
Comment 9 EWS Watchlist 2018-03-22 13:42:15 PDT Comment hidden (obsolete)
Comment 10 EWS Watchlist 2018-03-22 13:42:26 PDT Comment hidden (obsolete)
Comment 11 Daniel Bates 2018-03-22 15:10:18 PDT
(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.
Comment 12 Daniel Bates 2018-03-22 15:10:44 PDT
Comment on attachment 336293 [details]
Patch and unit tests

Clearing flags on attachment: 336293

Committed r229869: <https://trac.webkit.org/changeset/229869>
Comment 13 Daniel Bates 2018-03-22 15:10:45 PDT
All reviewed patches have been landed.  Closing bug.