Bug 66658 introduced a bug where the onchange event is not fired when changing color from the color chooser.
Created attachment 104997 [details] patch
Comment on attachment 104997 [details] patch Attachment 104997 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9487102
Comment on attachment 104997 [details] patch Attachment 104997 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9490042
Comment on attachment 104997 [details] patch Attachment 104997 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9481944
Comment on attachment 104997 [details] patch Attachment 104997 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/9487106
Comment on attachment 104997 [details] patch Attachment 104997 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/9489084
Created attachment 105115 [details] fixed typo and wrapped internals.idl with flag
Comment on attachment 105115 [details] fixed typo and wrapped internals.idl with flag View in context: https://bugs.webkit.org/attachment.cgi?id=105115&action=review > Source/WebCore/html/HTMLInputElement.h:240 > + // For test purpose. > + bool connectToColorChooser(); Can you remove this or move this to WebCore/testing/? The purpose of WebCore/testing/ is to avoid such test-only code. > Source/WebCore/testing/Internals.cpp:172 > + HTMLInputElement* inputElement = element->toInputElement(); > + if (!inputElement || !inputElement->connectToColorChooser()) Who disconnects the ColorChooserClient? Does ColorChooser have a stale pointer to a ColorChooserClient after the ColorChooserClient is deleted?
(In reply to comment #8) > (From update of attachment 105115 [details](apply) [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=105115&action=review > > > Source/WebCore/html/HTMLInputElement.h:240 > > + // For test purpose. > > + bool connectToColorChooser(); > > Can you remove this or move this to WebCore/testing/? The purpose of WebCore/testing/ is to avoid such test-only code. To do this we would need to make HTMLInputElement::inputType() public. The m_inputType is contained inside HTMLInputElement so I believe this is my only option. > > Source/WebCore/testing/Internals.cpp:172 > > + HTMLInputElement* inputElement = element->toInputElement(); > > + if (!inputElement || !inputElement->connectToColorChooser()) > > Who disconnects the ColorChooserClient? Does ColorChooser have a stale pointer to a ColorChooserClient after the ColorChooserClient is deleted? The destructor for ColorChooserClient disconnects the color chooser client, so that won't happen. http://trac.webkit.org/browser/trunk/Source/WebCore/platform/ColorChooser.cpp#L41
Comment on attachment 105115 [details] fixed typo and wrapped internals.idl with flag 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.
Created attachment 105298 [details] fixed patch
Comment on attachment 105115 [details] fixed typo and wrapped internals.idl with flag View in context: https://bugs.webkit.org/attachment.cgi?id=105115&action=review I also changed Internals.idl because I noticed #if ENABLE(INPUT_COLOR) was giving errors and it was only working because of the old generated code. >> Source/WebCore/WebCore.exp.in(edit):1917 >> +__ZN7WebCore5ColorC1ERKN3WTF6StringE > > Why the blank line? Please don’t add that blank line. Done. >> Source/WebCore/html/ColorInputType.h(edit):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. I see! Done. >> Source/WebCore/html/HTMLInputElement.cpp(edit):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. Done. >> Source/WebCore/html/HTMLInputElement.h(edit):114 >> +#endif > > Please don’t add this function, and if you do add it, don’t make it virtual. Done. >> Source/WebCore/testing/Internals.cpp(edit):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. Done. >> Source/WebCore/testing/Internals.h(edit):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. Done.
Comment on attachment 105298 [details] fixed patch It seems all of Darin's comments were addressed in the updated patch.
Comment on attachment 105298 [details] fixed patch View in context: https://bugs.webkit.org/attachment.cgi?id=105298&action=review > Source/WebCore/html/HTMLInputElement.h:235 > + // For test purpose. This should be "test purposes". > Source/WebCore/testing/Internals.cpp:177 > + if (!element->hasTagName(HTMLNames::inputTag)) > + return false; > + HTMLInputElement* inputElement = element->toInputElement(); > + if (!inputElement) > + return false; > + return inputElement->connectToColorChooser(); Just for the record, I was wrong about toInputElement. It does the check for you. But since we have another bug about getting rid of toInputElement altogether, this will get fixed, so we’re OK if you land the patch as-is. The correct style for future code would be: if (!element->hasTagName(HTMLNames::inputTag)) return false; return static_cast<HTMLInputElement*>(element)->connectToColorChooser();
Created attachment 105746 [details] fixed comment & rebased
(In reply to comment #14) > (From update of attachment 105298 [details](apply) [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=105298&action=review > > > Source/WebCore/html/HTMLInputElement.h:235 > > + // For test purpose. > > This should be "test purposes". Done. > Just for the record, I was wrong about toInputElement. It does the check for you. But since we have another bug about getting rid of toInputElement altogether, this will get fixed, so we’re OK if you land the patch as-is. The correct style for future code would be: > > if (!element->hasTagName(HTMLNames::inputTag)) > return false; > return static_cast<HTMLInputElement*>(element)->connectToColorChooser(); Okay. I've left it as it was.
I've left it as it is.
Created attachment 105749 [details] filled out reviewer in changelog
Comment on attachment 105749 [details] filled out reviewer in changelog Clearing flags on attachment: 105749 Committed r94158: <http://trac.webkit.org/changeset/94158>
All reviewed patches have been landed. Closing bug.