[webkit-reviews] review denied: [Bug 58300] [Windows, WinCairo] Support Transparent WebKit Background : [Attachment 90255] Patch (with dirty flag corrected)

[bugzilla-daemon at webkit.org]

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 90255: Patch (with dirty flag corrected)
https://bugs.webkit.org/attachment.cgi?id=90255&action=review

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

How does this new mode interact with the existing IWebView::[set]Transparent
APIs? We don't want to break users of those APIs.

Most of my comments are related to style issues. You're dealing with a bunch of
crufty code. Boy would it be nice to clean that code up first so your code
could be nice and clean.

> Source/WebKit/win/ChangeLog:27
> +	   * Interfaces/IWebView.idl:
> +	   * Interfaces/WebKit.idl: touch to force clean build
> +	   * WebCoreSupport/WebInspectorDelegate.h:
> +	   * WebView.cpp:
> +	   (WebView::updateBackingStore):
> +	   (drawChildWindow):
> +	   (WebView::performLayeredWindowUpdate):
> +	   (WebView::paint):
> +	   (WebView::WebViewWndProc):
> +	   (WebView::initWithFrame):
> +	   (WebView::initTransparentViewWithFrame):
> +	   (WebView::initWithFrameImpl):
> +	   (WebView::initializeToolTipWindow):
> +	   (WebView::active):
> +	   (WebView::setGlobalHistoryItem):
> +	   (WebView::paintTransparentView):
> +	   (WebView::forwardingWindowProc):
> +	   * WebView.h:

It would be great to fill in this boilerplate with explanations of the changes
you made.

