alternate ntp: implement right-aligned search token

alternate ntp: implement right-aligned search token

implement separator handling in layout system:
- separator behaves like a decoration, which is now 1 of 3 types: normal, auto-collapse or separator
- layout system decides if separator should show depending on if:
  * prev decoration is visible
  * separator is by the edge (leading) or trailing
  * 2 separators are side by side

when omnibox replaces query terms with the url and has enough space, show right-aligned search token in omnibox:
- it has text "<Search provider> Search", and a divider on the right if there's neighboring decorations.
- it's on the right side of the omnibox, and left of all icons.
- it disappears when query in omnibox is too long.

BUG=163190
TEST=verify per bug rpt
TBR=sky for chrome dir

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177601

[beaudoin, 2012-11-29 15:07:49]

Some ideas on ways to clean-up using a bool or enum.

https://codereview.chromium.org/11418229/diff/2001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11418229/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:712: ev_bubble_width =
ev_bubble_view_->GetPreferredSize().width();
Could we go with just a bool or an enum here and keep the size calculations for
later?
(Then, we could remove the cut-and-paste for calculating ev_bubble_width.)

https://codereview.chromium.org/11418229/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:725: int
entry_width_for_left_of_action_box = entry_width;
Comment and variable name here are hard to understand. I would personally prefer
a bool that you set to false in any of the "ifs" below, but the approach has
drawbacks too, so you can keep this approach. Just make sure you clean-up the
comment to mention that the only goal of this "int" is to determine whether any
other decoration is added.

https://codereview.chromium.org/11418229/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:764: if
(location_icon_width == 0) {  // EV bubble is on the left of edit box.
Again, I think a bool/enum would make this much easier to understand.

https://codereview.chromium.org/11418229/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:814: bool
ev_bubble_on_right = ev_bubble_width > 0 && location_icon_width > 0;
I would set that bool much earlier, as soon as you know it's on the right.
Probably start it as false and turn it true inside the "if" at line 707.

Better yet: a 3-way enum:
NOT_VISIBLE, VISIBLE_ON_LEFT, VISIBLE_ON_RIGHT
that could be used instead of ev_bubble_width as an indicator. It would make the
entire code clearer IMHO.

[kuan, 2012-11-29 16:48:27]

i've addressed beaudoin's comments in patch set 3.  ptal.  thx.

https://codereview.chromium.org/11418229/diff/2001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11418229/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:712: ev_bubble_width =
ev_bubble_view_->GetPreferredSize().width();
On 2012/11/29 15:07:50, beaudoin wrote:
> Could we go with just a bool or an enum here and keep the size calculations
for
> later?
> (Then, we could remove the cut-and-paste for calculating ev_bubble_width.)

Done.

https://codereview.chromium.org/11418229/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:725: int
entry_width_for_left_of_action_box = entry_width;
On 2012/11/29 15:07:50, beaudoin wrote:
> Comment and variable name here are hard to understand. I would personally
prefer
> a bool that you set to false in any of the "ifs" below, but the approach has
> drawbacks too, so you can keep this approach. Just make sure you clean-up the
> comment to mention that the only goal of this "int" is to determine whether
any
> other decoration is added.

Done.  i tried your bool approach before, but felt it was too repetitive, so i
changed to using an int.

https://codereview.chromium.org/11418229/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:764: if
(location_icon_width == 0) {  // EV bubble is on the left of edit box.
On 2012/11/29 15:07:50, beaudoin wrote:
> Again, I think a bool/enum would make this much easier to understand.

Done.

https://codereview.chromium.org/11418229/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:814: bool
ev_bubble_on_right = ev_bubble_width > 0 && location_icon_width > 0;
On 2012/11/29 15:07:50, beaudoin wrote:
> I would set that bool much earlier, as soon as you know it's on the right.
> Probably start it as false and turn it true inside the "if" at line 707.
> 
> Better yet: a 3-way enum:
> NOT_VISIBLE, VISIBLE_ON_LEFT, VISIBLE_ON_RIGHT
> that could be used instead of ev_bubble_width as an indicator. It would make
the
> entire code clearer IMHO.

Done.

[dhollowa, 2012-11-29 17:01:26]

lgtm

https://codereview.chromium.org/11418229/diff/7001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11418229/diff/7001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:724: // We'll
determine if EV bubble is beside edit box later.
nit: "Determine...below".

[kuan, 2012-11-29 17:19:20]

i've addressed david's comments in patch set 4.  ptal.  thx.

https://codereview.chromium.org/11418229/diff/7001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11418229/diff/7001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:724: // We'll
determine if EV bubble is beside edit box later.
On 2012/11/29 17:01:26, dhollowa wrote:
> nit: "Determine...below".

Done.

[sky, 2012-11-29 20:23:24]

Peter is a better reviewer for EV bubble related changes.

[beaudoin, 2012-11-30 01:24:30]

lgtm
I quite like your bitfield approach. Very nice!

https://codereview.chromium.org/11418229/diff/2001/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11418229/diff/2001/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:725: int
entry_width_for_left_of_action_box = entry_width;
On 2012/11/29 16:48:27, kuan wrote:
> On 2012/11/29 15:07:50, beaudoin wrote:
> > Comment and variable name here are hard to understand. I would personally
> prefer
> > a bool that you set to false in any of the "ifs" below, but the approach has
> > drawbacks too, so you can keep this approach. Just make sure you clean-up
the
> > comment to mention that the only goal of this "int" is to determine whether
> any
> > other decoration is added.
> 
> Done.  i tried your bool approach before, but felt it was too repetitive, so i
> changed to using an int.

Ack.

[kuan, 2012-12-01 00:02:13]

gideon, pls approve the string in generated_resources.grd.
peter, pls look at everything.
thx.

[gideonwald, 2012-12-01 00:09:30]

On 2012/12/01 00:02:13, kuan wrote:
> gideon, pls approve the string in generated_resources.grd.
> peter, pls look at everything.
> thx.

New GRD string LGTM. Thanks Kuan!

[Peter Kasting, 2012-12-01 01:29:37]

This implementation is too complex: we shouldn't need something like the enum
you've added.  Instead of trying to move a single "ev_bubble" around -- which is
also misleading, since the thing we're showing here is for non-EV anyway -- just
have two separate instances of the underlying EVBubbleView (which we should
probably rename SecurityIconView or something), one for the real "EV bubble" and
one for this other thing.

Positioning-wise, it seems like this should always be furthest-right in the
address bar even when the action box exists.  Otherwise, we don't agree on
whether functional icons inside the bar go to the left or right of this object. 
The bug is pretty clear on this always going to the right of everything, too. 
This should simplify the positioning.  OTOH you may have to change how the
action box is implemented.  If so, I suggest checking to see if we can just rip
out the action box entirely instead of spending time fixing it.

[beaudoin, 2012-12-02 14:26:31]

On 2012/12/01 01:29:37, Peter Kasting wrote:
> This implementation is too complex: we shouldn't need something like the enum
> you've added.  Instead of trying to move a single "ev_bubble" around -- which
is
> also misleading, since the thing we're showing here is for non-EV anyway --
just
> have two separate instances of the underlying EVBubbleView (which we should
> probably rename SecurityIconView or something), one for the real "EV bubble"
and
> one for this other thing.
> 
> Positioning-wise, it seems like this should always be furthest-right in the
> address bar even when the action box exists.  Otherwise, we don't agree on
> whether functional icons inside the bar go to the left or right of this
object. 
> The bug is pretty clear on this always going to the right of everything, too. 
> This should simplify the positioning.  OTOH you may have to change how the
> action box is implemented.  If so, I suggest checking to see if we can just
rip
> out the action box entirely instead of spending time fixing it.

I like the approach of two EVBubbleView. Wish I had thought of it. Would suggest
"SecurityBubbleView" though since the thing is not really an icon.

Also, this method seems ripe for a major refactoring. I wish there was a more
high-level way to specify positioning and spacing of omnibox thingies. Right now
the logic is just too twisted. I've added issue 163820 for this. Le me know over
there what you think.

"Rip out the action box" as in "take it away entirely"? If so, we should
double-check with the browser+ folks, I think it may be doing a comeback.
@stromme: Can you confirm?

[beaudoin, 2012-12-02 14:29:31]

+stromme to answer question in previous reply.

[stromme1, 2012-12-03 05:07:34]

On 2012/12/02 14:29:31, beaudoin wrote:
> "Rip out the action box" as in "take it away entirely"? If so, we should
double-check with the browser+ folks, I think it may be doing a comeback.

Yes, the action box will likely make a comeback. We had most everything worked
out but now I'm pausing again to consider apps V2 to make sure that it makes
sense going forward. In any case, it will use the same menu and visual
structure, so I would appreciate not ripping it out.

[kuan, 2012-12-03 23:34:14]

i've re-implemented the cl per peter's and philippe's comments.

however, i've not modified the action box when it's not the right edge item:
it's missing the right edge (which was intentional 'cos it was expected to use
the right edge of the location bar).  based on what i've figured out, this would
require:
- new hover and pressed images with the right edge
- public method in ActionBoxButtonView to switch between the images w/ and w/out
right edges, and LocationBarView calling this method when laying out action box.

should i impl the above in this cl or add a TODO for when action box is enabled
by default?

[Peter Kasting, 2012-12-04 00:06:35]

On 2012/12/03 23:34:14, kuan wrote:
> i've re-implemented the cl per peter's and philippe's comments.
> 
> however, i've not modified the action box when it's not the right edge item:
> it's missing the right edge (which was intentional 'cos it was expected to use
> the right edge of the location bar).  based on what i've figured out, this
would
> require:
> - new hover and pressed images with the right edge
> - public method in ActionBoxButtonView to switch between the images w/ and
w/out
> right edges, and LocationBarView calling this method when laying out action
box.
> 
> should i impl the above in this cl or add a TODO for when action box is
enabled
> by default?

