Bug 58300 - [Windows, WinCairo] Support Transparent WebKit Background
Summary: [Windows, WinCairo] Support Transparent WebKit Background
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Windows 7
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-11 23:15 PDT by Brent Fulgham
Modified: 2018-05-13 20:13 PDT (History)
8 users (show)

See Also:


Attachments
Initial hack for broad discussion. (52.65 KB, patch)
2011-04-11 23:19 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Initial patch without line ending changes. (28.69 KB, patch)
2011-04-11 23:26 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Some additional debugging (33.09 KB, patch)
2011-04-13 12:25 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Generally working! (34.68 KB, patch)
2011-04-16 01:35 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Fully Working (Plugins Commented Out) (35.85 KB, patch)
2011-04-16 23:47 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Example of transparent background display (263.63 KB, image/png)
2011-04-16 23:58 PDT, Brent Fulgham
no flags Details
Fully functional implementation (24.17 KB, patch)
2011-04-17 20:40 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Revised to correct style warnings. (23.82 KB, patch)
2011-04-17 21:01 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Updated with property change to force clean Windows rebuild. (24.66 KB, patch)
2011-04-17 21:45 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Try touching a WebKit/win vsprop to force rebuild (25.04 KB, patch)
2011-04-17 23:07 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (25.03 KB, patch)
2011-04-18 10:33 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (24.89 KB, patch)
2011-04-18 13:16 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (25.73 KB, patch)
2011-04-19 11:33 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (25.41 KB, patch)
2011-04-19 11:43 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (with dirty flag corrected) (25.43 KB, patch)
2011-04-19 14:29 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (13.16 KB, patch)
2011-04-20 12:57 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (29.18 KB, patch)
2011-04-20 13:09 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (28.89 KB, patch)
2011-04-20 15:50 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (26.21 KB, patch)
2011-04-22 15:48 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (26.18 KB, patch)
2011-04-22 17:10 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (26.22 KB, patch)
2011-04-26 13:33 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (26.24 KB, patch)
2011-04-26 16:59 PDT, Brent Fulgham
aroben: review+
Details | Formatted Diff | Diff
scrollbar is transparent (497.36 KB, image/png)
2011-05-18 20:34 PDT, xufan
no flags Details
patch for transparent scrollbar (3.36 KB, patch)
2011-05-18 21:31 PDT, xufan
no flags Details | Formatted Diff | Diff
patch for transparent scrollbar,Fix the problem when the page has opaque flash plugin in a frame (4.36 KB, patch)
2011-05-19 00:37 PDT, xufan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2011-04-11 23:15:27 PDT
A year or two ago, Appcelerator (http://www.appcelerator.com) implemented some changes in WebKit's WinCairo port to support a transparent background.  They were never submitted for upstream use, but I would like to propose making these additions to support more Mac (e.g., Dashboard) style uses on Windows.

This first patch is not meant for consideration for adding to WebKit.  However, I wanted to land these changes here for discussion.
Comment 1 Brent Fulgham 2011-04-11 23:19:32 PDT
Created attachment 89161 [details]
Initial hack for broad discussion.

This initial patch is just for discussion.  Lots of code needs to be cleaned up.
Comment 2 Brent Fulgham 2011-04-11 23:26:43 PDT
Created attachment 89163 [details]
Initial patch without line ending changes.
Comment 3 Brent Fulgham 2011-04-12 00:05:18 PDT
Comment on attachment 89163 [details]
Initial patch without line ending changes.

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

> Source/WebCore/platform/graphics/GraphicsContext.h:285
> +        void dumpToFile(const char*);

This change is going into its own bug.

> Source/WebKit/win/WebView.cpp:1004
> +        transparentPaint(dc);

I'm wondering if this could somehow be handled in the IWebViewPrivateDelegate's "drawBackground" method.

> Source/WebKit/win/WebView.cpp:1952
> +    if (virtualKeyCode == VK_CAPITAL)

This wasn't meant to be included.  I think this is cruft from the original Appcelerator changes.

> Tools/WinLauncher/WinLauncher.cpp:1
>  /*

Note: The entirety of WinLauncher is a mess.  This is a hacked-up set of changes that get a transparent-background test pattern running.  The whole thing should be wrapped up as a separate example with a useful display widget in a later patch.

> Tools/WinLauncher/WinLauncher.cpp:144
> +    if (HasTransparentBackground() || !gViewWindow)

I'm not sure why this was needed (it's from the Appcelerator diff).  If I take it out, the transparent view magically collapses on itself!

> Tools/WinLauncher/WinLauncher.cpp:296
> +    wcex.cbWndExtra     = 4;

Note: This is needed so we can carry our WebView context around with the window handle.
Comment 4 Brent Fulgham 2011-04-12 00:06:53 PDT
Note: In this initial patch, the backing store never seems to get updated with WebKit-generated changes.  For example, if I redirect the WebView to a new page, it still displays the old test pattern.

Also, even though the page is redrawn a number of times as the WebKit logo PNG is downloaded, the HBITMAP being composited to the screen is unchanged.
Comment 5 Brent Fulgham 2011-04-13 12:25:13 PDT
Created attachment 89434 [details]
Some additional debugging
Comment 6 Brent Fulgham 2011-04-16 01:35:52 PDT
Created attachment 89919 [details]
Generally working!
Comment 7 Brent Fulgham 2011-04-16 23:47:27 PDT
Created attachment 89941 [details]
Fully Working (Plugins Commented Out)
Comment 8 Brent Fulgham 2011-04-16 23:58:19 PDT
Created attachment 89942 [details]
Example of transparent background display

With the 89941 patch you can create opaque, transparent, and semi-transparent UI elements and interact with them.
Comment 9 Brent Fulgham 2011-04-17 20:40:08 PDT
Created attachment 89984 [details]
Fully functional implementation
Comment 10 WebKit Review Bot 2011-04-17 20:41:44 PDT
Attachment 89984 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/win/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit/win/WebView.cpp:997:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebKit/win/WebView.cpp:1050:  Extra space between SIZE and windowSize  [whitespace/declaration] [3]
Source/WebKit/win/WebView.cpp:2187:  This { should be at the end of the previous line  [whitespace/braces] [4]
Source/WebKit/win/WebView.cpp:2189:  One space before end of line comments  [whitespace/comments] [5]
Source/WebKit/win/WebView.cpp:2189:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit/win/WebView.cpp:2195:  One space before end of line comments  [whitespace/comments] [5]
Source/WebKit/win/WebView.cpp:2649:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebKit/win/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Tools/WinLauncher/WinLauncher.cpp:62:  Extra space between SIZE and windowSize  [whitespace/declaration] [3]
Tools/WinLauncher/WinLauncher.cpp:71:  Extra space between BOOL and HasTransparentBackground  [whitespace/declaration] [3]
Tools/WinLauncher/WinLauncher.cpp:189:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/WinLauncher/WinLauncher.cpp:230:  This { should be at the end of the previous line  [whitespace/braces] [4]
Tools/WinLauncher/WinLauncher.cpp:258:  Extra space before ( in function call  [whitespace/parens] [4]
Tools/WinLauncher/WinLauncher.cpp:266:  This { should be at the end of the previous line  [whitespace/braces] [4]
Tools/WinLauncher/WinLauncher.cpp:270:  Missing space after ,  [whitespace/comma] [3]
Tools/WinLauncher/WinLauncher.cpp:281:  This { should be at the end of the previous line  [whitespace/braces] [4]
Tools/WinLauncher/WinLauncher.cpp:283:  This { should be at the end of the previous line  [whitespace/braces] [4]
Tools/WinLauncher/WinLauncher.cpp:483:  This { should be at the end of the previous line  [whitespace/braces] [4]
Tools/WinLauncher/WinLauncher.cpp:487:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Tools/WinLauncher/WinLauncher.cpp:488:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Tools/WinLauncher/WinLauncher.cpp:509:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/WebKit/win/WebView.h:129:  Extra space after ( in function call  [whitespace/parens] [4]
Total errors found: 22 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Brent Fulgham 2011-04-17 21:01:31 PDT
Created attachment 89985 [details]
Revised to correct style warnings.
Comment 12 WebKit Review Bot 2011-04-17 21:05:10 PDT
Attachment 89985 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/win/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit/win/WebView.cpp:2187:  Should only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Source/WebKit/win/WebView.cpp:2648:  Extra space after ( in function call  [whitespace/parens] [4]
Tools/WinLauncher/WinLauncher.cpp:71:  Extra space between BOOL and HasTransparentBackground  [whitespace/declaration] [3]
Source/WebKit/win/WebView.h:129:  Extra space after ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Build Bot 2011-04-17 21:20:20 PDT
Attachment 89984 [details] did not build on win:
Build output: http://queues.webkit.org/results/8442862
Comment 14 Build Bot 2011-04-17 21:25:43 PDT
Attachment 89985 [details] did not build on win:
Build output: http://queues.webkit.org/results/8450795
Comment 15 Brent Fulgham 2011-04-17 21:45:36 PDT
Created attachment 89990 [details]
Updated with property change to force clean Windows rebuild.
Comment 16 WebKit Review Bot 2011-04-17 21:48:48 PDT
Attachment 89990 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/win/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit/win/WebView.cpp:2648:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebKit/win/WebView.h:129:  Extra space after ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Build Bot 2011-04-17 22:28:40 PDT
Attachment 89990 [details] did not build on win:
Build output: http://queues.webkit.org/results/8459852
Comment 18 Brent Fulgham 2011-04-17 22:59:41 PDT
It looks like my attempt to touch WebKitLibraries/win/tools/vsprops/common.vsprops did not force the WebKit/win/Interfaces IDL change to rebuild.  Should I be touching some other file?
Comment 19 Brent Fulgham 2011-04-17 23:07:26 PDT
Created attachment 89997 [details]
Try touching a WebKit/win vsprop to force rebuild
Comment 20 WebKit Review Bot 2011-04-17 23:10:05 PDT
Attachment 89997 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit/win/ChangeLog', u'Source/Web..." exit_code: 1

Source/WebKit/win/WebView.cpp:2648:  Extra space after ( in function call  [whitespace/parens] [4]
Source/WebKit/win/WebView.h:129:  Extra space after ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Build Bot 2011-04-18 00:03:34 PDT
Attachment 89997 [details] did not build on win:
Build output: http://queues.webkit.org/results/8452862
Comment 22 Eric Seidel (no email) 2011-04-18 09:33:00 PDT
Comment on attachment 89997 [details]
Try touching a WebKit/win vsprop to force rebuild

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

Looks like it breaks the win-ews.

>> Source/WebKit/win/WebView.cpp:2648
>> +HRESULT STDMETHODCALLTYPE WebView::initTransparentViewWithFrame( 
> 
> Extra space after ( in function call  [whitespace/parens] [4]

This is a valid complaint, please fix.  You have whitespace at the end of your line.

>> Source/WebKit/win/WebView.h:129
>> +    virtual HRESULT STDMETHODCALLTYPE initTransparentViewWithFrame( 
> 
> Extra space after ( in function call  [whitespace/parens] [4]

Same here.
Comment 23 Brent Fulgham 2011-04-18 10:33:12 PDT
Created attachment 90048 [details]
Patch
Comment 24 Build Bot 2011-04-18 11:20:29 PDT
Attachment 90048 [details] did not build on win:
Build output: http://queues.webkit.org/results/8469037
Comment 25 Brent Fulgham 2011-04-18 13:16:57 PDT
Created attachment 90079 [details]
Patch
Comment 26 Brent Fulgham 2011-04-18 13:19:52 PDT
I have confirmed that this modification builds and runs under the standard Apple Windows build of WebKit.
Comment 27 Brent Fulgham 2011-04-19 11:33:13 PDT
Created attachment 90224 [details]
Patch
Comment 28 Brent Fulgham 2011-04-19 11:43:57 PDT
Created attachment 90229 [details]
Patch
Comment 29 Brent Fulgham 2011-04-19 14:29:23 PDT
Created attachment 90255 [details]
Patch (with dirty flag corrected)
Comment 30 Brent Fulgham 2011-04-19 14:30:26 PDT
Comment on attachment 90255 [details]
Patch (with dirty flag corrected)

I noticed that I had left a flag set to 'true' (to redraw the entire backing store every time), which I had used during debugging.  This is not needed, and reduces the work load of this operation.
Comment 31 Adam Roben (:aroben) 2011-04-19 16:45:03 PDT
Comment on attachment 90255 [details]
Patch (with dirty flag corrected)

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!
Comment 32 Adam Roben (:aroben) 2011-04-19 16:45:49 PDT
CCing Jon Honeycutt, who perhaps can tell us a little about the existing support for transparent WebViews. I think a UI delegate callback to tell you when the view has painted is involved.
Comment 33 Brent Fulgham 2011-04-19 18:05:23 PDT
(In reply to comment #31)
> How does this new mode interact with the existing IWebView::[set]Transparent APIs? We don't want to break users of those APIs.

This change requires that you also use "setTransparent", as this changes the default background color of the various layers to the correct setting.

Can anyone explain what the existing API call does?  When using it from WinLauncher, for example, I don't see any change in the rendered output.  Do I need to be doing something in a delegate to get it to work?

> > 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?

If you enable accelerated compositing in the Windows build, the drawing is not compatible with the layered window I'm using, so you get no screen updates.  It's possible that the result of the accelerated compositing could be blitted out to the layered window, but I'm not sure where this would be connected.
Comment 34 Brent Fulgham 2011-04-19 23:12:34 PDT
Comment on attachment 90255 [details]
Patch (with dirty flag corrected)

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

>> Source/WebKit/win/WebView.cpp:958
>> +        oldBitmap = ::SelectObject(bitmapDC, m_backingStoreBitmap->handle());
> 
> This cleanup should probably be done separately.

I'm afraid I don't understand.  Sorry!

>> 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.

Done

>> 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.

Done

>> Source/WebKit/win/WebView.cpp:1044
>> +    HDC hdcMem = ::CreateCompatibleDC(hdcScreen);
> 
> You should use OwnPtr to hold hdcMem.

Done

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

Done

>> Source/WebKit/win/WebView.cpp:1055
>> +    DrawChildWindowData data = {hWnd, hdcMem};
> 
> Missing spaces around the braces here.

Done

>> 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.

Done

>> Source/WebKit/win/WebView.cpp:1077
>> +    }
> 
> 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?

You are right -- we don't.  The only problem is that accelerated compositing is not compatible with the UpdateLayeredWindow call, so if a "transparent" window is passed to this routine, it will not paint anything.

To address a point you make later, the check for "transparent()" isn't really right.  We only want to bypass this in the layered window case, which is signified by the m_hostWindow being set to null.

>> Source/WebKit/win/WebView.cpp:2195
>> +            break;
> 
> I believe you also want to set lResult to 1.

Okay -- fixed.

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

Whoops!

>> 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.

Renamed to "computeFullDesktopFrame"

>> 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.

Done

>> Tools/WinLauncher/WinLauncher.cpp:211
>> +            s_fullDesktop = true;
> 
> WCHAR and wcsicmp and L would be better than TCHAR and _tcsicmp and TEXT().

Done.

>> Tools/WinLauncher/WinLauncher.cpp:238
>> +        UpdateWindow(hURLBarWnd);
> 
> You should probably not pass WS_VISIBLE to CreateWindow, since you then call ShowWindow.

Done.

>> Tools/WinLauncher/WinLauncher.cpp:513

> 
> I think this is dead code.

Whoops!

>> 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.

Done

>> Tools/WinLauncher/WinLauncher.cpp:520

> 
> 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!

I just deleted it.  We don't provide a menu in the 'transparent' case, so theres not much sense providing a handler for it.  This is, after all, just an example!
Comment 35 Brent Fulgham 2011-04-19 23:20:23 PDT
After talking a bit with Jon a bit on-line, I think that this change could be handled by implementing the IWebUIDelegatePrivate2 "drawBackground" delegate method.  However, in an hour or so of experimenting with it tonight I couldn't get the passed HDC to provide me with a bitmap to anything but a black rectangle the size of my window.

I sort of think it wanted ME to draw a background into that bitmap.  Could that be?

So, I'm updating this patch once again based on your comments.

I think it has a couple of useful advantages over the delegate implementation:
1. I understand how it works, and can get it to draw pretty pictures.
2. It shields the user from having to figure out the arcane combination of flags and dimensions to get UpdateLayeredWindow to behave properly.
3. It means that at some point in the future we can secretly swap out the UpdateLayeredWindow operation for something accelerated, since that logic will be hidden from the user (rather than scattered into bits of client code all over the place.)

Given a few minutes, Jon could probably explain exactly where I was stupid about IWebUIDelegatePrivate2 with my test application, and how to make it work properly; this would render at least item (1) moot.

I do think arguments (2) and (3) have some small value.
Comment 36 Jon Honeycutt 2011-04-20 04:35:50 PDT
(In reply to comment #35)
> After talking a bit with Jon a bit on-line, I think that this change could be handled by implementing the IWebUIDelegatePrivate2 "drawBackground" delegate method.  However, in an hour or so of experimenting with it tonight I couldn't get the passed HDC to provide me with a bitmap to anything but a black rectangle the size of my window.
> 
> I sort of think it wanted ME to draw a background into that bitmap.  Could that be?

Yes, I believe the drawBackground delegate method allows the client to draw a WebView's background before the WebView paints the page content.

> Given a few minutes, Jon could probably explain exactly where I was stupid about IWebUIDelegatePrivate2 with my test application, and how to make it work properly; this would render at least item (1) moot.
> 

If you want to access the the WebView's contents when they change, you can wait for IWebUIDelegatePrivate::webViewPainted() to be called and call IWebViewPrivate::paintDocumentRectToContext().

I didn't look closely at the patch, but it looked like it would interfere with existing use of setTransparent(). It might be good to add a setUsesLayeredWindow() for this new functionality rather than relying on whether the WebView is "transparent" or whether its host window is 0.
Comment 37 Brent Fulgham 2011-04-20 12:57:18 PDT
Created attachment 90391 [details]
Patch
Comment 38 Brent Fulgham 2011-04-20 13:09:35 PDT
Created attachment 90393 [details]
Patch
Comment 39 Brent Fulgham 2011-04-20 15:50:48 PDT
Created attachment 90429 [details]
Patch
Comment 40 Adam Roben (:aroben) 2011-04-22 08:37:12 PDT
Comment on attachment 90429 [details]
Patch

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.
Comment 41 Brent Fulgham 2011-04-22 13:35:51 PDT
Comment on attachment 90429 [details]
Patch

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

>> Source/WebKit/win/WebView.cpp:1013
>> +    int yOffset = childWindowRect.top - windowRect.top;
> 
> I think you could also use ::MapWindowPoints for this.

Good point.  Changed.

>> Source/WebKit/win/WebView.cpp:1032
>> +        PRF_CLIENT | PRF_CHILDREN | PRF_OWNED);
> 
> I'd just put this all on one line.

Done

>> 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.

Okay.  I'll change that.

>> Source/WebKit/win/WebView.cpp:1041
>> +    OwnPtr<HDC> hdcMem(::CreateCompatibleDC(hdcScreen));
> 
> The more modern way to do this is to use assignment and adoptPtr.

Will do.

>> Source/WebKit/win/WebView.cpp:1053
>> +    ::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.

I did some tests today, and this logic doesn't seem to be needed.  I can run Flash plugins (which is what I believe this was meant to support) without problems.  It may be that this was a bug a few years ago that has since been fixed, or that when the drawing was being done in Appcelerator's fork of WebKit (external to WebKit) that updates weren't happening properly.

I'll remove this call, as well as the helper function above.

Whee!  Smaller patch!

>> Source/WebKit/win/WebView.cpp:2181
>> +            }
> 
> 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.

(1) You are right.  I removed the handled flag reset, and I get consistent good CPU use (identical to without the flag).  This code was copied from the external WndProc implementation, which needed to call into DefWndProc to get BeginPaint/EndPaint set properly.  So, I'll remove that logic.

(2) Note that we only call WM_PRINTCLIENT for child windows. However, based on your observation in the prior comment we don't need to do that either, so that's also being removed.

>> Source/WebKit/win/WebView.cpp:2658
>> +    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.

I decided to pass in a DWORD for style and a DWORD for exStyle, which is selected in the specific method used to create the window.

>> Source/WebKit/win/WebView.cpp:2663
>> +HRESULT WebView::initWithFrameImpl(BSTR frameName, BSTR groupName)
> 
> Maybe initWithFrameCommon would be a little clearer.

Sure thing!

>> 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.

Okay

>> Source/WebKit/win/WebView.cpp:5910
>> +    if (m_usesLayeredWindow == !!usesLayeredWindow)
> 
> No need for the !!.

Okay

>> Source/WebKit/win/WebView.cpp:5916
>> +    }
> 
> 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.

Oh, right!  This should be two-parts with a GetWindowLongPtr first.

>> 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.

Okay

>> Source/WebKit/win/WebView.h:834
>> +    virtual HRESULT STDMETHODCALLTYPE usesLayeredWindow(BOOL* transparent);
> 
> "transparent" doesn't seem right anymore.

Agreed.  Fixed.

>> Source/WebKit/win/Interfaces/IWebView.idl:242
>> +    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.

Okay.

>> Tools/WinLauncher/WinLauncher.cpp:51
>> +LONG_PTR DefWebKitProc = 0;
> 
> Maybe WNDPROC would be an even better type?

Done.

>> Tools/WinLauncher/WinLauncher.cpp:78
>> +}
> 
> Maybe usesLayeredWebView() would be more accurate?

Done

>> Tools/WinLauncher/WinLauncher.cpp:174
>> +    if (!::SystemParametersInfo(SPI_GETWORKAREA, 0, reinterpret_cast<void*>(&desktop), 0))
> 
> I think static_cast would work here.

Done.

>> Tools/WinLauncher/WinLauncher.cpp:254
>> +        goto exit;
> 
> Looks like this is leaked.

Fixed.

>> Tools/WinLauncher/WinLauncher.cpp:258
>> +        goto exit;
> 
> And this.

Ditto.

>> Tools/WinLauncher/WinLauncher.cpp:260
>> +    standardPreferences->setAcceleratedCompositingEnabled(true);
> 
> I'm surprised this doesn't default to true. (You should really be using TRUE here.)

It's surprising, but true!  I've changed it to TRUE as well.

>> Tools/WinLauncher/WinLauncher.cpp:269
>> +        goto exit;
> 
> I think you're leaking this.

Yes, you are right.

>> Tools/WinLauncher/WinLauncher.cpp:523
>> +        int y = (int)(short)HIWORD(lParam);
> 
> Are all the casts really needed?

That's how MSDN defined it in some example I was looking at.  I'll get rid of them.

>> Tools/WinLauncher/WinLauncher.cpp:525
>> +        if ((y > window.top) && (y < window.top + 30))
> 
> Using a named constant for "30" would be clearer.

Done.

>> 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.

Done.
Comment 42 Brent Fulgham 2011-04-22 14:12:12 PDT
I was thinking that another possibility might be to require the user to access the IWebViewPrivate object to set the "usesLayeredWindow" setting prior to calling "initWithFrame".  If we did so, we could then just build the right kind of window in initWithFrame, without having to add another interface for "initLayeredWindowWithFrame".

Do you think that would be preferable?  Or is it clearer to have two different kinds of initialization routines -- one for regular Child windows (99% of the use cases), with a special accessor to give you the custom top-level PopUp style window?
Comment 43 Brent Fulgham 2011-04-22 14:56:47 PDT
Comment on attachment 90429 [details]
Patch

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

>>> Source/WebKit/win/WebView.cpp:5910
>>> +    if (m_usesLayeredWindow == !!usesLayeredWindow)
>> 
>> No need for the !!.
> 
> Okay

It turns out that this construct prevents a compile failure due to the following warning (treated as an error);
..\WebView.cpp(5858) : warning C4805: '==' : unsafe mix of type 'bool' and type 'BOOL' in operation
Comment 44 Brent Fulgham 2011-04-22 15:48:03 PDT
Created attachment 90786 [details]
Patch
Comment 45 Brent Fulgham 2011-04-22 17:10:32 PDT
Created attachment 90808 [details]
Patch
Comment 46 Brent Fulgham 2011-04-26 13:33:28 PDT
Created attachment 91149 [details]
Patch
Comment 47 Adam Roben (:aroben) 2011-04-26 15:25:57 PDT
Comment on attachment 91149 [details]
Patch

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.
Comment 48 Brent Fulgham 2011-04-26 15:57:40 PDT
Comment on attachment 91149 [details]
Patch

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

>> Source/WebKit/win/WebView.cpp:5842
>> +}
> 
> I'd simplify this to: return ::SetWindowLongPtr(...) || !::GetLastError();

Done

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

It was also wrong --  It needs to make sure to turn of WS_CLIPSIBLINGS, which is not valid for layered windows.

>> Source/WebKit/win/WebView.cpp:5874
>> +            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?

I updated the logic to handle this.  It should also unset the parent window change in the failure case.

>> Source/WebKit/win/WebView.cpp:5890
>> +    // SetWindowPos is called
> 
> Missing a period at the end of the comment.
> 
> Is this actually a problem for us in practice?

No -- removed.

>> 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.

Whoops!

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

Done.

>> Source/WebKit/win/Interfaces/IWebViewPrivate.idl:263
>> +
> 
> Might as well remove this.

Done.
Comment 49 Brent Fulgham 2011-04-26 16:59:51 PDT
Created attachment 91183 [details]
Patch
Comment 50 Brent Fulgham 2011-04-26 17:30:03 PDT
Committed r84990: <http://trac.webkit.org/changeset/84990>
Comment 51 xufan 2011-05-18 20:34:57 PDT
Created attachment 94038 [details]
scrollbar is transparent

scrollbar is transparent in when WebKit is transparent.
Comment 52 xufan 2011-05-18 21:31:13 PDT
Created attachment 94040 [details]
patch for transparent scrollbar

patch for transparent scrollbar, but there are some problem when the page has opaque flash plugin in a frame.
Comment 53 xufan 2011-05-19 00:37:59 PDT
Created attachment 94050 [details]
patch for transparent scrollbar,Fix the problem when the page has opaque flash plugin in a frame

Fix the problem when the page has opaque flash plugin in a frame
Comment 54 xufan 2011-05-19 00:56:38 PDT
we can change
> LONG_PTR newExStyle = origExStyle | WS_EX_LAYERED;
> LONG_PTR newStyle = (origStyle & ~(WS_CHILD | WS_CLIPSIBLINGS | WS_CLIPCHILDREN)) | WS_POPUP;
to
> LONG_PTR newExStyle = origExStyle | WS_EX_LAYERED;
> LONG_PTR newStyle = (origStyle & ~(WS_CHILD | WS_CLIPSIBLINGS | WS_CLIPCHILDREN)) | WS_POPUP | WS_SYSMENU | WS_MINIMIZEBOX;
so we can close the window by right clicking in taskbar area.

(In reply to comment #48)
> (From update of attachment 91149 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91149&action=review
> 
> >> Source/WebKit/win/WebView.cpp:5842
> >> +}
> > 
> > I'd simplify this to: return ::SetWindowLongPtr(...) || !::GetLastError();
> 
> Done
> 
> >> Source/WebKit/win/WebView.cpp:5866
> >> +        newStyle = WS_POPUP;
> > 
> > Should be |=
> 
> It was also wrong --  It needs to make sure to turn of WS_CLIPSIBLINGS, which is not valid for layered windows.
> 
> >> Source/WebKit/win/WebView.cpp:5874
> >> +            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?
> 
> I updated the logic to handle this.  It should also unset the parent window change in the failure case.
> 
> >> Source/WebKit/win/WebView.cpp:5890
> >> +    // SetWindowPos is called
> > 
> > Missing a period at the end of the comment.
> > 
> > Is this actually a problem for us in practice?
> 
> No -- removed.
> 
> >> 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.
> 
> Whoops!
> 
> >> Source/WebKit/win/Interfaces/IWebViewPrivate.idl:157
> >> +    HRESULT usesLayeredWindow([out, retval] BOOL* transparent);
> > 
> > You should rename "transparent".
> 
> Done.
> 
> >> Source/WebKit/win/Interfaces/IWebViewPrivate.idl:263
> >> +
> > 
> > Might as well remove this.
> 
> Done.
Comment 55 Brent Fulgham 2011-05-19 09:01:13 PDT
(In reply to comment #54)
> we can change
> > LONG_PTR newExStyle = origExStyle | WS_EX_LAYERED;
> > LONG_PTR newStyle = (origStyle & ~(WS_CHILD | WS_CLIPSIBLINGS | WS_CLIPCHILDREN)) | WS_POPUP;
> to
> > LONG_PTR newExStyle = origExStyle | WS_EX_LAYERED;
> > LONG_PTR newStyle = (origStyle & ~(WS_CHILD | WS_CLIPSIBLINGS | WS_CLIPCHILDREN)) | WS_POPUP | WS_SYSMENU | WS_MINIMIZEBOX;
> so we can close the window by right clicking in taskbar area.

The intention here is to turn off the flags that are incompatible with the WS_EX_LAYERED feature.  Instead, these additional flags should be set by the user of the view.  I'll update the WinLauncher code to set these as you suggest when in transparent mode.
Comment 56 Brent Fulgham 2011-05-19 09:06:47 PDT
(In reply to comment #51)
> scrollbar is transparent in when WebKit is transparent.

Is this problem only present in Windows XP?  I don't see this when running under Windows 7.  I wonder if the controls in Windows 7/Vista have alpha channels and XP does not.
Comment 57 Adam Roben (:aroben) 2011-05-19 10:23:11 PDT
(In reply to comment #53)
> Created an attachment (id=94050) [details]
> patch for transparent scrollbar,Fix the problem when the page has opaque flash plugin in a frame
> 
> Fix the problem when the page has opaque flash plugin in a frame

Thanks for working to fix the issues! It would be great to file new bugs for new issues, rather than reusing this old bug. It makes the bookkeeping much easier.
Comment 58 Brent Fulgham 2011-05-19 12:37:31 PDT
I've moved the patch and screenshot to a new bug (https://bugs.webkit.org/show_bug.cgi?id=61136) to track this.
Comment 59 xufan 2011-05-19 19:16:38 PDT
Yes, it only present in Windows XP. In XP, IsThemeBackgroundPartiallyTransparent function return false.
> Source/WebCore/platform/win/ScrollbarThemeWin.cpp:351
> void ScrollbarThemeWin::paintThumb(GraphicsContext* context, Scrollbar* scrollbar, const IntRect& rect)
> {
> ...
>     bool alphaBlend = false;
>     if (scrollbarTheme)
>         alphaBlend = IsThemeBackgroundPartiallyTransparent(scrollbarTheme, scrollbar->orientation() == HorizontalScrollbar ? SP_THUMBHOR : SP_THUMBVERT, state);
>     HDC hdc = context->getWindowsContext(rect, alphaBlend);
>     RECT themeRect(rect);
>     if (scrollbarTheme) {
>         DrawThemeBackground(scrollbarTheme, hdc, scrollbar->orientation() == HorizontalScrollbar ? SP_THUMBHOR : SP_THUMBVERT, state, &themeRect, 0);
>         paintGripper(scrollbar, hdc, gripperRect(scrollbarThickness(), rect));
>     } else
>         ::DrawEdge(hdc, &themeRect, EDGE_RAISED, BF_RECT | BF_MIDDLE);
>     context->releaseWindowsContext(hdc, rect, alphaBlend);
> }


(In reply to comment #56)
> (In reply to comment #51)
> > scrollbar is transparent in when WebKit is transparent.
> 
> Is this problem only present in Windows XP?  I don't see this when running under Windows 7.  I wonder if the controls in Windows 7/Vista have alpha channels and XP does not.
Comment 60 Brent Fulgham 2011-05-19 20:18:57 PDT
(In reply to comment #59)
> Yes, it only present in Windows XP. In XP, IsThemeBackgroundPartiallyTransparent function return false.

Ah!  Thank you for catching this.  Let's move the conversation to the new bug so we can get this landed.
Comment 61 Adam Roben (:aroben) 2012-01-04 11:36:50 PST
Looks like this caused bug 75562.