Bug 188030 – Begin making WKWebViewConfiguration a wrapper around an API::PageConfiguration

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Begin making WKWebViewConfiguration a wrapper around an API::PageConfiguration

[Alex Christensen]

Created attachment 345802 [details]
Patch

[Sam Weinig]

Comment on attachment 345802 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        I've arbitrarily chosen _treatsSHA1SignedCertificatesAsInsecure and _urlSchemeHandlers
> +        as the first two properties to transition, and I've made a way to incrementally transition.

Because, what is happening here is a bit tricky, this would be much nicer if the ChangeLog included an explanation of the changes being made.

> Source/WebKit/UIProcess/API/APIPageConfiguration.h:153
> +    HashMap<WTF::String, RefPtr<WebKit::WebURLSchemeHandler>> m_urlSchemeHandlers;

Can you add a null WebURLSchemeHandler into this map? If not, can this be a HashMap<WTF::String, Ref<WebKit::WebURLSchemeHandler>>?

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:509
> +    auto pageConfiguration = [configuration copyPageConfiguration];

Why is copying the configuration necessary here? What precludes us from simply referencing the underlying configuration? Is this a short term necessity while conversion is happening?

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:119
>      BOOL _treatsSHA1SignedCertificatesAsInsecure;

Why is this not being removed?

> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:507
>      if (!canonicalScheme)
>          [NSException raise:NSInvalidArgumentException format:@"'%@' is not a valid URL scheme", urlScheme];
>  
> -    if ([urlSchemeHandlers objectForKey:(NSString *)canonicalScheme.value()])
> +    if (_pageConfiguration->urlSchemeHandlerForURLScheme(*canonicalScheme))
>          [NSException raise:NSInvalidArgumentException format:@"URL scheme '%@' already has a registered URL scheme handler", urlScheme];
>  
> -    [urlSchemeHandlers setObject:urlSchemeHandler forKey:(NSString *)canonicalScheme.value()];
> +    _pageConfiguration->setURLSchemeHandlerForURLScheme(WebKit::WebURLSchemeHandlerCocoa::create(urlSchemeHandler), *canonicalScheme);

Seems odd to have this much logic in the wrapper. Would be nice if it could move down to the underlying PageConfiguration. If it can't, a comment indicating why would be good.

[Chris Dumez]

Comment on attachment 345802 [details]
Patch

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

>> Source/WebKit/UIProcess/API/APIPageConfiguration.h:153
>> +    HashMap<WTF::String, RefPtr<WebKit::WebURLSchemeHandler>> m_urlSchemeHandlers;
> 
> Can you add a null WebURLSchemeHandler into this map? If not, can this be a HashMap<WTF::String, Ref<WebKit::WebURLSchemeHandler>>?

It will probably work with a String as key but be careful, our support for Ref<> as value in HashMap is buggy depending on the key traits :(
See https://bugs.webkit.org/show_bug.cgi?id=184012 for e.g.

[Chris Dumez]

Comment on attachment 345802 [details]
Patch

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

>>> Source/WebKit/UIProcess/API/APIPageConfiguration.h:153
>>> +    HashMap<WTF::String, RefPtr<WebKit::WebURLSchemeHandler>> m_urlSchemeHandlers;
>> 
>> Can you add a null WebURLSchemeHandler into this map? If not, can this be a HashMap<WTF::String, Ref<WebKit::WebURLSchemeHandler>>?
> 
> It will probably work with a String as key but be careful, our support for Ref<> as value in HashMap is buggy depending on the key traits :(
> See https://bugs.webkit.org/show_bug.cgi?id=184012 for e.g.

And by buggy, I mean that it builds but crashes at runtime :S

[Alex Christensen]

Comment on attachment 345802 [details]
Patch

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

>> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:509
>> +    auto pageConfiguration = [configuration copyPageConfiguration];
> 
> Why is copying the configuration necessary here? What precludes us from simply referencing the underlying configuration? Is this a short term necessity while conversion is happening?

It's at least necessary during conversion because this function mutates it, and it is the base of a WKWebViewConfiguration which can be used for other things later in the program and should not be mutated.  I think we'll have to copy it once we're done, too, because the WebPageProxy hangs on to it.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:119
>>      BOOL _treatsSHA1SignedCertificatesAsInsecure;
> 
> Why is this not being removed?

It should be.

>> Source/WebKit/UIProcess/API/Cocoa/WKWebViewConfiguration.mm:507
>> +    _pageConfiguration->setURLSchemeHandlerForURLScheme(WebKit::WebURLSchemeHandlerCocoa::create(urlSchemeHandler), *canonicalScheme);
> 
> Seems odd to have this much logic in the wrapper. Would be nice if it could move down to the underlying PageConfiguration. If it can't, a comment indicating why would be good.

Raising NSExceptions in wrapped C++ types is tricky.  I'd prefer not to change that behavior in this patch.

[Alex Christensen]

Created attachment 345886 [details]
Patch

[Alex Christensen]

https://trac.webkit.org/changeset/234313/webkit

[Radar WebKit Bug Importer]

<rdar://problem/42664760>