Bug 195740 - [Win][MinBrowser][WK2] Implement createNewPage of WKPageUIClient to open a new window
Summary: [Win][MinBrowser][WK2] Implement createNewPage of WKPageUIClient to open a ne...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-14 03:17 PDT by Fujii Hironori
Modified: 2019-03-14 19:17 PDT (History)
4 users (show)

See Also:

Attachments
Patch (14.90 KB, patch)
2019-03-14 03:31 PDT, Fujii Hironori 		no flags Details | Formatted Diff | Diff
Patch for landing (14.87 KB, patch)
2019-03-14 18:53 PDT, Fujii Hironori 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2019-03-14 03:17:25 PDT 
[Win][MinBrowser][WK2] Implement createNewPage of WKPageUIClient to open a new window
Comment 1 Fujii Hironori 2019-03-14 03:31:48 PDT 
Created attachment 364652 [details]
Patch
Comment 2 Ross Kirsling 2019-03-14 11:27:39 PDT 
Comment on attachment 364652 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364652&action=review

Seems fine overall from my perspective.

> Tools/MiniBrowser/win/MainWindow.h:36
> +    using BrowserWindowFactory = std::function<Ref<BrowserWindow>(HWND mainWnd, HWND urlBarWnd, bool usesLayeredWebView, bool pageLoadTesting)>;

Shouldn't this be WTF::Function?

> Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:304
> +    auto& thisWindow = toWebKitBrowserWindow(clientInfo);

Doesn't look like you're using this one.
Comment 3 Fujii Hironori 2019-03-14 18:39:37 PDT 
Comment on attachment 364652 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364652&action=review

>> Tools/MiniBrowser/win/MainWindow.h:36
>> +    using BrowserWindowFactory = std::function<Ref<BrowserWindow>(HWND mainWnd, HWND urlBarWnd, bool usesLayeredWebView, bool pageLoadTesting)>;
> 
> Shouldn't this be WTF::Function?

I prefer std::function to WTF::Function in MiniBrowser for the same reason I prefer std::vector to WTF::Vector.
Bug 189846 Comment 12
Bug 191101 Comment 3

>> Tools/MiniBrowser/win/WebKitBrowserWindow.cpp:304
>> +    auto& thisWindow = toWebKitBrowserWindow(clientInfo);
> 
> Doesn't look like you're using this one.

Will fix.
Comment 4 Fujii Hironori 2019-03-14 18:53:17 PDT 
Created attachment 364750 [details]
Patch for landing
Comment 5 Fujii Hironori 2019-03-14 19:16:13 PDT 
Committed r242984: <https://trac.webkit.org/changeset/242984>
Comment 6 Radar WebKit Bug Importer 2019-03-14 19:17:22 PDT 
<rdar://problem/48912595>