Remove origin chip v1 and "hide on input" v2 behavior.

Remove origin chip v1 and "hide on input" v2 behavior.

Eliminates "V2" from all origin chip v2 code/names except the field trial flag name (so as not to screw up in-process field trials).  Eliminates "hide on" flags and just uses a single "enabled" flag for v2.

BUG=none
TEST=Only one origin chip flag appears in about:flags.

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

[Greg Billock, 2014-05-13 16:46:10]

Good. I was just telling Justin we should scrap v1 -- I don't see any world
where we don't end up deleting that code.

[Peter Kasting, 2014-05-13 20:30:51]

Added Alexei in hopes he can answer the question I pose below.

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:1650: "origin-chip-in-omnibox",
Alexei, should we change the name of this to something else, since the choice
set is not the same as in the previous "origin-chip-in-omnibox"?  I want to
ensure I don't screw up any stat collection.

[Alexei Svitkine (slow), 2014-05-13 20:49:41]

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:1650: "origin-chip-in-omnibox",
On 2014/05/13 20:30:52, Peter Kasting wrote:
> Alexei, should we change the name of this to something else, since the choice
> set is not the same as in the previous "origin-chip-in-omnibox"?  I want to
> ensure I don't screw up any stat collection.

The name of the flag is actually not used for anything that relates to
experiments. The integration with experiments is with the flag names themselves
(i.e. the kOriginChipChoices values).

If you're changing those (and it looks like you are), you probably want to
update the internal configs that reference them as well. I'll give you a pointer
to that offline.

[Justin Donnelly, 2014-05-13 22:19:33]

I'm kind of disappointed.  I was looking forward to deleting this code myself.
:)  But obviously it was blocking you and Greg from making changes, so I
appreciate you doing the work.

You mind also moving chrome/browser/ui/toolbar/origin_chip_info.{h,cc} into
chrome/browser/ui/omnibox?  I left it in toolbar/ since it was still supporting
both but now it mercifully has no more relation to the toolbar.

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:365: { IDS_FLAGS_ORIGIN_CHIP_ON_SRP,
switches::kEnableOriginChipOnSrp, ""}
I think dropping the hide-on-input option here could cause a problem.  I'm not
certain, because I wasn't able to figure out what code governs this, but when I
added new options earlier it looked like the position within the
MULTI_VALUE_TYPE array was being encoded in the user profile.

You could test this.  If I'm right, then if you select "hide on input" (4th
position in old array) with an old binary you'll now get "on SRP" (4th position
in new array) running a binary with this change.

Unless that's an acceptable side effect?

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/search/se...
File chrome/browser/search/search.h (right):

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/search/se...
chrome/browser/search/search.h:54: ORIGIN_CHIP_UNUSED_PLACEHOLDER,  // In list
to avoid changing fieldtrial vals
Per Alexei's comment, I think this is unnecessary.

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/search/se...
chrome/browser/search/search.h:192: // Returns a value indicating what event
should trigger hiding the origin chip.
"What event should trigger hiding" is no longer accurate.  Suggest "Returns a
value indicating under what conditions the origin chip should be shown."

Relatedly, "enabled" and "on SRP" don't seem like the right names.  They're both
enabling the origin chip.  I would choose "enabled on all pages" and "enabled on
SRP".

I don't feel strongly about this, however, because it's difficult to say what's
next and I don't want to create a bunch of busy work for you here changing the
names.  But let me know what you think.

Speculating, the next step might be to add an option like showing the URL for a
short period on page load or scroll up or whatever.  In which case this value
might be more like "conditions under which the origin chip and URL are shown." 
And we'd have a variety of options like EnabledWithSuchAndSuchOption where none
is necessarily privileged over the others.

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/ui/toolba...
File chrome/browser/ui/toolbar/toolbar_model_unittest.cc (left):

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/toolbar_model_unittest.cc:44: // The expected text to
display when the origin chip (URL removal) is active.
These tests still seem useful to me.  Why remove them?

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.h:220: LocationIconView*
location_icon_view() { return location_icon_view_; }
We should also have a const version of this, no?  That's how it used be:

http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/ui/views/locat...

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_icon_view.cc (right):

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_icon_view.cc:57: // location bar
is at the NTP.  Also skip showing the page info if the
Remove "Also skip showing..."

