[OriginChip] Add options for the position of the origin chip.

[OriginChip] Add options for the position of the origin chip.

Also make the logic and formatting in ToolbarView::Layout more consistent.

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

[msw, 2014-01-10 18:26:04]

Nice patch; lgtm with nits.

https://codereview.chromium.org/134013004/diff/40001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/134013004/diff/40001/chrome/app/generated_res...
chrome/app/generated_resources.grd:6820: +      <message
name="IDS_FLAGS_ORIGIN_CHIP_LEFT_OF_LOCATION_BAR" desc="Option in a drop-down
menu to enable the display of the origin chip to the left of the location bar.">
nit: should all this terminology be changed like s/left/leading/
s/right/trailing/ or will "Left" still be left in RTL UI?

https://codereview.chromium.org/134013004/diff/40001/chrome/browser/search/se...
File chrome/browser/search/search_unittest.cc (right):

https://codereview.chromium.org/134013004/diff/40001/chrome/browser/search/se...
chrome/browser/search/search_unittest.cc:915:
EXPECT_FALSE(ShouldDisplayOriginChip());
nit: also check EXPECT_EQ(ORIGIN_CHIP_DISABLED, GetOriginChipPosition()); here
(it's a little redundant, but maybe worthwhile).

https://codereview.chromium.org/134013004/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/toolbar_view.cc (right):

https://codereview.chromium.org/134013004/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/toolbar_view.cc:641: next_element_x =
site_chip_view_->bounds().right() + kStandardSpacing;
nit: why add spacing between the origin chip and menu, if there is no space
between browser actions and menu? Does the browser actions container have
internal padding?

[Greg Billock, 2014-01-10 18:38:59]

https://codereview.chromium.org/134013004/diff/40001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/134013004/diff/40001/chrome/app/generated_res...
chrome/app/generated_resources.grd:6827: +        Enabled and positioned to the
left of the menu
perhaps "app menu button" here?

https://codereview.chromium.org/134013004/diff/40001/chrome/common/chrome_swi...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/134013004/diff/40001/chrome/common/chrome_swi...
chrome/common/chrome_switches.cc:705: // flag (enable-origin-chip) is equivalent
to the third
Would it be easier to keep just one flag name and have values be
empty = trailing omnibox
"right" = leading omnibox
"menu" = next to menu

[Justin Donnelly, 2014-01-10 20:11:31]

https://codereview.chromium.org/134013004/diff/40001/chrome/app/generated_res...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/134013004/diff/40001/chrome/app/generated_res...
chrome/app/generated_resources.grd:6820: +      <message
name="IDS_FLAGS_ORIGIN_CHIP_LEFT_OF_LOCATION_BAR" desc="Option in a drop-down
menu to enable the display of the origin chip to the left of the location bar.">
On 2014/01/10 18:26:04, msw wrote:
> nit: should all this terminology be changed like s/left/leading/
> s/right/trailing/ or will "Left" still be left in RTL UI? 

Yes, I think you're right.  Maybe someday I'll get used to thinking about i18n
from day one. :)

Done.  I considered rewording the language shown on the flags page to be less
stiff-sounding but given the audience it seems ok.  Let me know what you think.

https://codereview.chromium.org/134013004/diff/40001/chrome/app/generated_res...
chrome/app/generated_resources.grd:6827: +        Enabled and positioned to the
left of the menu
On 2014/01/10 18:39:00, Greg Billock wrote:
> perhaps "app menu button" here?

Done. Good point, it's sounds more consistent to refer to the UI element that
we're positioned relative to. Also added "button" to all of the identifiers.

https://codereview.chromium.org/134013004/diff/40001/chrome/browser/search/se...
File chrome/browser/search/search_unittest.cc (right):

https://codereview.chromium.org/134013004/diff/40001/chrome/browser/search/se...
chrome/browser/search/search_unittest.cc:915:
EXPECT_FALSE(ShouldDisplayOriginChip());
On 2014/01/10 18:26:04, msw wrote:
> nit: also check EXPECT_EQ(ORIGIN_CHIP_DISABLED, GetOriginChipPosition()); here
> (it's a little redundant, but maybe worthwhile).

Oops, yeah, meant to add this everywhere but missed this one. Done.

https://codereview.chromium.org/134013004/diff/40001/chrome/browser/ui/views/...
File chrome/browser/ui/views/toolbar/toolbar_view.cc (right):

https://codereview.chromium.org/134013004/diff/40001/chrome/browser/ui/views/...
chrome/browser/ui/views/toolbar/toolbar_view.cc:641: next_element_x =
site_chip_view_->bounds().right() + kStandardSpacing;
On 2014/01/10 18:26:04, msw wrote:
> nit: why add spacing between the origin chip and menu, if there is no space
> between browser actions and menu? Does the browser actions container have
> internal padding?

Yes, the browser actions container adds the spacing.  See
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

https://codereview.chromium.org/134013004/diff/40001/chrome/common/chrome_swi...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/134013004/diff/40001/chrome/common/chrome_swi...
chrome/common/chrome_switches.cc:705: // flag (enable-origin-chip) is equivalent
to the third
On 2014/01/10 18:39:00, Greg Billock wrote:
> Would it be easier to keep just one flag name and have values be
> empty = trailing omnibox
> "right" = leading omnibox
> "menu" = next to menu

That would be nicer, yes, but this is necessary to support the Finch
configuration we need.  Copied comment from about_flags.cc that explains this.

[msw, 2014-01-10 20:47:57]

Still lgtm, thanks.

[Greg Billock, 2014-01-10 21:09:42]

On 2014/01/10 20:47:57, msw wrote:
> Still lgtm, thanks.

lgtm

[commit-bot: I haz the power, 2014-01-10 21:13:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdonnelly@chromium.org/134013004/240001

[commit-bot: I haz the power, 2014-01-10 22:00:23]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Justin Donnelly, 2014-01-10 22:18:58]

+samarth for review of chrome/browser/search/search*

[samarth, 2014-01-10 22:42:06]

search lgtm

[commit-bot: I haz the power, 2014-01-10 23:47:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jdonnelly@chromium.org/134013004/240001

[commit-bot: I haz the power, 2014-01-12 02:41:48]

Change committed as 244417