[webkit-reviews] review granted: [Bug 186263] [Win][MiniBrowser] Support multiple windows properly : [Attachment 342034] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jun 6 22:53:45 PDT 2018


Ryosuke Niwa <rniwa at webkit.org> has granted Fujii Hironori
<Hironori.Fujii at sony.com>'s request for review:
Bug 186263: [Win][MiniBrowser] Support multiple windows properly
https://bugs.webkit.org/show_bug.cgi?id=186263

Attachment 342034: Patch

https://bugs.webkit.org/attachment.cgi?id=342034&action=review




--- Comment #15 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 342034
  --> https://bugs.webkit.org/attachment.cgi?id=342034
Patch

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

> Tools/MiniBrowser/win/MainWindow.cpp:110
> -    m_browserWindow = std::make_unique<MiniBrowser>(m_hMainWnd,
m_hURLBarWnd, usesLayeredWebView, pageLoadTesting);
> +    m_browserWindow = adoptRef(new MiniBrowser(m_hMainWnd, m_hURLBarWnd,
usesLayeredWebView, pageLoadTesting));

Please add MiniBrowser::create as we do elsewhere in WebKit instead.

> Tools/MiniBrowser/win/MainWindow.cpp:146
> +    RefPtr<MainWindow> thiz =
reinterpret_cast<MainWindow*>(GetWindowLongPtr(hWnd, GWLP_USERDATA));

Can we just call this thisWindow instead of thiz?

> Tools/MiniBrowser/win/MiniBrowser.cpp:79
> +    auto c = refCount();

Please don't use a single letter variable like c. Call it count instead.


More information about the webkit-reviews mailing list