[webkit-reviews] review granted: [Bug 268920] AX: Adopt non-blinking cursor API : [Attachment 469856] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Feb 13 16:39:26 PST 2024


Tyler Wilcock <tyler_w at apple.com> has granted  review:
Bug 268920: AX: Adopt non-blinking cursor API
https://bugs.webkit.org/show_bug.cgi?id=268920

Attachment 469856: Patch

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




--- Comment #21 from Tyler Wilcock <tyler_w at apple.com> ---
Comment on attachment 469856
  --> https://bugs.webkit.org/attachment.cgi?id=469856
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=469856&action=review

> Source/WebCore/testing/Internals.cpp:1140
> +    Document* document = contextDocument();

auto*?

> Source/WebCore/testing/Internals.cpp:1905
> +ExceptionOr<bool> Internals::isCaretBlinkingSuspended(Document& document)
> +{
> +    if (!document.frame())
> +	   return Exception { ExceptionCode::InvalidAccessError };
> +
> +    return document.frame()->selection().isCaretBlinkingSuspended();
> +}

Can we change the implementation of Internals::isCaretBlinkingSuspended() to
call this method with `contextDocument()` so we aren't duplicating the logic?

> Source/WebCore/testing/Internals.idl:508
>      boolean isCaretBlinkingSuspended();
> +    boolean isCaretBlinkingSuspended(Document document);

Maybe we can add a comment explaining what document the parameter-less method
queries now that we have two methods with the same name?

> LayoutTests/accessibility/mac/prefers-non-blinking-cursor.html:15
> +let iframe = document.getElementById("iframe")

Can this iframe be const? Also it's missing a semi-colon at the end of the
statement, might be nice to consistent with all of our other statements which
end in semicolons.


More information about the webkit-reviews mailing list