Bug 61136 – [Windows] Scrollbars Transparent in Windows XP if WebKit is using Layered Window

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Brent Fulgham]

Created attachment 94101 [details]
Screenshot showing the incorrect behavior.

xufan discovered that scrollbars may be transparent when WebKit is run with the WS_EX_LAYERED_WINDOW flag turned on.

[Brent Fulgham]

Created attachment 94102 [details]
Xufan's corrections for the missing scrollbars.

[Brent Fulgham]

Comment on attachment 94102 [details]
Xufan's corrections for the missing scrollbars.

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

Thanks for taking the time to figure this out!  There are some style issues with the patch, and I have a few questions I was hoping you could answer.

> Source/WebCore/platform/graphics/win/GraphicsContextCairoWin.cpp:96
> +static void setRGBABitmapAlpha(const XFORM& trans, DIBPixelData& pixelData, const IntRect& dstRect, unsigned char level)

Why do we need to adjust the dstRect values to account for the World Transform?  Maybe this should be done prior to calling this function so that the operation is performed on the passed rect.

Also, I think we should just pass the HBITMAP, rather than constructing the DIBPixelData object outside the routine.  The DIBPixelData is an implementation detail that is not used afterwards, so why make the caller construct it?

> Source/WebCore/platform/graphics/win/GraphicsContextCairoWin.cpp:99
> +    if (x < 0 || trans.eDx < 0) return ;

There are lots of little WebKit style violations in this file.  E.g., 'return' should be on its own line, and no space between the 'return' and the semicolon.

> Source/WebCore/platform/graphics/win/GraphicsContextCairoWin.cpp:123
> +    x = x << 2;

Why are these left shifts preferable to a multiplication?

> Source/WebCore/platform/graphics/win/GraphicsContextCairoWin.cpp:172
> +            setRGBABitmapAlpha(trans,  pixelData, dstRect, 255);

Question: Do we always want this to be fully opaque?  Are there cases where we should be looking at some user setting?

[Brent Fulgham]

Comment on attachment 94102 [details]
Xufan's corrections for the missing scrollbars.

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

>> Source/WebCore/platform/graphics/win/GraphicsContextCairoWin.cpp:172
>> +            setRGBABitmapAlpha(trans,  pixelData, dstRect, 255);
> 
> Question: Do we always want this to be fully opaque?  Are there cases where we should be looking at some user setting?

Follow up:  Should we maybe be using the result of the GraphicContext::getAlpha() call for this value?

[xufan]

Because scrollview has changed World Transform, we can also get it from context, but in Source/WebCore/plugins/win/PluginViewWin.cpp, it call directly SetWorldTransform function to do this.
> Source/WebCore/platform/scrollview.cpp:1035
>     context->translate(x(), y());
>     scrollViewDirtyRect.move(-x(), -y());
>     paintScrollbars(context, scrollViewDirtyRect);

> Source/WebCore/plugins/win/PluginViewWin.cpp:589
> SetWorldTransform(hdc, &transform);
> paintIntoTransformedContext(hdc);
> SetWorldTransform(hdc, &originalTransform);

the reason why I don't pass HBITMAP is that I want to write the code similar to the old code in Source/WebCore/platform/graphics/win/GraphicsContextCairoWin.cpp.

> Source/WebCore/platform/graphics/win/GraphicsContextCairoWin.cpp:
> void GraphicsContext::releaseWindowsContext(HDC hdc, const IntRect& dstRect, bool supportAlphaBlend, bool mayCreateBitmap)
> {
> ...
> OwnPtr<HBITMAP> bitmap = adoptPtr(static_cast<HBITMAP>(GetCurrentObject(hdc, OBJ_BITMAP)));

> DIBPixelData pixelData(bitmap.get());
> ASSERT(pixelData.bitsPerPixel() == 32);

> // If this context does not support alpha blending, then it may have
> // been drawn with GDI functions which always set the alpha channel
> // to zero. We need to manually set the bitmap to be fully opaque.
> unsigned char* bytes = reinterpret_cast<unsigned char*>(pixelData.buffer());
> if (!supportAlphaBlend)
>     setRGBABitmapAlpha(bytes, pixelData.size().height() * pixelData.bytesPerRow(), 255);

> drawBitmapToContext(m_data, platformContext()->cr(), pixelData, IntSize(dstRect.x(), dstRect.height() + dstRect.y()));

> ::DeleteDC(hdc);
> }

    
> Why do we need to adjust the dstRect values to account for the World Transform?  Maybe this should be done prior to calling this function so that the operation is performed on the passed rect.

> Also, I think we should just pass the HBITMAP, rather than constructing the DIBPixelData object outside the routine.  The DIBPixelData is an implementation detail that is not used afterwards, so why make the caller construct it?

Sorry, my falut.

> There are lots of little WebKit style violations in this file.  E.g., 'return' should be on its own line, and no space between the 'return' and the semicolon.

