[webkit-reviews] review denied: [Bug 58300] [Windows, WinCairo] Support Transparent WebKit Background : [Attachment 90429] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Apr 22 08:37:11 PDT 2011
Adam Roben (:aroben) <aroben at apple.com> has denied Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 58300: [Windows, WinCairo] Support Transparent WebKit Background
https://bugs.webkit.org/show_bug.cgi?id=58300
Attachment 90429: Patch
https://bugs.webkit.org/attachment.cgi?id=90429&action=review
------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=90429&action=review
Should IWebView::setHostWindow return an error if a layered WebView is given a
non-0 host window?
> Source/WebKit/win/WebView.cpp:1013
> + RECT childWindowRect;
> + ::GetWindowRect(hWnd, &childWindowRect);
> + RECT windowRect;
> + ::GetWindowRect(data->parentWindow, &windowRect);
> + int xOffset = childWindowRect.left - windowRect.left;
> + int yOffset = childWindowRect.top - windowRect.top;
I think you could also use ::MapWindowPoints for this.
> Source/WebKit/win/WebView.cpp:1032
> + ::SendMessage(hWnd, WM_PRINTCLIENT, reinterpret_cast<WPARAM>(hdc),
> + PRF_CLIENT | PRF_CHILDREN | PRF_OWNED);
I'd just put this all on one line.
> Source/WebKit/win/WebView.cpp:1038
> +void WebView::performLayeredWindowUpdate(HWND hWnd)
I don't think the HWND argument is needed. It will always be m_viewWindow.
> Source/WebKit/win/WebView.cpp:1041
> + OwnPtr<HDC> hdcMem(::CreateCompatibleDC(hdcScreen));
The more modern way to do this is to use assignment and adoptPtr.
> Source/WebKit/win/WebView.cpp:1053
> + // Force child windows (plugins) to draw onto our backing store.
> + // The graphics mode of the HDC must be GM_ADVANCED so it will be
capable
> + // of selecting world transforms (for positioning plugin draws).
> + ::SetGraphicsMode(hdcMem.get(), GM_ADVANCED);
> + DrawChildWindowData data = { hWnd, hdcMem.get() };
> + ::EnumChildWindows(hWnd, drawChildWindow,
reinterpret_cast<LPARAM>(&data));
The PaintWebViewAndChildren logic in WebView::paint is supposed to do this for
you when we get a WM_PRINTCLIENT message. If that isn't working, we should fix
it, rather than reimplementing that logic here in a different way.
> Source/WebKit/win/WebView.cpp:2181
> case WM_PAINT: {
> webView->paint(0, 0);
> + if (webView->usesLayeredWindow()) {
> + webView->performLayeredWindowUpdate(hWnd);
> + // Let default processing continue. If we don't, the window
will not
> + // be marked as processed and we will burn 100% of the CPU.
> + handled = false;
> + }
I'm surprised you need to call up to ::DefWndProcW in this case. WebView::paint
should have already called ::BeginPaint/::EndPaint for you, which should clear
the window's invalid region.
Why do we first need to call WebView::paint? performLayeredWindowUpdate sends a
WM_PRINTCLIENT message, which will force a paint, so it seems like we're
painting twice.
> Source/WebKit/win/WebView.cpp:2190
> + case WM_ERASEBKGND:
> + if (webView->usesLayeredWindow()) {
> + // Don't perform a background erase for transparent views.
> + handled = true;
> + lResult = 1;
> + }
> + break;
We could probably do this for all views (I'm a little surprised we weren't
already doing this), but limiting it for now is fine.
> Source/WebKit/win/WebView.cpp:2658
> + registerWebViewWindowClass();
> +
> + setUsesLayeredWindow(TRUE);
> +
> + m_viewWindow = CreateWindowEx(WS_EX_LAYERED, kWebViewWindowClassName, 0,
WS_POPUP | WS_VISIBLE,
> + frame.left, frame.top, frame.right - frame.left, frame.bottom -
frame.top, 0, 0, gInstance, 0);
> + ASSERT(::IsWindow(m_viewWindow));
Can we move registerWebViewWindowClass() and CreateWindowEx into
initWithFrameImpl? We can use the m_usesLayeredWindow flag to decide what
parameters to pass to CreateWindowEx.
> Source/WebKit/win/WebView.cpp:2663
> +HRESULT WebView::initWithFrameImpl(BSTR frameName, BSTR groupName)
Maybe initWithFrameCommon would be a little clearer.
> Source/WebKit/win/WebView.cpp:5908
> +HRESULT STDMETHODCALLTYPE WebView::setUsesLayeredWindow(BOOL
usesLayeredWindow)
No need for STDMETHODCALLTYPE, even though other functions in this file have
it.
> Source/WebKit/win/WebView.cpp:5910
> + if (m_usesLayeredWindow == !!usesLayeredWindow)
No need for the !!.
> Source/WebKit/win/WebView.cpp:5916
> + if (m_viewWindow) {
> + if (!::SetWindowLongPtr(m_viewWindow, GWL_EXSTYLE, WS_EX_LAYERED))
> + return E_FAIL;
> + }
This can be a single if that uses &&.
This will clear out any other extended styles the window has. It also only sets
WS_EX_LAYERED, and never unsets it even if usesLayeredWindow is false.
> Source/WebKit/win/WebView.cpp:5922
> +HRESULT STDMETHODCALLTYPE WebView::usesLayeredWindow(BOOL*
usesLayeredWindow)
No need for STDMETHODCALLTYPE, even though other functions in this file have
it.
> Source/WebKit/win/WebView.h:834
> + virtual HRESULT STDMETHODCALLTYPE setUsesLayeredWindow(BOOL
transparent);
> + virtual HRESULT STDMETHODCALLTYPE usesLayeredWindow(BOOL* transparent);
"transparent" doesn't seem right anymore.
> Source/WebKit/win/Interfaces/IWebView.idl:242
> /*!
> + @method initLayeredWindowWithFrame:frameName:groupName:
> + @abstract The designated initializer for WebView.
> + @discussion Initialize a Transparent WebView with the supplied
parameters. This method will
> + create a main WebFrame with the view. Passing a top level frame name
is useful if you
> + handle a targetted frame navigation that would normally open a
window in some other
> + way that still ends up creating a new WebView.
> + @param frame The frame used to create the view.
> + @param frameName The name to use for the top level frame. May be
nil.
> + @param groupName The name of the webView set to which this webView
will be added. May be nil.
> + @result Returns an initialized WebView.
> + - (id)initLayeredWindowWithFrame:(NSRect)frame frameName:(NSString
*)frameName groupName:(NSString *)groupName;
> + */
> + HRESULT initLayeredWindowWithFrame([in] RECT frame, [in] BSTR frameName,
[in] BSTR groupName);
Let's put this on IWebViewPrivate. Probably no need for the large headerdoc
comment, either.
> Tools/WinLauncher/WinLauncher.cpp:51
> +LONG_PTR DefEditProc = 0;
> +LONG_PTR DefWebKitProc = 0;
Maybe WNDPROC would be an even better type?
> Tools/WinLauncher/WinLauncher.cpp:78
> +static bool hasTransparentBackground()
> +{
> + return s_transparent;
> +}
Maybe usesLayeredWebView() would be more accurate?
> Tools/WinLauncher/WinLauncher.cpp:174
> + if (!::SystemParametersInfo(SPI_GETWORKAREA, 0,
reinterpret_cast<void*>(&desktop), 0))
I think static_cast would work here.
> Tools/WinLauncher/WinLauncher.cpp:254
> + IWebPreferences* tmpPreferences;
> + if (FAILED(WebKitCreateInstance(CLSID_WebPreferences, 0,
IID_IWebPreferences, reinterpret_cast<void**>(&tmpPreferences))))
> + goto exit;
Looks like this is leaked.
> Tools/WinLauncher/WinLauncher.cpp:258
> + IWebPreferences* standardPreferences;
> + if (FAILED(tmpPreferences->standardPreferences(&standardPreferences)))
> + goto exit;
And this.
> Tools/WinLauncher/WinLauncher.cpp:260
> + standardPreferences->setAcceleratedCompositingEnabled(true);
I'm surprised this doesn't default to true. (You should really be using TRUE
here.)
> Tools/WinLauncher/WinLauncher.cpp:269
> + IWebViewPrivate* webViewPrivate;
> + hr = gWebView->QueryInterface(IID_IWebViewPrivate,
reinterpret_cast<void**>(&webViewPrivate));
> + if (FAILED(hr))
> + goto exit;
I think you're leaking this.
> Tools/WinLauncher/WinLauncher.cpp:523
> + int y = (int)(short)HIWORD(lParam);
Are all the casts really needed?
> Tools/WinLauncher/WinLauncher.cpp:525
> + if ((y > window.top) && (y < window.top + 30))
Using a named constant for "30" would be clearer.
> Tools/WinLauncher/WinLauncher.cpp:550
> + return (LRESULT)CallWindowProc((WNDPROC)DefWebKitProc, hWnd,
message, wParam, lParam);
CallWindowProc already returns an LRESULT, so that cast is unneeded. And if you
change DefWebKitProc to be a WNDPROC, you won't need that cast, either.
More information about the webkit-reviews
mailing list