[Peter Kasting, 2012-12-04 00:08:19]

Oops, sorry for that last message, misclicked "send"...

I left some comments on the bug about some appearance changes that
Gideon/Justin/I decided on.  Unfortunately they end up meaning we'll revert a
number of the things you just got done writing (e.g. ev bubble renaming), sorry
about that...

[stromme1, 2012-12-04 08:07:01]

I think it's fine to have the TODO for now, because I don't want to block
your work and it shouldn't be hard to update once we get the images.
beaudoin, feel free to disagree, you might be making those changes down the
road so I'll leave it up to you :)

[kuan, 2012-12-04 10:46:35]

now that we're duplicating the style of SelectedKeywordView but without the
icon, i'm hoping to pick u guys' brains on which approach to take:
1) enhance IconLabelView to function without an icon and extend that, or
2) create a new class that duplicates the background painting of IconLabelView
with a child label.

i'm more inclined towards 1, maybe rename SelectedKeywordView to
SearchProviderView (or suggestions for a better name??) that both the current
selected_keyword_view_ and the new search bubble view will use.
there'll be less code duplication.  i also assume we want both the current
selected keyword view and the new search bubble view to have the same style.

[kuan, 2012-12-04 12:23:35]

On 2012/12/04 10:46:35, kuan wrote:
> now that we're duplicating the style of SelectedKeywordView but without the
> icon, i'm hoping to pick u guys' brains on which approach to take:
> 1) enhance IconLabelView to function without an icon and extend that, or
> 2) create a new class that duplicates the background painting of IconLabelView
> with a child label.
> 
> i'm more inclined towards 1, maybe rename SelectedKeywordView to
> SearchProviderView (or suggestions for a better name??) that both the current
> selected_keyword_view_ and the new search bubble view will use.
> there'll be less code duplication.  i also assume we want both the current
> selected keyword view and the new search bubble view to have the same style.

hm.. .on second thought, i think approach 2 is better:
- the background painting is simply done by a views::HorizontalPainter, and
label-related fns that i need are simpler, so code duplication is very minor
- i can accomplish the same style as SelectedKeywordView by using the same
background images.

[beaudoin1, 2012-12-04 14:04:57]

Going with option (1) would you even have to extend IconLabelBubbleView?
Looking at SelectedKeywordView it looks like all the specialization is in
order to support a short and a long label.

If you don't have to extend, (1) would be my preferred option. The
non-icon-related boilerplate in icon_label_bubble_view.cc is not that much,
but I think that duplicating it would be a mistake, especially if we're
positive that these two labels should share the same style. Moreover, it
doesn't seem unreasonable for IconLabelBubbleView to be able to function
without an image_.



On Tue, Dec 4, 2012 at 7:23 AM, <kuan@chromium.org> wrote:

> On 2012/12/04 10:46:35, kuan wrote:
>
>> now that we're duplicating the style of SelectedKeywordView but without
>> the
>> icon, i'm hoping to pick u guys' brains on which approach to take:
>> 1) enhance IconLabelView to function without an icon and extend that, or
>> 2) create a new class that duplicates the background painting of
>> IconLabelView
>> with a child label.
>>
>
>  i'm more inclined towards 1, maybe rename SelectedKeywordView to
>> SearchProviderView (or suggestions for a better name??) that both the
>> current
>> selected_keyword_view_ and the new search bubble view will use.
>> there'll be less code duplication.  i also assume we want both the current
>> selected keyword view and the new search bubble view to have the same
>> style.
>>
>
> hm.. .on second thought, i think approach 2 is better:
> - the background painting is simply done by a views::HorizontalPainter, and
> label-related fns that i need are simpler, so code duplication is very
> minor
> - i can accomplish the same style as SelectedKeywordView by using the same
> background images.
>
>
https://codereview.chromium.**org/11418229/<https://codereview.chromium.org/1...
>

[kuan, 2012-12-04 14:50:18]

i've re-implemented cl as similar to tab-to-search ui, using approach 2 i.e.
modifying IconLabelBubbleView to function without an icon.

well, i hv implementations for both approaches - by the time philippe responded,
i've just gotten approach 1 to work.  i think philippe is right - approach 1
seems cleaner, i.e. if we're ok w/ IconLabelBubbleView functioning without an
icon.

ptal.  thx.

[kuan, 2012-12-04 14:51:19]

On 2012/12/04 14:50:18, kuan wrote:
> i've re-implemented cl as similar to tab-to-search ui, using approach 2 i.e.
> modifying IconLabelBubbleView to function without an icon.
> 
> well, i hv implementations for both approaches - by the time philippe
responded,
> i've just gotten approach 1 to work.  i think philippe is right - approach 1
> seems cleaner, i.e. if we're ok w/ IconLabelBubbleView functioning without an
> icon.
> 
> ptal.  thx.

sorry, typo: s/using approach 2/using approach 1/

[beaudoin, 2012-12-04 15:20:09]

LGTM with some optional suggestions.

https://codereview.chromium.org/11418229/diff/9004/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right):

https://codereview.chromium.org/11418229/diff/9004/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:34: // Set
|contained_image| to -1 if there's no icon.
I think in this case it would be OK/cleaner to have an overloaded constructor
without contained_image. Up to you.

https://codereview.chromium.org/11418229/diff/9004/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:42: void
SetImage(const gfx::ImageSkia& image);
Should maybe add a ClearImage(); to go full-circle with this. Again, up to you.

[Peter Kasting, 2012-12-04 21:51:08]

