| Summary: | Disambiguate "background color" and "contents as solid color" on GraphicsLayer | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||||||||||
| Component: | Layout and Rendering | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | bdakin, cc-bugs, cdumez, dglazkov, eric, jamesr, mifenton, noam, ojan.autocc, ossy, peter+ews, rwlbuis, simon.fraser, tonikitoo, webkit-ews, webkit.review.bot, yong.li.webkit, zeno | ||||||||||||||||
| Priority: | P2 | ||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||
| Bug Blocks: | 104942 | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Simon Fraser (smfr)
2012-12-12 14:14:37 PST
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. 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 |