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

chrome/browser/search/search.h

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_SEARCH_SEARCH_H_
 #define CHROME_BROWSER_SEARCH_SEARCH_H_
 
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 30 matching lines @@
 };
 
 enum DisplaySearchButtonConditions {
   DISPLAY_SEARCH_BUTTON_NEVER,
   DISPLAY_SEARCH_BUTTON_FOR_STR,         // STR = Search Term Replacement
   DISPLAY_SEARCH_BUTTON_FOR_STR_OR_IIP,  // IIP = Input In Progress
   DISPLAY_SEARCH_BUTTON_ALWAYS,
   DISPLAY_SEARCH_BUTTON_NUM_VALUES,
 };
 
-enum OriginChipPosition {
-  ORIGIN_CHIP_DISABLED,
-  ORIGIN_CHIP_LEADING_LOCATION_BAR,
-  ORIGIN_CHIP_TRAILING_LOCATION_BAR,
-  ORIGIN_CHIP_LEADING_MENU_BUTTON,
+enum OriginChipCondition {
+  ORIGIN_CHIP_DISABLED = 0,
+  ORIGIN_CHIP_ENABLED,
+  ORIGIN_CHIP_UNUSED_PLACEHOLDER,  // In list to avoid changing fieldtrial vals

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

Per Alexei's comment, I think this is unnecessary.

[Peter Kasting, 2014/05/14 00:28:11]

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.

[Justin Donnelly, 2014/05/14 16:31:43]

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.

+  ORIGIN_CHIP_ON_SRP,
   ORIGIN_CHIP_NUM_VALUES,
 };
 
-enum OriginChipV2Condition {
-  ORIGIN_CHIP_V2_DISABLED,
-  ORIGIN_CHIP_V2_HIDE_ON_MOUSE_RELEASE,
-  ORIGIN_CHIP_V2_HIDE_ON_USER_INPUT,
-  ORIGIN_CHIP_V2_ON_SRP,
-  ORIGIN_CHIP_V2_NUM_VALUES,
-};
-
 // Use this value for "start margin" to prevent the "es_sm" parameter from
 // being used.
 extern const int kDisableStartMargin;
 
 // Returns whether the Instant Extended API is enabled.
 bool IsInstantExtendedAPIEnabled();
 
 // Returns whether the suggest is enabled for the given |profile|.
 bool IsSuggestPrefEnabled(Profile* profile);
 
@@ skipping 110 matching lines @@
 // to hide the top match in the native suggestions dropdown if it is a verbatim
 // match.  See comments on ShouldHideTopMatch in autocomplete_result.h.
 bool ShouldHideTopVerbatimMatch();
 
 // Returns when we should show a search button in the omnibox.  This may be any
 // of several values, some of which depend on whether the underlying state of
 // the page would normally be to perform search term replacement; see also
 // ToolbarModel::WouldPerformSearchTermReplacement().
 DisplaySearchButtonConditions GetDisplaySearchButtonConditions();
 
-// Returns true if the origin chip should be shown in the toolbar. This
-// also includes the related changes to the omnibox. Always returns false if
-// ShouldDisplayOriginChipV2() returns true.
+// Returns true if the origin chip should be shown.
 bool ShouldDisplayOriginChip();
 
-// Returns a value indicating where the origin chip should be positioned on the
-// toolbar.
-OriginChipPosition GetOriginChipPosition();
-
-// Returns true if version 2 of the origin chip should be shown.  This version
-// places the origin chip inside the location bar instead of the toolbar and
-// adds show/hide behavior and animations to make the relationship between the
-// chip and the text in the Omnibox clearer.
-bool ShouldDisplayOriginChipV2();
-
-// Returns a value indicating what event should trigger hiding the origin chip
-// in the location bar.
-OriginChipV2Condition GetOriginChipV2Condition();
+// Returns a value indicating what event should trigger hiding the origin chip.

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

"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.

[Peter Kasting, 2014/05/14 00:28:11]

Technically, it was already inaccurate before my change, which is why I hadn't
touched it.  But that seems fine.  Done.
I changed the names to be more like "enable origin chip always" and "enable
origin chip on srp".  See what you think.
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.

[Justin Donnelly, 2014/05/14 16:31:43]

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.

[Peter Kasting, 2014/05/14 22:55:34]

Ah, good catch.  Fixed.

+OriginChipCondition GetOriginChipCondition();
 
 // Returns true if the local new tab page should show a Google logo and search
 // box for users whose default search provider is Google, or false if not.
 bool ShouldShowGoogleLocalNTP();
 
 // Transforms the input |url| into its "effective URL". The returned URL
 // facilitates grouping process-per-site. The |url| is transformed, for
 // example, from
 //
 //   https://www.google.com/search?espv=1&q=tractors
@@ skipping 72 matching lines @@
 bool GetBoolValueForFlagWithDefault(const std::string& flag,
                                     bool default_value,
                                     const FieldTrialFlags& flags);
 
 // Returns the Cacheable New Tab Page URL for the given |profile|.
 GURL GetNewTabPageURL(Profile* profile);
 
 }  // namespace chrome
 
 #endif  // CHROME_BROWSER_SEARCH_SEARCH_H_