Bug 211342 – Make it possible to test overlay scrollbar interactions

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Make it possible to test overlay scrollbar interactions

[Simon Fraser (smfr)]

Created attachment 398292 [details]
Patch

[Daniel Bates]

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.

[Simon Fraser (smfr)]

Created attachment 398296 [details]
Patch

[Daniel Bates]

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.

[Simon Fraser (smfr)]

Created attachment 398297 [details]
Patch

[EWS]

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).

[Simon Fraser (smfr)]

https://trac.webkit.org/changeset/261056/webkit

[Radar WebKit Bug Importer]

<rdar://problem/62782371>

[Simon Fraser (smfr)]

*** Bug 199323 has been marked as a duplicate of this bug. ***