The original code is x *= 4, but lot's of bookes and people told us x = x << 2 may be a better way, because some compiler can't optimize it.

> Why are these left shifts preferable to a multiplication?

Yes, because This function is mainly used by native scroll(Source/WebCore/platform/win/ScrollbarThemeWin.cpp), plugin(Source/WebCore/plugins/win/PluginViewWin.cpp),native control(textbox, button and etc, drawControl function in source/webcore/rendering/renderthemewin.cpp), In XP, IsThemeBackgroundPartiallyTransparent function return false, so all of these is opaque(these don't support alpha blend). For example, if we create image button in the page, it will support alpha blend.
The old code in GraphicsContextCairoWin.cpp also set it to 255.
> if (!supportAlphaBlend)
>     setRGBABitmapAlpha(bytes, pixelData.size().height() * pixelData.bytesPerRow(), 255);

> Question: Do we always want this to be fully opaque?  Are there cases where we should be looking at some user setting?


(In reply to comment #2)
> (From update of attachment 94102 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94102&action=review
> 
> Thanks for taking the time to figure this out!  There are some style issues with the patch, and I have a few questions I was hoping you could answer.

[xufan]

Created attachment 94162 [details]
test page for this bug

You can test this page when WebKit is using Layered Window.

[xufan]

Created attachment 94164 [details]
corrections for the missing scrollbar, button and textbox

new patch, may be better.

[xufan]

another way to fix this bug is like below:
modify
> Source/WebCore/platform/graphics/win/GraphicsContextCairoWin.cpp
> void GraphicsContext::releaseWindowsContext(HDC hdc, const IntRect& dstRect, bool supportAlphaBlend, bool mayCreateBitmap)
> {
>     bool createdBitmap = mayCreateBitmap && (!m_data->m_hdc || inTransparencyLayer());
>     .....
> }
to
> void GraphicsContext::releaseWindowsContext(HDC hdc, const IntRect& dstRect, bool supportAlphaBlend, bool mayCreateBitmap)
> {
>     bool createdBitmap = mayCreateBitmap && (!m_data->m_hdc || !supportAlphaBlend || inTransparencyLayer());
>     .....
> }

modify
> Source/WebCore/platform/graphics/win/GraphicsContextWin.cpp:100
> HDC GraphicsContext::getWindowsContext(const IntRect& dstRect, bool supportAlphaBlend, bool mayCreateBitmap)
> {
>     // FIXME: Should a bitmap be created also when a shadow is set?
>     if (mayCreateBitmap && (!m_data->m_hdc || inTransparencyLayer())) {
>         ...
>     }
> }
to
> HDC GraphicsContext::getWindowsContext(const IntRect& dstRect, bool supportAlphaBlend, bool mayCreateBitmap)
> {
>     // FIXME: Should a bitmap be created also when a shadow is set?
>     if (mayCreateBitmap && (!m_data->m_hdc || !supportAlphaBlend || inTransparencyLayer())) {
>         ...
>     }
> }

This way has a problem: when we call WebView::setUsesLayeredWindow to change the window style dynamically(more than twice), it will fail in SetWindowLongPtr fuction, GetLastError() return ERROR_NOT_ENOUGH_MEMORY. May be some function called in GraphicsContext::getWindowsContext cause this problem.

[Brent Fulgham]

Comment on attachment 94164 [details]
corrections for the missing scrollbar, button and textbox

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

This is looking great!  I have a few questions that I need to run by some Apple people, and you will need to create a ChangeLog entry before we can land this patch.

> Source/WebCore/plugins/win/PluginViewWin.cpp:622
> +    LocalWindowsContext windowsContext(context, rectInWindow, m_isTransparent, false);

Is it always okay for this to be set to false?  I'm not sure under what circumstances the "mayCreateBitmap" flag is supposed to be true/false.

> Source/WebCore/plugins/win/PluginViewWin.cpp:644
> +        SetWorldTransform(windowsContext.hdc(), &originalTransform);

This looks correct to me.  Why was this not being done previously?  Are there cases (e.g., CoreGraphics) where we DO NOT want to restore the World Transform?  Maybe it's being handled at another level?

[xufan]

PluginViewWin.cpp don't need to call setRGBABitmapAlpha function in GraphicsContext::releaseWindowsContext function, because plugin may fit the alpha channel. The better way should be calling setRGBABitmapAlpha fuction directly in Source/WebCore/platform/win/ScrollbarThemeWin.cpp and drawControl function in Source/WebCore/Rendering/Renderthemewin.cpp.

(In reply to comment #8)
> (From update of attachment 94164 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94164&action=review
> 
> This is looking great!  I have a few questions that I need to run by some Apple people, and you will need to create a ChangeLog entry before we can land this patch.
> 
> > Source/WebCore/plugins/win/PluginViewWin.cpp:622
> > +    LocalWindowsContext windowsContext(context, rectInWindow, m_isTransparent, false);
> 
> Is it always okay for this to be set to false?  I'm not sure under what circumstances the "mayCreateBitmap" flag is supposed to be true/false.
> 
> > Source/WebCore/plugins/win/PluginViewWin.cpp:644
> > +        SetWorldTransform(windowsContext.hdc(), &originalTransform);
> 
> This looks correct to me.  Why was this not being done previously?  Are there cases (e.g., CoreGraphics) where we DO NOT want to restore the World Transform?  Maybe it's being handled at another level?

[xufan]

Created attachment 94323 [details]
This patch does not need to modify PluginViewWin.cpp 

This patch does not need to modify PluginViewWin.cpp

[Brent Fulgham]

Comment on attachment 94323 [details]
This patch does not need to modify PluginViewWin.cpp 

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

This looks much better -- now it's localized and clear as to exactly what it's doing.  Thank you for clarifying all of this!  I have added a few more suggestions after discussing it with some other WebKit folks (see below).

> Source/WebCore/rendering/RenderThemeWin.cpp:632
> +    x  += trans.eDx;

Some of this could be cleaned up by using the IntRect and IntSize types.  You could make an IntSize from the XFORM, then use it to adjust the rect position:
IntRect drawRect(dstRect);
IntSize transformedPosition(trans.eDx, trans.eDy);
drawRect.move(transformedPosition);

> Source/WebCore/rendering/RenderThemeWin.cpp:643
> +    if (y >= pixelDataHeight)

I would suggest doing this test and early return in the same place as the x coordinate (line 630 above)

> Source/WebCore/rendering/RenderThemeWin.cpp:653
> +    if (y + height > pixelDataHeight)

If you used an IntRect for the drawing area, you could use 'intersects' for these tests to see if there was any area that needed painting.

> Source/WebCore/rendering/RenderThemeWin.cpp:656
> +    if (x + width > pixelDataWidth)

ditto

[xufan]

Created attachment 95160 [details]
format code

[Brent Fulgham]

Comment on attachment 95160 [details]
format code

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

This looks great!  Nice job getting the rect math cleaned up in this last patch.

A couple of general comments:
1. Please provide a changelog entry for your modifications.
2. Please move the 'setRGBABitmapAlpha' implementation to the DIBPixelData cpp file, and declare it as a static function in the WebCore namespace.
3. Please don't bother making whitespace changes to areas of the code you are not modifying.  You can always create a second patch to handle whitespace if you wish.

Rejecting because there is no changelog entry.  Otherwise, looks great!

> Source/WebCore/rendering/RenderThemeWin.cpp:618
> +void setRGBABitmapAlpha(HDC hdc, const IntRect& dstRect, unsigned char level)

Please make this a static method in namespace WebCore in the DIBPixelData header/implementation.  Then we can get rid of the extern declaration and have both files #include the DIBPixelData.h header.

[Adam Roben (:aroben)]

Comment on attachment 95160 [details]
format code

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

> Source/WebCore/platform/win/ScrollbarThemeWin.cpp:221
> +extern void setRGBABitmapAlpha(HDC hdc, const IntRect& dstRect, unsigned char level);

It would be a lot better to declare this function in a header and include the header where needed.

> Source/WebCore/platform/win/ScrollbarThemeWin.cpp:275
> +    if(!alphaBlend && !context->inTransparencyLayer())

WebKit coding style says to put a space after "if". Please do this throughout your patch. (We should make the style bot check this, too!)

> Source/WebCore/rendering/RenderThemeWin.cpp:649
> +    size_t width = drawRect.width() << 2;
> +    size_t height = drawRect.height();
> +    int x = drawRect.x() << 2;
> +    for (size_t i = 0; i < height; i++) {
> +        unsigned char* p = bytes + x;
> +        for (size_t j = 0; j < width; j += 4)
> +            p[j + 3] = level;
> +        bytes += bytesPerRow;
> +    }

I think "* 4" would be clearer than " << 2". But even clearer would be to remove the multiplication and use an RGBQUAD* instead of unsigned char*.

[xufan]

Created attachment 96193 [details]
Move the setRGBABitmapAlpha implementation to the DIBPixelData.cpp and add ChangeLog

[Brent Fulgham]

Comment on attachment 96193 [details]
Move the setRGBABitmapAlpha implementation to the DIBPixelData.cpp and add ChangeLog

Looks great!  Now that the EFL bots are happy I'll land it.

[Brent Fulgham]

Committed r90053: <http://trac.webkit.org/changeset/90053>

[Brent Fulgham]

Note:  Added correction for WinCE build. <http://trac.webkit.org/changeset/90057>

[Brent Fulgham]

Note:  Added build rule correction for WinCE build. <http://trac.webkit.org/changeset/90061>

[Brent Fulgham]

Note:  Added final correction for WinCE build. <http://trac.webkit.org/changeset/90062>