Alternate NTP: Add search token to omnibox

Alternate NTP: Add search token to omnibox

This is the Mac verison of this Views CL:
https://chromiumcodereview.appspot.com/11418229/

This CL adds a search token decoration to the omnibox.

The search token is displayed when the user does a search from the omnibox.

The search token appears on the right with a separtor between it and other decorations.

Screenshot:
  no separator: http://i.imgur.com/TXCixtn.png
  with separator 1x: http://imgur.com/xtxv9hG
  with separator 2x: http://imgur.com/2Pzi7dN

BUG=163190
TBR=thakis@chromium.org

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

[sail, 2013-01-21 03:04:30]

Still needs tests but sending this out early to get feedback.

[sail, 2013-01-21 12:51:37]

added tests

[kuan, 2013-01-23 17:01:32]

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (right):

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:30: class
LocationBarViewMacBrowserTest;
nit: wrong alphabetical order

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/search_token_decoration.mm (right):

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/search_token_decoration.mm:18: NSRect
text_rect = NSInsetRect(frame, 0, 4);
why 4?

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/search_token_decoration.mm:26: return
desired_width;
where do u handle the long query string scenario?
on win/cros, i hide search token if padding between end of query and beginning
of search token is < 20px (which is a predefined views const).

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/separator_decoration.h (right):

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/separator_decoration.h:11: // This class is
used to draw a separtor between decorations in the omnibox.
nit: s/separtor/separator/

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/separator_decoration.mm (right):

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/separator_decoration.mm:19: NSRect rect =
NSInsetRect(frame, 0, 1);
why 1?

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/separator_decoration.mm:23:
NSRectFill(rect);
does this draw a 1-px thick vertical line in retina display?  cole wants a 1-px
thick vertical line in both 1x and 2x displays.

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/separator_decoration.mm:27: return 6;
is this for the padding between search token and separator?  that's what i use
for win/cros.  if so, what abt width of the separator?

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/separator_decoration.mm:39: return [color
colorWithAlphaComponent:0.5 / line_width];
the colors u use for search token text and separator are different from
win/cros, u may want to shoot cole a screenshot of ur colors to get his
approval.

[kuan, 2013-01-23 17:02:42]

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/separator_decoration.mm (right):

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/separator_decoration.mm:27: return 6;
On 2013/01/23 17:01:32, kuan wrote:
> is this for the padding between search token and separator?  that's what i use
> for win/cros.  if so, what abt width of the separator?

also, where do u handle the scenario where separator is hidden if search token
is hidden for long query?

[sail, 2013-01-27 21:11:07]

kuan: Added ability to hide the search token if it overlaps the omnibox text.
Please take another look.
shess: OWNERS review

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h (right):

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h:30: class
LocationBarViewMacBrowserTest;
On 2013/01/23 17:01:32, kuan wrote:
> nit: wrong alphabetical order

Done.

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/search_token_decoration.mm (right):

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/search_token_decoration.mm:18: NSRect
text_rect = NSInsetRect(frame, 0, 4);
On 2013/01/23 17:01:32, kuan wrote:
> why 4?

Done.
This is used to vertically center text. Each decoration had its own constant for
this so I've move this out to a common constant
(LocationBarDecoration::kTextYInset).

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/search_token_decoration.mm:26: return
desired_width;
On 2013/01/23 17:01:32, kuan wrote:
> where do u handle the long query string scenario?
> on win/cros, i hide search token if padding between end of query and beginning
> of search token is < 20px (which is a predefined views const).

Done.
My original patch didn't have this functionality. I've added it to
autocomplete_text_field_cell.mm CalculatePositionsHelper().

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/separator_decoration.h (right):

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/separator_decoration.h:11: // This class is
used to draw a separtor between decorations in the omnibox.
On 2013/01/23 17:01:32, kuan wrote:
> nit: s/separtor/separator/

Done.

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
File chrome/browser/ui/cocoa/location_bar/separator_decoration.mm (right):

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/separator_decoration.mm:19: NSRect rect =
NSInsetRect(frame, 0, 1);
On 2013/01/23 17:01:32, kuan wrote:
> why 1?

