[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