Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10442020:
Initial implementation of ColorChooser for Aura. (Closed)
|
Created:
8 years, 7 months ago by Jun Mukai Modified:
8 years, 6 months ago CC:
chromium-reviews, yoshiki, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionInitial implementation of ColorChooser for Aura.
BUG=121837
TEST=manually verified, see crbug.com/121837#c14
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=144111
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : lower case #Patch Set 5 : remove an unnecessary method #
Total comments: 6
Patch Set 6 : move to ui/views/color_chooser #Patch Set 7 : fix #
Total comments: 12
Patch Set 8 : #
Total comments: 8
Patch Set 9 : #Patch Set 10 : fix a silly mistake of windows build break #Patch Set 11 : minor fix for marker positions in case that color picker is invoked twice #Patch Set 12 : rebase #
Total comments: 44
Patch Set 13 : fix #
Total comments: 16
Patch Set 14 : fix #Patch Set 15 : . #Patch Set 16 : . #
Total comments: 12
Patch Set 17 : #Patch Set 18 : fix a wrong comment #
Total comments: 2
Patch Set 19 : . #
Total comments: 2
Patch Set 20 : GetInitiallyFocusedView() #Patch Set 21 : rebase #Messages
Total messages: 45 (0 generated)
See the screenshot in comment#13 of crbug.com/121837 (http://chromium.googlecode.com/issues/attachment?aid=1218370013000&name=Scree...) See also a brief design memo: https://docs.google.com/a/google.com/document/d/1ISJYI5p32vxtoJxZDkbhQXWyQBLK...
So, I think the color chooser view and its listener should move down into views. Can you do that first (leave the rest of the bindings in chrome for the time being) and then I'll be able to provide more comments. http://codereview.chromium.org/10442020/diff/6002/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_listener.h (right): http://codereview.chromium.org/10442020/diff/6002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_listener.h:15: virtual void DidChooseColor(SkColor color) = 0; OnColorChosen
http://codereview.chromium.org/10442020/diff/6002/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_listener.h (right): http://codereview.chromium.org/10442020/diff/6002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_listener.h:13: class ColorChooserListener { please add a protected dtor. http://codereview.chromium.org/10442020/diff/6002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_listener.h:16: virtual void DidEnd() = 0; nit: may be OnColorChooserDialogClosed() or something?
Also moved views code to ui/views/color_chooser/ http://codereview.chromium.org/10442020/diff/6002/chrome/browser/ui/views/col... File chrome/browser/ui/views/color_chooser_listener.h (right): http://codereview.chromium.org/10442020/diff/6002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_listener.h:13: class ColorChooserListener { On 2012/05/25 16:51:39, tfarina wrote: > please add a protected dtor. Done. http://codereview.chromium.org/10442020/diff/6002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_listener.h:15: virtual void DidChooseColor(SkColor color) = 0; On 2012/05/25 16:00:39, Ben Goodger (Google) wrote: > OnColorChosen Done. http://codereview.chromium.org/10442020/diff/6002/chrome/browser/ui/views/col... chrome/browser/ui/views/color_chooser_listener.h:16: virtual void DidEnd() = 0; On 2012/05/25 16:51:39, tfarina wrote: > nit: may be OnColorChooserDialogClosed() or something? Done.
http://codereview.chromium.org/10442020/diff/10002/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_listener.h (right): http://codereview.chromium.org/10442020/diff/10002/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_listener.h:13: class ColorChooserListener { VIEWS_EXPORT? http://codereview.chromium.org/10442020/diff/10002/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_listener.h:13: class ColorChooserListener { namespace views { ... } // namespace views http://codereview.chromium.org/10442020/diff/10002/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_listener.h:15: ColorChooserListener() {} nit: please, no ctor. It isn't needed, you can't instantiate an abstract class. http://codereview.chromium.org/10442020/diff/10002/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_listener.h:16: virtual ~ColorChooserListener() {} nit: can't you make this protected? We seem to be pretty consistent on ui/views/ about making dtor of listeners protected.
http://codereview.chromium.org/10442020/diff/10002/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_view.h (right): http://codereview.chromium.org/10442020/diff/10002/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.h:23: class VIEWS_EXPORT ColorChooserView : public views::WidgetDelegateView, namespace views { ... } // namespace views http://codereview.chromium.org/10442020/diff/10002/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.h:29: void End(); End() seems very vague, and doesn't match with the implementation, that just nulls the listener.
http://codereview.chromium.org/10442020/diff/10002/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_listener.h (right): http://codereview.chromium.org/10442020/diff/10002/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_listener.h:13: class ColorChooserListener { On 2012/05/25 18:09:50, tfarina wrote: > VIEWS_EXPORT? Done. http://codereview.chromium.org/10442020/diff/10002/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_listener.h:13: class ColorChooserListener { On 2012/05/25 18:09:50, tfarina wrote: > namespace views { > > ... > > } // namespace views Done. http://codereview.chromium.org/10442020/diff/10002/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_listener.h:15: ColorChooserListener() {} On 2012/05/25 18:09:50, tfarina wrote: > nit: please, no ctor. It isn't needed, you can't instantiate an abstract class. Done. http://codereview.chromium.org/10442020/diff/10002/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_listener.h:16: virtual ~ColorChooserListener() {} On 2012/05/25 18:09:50, tfarina wrote: > nit: can't you make this protected? We seem to be pretty consistent on ui/views/ > about making dtor of listeners protected. Done. http://codereview.chromium.org/10442020/diff/10002/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_view.h (right): http://codereview.chromium.org/10442020/diff/10002/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.h:23: class VIEWS_EXPORT ColorChooserView : public views::WidgetDelegateView, On 2012/05/25 18:12:23, tfarina wrote: > namespace views { > > ... > > } // namespace views Done. http://codereview.chromium.org/10442020/diff/10002/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.h:29: void End(); On 2012/05/25 18:12:23, tfarina wrote: > End() seems very vague, and doesn't match with the implementation, that just > nulls the listener. Changed to "OnOwningWindowClosed". Also changed method names a bit.
http://codereview.chromium.org/10442020/diff/15001/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_listener.h (right): http://codereview.chromium.org/10442020/diff/15001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_listener.h:21: protected: nit: indent one space. http://codereview.chromium.org/10442020/diff/15001/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_view.h (right): http://codereview.chromium.org/10442020/diff/15001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.h:22: class VIEWS_EXPORT ColorChooserView : public views::WidgetDelegateView, nit: remove views:: throughout in this file and in the source file. http://codereview.chromium.org/10442020/diff/15001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.h:49: virtual ui::ModalType GetModalType() const OVERRIDE; nit: please include compiler_specific.h for OVERRIDE. http://codereview.chromium.org/10442020/diff/15001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.h:64: }; DISALLOW_COPY_AND_ASSIGN and basictypes.h
Also could you change the TEST= to explain to QA how to verify this change? Instead of saying that you verified manually?
http://codereview.chromium.org/10442020/diff/15001/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_listener.h (right): http://codereview.chromium.org/10442020/diff/15001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_listener.h:21: protected: On 2012/05/25 19:37:38, tfarina wrote: > nit: indent one space. Done. http://codereview.chromium.org/10442020/diff/15001/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_view.h (right): http://codereview.chromium.org/10442020/diff/15001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.h:22: class VIEWS_EXPORT ColorChooserView : public views::WidgetDelegateView, On 2012/05/25 19:37:38, tfarina wrote: > nit: remove views:: throughout in this file and in the source file. Done. http://codereview.chromium.org/10442020/diff/15001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.h:49: virtual ui::ModalType GetModalType() const OVERRIDE; On 2012/05/25 19:37:38, tfarina wrote: > nit: please include compiler_specific.h for OVERRIDE. Done. http://codereview.chromium.org/10442020/diff/15001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.h:64: }; On 2012/05/25 19:37:38, tfarina wrote: > DISALLOW_COPY_AND_ASSIGN and basictypes.h Done.
On 2012/05/25 19:40:02, tfarina wrote: > Also could you change the TEST= to explain to QA how to verify this change? > Instead of saying that you verified manually? Hmm, I don't think TEST= line fits its verification, rather I want to put it to crbug.com/121837 because this CL doesn't solve the issue fully. This CL provides a new UI, but the UI invocation has been turned off in WebKit -- we need another WebKit patch for QAs to verify the behavior.
On 2012/05/25 20:08:22, Jun Mukai wrote: > On 2012/05/25 19:40:02, tfarina wrote: > > Also could you change the TEST= to explain to QA how to verify this change? > > Instead of saying that you verified manually? > > Hmm, I don't think TEST= line fits its verification, rather I want to put it to > crbug.com/121837 because this CL doesn't solve the issue fully. This CL > provides a new UI, but the UI invocation has been turned off in WebKit -- we > need another WebKit patch for QAs to verify the behavior. Wrote a note as http://code.google.com/p/chromium/issues/detail?id=121837#c14 and referring it in TEST= line.
On 2012/05/25 20:18:10, Jun Mukai wrote: > Wrote a note as http://code.google.com/p/chromium/issues/detail?id=121837#c14 > and referring it in TEST= line. Thanks is fine by me. ;)
On 2012/05/25 20:22:16, tfarina wrote: > On 2012/05/25 20:18:10, Jun Mukai wrote: > > Wrote a note as http://code.google.com/p/chromium/issues/detail?id=121837#c14 > > and referring it in TEST= line. > Thanks is fine by me. ;) That is*
Added Peter as a reviewer, who once reviewed the keishi's color chooser code. Can you take a look at this?
Peter, could you take time to review this CL?
http://codereview.chromium.org/10442020/diff/20001/chrome/browser/ui/views/co... File chrome/browser/ui/views/color_chooser_aura.cc (right): http://codereview.chromium.org/10442020/diff/20001/chrome/browser/ui/views/co... chrome/browser/ui/views/color_chooser_aura.cc:22: content::WebContents* tab_; members after methods http://codereview.chromium.org/10442020/diff/20001/chrome/browser/ui/views/co... chrome/browser/ui/views/color_chooser_aura.cc:32: }; DISALLOW_COPY_AND_ASSIGN http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_view.cc (right): http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:255: listener_ = NULL; is it valid for this window to still exist with the window that opened it closed? seems like this window should be destroyed when the window that opened it closes.
color_chooser_view.cc frequently interchanges float and SkScalar types (e.g. passing one to a function that expects the other, etc.). Skia doesn't guarantee this will work. You need to either make the types match (e.g. use SkScalar in more places) or use SkFloatToScalar/SkScalarToFloat in more places, or both. (Use SkIntToScalar if you're ever converting an int to an SkScalar.) Can you give screenshots of how this looks? http://codereview.chromium.org/10442020/diff/20001/chrome/browser/ui/views/co... File chrome/browser/ui/views/color_chooser_win.cc (right): http://codereview.chromium.org/10442020/diff/20001/chrome/browser/ui/views/co... chrome/browser/ui/views/color_chooser_win.cc:16: class ColorChooserWin : public content::ColorChooser, Nit: Alphabetical order http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_view.cc (right): http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:23: const int kHueBarWidth = 20; Nit Blank line above http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:30: return UTF8ToUTF16(base::StringPrintf("#%02x%02x%02x", Nit: ASCIIToUTF16() http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:73: : chooser_view_(chooser_view), level_(0) { Nit: One initializer per line (2 places) http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:86: return gfx::Size(kHueBarWidth + kHueIndicatorSize * 4, kSaturationValueSize); Nit: This * 4 is magic, what does it mean? (I think it comes from kHueIndicatorSize actually being a radius, so there's 2 indicators * (2 radius values per indicator diameter); better naming of the constants, e.g. kHueIndicatorRadiusPx, could help here, as could comments somewhere that describe the control layout so we know there's an indicator on each side of the bar. Maybe using a constant for the diameter instead of the radius?) http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:91: // bightness/value are 100% Nit: Comments should be complete sentences (initial capital, trailing period) http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:92: hsv[1] = 1.0; Nit: Use SK_Scalar1 (2 places) http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:100: hsv[0] = 360 * static_cast<float>(y) / height(); Nit: 360.0f * y / height() http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:121: chooser_view_->OnHueChosen(360 * static_cast<float>(level_) / height()); Nit: 360.0f * level_ / height() http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:157: if (hue_ == hue) Tiny nit: Early return or using {} are both fine, but make this function and HueView::OnHueChanged() do the same thing. http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:167: gfx::Size size = GetPreferredSize(); Nit: const& ? Or just use kSaturationValueSize directly in this function. http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:197: // The background is very dark at the bottom of the view. Use white Nit: "Use a white" http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:202: canvas->DrawCircle( Nit: Preferred linebreaking: canvas->DrawCircle(marker_position_, kSaturationValueIndicatorSize, circle_paint); http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:234: new BoxLayout(BoxLayout::kHorizontal, 0, 0, kMarginWidth)); Nit: See preferred linebreaking above http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:247: FROM_HERE, Nit: Can go on prior line http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:310: key_event.key_code() == ui::VKEY_ESCAPE) { Nit: Can avoid {} if you reverse conditional and early-return http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_view.h (right): http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.h:23: // Pure views implementation of color chooser UI. More comments would be nice, especially something describing the layout briefly. http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.h:24: class VIEWS_EXPORT ColorChooserView : public WidgetDelegateView, Nit: Alphabetical order
http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_view.h (right): http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.h:24: class VIEWS_EXPORT ColorChooserView : public WidgetDelegateView, On 2012/06/18 21:24:03, Peter Kasting wrote: > Nit: Alphabetical order I know the order doesn't matter, but I tend to think that there is some precedence order (implicit) here. The ColorChooserView IS-A WidgetDelegateView and that information is more important/relevant than knowing that it's a TextfieldController (when grepping?). But that is just how I see this, other might have different opinion.
I tend to always put the concrete class first. -Ben On Mon, Jun 18, 2012 at 2:45 PM, <tfarina@chromium.org> wrote: > > http://codereview.chromium.**org/10442020/diff/20001/ui/** > views/color_chooser/color_**chooser_view.h<http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/color_chooser_view.h> > File ui/views/color_chooser/color_**chooser_view.h (right): > > http://codereview.chromium.**org/10442020/diff/20001/ui/** > views/color_chooser/color_**chooser_view.h#newcode24<http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/color_chooser_view.h#newcode24> > ui/views/color_chooser/color_**chooser_view.h:24: class VIEWS_EXPORT > ColorChooserView : public WidgetDelegateView, > On 2012/06/18 21:24:03, Peter Kasting wrote: > >> Nit: Alphabetical order >> > I know the order doesn't matter, but I tend to think that there is some > precedence order (implicit) here. The ColorChooserView IS-A > WidgetDelegateView and that information is more important/relevant than > knowing that it's a TextfieldController (when grepping?). But that is > just how I see this, other might have different opinion. > > http://codereview.chromium.**org/10442020/<http://codereview.chromium.org/104... >
http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_view.h (right): http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.h:24: class VIEWS_EXPORT ColorChooserView : public WidgetDelegateView, On 2012/06/18 21:45:41, tfarina wrote: > On 2012/06/18 21:24:03, Peter Kasting wrote: > > Nit: Alphabetical order > I know the order doesn't matter, but I tend to think that there is some > precedence order (implicit) here. The ColorChooserView IS-A WidgetDelegateView > and that information is more important/relevant than knowing that it's a > TextfieldController (when grepping?). It IS-A TextfieldController too. That's what inheritance means. The style guide is not explicit, but for e.g. #includes (another case where we can have a list of strings at the same scope/indentation level), the direction is to be alphabetical; applying this rule to subclass names seems reasonable too, in the absence of a clear and easy-to-define alternative complete ordering.
On Mon, Jun 18, 2012 at 9:48 PM, <pkasting@chromium.org> wrote: > > http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... > File ui/views/color_chooser/color_chooser_view.h (right): > > http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... > ui/views/color_chooser/color_chooser_view.h:24: class VIEWS_EXPORT > ColorChooserView : public WidgetDelegateView, > On 2012/06/18 21:45:41, tfarina wrote: >> >> On 2012/06/18 21:24:03, Peter Kasting wrote: >> > Nit: Alphabetical order >> I know the order doesn't matter, but I tend to think that there is > > some >> >> precedence order (implicit) here. The ColorChooserView IS-A > > WidgetDelegateView >> >> and that information is more important/relevant than knowing that it's > > a >> >> TextfieldController (when grepping?). > > > It IS-A TextfieldController too. Â That's what inheritance means. > > The style guide is not explicit, but for e.g. #includes (another case > where we can have a list of strings at the same scope/indentation > level), the direction is to be alphabetical; applying this rule to > subclass names seems reasonable too, in the absence of a clear and > easy-to-define alternative complete ordering. > But we have been consistent about this order, at least inheriting from WidgetDelegateView/WidgetDelegate/DialogDelegate/DialogDelegateView/View, we usually inherit from these classes first then the others. I don't know why diverging from this now is good. Although I lease the decision to you guys, you and Ben ;) I just I like to keep the code consistent whenever we can, and this is introducing a inconsistency IMO. -- Thiago
On 2012/06/19 00:59:36, tfarina wrote: > But we have been consistent about this order, at least inheriting from > WidgetDelegateView/WidgetDelegate/DialogDelegate/DialogDelegateView/View, > we usually inherit from these classes first then the others. I have never heard of this rule, either as an author or reviewer. By contrast, I've asked for alphabetical superclass declarations for years.
On Mon, Jun 18, 2012 at 10:02 PM, <pkasting@chromium.org> wrote: > On 2012/06/19 00:59:36, tfarina wrote: >> >> But we have been consistent about this order, at least inheriting from >> WidgetDelegateView/WidgetDelegate/DialogDelegate/DialogDelegateView/View, >> we usually inherit from these classes first then the others. > > > I have never heard of this rule, either as an author or reviewer. Â By > contrast, > I've asked for alphabetical superclass declarations for years. > The way I see this is the same as when we do cross-platform. i.e: class DialogGtk : public Dialog, public Delegate { }; class DialogView : public Dialog, public Delegate { }; You are not going to inherit first from Delegate instead of Dialog :) -- Thiago
+1. At the very least in Views this is the convention. -Ben On Mon, Jun 18, 2012 at 6:07 PM, Thiago Farina <tfarina@chromium.org> wrote: > On Mon, Jun 18, 2012 at 10:02 PM, <pkasting@chromium.org> wrote: > > On 2012/06/19 00:59:36, tfarina wrote: > >> > >> But we have been consistent about this order, at least inheriting from > >> > WidgetDelegateView/WidgetDelegate/DialogDelegate/DialogDelegateView/View, > >> we usually inherit from these classes first then the others. > > > > > > I have never heard of this rule, either as an author or reviewer. By > > contrast, > > I've asked for alphabetical superclass declarations for years. > > > The way I see this is the same as when we do cross-platform. i.e: > > class DialogGtk : public Dialog, > public Delegate { > }; > > class DialogView : public Dialog, > public Delegate { > }; > > You are not going to inherit first from Delegate instead of Dialog :) > > -- > Thiago >
On 2012/06/19 01:07:03, tfarina wrote:
> The way I see this is the same as when we do cross-platform. i.e:
>
> class DialogGtk : public Dialog,
> public Delegate {
> };
>
> class DialogView : public Dialog,
> public Delegate {
> };
>
> You are not going to inherit first from Delegate instead of Dialog :)
I don't see why not. I would ask for someone to inherit from Delegate first.
There is no semantic meaning associated with the order in which superclasses are
listed. Therefore there is no particular value to placing one first.
Ben's rule makes some sense if we actually tried to enforce the style guide's
rule about Interface classes w.r.t multiple inheritance ( :( ), but even then it
leaves the ordering of all the Interface classes unspecified.
On Mon, Jun 18, 2012 at 10:09 PM, <pkasting@chromium.org> wrote: > On 2012/06/19 01:07:03, tfarina wrote: >> >> The way I see this is the same as when we do cross-platform. i.e: > > >> class DialogGtk : public Dialog, >> Â Â Â Â Â Â Â Â Â Â Â Â Â public Delegate { >> }; > > >> class DialogView : public Dialog, >> Â Â Â Â Â Â Â Â Â Â Â Â Â Â public Delegate { >> }; > > >> You are not going to inherit first from Delegate instead of Dialog :) > > > I don't see why not. Â I would ask for someone to inherit from Delegate > first. > > There is no semantic meaning associated with the order in which superclasses > are > listed. Â Therefore there is no particular value to placing one first. > For me the value is associated with the search you do when grepping. And knowing forehand that your dialog inherits from this View. Is my Dialog a WidgetDelegate or a DialogDelegate? If it doesn't inherit first from one of these, then I'll have to open each file and look to see and answer this question. I don't care about the order of the Listeners/Delegates/Observers though ;). The only one that I think matters is the Dialog one. And I think it makes sense and is consistent in views case. I might not make sense with the other APIs/frameworks ;) And Ben seems to agree with this ;) -- Thiago
Agreed. The concrete base is typically the one I identify the object as "being". The interfaces I'll oftne group if they're related, otherwise it's phases of the moon. Mostly it's only important to me that .cc definition order match .h order. Let's take the path of least resistance and just follow this convention as it is already widely applied. -Ben On Mon, Jun 18, 2012 at 6:15 PM, Thiago Farina <tfarina@chromium.org> wrote: > On Mon, Jun 18, 2012 at 10:09 PM, <pkasting@chromium.org> wrote: > > On 2012/06/19 01:07:03, tfarina wrote: > >> > >> The way I see this is the same as when we do cross-platform. i.e: > > > > > >> class DialogGtk : public Dialog, > >> public Delegate { > >> }; > > > > > >> class DialogView : public Dialog, > >> public Delegate { > >> }; > > > > > >> You are not going to inherit first from Delegate instead of Dialog :) > > > > > > I don't see why not. I would ask for someone to inherit from Delegate > > first. > > > > There is no semantic meaning associated with the order in which > superclasses > > are > > listed. Therefore there is no particular value to placing one first. > > > For me the value is associated with the search you do when grepping. > And knowing forehand that your dialog inherits from this View. > > Is my Dialog a WidgetDelegate or a DialogDelegate? If it doesn't > inherit first from one of these, then I'll have to open each file and > look to see and answer this question. > > I don't care about the order of the Listeners/Delegates/Observers > though ;). The only one that I think matters is the Dialog one. > > And I think it makes sense and is consistent in views case. I might > not make sense with the other APIs/frameworks ;) > > And Ben seems to agree with this ;) > > -- > Thiago >
http://codereview.chromium.org/10442020/diff/20001/chrome/browser/ui/views/co... File chrome/browser/ui/views/color_chooser_aura.cc (right): http://codereview.chromium.org/10442020/diff/20001/chrome/browser/ui/views/co... chrome/browser/ui/views/color_chooser_aura.cc:22: content::WebContents* tab_; On 2012/06/18 17:00:46, Ben Goodger (Google) wrote: > members after methods Done. http://codereview.chromium.org/10442020/diff/20001/chrome/browser/ui/views/co... chrome/browser/ui/views/color_chooser_aura.cc:32: }; On 2012/06/18 17:00:46, Ben Goodger (Google) wrote: > DISALLOW_COPY_AND_ASSIGN Done. http://codereview.chromium.org/10442020/diff/20001/chrome/browser/ui/views/co... File chrome/browser/ui/views/color_chooser_win.cc (right): http://codereview.chromium.org/10442020/diff/20001/chrome/browser/ui/views/co... chrome/browser/ui/views/color_chooser_win.cc:16: class ColorChooserWin : public content::ColorChooser, On 2012/06/18 21:24:03, Peter Kasting wrote: > Nit: Alphabetical order Done. http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_view.cc (right): http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:23: const int kHueBarWidth = 20; On 2012/06/18 21:24:03, Peter Kasting wrote: > Nit Blank line above Done. http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:30: return UTF8ToUTF16(base::StringPrintf("#%02x%02x%02x", On 2012/06/18 21:24:03, Peter Kasting wrote: > Nit: ASCIIToUTF16() Done. http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:73: : chooser_view_(chooser_view), level_(0) { On 2012/06/18 21:24:03, Peter Kasting wrote: > Nit: One initializer per line (2 places) Done. http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:86: return gfx::Size(kHueBarWidth + kHueIndicatorSize * 4, kSaturationValueSize); On 2012/06/18 21:24:03, Peter Kasting wrote: > Nit: This * 4 is magic, what does it mean? > > (I think it comes from kHueIndicatorSize actually being a radius, so there's 2 > indicators * (2 radius values per indicator diameter); better naming of the > constants, e.g. kHueIndicatorRadiusPx, could help here, as could comments > somewhere that describe the control layout so we know there's an indicator on > each side of the bar. Maybe using a constant for the diameter instead of the > radius?) Changed as: - use -Radius rather than Size, for the radius of indicator - declare -Size = -Radius * 2 - put a comment of why we enlarge the width here Please let me know if this still doesn't make sense. http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:91: // bightness/value are 100% On 2012/06/18 21:24:03, Peter Kasting wrote: > Nit: Comments should be complete sentences (initial capital, trailing period) Done. http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:92: hsv[1] = 1.0; On 2012/06/18 21:24:03, Peter Kasting wrote: > Nit: Use SK_Scalar1 (2 places) Done. http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:100: hsv[0] = 360 * static_cast<float>(y) / height(); On 2012/06/18 21:24:03, Peter Kasting wrote: > Nit: 360.0f * y / height() Done. http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:121: chooser_view_->OnHueChosen(360 * static_cast<float>(level_) / height()); On 2012/06/18 21:24:03, Peter Kasting wrote: > Nit: 360.0f * level_ / height() Done. http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:157: if (hue_ == hue) On 2012/06/18 21:24:03, Peter Kasting wrote: > Tiny nit: Early return or using {} are both fine, but make this function and > HueView::OnHueChanged() do the same thing. Using if {} here. http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:167: gfx::Size size = GetPreferredSize(); On 2012/06/18 21:24:03, Peter Kasting wrote: > Nit: const& ? Or just use kSaturationValueSize directly in this function. Used kSaturationValueSize http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:197: // The background is very dark at the bottom of the view. Use white On 2012/06/18 21:24:03, Peter Kasting wrote: > Nit: "Use a white" Done. http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:202: canvas->DrawCircle( On 2012/06/18 21:24:03, Peter Kasting wrote: > Nit: Preferred linebreaking: > > canvas->DrawCircle(marker_position_, kSaturationValueIndicatorSize, > circle_paint); Done. http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:234: new BoxLayout(BoxLayout::kHorizontal, 0, 0, kMarginWidth)); On 2012/06/18 21:24:03, Peter Kasting wrote: > Nit: See preferred linebreaking above Done. http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:247: FROM_HERE, On 2012/06/18 21:24:03, Peter Kasting wrote: > Nit: Can go on prior line Done. http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:255: listener_ = NULL; On 2012/06/18 17:00:46, Ben Goodger (Google) wrote: > is it valid for this window to still exist with the window that opened it > closed? seems like this window should be destroyed when the window that opened > it closes. When the owning window is closed, the color chooser is closed automatically because of it's window-modal dialog. In that case, OnOwningWindowClosed() is called before WindowClosing(), to guard calling OnColorChooserDialogClosed(). http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:310: key_event.key_code() == ui::VKEY_ESCAPE) { On 2012/06/18 21:24:03, Peter Kasting wrote: > Nit: Can avoid {} if you reverse conditional and early-return Done. http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_view.h (right): http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.h:23: // Pure views implementation of color chooser UI. On 2012/06/18 21:24:03, Peter Kasting wrote: > More comments would be nice, especially something describing the layout briefly. Done. http://codereview.chromium.org/10442020/diff/20001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.h:24: class VIEWS_EXPORT ColorChooserView : public WidgetDelegateView, On 2012/06/19 00:48:40, Peter Kasting wrote: > On 2012/06/18 21:45:41, tfarina wrote: > > On 2012/06/18 21:24:03, Peter Kasting wrote: > > > Nit: Alphabetical order > > I know the order doesn't matter, but I tend to think that there is some > > precedence order (implicit) here. The ColorChooserView IS-A WidgetDelegateView > > and that information is more important/relevant than knowing that it's a > > TextfieldController (when grepping?). > > It IS-A TextfieldController too. That's what inheritance means. > > The style guide is not explicit, but for e.g. #includes (another case where we > can have a list of strings at the same scope/indentation level), the direction > is to be alphabetical; applying this rule to subclass names seems reasonable > too, in the absence of a clear and easy-to-define alternative complete ordering. Either is fine to me. As far as I see in the discussion, I keep this order for the consistency with views files.
You did not correct the issues with assuming SkScalar and float are the same type. Please see the top of my first mail with comments. http://codereview.chromium.org/10442020/diff/34001/chrome/browser/ui/views/co... File chrome/browser/ui/views/color_chooser_aura.cc (right): http://codereview.chromium.org/10442020/diff/34001/chrome/browser/ui/views/co... chrome/browser/ui/views/color_chooser_aura.cc:30: content::WebContents* tab_; Nit: Add comments about these, especially the ownership of |view_|. http://codereview.chromium.org/10442020/diff/34001/chrome/browser/ui/views/co... File chrome/browser/ui/views/color_chooser_win.cc (right): http://codereview.chromium.org/10442020/diff/34001/chrome/browser/ui/views/co... chrome/browser/ui/views/color_chooser_win.cc:17: public content::WebContentsObserver, What is this base class used for? http://codereview.chromium.org/10442020/diff/34001/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_view.cc (right): http://codereview.chromium.org/10442020/diff/34001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:27: const int kSaturationValueIndicatorRadius = 3; Nit: Looking at the screenshot, all the circles are an even number of pixels in diameter. This makes it impossible to tell which precise pixel is at the center of the saturation/value circle, and looks buggy on the edges of the hue bar. Can you use indicators that are an odd number of pixels on an edge, so that there's a distinct center pixel? These might not be circles, e.g. you could use "> <" symbols around the hue bar. http://codereview.chromium.org/10442020/diff/34001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:95: // In the hue bar, bightness and value for the color should be always 100%. Nit: bightness [sic] -> saturation http://codereview.chromium.org/10442020/diff/34001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:102: paint.setColor(SK_ColorBLACK); This has the unfortunate side effect of making completely invisible the actual selected color. Could we either not draw this bar, or draw it with a gap in the middle so the user can still see the color, or something? http://codereview.chromium.org/10442020/diff/34001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:236: new BoxLayout(BoxLayout::kHorizontal, 0, 0, kMarginWidth)); Nit: For consistency: container->SetLayoutManager(new BoxLayout(BoxLayout::kHorizontal, 0, 0, kMarginWidth)); http://codereview.chromium.org/10442020/diff/34001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:249: base::Bind( Nit: Why did you indent this and the next lines so far? They were better as they were. http://codereview.chromium.org/10442020/diff/34001/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_view.h (right): http://codereview.chromium.org/10442020/diff/34001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.h:27: // http://dev.chromium.org/developers/design-documents/aura/ash-color-chooser Nit: It's easy for external documentation to be moved, changed, or deleted without realizing that source code refers to it. Consider putting everything you need to say directly into this comment without external references.
http://codereview.chromium.org/10442020/diff/34001/chrome/browser/ui/views/co... File chrome/browser/ui/views/color_chooser_aura.cc (right): http://codereview.chromium.org/10442020/diff/34001/chrome/browser/ui/views/co... chrome/browser/ui/views/color_chooser_aura.cc:30: content::WebContents* tab_; On 2012/06/19 21:10:31, Peter Kasting wrote: > Nit: Add comments about these, especially the ownership of |view_|. Done. http://codereview.chromium.org/10442020/diff/34001/chrome/browser/ui/views/co... File chrome/browser/ui/views/color_chooser_win.cc (right): http://codereview.chromium.org/10442020/diff/34001/chrome/browser/ui/views/co... chrome/browser/ui/views/color_chooser_win.cc:17: public content::WebContentsObserver, On 2012/06/19 21:10:31, Peter Kasting wrote: > What is this base class used for? I didn't introduce this and not so sure what was the original motivation. According to the code, I guess that's used only for the 'web_contents()' methods which holds the "tab" in the constructor... Removed and introduced tab_ field as _aura.cc does. http://codereview.chromium.org/10442020/diff/34001/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_view.cc (right): http://codereview.chromium.org/10442020/diff/34001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:27: const int kSaturationValueIndicatorRadius = 3; On 2012/06/19 21:10:31, Peter Kasting wrote: > Nit: Looking at the screenshot, all the circles are an even number of pixels in > diameter. This makes it impossible to tell which precise pixel is at the center > of the saturation/value circle, and looks buggy on the edges of the hue bar. > Can you use indicators that are an odd number of pixels on an edge, so that > there's a distinct center pixel? These might not be circles, e.g. you could use > "> <" symbols around the hue bar. Changed to the triangular indicators as you said. http://codereview.chromium.org/10442020/diff/34001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:95: // In the hue bar, bightness and value for the color should be always 100%. On 2012/06/19 21:10:31, Peter Kasting wrote: > Nit: bightness [sic] -> saturation Done. http://codereview.chromium.org/10442020/diff/34001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:102: paint.setColor(SK_ColorBLACK); On 2012/06/19 21:10:31, Peter Kasting wrote: > This has the unfortunate side effect of making completely invisible the actual > selected color. Could we either not draw this bar, or draw it with a gap in the > middle so the user can still see the color, or something? Removed. I tried 50% saturation/value but it's still hard to see the backing hue. Thanks. http://codereview.chromium.org/10442020/diff/34001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:236: new BoxLayout(BoxLayout::kHorizontal, 0, 0, kMarginWidth)); On 2012/06/19 21:10:31, Peter Kasting wrote: > Nit: For consistency: > > container->SetLayoutManager(new BoxLayout(BoxLayout::kHorizontal, 0, 0, > kMarginWidth)); Done. http://codereview.chromium.org/10442020/diff/34001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:249: base::Bind( On 2012/06/19 21:10:31, Peter Kasting wrote: > Nit: Why did you indent this and the next lines so far? They were better as > they were. Done. http://codereview.chromium.org/10442020/diff/34001/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_view.h (right): http://codereview.chromium.org/10442020/diff/34001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.h:27: // http://dev.chromium.org/developers/design-documents/aura/ash-color-chooser On 2012/06/19 21:10:31, Peter Kasting wrote: > Nit: It's easy for external documentation to be moved, changed, or deleted > without realizing that source code refers to it. Consider putting everything > you need to say directly into this comment without external references. Then removed the link itself. I think the existing description is enough to understand what will be there.
You still did not fix the SkScalar<->float issues. I don't want to be saying this three times :(
On 2012/06/20 18:32:26, Peter Kasting wrote: > You still did not fix the SkScalar<->float issues. I don't want to be saying > this three times :( I am so sorry for my misunderstanding. Uploaded patchset 16, which totally replaced float by SkScalar and their operations by SkScalar...() functions.
LGTM http://codereview.chromium.org/10442020/diff/41001/chrome/browser/ui/views/co... File chrome/browser/ui/views/color_chooser_aura.cc (right): http://codereview.chromium.org/10442020/diff/41001/chrome/browser/ui/views/co... chrome/browser/ui/views/color_chooser_aura.cc:30: // The web contents invoking the color chooser. No ownership. Nit: Does "No ownership" here mean "We don't own this, and it outlives us"? Or can this ever be deleted while we're alive? Also in color_chooser_win.cc. http://codereview.chromium.org/10442020/diff/41001/chrome/browser/ui/views/co... chrome/browser/ui/views/color_chooser_aura.cc:33: // The actual view of the color chooser. No ownership because this will be Nit: Does the second sentence here mean "This owns itself," or "We create a widget that owns this and will delete it on closure"? Either one would probably be clearer. http://codereview.chromium.org/10442020/diff/41001/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_view.cc (right): http://codereview.chromium.org/10442020/diff/41001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:27: const int kSaturationValueIndicatorRadius = 3; The saturation/value square still has an even-pixel-width circle indicator. Is there a way to use some sort of odd-pixel-width indicator? Perhaps a sort of crosshair with the middle removed? http://codereview.chromium.org/10442020/diff/41001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:236: SkScalar value = SkScalarDiv( Nit: Extra space http://codereview.chromium.org/10442020/diff/41001/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_view.h (right): http://codereview.chromium.org/10442020/diff/41001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.h:67: // The pointer to the current color chooser for callbacks. No ownership. Nit: Does "No ownership" mean "This outlives us"? http://codereview.chromium.org/10442020/diff/41001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.h:70: // The view of hue chooser. No ownership because its ownership is taken in Nit: How about just putting this comment above the three of these: // Child views. These are owned as part of the normal view hierarchy.
http://codereview.chromium.org/10442020/diff/41001/chrome/browser/ui/views/co... File chrome/browser/ui/views/color_chooser_aura.cc (right): http://codereview.chromium.org/10442020/diff/41001/chrome/browser/ui/views/co... chrome/browser/ui/views/color_chooser_aura.cc:30: // The web contents invoking the color chooser. No ownership. On 2012/06/21 18:46:16, Peter Kasting wrote: > Nit: Does "No ownership" here mean "We don't own this, and it outlives us"? Or > can this ever be deleted while we're alive? Also in color_chooser_win.cc. it will outlives this class. Added it to the comment (and _win.cc too) http://codereview.chromium.org/10442020/diff/41001/chrome/browser/ui/views/co... chrome/browser/ui/views/color_chooser_aura.cc:33: // The actual view of the color chooser. No ownership because this will be On 2012/06/21 18:46:16, Peter Kasting wrote: > Nit: Does the second sentence here mean "This owns itself," or "We create a > widget that owns this and will delete it on closure"? Either one would probably > be clearer. views parent normally takes care of the lifetime of its children. Wrote this explicitly. Thanks for pointing. http://codereview.chromium.org/10442020/diff/41001/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_view.cc (right): http://codereview.chromium.org/10442020/diff/41001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:27: const int kSaturationValueIndicatorRadius = 3; On 2012/06/21 18:46:16, Peter Kasting wrote: > The saturation/value square still has an even-pixel-width circle indicator. Is > there a way to use some sort of odd-pixel-width indicator? Perhaps a sort of > crosshair with the middle removed? Changed to crosshair. http://codereview.chromium.org/10442020/diff/41001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:236: SkScalar value = SkScalarDiv( On 2012/06/21 18:46:16, Peter Kasting wrote: > Nit: Extra space Done. http://codereview.chromium.org/10442020/diff/41001/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_view.h (right): http://codereview.chromium.org/10442020/diff/41001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.h:67: // The pointer to the current color chooser for callbacks. No ownership. On 2012/06/21 18:46:16, Peter Kasting wrote: > Nit: Does "No ownership" mean "This outlives us"? Here I expected another class should take care of its lifetime, and Browser class will take care in case of <input type=color>. Wrote this more explicitly. http://codereview.chromium.org/10442020/diff/41001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.h:70: // The view of hue chooser. No ownership because its ownership is taken in On 2012/06/21 18:46:16, Peter Kasting wrote: > Nit: How about just putting this comment above the three of these: > > // Child views. These are owned as part of the normal view hierarchy. Done.
Ben, please take another look at this as the OWNER of ui/views.
http://codereview.chromium.org/10442020/diff/46003/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_view.cc (right): http://codereview.chromium.org/10442020/diff/46003/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:306: void ColorChooserView::OnOwningWindowClosed() { I don't think that this should be done here. it should be up to the caller to do this.
http://codereview.chromium.org/10442020/diff/46003/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_view.cc (right): http://codereview.chromium.org/10442020/diff/46003/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:306: void ColorChooserView::OnOwningWindowClosed() { On 2012/06/22 16:17:59, Ben Goodger (Google) wrote: > I don't think that this should be done here. it should be up to the caller to do > this. Moved to color_chooser_aura.cc
http://codereview.chromium.org/10442020/diff/55001/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_view.cc (right): http://codereview.chromium.org/10442020/diff/55001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:300: base::Bind(&View::RequestFocus, base::Unretained(textfield_))); instead of doing this, override GetInitiallyFocusedView and return textfield_;
http://codereview.chromium.org/10442020/diff/55001/ui/views/color_chooser/col... File ui/views/color_chooser/color_chooser_view.cc (right): http://codereview.chromium.org/10442020/diff/55001/ui/views/color_chooser/col... ui/views/color_chooser/color_chooser_view.cc:300: base::Bind(&View::RequestFocus, base::Unretained(textfield_))); On 2012/06/25 15:56:44, Ben Goodger (Google) wrote: > instead of doing this, override GetInitiallyFocusedView and return textfield_; Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/10442020/58002
Failed to apply patch for chrome/chrome_browser.gypi: While running patch -p1 --forward --force; patching file chrome/chrome_browser.gypi Hunk #1 FAILED at 3680. 1 out of 1 hunk FAILED -- saving rejects to file chrome/chrome_browser.gypi.rej
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/10442020/66002
Change committed as 144111 |
