[webkit-reviews] review denied: [Bug 60403] [HTML5] Implement the selectionDirection property on input and textarea : [Attachment 101191] fixed the test

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 60403: [HTML5] Implement the selectionDirection property on input and
textarea
https://bugs.webkit.org/show_bug.cgi?id=60403

Attachment 101191: fixed the test
https://bugs.webkit.org/attachment.cgi?id=101191&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=101191&action=review


Do you know why the Mac build failed?

review- because of the missing return statement in the JavaScript binding.

> Source/WebCore/bindings/js/JSHTMLInputElementCustom.cpp:76
> +    if (!input->canHaveSelection())
> +	   return throwTypeError(exec);

I don’t see tests for this.

> Source/WebCore/bindings/js/JSHTMLInputElementCustom.cpp:85
> +    if (!input->canHaveSelection())
> +	   throwTypeError(exec);

I don’t see tests for this.

There’s a missing return statement here, and we need a test that would have
discovered it.

> Source/WebCore/bindings/v8/custom/V8HTMLInputElementCustom.cpp:101
> +    if (!imp->canHaveSelection())
> +	   return throwError("Accessing selectionDirection on an input element
that cannot have a selection.");

You are putting in a custom DOM message in the error object for V8, but not for
JavaScriptCore. That’s not good. We want the DOM bindings to be the same.

> Source/WebCore/html/HTMLTextFormControlElement.h:36
> +enum TextFieldSelectionDirection {SelectionHasNoDirection,
SelectionHasForwardDirection, SelectionHasBackwardDirection};

Missing spaces inside the braces.

> Source/WebCore/html/HTMLTextFormControlElement.h:58
> +    String selectionDirection() const;

This could return a const String& rather than a String. Might also be slightly
better to return a const AtomicString&; the cost to atomize would be a one-time
cost in the directionString function.