Support color chooser inside extesions, apps, chrome frame, dev tool

Support color chooser inside extesions, apps, chrome frame, dev tool

This introduces ColorChooserController for the code shared between the WebContentsDelegate.
This also solves the problem of multiple color chooser dialogs per window in Gtk.
We only want one color chooser open per app ever.

BUG=174112, 152923
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201522

[keishi, 2013-03-28 12:49:44]

ColorChooser wasn't working inside extesnions, apps, chrome frames, and dev tool
because I didn't implement OpenColorChooser method.

[sky, 2013-03-28 16:31:46]

sky->ben

[Ben Goodger (Google), 2013-03-29 14:56:49]

https://codereview.chromium.org/13150004/diff/1/chrome/browser/browser_process.h
File chrome/browser/browser_process.h (right):

https://codereview.chromium.org/13150004/diff/1/chrome/browser/browser_proces...
chrome/browser/browser_process.h:224: virtual ColorChooserController*
color_chooser_controller() = 0;
BrowserProcess is a Chrome aggregation type of doom.

Please consider the browser process deprecated as a rule of thumb, and try and
come up with other places to access your stuff.

https://codereview.chromium.org/13150004/diff/1/chrome/browser/ui/color_choos...
File chrome/browser/ui/color_chooser_controller.cc (right):

https://codereview.chromium.org/13150004/diff/1/chrome/browser/ui/color_choos...
chrome/browser/ui/color_chooser_controller.cc:30: if (color_chooser_.get())
It seems a little unusual to consider this case a "failure", since it seems like
it could be pretty common.

Why not return the existing color chooser?

https://codereview.chromium.org/13150004/diff/1/chrome/browser/ui/color_choos...
chrome/browser/ui/color_chooser_controller.cc:39: return color_chooser_.get();
It does seem like unnecessary indirection to have this class at all. Can the
ColorChooser just delete itself once it's done?

[keishi, 2013-04-01 03:47:21]

On 2013/03/29 14:56:49, Ben Goodger (Google) wrote:
>
https://codereview.chromium.org/13150004/diff/1/chrome/browser/browser_process.h
> File chrome/browser/browser_process.h (right):
> 
>
https://codereview.chromium.org/13150004/diff/1/chrome/browser/browser_proces...
> chrome/browser/browser_process.h:224: virtual ColorChooserController*
> color_chooser_controller() = 0;
> BrowserProcess is a Chrome aggregation type of doom.
> 
> Please consider the browser process deprecated as a rule of thumb, and try and
> come up with other places to access your stuff.

I've been trying to find the best place but I have no idea. Could you suggest an
alternative?
We only want one ColorChooser open at a time. i.e. we only need one ColorChooser
instance per app.
If a new window/tab requests to open the ColorChooser, the old ColorChooser
should close and a new one should open.

>
https://codereview.chromium.org/13150004/diff/1/chrome/browser/ui/color_choos...
> File chrome/browser/ui/color_chooser_controller.cc (right):
> 
>
https://codereview.chromium.org/13150004/diff/1/chrome/browser/ui/color_choos...
> chrome/browser/ui/color_chooser_controller.cc:30: if (color_chooser_.get())
> It seems a little unusual to consider this case a "failure", since it seems
like
> it could be pretty common.
> 
> Why not return the existing color chooser?
That could be done.

>
https://codereview.chromium.org/13150004/diff/1/chrome/browser/ui/color_choos...
> chrome/browser/ui/color_chooser_controller.cc:39: return color_chooser_.get();
> It does seem like unnecessary indirection to have this class at all. Can the
> ColorChooser just delete itself once it's done?
ColorChooserController exists mainly to share the code in OpenColorChooser with
the WebContentDelegates. I could make it a subclass of
ColorChooserWin/Mac/Gtk/Aura.

[Ben Goodger (Google), 2013-04-01 16:28:18]

Seems like if you have this ColorChooserController, it can ensure there's
only one open, return the existing one if it's open (perhaps activating
it), and when that instance is closed clean up its instance. No need to add
to browser process. When you add to browser process you're also keeping
this thing live for the lifetime of the app, which seems excessive given
the inherent transient nature of color selection.

-Ben


On Sun, Mar 31, 2013 at 8:47 PM, <keishi@chromium.org> wrote:

