[webkit-reviews] review denied: [Bug 66848] input color: onchange event is not fired when changing color from color chooser : [Attachment 105115] fixed typo and wrapped internals.idl with flag

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Keishi Hattori <keishi at webkit.org>'s
request for review:
Bug 66848: input color: onchange event is not fired when changing color from
color chooser
https://bugs.webkit.org/show_bug.cgi?id=66848

Attachment 105115: fixed typo and wrapped internals.idl with flag
https://bugs.webkit.org/attachment.cgi?id=105115&action=review

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


Looks good. A few problems.

> Source/WebCore/WebCore.exp.in:1917
>  __ZNK7WebCore12ColorChooser13colorSelectedERKNS_5ColorE
> +
> +__ZN7WebCore5ColorC1ERKN3WTF6StringE

Why the blank line? Please don’t add that blank line.

> Source/WebCore/html/ColorInputType.h:46
> +    virtual bool isColorControl() const;

Why are we making this public? What code has a pointer to ColorInputType but
then has to call isColorControl on it? Why not just assume it’s true in that
case?

I think you may have thought you’d need this public to call it on an
InputType*, but that is not true. Please don’t make this change.

> Source/WebCore/html/HTMLInputElement.cpp:1901
> +bool HTMLInputElement::isColorControl() const

I see no reason to add this function. Please don’t add it. The one caller is
inside the input element class and so can call m_inputType->isColorControl()
directly.

> Source/WebCore/html/HTMLInputElement.h:114
> +#if ENABLE(INPUT_COLOR)
> +    virtual bool isColorControl() const;
> +#endif

Please don’t add this function, and if you do add it, don’t make it virtual.

> Source/WebCore/testing/Internals.cpp:171
> +    HTMLInputElement* inputElement = element->toInputElement();

A function like this should check if the element is an input element before
casting. The toInputElement function is only legal to call if you already have
checked that it’s an input element.

> Source/WebCore/testing/Internals.h:64
> +    void connectColorChooserClient(Element*, ExceptionCode&);

For testing purposes, I think it’s OK to have the function just silently fail.
It seems overkill to me to hook up a DOM exception for the test function.