[webkit-reviews] review denied: [Bug 91907] Implement setRangeText() on text controls : [Attachment 168308] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Oct 14 19:21:28 PDT 2012


Kent Tamura <tkent at chromium.org> has denied Pablo Flouret
<pablof at motorola.com>'s request for review:
Bug 91907: Implement setRangeText() on text controls
https://bugs.webkit.org/show_bug.cgi?id=91907

Attachment 168308: Patch
https://bugs.webkit.org/attachment.cgi?id=168308&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=168308&action=review


> Source/WebCore/html/HTMLInputElement.cpp:1882
> +    if (!m_inputType->isTextType() || m_inputType->isEmailField()) {

Please do not add type checking code in HTMLInputElement.  We should add
InputType::supportsSelectionAPI().

> Source/WebCore/html/HTMLTextAreaElement.h:65
> +    virtual void setRangeText(const String& replacement, ExceptionCode& ec)
OVERRIDE { HTMLTextFormControlElement::setRangeText(replacement, ec); }
> +    virtual void setRangeText(const String& replacement, unsigned start,
unsigned end, const String& selectionMode, ExceptionCode& ec) OVERRIDE {
HTMLTextFormControlElement::setRangeText(replacement, start, end,
selectionMode, ec); }
> +

They look unnecessary.

> Source/WebCore/html/HTMLTextFormControlElement.cpp:252
> +    start = min(start, textLength);
> +    end = min(end, textLength);

They should be std::min.
http://www.webkit.org/coding/coding-style.html#using-in-cpp

> LayoutTests/fast/forms/text-control-setrangetext.html:207
> +runTestsShouldFail("input", { type: "submit" });

We need to test other types; date, datetime, datetime-local, month, time, and
week.
Their support status depend on platforms. So we should prepare test files for
each of types and put them into the type directories.
e.g.
fast/forms/resource/common-setrangetext.js
fast/forms/text/text-setrangetext.html
fast/forms/search/search-setrangetext.html
fast/forms/input-button/input-button-setrangetext.html
fast/forms/file/file-setrangetext.html
fast/forms/date/date-setrangetext.html
....


More information about the webkit-reviews mailing list