Remove the dependency of INPUT_TYPE_COLOR on PAGE_POPUP
Created attachment 174702 [details] Patch
Comment on attachment 174702 [details] Patch Attachment 174702 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14860341
Created attachment 174938 [details] Patch
Comment on attachment 174938 [details] Patch Attachment 174938 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14911153
Created attachment 174939 [details] Patch
Created attachment 174946 [details] Patch
Adding a few folks from the discussion on https://bugs.webkit.org/show_bug.cgi?id=102404
Comment on attachment 174946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174946&action=review > Source/WebKit/chromium/src/ChromeClientImpl.cpp:41 > +#if ENABLE(PAGE_POPUP) > +#include "ColorChooserPopupUIController.h" nit: I prefer NOT wrapping a header inclusion with #if-#endif. Other reviewers might have a different preference. > Source/WebKit/chromium/src/ChromeClientImpl.cpp:692 > + ColorChooserUIController* controller; > +#if ENABLE(PAGE_POPUP) > + controller = new ColorChooserPopupUIController(this, chooserClient); > +#else > + controller = new ColorChooserUIController(this, chooserClient); Do not use a raw pointer here. OwnPtr<ColorChooserUIController> controller; controller = adoptPtr(new ...); controller->openUI(); return controller.release(); > Source/WebKit/chromium/src/ColorChooserPopupUIController.cpp:30 > + nit: blank line is not needed. > Source/WebKit/chromium/src/ColorChooserPopupUIController.cpp:40 > + ditto. > Source/WebKit/chromium/src/ColorChooserPopupUIController.h:29 > + ditto. > Source/WebKit/chromium/src/ColorChooserPopupUIController.h:32 > + ditto. > Source/WebKit/chromium/src/ColorChooserPopupUIController.h:41 > +namespace WebKit { > +class ColorChooserPopupUIController : public ColorChooserUIController, public WebCore::PagePopupClient { nit: add a blank line before "class" > Source/WebKit/chromium/src/ColorChooserPopupUIController.h:63 > + void closePopup(); > + ChromeClientImpl* m_chromeClient; nit: add a blank line between functions and data members. > Source/WebKit/chromium/src/ColorChooserUIController.h:63 > + OwnPtr<WebColorChooser> m_chooser; > +private: nit: add a blank line before "private:".
Created attachment 174982 [details] Patch
Created attachment 174983 [details] Patch
Comment on attachment 174946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174946&action=review >> Source/WebKit/chromium/src/ChromeClientImpl.cpp:41 >> +#include "ColorChooserPopupUIController.h" > > nit: I prefer NOT wrapping a header inclusion with #if-#endif. Other reviewers might have a different preference. I'd rather leave it as is if you are ok since this file already has #if - #endif for other headers. >> Source/WebKit/chromium/src/ChromeClientImpl.cpp:692 >> + controller = new ColorChooserUIController(this, chooserClient); > > Do not use a raw pointer here. > > OwnPtr<ColorChooserUIController> controller; > controller = adoptPtr(new ...); > controller->openUI(); > return controller.release(); Done >> Source/WebKit/chromium/src/ColorChooserPopupUIController.cpp:30 >> + > > nit: blank line is not needed. Done >> Source/WebKit/chromium/src/ColorChooserPopupUIController.cpp:40 >> + > > ditto. Done >> Source/WebKit/chromium/src/ColorChooserPopupUIController.h:29 >> + > > ditto. Done >> Source/WebKit/chromium/src/ColorChooserPopupUIController.h:32 >> + > > ditto. Done >> Source/WebKit/chromium/src/ColorChooserPopupUIController.h:41 >> +class ColorChooserPopupUIController : public ColorChooserUIController, public WebCore::PagePopupClient { > > nit: add a blank line before "class" Done >> Source/WebKit/chromium/src/ColorChooserPopupUIController.h:63 >> + ChromeClientImpl* m_chromeClient; > > nit: add a blank line between functions and data members. Done >> Source/WebKit/chromium/src/ColorChooserUIController.h:63 >> +private: > > nit: add a blank line before "private:". Done
Created attachment 174986 [details] Patch
Thanks for the review Kent! I addressed your comments I think it's ready for another look.
Comment on attachment 174986 [details] Patch ok
Comment on attachment 174986 [details] Patch Clearing flags on attachment: 174986 Committed r135197: <http://trac.webkit.org/changeset/135197>
All reviewed patches have been landed. Closing bug.