https://codereview.chromium.org/287543002/diff/60001/chrome/common/chrome_swi...
File chrome/common/chrome_switches.cc (left):

https://codereview.chromium.org/287543002/diff/60001/chrome/common/chrome_swi...
chrome/common/chrome_switches.cc:314: const char kDisableOriginChipV2[]         
   = "disable-origin-chip-v2";
It'd be better for opt-in/opt-out groups in the running experiments if we kept
the -v2 portions on the command line flags.  See:

http://cs/google3/googledata/googleclient/chrome/finch/studies/EmbeddedSearch...

[Peter Kasting, 2014-05-14 00:28:10]

New snap up, PTAL.

> You mind also moving chrome/browser/ui/toolbar/origin_chip_info.{h,cc} into
> chrome/browser/ui/omnibox?  I left it in toolbar/ since it was still
supporting
> both but now it mercifully has no more relation to the toolbar.

I think we should actually create a chrome/browser/ui/location_bar/ directory
and put it there.  I filed http://crbug.com/373019 to do this separately.

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:365: { IDS_FLAGS_ORIGIN_CHIP_ON_SRP,
switches::kEnableOriginChipOnSrp, ""}
On 2014/05/13 22:19:33, Justin Donnelly wrote:
> I think dropping the hide-on-input option here could cause a problem.  I'm not
> certain, because I wasn't able to figure out what code governs this, but when
I
> added new options earlier it looked like the position within the
> MULTI_VALUE_TYPE array was being encoded in the user profile.
> 
> You could test this.  If I'm right, then if you select "hide on input" (4th
> position in old array) with an old binary you'll now get "on SRP" (4th
position
> in new array) running a binary with this change.

I tested copying my profile from my Dev channel build -- which has the v2 origin
chip set to "hide on click" -- over to somewhere I then used for a build of this
CL.  If the position were encoded in the profile, I'd have expected to see the
origin chip flag set to "enabled".  Instead, it was set to "default".

I speculate that toggling the about:flags settings just changes the command-line
flags, and Chrome determines the about:flags UI to show by looking at the
current set of command-line flags.

I think this won't lead to meaningful problems.

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/search/se...
File chrome/browser/search/search.h (right):

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/search/se...
chrome/browser/search/search.h:54: ORIGIN_CHIP_UNUSED_PLACEHOLDER,  // In list
to avoid changing fieldtrial vals
On 2014/05/13 22:19:33, Justin Donnelly wrote:
> Per Alexei's comment, I think this is unnecessary.

After some more discussions with David, I think I figured out how to change both
this code and the server configs so that we can eliminate both the placeholder
here and the "v2" from the fieldtrial name, and yet have the server experimental
groups work seamlessly across both Chrome versions.  See cl/66843808 .

Unfortunately, making the experimental groups seamless between Chrome 36 and 37
requires putting "origin_chip:1 origin_chip_v2:1" in the name, and changing the
existing experiment names will mean Finch considers them brand new experiments. 
So the only way to do this without disturbing ongoing experiments is for someone
to create new experiments and then phase out the old ones.

David says that we originally packed fieldtrial info into the experiment names
to work around Finch having no other mechanism to do this; supposedly it's now
been fixed, and we could patch search.cc in some way to make use of this
capability.  It would be nice if someone (read: not me) could figure out what
this change is and make it on Chrome trunk, so we can start using it in the
server-side configs, which ought to make robustness against these sorts of flag
changes easier in the future.

In any case, I'm going ahead and making the client-side changes to use a
different fieldtrial name and eliminate the placeholder values in this CL.

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/search/se...
chrome/browser/search/search.h:192: // Returns a value indicating what event
should trigger hiding the origin chip.
On 2014/05/13 22:19:33, Justin Donnelly wrote:
> "What event should trigger hiding" is no longer accurate.  Suggest "Returns a
> value indicating under what conditions the origin chip should be shown."

Technically, it was already inaccurate before my change, which is why I hadn't
touched it.  But that seems fine.  Done.

> Relatedly, "enabled" and "on SRP" don't seem like the right names.  They're
both
> enabling the origin chip.  I would choose "enabled on all pages" and "enabled
on
> SRP".
> 
> I don't feel strongly about this, however, because it's difficult to say
what's
> next and I don't want to create a bunch of busy work for you here changing the
> names.  But let me know what you think.

I changed the names to be more like "enable origin chip always" and "enable
origin chip on srp".  See what you think.

