Closed Bug 1440118 Opened 6 years ago Closed 6 years ago

Add a mochitest for scrolling over an unlayerized scroll frame with non-default overscroll-behavior

Categories

(Core :: Panning and Zooming, enhancement, P3)

Product:
Core
Component:
Panning and Zooming
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: botond, Assigned: botond)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files)

Bug 1440118 - If the APZ content response timeout is set to zero, use the fallback (timeout) behavior unconditionally.
59 bytes, text/x-review-board-request
kats
: review+
Details
Bug 1440118 - Mochitest for scrolling over unlayerized subframe with overscroll-behavior.
59 bytes, text/x-review-board-request
kats
: review+
Details 
This bug tracks adding a mochitest for the scenario described in bug 1425573.
I have a WIP mochitest for this, but it's not failing as expected without my fix, and I haven't gotten to the bottom of why yet.

It also can't land until we either enable test_group_wheelevents.html for WebRender, or fix the bug for non-WR (the latter is tracked in bug 1440112).
Priority: -- → P3
Whiteboard: [gfx-noted]
(In reply to Botond Ballo [:botond] from comment #1)
> I have a WIP mochitest for this, but it's not failing as expected without my
> fix, and I haven't gotten to the bottom of why yet.

The cause turned out to be somewhat subtle: to make the test reliable, we wan't to run the main-thread timeout code right away if the timeout is set to 0, but we can't call it directly from InputQueue::ScheduleMainThreadTimeout(), because at the time that's called, the input event hasn't been added to the queue yet, and the timeout code expects it to be there.

> It also can't land until we either enable test_group_wheelevents.html for
> WebRender, or fix the bug for non-WR (the latter is tracked in bug 1440112).

I guess we could *land* it, it just wouldn't actually run until one of those things happen.
Comment on attachment 8953624 [details]
Bug 1440118 - If the APZ content response timeout is set to zero, use the fallback (timeout) behavior unconditionally.

https://reviewboard.mozilla.org/r/222836/#review228796


Code analysis found 2 defects in this patch:
 - 2 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: gfx/layers/apz/src/InputQueue.h:146
(Diff revision 1)
>  
> +  // RAII class for automatically running a timeout task that may
> +  // need to be run immediately after an event has been queued.
> +  class AutoRunImmediateTimeout {
> +  public:
> +    AutoRunImmediateTimeout(InputQueue* aQueue);

Error: Bad implicit conversion constructor for 'autorunimmediatetimeout' [clang-tidy: mozilla-implicit-constructor]

::: gfx/layers/apz/src/InputQueue.cpp:827
(Diff revision 1)
>    mActivePanGestureBlock = nullptr;
>    mActiveKeyboardBlock = nullptr;
>    mLastActiveApzc = nullptr;
>  }
>  
> +InputQueue::AutoRunImmediateTimeout::AutoRunImmediateTimeout(InputQueue* aQueue)

Error: Bad implicit conversion constructor for 'autorunimmediatetimeout' [clang-tidy: mozilla-implicit-constructor]
Comment on attachment 8953624 [details]
Bug 1440118 - If the APZ content response timeout is set to zero, use the fallback (timeout) behavior unconditionally.

https://reviewboard.mozilla.org/r/222836/#review228800


Code analysis found 2 defects in this patch:
 - 2 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: gfx/layers/apz/src/InputQueue.h:146
(Diff revision 1)
>  
> +  // RAII class for automatically running a timeout task that may
> +  // need to be run immediately after an event has been queued.
> +  class AutoRunImmediateTimeout {
> +  public:
> +    AutoRunImmediateTimeout(InputQueue* aQueue);

Error: Bad implicit conversion constructor for 'autorunimmediatetimeout' [clang-tidy: mozilla-implicit-constructor]

::: gfx/layers/apz/src/InputQueue.cpp:827
(Diff revision 1)
>    mActivePanGestureBlock = nullptr;
>    mActiveKeyboardBlock = nullptr;
>    mLastActiveApzc = nullptr;
>  }
>  
> +InputQueue::AutoRunImmediateTimeout::AutoRunImmediateTimeout(InputQueue* aQueue)

Error: Bad implicit conversion constructor for 'autorunimmediatetimeout' [clang-tidy: mozilla-implicit-constructor]
Comment on attachment 8953624 [details]
Bug 1440118 - If the APZ content response timeout is set to zero, use the fallback (timeout) behavior unconditionally.

https://reviewboard.mozilla.org/r/222836/#review228802


Code analysis found 2 defects in this patch:
 - 2 defects found by clang-tidy

You can run this analysis locally with:
 - `./mach static-analysis check path/to/file.cpp` (C/C++)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: gfx/layers/apz/src/InputQueue.h:146
(Diff revision 1)
>  
> +  // RAII class for automatically running a timeout task that may
> +  // need to be run immediately after an event has been queued.
> +  class AutoRunImmediateTimeout {
> +  public:
> +    AutoRunImmediateTimeout(InputQueue* aQueue);

Error: Bad implicit conversion constructor for 'autorunimmediatetimeout' [clang-tidy: mozilla-implicit-constructor]

::: gfx/layers/apz/src/InputQueue.cpp:827
(Diff revision 1)
>    mActivePanGestureBlock = nullptr;
>    mActiveKeyboardBlock = nullptr;
>    mLastActiveApzc = nullptr;
>  }
>  
> +InputQueue::AutoRunImmediateTimeout::AutoRunImmediateTimeout(InputQueue* aQueue)

Error: Bad implicit conversion constructor for 'autorunimmediatetimeout' [clang-tidy: mozilla-implicit-constructor]
Comment on attachment 8953624 [details]
Bug 1440118 - If the APZ content response timeout is set to zero, use the fallback (timeout) behavior unconditionally.

https://reviewboard.mozilla.org/r/222836/#review228808

::: gfx/layers/apz/src/InputQueue.cpp:829
(Diff revision 2)
>    mLastActiveApzc = nullptr;
>  }
>  
> +InputQueue::AutoRunImmediateTimeout::AutoRunImmediateTimeout(InputQueue* aQueue)
> +  : mQueue(aQueue)
> +{}

maybe also MOZ_ASSERT that mQueue->mImmediateTimeout is null when this class is constructed, just for a sanity check.
Attachment #8953624 - Flags: review?(bugmail) → review+
Comment on attachment 8953625 [details]
Bug 1440118 - Mochitest for scrolling over unlayerized subframe with overscroll-behavior.

https://reviewboard.mozilla.org/r/222838/#review228810

Nice, thanks!
Attachment #8953625 - Flags: review?(bugmail) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #11)
> maybe also MOZ_ASSERT that mQueue->mImmediateTimeout is null when this class
> is constructed, just for a sanity check.

Done.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7858972f010e3b30065e7e570d727f6c50df3e40
Pushed by bballo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b938517fe114
If the APZ content response timeout is set to zero, use the fallback (timeout) behavior unconditionally. r=kats
https://hg.mozilla.org/integration/autoland/rev/8c0beb6eb43a
Mochitest for scrolling over unlayerized subframe with overscroll-behavior. r=kats
Blocks: 1440112
https://hg.mozilla.org/mozilla-central/rev/b938517fe114
https://hg.mozilla.org/mozilla-central/rev/8c0beb6eb43a
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.