This is needed to avoid overlapping the omnibox bezel. Comment added.

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/separator_decoration.mm:23:
NSRectFill(rect);
On 2013/01/23 17:01:32, kuan wrote:
> does this draw a 1-px thick vertical line in retina display?  cole wants a
1-px
> thick vertical line in both 1x and 2x displays.

Yep, cr_lineWidth explicitly returns a 1pixel line width.

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/separator_decoration.mm:27: return 6;
On 2013/01/23 17:02:42, kuan wrote:
> On 2013/01/23 17:01:32, kuan wrote:
> > is this for the padding between search token and separator?  that's what i
use
> > for win/cros.  if so, what abt width of the separator?
> 
> also, where do u handle the scenario where separator is hidden if search token
> is hidden for long query?

Done.
My original patch didn't have this functionality. I've added it to
autocomplete_text_field_cell.mm HideUnneededSeparators().

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/separator_decoration.mm:27: return 6;
On 2013/01/23 17:01:32, kuan wrote:
> is this for the padding between search token and separator?  that's what i use
> for win/cros.  if so, what abt width of the separator?

I couldn't account for the width because that depends on the line width which
depends on the view. I think it's not too bad this way. I'll send screenshots to
Cole and you and get more feedback on the padding.

https://codereview.chromium.org/12042002/diff/5001/chrome/browser/ui/cocoa/lo...
chrome/browser/ui/cocoa/location_bar/separator_decoration.mm:39: return [color
colorWithAlphaComponent:0.5 / line_width];
On 2013/01/23 17:01:32, kuan wrote:
> the colors u use for search token text and separator are different from
> win/cros, u may want to shoot cole a screenshot of ur colors to get his
> approval.

Done.

The text color matches the deemphasized text here:
https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/cocoa/l...

I've changed the separator color to be the text color at 25% opacity.

I'll email you and Cole screenshots to verify that this looks ok.

[kuan, 2013-01-28 18:02:00]

