[webkit-reviews] review granted: [Bug 58300] [Windows, WinCairo] Support Transparent WebKit Background : [Attachment 91149] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has granted 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 91149: Patch
https://bugs.webkit.org/attachment.cgi?id=91149&action=review

------- Additional Comments from Adam Roben (:aroben) <aroben at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=91149&action=review

> Source/WebKit/win/WebView.cpp:5842
> +    if (::SetWindowLongPtr(window, index, newValue))
> +	   return true;
> +
> +    DWORD error = ::GetLastError();
> +    return (error) ? false : true;
> +}

I'd simplify this to: return ::SetWindowLongPtr(...) || !::GetLastError();

> Source/WebKit/win/WebView.cpp:5866
> +	   newStyle = WS_POPUP;

Should be |=

> Source/WebKit/win/WebView.cpp:5874
> +	   if (!setWindowStyle(m_viewWindow, GWL_STYLE, newStyle))
> +	       return E_FAIL;
> +
> +	   if (!setWindowStyle(m_viewWindow, GWL_EXSTYLE, newExStyle))
> +	       return E_FAIL;

It would be surprising for only the second call to fail, but in that unlikely
case should we try to undo the effect of the first call?

> Source/WebKit/win/WebView.cpp:5890
> +    // MSDN claims that SetWindowLongPtr doesn't take effect for some data
types until a
> +    // SetWindowPos is called

Missing a period at the end of the comment.

Is this actually a problem for us in practice?

> Source/WebKit/win/WebView.h:919
> +    HRESULT initWithFrameCommon(DWORD style, DWORD exStyle, RECT frame, HWND
parent, BSTR frameName, BSTR groupName);

I think this can be removed.

> Source/WebKit/win/Interfaces/IWebViewPrivate.idl:157
> +    HRESULT setUsesLayeredWindow([in] BOOL transparent);
> +    HRESULT usesLayeredWindow([out, retval] BOOL* transparent);

You should rename "transparent".

> Source/WebKit/win/Interfaces/IWebViewPrivate.idl:263
> +

Might as well remove this.

> Tools/WinLauncher/WinLauncher.cpp:519
> +	       return parentProc(hWnd, message, wParam, lParam);

We should still be using CallWindowProc.

> Tools/WinLauncher/WinLauncher.cpp:524
> +	   return parentProc(hWnd, message, wParam, lParam);

Ditto.