Bug 116896 – ENABLE(PAN_SCROLLING) AutoscrollController::updateAutoscrollRenderer calls hitTestResultAtPoint with `true` for HitTestRequestType

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joseph Pecoraro]

This looks like stale code after hitTestResultAtPoint changed signatures and replaced the second boolean parameter with an optional bitfield parameter.

AutoscrollController::updateAutoscrollRenderer passes bool true for a parameter that used to mean "allowsShadowDOM", but the parameter is now a bitfield of flags, one of which is AllowsShadowDOM.

This could would need to be updated. Or, better yet, if no ports enable PAN_SCROLLING the feature could be removed.

[Joseph Pecoraro]

See bug 95720:
<http://webkit.org/b/95720> Simplify hitTestResultAtPoint and nodesFromRect APIs

[Allan Sandfeld Jensen]

Thanks, that looks like a bit of confusion between two different refactorings.

[Allan Sandfeld Jensen]

Created attachment 203853 [details]
Patch

[Allan Sandfeld Jensen]

Note that allowsShadowDOM is now also implied, and you only need to add a flag to disallow it.

[Joseph Pecoraro]

Comment on attachment 203853 [details]
Patch

I think at the time this was written the signature of hitTestResultAtPoint was:

   HitTestResult hitTestResultAtPoint(const LayoutPoint&, bool allowShadowContent,
       bool ignoreClipping = false, HitTestScrollbars scrollbars = DontHitTestScrollbars,
       HitTestRequest::HitTestRequestType hitType = HitTestRequest::ReadOnly | HitTestRequest::Active,
       const LayoutSize& padding = LayoutSize());

Current trunk is:

    HitTestResult hitTestResultAtPoint(const LayoutPoint&,
        HitTestRequest::HitTestRequestType hitType = HitTestRequest::ReadOnly | HitTestRequest::Active | HitTestRequest::DisallowShadowContent,
        const LayoutSize& padding = LayoutSize());

So I believe this code intends to have:

    HitTestRequest::ReadOnly | HitTestRequest::Active (+ allow shadow DOM which is implicit)

But your change above ends up with just:

    HitTestRequest::ReadOnly (+ allow shadow DOM which is implicit)

Does this also need to include HitTestRequest::Active?

[Allan Sandfeld Jensen]

(In reply to comment #5)
> (From update of attachment 203853 [details])
> I think at the time this was written the signature of hitTestResultAtPoint was:
> 
>    HitTestResult hitTestResultAtPoint(const LayoutPoint&, bool allowShadowContent,
>        bool ignoreClipping = false, HitTestScrollbars scrollbars = DontHitTestScrollbars,
>        HitTestRequest::HitTestRequestType hitType = HitTestRequest::ReadOnly | HitTestRequest::Active,
>        const LayoutSize& padding = LayoutSize());
> 
> Current trunk is:
> 
>     HitTestResult hitTestResultAtPoint(const LayoutPoint&,
>         HitTestRequest::HitTestRequestType hitType = HitTestRequest::ReadOnly | HitTestRequest::Active | HitTestRequest::DisallowShadowContent,
>         const LayoutSize& padding = LayoutSize());
> 
> So I believe this code intends to have:
> 
>     HitTestRequest::ReadOnly | HitTestRequest::Active (+ allow shadow DOM which is implicit)
> 
> But your change above ends up with just:
> 
>     HitTestRequest::ReadOnly (+ allow shadow DOM which is implicit)
> 
> Does this also need to include HitTestRequest::Active?

To be the same call yes, it shouldn't matter, but I can re-add it to keep the code equivalent.

[Allan Sandfeld Jensen]

Created attachment 203932 [details]
Patch

[Andreas Kling]

Comment on attachment 203932 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 203932 [details]
Patch

Clearing flags on attachment: 203932

Committed r151281: <http://trac.webkit.org/changeset/151281>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.