thanks for addressing the comments and the screenshots.  lgtm, if cole approves
the screenshots.

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/location_bar_decoration.h (right):

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/location_bar_decoration.h:102: // How far
to inset the hint text area from the top and bottom. This
nit: is it for any text or just hint text?

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:804: if
(toolbar_model_->WouldReplaceSearchURLWithSearchTerms()) {
just heads up that in the bug rpt, pkasting@ is advocating hiding search token
while user is typing.

[Scott Hess - ex-Googler, 2013-01-28 21:37:27]

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:111: //
Collapse if necessary.
Way Back When, I thought that this kind of auto-adjusting layout decision was
being handled in LocationBarViewMac::Layout(), because that's where it was being
handled for other platforms.  But I don't see obvious evidence of this, and
other platforms have changed a bunch in this area.  The main rationale for this
was so that in the future people wouldn't distribute things randomly across the
code and make it hard to understand.  Since you're already looking at the code
from other platforms, please give it a pass to see if this code will make sense
when someone from that platform sees it, in case it needs adjustment.

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:115:
used_width = LocationBarDecoration::kOmittedWidth;
I wonder if this wouldn't make more sense to fold into GetWidthForSpace(),
rather than having two places for the knowledge to live?  I think that if we
really want this (*), it would be reasonable to pass down both "total available
space" as NSWidth(available) and "desired decoration space" as
NSWidth(available) - text_width.  Then the decoration can make the decision
about collapsing and how much padding to consider.

(*) I think that there used to be more collapsing going on in the location-bar
code, but we moved away from it for some reason.

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:180:
HideUnneededSeparators(&result_decorations);
The helper already knows about applying auto-collapse, I don't think it would be
materially worse to have it also know how to check for separators against the
endpoints, at least versus having this code duplication here.  It would need two
tests, one inside the loop and one outside, but HideUnneededSeparators() also
needs two tests.  I don't have the heart to dig into the other-platform code to
see if as-is matches that better.

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:258:
NSAttributedString* text = [self attributedStringValue];
Suggest either a helper for "[[self attributedStringValue] size].width", or
re-casting CalculatePositionsInFrame() as a method.  I think I'd prefer the
former, a big reason why it's a function in the anon namespace is because I want
to prevent people from reaching out to random area of the profile to influence
layout dynamically.

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:710: [cell
addRightDecoration:search_token_decoration_.get()];
Is the separator truly independent from the search token?  In the cell
implementation code, you have various code to handle removing extraneous
separators from the LHS and RHS decorations.  If the separator is actually just
part of the search token, then all of that goes away and you can implement one
less decoration.

[Apologies, I can't quite reason through whether web_intents_button_decoration
is guaranteed.  It is set not visible when there is no WebContents*, I can't
recall if that's a real case or something that can happen during tab close.]

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/search_token_decoration.h (right):

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/search_token_decoration.h:16: // "Google
Search".
Is this going to replace KeywordHintDecoration?

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/separator_decoration.mm (right):

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/separator_decoration.mm:21:
[SeparatorColor(control_view) set];
Why does this pass the view?

[sail, 2013-01-29 22:31:08]

Addressed comments. Please take another look.

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:111: //
Collapse if necessary.
On 2013/01/28 21:37:27, shess wrote:
> Way Back When, I thought that this kind of auto-adjusting layout decision was
> being handled in LocationBarViewMac::Layout(), because that's where it was
being
> handled for other platforms.  But I don't see obvious evidence of this, and
> other platforms have changed a bunch in this area.  The main rationale for
this
> was so that in the future people wouldn't distribute things randomly across
the
> code and make it hard to understand.  Since you're already looking at the code
> from other platforms, please give it a pass to see if this code will make
sense
> when someone from that platform sees it, in case it needs adjustment.

I think moving this might be a good idea. In views all this logic is in a
special class (LocationBarLayout). We could do the same.
I've filed http://crbug.com/172949 to track this work.

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:115:
used_width = LocationBarDecoration::kOmittedWidth;
On 2013/01/28 21:37:27, shess wrote:
> I wonder if this wouldn't make more sense to fold into GetWidthForSpace(),
> rather than having two places for the knowledge to live?  I think that if we
> really want this (*), it would be reasonable to pass down both "total
available
> space" as NSWidth(available) and "desired decoration space" as
> NSWidth(available) - text_width.  Then the decoration can make the decision
> about collapsing and how much padding to consider.
> 
> (*) I think that there used to be more collapsing going on in the location-bar
> code, but we moved away from it for some reason.

Done.
Moved the collapse logic to SearchTokenDecoration::GetWidthForSpace(). Added
text_width as a parameter to GetWidthForSpace().

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:180:
HideUnneededSeparators(&result_decorations);
On 2013/01/28 21:37:27, shess wrote:
> The helper already knows about applying auto-collapse, I don't think it would
be
> materially worse to have it also know how to check for separators against the
> endpoints, at least versus having this code duplication here.  It would need
two
> tests, one inside the loop and one outside, but HideUnneededSeparators() also
> needs two tests.  I don't have the heart to dig into the other-platform code
to
> see if as-is matches that better.

The views code also does two passes.
I tried moving the check inside the loop in CalculatePositionsHelper() but the
logic to update the frames became too complicated. 

I find doing two passes a lot easier to understand.

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:258:
NSAttributedString* text = [self attributedStringValue];
On 2013/01/28 21:37:27, shess wrote:
> Suggest either a helper for "[[self attributedStringValue] size].width", or
> re-casting CalculatePositionsInFrame() as a method.  I think I'd prefer the
> former, a big reason why it's a function in the anon namespace is because I
want
> to prevent people from reaching out to random area of the profile to influence
> layout dynamically.

Done.
Added a helper method -[AutocompleteTextFieldCell textWidth]

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/location_bar_decoration.h (right):

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/location_bar_decoration.h:102: // How far
to inset the hint text area from the top and bottom. This
On 2013/01/28 18:02:00, kuan wrote:
> nit: is it for any text or just hint text?

Done.
Removed the reference to hint text.

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:710: [cell
addRightDecoration:search_token_decoration_.get()];
On 2013/01/28 21:37:27, shess wrote:
> Is the separator truly independent from the search token?  In the cell
> implementation code, you have various code to handle removing extraneous
> separators from the LHS and RHS decorations.  If the separator is actually
just
> part of the search token, then all of that goes away and you can implement one
> less decoration.
> 
> [Apologies, I can't quite reason through whether web_intents_button_decoration
> is guaranteed.  It is set not visible when there is no WebContents*, I can't
> recall if that's a real case or something that can happen during tab close.]

Currently only the search token has a separator after it.
I think using a separate decorator has two advantages:
  - first it matches the views code
  - second it allows the hide / show separator code to live in the layout

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:804: if
(toolbar_model_->WouldReplaceSearchURLWithSearchTerms()) {
On 2013/01/28 18:02:00, kuan wrote:
> just heads up that in the bug rpt, pkasting@ is advocating hiding search token
> while user is typing.

Done.
Re-added the !GetInputInProgress() condition.

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/search_token_decoration.h (right):

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/search_token_decoration.h:16: // "Google
Search".
On 2013/01/28 21:37:27, shess wrote:
> Is this going to replace KeywordHintDecoration?

No it won't. The search token is only shown for the default search provider. The
keyword hint decoration will still be used when you're going to do a custom
search.

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/separator_decoration.mm (right):

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/separator_decoration.mm:21:
[SeparatorColor(control_view) set];
On 2013/01/28 21:37:27, shess wrote:
> Why does this pass the view?

Done.
Removed this code.

[sail, 2013-01-29 22:48:28]

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/separator_decoration.mm (right):

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/separator_decoration.mm:21:
[SeparatorColor(control_view) set];
On 2013/01/29 22:31:08, sail wrote:
> On 2013/01/28 21:37:27, shess wrote:
> > Why does this pass the view?
> 
> Done.
> Removed this code.

Oops. I was looking at the wrong code when I wrote that comment.

Cole wants the separator color to be darker in retina mode. To do this I need
the view.

The original CL wasn't using the view. I've updated the code to use it.

[sail, 2013-01-29 23:09:50]

On 2013/01/28 18:02:00, kuan wrote:
> thanks for addressing the comments and the screenshots.  lgtm, if cole
approves
> the screenshots.

Updated colors and padding. Got Cole's approval on latest screenshots.

[Scott Hess - ex-Googler, 2013-01-29 23:28:36]

Thanks for bearing with me.  Takes effort to swap all this back into brain.

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:180:
HideUnneededSeparators(&result_decorations);
On 2013/01/29 22:31:08, sail wrote:
> On 2013/01/28 21:37:27, shess wrote:
> > The helper already knows about applying auto-collapse, I don't think it
would
> be
> > materially worse to have it also know how to check for separators against
the
> > endpoints, at least versus having this code duplication here.  It would need
> two
> > tests, one inside the loop and one outside, but HideUnneededSeparators()
also
> > needs two tests.  I don't have the heart to dig into the other-platform code
> to
> > see if as-is matches that better.
> 
> The views code also does two passes.
> I tried moving the check inside the loop in CalculatePositionsHelper() but the
> logic to update the frames became too complicated. 
> 
> I find doing two passes a lot easier to understand.

OK.  My main concern is whether there will be subtle issues due to the passes
not returning the same values.

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:710: [cell
addRightDecoration:search_token_decoration_.get()];
On 2013/01/29 22:31:08, sail wrote:
> On 2013/01/28 21:37:27, shess wrote:
> > Is the separator truly independent from the search token?  In the cell
> > implementation code, you have various code to handle removing extraneous
> > separators from the LHS and RHS decorations.  If the separator is actually
> just
> > part of the search token, then all of that goes away and you can implement
one
> > less decoration.
> > 
> > [Apologies, I can't quite reason through whether
web_intents_button_decoration
> > is guaranteed.  It is set not visible when there is no WebContents*, I can't
> > recall if that's a real case or something that can happen during tab close.]
> 
> Currently only the search token has a separator after it.
> I think using a separate decorator has two advantages:
>   - first it matches the views code
>   - second it allows the hide / show separator code to live in the layout

Matching the views code is a good reason.

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/search_token_decoration.h (right):

https://codereview.chromium.org/12042002/diff/12001/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/search_token_decoration.h:16: // "Google
Search".
On 2013/01/29 22:31:08, sail wrote:
> On 2013/01/28 21:37:27, shess wrote:
> > Is this going to replace KeywordHintDecoration?
> 
> No it won't. The search token is only shown for the default search provider.
The
> keyword hint decoration will still be used when you're going to do a custom
> search.

Looking closer, it looks like this is exactly like the SelectedKeywordDecoration
case, except that you didn't select it, it is implicitly selected, and it draws
things entirely differently and in a different position and stuff.

https://codereview.chromium.org/12042002/diff/18002/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/location_bar_decoration.h (right):

https://codereview.chromium.org/12042002/diff/18002/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/location_bar_decoration.h:39: // the
omnibox text.
s/widget/width/ .  It might be worth adding ", which can be greater than
|width|" or something to that end, so that nobody makes inappropriate
assumptions about their relationship.

Sorry about adding the load of new files to modify :-).  I would have been happy
enough to have an alternate entry point which defaulted to the old version, but
it's really six-vs-half-dozen.

https://codereview.chromium.org/12042002/diff/18002/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/location_bar_decoration.h:93: virtual
SeparatorDecoration* AsSeparatorDecoration();
Obviously the "act as a button" part doesn't apply.  Going deeper, do you think
should should be IsSeparator()?  I can't think of cases where you'd need the
actual SeparatorDecoration instance.

https://codereview.chromium.org/12042002/diff/18002/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/search_token_decoration.mm (right):

https://codereview.chromium.org/12042002/diff/18002/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/search_token_decoration.mm:29: return
kOmittedWidth;
This test is now implicit in the later test, I think.

Also, the phrasing of the comment below makes it sound like they might visually
overlap, which would never happen.  Maybe instead suggest to collapse if the
search token would reduce the space left for the text.

https://codereview.chromium.org/12042002/diff/23004/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/12042002/diff/23004/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:137:
decorations->pop_back();
ObPedantic: pop_back() case first, in case you do both.  It'll save you like two
cycles.  Leveraged across the fleet, that'll add up!  [To nothing, since we can
never do both, currently.]

https://codereview.chromium.org/12042002/diff/23004/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/12042002/diff/23004/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:728: bool
show_search_token = !GetSearchProviderName().empty();
Style below has this const.

Also, I believe that the show_search_token and is_keyword_hint should be
exclusive?  Like you can show neither, or one of them, but never both?  If so,
it might make sense to change is_keyword_hint to something like
!show_search_token && is_keyword_hint() to reflect that.

Actually, I think it's exclusive WRT keyword mode, too.  Is it even possible to
have !keyword.empty() when show_search_token is true?  It would be sort of nice
to have it part of the else-if chain to reflect that.

This code is so wonderful!

https://codereview.chromium.org/12042002/diff/23004/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/separator_decoration.mm (right):

https://codereview.chromium.org/12042002/diff/23004/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/separator_decoration.mm:36: return
[[NSColor lightGrayColor] colorWithAlphaComponent:0.25 / lineWidth];
This seems like an odd and dangerous formulation to me.  I think it would be
clearer to have two alpha values, one for the special case and one for all
others, with a ternary operator or whatever works best.

https://codereview.chromium.org/12042002/diff/14015/chrome/browser/ui/cocoa/l...
File
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm
(right):

https://codereview.chromium.org/12042002/diff/14015/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm:47:
CGFloat my_width = 20.0;
Make it a real constant, then.

https://codereview.chromium.org/12042002/diff/14015/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm:371:
inFrame:bounds];
Alignment.

https://codereview.chromium.org/12042002/diff/14015/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/12042002/diff/14015/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:728: bool
show_search_token = !GetSearchProviderName().empty();
If you captured the GetSearchProviderName() results in a local variable, would
that result in two copies, or just one?  Asking due to having it called again if
non-empty().

https://codereview.chromium.org/12042002/diff/14015/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_browsertest.mm
(right):

https://codereview.chromium.org/12042002/diff/14015/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_browsertest.mm:15:
class MockToolbarModel : public ToolbarModel {
I have no idea what's really happening in here, but you rock!

https://codereview.chromium.org/12042002/diff/14015/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/search_token_decoration.h (right):

https://codereview.chromium.org/12042002/diff/14015/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/search_token_decoration.h:23:
search_provider_name_ = search_provider_name;
Is there any value to storing the original search_provider_name versus having
this function include the contents of GetAttributedString(), leaving the result
in a ScopedNSObject?

https://codereview.chromium.org/12042002/diff/14015/chrome/chrome_browser_ui....
File chrome/chrome_browser_ui.gypi (left):

https://codereview.chromium.org/12042002/diff/14015/chrome/chrome_browser_ui....
chrome/chrome_browser_ui.gypi:646:
'browser/ui/cocoa/location_bar/selected_keyword_decoration.h',
Did this lose a line inadvertantly?

[sail, 2013-01-30 01:40:55]

https://codereview.chromium.org/12042002/diff/18002/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/location_bar_decoration.h (right):

https://codereview.chromium.org/12042002/diff/18002/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/location_bar_decoration.h:39: // the
omnibox text.
On 2013/01/29 23:28:36, shess wrote:
> s/widget/width/ .  It might be worth adding ", which can be greater than
> |width|" or something to that end, so that nobody makes inappropriate
> assumptions about their relationship.
> 
> Sorry about adding the load of new files to modify :-).  I would have been
happy
> enough to have an alternate entry point which defaulted to the old version,
but
> it's really six-vs-half-dozen.

Done.

https://codereview.chromium.org/12042002/diff/18002/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/location_bar_decoration.h:93: virtual
SeparatorDecoration* AsSeparatorDecoration();
On 2013/01/29 23:28:36, shess wrote:
> Obviously the "act as a button" part doesn't apply. 
Oops, removed.

> Going deeper, do you think
> should should be IsSeparator()?  I can't think of cases where you'd need the
> actual SeparatorDecoration instance.

Done.
Changed to IsSeparator

https://codereview.chromium.org/12042002/diff/18002/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/search_token_decoration.mm (right):

https://codereview.chromium.org/12042002/diff/18002/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/search_token_decoration.mm:29: return
kOmittedWidth;
On 2013/01/29 23:28:36, shess wrote:
> This test is now implicit in the later test, I think.
> 
> Also, the phrasing of the comment below makes it sound like they might
visually
> overlap, which would never happen.  Maybe instead suggest to collapse if the
> search token would reduce the space left for the text.

Done.

https://codereview.chromium.org/12042002/diff/23004/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/12042002/diff/23004/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:137:
decorations->pop_back();
On 2013/01/29 23:28:36, shess wrote:
> ObPedantic: pop_back() case first, in case you do both.  It'll save you like
two
> cycles.  Leveraged across the fleet, that'll add up!  [To nothing, since we
can
> never do both, currently.]

Done.

https://codereview.chromium.org/12042002/diff/23004/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/12042002/diff/23004/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:728: bool
show_search_token = !GetSearchProviderName().empty();
On 2013/01/29 23:28:36, shess wrote:
> Style below has this const.
> 
> Also, I believe that the show_search_token and is_keyword_hint should be
> exclusive?  Like you can show neither, or one of them, but never both?  If so,
> it might make sense to change is_keyword_hint to something like
> !show_search_token && is_keyword_hint() to reflect that.
> 
> Actually, I think it's exclusive WRT keyword mode, too.  Is it even possible
to
> have !keyword.empty() when show_search_token is true?  It would be sort of
nice
> to have it part of the else-if chain to reflect that.
> 
> This code is so wonderful!

Done.
Made const and changed to ifelse.
I've also change the if statements better match the Views code here:
https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/views/l...

https://codereview.chromium.org/12042002/diff/23004/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/separator_decoration.mm (right):

https://codereview.chromium.org/12042002/diff/23004/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/separator_decoration.mm:36: return
[[NSColor lightGrayColor] colorWithAlphaComponent:0.25 / lineWidth];
On 2013/01/29 23:28:36, shess wrote:
> This seems like an odd and dangerous formulation to me.  I think it would be
> clearer to have two alpha values, one for the special case and one for all
> others, with a ternary operator or whatever works best.

I agree that this is weird but this is how we do it through out the code base.
Here are a few examples:
https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/cocoa/s...
https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/cocoa/n...

https://codereview.chromium.org/12042002/diff/14015/chrome/browser/ui/cocoa/l...
File
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm
(right):

https://codereview.chromium.org/12042002/diff/14015/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm:47:
CGFloat my_width = 20.0;
On 2013/01/29 23:28:37, shess wrote:
> Make it a real constant, then.

Done.

https://codereview.chromium.org/12042002/diff/14015/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm:371:
inFrame:bounds];
On 2013/01/29 23:28:37, shess wrote:
> Alignment.