> Source/WebKit/win/WebView.cpp:958
> +    HGDIOBJ oldBitmap = 0;
>      if (!dc) {
>	   windowDC = ::GetDC(m_viewWindow);
>	   bitmapDC = ::CreateCompatibleDC(windowDC);
> -	   ::SelectObject(bitmapDC, m_backingStoreBitmap->handle());
> +	   oldBitmap = ::SelectObject(bitmapDC,
m_backingStoreBitmap->handle());

This cleanup should probably be done separately.

> Source/WebKit/win/WebView.cpp:1008
> +    GetWindowRect(hWnd, &childWindowRect);

We've been moving toward using the :: prefix on Win32 calls, and you're already
doing that in other places in this patch, so I think you should do it in this
function, too.

> Source/WebKit/win/WebView.cpp:1014
> +    // Modify the world transform so that the plugin is positioned properly.


Changing "plugin" to "child window" would be more consistent with the rest of
this function.

> Source/WebKit/win/WebView.cpp:1044
> +    HDC hdcMem = ::CreateCompatibleDC(hdcScreen);

You should use OwnPtr to hold hdcMem.

> Source/WebKit/win/WebView.cpp:1045
> +    HBITMAP hbmOld = (HBITMAP)::SelectObject(hdcMem,
m_backingStoreBitmap->handle());

Please use static_cast.

> Source/WebKit/win/WebView.cpp:1055
> +    DrawChildWindowData data = {hWnd, hdcMem};

Missing spaces around the braces here.

> Source/WebKit/win/WebView.cpp:1065
> +    ::UpdateLayeredWindow(hWnd, hdcScreen, &windowPos, &windowSize, hdcMem,
&layerPos, 0, &blendFunction, ULW_ALPHA);

MSDN says you can pass 0 instead of &windowPos since you aren't changing the
window's position.

> Source/WebKit/win/WebView.cpp:1077
>  void WebView::paint(HDC dc, LPARAM options)
>  {
> +    if (transparent()) {
> +	   paintTransparentView(dc);
> +	   return;
> +    }

Why do we need a different painting path for transparent WebViews? (This is
something your ChangeLog could explain.) Will this break WebKit clients that
use the current implementation of transparent WebViews?

> Source/WebKit/win/WebView.cpp:2195
> +	   case WM_ERASEBKGND:
> +	       handled = webView->transparent(); // Don't perform a background
erase for transparent views.
> +	       break;

I believe you also want to set lResult to 1.

> Source/WebKit/win/WebView.cpp:2783
> +    HWND parentWindow = 0;
> +    if (m_hostWindow)
> +	   parentWindow = m_viewWindow;
> +
>      m_toolTipHwnd = CreateWindowEx(WS_EX_TRANSPARENT, TOOLTIPS_CLASS, 0,
WS_POPUP | TTS_NOPREFIX | TTS_ALWAYSTIP,
>				      CW_USEDEFAULT, CW_USEDEFAULT,
CW_USEDEFAULT, CW_USEDEFAULT,
> -				      m_viewWindow, 0, 0, 0);
> +				      parentWindow, 0, 0, 0);

Why is this change needed? (Again, a good thing to include in your ChangeLog.)

> Source/WebKit/win/WebView.cpp:6743
> +    HDC bitmapDC = ::CreateCompatibleDC(hdc);

OwnPtr would be better here.

> Tools/WinLauncher/WinLauncher.cpp:155
> +    if (hasTransparentBackground () || !gViewWindow)

Extra space before ().

> Tools/WinLauncher/WinLauncher.cpp:169
> +static void useTransparentWinProc()
> +{
> +    hMainWnd = gViewWindow;
> +    DefWebKitProc = GetWindowLong(hMainWnd, GWL_WNDPROC);
> +    SetWindowLong(hMainWnd, GWL_WNDPROC, (long)TransparentWndProc);
> +}

This function could use a clearer name. Maybe something containing the word
"subclass", since that's what you're doing.

You should be using Get/SetWindowLongPtr, and C++-style casts.

> Tools/WinLauncher/WinLauncher.cpp:171
> +static void setFrameToFullDesktop()

This doesn't set the frame of anything. It just updates the s_windowPosition
and s_windowSize variables.

> Tools/WinLauncher/WinLauncher.cpp:174
> +    if (::SystemParametersInfo(SPI_GETWORKAREA, 0, (void*)&desktop, 0)) {

This can be turned into an early return. You should use a C++ cast.

> Tools/WinLauncher/WinLauncher.cpp:211
> +    TCHAR** argv = CommandLineToArgvW(GetCommandLineW(), &argc);
> +    for (int i = 1; i < argc; ++i) {
> +	   if (!_tcsicmp(argv[i], TEXT("--transparent")))
> +	       s_transparent = true;
> +	   else if (!_tcsicmp(argv[i], TEXT("--desktop")))
> +	       s_fullDesktop = true;

WCHAR and wcsicmp and L would be better than TCHAR and _tcsicmp and TEXT().

> Tools/WinLauncher/WinLauncher.cpp:238
> +	   hURLBarWnd = CreateWindow(L"EDIT", L"Type URL Here",
> +		       WS_OVERLAPPEDWINDOW | WS_VISIBLE | WS_BORDER | ES_LEFT |
ES_AUTOVSCROLL, 
> +		       s_windowPosition.x, s_windowPosition.y +
s_windowSize.cy, s_windowSize.cx, URLBAR_HEIGHT,
> +		       0,
> +		       0,
> +		       hInstance, 0);
> +
> +	   ShowWindow(hURLBarWnd, nCmdShow);
> +	   UpdateWindow(hURLBarWnd);

You should probably not pass WS_VISIBLE to CreateWindow, since you then call
ShowWindow.

> Tools/WinLauncher/WinLauncher.cpp:507
> +	   // For our transparent window, we want all active regions of the
screen (i.e.,
> +	   // anything non-transparent) to allow movement. If we got to this
handler,
> +	   // we clicked on something non-transparent. So we return HT_CAPTION,
which 
> +	   // makes everything a draggable handle.
> +	   int y = (int)(short)HIWORD(lParam);
> +
> +	   if ((y > window.top) && (y < window.top + 30))
> +	       return HTCAPTION;

What's up with this 30-pixel bar? The comment above seems to say you want
everywhere to be draggable.

C++ casts!

> Tools/WinLauncher/WinLauncher.cpp:509
> +	   // Otherwise, just let the standard DefProc handle it.

I think you meant WndProc.

> Tools/WinLauncher/WinLauncher.cpp:513
> +    if (handled)
> +	   return handled;

I think this is dead code.

> Tools/WinLauncher/WinLauncher.cpp:515
> +    int wmId, wmEvent;

It would be better to declare these inside the WM_COMMAND case. Just surround
the case body in braces.

> Tools/WinLauncher/WinLauncher.cpp:520
> +	   wmId    = LOWORD(wParam);
> +	   wmEvent = HIWORD(wParam);

We don't normally line up equals signs like this. I guess you copied this from
WndProc. It would be much nicer if we could share the code!