Bug 153479 – Add a way to test ScrollAnimator

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Carlos Garcia Campos]

There's currently no way to test it.

[Carlos Garcia Campos]

Created attachment 270110 [details]
WIP patch

I've started to work on this. I'm not an expert in the testing webcore internals, so I don't know if this is the best way, but it doesn't seem to be easy to test things in platform without introducing layering violations. I'm not sure using console messages is the best way to log the events either. So, if this approach is acceptable, I'll finish the patch and add more tests, especially for the recently fixed issues, otherwise I'm open to any suggestion.

[Simon Fraser (smfr)]

Comment on attachment 270110 [details]
WIP patch

View in context: https://bugs.webkit.org/attachment.cgi?id=270110&action=review

I like this approach.

> Source/WebCore/rendering/RenderLayer.cpp:3237
> +    page->mainFrame().document()->addConsoleMessage(MessageSource::Other, MessageLevel::Debug, "RenderLayer: " + message);

Does this really have to log through the main frame? Seems odd to cross frame boundaries.

> Source/WebCore/rendering/RenderListBox.cpp:832
> +    page->mainFrame().document()->addConsoleMessage(MessageSource::Other, MessageLevel::Debug, "RenderListBox: " + message);

Ditto.

[Carlos Garcia Campos]

Created attachment 270194 [details]
Patch

It still needs the xcode changes, could someone with a mac add the new files to xcode files, please?

[Carlos Garcia Campos]

Patch doesn't apply because it depends on bug #153405.

[Carlos Garcia Campos]

Created attachment 270197 [details]
It should apply now

[Carlos Garcia Campos]

Created attachment 270198 [details]
Rebased patch

There was another conflict.

[WebKit Commit Bot]

Attachment 270198 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/mock/ScrollAnimatorMock.h:43:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/mock/ScrollAnimatorMock.h:45:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/mock/ScrollAnimatorMock.h:60:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/mock/ScrollAnimatorMock.cpp:39:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/mock/ScrollAnimatorMock.cpp:44:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/ScrollableArea.cpp:81:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 6 in 29 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Catanzaro]

The Mac and iOS bots are all red.

[Carlos Garcia Campos]

(In reply to comment #8)
> The Mac and iOS bots are all red.

See comment #3

[Alex Christensen]

Created attachment 270609 [details]
Patch

[Alex Christensen]

Created attachment 270625 [details]
Patch

[Alex Christensen]

Created attachment 270627 [details]
Patch

[Build Bot]

Comment on attachment 270627 [details]
Patch

Attachment 270627 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/780082

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 270632 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 270627 [details]
Patch

Attachment 270627 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/780099

New failing tests:
fast/scrolling/scrollable-area-overflow-auto-visibility-visible.html
fast/scrolling/scrollable-area-frame-scrolling-no.html
fast/scrolling/scroll-animator-select-list-events.html
fast/scrolling/scrollable-area-frame-scrolling-no-visibility-hidden-child.html
fast/scrolling/scroll-animator-basic-events.html
fast/scrolling/scrollable-area-overflow-auto-visibility-override.html
fast/scrolling/scrollable-area-overflow-auto-visibility-hidden.html
compositing/overflow/overflow-scrollbar-layer-positions.html
fast/scrolling/scrollable-area-frame-overflow-hidden.html
fast/scrolling/scrollable-area-frame-scrolling-yes.html
fast/scrolling/scrollbar-mousedown-move-mouseup.html
fast/scrolling/scrollable-area-frame.html
fast/scrolling/scrollbar-mousedown-mouseup.html
fast/scrolling/scroll-animator-overlay-scrollbars-hovered.html

[Build Bot]

Created attachment 270633 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 270627 [details]
Patch

Attachment 270627 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/780106

New failing tests:
fast/scrolling/scrollable-area-overflow-auto-visibility-visible.html
fast/scrolling/scrollable-area-frame-scrolling-no.html
fast/scrolling/scroll-animator-select-list-events.html
fast/scrolling/scrollable-area-frame-scrolling-no-visibility-hidden-child.html
tables/mozilla/bugs/bug149275-1.html
fast/scrolling/scroll-animator-basic-events.html
fast/scrolling/scrollable-area-overflow-auto-visibility-override.html
fast/scrolling/scrollable-area-overflow-auto-visibility-hidden.html
compositing/overflow/overflow-scrollbar-layer-positions.html
fast/scrolling/scrollable-area-frame-overflow-hidden.html
fast/scrolling/scrollable-area-frame-scrolling-yes.html
fast/scrolling/scrollbar-mousedown-move-mouseup.html
fast/scrolling/scrollable-area-frame.html
fast/scrolling/scrollbar-mousedown-mouseup.html
fast/scrolling/scroll-animator-overlay-scrollbars-hovered.html

[Build Bot]

Created attachment 270634 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Carlos Garcia Campos]

