[webkit-reviews] review denied: [Bug 61857] Value of a range input does not update visually when using setAttribute : [Attachment 95592] patch

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Keishi Hattori
<keishi at webkit.org>'s request for review:
Bug 61857: Value of a range input does not update visually when using
setAttribute
https://bugs.webkit.org/show_bug.cgi?id=61857

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

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=95592&action=review

> LayoutTests/fast/forms/script-tests/range-set-attribute.js:1
> +description('Test to see if setting the value attribute updates the
value.');

Please move this code into range-set-attribute.html. There is no need to create
separate files and make tests harder to read.

> Source/WebCore/html/HTMLInputElement.cpp:979
> +void HTMLInputElement::setValue(const String& value, bool sendChangeEvent,
bool onlyIfDirty)

Boolean parameters are bad, bad, bad. That's right: three times bad. At the
callsite, we have no idea what is happening. If you are going to modify this
function signature, I would convert both parameters into enums as cleanup.

> Source/WebCore/html/HTMLInputElement.h:137
> +    void setValue(const String&, bool sendChangeEvent = false, bool
onlyIfDirty = false);

onlyIfDirty is confusing to me. What's clean? What's dirty? It's a flag of some
sort, but I don't understand why it is here. The ChangeLog doesn't explain it
either.