Bug 104842

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:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

104942

Attachments:

Description	Flags
Patch	none
Apply changes to TextureMapper / coordinated graphics	webkit-ews: commit-queue-
Texmap/chromium	none
Test rebaseline	none
Patch (just needs review on the GraphicsLayer/GraphicsLayerCA parts)	none
Patch	none
Patch	dino: review+

Simon Fraser (smfr)

Reported 2012-12-12 14:14:37 PST

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

Attachments
Patch (15.58 KB, patch) 2012-12-12 14:36 PST, Simon Fraser (smfr)	no flags	Details Formatted Diff Diff
Apply changes to TextureMapper / coordinated graphics (10.48 KB, patch) 2012-12-12 16:39 PST, Noam Rosenthal	webkit-ews: commit-queue-	Details Formatted Diff Diff
Texmap/chromium (11.31 KB, patch) 2012-12-12 17:43 PST, Noam Rosenthal	no flags	Details Formatted Diff Diff
Test rebaseline (68.92 KB, patch) 2012-12-12 17:43 PST, Noam Rosenthal	no flags	Details Formatted Diff Diff
Patch (just needs review on the GraphicsLayer/GraphicsLayerCA parts) (96.12 KB, patch) 2012-12-13 17:16 PST, Simon Fraser (smfr)	no flags	Details Formatted Diff Diff
Patch (101.16 KB, patch) 2012-12-13 18:30 PST, Simon Fraser (smfr)	no flags	Details Formatted Diff Diff
Patch (101.16 KB, patch) 2012-12-14 14:01 PST, Simon Fraser (smfr)	dino: review+	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Simon Fraser (smfr)

Comment 1 2012-12-12 14:36:02 PST

Created attachment 179128 [details] Patch

Simon Fraser (smfr)

Comment 2 2012-12-12 14:36:30 PST

Comment on attachment 179128 [details] Patch Not ready for review yet. Noam is going to fix other ports and update test results.

Noam Rosenthal

Comment 3 2012-12-12 16:39:10 PST

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

Noam Rosenthal

Comment 4 2012-12-12 16:42:19 PST

(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 5 2012-12-12 16:52:17 PST

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 6 2012-12-12 16:58:03 PST

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 7 2012-12-12 17:21:37 PST

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

Comment 8 2012-12-12 17:43:12 PST

Created attachment 179161 [details] Texmap/chromium

Noam Rosenthal

Comment 9 2012-12-12 17:43:30 PST

Created attachment 179162 [details] Test rebaseline

Early Warning System Bot

Comment 10 2012-12-12 17:49:03 PST

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 11 2012-12-12 17:50:55 PST

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 12 2012-12-12 18:18:59 PST

Comment on attachment 179162 [details] Test rebaseline Can you also remove some Chromium-specific results now?

Noam Rosenthal

Comment 13 2012-12-12 18:41:28 PST

(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 14 2012-12-12 19:21:25 PST

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 15 2012-12-12 19:53:46 PST

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 16 2012-12-12 20:04:10 PST

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 17 2012-12-12 20:05:20 PST

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)

Comment 18 2012-12-12 20:43:00 PST

(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 19 2012-12-12 20:49:55 PST

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

Comment 20 2012-12-13 17:11:04 PST

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)

Comment 21 2012-12-13 17:11:51 PST

I am making one big patch right now.

Simon Fraser (smfr)

Comment 22 2012-12-13 17:16:26 PST

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

WebKit Review Bot

Comment 23 2012-12-13 18:04:19 PST

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 24 2012-12-13 18:19:21 PST

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)

Comment 25 2012-12-13 18:30:31 PST

Created attachment 179395 [details] Patch

Peter Beverloo (cr-android ews)

Comment 26 2012-12-13 18:46:44 PST

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 27 2012-12-13 18:49:53 PST

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)

Comment 28 2012-12-14 14:01:02 PST

Created attachment 179529 [details] Patch

Dean Jackson

Comment 29 2012-12-14 14:14:47 PST

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.

Simon Fraser (smfr)

Comment 30 2012-12-15 11:13:08 PST

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

Chris Dumez

Comment 31 2012-12-16 06:55:41 PST

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

Csaba Osztrogonác

Comment 32 2012-12-17 00:09:29 PST

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

Note You need to log in before you can comment on or make changes to this bug.