[OriginChip] Add an option to display only on search result pages.

[OriginChip] Add an option to display only on search result pages.

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

[Justin Donnelly, 2014-05-05 19:36:22]

Rachel, Marc-Antoine, can one of you test this on Mac?

[macourteau, 2014-05-05 19:37:42]

On 2014/05/05 19:36:22, Justin Donnelly wrote:
> Rachel, Marc-Antoine, can one of you test this on Mac?

Sure, I'll give it a test.

[macourteau, 2014-05-05 20:14:09]

On 2014/05/05 19:37:42, macourteau wrote:
> On 2014/05/05 19:36:22, Justin Donnelly wrote:
> > Rachel, Marc-Antoine, can one of you test this on Mac?
> 
> Sure, I'll give it a test.

Yep, works as expected on Mac (with search term replacement enabled).

[Justin Donnelly, 2014-05-07 17:28:55]

Hi Peter - did you see this?  If you've been busy, no problem, I just wanted to
make sure you hadn't missed it.

[Peter Kasting, 2014-05-08 20:42:19]

On 2014/05/07 17:28:55, Justin Donnelly wrote:
> Hi Peter - did you see this?  If you've been busy, no problem, I just wanted
to
> make sure you hadn't missed it.

I did miss this, until just now when Ruby IMed me.  Looks like that's because
you didn't actually mark anyone as a reviewer.  Adding myself as one now.

[Peter Kasting, 2014-05-08 21:32:31]

LGTM

https://codereview.chromium.org/266263002/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/266263002/diff/1/chrome/browser/about_flags.c...
chrome/browser/about_flags.cc:369: switches::kEnableOriginChipV2HideOnUserInput,
""},
While I'm here, I notice we still have both options above, even though (AFAIK?)
we implemented the hybrid solution already -- can we just remove one of these
and rename the other "Enabled, shown on all pages" or something?  That would
also make it less ambiguous why we have click/focus options for "all pages" but
not for "SRP only" (even though one of those doesn't really make sense for
SRPs).

(Could be done in a separate CL)

https://codereview.chromium.org/266263002/diff/1/chrome/browser/ui/toolbar/to...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/266263002/diff/1/chrome/browser/ui/toolbar/to...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:187:
chrome::GetOriginChipV2Condition() ==
Nit: Let's factor out some variable called chip_condition or something so we
don't call this multiple times.

Then the end of this function could be broken into a series of shorter
statements:

  if (!delegate_->InTabbedBrowser() || !ShouldDisplayURL() ||
      !url_replacement_enabled())
    return false;

  if (chrome::ShouldDisplayOriginChip())
    return true;

  const chrome::OriginChipV2Condition chip_condition =
      chrome::GetOriginChipV2Condition();
  return (chip_condition != chrome::ORIGIN_CHIP_V2_DISABLED) &&
      ((chip_condition != chrome::ORIGIN_CHIP_V2_ON_SRP) ||
       WouldPerformSearchTermReplacement(false));

[Justin Donnelly, 2014-05-08 22:26:12]

On 2014/05/08 20:42:19, Peter Kasting wrote:
> Looks like that's because you didn't actually mark anyone as a reviewer.

I'm working on a way I can blame this on Reitveld.  I haven't figured it out
yet, but I'll let you know when I do. ;)

[Justin Donnelly, 2014-05-08 22:27:15]

https://codereview.chromium.org/266263002/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/266263002/diff/1/chrome/browser/about_flags.c...
chrome/browser/about_flags.cc:369: switches::kEnableOriginChipV2HideOnUserInput,
""},
On 2014/05/08 21:32:31, Peter Kasting wrote:
> While I'm here, I notice we still have both options above, even though
(AFAIK?)
> we implemented the hybrid solution already -- can we just remove one of these
> and rename the other "Enabled, shown on all pages" or something?  That would
> also make it less ambiguous why we have click/focus options for "all pages"
but
> not for "SRP only" (even though one of those doesn't really make sense for
> SRPs).
> 
> (Could be done in a separate CL)

Getting rid of "HideOnUserInput" makes sense since "HideOnMouseRelease" enables
the hybrid solution. But I don't think I can just eliminate an existing option
here without affecting the choice that's been encoded and written down for users
who've already selected an option (correct me if you think I'm wrong).

I'll look into it some more after submitting this.

https://codereview.chromium.org/266263002/diff/1/chrome/browser/ui/toolbar/to...
File chrome/browser/ui/toolbar/toolbar_model_impl.cc (right):

https://codereview.chromium.org/266263002/diff/1/chrome/browser/ui/toolbar/to...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:187:
chrome::GetOriginChipV2Condition() ==
On 2014/05/08 21:32:31, Peter Kasting wrote:
> Nit: Let's factor out some variable called chip_condition or something so we
> don't call this multiple times.
> 
> Then the end of this function could be broken into a series of shorter
> statements:
> 
>   if (!delegate_->InTabbedBrowser() || !ShouldDisplayURL() ||
>       !url_replacement_enabled())
>     return false;
> 
>   if (chrome::ShouldDisplayOriginChip())
>     return true;
> 
>   const chrome::OriginChipV2Condition chip_condition =
>       chrome::GetOriginChipV2Condition();
>   return (chip_condition != chrome::ORIGIN_CHIP_V2_DISABLED) &&
>       ((chip_condition != chrome::ORIGIN_CHIP_V2_ON_SRP) ||
>        WouldPerformSearchTermReplacement(false));

Done.

[Justin Donnelly, 2014-05-08 22:32:40]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2014-05-08 22:39:26]

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

[Peter Kasting, 2014-05-08 22:39:39]

https://codereview.chromium.org/266263002/diff/1/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/266263002/diff/1/chrome/browser/about_flags.c...
chrome/browser/about_flags.cc:369: switches::kEnableOriginChipV2HideOnUserInput,
""},
On 2014/05/08 22:27:16, Justin Donnelly wrote:
> I don't think I can just eliminate an existing option
> here without affecting the choice that's been encoded and written down for
users
> who've already selected an option (correct me if you think I'm wrong).

You mean, you're worried about people who pick "option 3" having the meaning of
"3" change out from under them?

One way to do this is to just rename the flag/pref so everyone gets reset to the
default.  Another way is to write a migration pass and use a separate bool (or
version number) in the prefs to mark whether it's done -- IMO, more effort than
this is worth.

Incidentally, if instead of mixing all these options together as enumerated
choices for one flag, we had separate bools to toggle e.g. "origin chip
enabled", "always show/shown on SRPs", etc., we could more easily add/remove
choices without mucking up existing ones.

Anyway, if you're going to clean this up in the future, please also consider
completely nuking the "origin chip in toolbar" variant (from everywhere in the
codebase).

[commit-bot: I haz the power, 2014-05-09 03:12:11]

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:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-05-09 03:19:30]

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

[commit-bot: I haz the power, 2014-05-09 03:19:31]

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

[Justin Donnelly, 2014-05-09 14:43:56]

Samarth, can you review the changes in chrome/browser/search?

[samarth, 2014-05-09 17:07:19]

search lgtm

[Justin Donnelly, 2014-05-09 17:07:51]

The CQ bit was checked by jdonnelly@chromium.org

[commit-bot: I haz the power, 2014-05-09 17:10:00]

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

[commit-bot: I haz the power, 2014-05-09 22:50:36]

Change committed as 269446