[webkit-reviews] review denied: [Bug 188030] Begin making WKWebViewConfiguration a wrapper around an API::PageConfiguration : [Attachment 345802] Patch

[bugzilla-daemon at webkit.org]

Sam Weinig <sam at webkit.org> has denied Alex Christensen
<achristensen at apple.com>'s request for review:
Bug 188030: Begin making WKWebViewConfiguration a wrapper around an
API::PageConfiguration
https://bugs.webkit.org/show_bug.cgi?id=188030

Attachment 345802: Patch

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




--- Comment #2 from Sam Weinig <sam at webkit.org> ---
Comment on attachment 345802
  --> https://bugs.webkit.org/attachment.cgi?id=345802
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::WebURLSchemeHandler
Cocoa::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.