Without looking at the change (I'll do that tomorrow), my only comment on the
two approaches is that it's nice to have API and functional simplicity.  Having
IconLabelView make the icon optional makes it a little more complex to think
about.

OTOH, as Philippe mentioned, duplicating code is always sad, especially if you
want to keep behaviors in sync down the road.

So in the end it's a judgment call.

[Peter Kasting, 2012-12-05 21:13:19]

https://codereview.chromium.org/11418229/diff/9004/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right):

https://codereview.chromium.org/11418229/diff/9004/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:31: //   replaces
the URL with its query terms.
Nit: Instead of being so detailed here about the uses of this class, I'd just
say something simple and leave users to document things in more detail:

"View used to draw a bubble containing an optional icon followed by a label. 
This is used for e.g. the EV bubble and the tab-to-search UI."

https://codereview.chromium.org/11418229/diff/9004/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:34: // Set
|contained_image| to -1 if there's no icon.
On 2012/12/04 15:20:09, beaudoin wrote:
> I think in this case it would be OK/cleaner to have an overloaded constructor
> without contained_image. Up to you.

Because the two constructors' implementations would be so similar, I'd weakly
rather not.

https://codereview.chromium.org/11418229/diff/9004/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:42: void
SetImage(const gfx::ImageSkia& image);
On 2012/12/04 15:20:09, beaudoin wrote:
> Should maybe add a ClearImage(); to go full-circle with this. Again, up to
you.

I wouldn't do this if we don't need it; I'm not a fan of dead APIs.

https://codereview.chromium.org/11418229/diff/9004/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11418229/diff/9004/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:321: if
(chrome::search::IsInstantExtendedAPIEnabled(profile_)) {
I don't think you should check this here.  Let the code that controls replacing
the URL with the search terms decide when that is appropriate to trigger.  The
location bar view should just be a dumb handler for that state.

Also, this block should go right after the keyword hint view block.

https://codereview.chromium.org/11418229/diff/9004/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:749: if
(search_bubble_view_) {
This conditional can be removed once you make the change suggested above.

https://codereview.chromium.org/11418229/diff/9004/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:750:
search_bubble_view_->SetVisible(false);
You should be controlling the visibility of this object in Update(), not
Layout().

https://codereview.chromium.org/11418229/diff/9004/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:751: if
(model_->GetSecurityLevel() == ToolbarModel::SECURE &&
Nit: Again, I don't think you should check this.  Leave WouldReplace...() to do
that.  Besides, this check isn't even correct: you missed the EV_SECURE case.

https://codereview.chromium.org/11418229/diff/9004/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/11418229/diff/9004/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.h:498: // The search
bubble view with text "<Search provider> Search".
Nit: Say more about when this appears.

[kuan, 2012-12-11 21:10:29]

there's nothing to review for now; i just want to publish this, for the benefit
of mad and mathp, who might taking over this.

unfortunately, i think only the string GRD is usable.

https://codereview.chromium.org/11418229/diff/9004/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/icon_label_bubble_view.h (right):

https://codereview.chromium.org/11418229/diff/9004/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/icon_label_bubble_view.h:31: //   replaces
the URL with its query terms.
On 2012/12/05 21:13:20, Peter Kasting wrote:
> Nit: Instead of being so detailed here about the uses of this class, I'd just
> say something simple and leave users to document things in more detail:
> 
> "View used to draw a bubble containing an optional icon followed by a label. 
> This is used for e.g. the EV bubble and the tab-to-search UI."

Done.

https://codereview.chromium.org/11418229/diff/9004/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11418229/diff/9004/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:321: if
(chrome::search::IsInstantExtendedAPIEnabled(profile_)) {
On 2012/12/05 21:13:20, Peter Kasting wrote:
> I don't think you should check this here.  Let the code that controls
replacing
> the URL with the search terms decide when that is appropriate to trigger.  The
> location bar view should just be a dumb handler for that state.
> 
> Also, this block should go right after the keyword hint view block.

Done.

https://codereview.chromium.org/11418229/diff/9004/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:749: if
(search_bubble_view_) {
On 2012/12/05 21:13:20, Peter Kasting wrote:
> This conditional can be removed once you make the change suggested above.

Done.

https://codereview.chromium.org/11418229/diff/9004/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:750:
search_bubble_view_->SetVisible(false);
On 2012/12/05 21:13:20, Peter Kasting wrote:
> You should be controlling the visibility of this object in Update(), not
> Layout().

Done.

https://codereview.chromium.org/11418229/diff/9004/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.cc:751: if
(model_->GetSecurityLevel() == ToolbarModel::SECURE &&
On 2012/12/05 21:13:20, Peter Kasting wrote:
> Nit: Again, I don't think you should check this.  Leave WouldReplace...() to
do
> that.  Besides, this check isn't even correct: you missed the EV_SECURE case.

Done.

https://codereview.chromium.org/11418229/diff/9004/chrome/browser/ui/views/lo...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/11418229/diff/9004/chrome/browser/ui/views/lo...
chrome/browser/ui/views/location_bar/location_bar_view.h:498: // The search
bubble view with text "<Search provider> Search".
On 2012/12/05 21:13:20, Peter Kasting wrote:
> Nit: Say more about when this appears.

Done.

[kuan, 2012-12-13 02:28:18]

i've re-implemented cl as a right-aligned search token, as detailed in cl
description.  i've already gone through multiple iterations w/ cole to get
drawing and layout correct.  while awaiting final lgtm from cole, i think we're
very close, so i believe the code is ready for review.  ptal.  thx.

[kuan, 2012-12-13 02:33:24]

i think peter could be ooo, so i'm adding scott as reviewer.

[kuan, 2012-12-13 16:49:58]

On 2012/12/13 02:33:24, kuan wrote:
> i think peter could be ooo, so i'm adding scott as reviewer.

scott, peter's auto-reply indicates he's ooo till fri, so i'd appreciate it if u
cld review this; btw, it's no longer an EV bubble; it's just a token with a
label and divider.

[kuan, 2012-12-13 22:54:12]

we'll wait for peter to review the cl when he returns next mon, so i'm removing
sky@ as reviewer.

[beaudoin, 2012-12-17 14:01:43]

Two notes:
1) I'd go with Peter comments on my comments (ie. no overloaded constructor, no
dead APIs). He has much more experience in Chrome so his stylistic opinions are
more informed than mine.

2) I'm starting to work on: crbug.com/163820 as part of the fixit, which will
have a lot of impact on this. I'll ping this thread if I ever get to some CL
before this lands.

[Peter Kasting, 2012-12-17 21:01:00]

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:272:
search_token_view_->SetVisible(false);
Nit: For consistency and efficiency, can you make all the blocks in this
function have this order:

  foo = new Foo;
  foo->SetVisible(false);
  foo->SetFont(font_);
  AddChildView(foo);

This will do the minimum number of relayouts.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:928: // right.  As for
the padding to the left, the view itself manages it.
This isn't right, see comments in search_token_view.cc.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:931:
search_token_view_->SetVisible(pref_width < AvailableWidth(max_edit_width));
Use LayoutView() for calculating visibility and setting bounds, as we do above.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/search_token_view.cc (right):

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.cc:18: const int
kRightPadding = 8;
Don't do this for two reasons.

One is that these values shouldn't be custom-hardcoded.  You should be using the
same values that every other item in the omnibox is using for padding.

The other is that this view shouldn't be managing internal paddings anyway.  The
location bar should be laying it out where it needs to be.

Basically, your class should act exactly like the KeywordHintView both in terms
of who adjusts paddings where as well as the actual padding values and precise
point at which the view gets removed to save space.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.cc:26:
ToolbarModel::NONE, LocationBarView::BACKGROUND));
NONE isn't right here (or below).  We need to use the actual toolbar model
state.  Otherwise this will break if someone has a different background color
for secure modes.

This means the colors can be dynamic and need to be updated when the toolbar
model state changes.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.cc:30:
ToolbarModel::NONE, LocationBarView::TEXT), 64);  // 25% alpha.
Turning off auto-color and manually alpha-blending the text and background isn't
right.  Why not use the label color here?  And why disable auto-color in the
first place?  These will prevent weird artifacts from potentially occurring.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.cc:49: SetVisible(false);
It's not right for this class to set itself as invisible and then not set itself
as visible again anywhere.   Either this class shouldn't muck with its own
visibility at all, or it should manage it completely.  Right now you manage it
in two different classes and it looks like the implementation is buggy to boot.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.cc:64: void
SearchTokenView::OnPaintBorder(gfx::Canvas* canvas) {
Instead of this, why don't you simply use a real border? 
CreateSolidSidedBorder(0, 0, 0, 1, <color>) should do what you want.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/search_token_view.h (right):

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.h:13: 
Nit: I'd leave these two blank lines out.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.h:36: bool
HasSearchProvider() const {
Inline functions must be named unix_hacker()-style.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.h:49:
DISALLOW_IMPLICIT_CONSTRUCTORS(SearchTokenView);
Nit: This should be DISALLOW_COPY_AND_ASSIGN.

[kuan, 2012-12-20 00:26:14]

i've addressed peter's comments in patch set 13.  ptal.  thx.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:272:
search_token_view_->SetVisible(false);
On 2012/12/17 21:01:00, Peter Kasting wrote:
> Nit: For consistency and efficiency, can you make all the blocks in this
> function have this order:
> 
>   foo = new Foo;
>   foo->SetVisible(false);
>   foo->SetFont(font_);
>   AddChildView(foo);
> 
> This will do the minimum number of relayouts.

Done.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:928: // right.  As for
the padding to the left, the view itself manages it.
On 2012/12/17 21:01:00, Peter Kasting wrote:
> This isn't right, see comments in search_token_view.cc.

Done.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:931:
search_token_view_->SetVisible(pref_width < AvailableWidth(max_edit_width));
On 2012/12/17 21:01:00, Peter Kasting wrote:
> Use LayoutView() for calculating visibility and setting bounds, as we do
above.

i initially tried that, and discovered that LayoutView() only sets view
invisible if view's preferred/min width is less than location_bounds.width,
whereas i need to set view invisible if view's width is less than
AvailableWidth(max_edit_width).
e.g. when i manually resized the chrome window till the narrowest possible, the
left tab-to-search blue bubble always showed up, and instead the omnibox text
got shrunk.
the right-aligned search token, on the other hand, needs to disappear in this
scenario.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/search_token_view.cc (right):

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.cc:18: const int
kRightPadding = 8;
On 2012/12/17 21:01:00, Peter Kasting wrote:
> Don't do this for two reasons.
> 
> One is that these values shouldn't be custom-hardcoded.  You should be using
the
> same values that every other item in the omnibox is using for padding.
> 
> The other is that this view shouldn't be managing internal paddings anyway. 
The
> location bar should be laying it out where it needs to be.
> 
> Basically, your class should act exactly like the KeywordHintView both in
terms
> of who adjusts paddings where as well as the actual padding values and precise
> point at which the view gets removed to save space.

i've moved the paddings to location_bar_view.cc, but i still need them
custom-hardcoded there, 'cos the search token uses different paddings from the
other decorations.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.cc:26:
ToolbarModel::NONE, LocationBarView::BACKGROUND));
On 2012/12/17 21:01:00, Peter Kasting wrote:
> NONE isn't right here (or below).  We need to use the actual toolbar model
> state.  Otherwise this will break if someone has a different background color
> for secure modes.
> 
> This means the colors can be dynamic and need to be updated when the toolbar
> model state changes.

Done.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.cc:30:
ToolbarModel::NONE, LocationBarView::TEXT), 64);  // 25% alpha.
On 2012/12/17 21:01:00, Peter Kasting wrote:
> Turning off auto-color and manually alpha-blending the text and background
isn't
> right.  Why not use the label color here?  And why disable auto-color in the
> first place?  These will prevent weird artifacts from potentially occurring.

i removed the disabling of auto-color.  the reason why cole wants a 25% alpha is
because it appears too dark, compared to the text.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.cc:49: SetVisible(false);
On 2012/12/17 21:01:00, Peter Kasting wrote:
> It's not right for this class to set itself as invisible and then not set
itself
> as visible again anywhere.   Either this class shouldn't muck with its own
> visibility at all, or it should manage it completely.  Right now you manage it
> in two different classes and it looks like the implementation is buggy to
boot.

