Renamed LayerChromium::opaque to LayerChromium::contentsOpaque. Having both opaque and opacity on t…

Renamed LayerChromium::opaque to LayerChromium::contentsOpaque. Having both opaque and opacity on the same interface is quite confusing.


BUG=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=159781

[enne (OOO), 2012-10-02 18:06:09]

lgtm

http://codereview.chromium.org/11035020/diff/1/cc/CCLayerTreeHostImplTest.cpp
File cc/CCLayerTreeHostImplTest.cpp (left):

http://codereview.chromium.org/11035020/diff/1/cc/CCLayerTreeHostImplTest.cpp...
cc/CCLayerTreeHostImplTest.cpp:1487: // Layer with translucent content, but
opaque content, so drawn without blending.
In retrospect, I have no idea what this was trying to test.

[danakj, 2012-10-02 18:12:08]

http://codereview.chromium.org/11035020/diff/1/cc/CCLayerTreeHostImplTest.cpp
File cc/CCLayerTreeHostImplTest.cpp (left):

http://codereview.chromium.org/11035020/diff/1/cc/CCLayerTreeHostImplTest.cpp...
cc/CCLayerTreeHostImplTest.cpp:1487: // Layer with translucent content, but
opaque content, so drawn without blending.
On 2012/10/02 18:06:09, enne wrote:
> In retrospect, I have no idea what this was trying to test.

Think I wrote this.

It has opaque pixels detected by the opaque paint tracker, but opaque flag
false.

setOpaqueContents here is setting a value on the layer to replicate actual
opaque pixel content, not a hinting flag.

Removing it from all these tests is decreasing what they are testing.

[alokp, 2012-10-02 18:21:50]

http://codereview.chromium.org/11035020/diff/1/cc/CCLayerTreeHostImplTest.cpp
File cc/CCLayerTreeHostImplTest.cpp (left):

http://codereview.chromium.org/11035020/diff/1/cc/CCLayerTreeHostImplTest.cpp...
cc/CCLayerTreeHostImplTest.cpp:1487: // Layer with translucent content, but
opaque content, so drawn without blending.
CCLayerImpl::setContentsOpaque refers to pixel content being opaque. I could not
tell a difference between setOpaqueContents and setContentsOpaque. I thought it
was added by somebody confused (like me) by the meaning of
CCLayerImpl::setOpaque.

I removed this test because after you remove setOpaqueContents, it becomes
identical to the one above it.

BTW I did not try to understand what the tests were doing. Most of the times
setOpaque and setOpaqueContents were given the same argument. The cases where
they were different, I used true because of the OR operator on line 1406.

[danakj, 2012-10-02 18:24:43]

Look at BlendStateCheckLayer in CCLTHImplTest.cpp. It is setting a flag to mimic
the behaviour of OpaqueRegionSkia and OpaqueRectTrackingContentLayerDelegate
which detect actual opaque pixels.

Please don't remove this setting this flag anywhere in these tests.

If you want to rename it to something else that's okay.

[enne (OOO), 2012-10-02 18:25:05]

http://codereview.chromium.org/11035020/diff/1/cc/CCLayerTreeHostImplTest.cpp
File cc/CCLayerTreeHostImplTest.cpp (left):

http://codereview.chromium.org/11035020/diff/1/cc/CCLayerTreeHostImplTest.cpp...
cc/CCLayerTreeHostImplTest.cpp:1487: // Layer with translucent content, but
opaque content, so drawn without blending.
I disagree.  setOpaqueContents is setting a value on the derived test layer and
so isn't testing anything outside of the test itself.

If BlendStateCheckLayer derived from CCTiledLayerImpl and overrode opaque
regions, that would be testing opaque pixel content.

[danakj, 2012-10-02 18:28:45]

On 2012/10/02 18:25:05, enne wrote:
> http://codereview.chromium.org/11035020/diff/1/cc/CCLayerTreeHostImplTest.cpp
> File cc/CCLayerTreeHostImplTest.cpp (left):
> 
>
http://codereview.chromium.org/11035020/diff/1/cc/CCLayerTreeHostImplTest.cpp...
> cc/CCLayerTreeHostImplTest.cpp:1487: // Layer with translucent content, but
> opaque content, so drawn without blending.
> I disagree.  setOpaqueContents is setting a value on the derived test layer
and
> so isn't testing anything outside of the test itself.
> 
> If BlendStateCheckLayer derived from CCTiledLayerImpl and overrode opaque
> regions, that would be testing opaque pixel content.

Hm. I see, it is basically doubling the opaque() flag here, because they both
end up in the resulting quads the same way.

OK I see what you mean. Taking back my last comment.

[danakj, 2012-10-02 18:31:27]

lgtm

[commit-bot: I haz the power, 2012-10-02 19:08:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alokp@chromium.org/11035020/1

[commit-bot: I haz the power, 2012-10-02 22:33:18]

Change committed as 159781