Done.

https://codereview.chromium.org/12042002/diff/14015/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (right):

https://codereview.chromium.org/12042002/diff/14015/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:728: bool
show_search_token = !GetSearchProviderName().empty();
On 2013/01/29 23:28:37, shess wrote:
> If you captured the GetSearchProviderName() results in a local variable, would
> that result in two copies, or just one?  Asking due to having it called again
if
> non-empty().

Done.
Changed to use a local variable.

https://codereview.chromium.org/12042002/diff/14015/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_browsertest.mm
(right):

https://codereview.chromium.org/12042002/diff/14015/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac_browsertest.mm:15:
class MockToolbarModel : public ToolbarModel {
On 2013/01/29 23:28:37, shess wrote:
> I have no idea what's really happening in here, but you rock!

Done.
Added comment :-P

https://codereview.chromium.org/12042002/diff/14015/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/search_token_decoration.h (right):

https://codereview.chromium.org/12042002/diff/14015/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/search_token_decoration.h:23:
search_provider_name_ = search_provider_name;
On 2013/01/29 23:28:37, shess wrote:
> Is there any value to storing the original search_provider_name versus having
> this function include the contents of GetAttributedString(), leaving the
result
> in a ScopedNSObject?

None really. I'd rather have it simply store the string though. I think that's
more common.

https://codereview.chromium.org/12042002/diff/14015/chrome/chrome_browser_ui....
File chrome/chrome_browser_ui.gypi (left):

https://codereview.chromium.org/12042002/diff/14015/chrome/chrome_browser_ui....
chrome/chrome_browser_ui.gypi:646:
'browser/ui/cocoa/location_bar/selected_keyword_decoration.h',
On 2013/01/29 23:28:37, shess wrote:
> Did this lose a line inadvertantly?

Done.
Oops, fixed. Good catch.

[Scott Hess - ex-Googler, 2013-01-30 23:48:42]

LGTM, minor nits.  Ignore them all if you hate freedom.

https://codereview.chromium.org/12042002/diff/23004/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/separator_decoration.mm (right):

https://codereview.chromium.org/12042002/diff/23004/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/separator_decoration.mm:36: return
[[NSColor lightGrayColor] colorWithAlphaComponent:0.25 / lineWidth];
On 2013/01/30 01:40:55, sail wrote:
> On 2013/01/29 23:28:36, shess wrote:
> > This seems like an odd and dangerous formulation to me.  I think it would be
> > clearer to have two alpha values, one for the special case and one for all
> > others, with a ternary operator or whatever works best.
> 
> I agree that this is weird but this is how we do it through out the code base.
> Here are a few examples:
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/cocoa/s...
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/cocoa/n...

Not so great if |view| ends up nil in any of those places, though...

https://codereview.chromium.org/12042002/diff/14015/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/search_token_decoration.h (right):

https://codereview.chromium.org/12042002/diff/14015/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/search_token_decoration.h:23:
search_provider_name_ = search_provider_name;
On 2013/01/30 01:40:56, sail wrote:
> On 2013/01/29 23:28:37, shess wrote:
> > Is there any value to storing the original search_provider_name versus
having
> > this function include the contents of GetAttributedString(), leaving the
> result
> > in a ScopedNSObject?
> 
> None really. I'd rather have it simply store the string though. I think that's
> more common.

Why I suggested is because this code never actually uses the original
formulation.  I'm fine with leaving it alone, but suggest you put a log line in
GetAttributedString() and set_search_provider() and verify whether you're
comfortable with creating the object that often.  My memory is that location-bar
does awesome things like layout 45 times with no change happening, so if you
find it being called like 100x per character typed ... well, you can still feel
free to leave it as-is, I just don't want you to imagine that someone else in
this code is smart about such things.

https://codereview.chromium.org/12042002/diff/16044/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/12042002/diff/16044/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:105:
CGFloat used_width =
Now you can pull the const back on this line.

https://codereview.chromium.org/12042002/diff/16044/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:387:
&textFrame);
Reformatted all the other ones, might as well reformat this one.