Done.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.cc:64: void
SearchTokenView::OnPaintBorder(gfx::Canvas* canvas) {
On 2012/12/17 21:01:00, Peter Kasting wrote:
> Instead of this, why don't you simply use a real border? 
> CreateSolidSidedBorder(0, 0, 0, 1, <color>) should do what you want.

i tried that, but it paints a 2-px thick right border in retina display while
cole only wants 1-px in both 1x and 2x displays; SkPaint::setStrokeWidth(0)
accomplishes the 1-px thick border in both 1x and 2x displays for me.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/search_token_view.h (right):

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.h:13: 
On 2012/12/17 21:01:00, Peter Kasting wrote:
> Nit: I'd leave these two blank lines out.

Done.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.h:36: bool
HasSearchProvider() const {
On 2012/12/17 21:01:00, Peter Kasting wrote:
> Inline functions must be named unix_hacker()-style.

Done.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.h:49:
DISALLOW_IMPLICIT_CONSTRUCTORS(SearchTokenView);
On 2012/12/17 21:01:00, Peter Kasting wrote:
> Nit: This should be DISALLOW_COPY_AND_ASSIGN.

Done.

[Peter Kasting, 2012-12-20 01:45:39]

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:931:
search_token_view_->SetVisible(pref_width < AvailableWidth(max_edit_width));
On 2012/12/20 00:26:14, kuan wrote:
> On 2012/12/17 21:01:00, Peter Kasting wrote:
> > Use LayoutView() for calculating visibility and setting bounds, as we do
> above.
> 
> i initially tried that, and discovered that LayoutView() only sets view
> invisible if view's preferred/min width is less than location_bounds.width,
> whereas i need to set view invisible if view's width is less than
> AvailableWidth(max_edit_width).

I suggest modifying LayoutView() to do the right thing.  I would probably make
it take one more argument that, when the view doesn't fit in the available
width, toggles between "resize to min width" and "hide altogether".

Another possibility is that instead of modifying LayoutView() you could maybe
make the view's GetMinimumSize() function return zero width.  In this scenario,
though, LayoutView() would still reserve the requested amount of padding, which
might goof things up.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/search_token_view.cc (right):

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.cc:18: const int
kRightPadding = 8;
On 2012/12/20 00:26:14, kuan wrote:
> On 2012/12/17 21:01:00, Peter Kasting wrote:
> > Don't do this for two reasons.
> > 
> > One is that these values shouldn't be custom-hardcoded.  You should be using
> the
> > same values that every other item in the omnibox is using for padding.
> > 
> > The other is that this view shouldn't be managing internal paddings anyway. 
> The
> > location bar should be laying it out where it needs to be.
> > 
> > Basically, your class should act exactly like the KeywordHintView both in
> terms
> > of who adjusts paddings where as well as the actual padding values and
precise
> > point at which the view gets removed to save space.
> 
> i've moved the paddings to location_bar_view.cc, but i still need them
> custom-hardcoded there, 'cos the search token uses different paddings from the
> other decorations.

We shouldn't use different paddings than are available in the combo of (what
everything else uses) and (values in layout_constants.h).  We should deviate
from the mocks if necessary to make this true.

Perhaps use kUnrelatedControlLargeHorizontalSpacing for the padding between edit
and label.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.cc:30:
ToolbarModel::NONE, LocationBarView::TEXT), 64);  // 25% alpha.
On 2012/12/20 00:26:14, kuan wrote:
> On 2012/12/17 21:01:00, Peter Kasting wrote:
> > Turning off auto-color and manually alpha-blending the text and background
> isn't
> > right.  Why not use the label color here?  And why disable auto-color in the
> > first place?  These will prevent weird artifacts from potentially occurring.
> 
> i removed the disabling of auto-color.  the reason why cole wants a 25% alpha
is
> because it appears too dark, compared to the text.

Well, it shouldn't be as dark as the text, it should be as dark as the
de-emphasized text, which is already 50% blended IIRC.  (That will also save you
from weirdness if for some reason the two are actually totally different
colors.)  If we still need it more blended we could then blend that, but I'd
like to see screenshots.

Also, now that I think about it, I'm pretty sure you actually should leave
auto-coloring disabled here, because the edit control where we render
DEEMPHASIZED_TEXT otherwise isn't doing auto-coloring, so this could only make
the text stick out weirdly.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.cc:64: void
SearchTokenView::OnPaintBorder(gfx::Canvas* canvas) {
On 2012/12/20 00:26:14, kuan wrote:
> On 2012/12/17 21:01:00, Peter Kasting wrote:
> > Instead of this, why don't you simply use a real border? 
> > CreateSolidSidedBorder(0, 0, 0, 1, <color>) should do what you want.
> 
> i tried that, but it paints a 2-px thick right border in retina display while
> cole only wants 1-px in both 1x and 2x displays; SkPaint::setStrokeWidth(0)
> accomplishes the 1-px thick border in both 1x and 2x displays for me.

You should add comments to this effect, since future readers will perhaps wonder
the same thing I did.

[beaudoin, 2012-12-20 03:05:01]

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:931:
search_token_view_->SetVisible(pref_width < AvailableWidth(max_edit_width));
On 2012/12/20 01:45:39, Peter Kasting wrote:
> On 2012/12/20 00:26:14, kuan wrote:
> > On 2012/12/17 21:01:00, Peter Kasting wrote:
> > > Use LayoutView() for calculating visibility and setting bounds, as we do
> > above.
> > 
> > i initially tried that, and discovered that LayoutView() only sets view
> > invisible if view's preferred/min width is less than location_bounds.width,
> > whereas i need to set view invisible if view's width is less than
> > AvailableWidth(max_edit_width).
> 
> I suggest modifying LayoutView() to do the right thing.  I would probably make
> it take one more argument that, when the view doesn't fit in the available
> width, toggles between "resize to min width" and "hide altogether".
> 
> Another possibility is that instead of modifying LayoutView() you could maybe
> make the view's GetMinimumSize() function return zero width.  In this
scenario,
> though, LayoutView() would still reserve the requested amount of padding,
which
> might goof things up.

Before investing too much time in this, check out my CL that I'm readying for
review over here:
https://codereview.chromium.org/11648024/

LayoutView is gone, but I think it makes adding new decorations (or shifting
them around) much easier. It may not be a bad idea to wait for this to land.

[kuan, 2012-12-20 16:51:22]

i've addressed peter's comments in patch set 14, i'll follow up w/ screenshots. 
ptal.  thx.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:931:
search_token_view_->SetVisible(pref_width < AvailableWidth(max_edit_width));
On 2012/12/20 01:45:39, Peter Kasting wrote:
> On 2012/12/20 00:26:14, kuan wrote:
> > On 2012/12/17 21:01:00, Peter Kasting wrote:
> > > Use LayoutView() for calculating visibility and setting bounds, as we do
> > above.
> > 
> > i initially tried that, and discovered that LayoutView() only sets view
> > invisible if view's preferred/min width is less than location_bounds.width,
> > whereas i need to set view invisible if view's width is less than
> > AvailableWidth(max_edit_width).
> 
> I suggest modifying LayoutView() to do the right thing.  I would probably make
> it take one more argument that, when the view doesn't fit in the available
> width, toggles between "resize to min width" and "hide altogether".
> 
> Another possibility is that instead of modifying LayoutView() you could maybe
> make the view's GetMinimumSize() function return zero width.  In this
scenario,
> though, LayoutView() would still reserve the requested amount of padding,
which
> might goof things up.

should i wait for philippe's refactoring cl to land first?

in any case, LayoutView() or philippe's new design expects only 1 padding for
the view, either on the left or the right - SearchTokenView has both, one on
left and one on right.  i can't help but feel that the right padding between end
of label and separator should be handled by SearchTokenView itself?  of course,
we can pass the padding size into its constructor so theoretically, it's still
determined in LocationBarView.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/search_token_view.cc (right):

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.cc:18: const int
kRightPadding = 8;
On 2012/12/20 01:45:39, Peter Kasting wrote:
> On 2012/12/20 00:26:14, kuan wrote:
> > On 2012/12/17 21:01:00, Peter Kasting wrote:
> > > Don't do this for two reasons.
> > > 
> > > One is that these values shouldn't be custom-hardcoded.  You should be
using
> > the
> > > same values that every other item in the omnibox is using for padding.
> > > 
> > > The other is that this view shouldn't be managing internal paddings
anyway. 
> > The
> > > location bar should be laying it out where it needs to be.
> > > 
> > > Basically, your class should act exactly like the KeywordHintView both in
> > terms
> > > of who adjusts paddings where as well as the actual padding values and
> precise
> > > point at which the view gets removed to save space.
> > 
> > i've moved the paddings to location_bar_view.cc, but i still need them
> > custom-hardcoded there, 'cos the search token uses different paddings from
the
> > other decorations.
> 
> We shouldn't use different paddings than are available in the combo of (what
> everything else uses) and (values in layout_constants.h).  We should deviate
> from the mocks if necessary to make this true.
> 
> Perhaps use kUnrelatedControlLargeHorizontalSpacing for the padding between
edit
> and label.

