| Summary: | Begin making WKWebViewConfiguration a wrapper around an API::PageConfiguration | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||
| Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | andersca, cdumez, sam, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Alex Christensen
2018-07-25 16:56:11 PDT
Created attachment 345802 [details]
Patch
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. 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. 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 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. Created attachment 345886 [details]
Patch
|