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

[bugzilla-daemon at webkit.org]

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
https://bugs.webkit.org/show_bug.cgi?id=211342

Attachment 398292: Patch

https://bugs.webkit.org/attachment.cgi?id=398292&action=review




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