i dug thru the iteration emails w/ cole and remembered that when cole decided to
use 25 for left padding, he said to use the location height.  so, i've changed
to use the location height, which is actually 23, as the left padding.
as for right padding, the other decorations are using GetItemPadding() which is
3 for desktop, which may be too cramped for SearchTokenView.  i see that
kTouchItemPadding is 8, which is same as the mocks.  can i use kTouchItemPadding
for the right padding?
i'll attach screenshots.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.cc:30:
ToolbarModel::NONE, LocationBarView::TEXT), 64);  // 25% alpha.
On 2012/12/20 01:45:39, Peter Kasting wrote:
> On 2012/12/20 00:26:14, kuan wrote:
> > On 2012/12/17 21:01:00, Peter Kasting wrote:
> > > Turning off auto-color and manually alpha-blending the text and background
> > isn't
> > > right.  Why not use the label color here?  And why disable auto-color in
the
> > > first place?  These will prevent weird artifacts from potentially
occurring.
> > 
> > i removed the disabling of auto-color.  the reason why cole wants a 25%
alpha
> is
> > because it appears too dark, compared to the text.
> 
> Well, it shouldn't be as dark as the text, it should be as dark as the
> de-emphasized text, which is already 50% blended IIRC.  (That will also save
you
> from weirdness if for some reason the two are actually totally different
> colors.)  If we still need it more blended we could then blend that, but I'd
> like to see screenshots.
> 
> Also, now that I think about it, I'm pretty sure you actually should leave
> auto-coloring disabled here, because the edit control where we render
> DEEMPHASIZED_TEXT otherwise isn't doing auto-coloring, so this could only make
> the text stick out weirdly.

i've disabled back auto-coloring.  i meant the border was darker than the
de-emphasized text.  i'll attach screenshots.

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.cc:64: void
SearchTokenView::OnPaintBorder(gfx::Canvas* canvas) {
On 2012/12/20 01:45:39, Peter Kasting wrote:
> On 2012/12/20 00:26:14, kuan wrote:
> > On 2012/12/17 21:01:00, Peter Kasting wrote:
> > > Instead of this, why don't you simply use a real border? 
> > > CreateSolidSidedBorder(0, 0, 0, 1, <color>) should do what you want.
> > 
> > i tried that, but it paints a 2-px thick right border in retina display
while
> > cole only wants 1-px in both 1x and 2x displays; SkPaint::setStrokeWidth(0)
> > accomplishes the 1-px thick border in both 1x and 2x displays for me.
> 
> You should add comments to this effect, since future readers will perhaps
wonder
> the same thing I did.

Done.

[Peter Kasting, 2012-12-20 19:15:07]

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/search_token_view.cc (right):

https://codereview.chromium.org/11418229/diff/33002/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.cc:18: const int
kRightPadding = 8;
On 2012/12/20 16:51:22, kuan wrote:
> i dug thru the iteration emails w/ cole and remembered that when cole decided
to
> use 25 for left padding, he said to use the location height.  so, i've changed
> to use the location height, which is actually 23, as the left padding.

Don't do this.  The location height should have no relation to this value.  If
we were to radically change the location height (e.g. triple it) we wouldn't
want to triple this padding value.

> as for right padding, the other decorations are using GetItemPadding() which
is
> 3 for desktop, which may be too cramped for SearchTokenView.  i see that
> kTouchItemPadding is 8, which is same as the mocks.  can i use
kTouchItemPadding
> for the right padding?

No, you can't.  This isn't a touch item, so you can't use an unrelated constant
that happens to have the value you want.  Otherwise changing the touch padding
will inexplicably start affecting random other things.

The paddings between text, separator, and items to the left should probably be
the "item padding" value in the location bar, and I already suggested a layout
constant for the other padding value.

[kuan, 2013-01-04 17:56:03]

i've re-implemented based on philippe's new layout design in patch set 15. 
ptal.  thx.

[Peter Kasting, 2013-01-05 00:36:18]

Since Philippe has been staring at location bar layout, I'll let him take the
first pass at this, so he can say whether the method is the best.

[beaudoin, 2013-01-07 17:27:52]

One question for pkasting in there.

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_layout.h (right):

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.h:76: // Sets the
padding between edit and the decoration beside it.
Add to comment: 
This value must not be modified after LayoutPass1 has been called.

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:705: const bool
show_keyword_hint = !keyword.empty() && is_keyword_hint;
It probably does not make sense to show the keyword or keyword hint when
search_token_view_ is visible but I don't see any logic preventing that from
happening.

You should probably add logic here so that show_selected_keyword and
show_keyword_hint are always false when
search_token_view_->has_search_provider() is true.

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:791: if
(search_token_view_->has_search_provider()) {
Add a comment to indicate this must absolutely be the last item of the
right_decorations list, otherwise set_item_edit_padding() makes no sense.

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/search_token_view.cc (right):

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.cc:48:
size.Enlarge(LocationBarView::GetItemPadding(), 0);
Comment that the enlarge is not for the padding between this decoration and the
next one, but for padding between the decoration and the separator.

I'm not really comfortable with the decoration depending on a constant from the
layout system. I feel the problem here is that the separator should not be
managed by the decoration. After all, we probably don't want the separator if
the SearchTokenView is directly next to the omnibox edge or the action box. What
if the separator was another decoration that you added prior to the
SearchTokenView only if it's not the right-most decoration?

@pkasting What do you think of that proposal?

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/search_token_view.h (right):

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.h:36: void
SetSearchProvider(const string16& search_provider);
Comment that setting an empty string will remove the decoration.

[kuan, 2013-01-07 17:43:38]

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/search_token_view.cc (right):

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.cc:48:
size.Enlarge(LocationBarView::GetItemPadding(), 0);
On 2013/01/07 17:27:53, beaudoin wrote:
> Comment that the enlarge is not for the padding between this decoration and
the
> next one, but for padding between the decoration and the separator.
> 
> I'm not really comfortable with the decoration depending on a constant from
the
> layout system. I feel the problem here is that the separator should not be
> managed by the decoration. After all, we probably don't want the separator if
> the SearchTokenView is directly next to the omnibox edge or the action box.
What
> if the separator was another decoration that you added prior to the
> SearchTokenView only if it's not the right-most decoration?
> 
> @pkasting What do you think of that proposal?

actually, cole wants the padding between "Google Search" and the separator to be
8px; he thinks the item padding of 3px is way too cramped.  but peter wanted me
to use the usual item padding, hence the GetItemPadding() here.  if i shld use
the 8px padding, shld i still impl the separator as an individual decoration?

[beaudoin, 2013-01-07 18:42:30]

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/search_token_view.cc (right):

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.cc:48:
size.Enlarge(LocationBarView::GetItemPadding(), 0);
On 2013/01/07 17:43:38, kuan wrote:
> On 2013/01/07 17:27:53, beaudoin wrote:
> > Comment that the enlarge is not for the padding between this decoration and
> the
> > next one, but for padding between the decoration and the separator.
> > 
> > I'm not really comfortable with the decoration depending on a constant from
> the
> > layout system. I feel the problem here is that the separator should not be
> > managed by the decoration. After all, we probably don't want the separator
if
> > the SearchTokenView is directly next to the omnibox edge or the action box.
> What
> > if the separator was another decoration that you added prior to the
> > SearchTokenView only if it's not the right-most decoration?
> > 
> > @pkasting What do you think of that proposal?
> 
> actually, cole wants the padding between "Google Search" and the separator to
be
> 8px; he thinks the item padding of 3px is way too cramped.  but peter wanted
me
> to use the usual item padding, hence the GetItemPadding() here.  if i shld use
> the 8px padding, shld i still impl the separator as an individual decoration?

I'd say the question is more:
- Is this meant to separate this decoration from the next one or;
- Is this a part of a SearchTokenView.
From the code I would guess (1) since I don't believe you want to draw it if
there is nothing to the right of the SearchTokenView. In this case, no matter
the padding, I'd say go with a separator-as-a-separate-decoration and use an 8px
padding in your call to AddDecoration(..., search_token_view_).

[Peter Kasting, 2013-01-07 19:27:46]

I think the idea of having the separator get added separately is fine.

[beaudoin, 2013-01-07 22:18:40]

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:794: GetItemPadding(),
0, search_token_view_);
"true" means this component is auto-collapsible. Are you sure this is what you
want? If so, you'll probably have to add some logic if you extract the separator
as a separate decoration.

[kuan, 2013-01-09 19:52:58]

patch set 16 includes:
- re-implementing the search token as 2 view
- separator handling in layout system.
ptal.  thx.

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_layout.h (right):

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.h:76: // Sets the
padding between edit and the decoration beside it.
On 2013/01/07 17:27:53, beaudoin wrote:
> Add to comment: 
> This value must not be modified after LayoutPass1 has been called.

Done.

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:705: const bool
show_keyword_hint = !keyword.empty() && is_keyword_hint;
On 2013/01/07 17:27:53, beaudoin wrote:
> It probably does not make sense to show the keyword or keyword hint when
> search_token_view_ is visible but I don't see any logic preventing that from
> happening.
> 
> You should probably add logic here so that show_selected_keyword and
> show_keyword_hint are always false when
> search_token_view_->has_search_provider() is true.

Done.

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:791: if
(search_token_view_->has_search_provider()) {
On 2013/01/07 17:27:53, beaudoin wrote:
> Add a comment to indicate this must absolutely be the last item of the
> right_decorations list, otherwise set_item_edit_padding() makes no sense.

Done.

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:794: GetItemPadding(),
0, search_token_view_);
On 2013/01/07 22:18:40, beaudoin wrote:
> "true" means this component is auto-collapsible. Are you sure this is what you
> want? If so, you'll probably have to add some logic if you extract the
separator
> as a separate decoration.

Done.  yes, it is auto-collapsible.

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/search_token_view.cc (right):

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.cc:48:
size.Enlarge(LocationBarView::GetItemPadding(), 0);
On 2013/01/07 18:42:30, beaudoin wrote:
> On 2013/01/07 17:43:38, kuan wrote:
> > On 2013/01/07 17:27:53, beaudoin wrote:
> > > Comment that the enlarge is not for the padding between this decoration
and
> > the
> > > next one, but for padding between the decoration and the separator.
> > > 
> > > I'm not really comfortable with the decoration depending on a constant
from
> > the
> > > layout system. I feel the problem here is that the separator should not be
> > > managed by the decoration. After all, we probably don't want the separator
> if
> > > the SearchTokenView is directly next to the omnibox edge or the action
box.
> > What
> > > if the separator was another decoration that you added prior to the
> > > SearchTokenView only if it's not the right-most decoration?
> > > 
> > > @pkasting What do you think of that proposal?
> > 
> > actually, cole wants the padding between "Google Search" and the separator
to
> be
> > 8px; he thinks the item padding of 3px is way too cramped.  but peter wanted
> me
> > to use the usual item padding, hence the GetItemPadding() here.  if i shld
use
> > the 8px padding, shld i still impl the separator as an individual
decoration?
> 
> I'd say the question is more:
> - Is this meant to separate this decoration from the next one or;
> - Is this a part of a SearchTokenView.
> From the code I would guess (1) since I don't believe you want to draw it if
> there is nothing to the right of the SearchTokenView. In this case, no matter
> the padding, I'd say go with a separator-as-a-separate-decoration and use an
8px
> padding in your call to AddDecoration(..., search_token_view_).

Done.

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/search_token_view.h (right):

https://codereview.chromium.org/11418229/diff/54001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/search_token_view.h:36: void
SetSearchProvider(const string16& search_provider);
On 2013/01/07 17:27:53, beaudoin wrote:
> Comment that setting an empty string will remove the decoration.

Done.

[kuan, 2013-01-09 19:55:07]

fyi, the assets are new ones for action box, with the separator removed.  i
added them to this cl to verify my separator handling logic in layout system. 
if the assets get landed by someone from the action box team b4 i land my cl,
i'll remove them from this cl.

[beaudoin, 2013-01-09 20:36:52]

Proposing a slightly different separator design that should lead to much simpler
code.

https://codereview.chromium.org/11418229/diff/66001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_layout.cc (right):

https://codereview.chromium.org/11418229/diff/66001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:87: views::View*
separator_view)
If you change the API according to my proposal, the only thing you'll need to
add to LocationBarDecoration is a something to indicate it is a separator. (I
suggest a type enum with AUTO_COLLAPSE and SEPARATOR, so you can get rid of the
auto_collapse bool.)

