[webkit-reviews] review granted: [Bug 130061] Convert MiniBrowser to use WKWebView API : [Attachment 226537] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Mar 13 12:13:41 PDT 2014
Anders Carlsson <andersca at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 130061: Convert MiniBrowser to use WKWebView API
https://bugs.webkit.org/show_bug.cgi?id=130061
Attachment 226537: Patch
https://bugs.webkit.org/attachment.cgi?id=226537&action=review
------- Additional Comments from Anders Carlsson <andersca at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=226537&action=review
> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:-71
> - [_webView.browsingContextController removeObserver:self
forKeyPath:@"title" context:keyValueObservingContext];
> - [_webView.browsingContextController removeObserver:self
forKeyPath:@"activeURL" context:keyValueObservingContext];
You still need to remove these observers from the web view.
> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:-136
> - [_webView.browsingContextController reload];
Pretty sure we have SPI to call reload.
> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:179
> - return _webView && [_webView.browsingContextController canGoBack];
> + return _webView && [_webView canGoBack];
>
> if (action == @selector(goForward:))
> - return _webView && [_webView.browsingContextController
canGoForward];
> + return _webView && [_webView canGoForward];
No need to null check _webView here.
> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:202
> - (void)applicationTerminating
> {
> - // FIXME: Why are we bothering to close the page? This doesn't even
prevent LEAK output.
> - WKPageClose(_webView.pageRef);
> }
Just remove this.
> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:280
> + [alert beginSheetModalForWindow:self.window completionHandler:^void
(NSModalResponse response) {
No need for void here.
> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:296
> + [alert beginSheetModalForWindow:self.window completionHandler:^void
(NSModalResponse response) {
No need for void here.
> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:297
> + completionHandler(response == NSModalResponseStop);
Is stop really the right response here?
> Tools/MiniBrowser/mac/WK2BrowserWindowController.m:320
> + // FIXME: need to send OK/Cancel back.
> + completionHandler([input stringValue]);
Passing null implies cancel.
More information about the webkit-reviews
mailing list