Thank you very much Alex, I'll check the tests results.

[Carlos Garcia Campos]

Created attachment 270646 [details]
Updated patch

I forgot to reset the value and all other tests were also reporting scroll animator events.

[Build Bot]

Comment on attachment 270646 [details]
Updated patch

Attachment 270646 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/780984

New failing tests:
compositing/overflow/overflow-scrollbar-layer-positions.html
fast/scrolling/scroll-animator-basic-events.html
fast/scrolling/scroll-animator-select-list-events.html
fast/scrolling/scroll-animator-overlay-scrollbars-hovered.html

[Build Bot]

Created attachment 270650 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 270646 [details]
Updated patch

Attachment 270646 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/780991

New failing tests:
compositing/overflow/overflow-scrollbar-layer-positions.html
fast/scrolling/scrollbar-tickmarks-hittest.html

[Build Bot]

Created attachment 270651 [details]
Archive of layout-test-results from ews107 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 270646 [details]
Updated patch

Attachment 270646 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/780985

New failing tests:
compositing/overflow/overflow-scrollbar-layer-positions.html
fast/scrolling/scroll-animator-basic-events.html
fast/scrolling/scroll-animator-select-list-events.html
fast/scrolling/scroll-animator-overlay-scrollbars-hovered.html

[Build Bot]

Created attachment 270652 [details]
Archive of layout-test-results from ews116 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Carlos Garcia Campos]

In WebKit1 we don't get notifications about frame scrollbars, I guess Mac uses different scrollbars for frames in wk1? In that case we can probably add specific expectations for wk1, and skip the ones that only use frame scrollbars.

[Carlos Garcia Campos]

The failure in wk2, compositing/overflow/overflow-scrollbar-layer-positions.html could be because it was not actually using overlay scrollbars before. The test does internals.setUsesOverlayScrollbars(true), but ScrollbarThemeMock didn't implement usesOverlayScrollbars() so it was always returning false before. This patch shouldn't affect existing tests at all, expect for this detail.

[Carlos Garcia Campos]

Created attachment 270916 [details]
Updated patch

I've updated the results of compositing/overflow/overflow-scrollbar-layer-positions.html, since new resutls look correct and it probably fixes bug #60716. I don't know why fast/scrolling/scrollbar-tickmarks-hittest.html is timing out, and I don't have a mac to debug it, but the fact that it also uses window.internals.setUsesOverlayScrollbars(true) but ScrollbarThemeMock always returned false before makes me think the test might not be correct (it passes in GTK+, though). And regarding the wk1 tests, I don't know what to do, because I don't know how frame scrollbars work in wk1, but I guess we need specific results for some of the tests and skip the ones using only frame scrollbars.

[Build Bot]

Comment on attachment 270916 [details]
Updated patch

Attachment 270916 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/803907

New failing tests:
fast/scrolling/scroll-animator-basic-events.html
fast/scrolling/scroll-animator-select-list-events.html
fast/scrolling/scroll-animator-overlay-scrollbars-hovered.html

[Build Bot]

Created attachment 270917 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 270916 [details]
Updated patch

Attachment 270916 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/803912

New failing tests:
fast/scrolling/scrollbar-tickmarks-hittest.html

[Build Bot]

Created attachment 270918 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 270916 [details]
Updated patch

Attachment 270916 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/803909

