Bug 95858 – Move updateHoverActiveState to Document

• Component: UI Events
• Status: RESOLVED FIXED

[Allan Sandfeld Jensen]

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.

[Allan Sandfeld Jensen]

Created attachment 162256 [details]
Patch

[Kenneth Rohde Christiansen]

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.

[Allan Sandfeld Jensen]

(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.

[Allan Sandfeld Jensen]

Created attachment 162264 [details]
Patch

[Antonio Gomes]

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?

[Allan Sandfeld Jensen]

(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.

[Allan Sandfeld Jensen]

(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 ..

[Antonio Gomes]

(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.

[Allan Sandfeld Jensen]

Comment on attachment 162264 [details]
Patch

Seems to have trouble applying cleanly. Will land manually.

[Allan Sandfeld Jensen]

Committed r128468: <http://trac.webkit.org/changeset/128468>

[Allan Sandfeld Jensen]

Comment on attachment 162264 [details]
Patch

Clearing flags after landing