https://codereview.chromium.org/12042002/diff/16044/chrome/browser/ui/cocoa/l...
File
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm
(right):

https://codereview.chromium.org/12042002/diff/16044/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm:47:
const CGFloat my_width = 20.0;
Full Monty!  kMyWidth!

https://codereview.chromium.org/12042002/diff/16044/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm:345:
EXPECT_EQ(0, NSWidth(rect));
As long as you're here, check with both hidden, too.

[sail, 2013-01-31 03:08:00]

https://codereview.chromium.org/12042002/diff/23004/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/separator_decoration.mm (right):

https://codereview.chromium.org/12042002/diff/23004/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/separator_decoration.mm:36: return
[[NSColor lightGrayColor] colorWithAlphaComponent:0.25 / lineWidth];
On 2013/01/30 23:48:42, shess wrote:
> On 2013/01/30 01:40:55, sail wrote:
> > On 2013/01/29 23:28:36, shess wrote:
> > > This seems like an odd and dangerous formulation to me.  I think it would
be
> > > clearer to have two alpha values, one for the special case and one for all
> > > others, with a ternary operator or whatever works best.
> > 
> > I agree that this is weird but this is how we do it through out the code
base.
> > Here are a few examples:
> >
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/cocoa/s...
> >
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/ui/cocoa/n...
> 
> Not so great if |view| ends up nil in any of those places, though...

