[webkit-reviews] review granted: [Bug 128930] Add history delegate to WKWebView : [Attachment 224416] Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org <mitz at webkit.org> has granted Anders Carlsson
<andersca at apple.com>'s request for review:
Bug 128930: Add history delegate to WKWebView
https://bugs.webkit.org/show_bug.cgi?id=128930

Attachment 224416: Patch
https://bugs.webkit.org/attachment.cgi?id=224416&action=review

------- Additional Comments from mitz at webkit.org <mitz at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=224416&action=review


r=me but there is a critical typo

> Source/WebKit2/UIProcess/API/Cocoa/WKHistoryDelegatePrivate.h:40
> +- (void)webView:(WKWebView *)webView
didNavigateWithNavigationData:(WKNavigationData *)navigationData;
> +- (void)webView:(WKWebView *)webView didPerformClientRedirectFromURL:(NSURL
*)sourceURL toURL:(NSURL *)destinationURL;
> +- (void)webView:(WKWebView *)webView didPerformServerRedirectFromURL:(NSURL
*)sourceURL toURL:(NSURL *)destinationURL;
> +- (void)webView:(WKWebView *)webView didUpdateHistoryTitle:(NSString *)title
forURL:(NSURL *)URL;

Let’s prefix all methods in private delegates with _.

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:465
> +- (id<WKHistoryDelegatePrivate>)_historyDelegate

Missing space after id

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:470
> +- (void)_setHistoryDelegate:(id<WKHistoryDelegatePrivate>)historyDelegate

Ditto

> Source/WebKit2/UIProcess/Cocoa/NavigationState.mm:174
> +    [historyDelegate webView:m_webView
didPerformClientRedirectFromURL:wrapper(*API::URL::create(sourceURL))
toURL:wrapper(*API::URL::create(destinationURL))];

Let’s avoid wrappers here and use +[NSURL _web_URLWithWTFString:].

> Source/WebKit2/UIProcess/Cocoa/NavigationState.mm:186
> +    [historyDelegate webView:m_webView
didPerformClientRedirectFromURL:wrapper(*API::URL::create(sourceURL))
toURL:wrapper(*API::URL::create(destinationURL))];

Typo: s/Client/Server/!
Let’s avoid wrappers here and use +[NSURL _web_URLWithWTFString:].

> Source/WebKit2/UIProcess/Cocoa/NavigationState.mm:198
> +    [historyDelegate.get() webView:m_webView
didUpdateHistoryTitle:wrapper(*API::String::create(title))
forURL:wrapper(*API::URL::create(url))];

Let’s avoid wrappers here and use the ability of WTF::String to turn into an
NSString (and _web_URLWithWTFString:).