Bug 84088 – [chromium] Simplify occlusion tracker API by passing layer iterator data

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dana Jansens]

[chromium] Simplify occlusion tracker API by passing layer iterator data

[Dana Jansens]

Created attachment 137418 [details]
Patch

[Adrienne Walker]

Comment on attachment 137418 [details]
Patch

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

I like this a lot.  I think it makes the calling code so much cleaner.

> Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp:276
> +    void enterLayerAsItself(LayerChromium* layer, CCOcclusionTracker& occlusion)

Is it possible to add OcclusionTracker to Types as well and not duplicate all these functions?

> Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp:372
> +    void enterLayer(LayerChromium* layer, CCOcclusionTracker& occlusion)
> +    {
> +        if (m_lastLayerVisited)
> +            leaveLayer(m_lastLayerVisited, occlusion);

It wasn't obvious until I found this function why you were only entering layers and not leaving them in tests.  Is it possible to rename the enterLayerAsFoo functions something more like "processLayerAsFoo" or "updateOcclusionForFooLayer", and then do enterLayer/leaveLayer in the same function? I think that'd be a lot more clear than just adding another enterLayer(parent) to flush the occlusion.

[Dana Jansens]

Comment on attachment 137418 [details]
Patch

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

>> Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp:276
>> +    void enterLayerAsItself(LayerChromium* layer, CCOcclusionTracker& occlusion)
> 
> Is it possible to add OcclusionTracker to Types as well and not duplicate all these functions?

Ooh, yeh that should work.

>> Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp:372
>> +            leaveLayer(m_lastLayerVisited, occlusion);
> 
> It wasn't obvious until I found this function why you were only entering layers and not leaving them in tests.  Is it possible to rename the enterLayerAsFoo functions something more like "processLayerAsFoo" or "updateOcclusionForFooLayer", and then do enterLayer/leaveLayer in the same function? I think that'd be a lot more clear than just adding another enterLayer(parent) to flush the occlusion.

Some tests want to test things before the layer is left (for example while the layer is visited as a contributed surface, we test occlusion of the contributed surface).

Maybe there is a better name that signifies we are leaving the previous step as well..

[Adrienne Walker]

Comment on attachment 137418 [details]
Patch

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

>>> Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp:372
>>> +            leaveLayer(m_lastLayerVisited, occlusion);
>> 
>> It wasn't obvious until I found this function why you were only entering layers and not leaving them in tests.  Is it possible to rename the enterLayerAsFoo functions something more like "processLayerAsFoo" or "updateOcclusionForFooLayer", and then do enterLayer/leaveLayer in the same function? I think that'd be a lot more clear than just adding another enterLayer(parent) to flush the occlusion.
> 
> Some tests want to test things before the layer is left (for example while the layer is visited as a contributed surface, we test occlusion of the contributed surface).
> 
> Maybe there is a better name that signifies we are leaving the previous step as well..

I'm not sure I follow.  For the case of a contributing surface, it seems like you could test things before entering/leaving that layer as a contributed surface and that you don't need to test after entering but before leaving? Is there something I'm missing?

[Dana Jansens]

Comment on attachment 137418 [details]
Patch

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

>>>> Source/WebKit/chromium/tests/CCOcclusionTrackerTest.cpp:372
>>>> +            leaveLayer(m_lastLayerVisited, occlusion);
>>> 
>>> It wasn't obvious until I found this function why you were only entering layers and not leaving them in tests.  Is it possible to rename the enterLayerAsFoo functions something more like "processLayerAsFoo" or "updateOcclusionForFooLayer", and then do enterLayer/leaveLayer in the same function? I think that'd be a lot more clear than just adding another enterLayer(parent) to flush the occlusion.
>> 
>> Some tests want to test things before the layer is left (for example while the layer is visited as a contributed surface, we test occlusion of the contributed surface).
>> 
>> Maybe there is a better name that signifies we are leaving the previous step as well..
> 
> I'm not sure I follow.  For the case of a contributing surface, it seems like you could test things before entering/leaving that layer as a contributed surface and that you don't need to test after entering but before leaving? Is there something I'm missing?

Yes, I suppose you could, since CCOT::enterLayer() for a contributing surface is a no-op. I guess I'm trying vaguely to avoid exposing implementation details? But these tests are a mess of implementation details so at this point I'm probably being hypocritical, if it even makes sense here. :)

I'll do as you suggest, it should clean this up.

[Dana Jansens]

(In reply to comment #4)
> I'm not sure I follow.  For the case of a contributing surface, it seems like you could test things before entering/leaving that layer as a contributed surface and that you don't need to test after entering but before leaving? Is there something I'm missing?

Ok here's an example of my problem with this:


// The blur layer needs to throw away any occlusion from outside its subtree.
this->visitLayerAsItself(blurLayer, occlusion);
EXPECT_TRUE(occlusion.occlusionInScreenSpace().isEmpty());
EXPECT_TRUE(occlusion.occlusionInTargetSurface().isEmpty());

Here we want to test the occlusion that the tracker has when we enter the blurLayer's target surface, but without including the occlusion from the blurLayer itself.

The visit step since it leaves the blurLayer also adds occlusion for the layer itself. In this case it's okay.. but they are really two different tests.

[Dana Jansens]

I propose s/enterLayer/moveToLayer/ and s/leaveLayer/finishWithLayer/ as a possible workaround.

[Adrienne Walker]

(In reply to comment #6)

> The visit step since it leaves the blurLayer also adds occlusion for the layer itself. In this case it's okay.. but they are really two different tests.

If it's just true for the AsItself case, then how about a visitLayerAsItself() which does both and you can use more commonly and an enterLayerAsItself/leaveLayerAsItself that you can use in the edge case where you need to check between?

[Dana Jansens]

Created attachment 137779 [details]
Patch

Another example:

        this->moveToLayerAsItself(parent, occlusion);
        EXPECT_TRUE(occlusion.occluded(parent, IntRect(30, 30, 70, 70)));

We want to check the occlsion of |parent|, but before |parent| is added to that occlusion. We must 'enter' parent if it is in a different render surface than the previous layer, or we violate the usage of the occlusion tracker as it needs to know the current target. So we need to be able to visit the layer without marking occlusion here.


Here's a CL with renaming enter/leave, and with Types::OcclusionTrackerType, for your consideration.

[WebKit Review Bot]

Comment on attachment 137779 [details]
Patch

Attachment 137779 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12437036

[Dana Jansens]

Created attachment 137807 [details]
Patch

Something to make the world into rainbows of joy. No hidden leaveLayer(), using visit() enter() and leave() explicitly.

[Adrienne Walker]

Comment on attachment 137807 [details]
Patch

That's so much clearer, thanks!  R=me.

[WebKit Review Bot]

Comment on attachment 137807 [details]
Patch

Clearing flags on attachment: 137807

Committed r114601: <http://trac.webkit.org/changeset/114601>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.