Bug 70085 - [Chromium] Disable blending when drawing opaque layers
Summary: [Chromium] Disable blending when drawing opaque layers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-13 18:52 PDT by Antoine Labour
Modified: 2011-10-27 07:24 PDT (History)
6 users (show)

See Also:


Attachments
Patch (4.51 KB, patch)
2011-10-13 18:54 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff
Patch (12.95 KB, patch)
2011-10-26 12:47 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff
Patch (12.96 KB, patch)
2011-10-26 13:12 PDT, Antoine Labour
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Labour 2011-10-13 18:52:19 PDT
Disable blending when drawing opaque layers
Comment 1 Antoine Labour 2011-10-13 18:54:07 PDT
Created attachment 110950 [details]
Patch
Comment 2 Adrienne Walker 2011-10-14 11:46:08 PDT
It's unfortunate that there's no way to test this in WebKit, since nothing in WebKit sets the opaque flag yet (except for Safari on the noncomposited content).
Comment 3 James Robinson 2011-10-20 16:05:01 PDT
Once https://bugs.webkit.org/show_bug.cgi?id=70554 lands you should be able to construct a layout test for this (or see if we have layout tests already), right? This patch looks pretty correct by inspection but I'd feel a lot better going forward if I knew we had test coverage for pixel correctness, in case a future factor accidentally messes this up.
Comment 4 James Robinson 2011-10-20 16:07:35 PDT
Oh - what about setting the opaque flag for the root layer?  Right now we disable blending for this layer using the 'is non-composited content host' bit:

http://google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp&q=disable%20BLEND%20file:chromium&exact_package=chromium&l=145

The code would be much better if this used the 'is opaque' bit instead, IMO.
Comment 5 Dana Jansens 2011-10-20 16:13:20 PDT
Is checking this==rootLayer() sufficient for what you are asking?

In a unified compositor, would the root layer be the desktop?  Presumably there is/would be some other flag to tell a tab contents root window. Is this something I shouldn't worry about?
Comment 6 James Robinson 2011-10-20 16:15:58 PDT
By "root" here I mean the non-composited content host layer. I don't know if there's an equivalent for users of the WebLayer API, but my point is you could clean up the webkit compositing logic by reusing this bit instead of what we currently do of special-casing this layer.
Comment 7 Antoine Labour 2011-10-26 12:47:37 PDT
Created attachment 112579 [details]
Patch
Comment 8 Antoine Labour 2011-10-26 12:49:11 PDT
This new patch added a unit test, please take a look (it also made MockWebGraphicsContext3D a bit more useful).
I'll try to see if I can also make a layout test.
Comment 9 James Robinson 2011-10-26 12:57:31 PDT
Comment on attachment 112579 [details]
Patch

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

Cool tests! R=me

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:204
> +    BlendStateCheckLayer(int id)

nit: explicit preferred on one-arg c'tors, although since this is private it's not a big deal
Comment 10 James Robinson 2011-10-26 12:58:45 PDT
I think we won't be able to make useful layout tests until https://bugs.webkit.org/show_bug.cgi?id=70634 or https://bugs.webkit.org/show_bug.cgi?id=70564 are addressed
Comment 11 Antoine Labour 2011-10-26 13:12:44 PDT
Created attachment 112580 [details]
Patch
Comment 12 Antoine Labour 2011-10-26 13:13:22 PDT
Same one, with explicit.
Comment 13 WebKit Review Bot 2011-10-27 07:24:04 PDT
Comment on attachment 112580 [details]
Patch

Clearing flags on attachment: 112580

Committed r98567: <http://trac.webkit.org/changeset/98567>
Comment 14 WebKit Review Bot 2011-10-27 07:24:11 PDT
All reviewed patches have been landed.  Closing bug.