RenderLayer::updateHoverActiveState contains the logic that updates hover and active states during hit-tests, but hover and active states are document states, and does have any relation to layers other than documents having at least one layer. By moving the function it will be in the same place as other active/hover state maintainace, plus we can avoid excessive updates of the states when some paths like RenderFlowThread::nodeAtPoint starts new hit-tests on a layer-level. This patch/bug is the first step of an effort to move the side-effects of hover/active update out of hit-testing, so hit-testing ends up only doing hit-testing and does not cause side-effects.
Created attachment 162256 [details] Patch
Comment on attachment 162256 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162256&action=review > Source/WebCore/dom/Document.cpp:6124 > +static RenderObject* commonAncestor(RenderObject* obj1, RenderObject* obj2) nearestCommonAncestor? or ? closestCommon? > Source/WebCore/dom/Document.cpp:6132 > + for (RenderObject* currObj1 = obj1; currObj1; currObj1 = currObj1->hoverAncestor()) > + for (RenderObject* currObj2 = obj2; currObj2; currObj2 = currObj2->hoverAncestor()) > + if (currObj1 == currObj2) > + return currObj1; style guide says this needs braces.
(In reply to comment #2) > (From update of attachment 162256 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162256&action=review > > > Source/WebCore/dom/Document.cpp:6124 > > +static RenderObject* commonAncestor(RenderObject* obj1, RenderObject* obj2) > > nearestCommonAncestor? or ? closestCommon? > Good point. I think nearest common ancestor (NCA) is the standard term in algorithmics. > > Source/WebCore/dom/Document.cpp:6132 > > + for (RenderObject* currObj1 = obj1; currObj1; currObj1 = currObj1->hoverAncestor()) > > + for (RenderObject* currObj2 = obj2; currObj2; currObj2 = currObj2->hoverAncestor()) > > + if (currObj1 == currObj2) > > + return currObj1; > > style guide says this needs braces. Will fix.
Created attachment 162264 [details] Patch
Comment on attachment 162264 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=162264&action=review Looks good overall. One question: > Source/WebCore/rendering/RenderView.cpp:98 > +bool RenderView::hitTest(const HitTestRequest& request, const HitTestLocation& location, HitTestResult& result) > +{ > + bool inside = layer()->hitTest(request, location, result); > + > + // Next set up the correct :hover/:active state along the new chain. > + document()->updateHoverActiveState(request, result); whoever does not go through this new API won't get hover/active states updated? Is that safe?
(In reply to comment #5) > (From update of attachment 162264 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=162264&action=review > > Looks good overall. > > One question: > > > Source/WebCore/rendering/RenderView.cpp:98 > > +bool RenderView::hitTest(const HitTestRequest& request, const HitTestLocation& location, HitTestResult& result) > > +{ > > + bool inside = layer()->hitTest(request, location, result); > > + > > + // Next set up the correct :hover/:active state along the new chain. > > + document()->updateHoverActiveState(request, result); > > whoever does not go through this new API won't get hover/active states updated? Is that safe? It is intentional. The point is that some code-paths like RenderFlowThread::nodeAtPoint calls RenderLayer::hitTest, but does not need to make that update. The actual update will be handled again later by the layer that originally triggered the hit-test that ended up in RenderFlowThread::nodeAtPoint. I did check all places that calls the hitTest functions, and the only place the layer hitTest was called directly instead of going over RenderLayer, was something I had written myself for RenderFrameBase, and that was only written that way because I needed to call hitTest with a hitTestPoint that was not the same as the point in hitTestResult. Which is why the API for RenderView hit-test was expanded in this patch to make that possible.
(In reply to comment #6) > I did check all places that calls the hitTest functions, and the only place the layer hitTest was called directly instead of going over RenderLayer make that: instead of going over RenderView ..
(In reply to comment #6) > (In reply to comment #5) > > (From update of attachment 162264 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=162264&action=review > > > > Looks good overall. > > > > One question: > > > > > Source/WebCore/rendering/RenderView.cpp:98 > > > +bool RenderView::hitTest(const HitTestRequest& request, const HitTestLocation& location, HitTestResult& result) > > > +{ > > > + bool inside = layer()->hitTest(request, location, result); > > > + > > > + // Next set up the correct :hover/:active state along the new chain. > > > + document()->updateHoverActiveState(request, result); > > > > whoever does not go through this new API won't get hover/active states updated? Is that safe? > > It is intentional. The point is that some code-paths like RenderFlowThread::nodeAtPoint calls RenderLayer::hitTest, but does not need to make that update. The actual update will be handled again later by the layer that originally triggered the hit-test that ended up in RenderFlowThread::nodeAtPoint. > > I did check all places that calls the hitTest functions, and the only place the layer hitTest was called directly instead of going over RenderLayer, was something I had written myself for RenderFrameBase, and that was only written that way because I needed to call hitTest with a hitTestPoint that was not the same as the point in hitTestResult. Which is why the API for RenderView hit-test was expanded in this patch to make that possible. Ok. Adding Simon, in order to give him some time to comment.
Comment on attachment 162264 [details] Patch Seems to have trouble applying cleanly. Will land manually.
Committed r128468: <http://trac.webkit.org/changeset/128468>
Comment on attachment 162264 [details] Patch Clearing flags after landing