https://codereview.chromium.org/11418229/diff/66001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:176: entry_width);
I would not bother to determine separator visibility before Pass3. Just assume
they are visible all the time.

https://codereview.chromium.org/11418229/diff/66001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:240: }
I would layout separators normally here, however, before you exist Pass3, I
would do another pass to remove all leading, trailing and duplicate separators
(skipping over non-visible decorations). This will require adjusting the layout
of the remaining decoration a little, but it shouldn't be too hard to do and you
can probably get rid of DetermineSeparatorVisibility and SetSeparatorBounds in
the process.

https://codereview.chromium.org/11418229/diff/66001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:280: if
(((*it)->auto_collapse && !(*it)->view->visible()) ||
Checking (*it)->auto_collapse is not needed.

https://codereview.chromium.org/11418229/diff/66001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_layout.h (right):

https://codereview.chromium.org/11418229/diff/66001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.h:29: enum Separator {
Should probably be SeparatorPosition

https://codereview.chromium.org/11418229/diff/66001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.h:32: // decoration for
RIGHT_EDGE.
I would rephrase the comment:
SEPARATOR_BEFORE places the separator to the left of LEFT_EDGE decorations and
to the right of RIGHT_EDGE decorations.

https://codereview.chromium.org/11418229/diff/66001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.h:35: // decoration for
RIGHT_EDGE.
Same here.

https://codereview.chromium.org/11418229/diff/66001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.h:84: views::View*
separator_view);
I think it would be much simpler to replace the above with:
void AddSeparator(int y, int height, int padding, views::View* separator_view);

This will allow you to drop the Separator enum entirely. To add the separator
before the decoration you would just call:
AddSeparator(...);
AddDecoration(...);

I don't think the separator has to be tied to a specific decoration: since your
SearchTokenView is always the left-most decoration, you just have to check
whether there is something visible to both left and right of the separator.

https://codereview.chromium.org/11418229/diff/66001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.h:123: // Set bounds of
separator is if it's visible, returns true if bounds is set.
typo: if it is

https://codereview.chromium.org/11418229/diff/66001/chrome/chrome_browser_ui....
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/11418229/diff/66001/chrome/chrome_browser_ui....
chrome/chrome_browser_ui.gypi:1: # Copyright 2013 The Chromium Authors. All
rights reserved.
I think the new policy said we should not change the copyright year when we
touch a file.

[kuan, 2013-01-11 21:21:20]

in patch set 17, i've:
- re-implemented layout system using philippe's proposal
- addressed remaining comments that still apply
- removed SearchTokenView class since it's now a simple label.
ptal.  thx.

https://codereview.chromium.org/11418229/diff/66001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_layout.cc (right):

https://codereview.chromium.org/11418229/diff/66001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:87: views::View*
separator_view)
On 2013/01/09 20:36:52, beaudoin wrote:
> If you change the API according to my proposal, the only thing you'll need to
> add to LocationBarDecoration is a something to indicate it is a separator. (I
> suggest a type enum with AUTO_COLLAPSE and SEPARATOR, so you can get rid of
the
> auto_collapse bool.)

Done.

https://codereview.chromium.org/11418229/diff/66001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:176: entry_width);
On 2013/01/09 20:36:52, beaudoin wrote:
> I would not bother to determine separator visibility before Pass3. Just assume
> they are visible all the time.

Done.

https://codereview.chromium.org/11418229/diff/66001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:240: }
On 2013/01/09 20:36:52, beaudoin wrote:
> I would layout separators normally here, however, before you exist Pass3, I
> would do another pass to remove all leading, trailing and duplicate separators
> (skipping over non-visible decorations). This will require adjusting the
layout
> of the remaining decoration a little, but it shouldn't be too hard to do and
you
> can probably get rid of DetermineSeparatorVisibility and SetSeparatorBounds in
> the process.

Done.

https://codereview.chromium.org/11418229/diff/66001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:280: if
(((*it)->auto_collapse && !(*it)->view->visible()) ||
On 2013/01/09 20:36:52, beaudoin wrote:
> Checking (*it)->auto_collapse is not needed.

removed, based on new design.

https://codereview.chromium.org/11418229/diff/66001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_layout.h (right):

https://codereview.chromium.org/11418229/diff/66001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.h:29: enum Separator {
On 2013/01/09 20:36:52, beaudoin wrote:
> Should probably be SeparatorPosition

removed based on new design.

https://codereview.chromium.org/11418229/diff/66001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.h:32: // decoration for
RIGHT_EDGE.
On 2013/01/09 20:36:52, beaudoin wrote:
> I would rephrase the comment:
> SEPARATOR_BEFORE places the separator to the left of LEFT_EDGE decorations and
> to the right of RIGHT_EDGE decorations. 

ditto.

https://codereview.chromium.org/11418229/diff/66001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.h:35: // decoration for
RIGHT_EDGE.
On 2013/01/09 20:36:52, beaudoin wrote:
> Same here.

ditto.

https://codereview.chromium.org/11418229/diff/66001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.h:84: views::View*
separator_view);
On 2013/01/09 20:36:52, beaudoin wrote:
> I think it would be much simpler to replace the above with:
> void AddSeparator(int y, int height, int padding, views::View*
separator_view);
> 
> This will allow you to drop the Separator enum entirely. To add the separator
> before the decoration you would just call:
> AddSeparator(...);
> AddDecoration(...);
> 
> I don't think the separator has to be tied to a specific decoration: since
your
> SearchTokenView is always the left-most decoration, you just have to check
> whether there is something visible to both left and right of the separator.

Done.

https://codereview.chromium.org/11418229/diff/66001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.h:123: // Set bounds of
separator is if it's visible, returns true if bounds is set.
On 2013/01/09 20:36:52, beaudoin wrote:
> typo: if it is

removed, based on new design.

