Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
75873002:
[SiteChip] Add the basic painting of the site chip button (Closed)
|
Created:
7 years, 1 month ago by Greg Billock Modified:
7 years ago CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[SiteChip] Add the basic painting of the site chip button.
R=pkasting@chromium.org
BUG=315944
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238732
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238890
Patch Set 1 #
Total comments: 32
Patch Set 2 : . #
Total comments: 4
Patch Set 3 : . #Patch Set 4 : Update spacing #Patch Set 5 : Rebase #Patch Set 6 : Use constants #
Total comments: 40
Patch Set 7 : Move painter, etc. #
Total comments: 11
Patch Set 8 : . #Patch Set 9 : Use MediumFont #
Total comments: 5
Patch Set 10 : Don't addChild if site chip not shown #
Total comments: 2
Patch Set 11 : Fix asan and focus. #
Total comments: 6
Patch Set 12 : Move focus set to ToolbarView #
Total comments: 4
Patch Set 13 : . #
Total comments: 1
Messages
Total messages: 35 (0 generated)
https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.cc:29: toolbar_view_(toolbar_view) {} Nit: I slightly prefer linebreaking between braces (2 places) https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.cc:35: toolbar_view_->location_bar()->GetLocationEntry()->SetFocus(); The fact that all these calls are on the OmniboxView suggests that either that or the LocationBarView would be a better ButtonListener than |this|. I suggest the LcoationBarView, which is already a button listener for various things. https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.cc:44: // TODO(gbillock): Would be nice to just use stock LabelButton stuff here. Why can't you? https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.cc:79: // Item spacing within the chip are LocationBarView::GetItemPadding. This comment doesn't add anything to the code. https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.cc:86: gfx::Size size = Nit: Don't do "Size size = Size(...)", just do "Size size(...)" https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.cc:88: 2*ToolbarView::kStandardSpacing + Nit: Spaces around operators (2 places) Decide whether you're doing "x * 2" or "2 * x" and stick with it. https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.cc:107: host_label_width, height() - LocationBarView::kNormalEdgeThickness*2); Nit: All lines of args should begin at the same horizontal location https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... File chrome/browser/ui/views/toolbar/site_chip_view.h (right): https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.h:13: #include "ui/views/view.h" Nit: Almost none of these #includes seem necessary. Eliminate the ones you don't need. https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.h:35: void OnChanged(); This doesn't seem to be called, nor do the two functions below. It's harder to know what to say about them in such a case than if you added them later in the context of code that actually called them. https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.h:38: const views::ImageView* GetLocationIconView() const; It seems like these should be inlined unix_hacker()-style accessors. https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.h:40: // views::ButtonListener Nit: You don't inherit directly from views::ButtonListener (or views::View, below). Give your actual parent class, then declare overrides in the order they'd actually appear in the parent, assuming we listed the methods declared in base classes before those in subclasses. https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/toolbar_view.cc:230: // use --force-fieldtrials=EmbeddedSearch/Group1 origin_chip:1/ to force Nit: I would prefer not to document this here. https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/toolbar_view.cc:231: site_chip_view_ = new SiteChipView(this); Nit: Instead of conditionally creating this, consider always creating it and adding it, and then only make it visible in some cases. This is consistent with how the LocationBarView treats experimental-feature-related child views, and usually leads to slightly simpler code overall, especially since ultimately you're going to need to dynamically modify the site chip's visibility anyway (e.g. for NTP) and thus will need to handle the "invisible" case correctly everywhere. https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/toolbar_view.cc:565: site_chip_width = std::min(site_chip_width, available_width/2); Nit: Spaces around operators Be careful in your size calculations; you'll want to make sure we don't allow site chip or location bar to shrink too small to render correctly. (Chrome seems buggier here than it used to; IIRC we used to correctly calculate minimum widths for the location bar and toolbar such that the user couldn't shrink the location bar below about "enough width to show ten characters of text", because I worked on putting that in.)
https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.cc:29: toolbar_view_(toolbar_view) {} On 2013/11/19 02:47:32, Peter Kasting wrote: > Nit: I slightly prefer linebreaking between braces (2 places) Done. https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.cc:35: toolbar_view_->location_bar()->GetLocationEntry()->SetFocus(); On 2013/11/19 02:47:32, Peter Kasting wrote: > The fact that all these calls are on the OmniboxView suggests that either that > or the LocationBarView would be a better ButtonListener than |this|. I suggest > the LcoationBarView, which is already a button listener for various things. Yeah. This is mostly to isolate the experimental code as much as possible. https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.cc:44: // TODO(gbillock): Would be nice to just use stock LabelButton stuff here. On 2013/11/19 02:47:32, Peter Kasting wrote: > Why can't you? Need to catch up with other changes -- making the LabelButton image accessible was the crucial gap, and I did that for the base class. https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.cc:79: // Item spacing within the chip are LocationBarView::GetItemPadding. On 2013/11/19 02:47:32, Peter Kasting wrote: > This comment doesn't add anything to the code. Yeah, this was scaffolding. :-) https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.cc:86: gfx::Size size = On 2013/11/19 02:47:32, Peter Kasting wrote: > Nit: Don't do "Size size = Size(...)", just do "Size size(...)" Switched up. https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.cc:88: 2*ToolbarView::kStandardSpacing + On 2013/11/19 02:47:32, Peter Kasting wrote: > Nit: Spaces around operators (2 places) > > Decide whether you're doing "x * 2" or "2 * x" and stick with it. Done. https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.cc:107: host_label_width, height() - LocationBarView::kNormalEdgeThickness*2); On 2013/11/19 02:47:32, Peter Kasting wrote: > Nit: All lines of args should begin at the same horizontal location Done. https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... File chrome/browser/ui/views/toolbar/site_chip_view.h (right): https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.h:13: #include "ui/views/view.h" On 2013/11/19 02:47:32, Peter Kasting wrote: > Nit: Almost none of these #includes seem necessary. Eliminate the ones you > don't need. Done. https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.h:35: void OnChanged(); On 2013/11/19 02:47:32, Peter Kasting wrote: > This doesn't seem to be called, nor do the two functions below. It's harder to > know what to say about them in such a case than if you added them later in the > context of code that actually called them. Yeah, I'll get rid of them here. https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.h:38: const views::ImageView* GetLocationIconView() const; On 2013/11/19 02:47:32, Peter Kasting wrote: > It seems like these should be inlined unix_hacker()-style accessors. They're the same names as the virtuals in the location bar (for when the experimental location bar will want to delegate bubble anchoring here). https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.h:40: // views::ButtonListener On 2013/11/19 02:47:32, Peter Kasting wrote: > Nit: You don't inherit directly from views::ButtonListener (or views::View, > below). Give your actual parent class, then declare overrides in the order > they'd actually appear in the parent, assuming we listed the methods declared in > base classes before those in subclasses. Done. https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/toolbar_view.cc:230: // use --force-fieldtrials=EmbeddedSearch/Group1 origin_chip:1/ to force On 2013/11/19 02:47:32, Peter Kasting wrote: > Nit: I would prefer not to document this here. Done. https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/toolbar_view.cc:565: site_chip_width = std::min(site_chip_width, available_width/2); On 2013/11/19 02:47:32, Peter Kasting wrote: > Nit: Spaces around operators > > Be careful in your size calculations; you'll want to make sure we don't allow > site chip or location bar to shrink too small to render correctly. (Chrome > seems buggier here than it used to; IIRC we used to correctly calculate minimum > widths for the location bar and toolbar such that the user couldn't shrink the > location bar below about "enough width to show ten characters of text", because > I worked on putting that in.) Yeah, that's a regression -- it will currently shrink to zero. Perhaps we should fix that in a non-experiment-code change, though.
https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/toolbar_view.cc:231: site_chip_view_ = new SiteChipView(this); On 2013/11/19 02:47:32, Peter Kasting wrote: > Nit: Instead of conditionally creating this, consider always creating it and > adding it, and then only make it visible in some cases. This is consistent with > how the LocationBarView treats experimental-feature-related child views, and > usually leads to slightly simpler code overall, especially since ultimately > you're going to need to dynamically modify the site chip's visibility anyway > (e.g. for NTP) and thus will need to handle the "invisible" case correctly > everywhere. Good point. Will do that in another patch.
LGTM https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.cc:35: toolbar_view_->location_bar()->GetLocationEntry()->SetFocus(); On 2013/11/20 01:27:35, Greg Billock wrote: > On 2013/11/19 02:47:32, Peter Kasting wrote: > > The fact that all these calls are on the OmniboxView suggests that either that > > or the LocationBarView would be a better ButtonListener than |this|. I > suggest > > the LcoationBarView, which is already a button listener for various things. > > Yeah. This is mostly to isolate the experimental code as much as possible. FWIW, I wouldn't worry about that in principle. If it's cleaner to put a small block of code in the LocationBarView, let's just do that. In this case, it's probably not much of a win, so it doesn't matter much what you do. It was more important when ToolbarButton itself was being made a ButtonListener, which would have been nice to avoid. https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... File chrome/browser/ui/views/toolbar/site_chip_view.h (right): https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.h:38: const views::ImageView* GetLocationIconView() const; On 2013/11/20 01:27:35, Greg Billock wrote: > On 2013/11/19 02:47:32, Peter Kasting wrote: > > It seems like these should be inlined unix_hacker()-style accessors. > > They're the same names as the virtuals in the location bar (for when the > experimental location bar will want to delegate bubble anchoring here). Sure, but why does that matter? It doesn't seem like it adds any additional readability. https://codereview.chromium.org/75873002/diff/80001/chrome/browser/ui/views/t... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/75873002/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:28: // use --force-fieldtrials=EmbeddedSearch/Group1 origin_chip:1/ to force Nit: Comments should be complete sentences. Does this actually need to be documented in the code at all? https://codereview.chromium.org/75873002/diff/80001/chrome/browser/ui/views/t... File chrome/browser/ui/views/toolbar/site_chip_view.h (right): https://codereview.chromium.org/75873002/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.h:36: // views::ButtonListener: Nit: Declare blocks of overrides in the same order as the parent classes
Awaiting parent CL before submitting. https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.cc:35: toolbar_view_->location_bar()->GetLocationEntry()->SetFocus(); On 2013/11/20 01:49:13, Peter Kasting wrote: > On 2013/11/20 01:27:35, Greg Billock wrote: > > On 2013/11/19 02:47:32, Peter Kasting wrote: > > > The fact that all these calls are on the OmniboxView suggests that either > that > > > or the LocationBarView would be a better ButtonListener than |this|. I > > suggest > > > the LcoationBarView, which is already a button listener for various things. > > > > Yeah. This is mostly to isolate the experimental code as much as possible. > > FWIW, I wouldn't worry about that in principle. If it's cleaner to put a small > block of code in the LocationBarView, let's just do that. > > In this case, it's probably not much of a win, so it doesn't matter much what > you do. It was more important when ToolbarButton itself was being made a > ButtonListener, which would have been nice to avoid. ack. I'll either move this later or if it grows hair, keep it here. I can't think now why it'd need to be here, though. https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... File chrome/browser/ui/views/toolbar/site_chip_view.h (right): https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... chrome/browser/ui/views/toolbar/site_chip_view.h:38: const views::ImageView* GetLocationIconView() const; On 2013/11/20 01:49:13, Peter Kasting wrote: > On 2013/11/20 01:27:35, Greg Billock wrote: > > On 2013/11/19 02:47:32, Peter Kasting wrote: > > > It seems like these should be inlined unix_hacker()-style accessors. > > > > They're the same names as the virtuals in the location bar (for when the > > experimental location bar will want to delegate bubble anchoring here). > > Sure, but why does that matter? It doesn't seem like it adds any additional > readability. ok, I'll inline them https://codereview.chromium.org/75873002/diff/80001/chrome/browser/ui/views/t... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/75873002/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.cc:28: // use --force-fieldtrials=EmbeddedSearch/Group1 origin_chip:1/ to force On 2013/11/20 01:49:13, Peter Kasting wrote: > Nit: Comments should be complete sentences. > > Does this actually need to be documented in the code at all? No. I just have to look to remember it. :-) https://codereview.chromium.org/75873002/diff/80001/chrome/browser/ui/views/t... File chrome/browser/ui/views/toolbar/site_chip_view.h (right): https://codereview.chromium.org/75873002/diff/80001/chrome/browser/ui/views/t... chrome/browser/ui/views/toolbar/site_chip_view.h:36: // views::ButtonListener: On 2013/11/20 01:49:13, Peter Kasting wrote: > Nit: Declare blocks of overrides in the same order as the parent classes Done.
On 2013/11/21 19:29:34, Greg Billock wrote: > Awaiting parent CL before submitting. > > https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... > File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): > > https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... > chrome/browser/ui/views/toolbar/site_chip_view.cc:35: > toolbar_view_->location_bar()->GetLocationEntry()->SetFocus(); > On 2013/11/20 01:49:13, Peter Kasting wrote: > > On 2013/11/20 01:27:35, Greg Billock wrote: > > > On 2013/11/19 02:47:32, Peter Kasting wrote: > > > > The fact that all these calls are on the OmniboxView suggests that either > > that > > > > or the LocationBarView would be a better ButtonListener than |this|. I > > > suggest > > > > the LcoationBarView, which is already a button listener for various > things. > > > > > > Yeah. This is mostly to isolate the experimental code as much as possible. > > > > FWIW, I wouldn't worry about that in principle. If it's cleaner to put a > small > > block of code in the LocationBarView, let's just do that. > > > > In this case, it's probably not much of a win, so it doesn't matter much what > > you do. It was more important when ToolbarButton itself was being made a > > ButtonListener, which would have been nice to avoid. > > ack. I'll either move this later or if it grows hair, keep it here. I can't > think now why it'd need to be here, though. > > https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... > File chrome/browser/ui/views/toolbar/site_chip_view.h (right): > > https://codereview.chromium.org/75873002/diff/1/chrome/browser/ui/views/toolb... > chrome/browser/ui/views/toolbar/site_chip_view.h:38: const views::ImageView* > GetLocationIconView() const; > On 2013/11/20 01:49:13, Peter Kasting wrote: > > On 2013/11/20 01:27:35, Greg Billock wrote: > > > On 2013/11/19 02:47:32, Peter Kasting wrote: > > > > It seems like these should be inlined unix_hacker()-style accessors. > > > > > > They're the same names as the virtuals in the location bar (for when the > > > experimental location bar will want to delegate bubble anchoring here). > > > > Sure, but why does that matter? It doesn't seem like it adds any additional > > readability. > > ok, I'll inline them > > https://codereview.chromium.org/75873002/diff/80001/chrome/browser/ui/views/t... > File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): > > https://codereview.chromium.org/75873002/diff/80001/chrome/browser/ui/views/t... > chrome/browser/ui/views/toolbar/site_chip_view.cc:28: // use > --force-fieldtrials=EmbeddedSearch/Group1 origin_chip:1/ to force > On 2013/11/20 01:49:13, Peter Kasting wrote: > > Nit: Comments should be complete sentences. > > > > Does this actually need to be documented in the code at all? > > No. I just have to look to remember it. :-) > > https://codereview.chromium.org/75873002/diff/80001/chrome/browser/ui/views/t... > File chrome/browser/ui/views/toolbar/site_chip_view.h (right): > > https://codereview.chromium.org/75873002/diff/80001/chrome/browser/ui/views/t... > chrome/browser/ui/views/toolbar/site_chip_view.h:36: // views::ButtonListener: > On 2013/11/20 01:49:13, Peter Kasting wrote: > > Nit: Declare blocks of overrides in the same order as the parent classes > > Done. Ready for a final look-over. Made sure all values are documented constants, and resolved some lint and questions.
https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:30: const int SiteChipView::kEdgeThickness = 4; Is this a value we can auto-calculate from some image size? https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:32: const int SiteChipView::kTrailingLabelMargin = 3; Could we change these two values into some common spacing amount, then an offset that accounts for why we might need different space before and after the text? For example, maybe we really want 4 visual pixels in both cases, but 1 px is built into the label or edge image or something and thus the trailing margin is 4 - 1. Or something else that (a) keeps all the spacing in sync and (b) can be easily adjusted if we fix whatever's causing the difference later. More bonus points if we can then reuse toolbar constants (I think 3 px is already a toolbar spacing constant?) and/or location bar constants. https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:62: return chrome::ShouldDisplayOriginChip(); Are you planning on some more complex functionality here later? https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:72: toolbar_view_->location_bar()->GetOmniboxFontList()); Instead of a GetOmniboxFontList() accessor, we should just pass the font list in at Init() or construction time, which is how we do the fonts for e.g. the EV bubble, keyword search bubble, etc. https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:90: kIconTextSpacing; Nit: Indent 4, not even https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:92: kTrailingLabelMargin; Nit: Break after "=" to avoid wrapping the calculation https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:102: views::Painter::CreateImageGridPainter(kBackgroundImages)); Creating this painter in every OnPaint() call is heavyweight. Let's create it once during Init(). https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:105: LocationBarView::kNormalEdgeThickness); Can we avoid this inset call by just setting our own insets to kNormalEdgeThickness on every side? This will probably change Layout() and GetPreferredSize() too (but maybe by making them simpler). https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:113: toolbar_view_->location_bar()->GetOmniboxView()->SetFocus(); Nit: You probably still want a TODO here to make the LocationBarView or OmniboxView be the button listener eventually. https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:119: bool SiteChipView::DrawingBackground() { Nit: This isn't called or implemented yet, so I'd just leave it out until it's more fleshed out. https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.h (right): https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:35: bool ShouldShow(); Nit: Comment? https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:49: // Return true if we will draw an opaque background Nit: Return -> Returns; wrap as close to 80 cols as possible; comments should end with a period https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:51: bool DrawingBackground(); Nit: Place all non-virtuals in a group https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:56: static const int kTrailingLabelMargin; Nit: I suggest a blank line below here Really though, these can probably be file-scope constants instead of class-scope. https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:491: int site_chip_spacing = kStandardSpacing - 1; Nit: Don't calculate this value in two different places. Why is the -1 necessary? What about the site chip imagery makes the spacing need to be different? https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:501: 2*site_chip_spacing + 2*button_spacing : 0) + Nit: Spaces around operators Indent this line 4 more, then wrap after ':', so it's obvious how the expression is constructed. I'd place parens around the entire subexpression as well (i.e. "x ? (1 + 2 + 3) : 0"). https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:593: int browser_actions_x = location_bar_->x() + location_bar_->width(); So we put the browser actions flush against the location bar, but not flush against the site chip? That seems a bit odd. https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:596: site_chip_view_->SetBounds(location_bar_->x() + location_bar_->width() + Nit: Replace first two terms with "browser_actions_x" https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:601: browser_actions_x += site_chip_view_->GetPreferredSize().width() + Nit: Break after "+=" instead to avoid wrapping the calculation
A couple issues left -- FontList management the most salient. https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:30: const int SiteChipView::kEdgeThickness = 4; On 2013/12/03 00:58:10, Peter Kasting wrote: > Is this a value we can auto-calculate from some image size? It's more a quality of the image bits themselves and the UI feedback on how many pixels spacing we want from the apparent edges in the images to the apparent edge of the images we are using for the location icon view. So it's more about the internals of the images, not a property of them. https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:32: const int SiteChipView::kTrailingLabelMargin = 3; On 2013/12/03 00:58:10, Peter Kasting wrote: > Could we change these two values into some common spacing amount, then an offset > that accounts for why we might need different space before and after the text? > For example, maybe we really want 4 visual pixels in both cases, but 1 px is > built into the label or edge image or something and thus the trailing margin is > 4 - 1. Or something else that (a) keeps all the spacing in sync and (b) can be > easily adjusted if we fix whatever's causing the difference later. > > More bonus points if we can then reuse toolbar constants (I think 3 px is > already a toolbar spacing constant?) and/or location bar constants. Yeah, these are definitely unsatisfying. The deal with the icon-text spacing is an artifact, again, of the visual extent of the specific lock/page icons we're using. They don't go all the way to the edge, or we could do some calculation based on that. Getting new assets for all those might make these spacing calculations more adaptive and obvious, but even then, the visual edge may not be the image edge, given shading, transparency, etc. (Not to mention 2x scaling and all.) In addition, the dimensions speced for this aren't really taken from toolbar look, but from location bar icon spacing looks. I spent some time searching around in there for the right thing, but ended up just putting in a number and calling it a day. The location bar ends up getting this from a kind of mix of padding and spacings and icon internal widths around the leading decorations class and the text field. We could probably recalculate that, but I think it'd end up being harder to understand. https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:62: return chrome::ShouldDisplayOriginChip(); On 2013/12/03 00:58:10, Peter Kasting wrote: > Are you planning on some more complex functionality here later? Yes. There's still some policy issues around whether we show this for the NTP, for instance. My current later changes have some logic around that. It may end up being just a fall-through to the experiment control, but this gives us a handle to tweak things without fussing with ToolbarView. https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:72: toolbar_view_->location_bar()->GetOmniboxFontList()); On 2013/12/03 00:58:10, Peter Kasting wrote: > Instead of a GetOmniboxFontList() accessor, we should just pass the font list in > at Init() or construction time, which is how we do the fonts for e.g. the EV > bubble, keyword search bubble, etc. This FontList is calculated in LocationBarView. I at one point had copy-pasted that calculation here, but I think keeping that in one spot is wise. It could arguably be brought one level higher to the ToolbarView and then passed down. I haven't heard from UI whether we even want to mimic the font size of the omnibox in the site chip. I think it ends up looking a bit big, but that does make it more visible... https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:90: kIconTextSpacing; On 2013/12/03 00:58:10, Peter Kasting wrote: > Nit: Indent 4, not even Done. https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:92: kTrailingLabelMargin; On 2013/12/03 00:58:10, Peter Kasting wrote: > Nit: Break after "=" to avoid wrapping the calculation Done. https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:102: views::Painter::CreateImageGridPainter(kBackgroundImages)); On 2013/12/03 00:58:10, Peter Kasting wrote: > Creating this painter in every OnPaint() call is heavyweight. Let's create it > once during Init(). Oops! Thanks for reminding me. Done. https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:105: LocationBarView::kNormalEdgeThickness); On 2013/12/03 00:58:10, Peter Kasting wrote: > Can we avoid this inset call by just setting our own insets to > kNormalEdgeThickness on every side? This will probably change Layout() and > GetPreferredSize() too (but maybe by making them simpler). I think I tried that, but that brings the border painting in along with it, as I recall. I remember running into difficulties, but perhaps they were surmountable. https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:113: toolbar_view_->location_bar()->GetOmniboxView()->SetFocus(); On 2013/12/03 00:58:10, Peter Kasting wrote: > Nit: You probably still want a TODO here to make the LocationBarView or > OmniboxView be the button listener eventually. Done. https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:119: bool SiteChipView::DrawingBackground() { On 2013/12/03 00:58:10, Peter Kasting wrote: > Nit: This isn't called or implemented yet, so I'd just leave it out until it's > more fleshed out. deleted. At one point I thought we'd need the margin calculation to paint the background shading, but that was wrong. https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.h (right): https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:35: bool ShouldShow(); On 2013/12/03 00:58:10, Peter Kasting wrote: > Nit: Comment? Done. https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:49: // Return true if we will draw an opaque background On 2013/12/03 00:58:10, Peter Kasting wrote: > Nit: Return -> Returns; wrap as close to 80 cols as possible; comments should > end with a period Not using this any more. Deleting. https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:51: bool DrawingBackground(); On 2013/12/03 00:58:10, Peter Kasting wrote: > Nit: Place all non-virtuals in a group deleted https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:56: static const int kTrailingLabelMargin; On 2013/12/03 00:58:10, Peter Kasting wrote: > Nit: I suggest a blank line below here > > Really though, these can probably be file-scope constants instead of > class-scope. Done. https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:491: int site_chip_spacing = kStandardSpacing - 1; On 2013/12/03 00:58:10, Peter Kasting wrote: > Nit: Don't calculate this value in two different places. > > Why is the -1 necessary? What about the site chip imagery makes the spacing > need to be different? This is just an artifact of how the images are produced -- the distinct visual edge of the border is set in 2 pixels. And the spacing specified by UI is 1 pixel more than the kStandardSpacing distance. https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:501: 2*site_chip_spacing + 2*button_spacing : 0) + On 2013/12/03 00:58:10, Peter Kasting wrote: > Nit: Spaces around operators > > Indent this line 4 more, then wrap after ':', so it's obvious how the expression > is constructed. I'd place parens around the entire subexpression as well (i.e. > "x ? (1 + 2 + 3) : 0"). Done. https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:593: int browser_actions_x = location_bar_->x() + location_bar_->width(); On 2013/12/03 00:58:10, Peter Kasting wrote: > So we put the browser actions flush against the location bar, but not flush > against the site chip? That seems a bit odd. The browser actions are borderless, so this is giving us room to draw the site chip border around the edges of the internal controls. https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:596: site_chip_view_->SetBounds(location_bar_->x() + location_bar_->width() + On 2013/12/03 00:58:10, Peter Kasting wrote: > Nit: Replace first two terms with "browser_actions_x" Done. https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:601: browser_actions_x += site_chip_view_->GetPreferredSize().width() + On 2013/12/03 00:58:10, Peter Kasting wrote: > Nit: Break after "+=" instead to avoid wrapping the calculation Done.
https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:32: const int SiteChipView::kTrailingLabelMargin = 3; On 2013/12/03 17:35:08, Greg Billock wrote: > Yeah, these are definitely unsatisfying. The deal with the icon-text spacing is > an artifact, again, of the visual extent of the specific lock/page icons we're > using. They don't go all the way to the edge, or we could do some calculation > based on that. Getting new assets for all those might make these spacing > calculations more adaptive and obvious, but even then, the visual edge may not > be the image edge, given shading, transparency, etc. (Not to mention 2x scaling > and all.) In addition, the dimensions speced for this aren't really taken from > toolbar look, but from location bar icon spacing looks. I spent some time > searching around in there for the right thing, but ended up just putting in a > number and calling it a day. The location bar ends up getting this from a kind > of mix of padding and spacings and icon internal widths around the leading > decorations class and the text field. We could probably recalculate that, but I > think it'd end up being harder to understand. We're using the same icons for the location icon in the omnibox as the location icon in the site chip. It seems like we can use the same constants around its spacing. In particular, I'd like to ignore the precise pixel offsets given in the mocks where following them requires hardcoding random constants or they don't match existing omnibox assets. Mocks are mocks, they are not gospel, and we can override them where doing so makes things look or work better. Give me a screenshot of precisely how things look right now and I can tell you what I'd change. This is not the sort of thing that has to get fixed for this patch to land, but it should get fixed -- we shouldn't ultimately wind up with a bunch of magic numbers like this. https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:72: toolbar_view_->location_bar()->GetOmniboxFontList()); On 2013/12/03 17:35:08, Greg Billock wrote: > It could > arguably be brought one level higher to the ToolbarView and then passed down. If we want the same fonts in both locations, I might do that, but: > I > haven't heard from UI whether we even want to mimic the font size of the omnibox > in the site chip. I probably wouldn't. I'd just use the BaseFont or MediumFont to start with.
On 2013/12/03 19:08:19, Peter Kasting wrote: > https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... > File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): > > https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... > chrome/browser/ui/views/toolbar/site_chip_view.cc:32: const int > SiteChipView::kTrailingLabelMargin = 3; > On 2013/12/03 17:35:08, Greg Billock wrote: > > Yeah, these are definitely unsatisfying. The deal with the icon-text spacing > is > > an artifact, again, of the visual extent of the specific lock/page icons we're > > using. They don't go all the way to the edge, or we could do some calculation > > based on that. Getting new assets for all those might make these spacing > > calculations more adaptive and obvious, but even then, the visual edge may not > > be the image edge, given shading, transparency, etc. (Not to mention 2x > scaling > > and all.) In addition, the dimensions speced for this aren't really taken from > > toolbar look, but from location bar icon spacing looks. I spent some time > > searching around in there for the right thing, but ended up just putting in a > > number and calling it a day. The location bar ends up getting this from a kind > > of mix of padding and spacings and icon internal widths around the leading > > decorations class and the text field. We could probably recalculate that, but > I > > think it'd end up being harder to understand. > > We're using the same icons for the location icon in the omnibox as the location > icon in the site chip. It seems like we can use the same constants around its > spacing. These are kind of smeared about in LocationBarView and LocationBarLayout. I spent a fair amount of time digging there, trying to see how to either re-use the LocationBarLayout, or extract constants. Ultimately, I didn't find anything that seemed better than new constants. :-( The math of how it does layout is pretty complicated, with multi-pass layout accounting for various context-sensitive constants, most of which are just "8" or "1" and such. There's some re-usability -- we want our edge thickness to be the same as the omnibox, for instance, so I'm reusing that. The internal paddings are less re-usable. > In particular, I'd like to ignore the precise pixel offsets given in the mocks > where following them requires hardcoding random constants or they don't match > existing omnibox assets. Mocks are mocks, they are not gospel, and we can > override them where doing so makes things look or work better. Give me a > screenshot of precisely how things look right now and I can tell you what I'd > change. > > This is not the sort of thing that has to get fixed for this patch to land, but > it should get fixed -- we shouldn't ultimately wind up with a bunch of magic > numbers like this. I totally agree, and if we want to push on UI to just use existing spacing constants, we can definitely re-use a couple more (i.e. LocationBarView::GetItemPadding). But even there, that constant is touch-screen sensitive and it isn't clear we want to follow that for the site chip. Honestly, while fiddling, I think there's a couple pixels wiggle room that still look ok, but Alan's spacings I agree are at peak awesome. I don't think we can get to the peak without our own constants. > https://codereview.chromium.org/75873002/diff/290001/chrome/browser/ui/views/... > chrome/browser/ui/views/toolbar/site_chip_view.cc:72: > toolbar_view_->location_bar()->GetOmniboxFontList()); > On 2013/12/03 17:35:08, Greg Billock wrote: > > It could > > arguably be brought one level higher to the ToolbarView and then passed down. > > If we want the same fonts in both locations, I might do that, but: > > > I > > haven't heard from UI whether we even want to mimic the font size of the > omnibox > > in the site chip. > > I probably wouldn't. I'd just use the BaseFont or MediumFont to start with. That's legit. It's how I started, but it looked really small. Using the Omnibox fontlist looks better. Using the omnibox fontlist but lighter weight might be the best approach.
https://codereview.chromium.org/75873002/diff/310001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/75873002/diff/310001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:29: const int kEdgeThickness = 4; I think we can get the right value for this and kTrailingLabelMargin, at least, if we replace both as well as LocationBarView's kDesktopItemPadding with ToolbarView's kStandardSpacing; and then use LocationBarView's kNormalEdgeThickness (which maybe should be moved to the ToolbarView too?) as the thickness of the edge. So the icon would be at kNormalEdgeThickness + kStandardSpacing = 5, which matches my comment on the bug that this should be 1 more than the current value to match the omnibox. https://codereview.chromium.org/75873002/diff/310001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:94: kIconTextSpacing; Nit: I didn't mean "Indent even + 4" -- just "Indent 4", i.e. 4 from "int hos..." at the beginning of the previous line. https://codereview.chromium.org/75873002/diff/310001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:107: if (background_painter_.get()) Nit: get() unnecessary Can we ever reach here with this NULL? It doesn't seem like it. Though maybe we can in the future when we don't always have a background? https://codereview.chromium.org/75873002/diff/310001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.h (right): https://codereview.chromium.org/75873002/diff/310001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:35: // Return whether the Site Chip should be shown in the toolbar. Nit: Return -> Returns, and similar changes to any other function comments (e.g. Recalculate below) Might want to say when this would return true? https://codereview.chromium.org/75873002/diff/310001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/toolbar_view.h (right): https://codereview.chromium.org/75873002/diff/310001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.h:159: // The apparent horizontal space around the site chip. Based on my comments on the bug: we should nuke this and use kStandardSpacing instead. Using 2 instead of 3 here just looks strange.
Also: using MediumFont per Alan. Removing the omnibox font access. https://codereview.chromium.org/75873002/diff/310001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/75873002/diff/310001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:29: const int kEdgeThickness = 4; On 2013/12/03 23:04:52, Peter Kasting wrote: > I think we can get the right value for this and kTrailingLabelMargin, at least, > if we replace both as well as LocationBarView's kDesktopItemPadding with > ToolbarView's kStandardSpacing; and then use LocationBarView's > kNormalEdgeThickness (which maybe should be moved to the ToolbarView too?) as > the thickness of the edge. > > So the icon would be at kNormalEdgeThickness + kStandardSpacing = 5, which > matches my comment on the bug that this should be 1 more than the current value > to match the omnibox. I have no problem with making those changes. I don't want to end up going back and forth depending on what UI folks say, though. Can we submit this and then work it out with UI? I'd like to get the functionality into the review stage, and I think we have the knobs we need to adjust the pixel perfection. These knobs are governing the internal margins, and the inter- and trailing paddings, so I'm certain we have enough leverage there to make point changes to share constants where possible. I don't think we want to fiddle too much with our 9-box graphics and location icon images, since those aren't part of the experiment. So we may end up needing to nudge by a pixel here or there to get the experiment closer to what in the end we may achieve by just adjusting graphics visually. https://codereview.chromium.org/75873002/diff/310001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:94: kIconTextSpacing; On 2013/12/03 23:04:52, Peter Kasting wrote: > Nit: I didn't mean "Indent even + 4" -- just "Indent 4", i.e. 4 from "int > hos..." at the beginning of the previous line. Done. https://codereview.chromium.org/75873002/diff/310001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:107: if (background_painter_.get()) On 2013/12/03 23:04:52, Peter Kasting wrote: > Nit: get() unnecessary > > Can we ever reach here with this NULL? It doesn't seem like it. Though maybe > we can in the future when we don't always have a background? Correct. (This will be the usual case in the end.) https://codereview.chromium.org/75873002/diff/310001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.h (right): https://codereview.chromium.org/75873002/diff/310001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:35: // Return whether the Site Chip should be shown in the toolbar. On 2013/12/03 23:04:52, Peter Kasting wrote: > Nit: Return -> Returns, and similar changes to any other function comments (e.g. > Recalculate below) > > Might want to say when this would return true? Done. https://codereview.chromium.org/75873002/diff/310001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/toolbar_view.h (right): https://codereview.chromium.org/75873002/diff/310001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.h:159: // The apparent horizontal space around the site chip. On 2013/12/03 23:04:52, Peter Kasting wrote: > Based on my comments on the bug: we should nuke this and use kStandardSpacing > instead. Using 2 instead of 3 here just looks strange. Done.
LGTM https://codereview.chromium.org/75873002/diff/310001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/75873002/diff/310001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:29: const int kEdgeThickness = 4; On 2013/12/04 00:33:34, Greg Billock wrote: > I have no problem with making those changes. I don't want to end up going back > and forth depending on what UI folks say, though. Can we submit this and then > work it out with UI? Yes, though I don't think this change is very controversial -- the one about space between the location bar and the things on either side seems more up in the air. https://codereview.chromium.org/75873002/diff/350001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/75873002/diff/350001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:32: const int kTrailingLabelMargin = 2; This change doesn't seem to match anything in the new mock or anything that I requested. He changed the spacing between the outer edge of the chip and the menu button, but not the spacing between the inside of the edge and the trailing end of the label.
Forgot to send this https://codereview.chromium.org/75873002/diff/350001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.h (right): https://codereview.chromium.org/75873002/diff/350001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:36: // true if the experiment is enabled and the site chip should be shown. Nit: Hmm, this added second sentence just sounds redundant. I guess I was thinking more like: Returns true if the site chip experiment is enabled and thus the site chip should be shown in the toolbar. (Really I was thinking the condition was more complex and we'd have more to say here but I guess it currently isn't.)
https://codereview.chromium.org/75873002/diff/350001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/75873002/diff/350001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:32: const int kTrailingLabelMargin = 2; On 2013/12/04 00:55:30, Peter Kasting wrote: > This change doesn't seem to match anything in the new mock or anything that I > requested. He changed the spacing between the outer edge of the chip and the > menu button, but not the spacing between the inside of the edge and the trailing > end of the label. Yeah. This is something Alan and I were discussing on IM. I expect these to all mutate a bit -- hopefully disappearing as we coalesce with the toolbar constants. https://codereview.chromium.org/75873002/diff/350001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.h (right): https://codereview.chromium.org/75873002/diff/350001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:36: // true if the experiment is enabled and the site chip should be shown. On 2013/12/04 00:57:31, Peter Kasting wrote: > Nit: Hmm, this added second sentence just sounds redundant. I guess I was > thinking more like: > > Returns true if the site chip experiment is enabled and thus the site chip > should be shown in the toolbar. > > (Really I was thinking the condition was more complex and we'd have more to say > here but I guess it currently isn't.) Yeah, the choice to just always show it simplifies things. I'll clean this in the subsequent change. CQing now to try to hit tonight's canary.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/75873002/350001
Retried try job too often on linux_chromeos for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Switched to not adding the site chip as a child if it isn't shown -- easiest way to deal with interactive UI focus tests. https://codereview.chromium.org/75873002/diff/350001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.h (right): https://codereview.chromium.org/75873002/diff/350001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.h:36: // true if the experiment is enabled and the site chip should be shown. On 2013/12/04 04:05:56, Greg Billock wrote: > On 2013/12/04 00:57:31, Peter Kasting wrote: > > Nit: Hmm, this added second sentence just sounds redundant. I guess I was > > thinking more like: > > > > Returns true if the site chip experiment is enabled and thus the site chip > > should be shown in the toolbar. > > > > (Really I was thinking the condition was more complex and we'd have more to > say > > here but I guess it currently isn't.) > > Yeah, the choice to just always show it simplifies things. I'll clean this in > the subsequent change. CQing now to try to hit tonight's canary. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/75873002/370001
Message was sent while issue was closed.
Change committed as 238732
Message was sent while issue was closed.
On 2013/12/04 17:01:17, Greg Billock wrote: > Switched to not adding the site chip as a child if it isn't shown -- easiest way > to deal with interactive UI focus tests. Oh? Why did these break? Non-visible children aren't supposed to allow themselves to receive focus, I thought. It seems like the better route would be to see why this isn't happening (or why the tests are breaking for some other reason).
Message was sent while issue was closed.
On 2013/12/04 20:02:50, Peter Kasting wrote: > On 2013/12/04 17:01:17, Greg Billock wrote: > > Switched to not adding the site chip as a child if it isn't shown -- easiest > way > > to deal with interactive UI focus tests. > > Oh? Why did these break? Non-visible children aren't supposed to allow > themselves to receive focus, I thought. > > It seems like the better route would be to see why this isn't happening (or why > the tests are breaking for some other reason). It was breaking because of this -- basically a child in the focus chain messed up the counting. I spent a fair amount of time investigating the test and the focus settings, but ended up taking the easy route for now. Will investigate further.
Message was sent while issue was closed.
https://codereview.chromium.org/75873002/diff/370001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/75873002/diff/370001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:232: site_chip_view_ = new SiteChipView(this); drive by from sheriff: I think this might leak if AddChildView() isn't called below
Message was sent while issue was closed.
Hi Greg, We had to revert this change due to ASAN bot failures. http://build.chromium.org/p/chromium.memory/builders/Linux%20Chromium%20OS%20... Here's the revert CL https://codereview.chromium.org/98493004/
Message was sent while issue was closed.
https://codereview.chromium.org/75873002/diff/370001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/75873002/diff/370001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:232: site_chip_view_ = new SiteChipView(this); On 2013/12/04 20:43:52, James Cook wrote: > drive by from sheriff: I think this might leak if AddChildView() isn't called > below Yes, it will.
Message was sent while issue was closed.
On 2013/12/04 21:23:51, Peter Kasting wrote: > https://codereview.chromium.org/75873002/diff/370001/chrome/browser/ui/views/... > File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): > > https://codereview.chromium.org/75873002/diff/370001/chrome/browser/ui/views/... > chrome/browser/ui/views/toolbar/toolbar_view.cc:232: site_chip_view_ = new > SiteChipView(this); > On 2013/12/04 20:43:52, James Cook wrote: > > drive by from sheriff: I think this might leak if AddChildView() isn't called > > below > > Yes, it will. Yep, that was the problem. Sorry about that.
On 2013/12/04 21:24:57, Greg Billock wrote: > On 2013/12/04 21:23:51, Peter Kasting wrote: > > > https://codereview.chromium.org/75873002/diff/370001/chrome/browser/ui/views/... > > File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): > > > > > https://codereview.chromium.org/75873002/diff/370001/chrome/browser/ui/views/... > > chrome/browser/ui/views/toolbar/toolbar_view.cc:232: site_chip_view_ = new > > SiteChipView(this); > > On 2013/12/04 20:43:52, James Cook wrote: > > > drive by from sheriff: I think this might leak if AddChildView() isn't > called > > > below > > > > Yes, it will. > > Yep, that was the problem. Sorry about that. ok, fixed up now. I was just too anxious in 10 and didn't look close enough to home -- the cause for the focus interactive ui test problems is that the superclass sets focusable, which we can just reset in the site chip Init method. Problem solved the right way.
https://codereview.chromium.org/75873002/diff/380001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/75873002/diff/380001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:52: set_focusable(false); I'm still not convinced this should be needed as long as our embedder is properly doing SetVisible(false) on us when we don't show. It really seems like a View should not accept focus if it's not visible. set_focusable() is just supposed to control whether, in theory, a visible view should allow taking focus. Nit: In any case, I'd do this adjacent to the Init() call. https://codereview.chromium.org/75873002/diff/380001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:82: return gfx::Size(); Is this really necessary? It doesn't seem like we should need to return an empty size ever; our embedder should just not include our size in its calculations in that case.
https://codereview.chromium.org/75873002/diff/380001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/75873002/diff/380001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:52: set_focusable(false); On 2013/12/04 22:30:51, Peter Kasting wrote: > I'm still not convinced this should be needed as long as our embedder is > properly doing SetVisible(false) on us when we don't show. It really seems like > a View should not accept focus if it's not visible. set_focusable() is just > supposed to control whether, in theory, a visible view should allow taking > focus. > > Nit: In any case, I'd do this adjacent to the Init() call. I see what you mean. It looks in view.cc like that works -- SetEnabled and SetVisible should both mean IsFocusable is false. It looks wise to set them both to the value of ShouldShow(). ok, tested and it works. Switched to doing that by the Init() call in ToolbarView. https://codereview.chromium.org/75873002/diff/380001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:82: return gfx::Size(); On 2013/12/04 22:30:51, Peter Kasting wrote: > Is this really necessary? It doesn't seem like we should need to return an > empty size ever; our embedder should just not include our size in its > calculations in that case. Yes, this is more an abundance of caution -- if we're disabled, we should be 0 size.
https://codereview.chromium.org/75873002/diff/380001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/75873002/diff/380001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:82: return gfx::Size(); On 2013/12/05 00:00:46, Greg Billock wrote: > On 2013/12/04 22:30:51, Peter Kasting wrote: > > Is this really necessary? It doesn't seem like we should need to return an > > empty size ever; our embedder should just not include our size in its > > calculations in that case. > > Yes, this is more an abundance of caution -- if we're disabled, we should be 0 > size. I wouldn't do that. That's not how other parts of Views normally work, so this can be confusing. https://codereview.chromium.org/75873002/diff/400001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/75873002/diff/400001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:247: site_chip_view_->SetEnabled(site_chip_view_->ShouldShow()); Is this call necessary? It seems like the site chip should always be enabled, which would also be its default state. https://codereview.chromium.org/75873002/diff/400001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:248: site_chip_view_->SetVisible(site_chip_view_->ShouldShow()); Do this in Layout() in the same place where we set the bounds.
https://codereview.chromium.org/75873002/diff/380001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/site_chip_view.cc (right): https://codereview.chromium.org/75873002/diff/380001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/site_chip_view.cc:82: return gfx::Size(); On 2013/12/05 00:03:48, Peter Kasting wrote: > On 2013/12/05 00:00:46, Greg Billock wrote: > > On 2013/12/04 22:30:51, Peter Kasting wrote: > > > Is this really necessary? It doesn't seem like we should need to return an > > > empty size ever; our embedder should just not include our size in its > > > calculations in that case. > > > > Yes, this is more an abundance of caution -- if we're disabled, we should be 0 > > size. > > I wouldn't do that. That's not how other parts of Views normally work, so this > can be confusing. Done. https://codereview.chromium.org/75873002/diff/400001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/75873002/diff/400001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:247: site_chip_view_->SetEnabled(site_chip_view_->ShouldShow()); On 2013/12/05 00:03:48, Peter Kasting wrote: > Is this call necessary? It seems like the site chip should always be enabled, > which would also be its default state. Done. https://codereview.chromium.org/75873002/diff/400001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:248: site_chip_view_->SetVisible(site_chip_view_->ShouldShow()); On 2013/12/05 00:03:48, Peter Kasting wrote: > Do this in Layout() in the same place where we set the bounds. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/75873002/420001
LGTM https://codereview.chromium.org/75873002/diff/420001/chrome/browser/ui/views/... File chrome/browser/ui/views/toolbar/toolbar_view.cc (right): https://codereview.chromium.org/75873002/diff/420001/chrome/browser/ui/views/... chrome/browser/ui/views/toolbar/toolbar_view.cc:595: if (site_chip_view_->ShouldShow()) { Nit: Can now change this to call visible() instead, which is guaranteed to always be a cheap accessor.
Message was sent while issue was closed.
Change committed as 238890 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
