[chromium] Simplify occlusion tracker API by passing layer iterator data
Created attachment 137418 [details] Patch
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.
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..
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?
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.
(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.
I propose s/enterLayer/moveToLayer/ and s/leaveLayer/finishWithLayer/ as a possible workaround.
(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?
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.
Comment on attachment 137779 [details] Patch Attachment 137779 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12437036
Created attachment 137807 [details] Patch Something to make the world into rainbows of joy. No hidden leaveLayer(), using visit() enter() and leave() explicitly.
Comment on attachment 137807 [details] Patch That's so much clearer, thanks! R=me.
Comment on attachment 137807 [details] Patch Clearing flags on attachment: 137807 Committed r114601: <http://trac.webkit.org/changeset/114601>
All reviewed patches have been landed. Closing bug.