https://codereview.chromium.org/11418229/diff/66001/chrome/chrome_browser_ui....
File chrome/chrome_browser_ui.gypi (right):

https://codereview.chromium.org/11418229/diff/66001/chrome/chrome_browser_ui....
chrome/chrome_browser_ui.gypi:1: # Copyright 2013 The Chromium Authors. All
rights reserved.
On 2013/01/09 20:36:52, beaudoin wrote:
> I think the new policy said we should not change the copyright year when we
> touch a file.

Done.

[beaudoin, 2013-01-12 05:47:15]

Just a couple of nits and minor changes to the algo.

https://codereview.chromium.org/11418229/diff/78001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_layout.cc (right):

https://codereview.chromium.org/11418229/diff/78001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:88: DCHECK(type !=
AUTO_COLLAPSE || max_fraction == 0.0);
DCHECK(type == NORMAL || max_fraction == 0.0);

https://codereview.chromium.org/11418229/diff/78001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:135: padding,
padding, 0, view));
edge_item_padding can be 0 because edge separators will necessarily be removed.
It's not a big deal but it will lead to slightly more accurate evaluation of
entry_width in Pass1.

https://codereview.chromium.org/11418229/diff/78001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:207: void
LocationBarLayout::SetSeparatorsInvisible(int* available_width) {
Maybe rename to: HideUnneededSeparators

https://codereview.chromium.org/11418229/diff/78001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:217: // Previous
item is a visible non-spearator decoration.
typo: separator

https://codereview.chromium.org/11418229/diff/78001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:218: ((*prev)->type
!= SEPARATOR && (*prev)->view->visible());
Not sure the logic is correct here. If the list is:
  Decorator (visible), Decorator (not-visible), Separator, Decorator (visible)
Then visible = false, but you would like it to be visible.

Instead of "prev" consider initialising outside the loop:
bool last_visible_was_a_separator = false;

Then, inside the loop:
if ((*it)->type == SEPARATOR) {
  if (last_visible_was_a_separator) {
    // Hide this separator.
  } else {
    // Show this separator.
  }
} else if ((*it)->view->visible()) {
  last_visible_was_a_separator = false;
}

https://codereview.chromium.org/11418229/diff/78001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:235:
last_visible_separator > last_visible_decoration) {
I think it would be simpler to keep just one extra iterator:
  trailing_separator
In the loop, set
  trailing_separator = it;
when you have a visible separator and set
  trailing_separator = decorations_.end()
when you have a visible decoration.

Then you just have to check if trailing_separator != decorations_.end()

https://codereview.chromium.org/11418229/diff/78001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:249: continue;
indentation (2 lines above)

[kuan, 2013-01-14 16:47:47]

i've addressed philippe's comments in patch set 18.  ptal.  thx.

https://codereview.chromium.org/11418229/diff/78001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_layout.cc (right):

https://codereview.chromium.org/11418229/diff/78001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:88: DCHECK(type !=
AUTO_COLLAPSE || max_fraction == 0.0);
On 2013/01/12 05:47:15, beaudoin wrote:
> DCHECK(type == NORMAL || max_fraction == 0.0);

Done.

https://codereview.chromium.org/11418229/diff/78001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:135: padding,
padding, 0, view));
On 2013/01/12 05:47:15, beaudoin wrote:
> edge_item_padding can be 0 because edge separators will necessarily be
removed.
> It's not a big deal but it will lead to slightly more accurate evaluation of
> entry_width in Pass1.

Done.

https://codereview.chromium.org/11418229/diff/78001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:207: void
LocationBarLayout::SetSeparatorsInvisible(int* available_width) {
On 2013/01/12 05:47:15, beaudoin wrote:
> Maybe rename to: HideUnneededSeparators

Done.

https://codereview.chromium.org/11418229/diff/78001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:217: // Previous
item is a visible non-spearator decoration.
On 2013/01/12 05:47:15, beaudoin wrote:
> typo: separator

Done.

https://codereview.chromium.org/11418229/diff/78001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:218: ((*prev)->type
!= SEPARATOR && (*prev)->view->visible());
On 2013/01/12 05:47:15, beaudoin wrote:
> Not sure the logic is correct here. If the list is:
>   Decorator (visible), Decorator (not-visible), Separator, Decorator (visible)
> Then visible = false, but you would like it to be visible.
> 
> Instead of "prev" consider initialising outside the loop:
> bool last_visible_was_a_separator = false;
> 
> Then, inside the loop:
> if ((*it)->type == SEPARATOR) {
>   if (last_visible_was_a_separator) {
>     // Hide this separator.
>   } else {
>     // Show this separator.
>   }
> } else if ((*it)->view->visible()) {
>   last_visible_was_a_separator = false;
> }

Done.  i added one more bool at_least_one_non_separator_visible to track leading
separator.

https://codereview.chromium.org/11418229/diff/78001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:235:
last_visible_separator > last_visible_decoration) {
On 2013/01/12 05:47:15, beaudoin wrote:
> I think it would be simpler to keep just one extra iterator:
>   trailing_separator
> In the loop, set
>   trailing_separator = it;
> when you have a visible separator and set
>   trailing_separator = decorations_.end()
> when you have a visible decoration.
> 
> Then you just have to check if trailing_separator != decorations_.end()

Done.

https://codereview.chromium.org/11418229/diff/78001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:249: continue;
On 2013/01/12 05:47:15, beaudoin wrote:
> indentation (2 lines above)

Done.

[beaudoin, 2013-01-14 18:12:14]

LGTM but I think you can remove at_least_one_non_separator_visible before
committing.

Good work on that one. It's hard to keep clean code in the face of tricky
requirements like this. :)

https://codereview.chromium.org/11418229/diff/78001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_layout.cc (right):

https://codereview.chromium.org/11418229/diff/78001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:218: ((*prev)->type
!= SEPARATOR && (*prev)->view->visible());
On 2013/01/14 16:47:47, kuan wrote:
> On 2013/01/12 05:47:15, beaudoin wrote:
> > Not sure the logic is correct here. If the list is:
> >   Decorator (visible), Decorator (not-visible), Separator, Decorator
(visible)
> > Then visible = false, but you would like it to be visible.
> > 
> > Instead of "prev" consider initialising outside the loop:
> > bool last_visible_was_a_separator = false;
> > 
> > Then, inside the loop:
> > if ((*it)->type == SEPARATOR) {
> >   if (last_visible_was_a_separator) {
> >     // Hide this separator.
> >   } else {
> >     // Show this separator.
> >   }
> > } else if ((*it)->view->visible()) {
> >   last_visible_was_a_separator = false;
> > }
> 
> Done.  i added one more bool at_least_one_non_separator_visible to track
leading
> separator.

I though last_visible_was_a_separator would be enough? Start it at "true" and
the leading separators should be hidden.

[kuan, 2013-01-14 18:33:11]

i've addressed philippe's last comment in patch set 19.  ptal.  thx.

https://codereview.chromium.org/11418229/diff/78001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_layout.cc (right):

https://codereview.chromium.org/11418229/diff/78001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:218: ((*prev)->type
!= SEPARATOR && (*prev)->view->visible());
On 2013/01/14 18:12:14, beaudoin wrote:
> On 2013/01/14 16:47:47, kuan wrote:
> > On 2013/01/12 05:47:15, beaudoin wrote:
> > > Not sure the logic is correct here. If the list is:
> > >   Decorator (visible), Decorator (not-visible), Separator, Decorator
> (visible)
> > > Then visible = false, but you would like it to be visible.
> > > 
> > > Instead of "prev" consider initialising outside the loop:
> > > bool last_visible_was_a_separator = false;
> > > 
> > > Then, inside the loop:
> > > if ((*it)->type == SEPARATOR) {
> > >   if (last_visible_was_a_separator) {
> > >     // Hide this separator.
> > >   } else {
> > >     // Show this separator.
> > >   }
> > > } else if ((*it)->view->visible()) {
> > >   last_visible_was_a_separator = false;
> > > }
> > 
> > Done.  i added one more bool at_least_one_non_separator_visible to track
> leading
> > separator.
> 
> I though last_visible_was_a_separator would be enough? Start it at "true" and
> the leading separators should be hidden.

Done.  i thot of that, but worried that it might be misleading to init that to
true; i added comment as to why it's init to true.

[beaudoin, 2013-01-14 19:20:31]

LGTM

https://codereview.chromium.org/11418229/diff/80004/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_layout.cc (right):

https://codereview.chromium.org/11418229/diff/80004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:209: // separator
will be hidden.
Nit: separators

[kuan, 2013-01-14 19:30:32]

peter, cld u take a look now that philippe has lgtm'ed?  thx.

https://codereview.chromium.org/11418229/diff/80004/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_layout.cc (right):

https://codereview.chromium.org/11418229/diff/80004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:209: // separator
will be hidden.
On 2013/01/14 19:20:31, beaudoin wrote:
> Nit: separators

Done.

[Peter Kasting, 2013-01-17 04:14:22]

Basically fine, most of these are small.

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_layout.cc (right):

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:89:
DCHECK(max_fraction >= 0.0);
Nit: It's longer, but this seems less redundant to me:

if (type == NORMAL)
  DCHECK_GE(max_fraction, 0);