> On 2013/03/29 14:56:49, Ben Goodger (Google) wrote:
>
> https://codereview.chromium.**org/13150004/diff/1/chrome/**
>
browser/browser_process.h<https://codereview.chromium.org/13150004/diff/1/chrome/browser/browser_process.h>
>
>> File chrome/browser/browser_**process.h (right):
>>
>
>
> https://codereview.chromium.**org/13150004/diff/1/chrome/**
>
browser/browser_process.h#**newcode224<https://codereview.chromium.org/13150004/diff/1/chrome/browser/browser_process.h#newcode224>
>
>> chrome/browser/browser_**process.h:224: virtual ColorChooserController*
>> color_chooser_controller() = 0;
>> BrowserProcess is a Chrome aggregation type of doom.
>>
>
>  Please consider the browser process deprecated as a rule of thumb, and
>> try and
>> come up with other places to access your stuff.
>>
>
> I've been trying to find the best place but I have no idea. Could you
> suggest an
> alternative?
> We only want one ColorChooser open at a time. i.e. we only need one
> ColorChooser
> instance per app.
> If a new window/tab requests to open the ColorChooser, the old ColorChooser
> should close and a new one should open.
>
>
>
> https://codereview.chromium.**org/13150004/diff/1/chrome/**
>
browser/ui/color_chooser_**controller.cc<https://codereview.chromium.org/13150004/diff/1/chrome/browser/ui/color_chooser_controller.cc>
>
>> File chrome/browser/ui/color_**chooser_controller.cc (right):
>>
>
>
> https://codereview.chromium.**org/13150004/diff/1/chrome/**
>
browser/ui/color_chooser_**controller.cc#newcode30<https://codereview.chromium.org/13150004/diff/1/chrome/browser/ui/color_chooser_controller.cc#newcode30>
>
>> chrome/browser/ui/color_**chooser_controller.cc:30: if
>> (color_chooser_.get())
>> It seems a little unusual to consider this case a "failure", since it
>> seems
>>
> like
>
>> it could be pretty common.
>>
>
>  Why not return the existing color chooser?
>>
> That could be done.
>
>
>
> https://codereview.chromium.**org/13150004/diff/1/chrome/**
>
browser/ui/color_chooser_**controller.cc#newcode39<https://codereview.chromium.org/13150004/diff/1/chrome/browser/ui/color_chooser_controller.cc#newcode39>
>
>> chrome/browser/ui/color_**chooser_controller.cc:39: return
>> color_chooser_.get();
>> It does seem like unnecessary indirection to have this class at all. Can
>> the
>> ColorChooser just delete itself once it's done?
>>
> ColorChooserController exists mainly to share the code in OpenColorChooser
> with
> the WebContentDelegates. I could make it a subclass of
> ColorChooserWin/Mac/Gtk/Aura.
>
>
https://codereview.chromium.**org/13150004/<https://codereview.chromium.org/1...
>

[keishi, 2013-04-02 06:24:55]

On 2013/04/01 16:28:18, Ben Goodger (Google) wrote:
> Seems like if you have this ColorChooserController, it can ensure there's
> only one open, return the existing one if it's open (perhaps activating
> it), and when that instance is closed clean up its instance. No need to add
> to browser process. When you add to browser process you're also keeping
> this thing live for the lifetime of the app, which seems excessive given
> the inherent transient nature of color selection.
> 
> -Ben
Yes, ColorChooserController ensures only ColorChooser instance per
ColorChooserController.
But how do I ensure one ColorChooserController per app?
My understanding is that I am not allowed to make ColorChooserController into a
Singleton. Am I allowed to make ColorChooserController::color_chooser_ into a
static class member?

[Ben Goodger (Google), 2013-04-02 20:29:19]

Sure we often do that.

-Ben


On Mon, Apr 1, 2013 at 11:24 PM, <keishi@chromium.org> wrote:

> On 2013/04/01 16:28:18, Ben Goodger (Google) wrote:
>
>> Seems like if you have this ColorChooserController, it can ensure there's
>> only one open, return the existing one if it's open (perhaps activating
>> it), and when that instance is closed clean up its instance. No need to
>> add
>> to browser process. When you add to browser process you're also keeping
>> this thing live for the lifetime of the app, which seems excessive given
>> the inherent transient nature of color selection.
>>
>
>  -Ben
>>
> Yes, ColorChooserController ensures only ColorChooser instance per
> ColorChooserController.
> But how do I ensure one ColorChooserController per app?
> My understanding is that I am not allowed to make ColorChooserController
> into a
> Singleton. Am I allowed to make ColorChooserController::color_**chooser_
> into a
> static class member?
>
>
>
https://codereview.chromium.**org/13150004/<https://codereview.chromium.org/1...
>

[keishi, 2013-04-12 11:44:54]

On 2013/04/02 20:29:19, Ben Goodger (Google) wrote:
> Sure we often do that.
> 
> -Ben
Hi beng@. could you take a look at the new patch.
Thanks.

[Ben Goodger (Google), 2013-04-16 16:20:16]

https://codereview.chromium.org/13150004/diff/10001/chrome/browser/ui/color_c...
File chrome/browser/ui/color_chooser.h (right):

