Bug 102525 – [Chromium] Remove the dependency of INPUT_TYPE_COLOR on PAGE_POPUP

• Component: New Bugs
• Status: RESOLVED FIXED

[Miguel Garcia]

Remove the dependency of INPUT_TYPE_COLOR on PAGE_POPUP

[Miguel Garcia]

Created attachment 174702 [details]
Patch

[WebKit Review Bot]

Comment on attachment 174702 [details]
Patch

Attachment 174702 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14860341

[Miguel Garcia]

Created attachment 174938 [details]
Patch

[WebKit Review Bot]

Comment on attachment 174938 [details]
Patch

Attachment 174938 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14911153

[Miguel Garcia]

Created attachment 174939 [details]
Patch

[Miguel Garcia]

Created attachment 174946 [details]
Patch

[Miguel Garcia]

Adding a few folks from the discussion on 

https://bugs.webkit.org/show_bug.cgi?id=102404

[Kent Tamura]

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:".

[Miguel Garcia]

Created attachment 174982 [details]
Patch

[Miguel Garcia]

Created attachment 174983 [details]
Patch

[Miguel Garcia]

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

[Miguel Garcia]

Created attachment 174986 [details]
Patch

[Miguel Garcia]

Thanks for the review Kent! I addressed your comments I think it's ready for another look.

[Kent Tamura]

Comment on attachment 174986 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 174986 [details]
Patch

Clearing flags on attachment: 174986

Committed r135197: <http://trac.webkit.org/changeset/135197>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.