[webkit-reviews] review denied: [Bug 58300] [Windows, WinCairo] Support Transparent WebKit Background : [Attachment 90255] Patch (with dirty flag corrected)
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Apr 19 16:45:03 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 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!
More information about the webkit-reviews
mailing list