https://codereview.chromium.org/13150004/diff/10001/chrome/browser/ui/color_c...
chrome/browser/ui/color_chooser.h:18: // Returns NULL on failure.
I left you a comment earlier about this. It's not clear why this method would
ever "fail". You should just refocus the existing one. For that matter, it's
probably not necessary to expose the current color chooser above. Just having
this single method that opens a new one or refocuses the existing should be
sufficient.

https://codereview.chromium.org/13150004/diff/10001/chrome/browser/ui/color_c...
chrome/browser/ui/color_chooser.h:22: explicit
ColorChooser(content::WebContents* web_contents);
ctor/dtor should be protected.

[keishi, 2013-04-17 10:49:48]

https://codereview.chromium.org/13150004/diff/10001/chrome/browser/ui/color_c...
File chrome/browser/ui/color_chooser.h (right):

https://codereview.chromium.org/13150004/diff/10001/chrome/browser/ui/color_c...
chrome/browser/ui/color_chooser.h:18: // Returns NULL on failure.
On 2013/04/16 16:20:17, Ben Goodger (Google) wrote:
> I left you a comment earlier about this. It's not clear why this method would
> ever "fail". You should just refocus the existing one. For that matter, it's
> probably not necessary to expose the current color chooser above. Just having
> this single method that opens a new one or refocuses the existing should be
> sufficient.

I'm sorry I didn't understand what you were getting at earlier but you are
right, I don't need to return NULL.

https://codereview.chromium.org/13150004/diff/10001/chrome/browser/ui/color_c...
chrome/browser/ui/color_chooser.h:22: explicit
ColorChooser(content::WebContents* web_contents);
On 2013/04/16 16:20:17, Ben Goodger (Google) wrote:
> ctor/dtor should be protected.

Done.

[Ben Goodger (Google), 2013-04-17 15:34:14]

https://codereview.chromium.org/13150004/diff/16001/chrome/browser/ui/color_c...
File chrome/browser/ui/color_chooser.cc (right):

