Bug 104842 – Disambiguate "background color" and "contents as solid color" on GraphicsLayer

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Simon Fraser (smfr)]

GraphicsLayer is confused about background color, vs setting the contents to a solid color. This is causing some bugs and test failures.

[Simon Fraser (smfr)]

Created attachment 179128 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 179128 [details]
Patch

Not ready for review yet. Noam is going to fix other ports and update test results.

[Noam Rosenthal]

Created attachment 179148 [details]
Apply changes to TextureMapper / coordinated graphics

[Noam Rosenthal]

(In reply to comment #2)
> (From update of attachment 179128 [details])
> Not ready for review yet. Noam is going to fix other ports and update test results.

I think we can review the patches and then commit when we have all of them.

[Early Warning System Bot]

Comment on attachment 179148 [details]
Apply changes to TextureMapper / coordinated graphics

Attachment 179148 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15311018

[Early Warning System Bot]

Comment on attachment 179128 [details]
Patch

Attachment 179128 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15312033

[WebKit Review Bot]

Comment on attachment 179128 [details]
Patch

Attachment 179128 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15316027

[Noam Rosenthal]

Created attachment 179161 [details]
Texmap/chromium

[Noam Rosenthal]

Created attachment 179162 [details]
Test rebaseline

[Early Warning System Bot]

Comment on attachment 179161 [details]
Texmap/chromium

Attachment 179161 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15319021

[Peter Beverloo (cr-android ews)]

Comment on attachment 179128 [details]
Patch

Attachment 179128 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15318030

[Simon Fraser (smfr)]

Comment on attachment 179162 [details]
Test rebaseline

Can you also remove some Chromium-specific results now?

[Noam Rosenthal]

(In reply to comment #12)
> (From update of attachment 179162 [details])
> Can you also remove some Chromium-specific results now?

no, because those test return drawsContents for chromium.

[Darin Adler]

Comment on attachment 179161 [details]
Texmap/chromium

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

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:814
> -    if (m_backgroundColorSet)
> +    if (m_backgroundColor.isValid())
>          contentsLayer->setBackgroundColor(m_backgroundColor.rgb());
>      else
>          contentsLayer->setBackgroundColor(static_cast<RGBA32>(0));

This if statement isn’t needed. When you call Color::rgb() on an invalid color, you get 0, so we can just let it do its thing.

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:354
> -void GraphicsLayerTextureMapper::setContentsToBackgroundColor(const Color& color)
> +void GraphicsLayerTextureMapper::setContentsToSolidColor(const Color& color)

Does solid color mean a color with no alpha? If so, should this assert the color has a fully opaque alpha?

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:365
> -void CoordinatedGraphicsLayer::setContentsToBackgroundColor(const Color& color)
> +void CoordinatedGraphicsLayer::setContentsToSolidColor(const Color& color)

Does solid color mean a color with no alpha? If so, should this assert the color has a fully opaque alpha?

[EFL EWS Bot]

Comment on attachment 179128 [details]
Patch

Attachment 179128 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15321059

[EFL EWS Bot]

Comment on attachment 179161 [details]
Texmap/chromium

Attachment 179161 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15319088

[Build Bot]

Comment on attachment 179162 [details]
Test rebaseline

Attachment 179162 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15318076

New failing tests:
compositing/iframes/invisible-nested-iframe-hide.html
compositing/geometry/clip.html
compositing/layer-creation/fixed-position-and-transform.html
compositing/iframes/iframe-resize.html
compositing/geometry/bounds-ignores-hidden-composited-descendant.html
compositing/iframes/connect-compositing-iframe-delayed.html
compositing/iframes/connect-compositing-iframe2.html
compositing/columns/composited-in-paginated.html
compositing/geometry/fixed-position-composited-switch.html
compositing/iframes/become-overlapped-iframe.html
compositing/geometry/composited-in-columns.html
compositing/iframes/connect-compositing-iframe3.html
compositing/geometry/layer-due-to-layer-children-deep-switch.html
compositing/tiled-layers-hidpi.html
compositing/iframes/enter-compositing-iframe.html
compositing/geometry/ancestor-overflow-change.html
compositing/geometry/clip-inside.html
compositing/geometry/bounds-ignores-hidden-dynamic-negzindex.html
compositing/iframes/connect-compositing-iframe.html
compositing/iframes/become-composited-nested-iframes.html
compositing/iframes/composited-parent-iframe.html
compositing/iframes/invisible-nested-iframe-show.html
compositing/geometry/flipped-writing-mode.html
compositing/overflow-trumps-transform-style.html
compositing/backing/no-backing-for-clip.html
compositing/backing/no-backing-for-clip-overlap.html
compositing/backing/no-backing-for-perspective.html
compositing/layer-creation/animation-overlap-with-children.html

[Simon Fraser (smfr)]

(In reply to comment #14)
> > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:354
> > -void GraphicsLayerTextureMapper::setContentsToBackgroundColor(const Color& color)
> > +void GraphicsLayerTextureMapper::setContentsToSolidColor(const Color& color)
> 
> Does solid color mean a color with no alpha? If so, should this assert the color has a fully opaque alpha?
> 
> > Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedGraphicsLayer.cpp:365
> > -void CoordinatedGraphicsLayer::setContentsToBackgroundColor(const Color& color)
> > +void CoordinatedGraphicsLayer::setContentsToSolidColor(const Color& color)
> 
> Does solid color mean a color with no alpha? If so, should this assert the color has a fully opaque alpha?

I pondered on the "solid color" terminology too. It means "a uniform color"; we use it in the same way in Image::isSolidColor(), which just says whether the all the pixels in the image have the same color (which may include alpha).

[Build Bot]

Comment on attachment 179128 [details]
Patch

Attachment 179128 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15316106

New failing tests:
compositing/iframes/invisible-nested-iframe-hide.html
compositing/geometry/clip.html
compositing/layer-creation/fixed-position-and-transform.html
compositing/iframes/iframe-resize.html
compositing/geometry/bounds-ignores-hidden-composited-descendant.html
compositing/iframes/connect-compositing-iframe-delayed.html
compositing/iframes/connect-compositing-iframe2.html
compositing/columns/composited-in-paginated.html
compositing/geometry/fixed-position-composited-switch.html
compositing/layer-creation/no-compositing-for-preserve-3d.html
compositing/iframes/become-overlapped-iframe.html
compositing/geometry/composited-in-columns.html
compositing/layer-creation/fixed-position-under-transform.html
compositing/iframes/connect-compositing-iframe3.html
compositing/tiled-layers-hidpi.html
compositing/iframes/enter-compositing-iframe.html
compositing/geometry/ancestor-overflow-change.html
compositing/geometry/clip-inside.html
compositing/geometry/bounds-ignores-hidden-dynamic-negzindex.html
compositing/iframes/connect-compositing-iframe.html
compositing/iframes/become-composited-nested-iframes.html
compositing/layer-creation/overflow-scroll-overlap.html
compositing/iframes/composited-parent-iframe.html
compositing/iframes/invisible-nested-iframe-show.html
compositing/geometry/flipped-writing-mode.html
compositing/overflow-trumps-transform-style.html
compositing/backing/no-backing-for-clip.html
compositing/backing/no-backing-for-clip-overlap.html
compositing/backing/no-backing-for-perspective.html
compositing/layer-creation/animation-overlap-with-children.html

[Noam Rosenthal]

Those patches fail on EWS because they need to be committed as one block...
Simon, once you want your patch reviewed I can upload them together as a batch for EWS, and at that point I can also remove the unnecessary comments as Darin suggested.

[Simon Fraser (smfr)]

I am making one big patch right now.

[Simon Fraser (smfr)]

Created attachment 179383 [details]
Patch (just needs review on the GraphicsLayer/GraphicsLayerCA parts)

[WebKit Review Bot]

Comment on attachment 179383 [details]
Patch (just needs review on the GraphicsLayer/GraphicsLayerCA parts)

Attachment 179383 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15312491

[Peter Beverloo (cr-android ews)]

Comment on attachment 179383 [details]
Patch (just needs review on the GraphicsLayer/GraphicsLayerCA parts)

Attachment 179383 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15322446

[Simon Fraser (smfr)]

Created attachment 179395 [details]
Patch

[Peter Beverloo (cr-android ews)]

Comment on attachment 179395 [details]
Patch

Attachment 179395 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/15320465

[WebKit Review Bot]

Comment on attachment 179395 [details]
Patch

Attachment 179395 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15310543

[Simon Fraser (smfr)]

Created attachment 179529 [details]
Patch

[Dean Jackson]

Comment on attachment 179529 [details]
Patch

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

> Source/WebCore/ChangeLog:23
> [phone]  No'am Rosenthal  <[e-mail]>
> +
> +        Disambiguate "background color" and "contents as solid color" on GraphicsLayer
> +        https://bugs.webkit.org/show_bug.cgi?id=104842
> +
> +        Reviewed by Simon Fraser.
> +
> +        Rename overloads in GraphicsLayerTextureMapper and GraphicsLayerChromium to account for the rename in GraphicsLayer.
> +
> +        Covered by existing tests.
> +
> +        * platform/graphics/chromium/GraphicsLayerChromium.cpp:
> +        * platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:
> +        (WebCore::GraphicsLayerTextureMapper::setContentsToSolidColor):
> +        * platform/graphics/texmap/GraphicsLayerTextureMapper.h:
> +        (GraphicsLayerTextureMapper):
> +        (WebCore::GraphicsLayerTextureMapper::solidColor):
> +        * platform/graphics/texmap/TextureMapperLayer.cpp:
> +        (WebCore::TextureMapperLayer::paintSelf):
> +        (WebCore::TextureMapperLayer::flushCompositingStateSelf):
> +        * platform/graphics/texmap/TextureMapperLayer.h:
> +        (State):
> +

This confused me, but I understand now.

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/137798

[Chris Dumez]

FYI, I rebaselined several compositing test cases due to this commit:
http://trac.webkit.org/changeset/137837

[Csaba Osztrogonác]

And Qt rebaselines landed in https://trac.webkit.org/changeset/137882