New failing tests:
fast/scrolling/scroll-animator-basic-events.html
fast/scrolling/scroll-animator-select-list-events.html
fast/scrolling/scroll-animator-overlay-scrollbars-hovered.html

[Build Bot]

Created attachment 270919 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Carlos Garcia Campos]

What can I do about the test failures? Do I add specific results for wk1 ports? does iOS also have its own frame scrollbars? Do I skip the tests that only receive frame scrollbars events? Regarding fast/scrolling/scrollbar-tickmarks-hittest.html, is that something specific to overlay scrollbars? or can I just remove the setUsesOverlayScrollbars(true) from the test?

[Carlos Garcia Campos]

Created attachment 271342 [details]
Try to fix mac tests

[Michael Catanzaro]

Comment on attachment 271342 [details]
Try to fix mac tests

View in context: https://bugs.webkit.org/attachment.cgi?id=271342&action=review

Good code, great tests, as I know to expect from you. ;)

I think you could/should add non-overlay scrollbar variants for some of these tests, though; it's arguably even more valuable to test those, as overlay scrollbars will be the default for us we'll be less likely to notice regressions in non-overlay scrollbars, which will still be important for sites that use them like Google domains. This could be done in a follow-up patch.

I've only checked the exact order of events in the expected results for the first tests; I presume you've checked them all.

Please wait a day or two before committing to get comments from others.

> LayoutTests/fast/scrolling/overlay-scrollbars-scroll-corner.html:19
> +  <p>This is a test for <a href="https://bugs.webkit.org/show_bug.cgi?id=153352">https://bugs.webkit.org/show_bug.cgi?id=153352</a></p>

This would be easier for folks debugging this in the future to understand if you add a sentence explaining what the result should look like.

> LayoutTests/fast/scrolling/scroll-animator-overlay-scrollbars-hovered.html:27
> +      <p style='white-space: nowrap'>Scroll animator should be notified when overlay scrollbars in main frame are hovered</p>

It's a good test, but think back to bug #152319, which was basically this bug for non-overlay scrollbars, but it only occurred when the scrollbar slider was a certain distance down its track (when you had scrolled down the page). This test would not catch a regression like that as-is. I would add a second step, where you scroll down and then repeat to verify the right events are still generated.

I also think we should test this in the non-overlay scrollbar case.

> LayoutTests/fast/scrolling/scrollbar-tickmarks-hittest.html:-16
> -        window.internals.setUsesOverlayScrollbars(true);

It was just a bug in the test?

> Source/WebCore/page/Settings.cpp:658
> +void Settings::setUseMockScrollAnimator(bool flag)

Did you mean to type Settings::setUsesMockScrollAnimator? If not, consider that name instead, to match Settings::setUsesPageCache and Settings::setUsesOverlayScrollbars. The alternative would be to match Settings::setShouldUseHighResolutionTimers, but I prefer setUses.

> Source/WebCore/page/Settings.cpp:663
> +bool Settings::useMockScrollAnimator()

Ditto on use -> uses.

And make it const, please; even though it's missing from some of the other functions here, most of them do have it.

> Source/WebCore/page/Settings.h:220
> +    static bool useMockScrollAnimator();

Ditto.