https://codereview.chromium.org/13150004/diff/16001/chrome/browser/ui/color_c...
chrome/browser/ui/color_chooser.cc:21: void ColorChooser::DidChooseColor(SkColor
color) {
... but looking at how little this class does makes me believe that it's not
essential at all and is just extra complexity in the class structure.

Just have the view manage its instance itself, and have the view implement
content::ColorChooser directly and manage its own instance.

https://codereview.chromium.org/13150004/diff/16001/chrome/browser/ui/color_c...
File chrome/browser/ui/color_chooser.h (right):

https://codereview.chromium.org/13150004/diff/16001/chrome/browser/ui/color_c...
chrome/browser/ui/color_chooser.h:14: static ColorChooser*
get_current_color_chooser() {
you don't need this. having current_color_chooser_ as a protected static
instance should be enough...

[keishi, 2013-04-19 14:40:22]

On 2013/04/17 15:34:14, Ben Goodger (Google) wrote:
>
https://codereview.chromium.org/13150004/diff/16001/chrome/browser/ui/color_c...
> File chrome/browser/ui/color_chooser.cc (right):
> 
>
https://codereview.chromium.org/13150004/diff/16001/chrome/browser/ui/color_c...
> chrome/browser/ui/color_chooser.cc:21: void
ColorChooser::DidChooseColor(SkColor
> color) {
> ... but looking at how little this class does makes me believe that it's not
> essential at all and is just extra complexity in the class structure.
> 
> Just have the view manage its instance itself, and have the view implement
> content::ColorChooser directly and manage its own instance.

I added chrome/browser/ui/color_chooser to share the code in the ColorChooser*
classes.

I've uploaded a new patch that removes it.

[Ben Goodger (Google), 2013-04-19 16:24:54]

https://codereview.chromium.org/13150004/diff/89001/components/web_contents_d...
File components/web_contents_delegate_android/color_chooser_android.h (right):

https://codereview.chromium.org/13150004/diff/89001/components/web_contents_d...
components/web_contents_delegate_android/color_chooser_android.h:36:
content::WebContents* web_contents;
web_contents_

https://codereview.chromium.org/13150004/diff/89001/components/web_contents_d...
File components/web_contents_delegate_android/web_contents_delegate_android.h
(right):

https://codereview.chromium.org/13150004/diff/89001/components/web_contents_d...
components/web_contents_delegate_android/web_contents_delegate_android.h:55: 
delete nl

https://codereview.chromium.org/13150004/diff/89001/content/public/browser/co...
File content/public/browser/color_chooser.h (right):

https://codereview.chromium.org/13150004/diff/89001/content/public/browser/co...
content/public/browser/color_chooser.h:17: static ColorChooser*
Open(content::WebContents* web_contents,
you shouldn't put this method on content api. since you implement in chrome it
means you can't use content in another library without a link error. this method
should live in chrome.

[keishi, 2013-04-22 01:54:29]

https://codereview.chromium.org/13150004/diff/89001/components/web_contents_d...
File components/web_contents_delegate_android/color_chooser_android.h (right):

https://codereview.chromium.org/13150004/diff/89001/components/web_contents_d...
components/web_contents_delegate_android/color_chooser_android.h:36:
content::WebContents* web_contents;
On 2013/04/19 16:24:54, Ben Goodger (Google) wrote:
> web_contents_

Done.

https://codereview.chromium.org/13150004/diff/89001/components/web_contents_d...
File components/web_contents_delegate_android/web_contents_delegate_android.h
(right):

https://codereview.chromium.org/13150004/diff/89001/components/web_contents_d...
components/web_contents_delegate_android/web_contents_delegate_android.h:55: 
On 2013/04/19 16:24:54, Ben Goodger (Google) wrote:
> delete nl

Done.

https://codereview.chromium.org/13150004/diff/89001/content/public/browser/co...
File content/public/browser/color_chooser.h (right):

https://codereview.chromium.org/13150004/diff/89001/content/public/browser/co...
content/public/browser/color_chooser.h:17: static ColorChooser*
Open(content::WebContents* web_contents,
On 2013/04/19 16:24:54, Ben Goodger (Google) wrote:
> you shouldn't put this method on content api. since you implement in chrome it
> means you can't use content in another library without a link error. this
method
> should live in chrome.

I moved it to chrome/browser/ui/color_chooser_util.h

[keishi, 2013-04-24 14:21:32]

ben@ could you take a look at the new patch? Thanks.

[keishi, 2013-05-10 12:12:26]

ben@ could you please review the new patch. Thanks.

[Ben Goodger (Google), 2013-05-20 16:14:30]

https://codereview.chromium.org/13150004/diff/103001/chrome/browser/ui/color_...
File chrome/browser/ui/color_chooser_util.h (right):

https://codereview.chromium.org/13150004/diff/103001/chrome/browser/ui/color_...
chrome/browser/ui/color_chooser_util.h:15: content::ColorChooser*
OpenColorChooser(content::WebContents* web_contents,
1. put this in browser/ui/browser_dialogs.h instead of making a new file.
2. rename this method ShowColorChooser(..) like the other methods in
browser_dialogs. This will make it so you won't have a function name that
collides with the WCD method.

[tfarina, 2013-05-20 16:45:52]

https://codereview.chromium.org/13150004/diff/103001/chrome/browser/ui/color_...
File chrome/browser/ui/color_chooser_util.h (right):

https://codereview.chromium.org/13150004/diff/103001/chrome/browser/ui/color_...
chrome/browser/ui/color_chooser_util.h:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
Copyright 2013

https://codereview.chromium.org/13150004/diff/103001/chrome/browser/ui/color_...
chrome/browser/ui/color_chooser_util.h:5: #ifndef
CHROME_BROWSER_UI_COLOR_CHOOSER_UTIL_H_
I'd name this file color_chooser_dialog.h

https://codereview.chromium.org/13150004/diff/103001/chrome/browser/ui/color_...
chrome/browser/ui/color_chooser_util.h:12: class ColorChooser;
sort

https://codereview.chromium.org/13150004/diff/103001/chrome/browser/ui/color_...
chrome/browser/ui/color_chooser_util.h:15: content::ColorChooser*
OpenColorChooser(content::WebContents* web_contents,
I'd also put this in chrome namespace. I don't recall if browser_dialogs.h is
already in that namespace.

[keishi, 2013-05-21 16:48:05]

https://codereview.chromium.org/13150004/diff/103001/chrome/browser/ui/color_...
File chrome/browser/ui/color_chooser_util.h (right):

https://codereview.chromium.org/13150004/diff/103001/chrome/browser/ui/color_...
chrome/browser/ui/color_chooser_util.h:15: content::ColorChooser*
OpenColorChooser(content::WebContents* web_contents,
On 2013/05/20 16:14:30, Ben Goodger (Google) wrote:
> 1. put this in browser/ui/browser_dialogs.h instead of making a new file.
> 2. rename this method ShowColorChooser(..) like the other methods in
> browser_dialogs. This will make it so you won't have a function name that
> collides with the WCD method.

Done. Thanks, this looks like the perfect place to put it.

[Ben Goodger (Google), 2013-05-21 16:49:57]

lgtm

[commit-bot: I haz the power, 2013-05-21 18:09:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keishi@chromium.org/13150004/110001

[commit-bot: I haz the power, 2013-05-21 18:44:40]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-21 23:38:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/keishi@chromium.org/13150004/134001

[commit-bot: I haz the power, 2013-05-22 15:16:23]

Change committed as 201522