[webkit-reviews] review granted: [Bug 211342] Make it possible to test overlay scrollbar interactions : [Attachment 398292] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat May 2 13:39:54 PDT 2020

Daniel Bates <dbates at webkit.org> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 211342: Make it possible to test overlay scrollbar interactions

Attachment 398292: Patch


--- Comment #2 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 398292
  --> https://bugs.webkit.org/attachment.cgi?id=398292

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


> LayoutTests/fast/scrolling/mac/scrollbars/overlay-scrollbar-hovered.html:28
> +	       debug('Hovering vertical scrollbar should show expanded

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.

More information about the webkit-reviews mailing list