Summary: | [Chromium] Remove the dependency of INPUT_TYPE_COLOR on PAGE_POPUP | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Miguel Garcia <miguelg> | ||||||||||||||||
Component: | New Bugs | Assignee: | Miguel Garcia <miguelg> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, peter, tkent, webkit.review.bot | ||||||||||||||||
Priority: | P2 | ||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||
Hardware: | All | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Miguel Garcia
2012-11-16 08:54:14 PST
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. |