Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
9203001:
Implement input type=color UI (Closed)
|
Created:
8 years, 11 months ago by keishi Modified:
8 years, 9 months ago Reviewers:
Yaron, Peter Kasting, Elliot Glaysher, jam, Evan Stade, Nico, Robert Sesek, Ben Goodger (Google) CC:
chromium-reviews, Avi (use Gerrit), creis+watch_chromium.org, brettw-cc_chromium.org, dpranke-watch+content_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, ajwong+watch_chromium.org Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionImplement input type=color UI
BUG=92608
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=126889
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=127656
Patch Set 1 #Patch Set 2 : fixed bug #Patch Set 3 : fixed flag #Patch Set 4 : Removed ENABLE_INPUT_COLOR flag #Patch Set 5 : renamed method #
Total comments: 15
Patch Set 6 : removed OS ifdef and consolidated patches for all ports #Patch Set 7 : removed argument for endColorChooser #Patch Set 8 : fixed comment #
Total comments: 13
Patch Set 9 : fixed issues/removed color_select_helper/added color_chooser_id #
Total comments: 71
Patch Set 10 : Fixed issues #
Total comments: 47
Patch Set 11 : fixed #Patch Set 12 : added content::ColorChooser #
Total comments: 7
Patch Set 13 : fixed #
Total comments: 57
Patch Set 14 : fixed #
Total comments: 32
Patch Set 15 : fixed #
Total comments: 8
Patch Set 16 : fixed mac nits #
Total comments: 10
Patch Set 17 : fixed nits #
Total comments: 2
Patch Set 18 : fixed #Patch Set 19 : rebased #
Total comments: 4
Patch Set 20 : fixed nits #Patch Set 21 : Added stubs for android and aura #
Total comments: 4
Patch Set 22 : use g_object_get #Patch Set 23 : rebased #Patch Set 24 : had to add init TabContent::color_chooser_ #Patch Set 25 : rebased #Messages
Total messages: 69 (0 generated)
This patch implements the common part for the color chooser.
You added 5 reviewers, please specify who you would like to review what.
On 2012/01/16 02:50:40, brettw wrote: > You added 5 reviewers, please specify who you would like to review what. Sorry. This is what I had in mind. Please remove if I specified wrong or too many. ben@chromium.org, avi@chromium.org chrome/browser/ui/color_chooser.h (http://codereview.chromium.org/9203001/diff/4001/chrome/browser/ui/color_choo...) chrome/browser/ui/color_chooser.cc (http://codereview.chromium.org/9203001/diff/4001/chrome/browser/ui/color_choo...) chrome/browser/color_select_observer.h (http://codereview.chromium.org/9203001/diff/4001/chrome/browser/color_select_...) chrome/browser/color_select_observer.cc (http://codereview.chromium.org/9203001/diff/4001/chrome/browser/color_select_...) avi@chromium.org, pkasting@chromium.org chrome/browser/ui/tab_contents/tab_contents_wrapper.h (http://codereview.chromium.org/9203001/diff/4001/chrome/browser/ui/tab_conten...) chrome/browser/ui/tab_contents/tab_contents_wrapper.cc (http://codereview.chromium.org/9203001/diff/4001/chrome/browser/ui/tab_conten...) brettw@chromium.org, avi@chromium.org, darin@chromium.org content/browser/renderer_host/render_view_host.h (http://codereview.chromium.org/9203001/diff/4001/content/renderer_host/render...) content/browser/renderer_host/render_view_host.cc (http://codereview.chromium.org/9203001/diff/4001/content/renderer_host/render...) content/renderer/render_view_impl.h (http://codereview.chromium.org/9203001/diff/4001/content/renderer/render_view...) content/renderer/render_view_impl.cc (http://codereview.chromium.org/9203001/diff/4001/content/renderer/render_view...) content/renderer/renderer_webcolorchooser_impl.h (http://codereview.chromium.org/9203001/diff/4001/content/renderer/renderer_we...) content/renderer/renderer_webcolorchooser_impl.cc (http://codereview.chromium.org/9203001/diff/4001/content/renderer/renderer_we...)
I'm on vacation; you will want to pick a different reviewer. John can do content; thakis can do Mac if needed. On Sunday, January 15, 2012, wrote: > On 2012/01/16 02:50:40, brettw wrote: > >> You added 5 reviewers, please specify who you would like to review what. >> > > Sorry. This is what I had in mind. Please remove if I specified wrong or > too > many. > > ben@chromium.org, avi@chromium.org > chrome/browser/ui/color_**chooser.h > (http://codereview.chromium.**org/9203001/diff/4001/chrome/** > browser/ui/color_chooser.h<http://codereview.chromium.org/9203001/diff/4001/chrome/browser/ui/color_chooser.h> > ) > chrome/browser/ui/color_**chooser.cc > (http://codereview.chromium.**org/9203001/diff/4001/chrome/** > browser/ui/color_chooser.cc<http://codereview.chromium.org/9203001/diff/4001/chrome/browser/ui/color_chooser.cc> > ) > chrome/browser/color_select_**observer.h > (http://codereview.chromium.**org/9203001/diff/4001/chrome/** > browser/color_select_observer.**h<http://codereview.chromium.org/9203001/diff/4001/chrome/browser/color_select_observer.h> > ) > chrome/browser/color_select_**observer.cc > (http://codereview.chromium.**org/9203001/diff/4001/chrome/** > browser/color_select_observer.**cc<http://codereview.chromium.org/9203001/diff/4001/chrome/browser/color_select_observer.cc> > ) > > avi@chromium.org, pkasting@chromium.org > chrome/browser/ui/tab_**contents/tab_contents_wrapper.**h > (http://codereview.chromium.**org/9203001/diff/4001/chrome/** > browser/ui/tab_contents/tab_**contents_wrapper.h<http://codereview.chromium.org/9203001/diff/4001/chrome/browser/ui/tab_contents/tab_contents_wrapper.h> > ) > chrome/browser/ui/tab_**contents/tab_contents_wrapper.**cc > (http://codereview.chromium.**org/9203001/diff/4001/chrome/** > browser/ui/tab_contents/tab_**contents_wrapper.cc<http://codereview.chromium.org/9203001/diff/4001/chrome/browser/ui/tab_contents/tab_contents_wrapper.cc> > ) > > brettw@chromium.org, avi@chromium.org, darin@chromium.org > content/browser/renderer_host/**render_view_host.h > (http://codereview.chromium.**org/9203001/diff/4001/content/** > renderer_host/render_view_**host.h<http://codereview.chromium.org/9203001/diff/4001/content/renderer_host/render_view_host.h> > ) > content/browser/renderer_host/**render_view_host.cc > (http://codereview.chromium.**org/9203001/diff/4001/content/** > renderer_host/render_view_**host.h<http://codereview.chromium.org/9203001/diff/4001/content/renderer_host/render_view_host.h> > ) > content/renderer/render_view_**impl.h > (http://codereview.chromium.**org/9203001/diff/4001/content/** > renderer/render_view_impl.h<http://codereview.chromium.org/9203001/diff/4001/content/renderer/render_view_impl.h> > ) > content/renderer/render_view_**impl.cc > (http://codereview.chromium.**org/9203001/diff/4001/content/** > renderer/render_view_impl.cc<http://codereview.chromium.org/9203001/diff/4001/content/renderer/render_view_impl.cc> > ) > content/renderer/renderer_**webcolorchooser_impl.h > (http://codereview.chromium.**org/9203001/diff/4001/content/** > renderer/renderer_**webcolorchooser_impl.h<http://codereview.chromium.org/9203001/diff/4001/content/renderer/renderer_webcolorchooser_impl.h> > ) > content/renderer/renderer_**webcolorchooser_impl.cc > (http://codereview.chromium.**org/9203001/diff/4001/content/** > renderer/renderer_**webcolorchooser_impl.cc<http://codereview.chromium.org/9203001/diff/4001/content/renderer/renderer_webcolorchooser_impl.cc> > ) > > > > http://codereview.chromium.**org/9203001/<http://codereview.chromium.org/9203... >
On 2012/01/16 03:29:00, keishi1 wrote: > On 2012/01/16 02:50:40, brettw wrote: > > You added 5 reviewers, please specify who you would like to review what. > > Sorry. This is what I had in mind. Please remove if I specified wrong or too > many. Thanks for the clarification. Normally just one reviewer is assigned to any particular file. This makes it clear who has responsibility to get back to you. If you list multiple people for a file, you should make it clear whether you want review from all of them or merely any of them. Normally in the latter case I'd suggest just picking one person. You can always change it if, like Avi, your reviewer is on vacation or otherwise unavailable. For the two files you listed for me, my main question concern is that ENABLE_XXX is not how we build experimental features in Chromium -- this is a WebKit-ism. If possible, you should just build your feature on by default in Chromium (and if it needs to be invisible to users for a while, control that on the WebKit side). If you need to have this code not run on the Chromium side by default for a while, then use a command-line flag, possibly with an about:flags entry attached, to do run-time control over this rather than compile-time.
On 2012/01/16 20:19:22, Peter Kasting wrote: > Normally just one reviewer is assigned to any particular file. This makes it > clear who has responsibility to get back to you. If you list multiple people > for a file, you should make it clear whether you want review from all of them or > merely any of them. Normally in the latter case I'd suggest just picking one > person. You can always change it if, like Avi, your reviewer is on vacation or > otherwise unavailable. Okay. Picking one reviewer and removing others. pkasting@chromium.org chrome/browser/ui/color_chooser.h chrome/browser/color_select_observer.h chrome/browser/color_select_observer.cc chrome/browser/ui/tab_contents/tab_contents_wrapper.h chrome/browser/ui/tab_contents/tab_contents_wrapper.cc jam@chromium.org content/browser/renderer_host/render_view_host.h content/browser/renderer_host/render_view_host.cc content/renderer/render_view_impl.h content/renderer/render_view_impl.cc  content/renderer/renderer_webcolorchooser_impl.h  content/renderer/renderer_webcolorchooser_impl.cc > For the two files you listed for me, my main question concern is that ENABLE_XXX > is not how we build experimental features in Chromium -- this is a WebKit-ism. > If possible, you should just build your feature on by default in Chromium (and > if it needs to be invisible to users for a while, control that on the WebKit > side). If you need to have this code not run on the Chromium side by default > for a while, then use a command-line flag, possibly with an about:flags entry > attached, to do run-time control over this rather than compile-time. Okay. I removed the flag, I'll just have to land all the patches at the same time.
I drew a diagram of how the color chooser will work. https://docs.google.com/open?id=0BwRi59l_ri74MzRjOTIzMzUtZDRlYy00NTY5LWEyNjAt...
http://codereview.chromium.org/9203001/diff/13001/chrome/browser/color_select... File chrome/browser/color_select_observer.cc (right): http://codereview.chromium.org/9203001/diff/13001/chrome/browser/color_select... chrome/browser/color_select_observer.cc:11: #include "content/common/view_messages.h" chrome can't include view_messages.h, since that's an internal implementation detail of content. unfortunately we can't turn on deps checking yet because we're waiting for the last file to be moved to content (TabContentsViewMac). so chrome layer doesn't filter content IPCs. instead, content notifies its embedders through the use of existing interfaces like RenderViewHostDelegate or WebContentsDelegate. http://codereview.chromium.org/9203001/diff/13001/content/common/view_messages.h File content/common/view_messages.h (right): http://codereview.chromium.org/9203001/diff/13001/content/common/view_message... content/common/view_messages.h:1006: IPC_STRUCT_BEGIN(ViewHostMsg_SetSelectedColorInColorChooser_Params) we only create structs when we need to pass a message with more than 5 input or output parameters. if you just have one, just send the enum directly as the parameter http://codereview.chromium.org/9203001/diff/13001/content/renderer/renderer_w... File content/renderer/renderer_webcolorchooser_impl.cc (right): http://codereview.chromium.org/9203001/diff/13001/content/renderer/renderer_w... content/renderer/renderer_webcolorchooser_impl.cc:41: static_cast<RenderViewImpl*>(render_view())->endColorChooser(this); nit: chrome function names are CamelCase. also, why pass |this| to the RenderViewImpl even though the RVI only has one ColorChooserImpl?
http://codereview.chromium.org/9203001/diff/13001/chrome/browser/color_select... File chrome/browser/color_select_observer.h (right): http://codereview.chromium.org/9203001/diff/13001/chrome/browser/color_select... chrome/browser/color_select_observer.h:12: #include "chrome/browser/ui/color_chooser_dialog.h" Where is this file? For that matter, why is it a different file/class? http://codereview.chromium.org/9203001/diff/13001/chrome/browser/color_select... chrome/browser/color_select_observer.h:23: public ColorChooserDialog::Listener { this kind of ifdef is very ugly and usually indicative of a poor design. we shouldn't be adding new features that do this kind of thing.
http://codereview.chromium.org/9203001/diff/13001/chrome/browser/color_select... File chrome/browser/color_select_observer.cc (right): http://codereview.chromium.org/9203001/diff/13001/chrome/browser/color_select... chrome/browser/color_select_observer.cc:36: #if defined(OS_WIN) As Ben noted, we shouldn't have bunches of #ifdefs in common code. Think carefully about what functionality truly needs to be platform-specific versus what is merely defined for Windows because you haven't had the time or knowledge to implement it for other platforms. Hopefully the result is that most code can be cross-platform with a few platform-specific bits separated out to separate .cc files that live in the relevant ui/*/ subdirectories. http://codereview.chromium.org/9203001/diff/13001/chrome/browser/ui/color_cho... File chrome/browser/ui/color_chooser.h (right): http://codereview.chromium.org/9203001/diff/13001/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser.h:24: // Returns singleton instance of ColorChooser. This seems wrong -- why can't multiple tabs ask for color choosers? Restricting to a singleton doesn't seem necessary. http://codereview.chromium.org/9203001/diff/13001/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser.h:27: virtual void open(Listener* listener, WebKit::WebColor initial_color) = 0; Use Chrome-style function names, not WebKit-style.
http://codereview.chromium.org/9203001/diff/13001/chrome/browser/color_select... File chrome/browser/color_select_observer.cc (right): http://codereview.chromium.org/9203001/diff/13001/chrome/browser/color_select... chrome/browser/color_select_observer.cc:11: #include "content/common/view_messages.h" On 2012/01/17 06:49:55, John Abd-El-Malek wrote: > chrome can't include view_messages.h, since that's an internal implementation > detail of content. unfortunately we can't turn on deps checking yet because > we're waiting for the last file to be moved to content (TabContentsViewMac). > > so chrome layer doesn't filter content IPCs. instead, content notifies its > embedders through the use of existing interfaces like RenderViewHostDelegate or > WebContentsDelegate. Done. I've made it go through TabContents and Browser. http://codereview.chromium.org/9203001/diff/13001/chrome/browser/color_select... chrome/browser/color_select_observer.cc:36: #if defined(OS_WIN) On 2012/01/17 20:53:10, Peter Kasting wrote: > As Ben noted, we shouldn't have bunches of #ifdefs in common code. Think > carefully about what functionality truly needs to be platform-specific versus > what is merely defined for Windows because you haven't had the time or knowledge > to implement it for other platforms. Hopefully the result is that most code can > be cross-platform with a few platform-specific bits separated out to separate > .cc files that live in the relevant ui/*/ subdirectories. Done. http://codereview.chromium.org/9203001/diff/13001/chrome/browser/color_select... File chrome/browser/color_select_observer.h (right): http://codereview.chromium.org/9203001/diff/13001/chrome/browser/color_select... chrome/browser/color_select_observer.h:12: #include "chrome/browser/ui/color_chooser_dialog.h" On 2012/01/17 16:29:09, Ben Goodger (Google) wrote: > Where is this file? > > For that matter, why is it a different file/class? It was in a different patch, but I've consolidated all the patches for the different ports into this one. ColorChooserDialog inherits BaseShellDialog and it is used for the Windows color chooser which is a modal dialog. Mac and Gtk color choosers are modeless dialogs and don't need to inherit BaseShellDialog. http://codereview.chromium.org/9203001/diff/13001/chrome/browser/color_select... chrome/browser/color_select_observer.h:23: public ColorChooserDialog::Listener { On 2012/01/17 16:29:09, Ben Goodger (Google) wrote: > this kind of ifdef is very ugly and usually indicative of a poor design. we > shouldn't be adding new features that do this kind of thing. Removed. http://codereview.chromium.org/9203001/diff/13001/chrome/browser/ui/color_cho... File chrome/browser/ui/color_chooser.h (right): http://codereview.chromium.org/9203001/diff/13001/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser.h:24: // Returns singleton instance of ColorChooser. On 2012/01/17 20:53:10, Peter Kasting wrote: > This seems wrong -- why can't multiple tabs ask for color choosers? Restricting > to a singleton doesn't seem necessary. Multiple tabs could ask for color choosers but there should only be one open at the same time. And thats why I used a singleton, but I guess it wasn't a good choice. I've quit using singletons. http://codereview.chromium.org/9203001/diff/13001/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser.h:27: virtual void open(Listener* listener, WebKit::WebColor initial_color) = 0; On 2012/01/17 20:53:10, Peter Kasting wrote: > Use Chrome-style function names, not WebKit-style. Done. http://codereview.chromium.org/9203001/diff/13001/content/common/view_messages.h File content/common/view_messages.h (right): http://codereview.chromium.org/9203001/diff/13001/content/common/view_message... content/common/view_messages.h:1006: IPC_STRUCT_BEGIN(ViewHostMsg_SetSelectedColorInColorChooser_Params) On 2012/01/17 06:49:55, John Abd-El-Malek wrote: > we only create structs when we need to pass a message with more than 5 input or > output parameters. if you just have one, just send the enum directly as the > parameter Done.
Can you say who is reviewing what at this point? Make sure you also have dedicated GTK and Mac reviewers for those parts.
(just looked at content) http://codereview.chromium.org/9203001/diff/22034/content/browser/renderer_ho... File content/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/9203001/diff/22034/content/browser/renderer_ho... content/browser/renderer_host/render_view_host.cc:758: IPC_MESSAGE_HANDLER(ViewHostMsg_OpenColorChooser, OnOpenColorChooser) there's no need to filter the message in RenderViewHost just to call TabContents and have it call back here. You can dispatch the IPCs in TabContents instead. http://codereview.chromium.org/9203001/diff/22034/content/public/browser/rend... File content/public/browser/render_view_host_delegate.h (right): http://codereview.chromium.org/9203001/diff/22034/content/public/browser/rend... content/public/browser/render_view_host_delegate.h:378: virtual void OpenColorChooser(RenderViewHost* render_view_host, nit: please add comments like all the other functions in this file http://codereview.chromium.org/9203001/diff/22034/content/public/browser/web_... File content/public/browser/web_contents_delegate.h (right): http://codereview.chromium.org/9203001/diff/22034/content/public/browser/web_... content/public/browser/web_contents_delegate.h:324: virtual void OpenColorChooser(WebContents* tab, ditto http://codereview.chromium.org/9203001/diff/22034/content/renderer/render_vie... File content/renderer/render_view_impl.cc (right): http://codereview.chromium.org/9203001/diff/22034/content/renderer/render_vie... content/renderer/render_view_impl.cc:1623: void RenderViewImpl::endColorChooser() { I don't think this function is needed. The colorchooser object can watch ClosePage through the RenderViewObserver interface and delete itself then. it doesn't need to call RVImpl. then the latter doesn't need to store a pointer to it.
http://codereview.chromium.org/9203001/diff/22034/chrome/browser/ui/color_cho... File chrome/browser/ui/color_chooser.h (right): http://codereview.chromium.org/9203001/diff/22034/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser.h:21: virtual void DidChooseColor(WebKit::WebColor color) = 0; Don't use WebKit:: types in Chrome code. Use SkColor, for which platform-native color conversion functions already exist. http://codereview.chromium.org/9203001/diff/22034/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser.h:26: static ColorChooser* Create(); Why not have this function take a listener, instead of Open() and End(), and have the concrete class store this as the listener object? Seems like this would also eliminate the need for your ColorSelectHelper object.
I would like to request reviews for these files from the following reviewers. # pkasting@chromium.org chrome/app/generated_resources.grd chrome/browser/ui/base_shell_dialog.h chrome/browser/ui/browser.cc chrome/browser/ui/browser.h chrome/browser/ui/color_chooser.h chrome/browser/ui/color_chooser_dialog.h chrome/browser/ui/select_file_dialog.h chrome/browser/ui/views/base_shell_dialog_win.cc chrome/browser/ui/views/base_shell_dialog_win.h chrome/browser/ui/views/color_chooser_dialog_win.cc chrome/browser/ui/views/color_chooser_dialog_win.h chrome/browser/ui/views/color_chooser_win.cc chrome/browser/ui/views/select_file_dialog_win.cc chrome/chrome_browser.gypi # estade@chromium.org chrome/browser/ui/gtk/color_chooser_gtk.cc # jam@chromium.org content/browser/renderer_host/render_view_host.cc content/browser/renderer_host/render_view_host.h content/browser/tab_contents/tab_contents.cc content/browser/tab_contents/tab_contents.h content/common/view_messages.h content/content_renderer.gypi content/public/browser/render_view_host_delegate.h content/public/browser/web_contents_delegate.h content/renderer/render_view_impl.cc content/renderer/render_view_impl.h content/renderer/renderer_webcolorchooser_impl.cc content/renderer/renderer_webcolorchooser_impl.h # thakis@chromium.org chrome/browser/ui/cocoa/color_chooser_mac.mm skia/ext/skia_utils_mac.h skia/ext/skia_utils_mac.mm http://codereview.chromium.org/9203001/diff/22034/chrome/browser/ui/color_cho... File chrome/browser/ui/color_chooser.h (right): http://codereview.chromium.org/9203001/diff/22034/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser.h:21: virtual void DidChooseColor(WebKit::WebColor color) = 0; On 2012/01/27 18:27:15, Peter Kasting wrote: > Don't use WebKit:: types in Chrome code. Use SkColor, for which platform-native > color conversion functions already exist. Done. http://codereview.chromium.org/9203001/diff/22034/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser.h:26: static ColorChooser* Create(); On 2012/01/27 18:27:15, Peter Kasting wrote: > Why not have this function take a listener, instead of Open() and End(), and > have the concrete class store this as the listener object? Seems like this > would also eliminate the need for your ColorSelectHelper object. Done. http://codereview.chromium.org/9203001/diff/22034/content/browser/renderer_ho... File content/browser/renderer_host/render_view_host.cc (right): http://codereview.chromium.org/9203001/diff/22034/content/browser/renderer_ho... content/browser/renderer_host/render_view_host.cc:758: IPC_MESSAGE_HANDLER(ViewHostMsg_OpenColorChooser, OnOpenColorChooser) On 2012/01/27 18:13:40, John Abd-El-Malek wrote: > there's no need to filter the message in RenderViewHost just to call TabContents > and have it call back here. You can dispatch the IPCs in TabContents instead. Done. http://codereview.chromium.org/9203001/diff/22034/content/public/browser/rend... File content/public/browser/render_view_host_delegate.h (right): http://codereview.chromium.org/9203001/diff/22034/content/public/browser/rend... content/public/browser/render_view_host_delegate.h:378: virtual void OpenColorChooser(RenderViewHost* render_view_host, On 2012/01/27 18:13:40, John Abd-El-Malek wrote: > nit: please add comments like all the other functions in this file Done. http://codereview.chromium.org/9203001/diff/22034/content/public/browser/web_... File content/public/browser/web_contents_delegate.h (right): http://codereview.chromium.org/9203001/diff/22034/content/public/browser/web_... content/public/browser/web_contents_delegate.h:324: virtual void OpenColorChooser(WebContents* tab, On 2012/01/27 18:13:40, John Abd-El-Malek wrote: > ditto Done. http://codereview.chromium.org/9203001/diff/22034/content/renderer/render_vie... File content/renderer/render_view_impl.cc (right): http://codereview.chromium.org/9203001/diff/22034/content/renderer/render_vie... content/renderer/render_view_impl.cc:1623: void RenderViewImpl::endColorChooser() { On 2012/01/27 18:13:40, John Abd-El-Malek wrote: > I don't think this function is needed. > > The colorchooser object can watch ClosePage through the RenderViewObserver > interface and delete itself then. it doesn't need to call RVImpl. then the > latter doesn't need to store a pointer to it. This pointer was used to end the ColorChooser inside WebKit inside the open call so that there isn't two RendererWebColorChooserImpl instances at the same time. Two remove this pointer and to make the code more robust I introduced color chooser ids in my new patch.
http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... File chrome/browser/ui/gtk/color_chooser_gtk.cc (right): http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:23: virtual void Open(SkColor initial_color); shouldn't these have OVERRIDE http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:31: GtkColorSelection* colorSelection(); docs http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:39: GtkWidget* color_selecting_dialog_; why is this not color_selection_dialog_ http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:47: printf("> ColorChooserGtk::~ColorChooserGtk %p\n", this); remove debugging line http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:52: GtkColorSelection* ColorChooserGtk::colorSelection() { ColorSelection() http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:54: return NULL; nit: \n http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:77: printf("> ColorChooserGtk::Open %p %d\n", this, !color_selecting_dialog_); remove debugging line http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:89: G_CALLBACK(OnColorChooserDestroyThunk), this); I think you might also need to worry about delete-event. Test if clicking the [x] to close the dialog without a selection works as expected. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:90: } nit: \n http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:98: printf("> ColorChooserMac::End %p\n", this); remove debugging line http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:100: return; nit: \n http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:106: return; nit: \n
I'm not sure what the IDs are used for precisely, so I'm hoping John can make some sense out of the content side of things. Why do we need to plumb an explicit "the dialog closed" message back through the various layers? Can't we just get by with an optional "web page should reset its color to x" message? Has this gone through UI review? Is a dialog that's window-modal what they want to see happen? http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:4028: printf("! Browser::OpenColorChooser %p %d\n", this, color_chooser_id); No printfs (3 places) http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:4037: void Browser::EndColorChooser(WebContents* tab, unsigned color_chooser_id) { In general I don't understand why you check both the RVH pointers for equality AND have the ID checks. It seems like one must be a superset of the other? Or is that not true? http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:4041: if (tab->GetRenderViewHost() != color_chooser_->render_view_host()) Nit: Two copies of this line, I think you meant to check the ID http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:4046: unsigned color_chooser_id, Nit: Indent even. If it won't fit, indent even the first arg 4 http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:4048: if (!color_chooser_.get()) Nit: Combine these conditionals http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/browser.h File chrome/browser/ui/browser.h (right): http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/browser.h... chrome/browser/ui/browser.h:72: class ColorChooser; Nit: Declare in alphabetical order http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/color_cho... File chrome/browser/ui/color_chooser.h (right): http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser.h:14: // Abstraction object for color choosers of each platforms. Nit: "of each platforms" -> "for each platform" http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser.h:17: static ColorChooser* Create(unsigned identifier, RenderViewHost*); Nit: "unsigned" is banned by the style guide. Use int or size_t depending on whether you want "generic numeric identifier" or "count of objects in memory-based container". Also, name all arguments. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser.h:23: virtual void Open(SkColor initial_color) = 0; Nit: Why isn't this an arg to the constructor instead? http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/color_cho... File chrome/browser/ui/color_chooser_dialog.h (right): http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser_dialog.h:14: class ColorChooserDialog ColorChooserDialogWin is the only implementer of this class. Seems like the two should just be combined into a single class definition. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser_dialog.h:29: static ColorChooserDialog* Create(Listener* listener); Nit: The combined class can just use a standard constructor since there's no platform-independence benefit from running a Create() function. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser_dialog.h:33: gfx::NativeWindow owning_window) = 0; Nit: Why aren't these args to the constructor instead? http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser_dialog.h:35: explicit ColorChooserDialog() { } Nit: Don't use "explicit" on no-arg constructor forms. No spaces between {}. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/bas... File chrome/browser/ui/views/base_shell_dialog_win.h (right): http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/bas... chrome/browser/ui/views/base_shell_dialog_win.h:18: class ShellDialogThread : public base::Thread { Nit: This class can probably move to the .cc file. When you do so, also de-inline the function definitions. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/bas... chrome/browser/ui/views/base_shell_dialog_win.h:108: typedef std::set<HWND> Owners; Nit: This typedef belongs atop the private section http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_dialog_win.cc (right): http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog_win.cc:41: base::Bind(&ColorChooserDialogWin::ExecuteOpen, this, execute_params)); Nit: Indent 4 http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog_win.cc:50: cc.lpCustColors = custom_colors_; Pointing at a member isn't going to be very useful because this dialog object won't live past this invocation. Instead we should probably point at some piece of storage that's owned by the window? (Pointing at a global might be dangerous because who knows what could happen if two different color choosers try to access it simultaneously.) http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog_win.cc:51: cc.Flags = CC_FULLOPEN | CC_RGBINIT; Nit: Do you need to set CC_ANYCOLOR? (What difference does that flag make, anyway?) http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_dialog_win.h (right): http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog_win.h:28: ExecuteOpenParams(SkColor color, Nit: Don't inline this constructor, since the members are not all basic types. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_win.cc (right): http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_win.cc:17: virtual void End() {} Shouldn't these two functions do something? http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_win.cc:26: color_chooser_dialog_(NULL) {} Nit: Don't inline this constructor http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_win.cc:61: color_chooser_dialog_ = NULL; Nit: Use .reset() http://codereview.chromium.org/9203001/diff/28001/content/public/browser/web_... File content/public/browser/web_contents_delegate.h (right): http://codereview.chromium.org/9203001/diff/28001/content/public/browser/web_... content/public/browser/web_contents_delegate.h:56: namespace WebKit { Remove this. http://codereview.chromium.org/9203001/diff/28001/content/public/browser/web_... content/public/browser/web_contents_delegate.h:326: unsigned color_chooser_id, Nit: See other nit regarding "unsigned"
http://codereview.chromium.org/9203001/diff/22034/content/renderer/render_vie... File content/renderer/render_view_impl.cc (right): http://codereview.chromium.org/9203001/diff/22034/content/renderer/render_vie... content/renderer/render_view_impl.cc:1623: void RenderViewImpl::endColorChooser() { On 2012/02/06 15:01:57, keishi1 wrote: > On 2012/01/27 18:13:40, John Abd-El-Malek wrote: > > I don't think this function is needed. > > > > The colorchooser object can watch ClosePage through the RenderViewObserver > > interface and delete itself then. it doesn't need to call RVImpl. then the > > latter doesn't need to store a pointer to it. > > This pointer was used to end the ColorChooser inside WebKit inside the open call > so that there isn't two RendererWebColorChooserImpl instances at the same time. > Two remove this pointer and to make the code more robust I introduced color > chooser ids in my new patch. Perhaps I wasn't clear. I don't think this is needed because when WebKit wants to end the color chooser, it currently calls RendererWebColorChooserImpl::endChooser(). Now this turns around and calls RenderViewImpl::endChooser, which RVI now needs to reset the pointer it's holding on to. But why is it holding on to the pointer in the first place? It looks like you do that so that you can call it in OnShouldClose and to avoid having two objects per RenderViewImpl. For the first use case, you can watch ClosePage through RenderViewObserver. For the second, you can derive from the RenderViewObserverTracker template which will allow you to find if there are any instances.
> Why do we need to plumb an explicit "the dialog closed" message back through the > various layers? Can't we just get by with an optional "web page should reset its color to x" message? The mac color picker sends realtime color chooser updates and sends close when the user closes the NSColorPanel. > Has this gone through UI review? Is a dialog that's window-modal what they want > to see happen? I've already asked the UI team. Glen said that modeless dialogs for mac/gtk and modal dialog for Windows is OK. And that we'll probably want to make a better custom color picker for Windows in the future. > Perhaps I wasn't clear. I don't think this is needed because when WebKit wants > to end the color chooser, it currently calls > RendererWebColorChooserImpl::endChooser(). Now this turns around and calls > RenderViewImpl::endChooser, which RVI now needs to reset the pointer it's > holding on to. But why is it holding on to the pointer in the first place? It > looks like you do that so that you can call it in OnShouldClose and to avoid > having two objects per RenderViewImpl. For the first use case, you can watch > ClosePage through RenderViewObserver. For the second, you can derive from the > RenderViewObserverTracker template which will allow you to find if there are any > instances. I decided to keep the color_chooser_id to avoid the situation in the figure below. https://docs.google.com/open?id=0BwRi59l_ri74ODVmNjQ1ZGYtNmFiMi00MjBkLWJjNDgt... I used ClosePage but I didn't need RenderViewObserverTracker. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:4028: printf("! Browser::OpenColorChooser %p %d\n", this, color_chooser_id); On 2012/02/07 01:46:24, Peter Kasting wrote: > No printfs (3 places) Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:4037: void Browser::EndColorChooser(WebContents* tab, unsigned color_chooser_id) { On 2012/02/07 01:46:24, Peter Kasting wrote: > In general I don't understand why you check both the RVH pointers for equality > AND have the ID checks. It seems like one must be a superset of the other? Or > is that not true? The color chooser id is generated by incrementing a counter inside the renderer process so it overlaps for different renderers. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:4041: if (tab->GetRenderViewHost() != color_chooser_->render_view_host()) On 2012/02/07 01:46:24, Peter Kasting wrote: > Nit: Two copies of this line, I think you meant to check the ID Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:4046: unsigned color_chooser_id, On 2012/02/07 01:46:24, Peter Kasting wrote: > Nit: Indent even. If it won't fit, indent even the first arg 4 Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:4048: if (!color_chooser_.get()) On 2012/02/07 01:46:24, Peter Kasting wrote: > Nit: Combine these conditionals Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/browser.h File chrome/browser/ui/browser.h (right): http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/browser.h... chrome/browser/ui/browser.h:72: class ColorChooser; On 2012/02/07 01:46:24, Peter Kasting wrote: > Nit: Declare in alphabetical order Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/color_cho... File chrome/browser/ui/color_chooser.h (right): http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser.h:14: // Abstraction object for color choosers of each platforms. On 2012/02/07 01:46:24, Peter Kasting wrote: > Nit: "of each platforms" -> "for each platform" Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser.h:17: static ColorChooser* Create(unsigned identifier, RenderViewHost*); On 2012/02/07 01:46:24, Peter Kasting wrote: > Nit: "unsigned" is banned by the style guide. Use int or size_t depending on > whether you want "generic numeric identifier" or "count of objects in > memory-based container". > > Also, name all arguments. Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser.h:23: virtual void Open(SkColor initial_color) = 0; On 2012/02/07 01:46:24, Peter Kasting wrote: > Nit: Why isn't this an arg to the constructor instead? Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/color_cho... File chrome/browser/ui/color_chooser_dialog.h (right): http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser_dialog.h:14: class ColorChooserDialog On 2012/02/07 01:46:24, Peter Kasting wrote: > ColorChooserDialogWin is the only implementer of this class. Seems like the two > should just be combined into a single class definition. Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser_dialog.h:29: static ColorChooserDialog* Create(Listener* listener); On 2012/02/07 01:46:24, Peter Kasting wrote: > Nit: The combined class can just use a standard constructor since there's no > platform-independence benefit from running a Create() function. Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser_dialog.h:33: gfx::NativeWindow owning_window) = 0; On 2012/02/07 01:46:24, Peter Kasting wrote: > Nit: Why aren't these args to the constructor instead? Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser_dialog.h:35: explicit ColorChooserDialog() { } On 2012/02/07 01:46:24, Peter Kasting wrote: > Nit: Don't use "explicit" on no-arg constructor forms. No spaces between {}. Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... File chrome/browser/ui/gtk/color_chooser_gtk.cc (right): http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:23: virtual void Open(SkColor initial_color); On 2012/02/06 21:16:25, Evan Stade wrote: > shouldn't these have OVERRIDE Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:31: GtkColorSelection* colorSelection(); On 2012/02/06 21:16:25, Evan Stade wrote: > docs What does docs mean? http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:39: GtkWidget* color_selecting_dialog_; On 2012/02/06 21:16:25, Evan Stade wrote: > why is this not color_selection_dialog_ Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:47: printf("> ColorChooserGtk::~ColorChooserGtk %p\n", this); On 2012/02/06 21:16:25, Evan Stade wrote: > remove debugging line Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:52: GtkColorSelection* ColorChooserGtk::colorSelection() { On 2012/02/06 21:16:25, Evan Stade wrote: > ColorSelection() Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:54: return NULL; On 2012/02/06 21:16:25, Evan Stade wrote: > nit: \n Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:77: printf("> ColorChooserGtk::Open %p %d\n", this, !color_selecting_dialog_); On 2012/02/06 21:16:25, Evan Stade wrote: > remove debugging line Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:89: G_CALLBACK(OnColorChooserDestroyThunk), this); On 2012/02/06 21:16:25, Evan Stade wrote: > I think you might also need to worry about delete-event. Test if clicking the > [x] to close the dialog without a selection works as expected. Clicking the [x] seems to call "destroy". I looked at SelectFileDialogImplGTK and added a callback for "delete-event" so it matches it. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:90: } On 2012/02/06 21:16:25, Evan Stade wrote: > nit: \n Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:98: printf("> ColorChooserMac::End %p\n", this); On 2012/02/06 21:16:25, Evan Stade wrote: > remove debugging line Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:100: return; On 2012/02/06 21:16:25, Evan Stade wrote: > nit: \n Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:106: return; On 2012/02/06 21:16:25, Evan Stade wrote: > nit: \n Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/bas... File chrome/browser/ui/views/base_shell_dialog_win.h (right): http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/bas... chrome/browser/ui/views/base_shell_dialog_win.h:18: class ShellDialogThread : public base::Thread { On 2012/02/07 01:46:24, Peter Kasting wrote: > Nit: This class can probably move to the .cc file. When you do so, also > de-inline the function definitions. Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/bas... chrome/browser/ui/views/base_shell_dialog_win.h:108: typedef std::set<HWND> Owners; On 2012/02/07 01:46:24, Peter Kasting wrote: > Nit: This typedef belongs atop the private section Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_dialog_win.cc (right): http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog_win.cc:41: base::Bind(&ColorChooserDialogWin::ExecuteOpen, this, execute_params)); On 2012/02/07 01:46:24, Peter Kasting wrote: > Nit: Indent 4 Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog_win.cc:51: cc.Flags = CC_FULLOPEN | CC_RGBINIT; On 2012/02/07 01:46:24, Peter Kasting wrote: > Nit: Do you need to set CC_ANYCOLOR? (What difference does that flag make, > anyway?) Seems like the original purpose was to show all colors including dithered colors. Added it anyway. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_dialog_win.h (right): http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog_win.h:28: ExecuteOpenParams(SkColor color, On 2012/02/07 01:46:24, Peter Kasting wrote: > Nit: Don't inline this constructor, since the members are not all basic types. Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_win.cc (right): http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_win.cc:17: virtual void End() {} On 2012/02/07 01:46:24, Peter Kasting wrote: > Shouldn't these two functions do something? Unlike other platforms the Windows color chooser will be modal dialog so the page shouldn't be able to close it or set its color. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_win.cc:26: color_chooser_dialog_(NULL) {} On 2012/02/07 01:46:24, Peter Kasting wrote: > Nit: Don't inline this constructor Done. http://codereview.chromium.org/9203001/diff/28001/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_win.cc:61: color_chooser_dialog_ = NULL; On 2012/02/07 01:46:24, Peter Kasting wrote: > Nit: Use .reset() color_chooser_dialog_ is a scoped_refptr so I can't use reset. I've left it as it was. http://codereview.chromium.org/9203001/diff/28001/content/public/browser/web_... File content/public/browser/web_contents_delegate.h (right): http://codereview.chromium.org/9203001/diff/28001/content/public/browser/web_... content/public/browser/web_contents_delegate.h:56: namespace WebKit { On 2012/02/07 01:46:24, Peter Kasting wrote: > Remove this. Done. http://codereview.chromium.org/9203001/diff/28001/content/public/browser/web_... content/public/browser/web_contents_delegate.h:326: unsigned color_chooser_id, On 2012/02/07 01:46:24, Peter Kasting wrote: > Nit: See other nit regarding "unsigned" Done.
Here is the updated list of review requests. # estade@chromium.org chrome/app/generated_resources.grd chrome/browser/ui/gtk/color_chooser_gtk.cc # thakis@chromium.org chrome/browser/ui/cocoa/color_chooser_mac.mm skia/ext/skia_utils_mac.h skia/ext/skia_utils_mac.mm # pkasting@chromium.org chrome/browser/ui/base_shell_dialog.h chrome/browser/ui/browser.h chrome/browser/ui/browser.cc chrome/browser/ui/color_chooser.h chrome/browser/ui/select_file_dialog.h chrome/browser/ui/views/base_shell_dialog_win.h chrome/browser/ui/views/base_shell_dialog_win.cc chrome/browser/ui/views/color_chooser_dialog.h chrome/browser/ui/views/color_chooser_dialog.cc chrome/browser/ui/views/color_chooser_win.cc chrome/browser/ui/views/select_file_dialog_win.cc chrome/chrome_browser.gypi # jam@chromium.org content/browser/renderer_host/render_view_host.h content/browser/renderer_host/render_view_host.cc content/browser/tab_contents/tab_contents.h content/browser/tab_contents/tab_contents.cc content/common/view_messages.h content/content_renderer.gypi content/public/browser/web_contents_delegate.h content/renderer/render_view_impl.h content/renderer/render_view_impl.cc content/renderer/renderer_webcolorchooser_impl.h content/renderer/renderer_webcolorchooser_impl.cc
Looks like there's no tests for this? http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/cocoa/col... File chrome/browser/ui/cocoa/color_chooser_mac.mm (right): http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:27: // Called from NSColorPanel . http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:28: - (void)didChooseColor:(NSColorPanel *)panel; No space in front of * http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:30: - (void)setColor:(NSColor *)color; No space in front of * http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:41: // Called from ColorPanelBridge . http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:69: // setColor needs to be called after panel's target is set. what does "panel" refer to? http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:115: [panel setAction:@selector(didChooseColor:)]; What happens if two web sites open this chooser? It looks like the 2nd page will overwrite the action on the (singleton) NSColorPanel and kind of left there hanging? (…and never get the "chooser did close" notification either) http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:138: nonUserChange_ = NO; dedent 2 http://codereview.chromium.org/9203001/diff/39001/skia/ext/skia_utils_mac.h File skia/ext/skia_utils_mac.h (right): http://codereview.chromium.org/9203001/diff/39001/skia/ext/skia_utils_mac.h#n... skia/ext/skia_utils_mac.h:68: SkColor NSColorToSkColor(NSColor* color); This should mention color spaces somehow ("Returns raw rgb values and does no colorspace conversion. Only valid for colors in calibrated and device color spaces" or something like that.) Also, this needs an SK_API (those aren't used for anything yet but hopefully will be soon). http://codereview.chromium.org/9203001/diff/39001/skia/ext/skia_utils_mac.mm File skia/ext/skia_utils_mac.mm (right): http://codereview.chromium.org/9203001/diff/39001/skia/ext/skia_utils_mac.mm#... skia/ext/skia_utils_mac.mm:191: CGFloat red, green, blue, alpha; DCHECK([color colorSpace] == NSCalibratedRGBColorSpace || [color colorSpace] == NSDeviceRGBColorSpace)? http://codereview.chromium.org/9203001/diff/39001/skia/ext/skia_utils_mac.mm#... skia/ext/skia_utils_mac.mm:192: [color getRed:&red green:&green blue:&blue alpha:&alpha]; Are you sure you don't want to call this NSDeviceColorToSkColor() and make it call `color = [color colorUsingColorSpace:NSDeviceRGBColorSpace] first?
http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/color_cho... File chrome/browser/ui/color_chooser.h (right): http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser.h:22: virtual int identifier() const = 0; these two should have C++ style names, not c style http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/gtk/color... File chrome/browser/ui/gtk/color_chooser_gtk.cc (right): http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:33: GtkColorSelection* ColorSelection(); docs http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:39: int identifier_; docs http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:41: bool is_render_view_host_being_destroyed; docs http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:122: return content::RenderViewHostObserver::render_view_host(); too much indent
http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/browser.h File chrome/browser/ui/browser.h (right): http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/browser.h... chrome/browser/ui/browser.h:1463: scoped_ptr<ColorChooser> color_chooser_; it doesn't look like this gets destroyed after it's used, why is that? http://codereview.chromium.org/9203001/diff/39001/content/public/browser/web_... File content/public/browser/web_contents_delegate.h (right): http://codereview.chromium.org/9203001/diff/39001/content/public/browser/web_... content/public/browser/web_contents_delegate.h:317: virtual void EndColorChooser(WebContents* tab, int color_chooser_id) {} it seems that this method and the one below are properties on the color chooser object and not the delegate. can we make a small interface (in content/public/browser) that the embedder returns as a return value to OpenColorChooser? i.e. content::ColorChooser. it can then have these two methods on it. http://codereview.chromium.org/9203001/diff/39001/content/renderer/renderer_w... File content/renderer/renderer_webcolorchooser_impl.cc (right): http://codereview.chromium.org/9203001/diff/39001/content/renderer/renderer_w... content/renderer/renderer_webcolorchooser_impl.cc:10: static unsigned generateColorChooserIdentifier() { nit: GenerateColorChooserIdentifier http://codereview.chromium.org/9203001/diff/39001/content/renderer/renderer_w... content/renderer/renderer_webcolorchooser_impl.cc:11: static unsigned next = 0; nit: 2 space indents per chrome C++ style guide http://codereview.chromium.org/9203001/diff/39001/content/renderer/renderer_w... content/renderer/renderer_webcolorchooser_impl.cc:50: client_->didEndChooser(); I don't see the webkit side giving NULL pointers, so why do you null check everywhere? also it's a little odd from an API design that this object deletes its delegate. i haven't seen that before with WebKit API, did Darin review that? http://codereview.chromium.org/9203001/diff/39001/content/renderer/renderer_w... content/renderer/renderer_webcolorchooser_impl.cc:63: if (identifier_ != color_chooser_id) is this a race condition that you sent the picture of? http://codereview.chromium.org/9203001/diff/39001/content/renderer/renderer_w... File content/renderer/renderer_webcolorchooser_impl.h (right): http://codereview.chromium.org/9203001/diff/39001/content/renderer/renderer_w... content/renderer/renderer_webcolorchooser_impl.h:33: virtual WebKit::WebColorChooserClient* client(); why is this virtual? also, per C++ style guide only getters that are inline can have unix_hacker function names
> Looks like there's no tests for this? I'll try to include some tests in the next patch. Please let me know if you have ideas for must-include tests. http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/browser.h File chrome/browser/ui/browser.h (right): http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/browser.h... chrome/browser/ui/browser.h:1463: scoped_ptr<ColorChooser> color_chooser_; On 2012/02/22 02:55:20, John Abd-El-Malek wrote: > it doesn't look like this gets destroyed after it's used, why is that? I forgot to add color_chooser_.reset() to EndColorChooser(). http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/cocoa/col... File chrome/browser/ui/cocoa/color_chooser_mac.mm (right): http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:27: // Called from NSColorPanel On 2012/02/21 19:40:17, Nico wrote: > . Done. http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:28: - (void)didChooseColor:(NSColorPanel *)panel; On 2012/02/21 19:40:17, Nico wrote: > No space in front of * Done. http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:30: - (void)setColor:(NSColor *)color; On 2012/02/21 19:40:17, Nico wrote: > No space in front of * Done. http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:41: // Called from ColorPanelBridge On 2012/02/21 19:40:17, Nico wrote: > . Done. http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:69: // setColor needs to be called after panel's target is set. On 2012/02/21 19:40:17, Nico wrote: > what does "panel" refer to? I meant the shared color panel. I added this comment when I used to set the target/action inside this method, but I moved that inside the bridge constructor so I will remove this comment to avoid confusion. http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:115: [panel setAction:@selector(didChooseColor:)]; If a second site tries to open the color chooser, the Browser instance has a pointer to the currently open color chooser, so it gets destructed. On 2012/02/21 19:40:17, Nico wrote: > What happens if two web sites open this chooser? It looks like the 2nd page will > overwrite the action on the (singleton) NSColorPanel and kind of left there > hanging? (…and never get the "chooser did close" notification either) http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:138: nonUserChange_ = NO; On 2012/02/21 19:40:17, Nico wrote: > dedent 2 Done. http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/color_cho... File chrome/browser/ui/color_chooser.h (right): http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser.h:22: virtual int identifier() const = 0; On 2012/02/21 20:16:54, Evan Stade wrote: > these two should have C++ style names, not c style Done. http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/gtk/color... File chrome/browser/ui/gtk/color_chooser_gtk.cc (right): http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:33: GtkColorSelection* ColorSelection(); On 2012/02/21 20:16:54, Evan Stade wrote: > docs Done. http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:39: int identifier_; On 2012/02/21 20:16:54, Evan Stade wrote: > docs Done. http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:41: bool is_render_view_host_being_destroyed; On 2012/02/21 20:16:54, Evan Stade wrote: > docs This was left over code. Removed. http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/gtk/color... chrome/browser/ui/gtk/color_chooser_gtk.cc:122: return content::RenderViewHostObserver::render_view_host(); On 2012/02/21 20:16:54, Evan Stade wrote: > too much indent Done. http://codereview.chromium.org/9203001/diff/39001/content/public/browser/web_... File content/public/browser/web_contents_delegate.h (right): http://codereview.chromium.org/9203001/diff/39001/content/public/browser/web_... content/public/browser/web_contents_delegate.h:317: virtual void EndColorChooser(WebContents* tab, int color_chooser_id) {} On 2012/02/22 02:55:20, John Abd-El-Malek wrote: > it seems that this method and the one below are properties on the color chooser > object and not the delegate. > > can we make a small interface (in content/public/browser) that the embedder > returns as a return value to OpenColorChooser? i.e. content::ColorChooser. it > can then have these two methods on it. If I were to add content::ColorChooser interface, what should implement it? Should TabContents keep the returned pointer from OpenColorChooser as a member? I don't want multiple color chooser dialogs open at the same time, so I want to close the currently open one when opening a new one. This patch keeps the pointer to the currently open ColorChooser in the Browser object but I don't think it's a good place to keep it. Could you recommend a better place (I think maybe a singleton object) to keep this pointer ? http://codereview.chromium.org/9203001/diff/39001/content/renderer/renderer_w... File content/renderer/renderer_webcolorchooser_impl.cc (right): http://codereview.chromium.org/9203001/diff/39001/content/renderer/renderer_w... content/renderer/renderer_webcolorchooser_impl.cc:10: static unsigned generateColorChooserIdentifier() { On 2012/02/22 02:55:20, John Abd-El-Malek wrote: > nit: GenerateColorChooserIdentifier Done. http://codereview.chromium.org/9203001/diff/39001/content/renderer/renderer_w... content/renderer/renderer_webcolorchooser_impl.cc:11: static unsigned next = 0; On 2012/02/22 02:55:20, John Abd-El-Malek wrote: > nit: 2 space indents per chrome C++ style guide Done. http://codereview.chromium.org/9203001/diff/39001/content/renderer/renderer_w... content/renderer/renderer_webcolorchooser_impl.cc:50: client_->didEndChooser(); On 2012/02/22 02:55:20, John Abd-El-Malek wrote: > I don't see the webkit side giving NULL pointers, so why do you null check > everywhere? OK. Removed all checks. > also it's a little odd from an API design that this object deletes its delegate. > i haven't seen that before with WebKit API, did Darin review that? Sorry. I shouldn't have used a scoped_ptr for client_. The ColorChooserClient calls createColorChooser so it is the owner of the ColorChooser. http://codereview.chromium.org/9203001/diff/39001/content/renderer/renderer_w... content/renderer/renderer_webcolorchooser_impl.cc:63: if (identifier_ != color_chooser_id) On 2012/02/22 02:55:20, John Abd-El-Malek wrote: > is this a race condition that you sent the picture of? No, it's for another race condition. But this race condition is less likely to occur because the user has to click another input element while the ViewMsg_DidChooseColorResponse message is in the queue. http://codereview.chromium.org/9203001/diff/39001/content/renderer/renderer_w... File content/renderer/renderer_webcolorchooser_impl.h (right): http://codereview.chromium.org/9203001/diff/39001/content/renderer/renderer_w... content/renderer/renderer_webcolorchooser_impl.h:33: virtual WebKit::WebColorChooserClient* client(); On 2012/02/22 02:55:20, John Abd-El-Malek wrote: > why is this virtual? > also, per C++ style guide only getters that are inline can have unix_hacker > function names Removed virtual. Turned it into inline getter. http://codereview.chromium.org/9203001/diff/39001/skia/ext/skia_utils_mac.h File skia/ext/skia_utils_mac.h (right): http://codereview.chromium.org/9203001/diff/39001/skia/ext/skia_utils_mac.h#n... skia/ext/skia_utils_mac.h:68: SkColor NSColorToSkColor(NSColor* color); On 2012/02/21 19:40:17, Nico wrote: > This should mention color spaces somehow ("Returns raw rgb values and does no > colorspace conversion. Only valid for colors in calibrated and device color > spaces" or something like that.) > > Also, this needs an SK_API (those aren't used for anything yet but hopefully > will be soon). Done. http://codereview.chromium.org/9203001/diff/39001/skia/ext/skia_utils_mac.mm File skia/ext/skia_utils_mac.mm (right): http://codereview.chromium.org/9203001/diff/39001/skia/ext/skia_utils_mac.mm#... skia/ext/skia_utils_mac.mm:191: CGFloat red, green, blue, alpha; On 2012/02/21 19:40:17, Nico wrote: > DCHECK([color colorSpace] == NSCalibratedRGBColorSpace || [color colorSpace] == > NSDeviceRGBColorSpace)? Done. http://codereview.chromium.org/9203001/diff/39001/skia/ext/skia_utils_mac.mm#... skia/ext/skia_utils_mac.mm:192: [color getRed:&red green:&green blue:&blue alpha:&alpha]; OK. Done. On 2012/02/21 19:40:17, Nico wrote: > Are you sure you don't want to call this NSDeviceColorToSkColor() and make it > call `color = [color colorUsingColorSpace:NSDeviceRGBColorSpace] first?
http://codereview.chromium.org/9203001/diff/39001/content/public/browser/web_... File content/public/browser/web_contents_delegate.h (right): http://codereview.chromium.org/9203001/diff/39001/content/public/browser/web_... content/public/browser/web_contents_delegate.h:317: virtual void EndColorChooser(WebContents* tab, int color_chooser_id) {} On 2012/02/24 14:38:54, keishi1 wrote: > On 2012/02/22 02:55:20, John Abd-El-Malek wrote: > > it seems that this method and the one below are properties on the color > chooser > > object and not the delegate. > > > > can we make a small interface (in content/public/browser) that the embedder > > returns as a return value to OpenColorChooser? i.e. content::ColorChooser. it > > can then have these two methods on it. > > If I were to add content::ColorChooser interface, what should implement it? whichever code that implements EndColorChooser/SetSelectedColorInColorChooser now (i.e. your ColorChooser class). > Should TabContents keep the returned pointer from OpenColorChooser as a member? yes > > I don't want multiple color chooser dialogs open at the same time, so I want to > close the currently open one when opening a new one. if that's the case, then do you really need these color_chooser_ids? > > This patch keeps the pointer to the currently open ColorChooser in the Browser > object but I don't think it's a good place to keep it. > Could you recommend a better place (I think maybe a singleton object) to keep > this pointer ? singleton might mean that there is only one per process. i dont know when these color chooser UIs go away on all platforms. will they stay open on mac/linux when a user switches tabs or clicks on another chrome window? if so there could be multiple per process, in which case a singleton wouldn't work. if there's only one per tab, then on the TabContents seems fine. http://codereview.chromium.org/9203001/diff/39001/content/renderer/renderer_w... File content/renderer/renderer_webcolorchooser_impl.cc (right): http://codereview.chromium.org/9203001/diff/39001/content/renderer/renderer_w... content/renderer/renderer_webcolorchooser_impl.cc:63: if (identifier_ != color_chooser_id) On 2012/02/24 14:38:54, keishi1 wrote: > On 2012/02/22 02:55:20, John Abd-El-Malek wrote: > > is this a race condition that you sent the picture of? > > No, it's for another race condition. But this race condition is less likely to > occur because the user has to click another input element while the > ViewMsg_DidChooseColorResponse message is in the queue. Are you sure that's a race condition that can occur? If the user input event that causes the color picker to be hidden is in the renderer's IPC queue, won't that necessarily arrive before the input event that clicks on a different color picker? From what I understand on Mac and Linux the touch picker is non-modal, while on Windows it's modal because of a system constraint. But even on Mac/Linux, can there be more than one color picker visible at the same time on a page? I'd like to understand this better to know whether we need these color_chooser_ids.
On 2012/02/24 21:41:17, John Abd-El-Malek wrote: > > No, it's for another race condition. But this race condition is less likely to > > occur because the user has to click another input element while the > > ViewMsg_DidChooseColorResponse message is in the queue. > > Are you sure that's a race condition that can occur? If the user input event > that causes the color picker to be hidden is in the renderer's IPC queue, won't > that necessarily arrive before the input event that clicks on a different color > picker? On the Mac the DidChooseColorResponse is sent multiple times to reflect the selected color in the color picker in real-time. I drew a diagram for this race condition. https://docs.google.com/open?id=0BwRi59l_ri74NnRNUm1HUkJTWE9XdmlIcDBtZ0xmUQ If the mouse click event is sent from the browser to the renderer process through the same IPC queue as DidChooseColorResponse, this will not happen. So you might be right that this never happens. > From what I understand on Mac and Linux the touch picker is non-modal, while on > Windows it's modal because of a system constraint. But even on Mac/Linux, can > there be more than one color picker visible at the same time on a page? I'd like > to understand this better to know whether we need these color_chooser_ids. I made slides explaining why I think color_chooser_ids are necessary https://docs.google.com/presentation/d/1qDtKr8RB3xgOTJWAlaehOjxo-tTZuR8fbqNb6...
On 2012/02/26 10:54:47, keishi1 wrote: > On 2012/02/24 21:41:17, John Abd-El-Malek wrote: > > > No, it's for another race condition. But this race condition is less likely > to > > > occur because the user has to click another input element while the > > > ViewMsg_DidChooseColorResponse message is in the queue. > > > > Are you sure that's a race condition that can occur? If the user input event > > that causes the color picker to be hidden is in the renderer's IPC queue, > won't > > that necessarily arrive before the input event that clicks on a different > color > > picker? > > On the Mac the DidChooseColorResponse is sent multiple times to reflect the > selected color in the color picker in real-time. > I drew a diagram for this race condition. > https://docs.google.com/open?id=0BwRi59l_ri74NnRNUm1HUkJTWE9XdmlIcDBtZ0xmUQ > > If the mouse click event is sent from the browser to the renderer process > through the same IPC queue as DidChooseColorResponse, this will not happen. > So you might be right that this never happens. Yep there's only one IPC channel between the browser and each renderer process. > > > From what I understand on Mac and Linux the touch picker is non-modal, while > on > > Windows it's modal because of a system constraint. But even on Mac/Linux, can > > there be more than one color picker visible at the same time on a page? I'd > like > > to understand this better to know whether we need these color_chooser_ids. > > I made slides explaining why I think color_chooser_ids are necessary > https://docs.google.com/presentation/d/1qDtKr8RB3xgOTJWAlaehOjxo-tTZuR8fbqNb6... Thanks for the slides. I know understand why you need the id's.
> Yep there's only one IPC channel between the browser and each renderer process Okay. Turned that condition into a DCHECK. I followed jam@'s advice and made ColorChooser inherit content::ColorChooser. Could everyone review the new patch again? Thanks. # estade@chromium.org chrome/app/generated_resources.grd chrome/browser/ui/gtk/color_chooser_gtk.cc # thakis@chromium.org chrome/browser/ui/cocoa/color_chooser_mac.mm skia/ext/skia_utils_mac.h skia/ext/skia_utils_mac.mm # pkasting@chromium.org chrome/browser/ui/base_shell_dialog.h chrome/browser/ui/browser.h chrome/browser/ui/browser.cc chrome/browser/ui/color_chooser.h chrome/browser/ui/select_file_dialog.h chrome/browser/ui/views/base_shell_dialog_win.h chrome/browser/ui/views/base_shell_dialog_win.cc chrome/browser/ui/views/color_chooser_dialog.h chrome/browser/ui/views/color_chooser_dialog.cc chrome/browser/ui/views/color_chooser_win.cc chrome/browser/ui/views/select_file_dialog_win.cc chrome/chrome_browser.gypi # jam@chromium.org content/browser/renderer_host/render_view_host.h content/browser/renderer_host/render_view_host.cc content/browser/tab_contents/tab_contents.h content/browser/tab_contents/tab_contents.cc content/common/view_messages.h content/content_renderer.gypi content/public/browser/color_chooser.h content/public/browser/web_contents_delegate.h content/renderer/render_view_impl.h content/renderer/render_view_impl.cc content/renderer/renderer_webcolorchooser_impl.h content/renderer/renderer_webcolorchooser_impl.cc
Mac bits look fine. http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/cocoa/col... File chrome/browser/ui/cocoa/color_chooser_mac.mm (right): http://codereview.chromium.org/9203001/diff/39001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:115: [panel setAction:@selector(didChooseColor:)]; On 2012/02/24 14:38:54, keishi1 wrote: > If a second site tries to open the color chooser, the Browser instance has a > pointer to the currently open color chooser, so it gets destructed. This is something that might be worth having a test.
http://codereview.chromium.org/9203001/diff/51001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/9203001/diff/51001/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:4129: return NULL; I don't think this condition can happen? i.e. when will TabContents ask a delegate to open a color chooser with an id that it already asked it? http://codereview.chromium.org/9203001/diff/51001/content/browser/renderer_ho... File content/browser/renderer_host/render_view_host.h (right): http://codereview.chromium.org/9203001/diff/51001/content/browser/renderer_ho... content/browser/renderer_host/render_view_host.h:347: void DidChooseColorInColorChooser(int color_chooser_id, const SkColor&); it seems asymmetrical that the embedder's WebContentsDelegate is told to open a color chooser dialog, but then that has to callback the RenderViewHost with the response, instead of the WebContents. why is that? http://codereview.chromium.org/9203001/diff/51001/content/browser/tab_content... File content/browser/tab_contents/tab_contents.cc (right): http://codereview.chromium.org/9203001/diff/51001/content/browser/tab_content... content/browser/tab_contents/tab_contents.cc:542: GetRenderViewHost() == color_chooser_->GetRenderViewHost() && why is this check necessary, i.e. shouldn't the id be enough? http://codereview.chromium.org/9203001/diff/51001/content/browser/tab_content... File content/browser/tab_contents/tab_contents.h (right): http://codereview.chromium.org/9203001/diff/51001/content/browser/tab_content... content/browser/tab_contents/tab_contents.h:306: virtual void OnOpenColorChooser(int color_chooser_id, this section is for virtual methods, not message handlers. move these to a different section beside the rest of the ipc message handlers.
http://codereview.chromium.org/9203001/diff/51001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/9203001/diff/51001/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:4129: return NULL; On 2012/02/27 22:06:25, John Abd-El-Malek wrote: > I don't think this condition can happen? > > i.e. when will TabContents ask a delegate to open a color chooser with an id > that it already asked it? Yes, it should never happen. Removed. http://codereview.chromium.org/9203001/diff/51001/content/browser/tab_content... File content/browser/tab_contents/tab_contents.cc (right): http://codereview.chromium.org/9203001/diff/51001/content/browser/tab_content... content/browser/tab_contents/tab_contents.cc:542: GetRenderViewHost() == color_chooser_->GetRenderViewHost() && On 2012/02/27 22:06:25, John Abd-El-Malek wrote: > why is this check necessary, i.e. shouldn't the id be enough? You're right. Removed. http://codereview.chromium.org/9203001/diff/51001/content/browser/tab_content... File content/browser/tab_contents/tab_contents.h (right): http://codereview.chromium.org/9203001/diff/51001/content/browser/tab_content... content/browser/tab_contents/tab_contents.h:306: virtual void OnOpenColorChooser(int color_chooser_id, On 2012/02/27 22:06:25, John Abd-El-Malek wrote: > this section is for virtual methods, not message handlers. move these to a > different section beside the rest of the ipc message handlers. Done.
content lgtm, thanks for your patience :)
http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/cocoa/col... File chrome/browser/ui/cocoa/color_chooser_mac.mm (right): http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:19: @interface ColorPanelBridge : NSObject<NSWindowDelegate> { @private http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:19: @interface ColorPanelBridge : NSObject<NSWindowDelegate> { Naming: bridges are usually C++ -> ObjC. I'd call this ColorPanelCocoa. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:21: ColorChooserMac* chooser_; // weak, owns this http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:25: - (void)dealloc; No need to declare this http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:36: public: nit: indent 1 http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:50: private: nit: indent 1 http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:68: [[NSColorPanel sharedColorPanel] makeKeyAndOrderFront:NULL]; Use nil instead of NULL http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:124: - (void)windowWillClose:(NSNotification *)notification { nit: no space before * http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:129: - (void)didChooseColor:(NSColorPanel *)panel { here too http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:139: - (void)setColor:(NSColor *)color { and here
http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:4129: tab, Nit: Go ahead and put this arg on the previous line. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:4131: return static_cast<content::ColorChooser*>(color_chooser_.get()); Nit: Make |color_chooser_| be of this type so you don't need to static_cast<>(). http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/browser.h File chrome/browser/ui/browser.h (right): http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/browser.h... chrome/browser/ui/browser.h:1019: int color_chooser_id, Nit: Aligned with above arg, or all args (including first) indented 4 if that won't fit http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/browser.h... chrome/browser/ui/browser.h:1021: virtual void DidEndColorChooser() OVERRIDE; Nit: Indent 2, not 4 http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/browser.h... chrome/browser/ui/browser.h:1468: scoped_ptr<ColorChooser> color_chooser_; Nit: Add description (e.g. mentioning when this would be non-NULL) http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/color_cho... File chrome/browser/ui/color_chooser.h (right): http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser.h:16: class ColorChooser : public content::ColorChooser { Why do you need this second abstraction layer? Why can't the Create() function here be declared in the content header with the rest of content::ColorChooser, and eliminate this class entirely? http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_dialog.cc (right): http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.cc:29: BaseShellDialogImpl() { Nit: If you're going to explicitly call a superclass constructor, call all of them, in the correct order, before you init other members. (I suggest just dropping this call.) http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.cc:32: BeginRun(owning_window), Nit: This can go on the previous line http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.cc:52: static COLORREF custom_colors[16]; Don't do this here. This creates process-wide storage but accesses it in a thread-unsafe manner. Instead, declare both a static and a non-static version of this as class members. In the constructor, init the non-static member from the static one. Then pass the address of that in this function. Next, combine DidChooseColor() and DidDetach() into a single UI-thread callback that takes a bool |success| along with the |color| and |run_state|, and does something like: if (!listener_) return; EndRun(run_state); CopyColors(custom_colors_, g_custom_colors); if (success) listener_->DidChooseColor(color); listener_->DidEnd(); Finally write a CopyColors(COLORREF*, COLORREF*) function that copies the colors in one arg to the other to use in the above code as well as the constructor. This ensures thread-safety while still preserving a user's custom colors from one chooser to the next within the process. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.cc:53: COLORREF color = skia::SkColorToCOLORREF(params.color); Nit: Roll this into the assignment statement below http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_dialog.h (right): http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.h:30: explicit ColorChooserDialog(Listener* listener, Nit: No explicit http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.h:35: virtual bool IsRunning(HWND owning_hwnd) const OVERRIDE; Nit: Add "// BaseShellDialog:" comment above these two functions to clarify where they come from http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.h:44: }; Nit: DISALLOW_COPY_AND_ASSIGN()? http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_win.cc (right): http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_win.cc:13: class ColorChooserWin : public ColorChooser, Why is it that the GTK and Mac versions need to listen for the WebContents destruction but the Windows one doesn't? http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_win.cc:16: virtual void End() OVERRIDE {} Nit: Add "// ColorChooser:" and "//ColorChooserDialog::Listener:" comments above these groups http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_win.cc:23: int identifier, content::WebContents* tab, SkColor initial_color); Nit: One arg per line (3 places) http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_win.cc:26: virtual int GetIdentifier() const OVERRIDE { return identifier_; } Nit: Don't inline virtual functions http://codereview.chromium.org/9203001/diff/59002/content/public/browser/colo... File content/public/browser/color_chooser.h (right): http://codereview.chromium.org/9203001/diff/59002/content/public/browser/colo... content/public/browser/color_chooser.h:20: virtual int GetIdentifier() const = 0; Nit: Add comments about what these APIs mean.
http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:4129: tab, On 2012/02/29 01:40:07, Peter Kasting wrote: > Nit: Go ahead and put this arg on the previous line. Done. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/browser.c... chrome/browser/ui/browser.cc:4131: return static_cast<content::ColorChooser*>(color_chooser_.get()); On 2012/02/29 01:40:07, Peter Kasting wrote: > Nit: Make |color_chooser_| be of this type so you don't need to static_cast<>(). Done. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/browser.h File chrome/browser/ui/browser.h (right): http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/browser.h... chrome/browser/ui/browser.h:1019: int color_chooser_id, On 2012/02/29 01:40:07, Peter Kasting wrote: > Nit: Aligned with above arg, or all args (including first) indented 4 if that > won't fit Done. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/browser.h... chrome/browser/ui/browser.h:1021: virtual void DidEndColorChooser() OVERRIDE; On 2012/02/29 01:40:07, Peter Kasting wrote: > Nit: Indent 2, not 4 Done. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/browser.h... chrome/browser/ui/browser.h:1468: scoped_ptr<ColorChooser> color_chooser_; On 2012/02/29 01:40:07, Peter Kasting wrote: > Nit: Add description (e.g. mentioning when this would be non-NULL) Done. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/cocoa/col... File chrome/browser/ui/cocoa/color_chooser_mac.mm (right): http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:19: @interface ColorPanelBridge : NSObject<NSWindowDelegate> { On 2012/02/28 16:22:09, rsesek wrote: > @private Done. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:19: @interface ColorPanelBridge : NSObject<NSWindowDelegate> { On 2012/02/28 16:22:09, rsesek wrote: > Naming: bridges are usually C++ -> ObjC. I'd call this ColorPanelCocoa. Done. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:21: ColorChooserMac* chooser_; On 2012/02/28 16:22:09, rsesek wrote: > // weak, owns this Done. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:25: - (void)dealloc; On 2012/02/28 16:22:09, rsesek wrote: > No need to declare this Done. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:36: public: On 2012/02/28 16:22:09, rsesek wrote: > nit: indent 1 Done. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:50: private: On 2012/02/28 16:22:09, rsesek wrote: > nit: indent 1 Done. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:68: [[NSColorPanel sharedColorPanel] makeKeyAndOrderFront:NULL]; On 2012/02/28 16:22:09, rsesek wrote: > Use nil instead of NULL Done. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:124: - (void)windowWillClose:(NSNotification *)notification { On 2012/02/28 16:22:09, rsesek wrote: > nit: no space before * Done. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:129: - (void)didChooseColor:(NSColorPanel *)panel { On 2012/02/28 16:22:09, rsesek wrote: > here too Done. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:139: - (void)setColor:(NSColor *)color { On 2012/02/28 16:22:09, rsesek wrote: > and here Done. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/color_cho... File chrome/browser/ui/color_chooser.h (right): http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser.h:16: class ColorChooser : public content::ColorChooser { On 2012/02/29 01:40:07, Peter Kasting wrote: > Why do you need this second abstraction layer? Why can't the Create() function > here be declared in the content header with the rest of content::ColorChooser, > and eliminate this class entirely? I was thinking that too. Removed ColorChooser class. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_dialog.cc (right): http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.cc:29: BaseShellDialogImpl() { On 2012/02/29 01:40:07, Peter Kasting wrote: > Nit: If you're going to explicitly call a superclass constructor, call all of > them, in the correct order, before you init other members. (I suggest just > dropping this call.) Done. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.cc:32: BeginRun(owning_window), On 2012/02/29 01:40:07, Peter Kasting wrote: > Nit: This can go on the previous line Done. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.cc:52: static COLORREF custom_colors[16]; On 2012/02/29 01:40:07, Peter Kasting wrote: > Don't do this here. This creates process-wide storage but accesses it in a > thread-unsafe manner. > > Instead, declare both a static and a non-static version of this as class > members. In the constructor, init the non-static member from the static one. > Then pass the address of that in this function. > > Next, combine DidChooseColor() and DidDetach() into a single UI-thread callback > that takes a bool |success| along with the |color| and |run_state|, and does > something like: > > if (!listener_) > return; > EndRun(run_state); > CopyColors(custom_colors_, g_custom_colors); > if (success) > listener_->DidChooseColor(color); > listener_->DidEnd(); > > Finally write a CopyColors(COLORREF*, COLORREF*) function that copies the colors > in one arg to the other to use in the above code as well as the constructor. > > This ensures thread-safety while still preserving a user's custom colors from > one chooser to the next within the process. Done. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.cc:53: COLORREF color = skia::SkColorToCOLORREF(params.color); On 2012/02/29 01:40:07, Peter Kasting wrote: > Nit: Roll this into the assignment statement below Done. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_dialog.h (right): http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.h:30: explicit ColorChooserDialog(Listener* listener, On 2012/02/29 01:40:07, Peter Kasting wrote: > Nit: No explicit Done. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.h:35: virtual bool IsRunning(HWND owning_hwnd) const OVERRIDE; On 2012/02/29 01:40:07, Peter Kasting wrote: > Nit: Add "// BaseShellDialog:" comment above these two functions to clarify > where they come from Done. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.h:44: }; On 2012/02/29 01:40:07, Peter Kasting wrote: > Nit: DISALLOW_COPY_AND_ASSIGN()? Should I do DISALLOW_COPY_AND_ASSIGN(ExecuteOpenParams)? I did it but it didn't compile. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_win.cc (right): http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_win.cc:13: class ColorChooserWin : public ColorChooser, On 2012/02/29 01:40:07, Peter Kasting wrote: > Why is it that the GTK and Mac versions need to listen for the WebContents > destruction but the Windows one doesn't? I thought maybe the dialog would block the tab from closing, but I guess javascript is still running so it could close. Made it listen to WebContents destruction like the others. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_win.cc:16: virtual void End() OVERRIDE {} On 2012/02/29 01:40:07, Peter Kasting wrote: > Nit: Add "// ColorChooser:" and "//ColorChooserDialog::Listener:" comments above > these groups Done. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_win.cc:23: int identifier, content::WebContents* tab, SkColor initial_color); On 2012/02/29 01:40:07, Peter Kasting wrote: > Nit: One arg per line (3 places) Done. http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_win.cc:26: virtual int GetIdentifier() const OVERRIDE { return identifier_; } On 2012/02/29 01:40:07, Peter Kasting wrote: > Nit: Don't inline virtual functions Done. http://codereview.chromium.org/9203001/diff/59002/content/public/browser/colo... File content/public/browser/color_chooser.h (right): http://codereview.chromium.org/9203001/diff/59002/content/public/browser/colo... content/public/browser/color_chooser.h:20: virtual int GetIdentifier() const = 0; On 2012/02/29 01:40:07, Peter Kasting wrote: > Nit: Add comments about what these APIs mean. Done.
Hi, estade. Could you review the gtk part again. chrome/app/generated_resources.grd chrome/browser/ui/gtk/color_chooser_gtk.cc
I'm concerned about the interaction on Windows between "can only have one color chooser for a window at a time" and "script in the page can try to auto-close the existing chooser (which will fail because we can't programmatically close it on Windows) and then open a new one". What happens on Windows with the current implementation if script opens one chooser, then closes it and opens a second? http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_dialog.h (right): http://codereview.chromium.org/9203001/diff/59002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.h:44: }; On 2012/02/29 13:05:22, keishi1 wrote: > On 2012/02/29 01:40:07, Peter Kasting wrote: > > Nit: DISALLOW_COPY_AND_ASSIGN()? > > Should I do DISALLOW_COPY_AND_ASSIGN(ExecuteOpenParams)? > I did it but it didn't compile. Hmm, I guess you need to be able to copy these. Never mind. http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/browser.h File chrome/browser/ui/browser.h (right): http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/browser.h... chrome/browser/ui/browser.h:58: class ColorChooser; This declaration is bogus and should disappear. http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/color_cho... File chrome/browser/ui/color_chooser.h (right): http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/color_cho... chrome/browser/ui/color_chooser.h:16: class ColorChooser : public content::ColorChooser { This class is dead, please remove this file entirely http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_dialog.cc (right): http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.cc:17: static const COLORREF kColorBlack = 0x000000; You can drop this and all the initializers. The array will be zero-initialized automatically. http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.cc:61: CopyCustomColors(g_custom_colors, custom_colors_); You need to do this in the constructor like I asked, because that will run on the UI thread, and all access to |g_custom_colors| needs to happen on the same thread (unless you want locks, which you don't). http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.cc:90: memcpy(dst, src, sizeof(COLORREF) * 16); Nit: Use your constant or else arraysize() rather than 16. http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_dialog.h (right): http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.h:15: static const int kCustomColorCount = 16; Don't declare a static const at global scope in a header, as that creates it in every object that includes this header. Use a class-private static const that you define in the .cc file, or if that doesn't work, inline "16" directly into the declarations of the arrays below. http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_win.cc (right): http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_win.cc:42: int identifier, content::WebContents* tab, SkColor initial_color) { Nit: One arg per line http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_win.cc:47: int identifier, content::WebContents* tab, SkColor initial_color) Nit: One arg per line http://codereview.chromium.org/9203001/diff/69001/content/browser/tab_content... File content/browser/tab_contents/tab_contents.cc (right): http://codereview.chromium.org/9203001/diff/69001/content/browser/tab_content... content/browser/tab_contents/tab_contents.cc:1674: int color_chooser_id, const SkColor& color) { Nit: One arg per line. First arg on above line if possible. http://codereview.chromium.org/9203001/diff/69001/content/public/browser/colo... File content/public/browser/color_chooser.h (right): http://codereview.chromium.org/9203001/diff/69001/content/public/browser/colo... content/public/browser/color_chooser.h:17: // Abstraction object for color choosers for each platforms. Nit: platforms -> platform http://codereview.chromium.org/9203001/diff/69001/content/public/browser/colo... content/public/browser/color_chooser.h:21: int identifier, WebContents* tab, SkColor initial_color); Nit: One arg per line, first arg on above line, others indented even http://codereview.chromium.org/9203001/diff/69001/content/public/browser/colo... content/public/browser/color_chooser.h:25: // Returns the color chooser id that is unique per renderer process. Nit: How about providing more motivation for this ID, a la: // Returns a unique identifier for this chooser. Identifiers are unique // across a renderer process. This avoids race conditions in synchronizing // the browser and renderer processes. For example, if a renderer closes one // chooser and opens another, and simultaneously the user picks a color in the // first chooser, the IDs can be used to drop the "chose a color" message // rather than erroneously tell the renderer that the user picked a color in // the second chooser. http://codereview.chromium.org/9203001/diff/69001/content/public/browser/colo... content/public/browser/color_chooser.h:26: virtual int GetIdentifier() const = 0; This doesn't need to be virtual. All three subclasses implement this the same way, by returning |identifier_|. Instead, make |identifier_| a private member of this class and add an explicit one-arg constructor to set it; then make this function a unix_hacker()-style inlined const getter. http://codereview.chromium.org/9203001/diff/69001/content/renderer/renderer_w... File content/renderer/renderer_webcolorchooser_impl.cc (right): http://codereview.chromium.org/9203001/diff/69001/content/renderer/renderer_w... content/renderer/renderer_webcolorchooser_impl.cc:10: static unsigned GenerateColorChooserIdentifier() { Nit: Both this function and the static variable should be of type int, not unsigned, both to match the caller and because Google style bans "unsigned". http://codereview.chromium.org/9203001/diff/69001/content/renderer/renderer_w... content/renderer/renderer_webcolorchooser_impl.cc:16: RenderViewImpl* render_view, WebKit::WebColorChooserClient* client) Nit: One arg per line http://codereview.chromium.org/9203001/diff/69001/content/renderer/renderer_w... content/renderer/renderer_webcolorchooser_impl.cc:67: int color_chooser_id) { Nit: This arg will fit on the previous line
> I'm concerned about the interaction on Windows between "can only have one
color
> chooser for a window at a time" and "script in the page can try to auto-close
> the existing chooser (which will fail because we can't programmatically close
it
> on Windows) and then open a new one".
>
> What happens on Windows with the current implementation if script opens one
> chooser, then closes it and opens a second?
You're right, that will cause a problem.
This is the code that will cause a problem.
<script>
button.onclick = function() {
color1.click();
color2.click();
}
</script>
SelectFileDialog solves this by keeping a queue inside RenderViewImpl.
I will be making a new modeless color chooser for Windows+Chrome OS after this,
so I don't think its worth complicating the code just for this case.
Could we just ignore openColorChooser messages while the ColorChooser is open
for Windows?
http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):
http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/browser.h...
chrome/browser/ui/browser.h:58: class ColorChooser;
On 2012/03/01 20:44:18, Peter Kasting wrote:
> This declaration is bogus and should disappear.
Done.
http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/color_cho...
File chrome/browser/ui/color_chooser.h (right):
http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/color_cho...
chrome/browser/ui/color_chooser.h:16: class ColorChooser : public
content::ColorChooser {
On 2012/03/01 20:44:18, Peter Kasting wrote:
> This class is dead, please remove this file entirely
Done.
http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/views/col...
File chrome/browser/ui/views/color_chooser_dialog.cc (right):
http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/views/col...
chrome/browser/ui/views/color_chooser_dialog.cc:17: static const COLORREF
kColorBlack = 0x000000;
On 2012/03/01 20:44:18, Peter Kasting wrote:
> You can drop this and all the initializers. The array will be
zero-initialized
> automatically.
Done.
http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/views/col...
chrome/browser/ui/views/color_chooser_dialog.cc:61:
CopyCustomColors(g_custom_colors, custom_colors_);
On 2012/03/01 20:44:18, Peter Kasting wrote:
> You need to do this in the constructor like I asked, because that will run on
> the UI thread, and all access to |g_custom_colors| needs to happen on the same
> thread (unless you want locks, which you don't).
Done.
http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/views/col...
chrome/browser/ui/views/color_chooser_dialog.cc:90: memcpy(dst, src,
sizeof(COLORREF) * 16);
On 2012/03/01 20:44:18, Peter Kasting wrote:
> Nit: Use your constant or else arraysize() rather than 16.
Done.
http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/views/col...
File chrome/browser/ui/views/color_chooser_dialog.h (right):
http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/views/col...
chrome/browser/ui/views/color_chooser_dialog.h:15: static const int
kCustomColorCount = 16;
On 2012/03/01 20:44:18, Peter Kasting wrote:
> Don't declare a static const at global scope in a header, as that creates it
in
> every object that includes this header.
>
> Use a class-private static const that you define in the .cc file, or if that
> doesn't work, inline "16" directly into the declarations of the arrays below.
Done.
http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/views/col...
File chrome/browser/ui/views/color_chooser_win.cc (right):
http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/views/col...
chrome/browser/ui/views/color_chooser_win.cc:42: int identifier,
content::WebContents* tab, SkColor initial_color) {
On 2012/03/01 20:44:18, Peter Kasting wrote:
> Nit: One arg per line
Done.
http://codereview.chromium.org/9203001/diff/69001/chrome/browser/ui/views/col...
chrome/browser/ui/views/color_chooser_win.cc:47: int identifier,
content::WebContents* tab, SkColor initial_color)
On 2012/03/01 20:44:18, Peter Kasting wrote:
> Nit: One arg per line
Done.
http://codereview.chromium.org/9203001/diff/69001/content/browser/tab_content...
File content/browser/tab_contents/tab_contents.cc (right):
http://codereview.chromium.org/9203001/diff/69001/content/browser/tab_content...
content/browser/tab_contents/tab_contents.cc:1674: int color_chooser_id, const
SkColor& color) {
On 2012/03/01 20:44:18, Peter Kasting wrote:
> Nit: One arg per line. First arg on above line if possible.
Done.
http://codereview.chromium.org/9203001/diff/69001/content/public/browser/colo...
File content/public/browser/color_chooser.h (right):
http://codereview.chromium.org/9203001/diff/69001/content/public/browser/colo...
content/public/browser/color_chooser.h:17: // Abstraction object for color
choosers for each platforms.
On 2012/03/01 20:44:18, Peter Kasting wrote:
> Nit: platforms -> platform
Done.
http://codereview.chromium.org/9203001/diff/69001/content/public/browser/colo...
content/public/browser/color_chooser.h:21: int identifier, WebContents* tab,
SkColor initial_color);
On 2012/03/01 20:44:18, Peter Kasting wrote:
> Nit: One arg per line, first arg on above line, others indented even
Done.
http://codereview.chromium.org/9203001/diff/69001/content/public/browser/colo...
content/public/browser/color_chooser.h:25: // Returns the color chooser id that
is unique per renderer process.
On 2012/03/01 20:44:18, Peter Kasting wrote:
> Nit: How about providing more motivation for this ID, a la:
>
> // Returns a unique identifier for this chooser. Identifiers are unique
> // across a renderer process. This avoids race conditions in synchronizing
> // the browser and renderer processes. For example, if a renderer closes
one
> // chooser and opens another, and simultaneously the user picks a color in
the
> // first chooser, the IDs can be used to drop the "chose a color" message
> // rather than erroneously tell the renderer that the user picked a color in
> // the second chooser.
Done.
http://codereview.chromium.org/9203001/diff/69001/content/public/browser/colo...
content/public/browser/color_chooser.h:26: virtual int GetIdentifier() const =
0;
On 2012/03/01 20:44:18, Peter Kasting wrote:
> This doesn't need to be virtual. All three subclasses implement this the same
> way, by returning |identifier_|. Instead, make |identifier_| a private member
> of this class and add an explicit one-arg constructor to set it; then make
this
> function a unix_hacker()-style inlined const getter.
Done.
http://codereview.chromium.org/9203001/diff/69001/content/renderer/renderer_w...
File content/renderer/renderer_webcolorchooser_impl.cc (right):
http://codereview.chromium.org/9203001/diff/69001/content/renderer/renderer_w...
content/renderer/renderer_webcolorchooser_impl.cc:10: static unsigned
GenerateColorChooserIdentifier() {
On 2012/03/01 20:44:18, Peter Kasting wrote:
> Nit: Both this function and the static variable should be of type int, not
> unsigned, both to match the caller and because Google style bans "unsigned".
Done.
http://codereview.chromium.org/9203001/diff/69001/content/renderer/renderer_w...
content/renderer/renderer_webcolorchooser_impl.cc:16: RenderViewImpl*
render_view, WebKit::WebColorChooserClient* client)
On 2012/03/01 20:44:18, Peter Kasting wrote:
> Nit: One arg per line
Done.
http://codereview.chromium.org/9203001/diff/69001/content/renderer/renderer_w...
content/renderer/renderer_webcolorchooser_impl.cc:67: int color_chooser_id) {
On 2012/03/01 20:44:18, Peter Kasting wrote:
> Nit: This arg will fit on the previous line
Done.
On 2012/03/02 06:12:13, keishi1 wrote: > SelectFileDialog solves this by keeping a queue inside RenderViewImpl. I would like us to do the same thing as much as possible for select file dialogs versus color chooser dialogs. Copying the implementation is good. Sharing the code as much as possible is even better. Besides making it easier on maintainers and reviewers who don't have to worry about two similar features with different implementations and constraints, this also makes like slightly better for web developers because it's easier to explain and understand the browser's behavior.
On 2012/03/02 17:36:04, Peter Kasting wrote: > On 2012/03/02 06:12:13, keishi1 wrote: > > SelectFileDialog solves this by keeping a queue inside RenderViewImpl. > > I would like us to do the same thing as much as possible for select file dialogs > versus color chooser dialogs. Oh, and since I don't think select file dialogs need unique IDs, this suggests that perhaps if we did things identically color chooser dialogs might not need them either? That'd be nice too.
On 2012/03/02 17:37:08, Peter Kasting wrote: > On 2012/03/02 17:36:04, Peter Kasting wrote: > > On 2012/03/02 06:12:13, keishi1 wrote: > > > SelectFileDialog solves this by keeping a queue inside RenderViewImpl. > > > > I would like us to do the same thing as much as possible for select file > dialogs > > versus color chooser dialogs. > > Oh, and since I don't think select file dialogs need unique IDs, this suggests > that perhaps if we did things identically color chooser dialogs might not need > them either? That'd be nice too. The color chooser dialog is modeless so there is no code that can be reused with the modal select file dialog. As I explained in these slides https://docs.google.com/presentation/d/1qDtKr8RB3xgOTJWAlaehOjxo-tTZuR8fbqNb6... 1. The Windows color chooser is modal dialog just for this initial implementation. I've designed this mainly for modeless dialogs. 2. The color_chooser_ids are necessary for a modeless one per app dialog. I've seriously considered your advice but I don't think I can share any code with the select file dialog for these reasons.
On 2012/03/05 12:33:17, keishi1 wrote: > As I explained in these slides > https://docs.google.com/presentation/d/1qDtKr8RB3xgOTJWAlaehOjxo-tTZuR8fbqNb6... > 1. The Windows color chooser is modal dialog just for this initial > implementation. I've designed this mainly for modeless dialogs. > 2. The color_chooser_ids are necessary for a modeless one per app dialog. > > I've seriously considered your advice but I don't think I can share any code > with the select file dialog for these reasons. Surely any platform that can actually implement a modeless dialog can use that to implement a modal one. Furthermore, I am somewhat opposed to "modeless but one per app" as that's an unusual combination of constraints. If we want to force color choosers to be unique per app I'm not sure what we lose by making them modal. Finally, this is a complex change. I think there are serious benefits if we can re-use the code for an existing similar feature in addition to the benefits I already mentioned for web developers of having the browser UI behave consistently. For all these reasons, I think you should change the UX of this dialog to be modal and then implement it in terms of sharing code with the select file dialog.
Having a single modeless color picker is extremely common ux on mac.
On 2012/03/05 22:25:40, Nico wrote: > Having a single modeless color picker is extremely common ux on mac. How does the select file dialog work on Mac? Is it modal or modeless?
It depends. Save is usually window modal (though we'd like to make it tab modal in chrome eventually), open is usually app modal. I don't think that analogy is all that useful: opening and saving files is more a one-time thing while tweaking colors, line widths, etc is a lot more iterative. On Mar 5, 2012 2:29 PM, <pkasting@chromium.org> wrote: > On 2012/03/05 22:25:40, Nico wrote: > >> Having a single modeless color picker is extremely common ux on mac. >> > > How does the select file dialog work on Mac? Is it modal or modeless? > > http://codereview.chromium.**org/9203001/<http://codereview.chromium.org/9203... >
http://codereview.chromium.org/9203001/diff/77001/chrome/browser/ui/cocoa/col... File chrome/browser/ui/cocoa/color_chooser_mac.mm (right): http://codereview.chromium.org/9203001/diff/77001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:13: #import <Cocoa/Cocoa.h> nit: this goes after the .h and before the other includes http://codereview.chromium.org/9203001/diff/77001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:21: BOOL nonUserChange_; Document what this is for. http://codereview.chromium.org/9203001/diff/77001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:30: - (void)setColor:(NSColor*)color; Add a comment for this. http://codereview.chromium.org/9203001/diff/77001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:83: panel_.reset(); Does this close the color chooser when it's done?
On 2012/03/05 22:08:49, Peter Kasting wrote: > On 2012/03/05 12:33:17, keishi1 wrote: > > As I explained in these slides > > > https://docs.google.com/presentation/d/1qDtKr8RB3xgOTJWAlaehOjxo-tTZuR8fbqNb6... > > 1. The Windows color chooser is modal dialog just for this initial > > implementation. I've designed this mainly for modeless dialogs. > > 2. The color_chooser_ids are necessary for a modeless one per app dialog. > > > > I've seriously considered your advice but I don't think I can share any code > > with the select file dialog for these reasons. > > Surely any platform that can actually implement a modeless dialog can use that > to implement a modal one. > > Furthermore, I am somewhat opposed to "modeless but one per app" as that's an > unusual combination of constraints. If we want to force color choosers to be > unique per app I'm not sure what we lose by making them modal. > > Finally, this is a complex change. I think there are serious benefits if we can > re-use the code for an existing similar feature in addition to the benefits I > already mentioned for web developers of having the browser UI behave > consistently. > > For all these reasons, I think you should change the UX of this dialog to be > modal and then implement it in terms of sharing code with the select file > dialog. Modeless is a necessary because of the color pipette feature (pick color from the screen feature). Users will want to 1. open the color chooser 2. switch to tab with pretty colors 3. use color pipette to pick that color The color pipette feature is important because you can't create it with JavaScript so it will make the <input type=color> much more useful. The reason for the one per app limitation is 1. Mac can't have multiple NSColorPanels open 2. Many modeless dialogs open makes it hard to figure out which one is for which element I have already asked the UI leads about this modeless one per app color chooser dialog. They don't like the Windows color chooser dialog, so I will be making a better modeless one with views for Windows+ChromeOS next.
http://codereview.chromium.org/9203001/diff/77001/chrome/browser/ui/cocoa/col... File chrome/browser/ui/cocoa/color_chooser_mac.mm (right): http://codereview.chromium.org/9203001/diff/77001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:13: #import <Cocoa/Cocoa.h> On 2012/03/05 22:39:20, rsesek wrote: > nit: this goes after the .h and before the other includes Done. http://codereview.chromium.org/9203001/diff/77001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:21: BOOL nonUserChange_; On 2012/03/05 22:39:20, rsesek wrote: > Document what this is for. Done. http://codereview.chromium.org/9203001/diff/77001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:30: - (void)setColor:(NSColor*)color; On 2012/03/05 22:39:20, rsesek wrote: > Add a comment for this. Done. http://codereview.chromium.org/9203001/diff/77001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:83: panel_.reset(); On 2012/03/05 22:39:20, rsesek wrote: > Does this close the color chooser when it's done? No. According to Apple people I talked to on the WebKit bug, it is against Apple UI guidelines to close the NSColorPanel from the app. That is why I had to use "End" instead of "Close"
http://codereview.chromium.org/9203001/diff/84002/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_dialog.cc (right): http://codereview.chromium.org/9203001/diff/84002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.cc:62: if (success) { Nit: Simpler: BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(&ColorChooserDialog::DidChooseColor, this, success, skia::COLORREFToSkColor(cc.rgbResult), params.run_state)); http://codereview.chromium.org/9203001/diff/84002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.cc:74: void ColorChooserDialog::DidChooseColor(bool success, SkColor color, Nit: One arg per line http://codereview.chromium.org/9203001/diff/84002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.cc:86: memcpy(dst, src, arraysize(g_custom_colors)); You need to multiply by sizeof(COLORREF). http://codereview.chromium.org/9203001/diff/84002/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_dialog.h (right): http://codereview.chromium.org/9203001/diff/84002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.h:45: void ExecuteOpen(const ExecuteOpenParams& params); Nit: Add blank line above. Add comments about what these do. http://codereview.chromium.org/9203001/diff/84002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.h:49: static COLORREF g_custom_colors[16]; Nit: Add comments about what these are for and how they're used.
http://codereview.chromium.org/9203001/diff/84002/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_dialog.cc (right): http://codereview.chromium.org/9203001/diff/84002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.cc:62: if (success) { On 2012/03/09 00:45:53, Peter Kasting wrote: > Nit: Simpler: > > BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, > base::Bind(&ColorChooserDialog::DidChooseColor, this, success, > skia::COLORREFToSkColor(cc.rgbResult), params.run_state)); Done. http://codereview.chromium.org/9203001/diff/84002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.cc:74: void ColorChooserDialog::DidChooseColor(bool success, SkColor color, On 2012/03/09 00:45:53, Peter Kasting wrote: > Nit: One arg per line Done. http://codereview.chromium.org/9203001/diff/84002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.cc:86: memcpy(dst, src, arraysize(g_custom_colors)); On 2012/03/09 00:45:53, Peter Kasting wrote: > You need to multiply by sizeof(COLORREF). Done. http://codereview.chromium.org/9203001/diff/84002/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_dialog.h (right): http://codereview.chromium.org/9203001/diff/84002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.h:45: void ExecuteOpen(const ExecuteOpenParams& params); On 2012/03/09 00:45:53, Peter Kasting wrote: > Nit: Add blank line above. Add comments about what these do. Done. http://codereview.chromium.org/9203001/diff/84002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.h:49: static COLORREF g_custom_colors[16]; On 2012/03/09 00:45:53, Peter Kasting wrote: > Nit: Add comments about what these are for and how they're used. Done.
LGTM http://codereview.chromium.org/9203001/diff/92001/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_dialog.h (right): http://codereview.chromium.org/9203001/diff/92001/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.h:46: // Shows the color chooser dialog modal to |owner| and calls the result Nit: How about these comments and declarations. Note that DidChooseColor() has been renamed to DidCloseDialog() and its third arg changed to a const ref. // Called on the dialog thread to show the actual color chooser. This is // shown modal to |params.owner|. Once it's closed, calls back to // DidCloseDialog() on the UI thread. void ExecuteOpen(const ExecuteOpenParams& params); // Called on the UI thread when a color chooser is closed. |chose_color| is // true if the user actually chose a color, in which case |color| is the // chosen color. Calls back to the |listener_| (if applicable) to notify it // of the results, and copies the modified array of |custom_colors_| back to // |g_custom_colors| so future dialogs will see the changes. void DidCloseDialog(bool chose_color, SkColor color, const RunState& run_state); // Copies the array of colors in |src| to |dst|. void CopyCustomColors(COLORREF* src, COLORREF* dst); // The user's custom colors. Kept process-wide so that they can be persisted // from one dialog invocation to the next. static COLORREF g_custom_colors[16]; // A copy of the custom colors for the current dialog to display and modify. // This allows us to safely access the colors even if multiple windows are // simultaneously showing color choosers (which would cause thread safety // problems if we gave them direct handles to |g_custom_colors|). COLORREF custom_colors_[16]; // The listener to notify when the user closes the dialog. This may be set to // NULL before the color chooser is closed, signalling that the listener no // longer cares about the outcome. Listener* listener_;
http://codereview.chromium.org/9203001/diff/92001/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_dialog.h (right): http://codereview.chromium.org/9203001/diff/92001/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_dialog.h:46: // Shows the color chooser dialog modal to |owner| and calls the result On 2012/03/09 02:17:44, Peter Kasting wrote: > Nit: How about these comments and declarations. Note that DidChooseColor() has > been renamed to DidCloseDialog() and its third arg changed to a const ref. > > // Called on the dialog thread to show the actual color chooser. This is > // shown modal to |params.owner|. Once it's closed, calls back to > // DidCloseDialog() on the UI thread. > void ExecuteOpen(const ExecuteOpenParams& params); > > // Called on the UI thread when a color chooser is closed. |chose_color| is > // true if the user actually chose a color, in which case |color| is the > // chosen color. Calls back to the |listener_| (if applicable) to notify it > // of the results, and copies the modified array of |custom_colors_| back to > // |g_custom_colors| so future dialogs will see the changes. > void DidCloseDialog(bool chose_color, > SkColor color, > const RunState& run_state); > > // Copies the array of colors in |src| to |dst|. > void CopyCustomColors(COLORREF* src, COLORREF* dst); > > // The user's custom colors. Kept process-wide so that they can be persisted > // from one dialog invocation to the next. > static COLORREF g_custom_colors[16]; > > // A copy of the custom colors for the current dialog to display and modify. > // This allows us to safely access the colors even if multiple windows are > // simultaneously showing color choosers (which would cause thread safety > // problems if we gave them direct handles to |g_custom_colors|). > COLORREF custom_colors_[16]; > > // The listener to notify when the user closes the dialog. This may be set to > // NULL before the color chooser is closed, signalling that the listener no > // longer cares about the outcome. > Listener* listener_; Thanks. This is better. Done.
Hi thakis@ and estade@ Could you look at the patch again? I am still hoping to squeeze this into M19. On 2012/03/09 02:30:09, keishi1 wrote: > http://codereview.chromium.org/9203001/diff/92001/chrome/browser/ui/views/col... > File chrome/browser/ui/views/color_chooser_dialog.h (right): > > http://codereview.chromium.org/9203001/diff/92001/chrome/browser/ui/views/col... > chrome/browser/ui/views/color_chooser_dialog.h:46: // Shows the color chooser > dialog modal to |owner| and calls the result > On 2012/03/09 02:17:44, Peter Kasting wrote: > > Nit: How about these comments and declarations. Note that DidChooseColor() > has > > been renamed to DidCloseDialog() and its third arg changed to a const ref. > > > > // Called on the dialog thread to show the actual color chooser. This is > > // shown modal to |params.owner|. Once it's closed, calls back to > > // DidCloseDialog() on the UI thread. > > void ExecuteOpen(const ExecuteOpenParams& params); > > > > // Called on the UI thread when a color chooser is closed. |chose_color| is > > // true if the user actually chose a color, in which case |color| is the > > // chosen color. Calls back to the |listener_| (if applicable) to notify it > > // of the results, and copies the modified array of |custom_colors_| back to > > // |g_custom_colors| so future dialogs will see the changes. > > void DidCloseDialog(bool chose_color, > > SkColor color, > > const RunState& run_state); > > > > // Copies the array of colors in |src| to |dst|. > > void CopyCustomColors(COLORREF* src, COLORREF* dst); > > > > // The user's custom colors. Kept process-wide so that they can be > persisted > > // from one dialog invocation to the next. > > static COLORREF g_custom_colors[16]; > > > > // A copy of the custom colors for the current dialog to display and modify. > > // This allows us to safely access the colors even if multiple windows are > > // simultaneously showing color choosers (which would cause thread safety > > // problems if we gave them direct handles to |g_custom_colors|). > > COLORREF custom_colors_[16]; > > > > // The listener to notify when the user closes the dialog. This may be set > to > > // NULL before the color chooser is closed, signalling that the listener no > > // longer cares about the outcome. > > Listener* listener_; > > Thanks. This is better. Done.
Mac bits still look fine. Having a test would still be nice.
lgtm % nits http://codereview.chromium.org/9203001/diff/99001/chrome/browser/ui/cocoa/col... File chrome/browser/ui/cocoa/color_chooser_mac.mm (right): http://codereview.chromium.org/9203001/diff/99001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:24: ColorChooserMac* chooser_; // weak, owns this nit: two spaces after ; http://codereview.chromium.org/9203001/diff/99001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:96: if (self = [super init]) { nit: clang may need you to have an extra set of () around this condition to make it explicit that the assignment is intended
http://codereview.chromium.org/9203001/diff/99001/chrome/browser/ui/cocoa/col... File chrome/browser/ui/cocoa/color_chooser_mac.mm (right): http://codereview.chromium.org/9203001/diff/99001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:24: ColorChooserMac* chooser_; // weak, owns this On 2012/03/13 03:03:58, rsesek wrote: > nit: two spaces after ; Done. http://codereview.chromium.org/9203001/diff/99001/chrome/browser/ui/cocoa/col... chrome/browser/ui/cocoa/color_chooser_mac.mm:96: if (self = [super init]) { On 2012/03/13 03:03:58, rsesek wrote: > nit: clang may need you to have an extra set of () around this condition to make > it explicit that the assignment is intended Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/9203001/106002
Try job failure for 9203001-106002 (retry) on linux_chromeos for step "compile" (clobber build). It's a second try, previously, step "compile" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Had to add stubs for android and aura. estade@ isn't responding so could evan@ take a look at the gtk code? chrome/app/generated_resources.grd chrome/browser/ui/gtk/color_chooser_gtk.cc And could yfriedman@ and ben@ look at the stub classes for android and aura? chrome/browser/ui/android/color_chooser_android.cc chrome/browser/ui/views/ash/color_chooser_aura.cc Thanks.
On 2012/03/13 12:23:30, keishi1 wrote: > Had to add stubs for android and aura. > > estade@ isn't responding so could evan@ take a look at the gtk code? > > chrome/app/generated_resources.grd > chrome/browser/ui/gtk/color_chooser_gtk.cc > > And could yfriedman@ and ben@ look at the stub classes for android and aura? > > chrome/browser/ui/android/color_chooser_android.cc > chrome/browser/ui/views/ash/color_chooser_aura.cc > > Thanks. lgtm Android stub is fine.
http://codereview.chromium.org/9203001/diff/108031/chrome/browser/ui/gtk/colo... File chrome/browser/ui/gtk/color_chooser_gtk.cc (right): http://codereview.chromium.org/9203001/diff/108031/chrome/browser/ui/gtk/colo... chrome/browser/ui/gtk/color_chooser_gtk.cc:49: g_signal_connect(G_OBJECT( You don't need these G_OBJECT casts; g_signal_connect takes a void*. http://codereview.chromium.org/9203001/diff/108031/chrome/browser/ui/gtk/colo... chrome/browser/ui/gtk/color_chooser_gtk.cc:50: GTK_COLOR_SELECTION_DIALOG(color_selection_dialog_)->ok_button), Multiple times in this file, you're accessing raw struct members. This ability is going away Real Soon Now and gtk users are supposed to transition to either dedicated accessors or gobject properties. You can access these with g_object_get(). For example, the page http://developer.gnome.org/gtk/2.24/GtkColorSelectionDialog.html names the properties "ok-button", "color-selection", etc. You can access these properties with g_object_get().
http://codereview.chromium.org/9203001/diff/108031/chrome/browser/ui/gtk/colo... File chrome/browser/ui/gtk/color_chooser_gtk.cc (right): http://codereview.chromium.org/9203001/diff/108031/chrome/browser/ui/gtk/colo... chrome/browser/ui/gtk/color_chooser_gtk.cc:49: g_signal_connect(G_OBJECT( On 2012/03/13 17:50:17, Elliot Glaysher wrote: > You don't need these G_OBJECT casts; g_signal_connect takes a void*. Done. http://codereview.chromium.org/9203001/diff/108031/chrome/browser/ui/gtk/colo... chrome/browser/ui/gtk/color_chooser_gtk.cc:50: GTK_COLOR_SELECTION_DIALOG(color_selection_dialog_)->ok_button), On 2012/03/13 17:50:17, Elliot Glaysher wrote: > Multiple times in this file, you're accessing raw struct members. This ability > is going away Real Soon Now and gtk users are supposed to transition to either > dedicated accessors or gobject properties. You can access these with > g_object_get(). > > For example, the page > http://developer.gnome.org/gtk/2.24/GtkColorSelectionDialog.html names the > properties "ok-button", "color-selection", etc. You can access these properties > with g_object_get(). Done.
thank you. lgtm.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/9203001/110001
Can't apply patch for file chrome/browser/ui/select_file_dialog.h. While running patch -p1 --forward --force; patching file chrome/browser/ui/select_file_dialog.h Hunk #1 FAILED at 1. Hunk #3 succeeded at 26 (offset 1 line). 1 out of 3 hunks FAILED -- saving rejects to file chrome/browser/ui/select_file_dialog.h.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/9203001/110004
Try job failure for 9203001-110004 (retry) on linux_rel for steps "ui_tests, browser_tests, unit_tests, content_unittests". It's a second try, previously, steps "ui_tests, browser_tests, unit_tests, content_unittests" failed. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/keishi@chromium.org/9203001/113006
Change committed as 126889 |
