Bug 211342 - Make it possible to test overlay scrollbar interactions
Summary: Make it possible to test overlay scrollbar interactions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
: 199323 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-05-02 12:30 PDT by Simon Fraser (smfr)
Modified: 2020-05-08 16:06 PDT (History)
5 users (show)

See Also:

Attachments
Patch (29.75 KB, patch)
2020-05-02 12:33 PDT, Simon Fraser (smfr) 		no flags Details | Formatted Diff | Diff
Patch (29.84 KB, patch)
2020-05-02 13:40 PDT, Simon Fraser (smfr) 		dbates: review+
Details | Formatted Diff | Diff
Patch (29.92 KB, patch)
2020-05-02 16:20 PDT, Simon Fraser (smfr) 		ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2020-05-02 12:30:21 PDT 
Make it possible to test overlay scrollbar interactions
Comment 1 Simon Fraser (smfr) 2020-05-02 12:33:17 PDT 
Created attachment 398292 [details]
Patch
Comment 2 Daniel Bates 2020-05-02 13:39:54 PDT 
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.
Comment 3 Simon Fraser (smfr) 2020-05-02 13:40:01 PDT 
Created attachment 398296 [details]
Patch
Comment 4 Daniel Bates 2020-05-02 14:20:03 PDT 
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.
Comment 5 Simon Fraser (smfr) 2020-05-02 16:20:53 PDT 
Created attachment 398297 [details]
Patch
Comment 6 EWS 2020-05-02 20:30:01 PDT 
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).
Comment 7 Simon Fraser (smfr) 2020-05-02 20:49:40 PDT 
https://trac.webkit.org/changeset/261056/webkit
Comment 8 Radar WebKit Bug Importer 2020-05-02 20:50:21 PDT 
<rdar://problem/62782371>
Comment 9 Simon Fraser (smfr) 2020-05-08 16:06:45 PDT 
*** Bug 199323 has been marked as a duplicate of this bug. ***