Done.
Used an if statement instead.

https://codereview.chromium.org/12042002/diff/14015/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/search_token_decoration.h (right):

https://codereview.chromium.org/12042002/diff/14015/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/search_token_decoration.h:23:
search_provider_name_ = search_provider_name;
On 2013/01/30 23:48:42, shess wrote:
> On 2013/01/30 01:40:56, sail wrote:
> > On 2013/01/29 23:28:37, shess wrote:
> > > Is there any value to storing the original search_provider_name versus
> having
> > > this function include the contents of GetAttributedString(), leaving the
> > result
> > > in a ScopedNSObject?
> > 
> > None really. I'd rather have it simply store the string though. I think
that's
> > more common.
> 
> Why I suggested is because this code never actually uses the original
> formulation.  I'm fine with leaving it alone, but suggest you put a log line
in
> GetAttributedString() and set_search_provider() and verify whether you're
> comfortable with creating the object that often.  My memory is that
location-bar
> does awesome things like layout 45 times with no change happening, so if you
> find it being called like 100x per character typed ... well, you can still
feel
> free to leave it as-is, I just don't want you to imagine that someone else in
> this code is smart about such things.

Done.
Ahh, scary. Changed to cache the attributed string.

https://codereview.chromium.org/12042002/diff/16044/chrome/browser/ui/cocoa/l...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm
(right):

