Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
62873007:
[Toolbar] Base toolbar button class with background images for button states (Closed)
|
Created:
7 years, 1 month ago by Greg Billock Modified:
7 years ago CC:
chromium-reviews, tfarina, oshima+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Toolbar] Base toolbar button class with background images for button states
R=pkasting@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238218
Patch Set 1 #Patch Set 2 : Graphics from UI. #
Total comments: 16
Patch Set 3 : Switch over back and forward buttons #Patch Set 4 : Rebase to 74483002 #Patch Set 5 : Fix rebase #Patch Set 6 : Add home button #
Total comments: 62
Patch Set 7 : Rebase #Patch Set 8 : . #Patch Set 9 : Account for maximized state. #
Total comments: 20
Patch Set 10 : tweaks #
Total comments: 4
Patch Set 11 : . #Patch Set 12 : Unit test update #Patch Set 13 : Add back button subclass #
Total comments: 14
Patch Set 14 : . #Patch Set 15 : Rebase #
Total comments: 4
Patch Set 16 : Remove 'overridden' #Patch Set 17 : overriding #Messages
Total messages: 33 (0 generated)
On 2013/11/14 21:49:02, Greg Billock wrote: Here's the sense of what a toolbar button base class ends up looking like, I think. As far as I can tell, the toolbar is the only user of ButtonDropDown, so I think we want to end up pretty much basing the base class on that. I'm not sure whether we want to leave it in controls/ or just wholesale move it but keep the same name or what, exactly. Any feelings on that? In all, I think this ends up being a good move: we end up with simpler-to-understand graphics, and probably end up saving a few binary bytes on the repetition we had. Conceivably even enough to offset for all the new backgrounds. :-) So what do you think? Shall we commit something like this to establish a base toolbar button drawing class?
https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/reload_button.cc (right): https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/reload_button.cc:64: Nit: Extra semicolon and newline https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/reload_button.cc:258: std::swap(images_[i], alternate_images_[i]); Nit: It seems like at the least we could name these arrays better (|reload_images_| and |stop_images_|?) and avoid the swap() call. I'd think we could do even better than this, but I can't actually figure out from just staring at things how ReloadButton actually paints the selected image set before your patch. https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/reload_button.cc:259: SetImage((views::Button::ButtonState)i, images_[i]); Nit: Don't use C-style casts https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.h (right): https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:26: class SiteChipView : public ToolbarButton { Is the site chip going to want a dropdown? If not, should we attempt to separate that functionality from the base toolbar button class, or make the site chip not inherit from ToolbarButton? (Originally I had only envisioned reusing the graphics to start with, and not the code from the other toolbar buttons; I'm not opposed to code reuse if it makes sense, but I want to make sure we really want the site chip to use all that code.) https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/toolbar_button.h (right): https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_button.h:22: // See ButtonDropDown. ButtonDropDown no longer exists, AFAICT. https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_button.h:23: class ToolbarButton : public views::LabelButton, Would we also make the "wrench" button (as it's still called in the code) be an instance of this? It seems on the surface like a good move.
This is looking pretty ok to me now. B/F/R buttons look good. Reload is definitely simpler without all the array swapping. Need to do home and app menu and wrench menu. If this toolbar change ever commits, there's some reload and wrench menu refactoring I may split out to an intermediate. https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/reload_button.cc (right): https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/reload_button.cc:64: On 2013/11/15 03:28:58, Peter Kasting wrote: > Nit: Extra semicolon and newline Done. https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/reload_button.cc:258: std::swap(images_[i], alternate_images_[i]); On 2013/11/15 03:28:58, Peter Kasting wrote: > Nit: It seems like at the least we could name these arrays better > (|reload_images_| and |stop_images_|?) and avoid the swap() call. > > I'd think we could do even better than this, but I can't actually figure out > from just staring at things how ReloadButton actually paints the selected image > set before your patch. It's in the guts of ButtonDropDown > ImageButton, and the existing use isn't super-pretty -- there's a variable overload of a private superclass member. The works. I agree this will improve with LabelButton. https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/reload_button.cc:259: SetImage((views::Button::ButtonState)i, images_[i]); On 2013/11/15 03:28:58, Peter Kasting wrote: > Nit: Don't use C-style casts Done. https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.h (right): https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:26: class SiteChipView : public ToolbarButton { On 2013/11/15 03:28:58, Peter Kasting wrote: > Is the site chip going to want a dropdown? If not, should we attempt to > separate that functionality from the base toolbar button class, or make the site > chip not inherit from ToolbarButton? > > (Originally I had only envisioned reusing the graphics to start with, and not > the code from the other toolbar buttons; I'm not opposed to code reuse if it > makes sense, but I want to make sure we really want the site chip to use all > that code.) The problem is the diamond inheritance -- we have the ButtonDropDown stuff, and the (much simpler) border stuff. I think there's a good case for using ~ButtonDropDown as a superclass -- I wouldn't be surprised to see the site chip end up growing a menu if we keep it as a toolbar button. But in the meantime, it's probably a good plan to just aim for a toolbar button superclass aside from site chip needs, as the hope is it makes the buttons more uniform as well as making the experiment easy. https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/toolbar_button.h (right): https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_button.h:22: // See ButtonDropDown. On 2013/11/15 03:28:58, Peter Kasting wrote: > ButtonDropDown no longer exists, AFAICT. ui/views/controls/button/button_dropdown.h still exists, but as I put, I'm suspicious that we may want to just basically move it here to the toolbar, as it has no other users. What do you think? Is it a useful piece of generic kit? Or should we just move/rename it as here? I could delete ui/views/controls/button/button_dropdown.h as part of this change, or tear off the move as a refactor assuming the initial toolbar/ move CL ever makes it through the cq.... https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_button.h:23: class ToolbarButton : public views::LabelButton, On 2013/11/15 03:28:58, Peter Kasting wrote: > Would we also make the "wrench" button (as it's still called in the code) be an > instance of this? It seems on the surface like a good move. Yes. I only did the one here to make sure we have the same plan, but if this is the right direction let's do them all. I might keep just Reload in this cl to make it a bit more digestible, though. I need to take out the site chip stuff, too. Mainly the current state is a WIP to make sure we have the same design in mind and that it seems like it'll work.
https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.h (right): https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:26: class SiteChipView : public ToolbarButton { On 2013/11/15 19:36:50, Greg Billock wrote: > The problem is the diamond inheritance -- we have the ButtonDropDown stuff, and > the (much simpler) border stuff. Could we make the toolbar button base class include painting the border and supporting clicks, and then add dropdown support via composition with a class that adds the menu and the like? Or perhaps have a simple inheritance hierarchy like: ToolbarButton (provides border and some basic behaviors) / \ ToolbarDropdownButton SiteChip | [All other toolbar buttons] I definitely don't want diamond inheritance. It may be that you're right and including the dropdown functionality in the base class -- and then simply not enabling it in the site chip -- is the best route. I leave it to your judgment. https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/toolbar_button.h (right): https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_button.h:22: // See ButtonDropDown. On 2013/11/15 19:36:50, Greg Billock wrote: > On 2013/11/15 03:28:58, Peter Kasting wrote: > > ButtonDropDown no longer exists, AFAICT. > > ui/views/controls/button/button_dropdown.h still exists, but as I put, I'm > suspicious that we may want to just basically move it here to the toolbar, as it > has no other users. What do you think? My guiding principle is "scope things narrowly", so I would move rather than copying this. https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_button.h:23: class ToolbarButton : public views::LabelButton, On 2013/11/15 19:36:50, Greg Billock wrote: > On 2013/11/15 03:28:58, Peter Kasting wrote: > > Would we also make the "wrench" button (as it's still called in the code) be > an > > instance of this? It seems on the surface like a good move. > > Yes. I only did the one here to make sure we have the same plan, but if this is > the right direction let's do them all. I might keep just Reload in this cl to > make it a bit more digestible, though. I need to take out the site chip stuff, > too. K. Either way is OK, whatever ends up being simpler.
https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.h (right): https://codereview.chromium.org/62873007/diff/240001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:26: class SiteChipView : public ToolbarButton { On 2013/11/15 22:11:56, Peter Kasting wrote: > On 2013/11/15 19:36:50, Greg Billock wrote: > > The problem is the diamond inheritance -- we have the ButtonDropDown stuff, > and > > the (much simpler) border stuff. > > Could we make the toolbar button base class include painting the border and > supporting clicks, and then add dropdown support via composition with a class > that adds the menu and the like? > > Or perhaps have a simple inheritance hierarchy like: > > ToolbarButton (provides border and some basic behaviors) > / \ > ToolbarDropdownButton SiteChip > | > [All other toolbar buttons] > > I definitely don't want diamond inheritance. It may be that you're right and > including the dropdown functionality in the base class -- and then simply not > enabling it in the site chip -- is the best route. I leave it to your judgment. The border stuff is pretty simple, so I think that'd be easier mix-in. My intuition is being guided by a kind of "what if this had already been done" -- if there was a ToolbarButton, it'd be pretty similar to what this CL has, I think, since all existing buttons have menus. So in that world, we'd make the site chip view based on ToolbarButton with perhaps a small chnage to make it so the superclass wouldn't show the menu. I still think it is <50% that we end up with a toolbar button for the site chip. (I think it much more likely we end up with something in the location bar.) So I'm inclined to not lean too heavily on the site chip for the class design.
https://codereview.chromium.org/62873007/diff/1470001/chrome/app/theme/theme_... File chrome/app/theme/theme_resources.grd (right): https://codereview.chromium.org/62873007/diff/1470001/chrome/app/theme/theme_... chrome/app/theme/theme_resources.grd:858: <structure type="chrome_scaled_image" name="IDR_RELOAD" file="common/browser_reload_normal.png" /> Are these old resources still referenced anywhere? If this change removes the references, then also remove these lines from the .grd and delete the image files. Note: Since trybots don't like binary file changes, you may want to ultimately do this CL as three commits: (1) Land new image files (2) All code changes and .grd changes (3) Remove unreferenced old image files and rename new files to match final .grd names You may wonder what the "rename" bit of step (3) means. I think ultimately we should just call these resources "RELOAD{_DIMMED}" instead of "RELOAD_INNER{_DIMMED}" (same with Stop below). But the .grd names should match the file names, and if you do the three-step process above using filenames without "inner" in step 1, you'll break the existing behavior. This isn't so bad if you already have approval on everything and are landing all three changes within seconds of each other, but it's probably better to land with unique names in (1), then use the correct .grd names without changing the filenames in step (2), then make the filenames match in step (3). https://codereview.chromium.org/62873007/diff/1470001/chrome/app/theme/theme_... chrome/app/theme/theme_resources.grd:923: <structure type="chrome_scaled_image" name="IDR_STOP_INNER_D" file="common/browser_stop_inner_disabled.png" /> Nit: Be consistent in naming with how you named the reload images. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/home_button.h (right): https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/home_button.h:14: class HomeImageButton : public ToolbarButton { Nit: Incidentally, we maybe should rename this HomeButton at some point. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/reload_button.cc (right): https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/reload_button.cc:56: Nit: Extra blank line. I actually preferred the old way of putting "}" on its own line, which would be consistent with the constructor too. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/reload_button.cc:237: LOG(INFO) << "Switch to mode " << mode; Don't land this https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/reload_button.cc:239: if (mode == MODE_RELOAD) { Nit: Shorter: SetImage(views::Button::STATE_NORMAL, *(tp->GetImageSkiaNamed( (mode == MODE_RELOAD) ? IDR_RELOAD : IDR_STOP))); SetImage(views::Button::STATE_DISABLED, *(tp->GetImageSkiaNamed( (mode == MODE_RELOAD) ? IDR_RELOAD_D : IDR_STOP_D))); https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/reload_button.cc:240: SetImage(views::CustomButton::STATE_NORMAL, Nit: Qualify this with Button:: instead of CustomButton:: (2 places) https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/reload_button.cc:241: *(tp->GetImageSkiaNamed(IDR_RELOAD))); Did you mean to be using IDR_RELOAD_INNER here and similar below? (But see my .grd comments) https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/reload_button.h (right): https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/reload_button.h:106: gfx::ImageSkia images_[STATE_COUNT]; These two arrays are dead and should be removed. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:23: // TODO(gbillock): replace with virtual method. Why do we need a virtual method? https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:32: show_menu_factory_(this) {} Nit: I slightly prefer the original "linebreak between braces" style of this file (3 places) https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:51: border->SetPainter(true, views::Button::STATE_NORMAL, NULL); Don't we also need to set painters for the other focused states, as well as all disabled states? Otherwise they'll fall back to the default painters in LabelButtonBorder(). https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:141: LocationBarView::GetItemPadding(), Nit: Prefer to put this entire first arg on the same line if there's a way to linebreak that will accommodate it. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:143: } else { Nit: No else after return. If you reverse the conditional, you can avoid {} as well. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.h (right): https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.h:22: // See ButtonDropDown. I don't understand why we're saying "See ButtonDropDown" but this class does not inherit from or compose with ButtonDropDown. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.h:32: // Set up basic behavior. Should be called by any subclasses. Nit: When? https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.h:33: void Init(); Nit: Place all overrides together and all non-overrides together. My normal ordering for this is overrides above non-overrides. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.h:35: // views::ButtonListener. Default implementation does nothing. Why do we need to subclass ButtonListener at all? It doesn't seem like any ToolbarButtons want to be listeners for themselves. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.h:43: // Overridden from views::View Nit: While here, fix these comments to just look like: "views::View:" (2 places) Also, in this case, we should be saying "views::LabelButton:" instead, since that's our actual direct parent class. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.h:48: virtual void OnMouseCaptureLost() OVERRIDE {} Nit: While here, don't define virtual function bodies in a .h file https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.h:52: virtual gfx::Size GetPreferredSize(); Nit: This should be at the top of the list instead of the bottom, to match the declaration order in view.h https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (left): https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:172: views::ImageButton::ALIGN_TOP); This alignment statement was necessary in maximized mode, where we extend the back button width to the screen edge. Make sure your change does not break the button positioning in this case. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:703: back_->SetImage(views::CustomButton::STATE_NORMAL, Nit: Qualify with Button:: instead of CustomButton:: (5 places) https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:704: *(tp->GetImageSkiaNamed(IDR_BACK))); Nit: All lines of args should be aligned (5 places) Did you intend to provide new, borderless center images for back/forward/home in this CL? https://codereview.chromium.org/62873007/diff/1470001/ui/views/controls/butto... File ui/views/controls/button/label_button.h (right): https://codereview.chromium.org/62873007/diff/1470001/ui/views/controls/butto... ui/views/controls/button/label_button.h:75: ImageView* image() const { return image_; } Nit: Put this above label() to match the member declaration order. Also, neither of these functions should be const, since they return non-const pointers.
On 2013/11/19 02:28:50, Peter Kasting wrote: > https://codereview.chromium.org/62873007/diff/1470001/chrome/app/theme/theme_... > File chrome/app/theme/theme_resources.grd (right): > > https://codereview.chromium.org/62873007/diff/1470001/chrome/app/theme/theme_... > chrome/app/theme/theme_resources.grd:858: <structure type="chrome_scaled_image" > name="IDR_RELOAD" file="common/browser_reload_normal.png" /> > Are these old resources still referenced anywhere? If this change removes the > references, then also remove these lines from the .grd and delete the image > files. > > Note: Since trybots don't like binary file changes, you may want to ultimately > do this CL as three commits: > (1) Land new image files > (2) All code changes and .grd changes > (3) Remove unreferenced old image files and rename new files to match final .grd > names > > You may wonder what the "rename" bit of step (3) means. I think ultimately we > should just call these resources "RELOAD{_DIMMED}" instead of > "RELOAD_INNER{_DIMMED}" (same with Stop below). But the .grd names should match > the file names, and if you do the three-step process above using filenames > without "inner" in step 1, you'll break the existing behavior. This isn't so > bad if you already have approval on everything and are landing all three changes > within seconds of each other, but it's probably better to land with unique names > in (1), then use the correct .grd names without changing the filenames in step > (2), then make the filenames match in step (3). I've split out (1). For now we can make things work with the existing icons for forward/back/home/reload/stop. I'll just use them for now. I think we'll want to go to smaller icons, but for now we're ok. (They'll probably save a few bytes and be more comprehensible in the future.) Getting to the rest of these...
https://codereview.chromium.org/62873007/diff/1470001/chrome/app/theme/theme_... File chrome/app/theme/theme_resources.grd (right): https://codereview.chromium.org/62873007/diff/1470001/chrome/app/theme/theme_... chrome/app/theme/theme_resources.grd:923: <structure type="chrome_scaled_image" name="IDR_STOP_INNER_D" file="common/browser_stop_inner_disabled.png" /> On 2013/11/19 02:28:50, Peter Kasting wrote: > Nit: Be consistent in naming with how you named the reload images. Took this file out of this change -- new graphics are in another Cl that's committed, and the other image mods will be done later. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/home_button.h (right): https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/home_button.h:14: class HomeImageButton : public ToolbarButton { On 2013/11/19 02:28:50, Peter Kasting wrote: > Nit: Incidentally, we maybe should rename this HomeButton at some point. Done. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/reload_button.cc (right): https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/reload_button.cc:56: On 2013/11/19 02:28:50, Peter Kasting wrote: > Nit: Extra blank line. > > I actually preferred the old way of putting "}" on its own line, which would be > consistent with the constructor too. Done. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/reload_button.cc:237: LOG(INFO) << "Switch to mode " << mode; On 2013/11/19 02:28:50, Peter Kasting wrote: > Don't land this Removed debugging stuff. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/reload_button.cc:239: if (mode == MODE_RELOAD) { On 2013/11/19 02:28:50, Peter Kasting wrote: > Nit: Shorter: > > SetImage(views::Button::STATE_NORMAL, *(tp->GetImageSkiaNamed( > (mode == MODE_RELOAD) ? IDR_RELOAD : IDR_STOP))); > SetImage(views::Button::STATE_DISABLED, *(tp->GetImageSkiaNamed( > (mode == MODE_RELOAD) ? IDR_RELOAD_D : IDR_STOP_D))); Done. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/reload_button.cc:240: SetImage(views::CustomButton::STATE_NORMAL, On 2013/11/19 02:28:50, Peter Kasting wrote: > Nit: Qualify this with Button:: instead of CustomButton:: (2 places) Done. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/reload_button.cc:241: *(tp->GetImageSkiaNamed(IDR_RELOAD))); On 2013/11/19 02:28:50, Peter Kasting wrote: > Did you mean to be using IDR_RELOAD_INNER here and similar below? (But see my > .grd comments) Decided to not deal with the graphics switch in this cl. I'll figure that out separately. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/reload_button.h (right): https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/reload_button.h:106: gfx::ImageSkia images_[STATE_COUNT]; On 2013/11/19 02:28:50, Peter Kasting wrote: > These two arrays are dead and should be removed. Done. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:23: // TODO(gbillock): replace with virtual method. On 2013/11/19 02:28:50, Peter Kasting wrote: > Why do we need a virtual method? This pollutes the global namespace -- should be a class field/method if it needs changed (trying to think ahead to the WrenchMenu case which is different). https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:32: show_menu_factory_(this) {} On 2013/11/19 02:28:50, Peter Kasting wrote: > Nit: I slightly prefer the original "linebreak between braces" style of this > file (3 places) Done. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:51: border->SetPainter(true, views::Button::STATE_NORMAL, NULL); On 2013/11/19 02:28:50, Peter Kasting wrote: > Don't we also need to set painters for the other focused states, as well as all > disabled states? Otherwise they'll fall back to the default painters in > LabelButtonBorder(). I think the default painter is fine since the toolbar buttons have no border when disabled/normal. That's what my visual testing supports as well -- comparing buttons beside each other during conversion I couldn't tell them apart in any state. I'm not sure about focus states, though. I need to figure out how to get focus onto the toolbar buttons to test -- but I think we just have default focus indication for them, since I don't think there's anything particular that we do for these buttons currently. Does LabelButton need focus setup other than just set_focusable(true) that ImageButton does by default? Testing, it looks like focus doesn't move through the toolbar buttons -- it goes from the web page into the omnibox and back, but not through any of the buttons. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:141: LocationBarView::GetItemPadding(), On 2013/11/19 02:28:50, Peter Kasting wrote: > Nit: Prefer to put this entire first arg on the same line if there's a way to > linebreak that will accommodate it. Done. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:143: } else { On 2013/11/19 02:28:50, Peter Kasting wrote: > Nit: No else after return. > > If you reverse the conditional, you can avoid {} as well. Done. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.h (right): https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.h:22: // See ButtonDropDown. On 2013/11/19 02:28:50, Peter Kasting wrote: > I don't understand why we're saying "See ButtonDropDown" but this class does not > inherit from or compose with ButtonDropDown. This was stale. Deleting. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.h:32: // Set up basic behavior. Should be called by any subclasses. On 2013/11/19 02:28:50, Peter Kasting wrote: > Nit: When? Done. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.h:33: void Init(); On 2013/11/19 02:28:50, Peter Kasting wrote: > Nit: Place all overrides together and all non-overrides together. > > My normal ordering for this is overrides above non-overrides. Done. I like Init() type methods right after the constructor/destructor, but I'll move it if you want them all together in this order. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.h:35: // views::ButtonListener. Default implementation does nothing. On 2013/11/19 02:28:50, Peter Kasting wrote: > Why do we need to subclass ButtonListener at all? It doesn't seem like any > ToolbarButtons want to be listeners for themselves. It's something we'll want for the site chip, but agreed it doesn't really belong here. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.h:43: // Overridden from views::View On 2013/11/19 02:28:50, Peter Kasting wrote: > Nit: While here, fix these comments to just look like: "views::View:" (2 places) > > Also, in this case, we should be saying "views::LabelButton:" instead, since > that's our actual direct parent class. Done. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.h:48: virtual void OnMouseCaptureLost() OVERRIDE {} On 2013/11/19 02:28:50, Peter Kasting wrote: > Nit: While here, don't define virtual function bodies in a .h file Done. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.h:52: virtual gfx::Size GetPreferredSize(); On 2013/11/19 02:28:50, Peter Kasting wrote: > Nit: This should be at the top of the list instead of the bottom, to match the > declaration order in view.h Done. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (left): https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:172: views::ImageButton::ALIGN_TOP); On 2013/11/19 02:28:50, Peter Kasting wrote: > This alignment statement was necessary in maximized mode, where we extend the > back button width to the screen edge. Make sure your change does not break the > button positioning in this case. I'll check on that. OK, it looks like what's happening is we're extending to the border, but the back button is showing up slightly wider than the other buttons. I think that's an error -- we always want the button to cap to the current image width. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:703: back_->SetImage(views::CustomButton::STATE_NORMAL, On 2013/11/19 02:28:50, Peter Kasting wrote: > Nit: Qualify with Button:: instead of CustomButton:: (5 places) Done. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:704: *(tp->GetImageSkiaNamed(IDR_BACK))); On 2013/11/19 02:28:50, Peter Kasting wrote: > Nit: All lines of args should be aligned (5 places) Done. > Did you intend to provide new, borderless center images for back/forward/home in > this CL? No, I was just going to use the existing ones. I do think we should change them to borderless, though, but we can do it in-place and delete the unneeded ones. https://codereview.chromium.org/62873007/diff/1470001/ui/views/controls/butto... File ui/views/controls/button/label_button.h (right): https://codereview.chromium.org/62873007/diff/1470001/ui/views/controls/butto... ui/views/controls/button/label_button.h:75: ImageView* image() const { return image_; } On 2013/11/19 02:28:50, Peter Kasting wrote: > Nit: Put this above label() to match the member declaration order. > > Also, neither of these functions should be const, since they return non-const > pointers. Yeah, you're probably right there. I mean technically they can but I'll bet they can just return const X*.
https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:23: // TODO(gbillock): replace with virtual method. On 2013/11/20 00:59:03, Greg Billock wrote: > On 2013/11/19 02:28:50, Peter Kasting wrote: > > Why do we need a virtual method? > > This pollutes the global namespace -- should be a class field/method if it needs > changed (trying to think ahead to the WrenchMenu case which is different). OK, I'm not looking at the WrenchMenu code. At least for the current code, this constant should just be moved into ToolbarButton::OnMousePressed() right above where it's actually used, which avoids polluting any namespace and is more compliant with the style guide's "declare things in the narrowest scope possible" sentiment anyway. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:51: border->SetPainter(true, views::Button::STATE_NORMAL, NULL); On 2013/11/20 00:59:03, Greg Billock wrote: > On 2013/11/19 02:28:50, Peter Kasting wrote: > > Don't we also need to set painters for the other focused states, as well as > all > > disabled states? Otherwise they'll fall back to the default painters in > > LabelButtonBorder(). > > I think the default painter is fine since the toolbar buttons have no border > when disabled/normal. I know, but the default painters will be drawing such a border. Won't they? Else what do the IDR_BUTTON_XXX painters there actually do? In fact maybe we should just be using one of the other styles that does less? Also seems like we should probably clear the insets as well. > I'm not sure about focus states, though. I need to figure out how to get focus > onto the toolbar buttons to test Ask dmazzoni. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (left): https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:172: views::ImageButton::ALIGN_TOP); On 2013/11/20 00:59:03, Greg Billock wrote: > OK, it looks like what's happening is we're extending to the border, but the > back button is showing up slightly wider than the other buttons. I think that's > an error -- we always want the button to cap to the current image width. Yeah, we'll need some way of drawing the border narrower than the button actually is. Ugh, I hope that's not too hard. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:704: *(tp->GetImageSkiaNamed(IDR_BACK))); On 2013/11/20 00:59:03, Greg Billock wrote: > No, I was just going to use the existing ones. I do think we should change them > to borderless, though, but we can do it in-place and delete the unneeded ones. OK, I was mostly worried that we'd get two borders on buttons or something if we drew a border + a center image that also had a border. I guess they'll overlay. That might make slight changes for alpha-blending if we alpha-blend to support different toolbar colors, but that probably doesn't matter if this is transitionary. That does remind me though: you should check your changes with some themes that set very different toolbar colors. I bet you have to add these new border images to the set of images that have to be tinted for theme changes. Search the codebase for other references to these image IDs. https://codereview.chromium.org/62873007/diff/1470001/ui/views/controls/butto... File ui/views/controls/button/label_button.h (right): https://codereview.chromium.org/62873007/diff/1470001/ui/views/controls/butto... ui/views/controls/button/label_button.h:75: ImageView* image() const { return image_; } On 2013/11/20 00:59:03, Greg Billock wrote: > On 2013/11/19 02:28:50, Peter Kasting wrote: > > Nit: Put this above label() to match the member declaration order. > > > > Also, neither of these functions should be const, since they return non-const > > pointers. > > Yeah, you're probably right there. I mean technically they can but I'll bet they > can just return const X*. Bonus points if you get them to return const X* instead of X*.
(Tackling other points tomorrow.) https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (left): https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:172: views::ImageButton::ALIGN_TOP); On 2013/11/20 01:10:20, Peter Kasting wrote: > On 2013/11/20 00:59:03, Greg Billock wrote: > > OK, it looks like what's happening is we're extending to the border, but the > > back button is showing up slightly wider than the other buttons. I think > that's > > an error -- we always want the button to cap to the current image width. > > Yeah, we'll need some way of drawing the border narrower than the button > actually is. Ugh, I hope that's not too hard. We ought to be able to sense that and just add a touch of maximization padding at the beginning of the toolbar in toolbar_view, right? Or is the whole point to make the button go to the edge for greater fitt's law glory? https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:704: *(tp->GetImageSkiaNamed(IDR_BACK))); On 2013/11/20 01:10:20, Peter Kasting wrote: > On 2013/11/20 00:59:03, Greg Billock wrote: > > No, I was just going to use the existing ones. I do think we should change > them > > to borderless, though, but we can do it in-place and delete the unneeded ones. > > OK, I was mostly worried that we'd get two borders on buttons or something if we > drew a border + a center image that also had a border. I guess they'll overlay. > That might make slight changes for alpha-blending if we alpha-blend to support > different toolbar colors, but that probably doesn't matter if this is > transitionary. > > That does remind me though: you should check your changes with some themes that > set very different toolbar colors. I bet you have to add these new border > images to the set of images that have to be tinted for theme changes. Search > the codebase for other references to these image IDs. This change stops using all the images with borders, so there's no danger there -- we are definitely drawing transparencies on top of each other. That's OK for now, but I think having smaller center images is easier to reason about, as well as saving a few bytes. WRT themes: These images are pretty much just transparency masks, so they ought to look awsumm with any background color, or even a gradient or many patterns. I'll try a few and make sure it isn't nuts, but I'm hopeful. :-) The themes specify a solid for the background there, so I'm pretty sure we're OK, except that for very dark colors I'm not sure how it'll work out. Need to check, but I'm not sure our current system reacts well to that, since they're transparency masks as well. https://codereview.chromium.org/62873007/diff/1470001/ui/views/controls/butto... File ui/views/controls/button/label_button.h (right): https://codereview.chromium.org/62873007/diff/1470001/ui/views/controls/butto... ui/views/controls/button/label_button.h:75: ImageView* image() const { return image_; } On 2013/11/20 01:10:20, Peter Kasting wrote: > On 2013/11/20 00:59:03, Greg Billock wrote: > > On 2013/11/19 02:28:50, Peter Kasting wrote: > > > Nit: Put this above label() to match the member declaration order. > > > > > > Also, neither of these functions should be const, since they return > non-const > > > pointers. > > > > Yeah, you're probably right there. I mean technically they can but I'll bet > they > > can just return const X*. > > Bonus points if you get them to return const X* instead of X*. :-) yeah. I realized even what I want to do with the site chip view won't allow that. We may be able to get rid of const through, or have a const/non-const pair if there's enough good const usage.
https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (left): https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:172: views::ImageButton::ALIGN_TOP); On 2013/11/20 01:30:46, Greg Billock wrote: > On 2013/11/20 01:10:20, Peter Kasting wrote: > > On 2013/11/20 00:59:03, Greg Billock wrote: > > > OK, it looks like what's happening is we're extending to the border, but the > > > back button is showing up slightly wider than the other buttons. I think > > that's > > > an error -- we always want the button to cap to the current image width. > > > > Yeah, we'll need some way of drawing the border narrower than the button > > actually is. Ugh, I hope that's not too hard. > > We ought to be able to sense that and just add a touch of maximization padding > at the beginning of the toolbar in toolbar_view, right? Or is the whole point to > make the button go to the edge for greater fitt's law glory? Yes, we're intentionally extending the button hit region beyond the graphical border of the button to exploit Fitts' Law.
https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:23: // TODO(gbillock): replace with virtual method. On 2013/11/20 01:10:20, Peter Kasting wrote: > On 2013/11/20 00:59:03, Greg Billock wrote: > > On 2013/11/19 02:28:50, Peter Kasting wrote: > > > Why do we need a virtual method? > > > > This pollutes the global namespace -- should be a class field/method if it > needs > > changed (trying to think ahead to the WrenchMenu case which is different). > > OK, I'm not looking at the WrenchMenu code. At least for the current code, this > constant should just be moved into ToolbarButton::OnMousePressed() right above > where it's actually used, which avoids polluting any namespace and is more > compliant with the style guide's "declare things in the narrowest scope > possible" sentiment anyway. Done. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:51: border->SetPainter(true, views::Button::STATE_NORMAL, NULL); On 2013/11/20 01:10:20, Peter Kasting wrote: > On 2013/11/20 00:59:03, Greg Billock wrote: > > On 2013/11/19 02:28:50, Peter Kasting wrote: > > > Don't we also need to set painters for the other focused states, as well as > > all > > > disabled states? Otherwise they'll fall back to the default painters in > > > LabelButtonBorder(). > > > > I think the default painter is fine since the toolbar buttons have no border > > when disabled/normal. > > I know, but the default painters will be drawing such a border. Won't they? > Else what do the IDR_BUTTON_XXX painters there actually do? In fact maybe we > should just be using one of the other styles that does less? > > Also seems like we should probably clear the insets as well. The insets get established by the border. I think they're what we want, since as nearly as I can tell the borders are identical to the existing image borders. I looked to see whether the existing LabelButtonBorders turn out to be the same images, but they don't. That may be fodder for a future refactor, depending on what UI thinks. For now I think we should keep exactly the same look we currently have. Another wrinkle: on Linux Aura, the internal image icons are getting clipped, presumably because of these insets. Smaller assets ought to clear that up, but does that mean we should do those assets now before this goes in as is? I'm leaning towards tearing out the existing button migrations so we can get the base class checked in and make the site chip happen sooner. > > > I'm not sure about focus states, though. I need to figure out how to get focus > > onto the toolbar buttons to test > > Ask dmazzoni. Alt-Shift-T for future reference. This is working fine. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (left): https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:172: views::ImageButton::ALIGN_TOP); On 2013/11/20 01:42:20, Peter Kasting wrote: > On 2013/11/20 01:30:46, Greg Billock wrote: > > On 2013/11/20 01:10:20, Peter Kasting wrote: > > > On 2013/11/20 00:59:03, Greg Billock wrote: > > > > OK, it looks like what's happening is we're extending to the border, but > the > > > > back button is showing up slightly wider than the other buttons. I think > > > that's > > > > an error -- we always want the button to cap to the current image width. > > > > > > Yeah, we'll need some way of drawing the border narrower than the button > > > actually is. Ugh, I hope that's not too hard. > > > > We ought to be able to sense that and just add a touch of maximization padding > > at the beginning of the toolbar in toolbar_view, right? Or is the whole point > to > > make the button go to the edge for greater fitt's law glory? > > Yes, we're intentionally extending the button hit region beyond the graphical > border of the button to exploit Fitts' Law. OK, the latest patch addresses this. I ended up needing very minimal changes to the LabelButton class, which I believe are semantically compatible with the existing API. https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_view.cc:704: *(tp->GetImageSkiaNamed(IDR_BACK))); On 2013/11/20 01:30:46, Greg Billock wrote: > On 2013/11/20 01:10:20, Peter Kasting wrote: > > On 2013/11/20 00:59:03, Greg Billock wrote: > > > No, I was just going to use the existing ones. I do think we should change > > them > > > to borderless, though, but we can do it in-place and delete the unneeded > ones. > > > > OK, I was mostly worried that we'd get two borders on buttons or something if > we > > drew a border + a center image that also had a border. I guess they'll > overlay. > > That might make slight changes for alpha-blending if we alpha-blend to > support > > different toolbar colors, but that probably doesn't matter if this is > > transitionary. > > > > That does remind me though: you should check your changes with some themes > that > > set very different toolbar colors. I bet you have to add these new border > > images to the set of images that have to be tinted for theme changes. Search > > the codebase for other references to these image IDs. > > This change stops using all the images with borders, so there's no danger there > -- we are definitely drawing transparencies on top of each other. That's OK for > now, but I think having smaller center images is easier to reason about, as well > as saving a few bytes. > > WRT themes: These images are pretty much just transparency masks, so they ought > to look awsumm with any background color, or even a gradient or many patterns. > I'll try a few and make sure it isn't nuts, but I'm hopeful. :-) > > The themes specify a solid for the background there, so I'm pretty sure we're > OK, except that for very dark colors I'm not sure how it'll work out. Need to > check, but I'm not sure our current system reacts well to that, since they're > transparency masks as well. Still need to deal with this. (Chrome store crashing v8 at ToT making it hard to check.)
On 2013/11/20 23:13:22, Greg Billock wrote: > https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... > File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): > > https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... > chrome/browser/ui/views/toolbar/toolbar_button.cc:23: // TODO(gbillock): replace > with virtual method. > On 2013/11/20 01:10:20, Peter Kasting wrote: > > On 2013/11/20 00:59:03, Greg Billock wrote: > > > On 2013/11/19 02:28:50, Peter Kasting wrote: > > > > Why do we need a virtual method? > > > > > > This pollutes the global namespace -- should be a class field/method if it > > needs > > > changed (trying to think ahead to the WrenchMenu case which is different). > > > > OK, I'm not looking at the WrenchMenu code. At least for the current code, > this > > constant should just be moved into ToolbarButton::OnMousePressed() right above > > where it's actually used, which avoids polluting any namespace and is more > > compliant with the style guide's "declare things in the narrowest scope > > possible" sentiment anyway. > > Done. > > https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... > chrome/browser/ui/views/toolbar/toolbar_button.cc:51: border->SetPainter(true, > views::Button::STATE_NORMAL, NULL); > On 2013/11/20 01:10:20, Peter Kasting wrote: > > On 2013/11/20 00:59:03, Greg Billock wrote: > > > On 2013/11/19 02:28:50, Peter Kasting wrote: > > > > Don't we also need to set painters for the other focused states, as well > as > > > all > > > > disabled states? Otherwise they'll fall back to the default painters in > > > > LabelButtonBorder(). > > > > > > I think the default painter is fine since the toolbar buttons have no border > > > when disabled/normal. > > > > I know, but the default painters will be drawing such a border. Won't they? > > Else what do the IDR_BUTTON_XXX painters there actually do? In fact maybe we > > should just be using one of the other styles that does less? > > > > Also seems like we should probably clear the insets as well. > > The insets get established by the border. I think they're what we want, since as > nearly as I can tell the borders are identical to the existing image borders. I > looked to see whether the existing LabelButtonBorders turn out to be the same > images, but they don't. That may be fodder for a future refactor, depending on > what UI thinks. For now I think we should keep exactly the same look we > currently have. > > Another wrinkle: on Linux Aura, the internal image icons are getting clipped, > presumably because of these insets. Smaller assets ought to clear that up, but > does that mean we should do those assets now before this goes in as is? I'm > leaning towards tearing out the existing button migrations so we can get the > base class checked in and make the site chip happen sooner. Asked Alan for these trimmed resources. Should we wait for those? > > > > > > I'm not sure about focus states, though. I need to figure out how to get > focus > > > onto the toolbar buttons to test > > > > Ask dmazzoni. > > Alt-Shift-T for future reference. This is working fine. > > https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... > File chrome/browser/ui/views/toolbar/toolbar_view.cc (left): > > https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... > chrome/browser/ui/views/toolbar/toolbar_view.cc:172: > views::ImageButton::ALIGN_TOP); > On 2013/11/20 01:42:20, Peter Kasting wrote: > > On 2013/11/20 01:30:46, Greg Billock wrote: > > > On 2013/11/20 01:10:20, Peter Kasting wrote: > > > > On 2013/11/20 00:59:03, Greg Billock wrote: > > > > > OK, it looks like what's happening is we're extending to the border, but > > the > > > > > back button is showing up slightly wider than the other buttons. I think > > > > that's > > > > > an error -- we always want the button to cap to the current image width. > > > > > > > > Yeah, we'll need some way of drawing the border narrower than the button > > > > actually is. Ugh, I hope that's not too hard. > > > > > > We ought to be able to sense that and just add a touch of maximization > padding > > > at the beginning of the toolbar in toolbar_view, right? Or is the whole > point > > to > > > make the button go to the edge for greater fitt's law glory? > > > > Yes, we're intentionally extending the button hit region beyond the graphical > > border of the button to exploit Fitts' Law. > > OK, the latest patch addresses this. I ended up needing very minimal changes to > the LabelButton class, which I believe are semantically compatible with the > existing API. > > https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... > File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): > > https://codereview.chromium.org/62873007/diff/1470001/chrome/browser/ui/views... > chrome/browser/ui/views/toolbar/toolbar_view.cc:704: > *(tp->GetImageSkiaNamed(IDR_BACK))); > On 2013/11/20 01:30:46, Greg Billock wrote: > > On 2013/11/20 01:10:20, Peter Kasting wrote: > > > On 2013/11/20 00:59:03, Greg Billock wrote: > > > > No, I was just going to use the existing ones. I do think we should change > > > them > > > > to borderless, though, but we can do it in-place and delete the unneeded > > ones. > > > > > > OK, I was mostly worried that we'd get two borders on buttons or something > if > > we > > > drew a border + a center image that also had a border. I guess they'll > > overlay. > > > That might make slight changes for alpha-blending if we alpha-blend to > > support > > > different toolbar colors, but that probably doesn't matter if this is > > > transitionary. > > > > > > That does remind me though: you should check your changes with some themes > > that > > > set very different toolbar colors. I bet you have to add these new border > > > images to the set of images that have to be tinted for theme changes. > Search > > > the codebase for other references to these image IDs. > > > > This change stops using all the images with borders, so there's no danger > there > > -- we are definitely drawing transparencies on top of each other. That's OK > for > > now, but I think having smaller center images is easier to reason about, as > well > > as saving a few bytes. > > > > WRT themes: These images are pretty much just transparency masks, so they > ought > > to look awsumm with any background color, or even a gradient or many patterns. > > I'll try a few and make sure it isn't nuts, but I'm hopeful. :-) > > > > The themes specify a solid for the background there, so I'm pretty sure we're > > OK, except that for very dark colors I'm not sure how it'll work out. Need to > > check, but I'm not sure our current system reacts well to that, since they're > > transparency masks as well. > > Still need to deal with this. (Chrome store crashing v8 at ToT making it hard to > check.) OK, I've checked on dark themes. All looks A-OK. The transparencies render the buttons and borders light. Focus shows up OK, all looks good. I still would like to see this work on windows, but my win box is unhappy and I've spent the last 7 hours getting 1/2 of the way through a build. :-(
Want to have another look?
https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/reload_button.cc (right): https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/reload_button.cc:237: Nit: Blank line here not necessary https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/reload_button.h (right): https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/reload_button.h:51: // views::View: Nit: Either in this change or a followup, merge the overrides in this section into those for ToolbarButton (probably at the top of that list), since you don't directly subclass views::View. https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:71: icon_size.height()); Nit: Simpler: gfx::Size size(image()->GetPreferredSize()); gfx::Size label_size(label()->GetPreferredSize()); if (!label_size.IsEmpty()) size.Enlarge(label_size.width() + LocationBarView::GetItemPadding(), 0); return size; https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:172: int margin_delta = margin - margin_left_; Nit: Inline into next statement https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:174: gfx::Insets(insets.top(), insets.left() + margin_delta, Nit: Indent 4, not 2 https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:179: 3 + margin, 3, 3, 3)); These 3s are magic. Either the focus border should have some way to get at the current insets so you can adjust them, or the "3" should be a constant defined in some relevant class that you can reuse here. https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:195: return model_.get() != NULL; Nit: .get() not necessary https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.h (right): https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.h:48: virtual gfx::Rect GetThemePaintRect() const OVERRIDE; Because only the back button needs this override, SetLeftMargin(), and |margin_left_|, it seems like maybe we should add a simple BackButton subclass of ToolbarButton that just defines these. This will also let us write more specific comments on these methods about what the back button does, instead of the somewhat vague wording on |margin_left_| right now. https://codereview.chromium.org/62873007/diff/1780001/ui/views/controls/butto... File ui/views/controls/button/label_button.h (right): https://codereview.chromium.org/62873007/diff/1780001/ui/views/controls/butto... ui/views/controls/button/label_button.h:86: // Overridden from NativeThemeDelegate: Nit: Just "NativeThemeDelegate:" (and fix other case of this just above)
OK, I've checked this out on Windows. Looks good. Fullscreen back button behavior is right. Buttons look good, hovers, activations. Everything I checked worked fine. On Fri, Nov 22, 2013 at 2:14 PM, <gbillock@chromium.org> wrote: > Want to have another look? > > https://codereview.chromium.org/62873007/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/reload_button.cc (right): https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/reload_button.cc:237: On 2013/11/23 00:23:55, Peter Kasting wrote: > Nit: Blank line here not necessary Done. https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/reload_button.h (right): https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/reload_button.h:51: // views::View: On 2013/11/23 00:23:55, Peter Kasting wrote: > Nit: Either in this change or a followup, merge the overrides in this section > into those for ToolbarButton (probably at the top of that list), since you don't > directly subclass views::View. Done. https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:71: icon_size.height()); On 2013/11/23 00:23:55, Peter Kasting wrote: > Nit: Simpler: > > gfx::Size size(image()->GetPreferredSize()); > gfx::Size label_size(label()->GetPreferredSize()); > if (!label_size.IsEmpty()) > size.Enlarge(label_size.width() + LocationBarView::GetItemPadding(), 0); > return size; I like it. Hadn't noticed Enlarge(). https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:172: int margin_delta = margin - margin_left_; On 2013/11/23 00:23:55, Peter Kasting wrote: > Nit: Inline into next statement Done. https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:174: gfx::Insets(insets.top(), insets.left() + margin_delta, On 2013/11/23 00:23:55, Peter Kasting wrote: > Nit: Indent 4, not 2 Done. https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:179: 3 + margin, 3, 3, 3)); On 2013/11/23 00:23:55, Peter Kasting wrote: > These 3s are magic. > > Either the focus border should have some way to get at the current insets so you > can adjust them, or the "3" should be a constant defined in some relevant class > that you can reuse here. See label_button.cc:150. Also TextButton, where they use a constant set to 3. I agree they're magic, but after much back and forth with UI about various things, I've been beaten into submission and accept that a very large portion of our layout is ad hoc magic. Believe me, I agree that this, and much else about our layout, should be in some kind of central constants class, or better yet baked into the framework so that it's really easy to get the right UI-blessed look. As it is, it requires code spelunking and lots of trial and error to figure this stuff out. :-( Added a constant and a TODO. https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:195: return model_.get() != NULL; On 2013/11/23 00:23:55, Peter Kasting wrote: > Nit: .get() not necessary Done. https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.h (right): https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.h:48: virtual gfx::Rect GetThemePaintRect() const OVERRIDE; On 2013/11/23 00:23:55, Peter Kasting wrote: > Because only the back button needs this override, SetLeftMargin(), and > |margin_left_|, it seems like maybe we should add a simple BackButton subclass > of ToolbarButton that just defines these. > > This will also let us write more specific comments on these methods about what > the back button does, instead of the somewhat vague wording on |margin_left_| > right now. Makes sense. Looking at the wrench menu, it wants the same thing, but the other side, so that'll fit into that subclass. https://codereview.chromium.org/62873007/diff/1780001/ui/views/controls/butto... File ui/views/controls/button/label_button.h (right): https://codereview.chromium.org/62873007/diff/1780001/ui/views/controls/butto... ui/views/controls/button/label_button.h:86: // Overridden from NativeThemeDelegate: On 2013/11/23 00:23:55, Peter Kasting wrote: > Nit: Just "NativeThemeDelegate:" (and fix other case of this just above) Done.
On 2013/11/25 17:16:30, Greg Billock wrote: > https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... > File chrome/browser/ui/views/toolbar/reload_button.cc (right): > > https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... > chrome/browser/ui/views/toolbar/reload_button.cc:237: > On 2013/11/23 00:23:55, Peter Kasting wrote: > > Nit: Blank line here not necessary > > Done. > > https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... > File chrome/browser/ui/views/toolbar/reload_button.h (right): > > https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... > chrome/browser/ui/views/toolbar/reload_button.h:51: // views::View: > On 2013/11/23 00:23:55, Peter Kasting wrote: > > Nit: Either in this change or a followup, merge the overrides in this section > > into those for ToolbarButton (probably at the top of that list), since you > don't > > directly subclass views::View. > > Done. > > https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... > File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): > > https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... > chrome/browser/ui/views/toolbar/toolbar_button.cc:71: icon_size.height()); > On 2013/11/23 00:23:55, Peter Kasting wrote: > > Nit: Simpler: > > > > gfx::Size size(image()->GetPreferredSize()); > > gfx::Size label_size(label()->GetPreferredSize()); > > if (!label_size.IsEmpty()) > > size.Enlarge(label_size.width() + LocationBarView::GetItemPadding(), 0); > > return size; > > I like it. Hadn't noticed Enlarge(). > > https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... > chrome/browser/ui/views/toolbar/toolbar_button.cc:172: int margin_delta = margin > - margin_left_; > On 2013/11/23 00:23:55, Peter Kasting wrote: > > Nit: Inline into next statement > > Done. > > https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... > chrome/browser/ui/views/toolbar/toolbar_button.cc:174: gfx::Insets(insets.top(), > insets.left() + margin_delta, > On 2013/11/23 00:23:55, Peter Kasting wrote: > > Nit: Indent 4, not 2 > > Done. > > https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... > chrome/browser/ui/views/toolbar/toolbar_button.cc:179: 3 + margin, 3, 3, 3)); > On 2013/11/23 00:23:55, Peter Kasting wrote: > > These 3s are magic. > > > > Either the focus border should have some way to get at the current insets so > you > > can adjust them, or the "3" should be a constant defined in some relevant > class > > that you can reuse here. > > See label_button.cc:150. Also TextButton, where they use a constant set to 3. I > agree they're magic, but after much back and forth with UI about various things, > I've been beaten into submission and accept that a very large portion of our > layout is ad hoc magic. > > Believe me, I agree that this, and much else about our layout, should be in some > kind of central constants class, or better yet baked into the framework so that > it's really easy to get the right UI-blessed look. As it is, it requires code > spelunking and lots of trial and error to figure this stuff out. :-( Added a > constant and a TODO. > > https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... > chrome/browser/ui/views/toolbar/toolbar_button.cc:195: return model_.get() != > NULL; > On 2013/11/23 00:23:55, Peter Kasting wrote: > > Nit: .get() not necessary > > Done. > > https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... > File chrome/browser/ui/views/toolbar/toolbar_button.h (right): > > https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... > chrome/browser/ui/views/toolbar/toolbar_button.h:48: virtual gfx::Rect > GetThemePaintRect() const OVERRIDE; > On 2013/11/23 00:23:55, Peter Kasting wrote: > > Because only the back button needs this override, SetLeftMargin(), and > > |margin_left_|, it seems like maybe we should add a simple BackButton subclass > > of ToolbarButton that just defines these. > > > > This will also let us write more specific comments on these methods about what > > the back button does, instead of the somewhat vague wording on |margin_left_| > > right now. > > Makes sense. Looking at the wrench menu, it wants the same thing, but the other > side, so that'll fit into that subclass. > > https://codereview.chromium.org/62873007/diff/1780001/ui/views/controls/butto... > File ui/views/controls/button/label_button.h (right): > > https://codereview.chromium.org/62873007/diff/1780001/ui/views/controls/butto... > ui/views/controls/button/label_button.h:86: // Overridden from > NativeThemeDelegate: > On 2013/11/23 00:23:55, Peter Kasting wrote: > > Nit: Just "NativeThemeDelegate:" (and fix other case of this just above) > > Done. Anything more on this? My only remaining issue is the linux Aura images look like they are clipped. I'm not sure whether that's a big deal though -- they don't look like final images on my build.
Really the only major remaining thing is the whole left margin bit. LGTM if you wanted to do this separately. https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.h (right): https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.h:48: virtual gfx::Rect GetThemePaintRect() const OVERRIDE; On 2013/11/25 17:16:30, Greg Billock wrote: > On 2013/11/23 00:23:55, Peter Kasting wrote: > > Because only the back button needs this override, SetLeftMargin(), and > > |margin_left_|, it seems like maybe we should add a simple BackButton subclass > > of ToolbarButton that just defines these. > > > > This will also let us write more specific comments on these methods about what > > the back button does, instead of the somewhat vague wording on |margin_left_| > > right now. > > Makes sense. Looking at the wrench menu, it wants the same thing, but the other > side, so that'll fit into that subclass. Is your intent to do this in a separate change, then? https://codereview.chromium.org/62873007/diff/1970001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/62873007/diff/1970001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:196: return model_; Nit: I'd intended "model_ != NULL"; "!!model_" also works but is somewhat more cryptic. I'd rather not just do "model_", though, because even if it compiles on non-Windows (which I'm not sure of), it is rather confusing. https://codereview.chromium.org/62873007/diff/1970001/ui/views/controls/butto... File ui/views/controls/button/label_button.h (right): https://codereview.chromium.org/62873007/diff/1970001/ui/views/controls/butto... ui/views/controls/button/label_button.h:86: //NativeThemeDelegate: Nit: Add space after "//"
+sky for views owners https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.h (right): https://codereview.chromium.org/62873007/diff/1780001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.h:48: virtual gfx::Rect GetThemePaintRect() const OVERRIDE; On 2013/11/26 02:25:46, Peter Kasting wrote: > On 2013/11/25 17:16:30, Greg Billock wrote: > > On 2013/11/23 00:23:55, Peter Kasting wrote: > > > Because only the back button needs this override, SetLeftMargin(), and > > > |margin_left_|, it seems like maybe we should add a simple BackButton > subclass > > > of ToolbarButton that just defines these. > > > > > > This will also let us write more specific comments on these methods about > what > > > the back button does, instead of the somewhat vague wording on > |margin_left_| > > > right now. > > > > Makes sense. Looking at the wrench menu, it wants the same thing, but the > other > > side, so that'll fit into that subclass. > > Is your intent to do this in a separate change, then? Yeah I'd like to check this in so the site chip class can land. I'll factor the back button out next thing. https://codereview.chromium.org/62873007/diff/1970001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/toolbar_button.cc (right): https://codereview.chromium.org/62873007/diff/1970001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/toolbar_button.cc:196: return model_; On 2013/11/26 02:25:46, Peter Kasting wrote: > Nit: I'd intended "model_ != NULL"; "!!model_" also works but is somewhat more > cryptic. I'd rather not just do "model_", though, because even if it compiles > on non-Windows (which I'm not sure of), it is rather confusing. Done. https://codereview.chromium.org/62873007/diff/1970001/ui/views/controls/butto... File ui/views/controls/button/label_button.h (right): https://codereview.chromium.org/62873007/diff/1970001/ui/views/controls/butto... ui/views/controls/button/label_button.h:86: //NativeThemeDelegate: On 2013/11/26 02:25:46, Peter Kasting wrote: > Nit: Add space after "//" Done.
On 2013/11/26 02:25:46, Peter Kasting wrote: > Really the only major remaining thing is the whole left margin bit. LGTM if you > wanted to do this separately. Was going to, but sky has still to review, so put it in here. Give it a look.
LGTM https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/back_button.cc (right): https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/back_button.cc:20: gfx::Rect rect = LabelButton::GetThemePaintRect(); Nit: Prefer () to = for non-basic types https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/back_button.cc:21: if (margin_left_ > 0) { Nit: It seems like we can just eliminate this conditional and do the inset unconditionally, since when |margin_left_| is 0 it's a no-op. https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/back_button.cc:23: rect.width() - margin_left_, rect.height()); Nit: Simpler: rect.Inset(margin_left_, 0, 0, 0); https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/back_button.h (right): https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/back_button.h:9: #include "ui/views/controls/button/button.h" Nit: Is this #include necessary? Seems like we could just forward-declare ButtonListener? https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/back_button.h:19: // in the underlying button, and similarly adjusting the focus border. Nit: This comment might be clearer: A subclass of ToolbarButton to allow the back button's hittest region to extend all the way to the start of the toolbar (i.e. the screen edge) in maximized mode, to benefit from Fitt's Law. The button images and focus border are still drawn with the normal square shape. https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/back_button.h:27: void SetLeftMargin(int margin); Nit: Consider using "start" or "leading" in place of "left" here and below to avoid being misleading in RTL layouts. https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/reload_button.cc (right): https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/reload_button.cc:237: // Needed for unit tests. Nit: Does this mean "|tp| can be NULL in unit tests."? If so, I'd phrase it that way.
https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/back_button.cc (right): https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/back_button.cc:20: gfx::Rect rect = LabelButton::GetThemePaintRect(); On 2013/11/26 22:24:31, Peter Kasting wrote: > Nit: Prefer () to = for non-basic types Done. https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/back_button.cc:21: if (margin_left_ > 0) { On 2013/11/26 22:24:31, Peter Kasting wrote: > Nit: It seems like we can just eliminate this conditional and do the inset > unconditionally, since when |margin_left_| is 0 it's a no-op. I think that's safe. I think I was thinking about negative values, but I don't think that ever happens. https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/back_button.cc:23: rect.width() - margin_left_, rect.height()); On 2013/11/26 22:24:31, Peter Kasting wrote: > Nit: Simpler: > > rect.Inset(margin_left_, 0, 0, 0); Done. https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/back_button.h (right): https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/back_button.h:9: #include "ui/views/controls/button/button.h" On 2013/11/26 22:24:31, Peter Kasting wrote: > Nit: Is this #include necessary? Seems like we could just forward-declare > ButtonListener? Done. https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/back_button.h:19: // in the underlying button, and similarly adjusting the focus border. On 2013/11/26 22:24:31, Peter Kasting wrote: > Nit: This comment might be clearer: > > A subclass of ToolbarButton to allow the back button's hittest region to extend > all the way to the start of the toolbar (i.e. the screen edge) in maximized > mode, to benefit from Fitt's Law. The button images and focus border are still > drawn with the normal square shape. Sounds good. https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/back_button.h:27: void SetLeftMargin(int margin); On 2013/11/26 22:24:31, Peter Kasting wrote: > Nit: Consider using "start" or "leading" in place of "left" here and below to > avoid being misleading in RTL layouts. Done. https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... File chrome/browser/ui/views/toolbar/reload_button.cc (right): https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... chrome/browser/ui/views/toolbar/reload_button.cc:237: // Needed for unit tests. On 2013/11/26 22:24:31, Peter Kasting wrote: > Nit: Does this mean "|tp| can be NULL in unit tests."? If so, I'd phrase it > that way. Done.
On 2013/11/27 14:22:34, Greg Billock wrote: > https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... > File chrome/browser/ui/views/toolbar/back_button.cc (right): > > https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... > chrome/browser/ui/views/toolbar/back_button.cc:20: gfx::Rect rect = > LabelButton::GetThemePaintRect(); > On 2013/11/26 22:24:31, Peter Kasting wrote: > > Nit: Prefer () to = for non-basic types > > Done. > > https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... > chrome/browser/ui/views/toolbar/back_button.cc:21: if (margin_left_ > 0) { > On 2013/11/26 22:24:31, Peter Kasting wrote: > > Nit: It seems like we can just eliminate this conditional and do the inset > > unconditionally, since when |margin_left_| is 0 it's a no-op. > > I think that's safe. I think I was thinking about negative values, but I don't > think that ever happens. > > https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... > chrome/browser/ui/views/toolbar/back_button.cc:23: rect.width() - margin_left_, > rect.height()); > On 2013/11/26 22:24:31, Peter Kasting wrote: > > Nit: Simpler: > > > > rect.Inset(margin_left_, 0, 0, 0); > > Done. > > https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... > File chrome/browser/ui/views/toolbar/back_button.h (right): > > https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... > chrome/browser/ui/views/toolbar/back_button.h:9: #include > "ui/views/controls/button/button.h" > On 2013/11/26 22:24:31, Peter Kasting wrote: > > Nit: Is this #include necessary? Seems like we could just forward-declare > > ButtonListener? > > Done. > > https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... > chrome/browser/ui/views/toolbar/back_button.h:19: // in the underlying button, > and similarly adjusting the focus border. > On 2013/11/26 22:24:31, Peter Kasting wrote: > > Nit: This comment might be clearer: > > > > A subclass of ToolbarButton to allow the back button's hittest region to > extend > > all the way to the start of the toolbar (i.e. the screen edge) in maximized > > mode, to benefit from Fitt's Law. The button images and focus border are > still > > drawn with the normal square shape. > > Sounds good. > > https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... > chrome/browser/ui/views/toolbar/back_button.h:27: void SetLeftMargin(int > margin); > On 2013/11/26 22:24:31, Peter Kasting wrote: > > Nit: Consider using "start" or "leading" in place of "left" here and below to > > avoid being misleading in RTL layouts. > > Done. > > https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... > File chrome/browser/ui/views/toolbar/reload_button.cc (right): > > https://codereview.chromium.org/62873007/diff/2030001/chrome/browser/ui/views... > chrome/browser/ui/views/toolbar/reload_button.cc:237: // Needed for unit tests. > On 2013/11/26 22:24:31, Peter Kasting wrote: > > Nit: Does this mean "|tp| can be NULL in unit tests."? If so, I'd phrase it > > that way. > > Done. +ben, sadrul for views owners. Not sure who might be on vacation.
lgtm https://codereview.chromium.org/62873007/diff/2070001/ui/views/controls/butto... File ui/views/controls/button/label_button.h (right): https://codereview.chromium.org/62873007/diff/2070001/ui/views/controls/butto... ui/views/controls/button/label_button.h:68: // View: Mind keeping this consistent with the rest of the file (i.e. remove this change, and add 'Overridden from ' in line 87)?
https://codereview.chromium.org/62873007/diff/2070001/ui/views/controls/butto... File ui/views/controls/button/label_button.h (right): https://codereview.chromium.org/62873007/diff/2070001/ui/views/controls/butto... ui/views/controls/button/label_button.h:68: // View: On 2013/12/02 17:34:55, sadrul wrote: > Mind keeping this consistent with the rest of the file (i.e. remove this change, > and add 'Overridden from ' in line 87)? Don't do that; rather, remove "Overridden from" in the other comments if touching them at all. Though, because this class doesn't directly inherit from View, these comments should refer to the relevant direct parent, e.g. CustomButton in this case.
https://codereview.chromium.org/62873007/diff/2070001/ui/views/controls/butto... File ui/views/controls/button/label_button.h (right): https://codereview.chromium.org/62873007/diff/2070001/ui/views/controls/butto... ui/views/controls/button/label_button.h:68: // View: On 2013/12/02 17:38:53, Peter Kasting wrote: > On 2013/12/02 17:34:55, sadrul wrote: > > Mind keeping this consistent with the rest of the file (i.e. remove this > change, > > and add 'Overridden from ' in line 87)? > > Don't do that; rather, remove "Overridden from" in the other comments if > touching them at all. > > Though, because this class doesn't directly inherit from View, these comments > should refer to the relevant direct parent, e.g. CustomButton in this case. As long as these are consistent in the same file, I don't have a particular preference. Removing 'Overridden from' from the rest of the places sgtm.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/62873007/2090001
https://codereview.chromium.org/62873007/diff/2070001/ui/views/controls/butto... File ui/views/controls/button/label_button.h (right): https://codereview.chromium.org/62873007/diff/2070001/ui/views/controls/butto... ui/views/controls/button/label_button.h:68: // View: On 2013/12/02 17:41:48, sadrul wrote: > On 2013/12/02 17:38:53, Peter Kasting wrote: > > On 2013/12/02 17:34:55, sadrul wrote: > > > Mind keeping this consistent with the rest of the file (i.e. remove this > > change, > > > and add 'Overridden from ' in line 87)? > > > > Don't do that; rather, remove "Overridden from" in the other comments if > > touching them at all. > > > > Though, because this class doesn't directly inherit from View, these comments > > should refer to the relevant direct parent, e.g. CustomButton in this case. > > As long as these are consistent in the same file, I don't have a particular > preference. Removing 'Overridden from' from the rest of the places sgtm. Done.
Retried try job too often on linux_chromeos_clang for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/62873007/2110001
Message was sent while issue was closed.
Change committed as 238218 |
