Summary: | Make it possible to test overlay scrollbar interactions | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||
Component: | Tools / Tests | Assignee: | Simon Fraser (smfr) <simon.fraser> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | koivisto, simon.fraser, thorton, webkit-bug-importer, zalan | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Simon Fraser (smfr)
2020-05-02 12:30:21 PDT
Created attachment 398292 [details]
Patch
Comment on attachment 398292 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398292&action=review Patch looks good. Better patch would use js-test.js instead of js-test-{pre, post} in tests. > Source/WebCore/platform/mac/ScrollAnimatorMac.mm:893 > + result.append(",expanded"_s); OK as-is. No change needed. Optimal solution would have space after ',' to make results human pretty. > Source/WebCore/testing/Internals.cpp:2756 > +ExceptionOr<ScrollableArea*> Internals::scrollableAreaForNode(RefPtr<Node>& node) const This is ok-is. No change needed. The current signature is optimal, but need to amend to take out a Ref<Node> INSIDE the function before the updateLayout. Alternative optimal solution is take param by CONST lvalue reference + use a local for the nullptr case. > Source/WebCore/testing/Internals.cpp:2767 > if (is<Document>(*node)) { OK as-is. No change needed. If do ^^^ then optimal solution omits the * here. > Source/WebCore/testing/Internals.cpp:2773 > } else if (is<Element>(*node)) { Ditto. > LayoutTests/fast/scrolling/mac/scrollbars/overlay-scrollbar-hovered.html:28 > + debug('Hovering vertical scrollbar should show expanded scrollbar'); OK as-is. No change needed. Slightly better solution is to prefix string with <br> because results will be prettier to read. Same thing can be down throughout this patch. Created attachment 398296 [details]
Patch
Comment on attachment 398296 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398296&action=review > Source/WebCore/testing/Internals.cpp:2756 > +ExceptionOr<ScrollableArea*> Internals::scrollableAreaForNode(RefPtr<Node>& node) const OK as-is. No change needed. This change makes node an out-argument. Is this OK? Optimal solutions in previous comment. Created attachment 398297 [details]
Patch
Dan Bates found in /Volumes/Data/worker/Commit-Queue/build/LayoutTests/ChangeLog does not appear to be a valid reviewer according to contributors.json. /Volumes/Data/worker/Commit-Queue/build/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). *** Bug 199323 has been marked as a duplicate of this bug. *** |