https://codereview.chromium.org/12042002/diff/16044/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:105:
CGFloat used_width =
On 2013/01/30 23:48:42, shess wrote:
> Now you can pull the const back on this line.

Done.

https://codereview.chromium.org/12042002/diff/16044/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell.mm:387:
&textFrame);
On 2013/01/30 23:48:42, shess wrote:
> Reformatted all the other ones, might as well reformat this one.

Done.

https://codereview.chromium.org/12042002/diff/16044/chrome/browser/ui/cocoa/l...
File
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm
(right):

https://codereview.chromium.org/12042002/diff/16044/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm:47:
const CGFloat my_width = 20.0;
On 2013/01/30 23:48:42, shess wrote:
> Full Monty!  kMyWidth!

Done.

https://codereview.chromium.org/12042002/diff/16044/chrome/browser/ui/cocoa/l...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_cell_unittest.mm:345:
EXPECT_EQ(0, NSWidth(rect));
On 2013/01/30 23:48:42, shess wrote:
> As long as you're here, check with both hidden, too.

Done.

[sail, 2013-01-31 03:10:07]

TBR thakis for chrome/*.gyp* changes.

[commit-bot: I haz the power, 2013-01-31 03:13:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/12042002/19031

[commit-bot: I haz the power, 2013-01-31 07:55:57]

Change committed as 179818

[Nico, 2013-02-01 17:50:29]

Please do not TBR cls against me without putting me on the review. TBR means "to
be reviewed", not "submitted without review".

[sail, 2013-02-01 17:53:23]

On 2013/02/01 17:50:29, Nico wrote:
> Please do not TBR cls against me without putting me on the review. TBR means
"to
> be reviewed", not "submitted without review".

Oops. After adding the TBR to the CL description I made sure to hit "Publish"
but then I forgot to add to the reviewer's list. Sorry!