Bug 116896 - ENABLE(PAN_SCROLLING) AutoscrollController::updateAutoscrollRenderer calls hitTestResultAtPoint with `true` for HitTestRequestType
Summary: ENABLE(PAN_SCROLLING) AutoscrollController::updateAutoscrollRenderer calls hi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Allan Sandfeld Jensen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-28 15:22 PDT by Joseph Pecoraro
Modified: 2013-06-06 10:10 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.65 KB, patch)
2013-06-05 08:06 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff
Patch (1.68 KB, patch)
2013-06-06 07:04 PDT, Allan Sandfeld Jensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2013-05-28 15:22:07 PDT
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.
Comment 1 Joseph Pecoraro 2013-05-28 15:32:44 PDT
See bug 95720:
<http://webkit.org/b/95720> Simplify hitTestResultAtPoint and nodesFromRect APIs
Comment 2 Allan Sandfeld Jensen 2013-06-05 08:00:48 PDT
Thanks, that looks like a bit of confusion between two different refactorings.
Comment 3 Allan Sandfeld Jensen 2013-06-05 08:06:39 PDT
Created attachment 203853 [details]
Patch
Comment 4 Allan Sandfeld Jensen 2013-06-05 08:08:42 PDT
Note that allowsShadowDOM is now also implied, and you only need to add a flag to disallow it.
Comment 5 Joseph Pecoraro 2013-06-05 12:10:28 PDT
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?
Comment 6 Allan Sandfeld Jensen 2013-06-06 07:01:35 PDT
(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.
Comment 7 Allan Sandfeld Jensen 2013-06-06 07:04:04 PDT
Created attachment 203932 [details]
Patch
Comment 8 Andreas Kling 2013-06-06 09:56:42 PDT
Comment on attachment 203932 [details]
Patch

r=me
Comment 9 WebKit Commit Bot 2013-06-06 10:10:12 PDT
Comment on attachment 203932 [details]
Patch

Clearing flags on attachment: 203932

Committed r151281: <http://trac.webkit.org/changeset/151281>
Comment 10 WebKit Commit Bot 2013-06-06 10:10:15 PDT
All reviewed patches have been landed.  Closing bug.