> Source/WebCore/platform/ScrollableArea.cpp:81
> +            m_scrollAnimator = ScrollAnimatorMock::create(const_cast<ScrollableArea&>(*this), [this](const String& message) {

Good way to avoid a platform layering violation.

> Source/WebCore/platform/ScrollableArea.h:311
> +    virtual void logMockScrollAnimatorMessage(const String&) const { };

OK, I guess it has to be const even though it does something, else you wouldn't be able to log from const functions....

> Source/WebCore/platform/mock/ScrollAnimatorMock.cpp:109
> +        message.appendLiteral("Unknown");

Would this ever not be a bug? It would be good to print a more clear warning.

> Source/WebCore/platform/mock/ScrollAnimatorMock.cpp:124
> +        message.appendLiteral("Unknown");

Ditto.

> Source/WebCore/platform/mock/ScrollAnimatorMock.cpp:144
> +        message.appendLiteral("Unknown");

Ditto.

> Source/WebCore/platform/mock/ScrollAnimatorMock.h:45
> +    ScrollAnimatorMock(ScrollableArea&, std::function<void(const String&)>&&);

It's odd and seems pointless to have both a static create function and a public constructor. In my experience:

 * If there is a create function, the constructor is private. I don't see any value in having both public.
 * If there is a create function, it returns a refcounted object. I don't see any value in adding having create functions that return std::unqiue_ptrs; has this become common practice in WebKit without my noticing? The create functions are just good to ensure you don't forget to adopt the initial reference, like I do with GObjects all the time.

> Source/WebCore/platform/mock/ScrollAnimatorMock.h:49
> +    virtual bool allowsHorizontalStretching(const PlatformWheelEvent&) override { return false; }

I think for new files, it's better to omit the redundant virtual keyword; it doesn't provide any value now that we have override. Well, it's better to do whatever most of the other WebKit developers are doing nowadays, but I think that is it....

Anyway, wouldn't it make sense to eventually implement these functions for iOS to test rubber banding? If so, should probably leave a comment to indicate that they're not meant to remain stubs forever.

> Source/WebCore/platform/mock/ScrollAnimatorMock.h:62
> +    void didAddVerticalScrollbar(Scrollbar*) override;

And here you have omitted the redundant virtual. At least be consistent within the same file, even if existing files are not. :)

[Carlos Garcia Campos]

(In reply to comment #38)
> Comment on attachment 271342 [details]
> Try to fix mac tests
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271342&action=review
> 
> Good code, great tests, as I know to expect from you. ;)

Thanks.

> I think you could/should add non-overlay scrollbar variants for some of
> these tests, though; it's arguably even more valuable to test those, as
> overlay scrollbars will be the default for us we'll be less likely to notice
> regressions in non-overlay scrollbars, which will still be important for
> sites that use them like Google domains. This could be done in a follow-up
> patch.
> 
> I've only checked the exact order of events in the expected results for the
> first tests; I presume you've checked them all.

Of course, I checked them when I wrote the tests.

> Please wait a day or two before committing to get comments from others.

Sure.

> > LayoutTests/fast/scrolling/overlay-scrollbars-scroll-corner.html:19
> > +  <p>This is a test for <a href="https://bugs.webkit.org/show_bug.cgi?id=153352">https://bugs.webkit.org/show_bug.cgi?id=153352</a></p>
> 
> This would be easier for folks debugging this in the future to understand if
> you add a sentence explaining what the result should look like.
> 
> > LayoutTests/fast/scrolling/scroll-animator-overlay-scrollbars-hovered.html:27
> > +      <p style='white-space: nowrap'>Scroll animator should be notified when overlay scrollbars in main frame are hovered</p>
> 
> It's a good test, but think back to bug #152319, which was basically this
> bug for non-overlay scrollbars, but it only occurred when the scrollbar
> slider was a certain distance down its track (when you had scrolled down the
> page). This test would not catch a regression like that as-is. I would add a
> second step, where you scroll down and then repeat to verify the right
> events are still generated.

That's a different bug, this was basically to check that main frame scrollbars are included in hit testing by the EventHandler on mouse move events. the other bug was about coordinates not being converted IIRC. The problem of adding more unrelated checks to theses tests is that you end up with a lot of events in the expectation file, making it a lot more difficult to check they are correct. So, I prefer to add a different test for every bug or behavior we want to ensure, instead of having tests covering all possible cases and bugs.

> I also think we should test this in the non-overlay scrollbar case.
> 
> > LayoutTests/fast/scrolling/scrollbar-tickmarks-hittest.html:-16
> > -        window.internals.setUsesOverlayScrollbars(true);
> 
> It was just a bug in the test?

I don't know why this test was setting this, but it had no effect because ScrollbarThemeMock didn't implement usesOverlayScrollbars().

> > Source/WebCore/page/Settings.cpp:658
> > +void Settings::setUseMockScrollAnimator(bool flag)
> 
> Did you mean to type Settings::setUsesMockScrollAnimator? If not, consider
> that name instead, to match Settings::setUsesPageCache and
> Settings::setUsesOverlayScrollbars. The alternative would be to match
> Settings::setShouldUseHighResolutionTimers, but I prefer setUses.

No, I meant Use because we are telling ScrollableAreas to use MockScrollAnimator, not asking whether it uses it or not. I agree it's not consistent, so I'll use Uses for consistency.

> > Source/WebCore/page/Settings.cpp:663
> > +bool Settings::useMockScrollAnimator()
> 
> Ditto on use -> uses.
> 
> And make it const, please; even though it's missing from some of the other
> functions here, most of them do have it.
> 
> > Source/WebCore/page/Settings.h:220
> > +    static bool useMockScrollAnimator();
> 
> Ditto.
> 
> > Source/WebCore/platform/ScrollableArea.cpp:81
> > +            m_scrollAnimator = ScrollAnimatorMock::create(const_cast<ScrollableArea&>(*this), [this](const String& message) {
> 
> Good way to avoid a platform layering violation.
> 
> > Source/WebCore/platform/ScrollableArea.h:311
> > +    virtual void logMockScrollAnimatorMessage(const String&) const { };
> 
> OK, I guess it has to be const even though it does something, else you
> wouldn't be able to log from const functions....
> 
> > Source/WebCore/platform/mock/ScrollAnimatorMock.cpp:109
> > +        message.appendLiteral("Unknown");
> 
> Would this ever not be a bug? It would be good to print a more clear warning.

The test will fail for sure, so I don't see the problem.

> > Source/WebCore/platform/mock/ScrollAnimatorMock.cpp:124
> > +        message.appendLiteral("Unknown");
> 
> Ditto.
> 
> > Source/WebCore/platform/mock/ScrollAnimatorMock.cpp:144
> > +        message.appendLiteral("Unknown");
> 
> Ditto.
> 
> > Source/WebCore/platform/mock/ScrollAnimatorMock.h:45
> > +    ScrollAnimatorMock(ScrollableArea&, std::function<void(const String&)>&&);
> 
> It's odd and seems pointless to have both a static create function and a
> public constructor. In my experience:
> 
>  * If there is a create function, the constructor is private. I don't see
> any value in having both public.
>  * If there is a create function, it returns a refcounted object. I don't
> see any value in adding having create functions that return
> std::unqiue_ptrs; has this become common practice in WebKit without my
> noticing? The create functions are just good to ensure you don't forget to
> adopt the initial reference, like I do with GObjects all the time.
> 
> > Source/WebCore/platform/mock/ScrollAnimatorMock.h:49
> > +    virtual bool allowsHorizontalStretching(const PlatformWheelEvent&) override { return false; }
> 
> I think for new files, it's better to omit the redundant virtual keyword; it
> doesn't provide any value now that we have override. Well, it's better to do
> whatever most of the other WebKit developers are doing nowadays, but I think
> that is it....
> 
> Anyway, wouldn't it make sense to eventually implement these functions for
> iOS to test rubber banding? If so, should probably leave a comment to
> indicate that they're not meant to remain stubs forever.
> 
> > Source/WebCore/platform/mock/ScrollAnimatorMock.h:62
> > +    void didAddVerticalScrollbar(Scrollbar*) override;
> 
> And here you have omitted the redundant virtual. At least be consistent
> within the same file, even if existing files are not. :)

[Carlos Garcia Campos]

Created attachment 271421 [details]
Patch for landing

[Michael Catanzaro]

(In reply to comment #39)
> That's a different bug, this was basically to check that main frame
> scrollbars are included in hit testing by the EventHandler on mouse move
> events. the other bug was about coordinates not being converted IIRC. The
> problem of adding more unrelated checks to theses tests is that you end up
> with a lot of events in the expectation file, making it a lot more difficult
> to check they are correct. So, I prefer to add a different test for every
> bug or behavior we want to ensure, instead of having tests covering all
> possible cases and bugs.

Good point.

If you don't intend to add a test for this now (I think you should) then please file a bug. Same for testing the non-overlay scrollbar cases.

[Carlos Garcia Campos]

Committed r196688: <http://trac.webkit.org/changeset/196688>