[webkit-reviews] review granted: [Bug 196636] AX: Support API: accessibilityReplaceRange:withText : [Attachment 366835] patch

Mon Apr 8 12:55:28 PDT 2019

Daniel Bates <dbates at webkit.org> has granted chris fleizach
<cfleizach at apple.com>'s request for review:
Bug 196636: AX: Support API: accessibilityReplaceRange:withText
https://bugs.webkit.org/show_bug.cgi?id=196636

Attachment 366835: patch

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

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

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:2287
> +    if (auto frame = renderer()->document().frame()) {

auto* please

I could have sworn RnederObject has a frame() convenience method instead of
deref document.

> Source/WebCore/accessibility/AccessibilityObject.cpp:2296
> +	   downcast<HTMLInputElement>(element).setRangeText(replacementString,
range.start, range.length, "");

Bad cast if element is HTMLTextAreaElement.

> Tools/DumpRenderTree/AccessibilityUIElement.cpp:811
> +    JSStringRef text = 0;

nullptr? Don't we have some smart pointer like class we can use to manage this
value. Pretty sure we have one. Can you take a look at other code that make use
of JSStringRef?

> Tools/DumpRenderTree/AccessibilityUIElement.cpp:820
> +    JSValueRef result = JSValueMakeBoolean(context,
toAXElement(thisObject)->replaceTextInRange(text, position, length));

Assuming this handles 0 for position/length.

> Tools/DumpRenderTree/AccessibilityUIElement.cpp:822
> +    if (text)
> +	   JSStringRelease(text);

Ok as-is. Smart pointer would be preferred. If we don't have one then does
JSStringRelease() null check for us?

> LayoutTests/accessibility/mac/replace-text-with-range-expected.txt:2
> +HelloBlorg
> + 

Ok as-is. In my opinion this makes the test look a little messy. If I was
writing this test I would figure out a way to omit this.

> LayoutTests/accessibility/mac/replace-text-with-range.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

Old Doctype. Intentional? If not, use new hotness: <!DOCTYPE html> 😀

> LayoutTests/accessibility/mac/replace-text-with-range.html:4
> +<script src="../../resources/js-test-pre.js"></script>

New hotness is to just include js-test.js and omit -pre and -post.js files.