Bug 130061 - Convert MiniBrowser to use WKWebView API
Summary: Convert MiniBrowser to use WKWebView API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-10 18:59 PDT by Simon Fraser (smfr)
Modified: 2014-03-17 13:10 PDT (History)
3 users (show)

See Also:


Attachments
Patch (37.00 KB, patch)
2014-03-10 19:01 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (35.24 KB, patch)
2014-03-12 11:18 PDT, Simon Fraser (smfr)
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2014-03-10 18:59:05 PDT
Convert MiniBrowser to use WKWebView API
Comment 1 Simon Fraser (smfr) 2014-03-10 19:01:42 PDT
Created attachment 226365 [details]
Patch
Comment 2 Simon Fraser (smfr) 2014-03-10 19:02:12 PDT
I know there is some commented-out code in the patch.
Comment 3 Anders Carlsson 2014-03-10 20:44:41 PDT
Comment on attachment 226365 [details]
Patch

You still need to use the WK_API_ENABLED guards otherwise you'll break the 32-bit MiniBrowser build.
Comment 4 Simon Fraser (smfr) 2014-03-12 11:18:45 PDT
Created attachment 226537 [details]
Patch
Comment 5 Anders Carlsson 2014-03-13 12:14:05 PDT
Comment on attachment 226537 [details]
Patch

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.
Comment 6 Simon Fraser (smfr) 2014-03-17 13:10:53 PDT
https://trac.webkit.org/r165749