> Speculating, the next step might be to add an option like showing the URL for
a
> short period on page load or scroll up or whatever.  In which case this value
> might be more like "conditions under which the origin chip and URL are shown."

FWIW, I think we should put those particular controls orthogonal to these
options.  Otherwise, this enum has to combinatorially explode.  And really, the
idea of "what pages show the chip" and "what user actions should affect chip
visibility how" are distinct.

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/ui/toolba...
File chrome/browser/ui/toolbar/toolbar_model_unittest.cc (left):

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/toolbar_model_unittest.cc:44: // The expected text to
display when the origin chip (URL removal) is active.
On 2014/05/13 22:19:33, Justin Donnelly wrote:
> These tests still seem useful to me.  Why remove them?

These tests were only being used with origin chip v1.  They're being removed
here because they can't be triggered now.

I would be fine with the idea of writing up more tests for origin chip v2 if you
think they'd be useful, but I think that can happen separately, and it
presumably would be in a location_bar or omnibox test file instead of
toolbar_model_unittest.cc.  What I'm mostly concerned with here is that we're
not reducing the test coverage of v2.

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_bar_view.h (right):

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_bar_view.h:220: LocationIconView*
location_icon_view() { return location_icon_view_; }
On 2014/05/13 22:19:33, Justin Donnelly wrote:
> We should also have a const version of this, no?

No.  No one calls the const version, so we shouldn't have it; it's dead code.

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/ui/views/...
File chrome/browser/ui/views/location_bar/location_icon_view.cc (right):

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/ui/views/...
chrome/browser/ui/views/location_bar/location_icon_view.cc:57: // location bar
is at the NTP.  Also skip showing the page info if the
On 2014/05/13 22:19:33, Justin Donnelly wrote:
> Remove "Also skip showing..."

I could have sworn I did this already.  Done.

https://codereview.chromium.org/287543002/diff/60001/chrome/common/chrome_swi...
File chrome/common/chrome_switches.cc (left):

https://codereview.chromium.org/287543002/diff/60001/chrome/common/chrome_swi...
chrome/common/chrome_switches.cc:314: const char kDisableOriginChipV2[]         
   = "disable-origin-chip-v2";
On 2014/05/13 22:19:33, Justin Donnelly wrote:
> It'd be better for opt-in/opt-out groups in the running experiments if we kept
> the -v2 portions on the command line flags.

Take a look at my aforementioned internal CL to change the server configs.  I
talked about this a bunch with David to figure out what the changes should be,
so I think things should be OK.

Note, for example, that the experimental groups for beta enable the chip by
using the fieldtrial names rather than the flags.  So all we need the flags for
is to be able to map them backwards onto experimental groups.

[Justin Donnelly, 2014-05-14 16:31:42]

LGTM

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/about_fla...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/about_fla...
chrome/browser/about_flags.cc:365: { IDS_FLAGS_ORIGIN_CHIP_ON_SRP,
switches::kEnableOriginChipOnSrp, ""}
On 2014/05/14 00:28:11, Peter Kasting wrote:
> On 2014/05/13 22:19:33, Justin Donnelly wrote:
> > I think dropping the hide-on-input option here could cause a problem.  I'm
not
> > certain, because I wasn't able to figure out what code governs this, but
when
> I
> > added new options earlier it looked like the position within the
> > MULTI_VALUE_TYPE array was being encoded in the user profile.
> > 
> > You could test this.  If I'm right, then if you select "hide on input" (4th
> > position in old array) with an old binary you'll now get "on SRP" (4th
> position
> > in new array) running a binary with this change.
> 
> I tested copying my profile from my Dev channel build -- which has the v2
origin
> chip set to "hide on click" -- over to somewhere I then used for a build of
this
> CL.  If the position were encoded in the profile, I'd have expected to see the
> origin chip flag set to "enabled".  Instead, it was set to "default".
> 
> I speculate that toggling the about:flags settings just changes the
command-line
> flags, and Chrome determines the about:flags UI to show by looking at the
> current set of command-line flags.
> 
> I think this won't lead to meaningful problems.

