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.
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.
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.
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
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
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
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
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
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
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.
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.
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.
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
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
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
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?
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. :)
(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. :)
(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.
2016-01-28 05:56 PST, Carlos Garcia Campos
2016-01-29 02:12 PST, Carlos Garcia Campos
2016-01-29 03:01 PST, Carlos Garcia Campos
2016-01-29 03:46 PST, Carlos Garcia Campos
2016-02-03 16:32 PST, Alex Christensen
2016-02-03 20:56 PST, Alex Christensen
2016-02-03 21:21 PST, Alex Christensen
2016-02-03 22:08 PST, Build Bot
2016-02-03 22:16 PST, Build Bot
2016-02-03 22:20 PST, Build Bot
2016-02-04 02:12 PST, Carlos Garcia Campos
2016-02-04 03:07 PST, Build Bot
2016-02-04 03:11 PST, Build Bot
2016-02-04 03:13 PST, Build Bot
2016-02-09 01:46 PST, Carlos Garcia Campos
2016-02-09 02:41 PST, Build Bot
2016-02-09 02:45 PST, Build Bot
2016-02-09 02:50 PST, Build Bot
2016-02-15 08:37 PST, Carlos Garcia Campos
2016-02-16 03:07 PST, Carlos Garcia Campos