GraphicsLayer is confused about background color, vs setting the contents to a solid color. This is causing some bugs and test failures.
Created attachment 179128 [details] Patch
Comment on attachment 179128 [details] Patch Not ready for review yet. Noam is going to fix other ports and update test results.
Created attachment 179148 [details] Apply changes to TextureMapper / coordinated graphics
(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.
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
Comment on attachment 179128 [details] Patch Attachment 179128 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15312033
Comment on attachment 179128 [details] Patch Attachment 179128 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15316027
Created attachment 179161 [details] Texmap/chromium
Created attachment 179162 [details] Test rebaseline
Comment on attachment 179161 [details] Texmap/chromium Attachment 179161 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15319021
Comment on attachment 179128 [details] Patch Attachment 179128 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15318030
Comment on attachment 179162 [details] Test rebaseline Can you also remove some Chromium-specific results now?
(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.
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?
Comment on attachment 179128 [details] Patch Attachment 179128 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15321059
Comment on attachment 179161 [details] Texmap/chromium Attachment 179161 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15319088
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
(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).
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
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.
I am making one big patch right now.
Created attachment 179383 [details] Patch (just needs review on the GraphicsLayer/GraphicsLayerCA parts)
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
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
Created attachment 179395 [details] Patch
Comment on attachment 179395 [details] Patch Attachment 179395 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/15320465
Comment on attachment 179395 [details] Patch Attachment 179395 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15310543
Created attachment 179529 [details] Patch
Comment on attachment 179529 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179529&action=review > Source/WebCore/ChangeLog:23 > +2012-12-13 No'am Rosenthal <noam.rosenthal@nokia.com> > + > + 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.
http://trac.webkit.org/changeset/137798
FYI, I rebaselined several compositing test cases due to this commit: http://trac.webkit.org/changeset/137837
And Qt rebaselines landed in https://trac.webkit.org/changeset/137882