Ok, thanks for checking.

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/search/se...
File chrome/browser/search/search.h (right):

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/search/se...
chrome/browser/search/search.h:54: ORIGIN_CHIP_UNUSED_PLACEHOLDER,  // In list
to avoid changing fieldtrial vals
On 2014/05/14 00:28:11, Peter Kasting wrote:
> On 2014/05/13 22:19:33, Justin Donnelly wrote:
> > Per Alexei's comment, I think this is unnecessary.
> 
> After some more discussions with David, I think I figured out how to change
both
> this code and the server configs so that we can eliminate both the placeholder
> here and the "v2" from the fieldtrial name, and yet have the server
experimental
> groups work seamlessly across both Chrome versions.  See cl/66843808 .
> 
> Unfortunately, making the experimental groups seamless between Chrome 36 and
37
> requires putting "origin_chip:1 origin_chip_v2:1" in the name, and changing
the
> existing experiment names will mean Finch considers them brand new
experiments. 
> So the only way to do this without disturbing ongoing experiments is for
someone
> to create new experiments and then phase out the old ones.
> 
> David says that we originally packed fieldtrial info into the experiment names
> to work around Finch having no other mechanism to do this; supposedly it's now
> been fixed, and we could patch search.cc in some way to make use of this
> capability.  It would be nice if someone (read: not me) could figure out what
> this change is and make it on Chrome trunk, so we can start using it in the
> server-side configs, which ought to make robustness against these sorts of
flag
> changes easier in the future.
> 
> In any case, I'm going ahead and making the client-side changes to use a
> different fieldtrial name and eliminate the placeholder values in this CL.

I was concerned that we wouldn't be able to transparently roll the experiments
from m36 to m37 but now that I think about it (triggered by David's comment in
cl/66843808) we'd want to separate them anyway given the visible changes in m37.

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/search/se...
chrome/browser/search/search.h:192: // Returns a value indicating what event
should trigger hiding the origin chip.
> I changed the names to be more like "enable origin chip always" and "enable
> origin chip on srp".  See what you think.

Looks good to me.  But now we'll need to update the Finch configs to reflect the
new name, right?  It's "enable-origin-chip" instead of
"enable-origin-chip-always" in http://cl/66843808.

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/ui/toolba...
File chrome/browser/ui/toolbar/toolbar_model_unittest.cc (left):

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/ui/toolba...
chrome/browser/ui/toolbar/toolbar_model_unittest.cc:44: // The expected text to
display when the origin chip (URL removal) is active.
On 2014/05/14 00:28:11, Peter Kasting wrote:
> On 2014/05/13 22:19:33, Justin Donnelly wrote:
> > These tests still seem useful to me.  Why remove them?
> 
> These tests were only being used with origin chip v1.  They're being removed
> here because they can't be triggered now.

From the point of view of these tests, v1 and v2 have the same effect on the
toolbar model.  Now that "origin_chip" specifies v2, I'm pretty sure you could
restore this file to its previous state and all the tests would pass and provide
at least some coverage of the v2 chip.

[Peter Kasting, 2014-05-14 22:55:33]

+jered for chrome/browser/search/ OWNERS

I re-enabled the toolbar model tests.

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/search/se...
File chrome/browser/search/search.h (right):

https://codereview.chromium.org/287543002/diff/60001/chrome/browser/search/se...
chrome/browser/search/search.h:192: // Returns a value indicating what event
should trigger hiding the origin chip.
On 2014/05/14 16:31:43, Justin Donnelly wrote:
> > I changed the names to be more like "enable origin chip always" and "enable
> > origin chip on srp".  See what you think.
> 
> Looks good to me.  But now we'll need to update the Finch configs to reflect
the
> new name, right?  It's "enable-origin-chip" instead of
> "enable-origin-chip-always" in http://cl/66843808.

Ah, good catch.  Fixed.

[Peter Kasting, 2014-05-17 01:14:00]

Ping: Jered, review?

[Jered, 2014-05-19 16:38:41]

On 2014/05/17 01:14:00, Peter Kasting wrote:
> Ping: Jered, review?

search lgtm

[Peter Kasting, 2014-05-19 17:44:23]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-05-19 17:44:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkasting@chromium.org/287543002/140001

[commit-bot: I haz the power, 2014-05-19 21:02:00]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-19 22:24:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-19 22:24:55]

Try jobs failed on following builders:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[Peter Kasting, 2014-05-20 18:29:52]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-05-20 18:30:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkasting@chromium.org/287543002/140001

[commit-bot: I haz the power, 2014-05-20 21:12:52]

Change committed as 271746