else
  DCHECK_EQ(0, max_fraction);

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:122:
decorations_.push_back(new LocationBarDecoration(NORMAL,
Nit: This should go on the next line so all the lines of args have the same
indenting.

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:135: 0, padding, 0,
view));
Nit: Indent even

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:151: if ((*it)->type
!= AUTO_COLLAPSE && (*it)->max_fraction == 0.0) {
Nit: Personally, I prefer parens around binary sub-expressions.

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:208: // Initialize
|last_visible_was_a_separator| to true so that the leading
Nit: the -> any

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:210: bool
last_visible_was_a_separator = true;
Nit: Seems like you don't need this.  You can just check if |trailing_separator|
is pointing at anything.

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:211:
ScopedVector<LocationBarDecoration>::iterator trailing_separator =
Nit: This type name is long.  I suggest a private:

typedef ScopedVector<LocationBarDecoration> Decorations;

...in the header, then use it in this file.

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:244: if
(!(*it)->view->visible())
Nit: It probably makes sense to use a temp for either or both of (*it) and
(*it)->view.

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:249: int x;
Nit: I'd use ?: here so that the variable isn't declared without being
initialized.  (Use parens around the subexpressions.)

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:261:
(*it)->builtin_padding);
Nit: We don't indent even in the midst of a single subexpression.  I suggest
putting the whole subexpression on this second line if it will fit; otherwise
indent 4.

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_layout.h (right):

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.h:60: // - The |height|
in pixel;
Nit: pixels.  Though, it's weird to note that only here when all these first
three arguments are in pixels.

Frankly, I'd rename |padding| to |padding_from_previous_item| and |view| to
|separator|, and drop the comment entirely as at that point the function is
self-documenting.

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_separator_view.cc
(right):

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_separator_view.cc:11: :
separator_color_(SK_ColorBLACK) {
Nit: Indent 4

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_separator_view.h (right):

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_separator_view.h:25: //
views::View overrides.
Nit: I've been trying to standardize on "views::View:".

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_separator_view.h:28:
protected:
Why protected?  Why not private (for both overrides)?

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:246:
ev_bubble_view_->SetVisible(false);
Nit: Are all these SetVisible(false) calls even necessary, given the beginning
of Layout()?

[kuan, 2013-01-17 17:57:43]

i've addressed peter's comments in patch set 21.  ptal.  thx.

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_layout.cc (right):

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:89:
DCHECK(max_fraction >= 0.0);
On 2013/01/17 04:14:22, Peter Kasting wrote:
> Nit: It's longer, but this seems less redundant to me:
> 
> if (type == NORMAL)
>   DCHECK_GE(max_fraction, 0);
> else
>   DCHECK_EQ(0, max_fraction);

Done.

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:122:
decorations_.push_back(new LocationBarDecoration(NORMAL,
On 2013/01/17 04:14:22, Peter Kasting wrote:
> Nit: This should go on the next line so all the lines of args have the same
> indenting.

Done.

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:135: 0, padding, 0,
view));
On 2013/01/17 04:14:22, Peter Kasting wrote:
> Nit: Indent even

Done.

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:151: if ((*it)->type
!= AUTO_COLLAPSE && (*it)->max_fraction == 0.0) {
On 2013/01/17 04:14:22, Peter Kasting wrote:
> Nit: Personally, I prefer parens around binary sub-expressions.

Done.

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:208: // Initialize
|last_visible_was_a_separator| to true so that the leading
On 2013/01/17 04:14:22, Peter Kasting wrote:
> Nit: the -> any

Done.

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:210: bool
last_visible_was_a_separator = true;
On 2013/01/17 04:14:22, Peter Kasting wrote:
> Nit: Seems like you don't need this.  You can just check if
|trailing_separator|
> is pointing at anything.

Done.

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:211:
ScopedVector<LocationBarDecoration>::iterator trailing_separator =
On 2013/01/17 04:14:22, Peter Kasting wrote:
> Nit: This type name is long.  I suggest a private:
> 
> typedef ScopedVector<LocationBarDecoration> Decorations;
> 
> ...in the header, then use it in this file.

Done.

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:244: if
(!(*it)->view->visible())
On 2013/01/17 04:14:22, Peter Kasting wrote:
> Nit: It probably makes sense to use a temp for either or both of (*it) and
> (*it)->view.

Done.

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:249: int x;
On 2013/01/17 04:14:22, Peter Kasting wrote:
> Nit: I'd use ?: here so that the variable isn't declared without being
> initialized.  (Use parens around the subexpressions.)

Done.

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:261:
(*it)->builtin_padding);
On 2013/01/17 04:14:22, Peter Kasting wrote:
> Nit: We don't indent even in the midst of a single subexpression.  I suggest
> putting the whole subexpression on this second line if it will fit; otherwise
> indent 4.

Done.

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_layout.h (right):

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.h:60: // - The |height|
in pixel;
On 2013/01/17 04:14:22, Peter Kasting wrote:
> Nit: pixels.  Though, it's weird to note that only here when all these first
> three arguments are in pixels.
> 
> Frankly, I'd rename |padding| to |padding_from_previous_item| and |view| to
> |separator|, and drop the comment entirely as at that point the function is
> self-documenting.

Done.

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_separator_view.cc
(right):

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_separator_view.cc:11: :
separator_color_(SK_ColorBLACK) {
On 2013/01/17 04:14:22, Peter Kasting wrote:
> Nit: Indent 4

Done.

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_separator_view.h (right):

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_separator_view.h:25: //
views::View overrides.
On 2013/01/17 04:14:22, Peter Kasting wrote:
> Nit: I've been trying to standardize on "views::View:".

Done.

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_separator_view.h:28:
protected:
On 2013/01/17 04:14:23, Peter Kasting wrote:
> Why protected?  Why not private (for both overrides)?

Done.

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_view.cc (right):

https://codereview.chromium.org/11418229/diff/83004/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_view.cc:246:
ev_bubble_view_->SetVisible(false);
On 2013/01/17 04:14:23, Peter Kasting wrote:
> Nit: Are all these SetVisible(false) calls even necessary, given the beginning
> of Layout()?

Done.

[Peter Kasting, 2013-01-17 23:30:43]

LGTM

https://codereview.chromium.org/11418229/diff/97001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_layout.cc (right):

https://codereview.chromium.org/11418229/diff/97001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:88: if (type ==
NORMAL) {
Nit: {} not needed

https://codereview.chromium.org/11418229/diff/97001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:245:
LocationBarDecoration* curr = *it;
Nit: Can also use this in the conditional above

https://codereview.chromium.org/11418229/diff/97001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_layout.h (right):

https://codereview.chromium.org/11418229/diff/97001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.h:58: void
AddSeparator(int y, int height, int padding_from_previous_item,
Nit: One arg per line

[kuan, 2013-01-17 23:46:34]

i've addressed nits in patch set 22.  since they're really small nits, i'm going
to send to cq.

https://codereview.chromium.org/11418229/diff/97001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_layout.cc (right):

https://codereview.chromium.org/11418229/diff/97001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:88: if (type ==
NORMAL) {
On 2013/01/17 23:30:43, Peter Kasting wrote:
> Nit: {} not needed

it won't compile w/out it; i suspect it's because DCHECK expands to nothing in
release mode?

https://codereview.chromium.org/11418229/diff/97001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:245:
LocationBarDecoration* curr = *it;
On 2013/01/17 23:30:43, Peter Kasting wrote:
> Nit: Can also use this in the conditional above

Done.

https://codereview.chromium.org/11418229/diff/97001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_layout.h (right):

https://codereview.chromium.org/11418229/diff/97001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.h:58: void
AddSeparator(int y, int height, int padding_from_previous_item,
On 2013/01/17 23:30:43, Peter Kasting wrote:
> Nit: One arg per line

Done.

[commit-bot: I haz the power, 2013-01-17 23:48:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kuan@chromium.org/11418229/108001

[commit-bot: I haz the power, 2013-01-17 23:48:08]

Presubmit check for 11418229-108001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome
    chrome/app/theme

Presubmit checks took 2.7s to calculate.

[commit-bot: I haz the power, 2013-01-17 23:50:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kuan@chromium.org/11418229/108001

[Peter Kasting, 2013-01-18 00:01:50]

https://codereview.chromium.org/11418229/diff/97001/chrome/browser/ui/views/l...
File chrome/browser/ui/views/location_bar/location_bar_layout.cc (right):

https://codereview.chromium.org/11418229/diff/97001/chrome/browser/ui/views/l...
chrome/browser/ui/views/location_bar/location_bar_layout.cc:88: if (type ==
NORMAL) {
On 2013/01/17 23:46:34, kuan wrote:
> On 2013/01/17 23:30:43, Peter Kasting wrote:
> > Nit: {} not needed
> 
> it won't compile w/out it; i suspect it's because DCHECK expands to nothing in
> release mode?

Wow, interesting!  I consider that a very minor bug in DCHECK(), then.  It
should probably expand to "(void)0;" in release mode.  WIll file a bug.

[commit-bot: I haz the power, 2013-01-18 02:47:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kuan@chromium.org/11418229/108001

[commit-bot: I haz the power, 2013-01-18 05:21:29]

Change committed as 177601