[webkit-reviews] review denied: [Bug 102525] [Chromium] Remove the dependency of INPUT_TYPE_COLOR on PAGE_POPUP : [Attachment 174946] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura <tkent at chromium.org> has denied Miguel Garcia
<miguelg at chromium.org>'s request for review:
Bug 102525: [Chromium] Remove the dependency of INPUT_TYPE_COLOR on PAGE_POPUP
https://bugs.webkit.org/show_bug.cgi?id=102525

Attachment 174946: Patch
https://bugs.webkit.org/attachment.cgi?id=174946&action=review

------- Additional Comments from Kent Tamura <tkent at chromium.org>
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:".