Remove OriginChip code.

Remove OriginChip code.

Alas, the time has come to say goodbye to our old friend, the origin chip. He
meant well, and often he did well, but he could not overcome the obstacles in
front of him.

And so, with this change, we reserve his comforting space in changelog heaven.

And get some memory back in exchange.

BUG=424751
Committed: https://crrev.com/95e99d3247de8e0f9e5d5dc9718a98a562f24c7b
Cr-Commit-Position: refs/heads/master@{#316179}

[groby-ooo-7-16, 2014-11-18 01:28:43]

groby@chromium.org changed reviewers:
+ jdonnelly@chromium.org

[groby-ooo-7-16, 2014-11-18 01:33:50]

We deeply regret to inform you that this constitutes the first attempt to put
the OriginChip into a shallow grave.

* For now, Views only - I'll tackle Cocoa in a follow-up upload.

* For now, not CC'ed Peter. I want to make somewhat sure I got everything before
I present him with a CL of this size.

Would appreciate if you gave this a sanity check.

[tfarina, 2014-11-18 15:40:55]

Is there a crbug.com/ associated/filed to track this?

[groby-ooo-7-16, 2014-11-18 18:10:31]

No, there is not.

On Tue, Nov 18, 2014 at 7:40 AM, <tfarina@chromium.org> wrote:

> Is there a crbug.com/ associated/filed to track this?
>
> https://codereview.chromium.org/731423002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Justin Donnelly, 2014-11-18 18:52:34]

I'll take a look at this soon.  Isn't https://crbug.com/424751 meant for
this purpose?

On Tue, Nov 18, 2014 at 10:10 AM, Rachel Blum <groby@chromium.org> wrote:

> No, there is not.
>
> On Tue, Nov 18, 2014 at 7:40 AM, <tfarina@chromium.org> wrote:
>
>> Is there a crbug.com/ associated/filed to track this?
>>
>> https://codereview.chromium.org/731423002/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[groby-ooo-7-16, 2014-11-18 20:44:18]

Indeed, and thank you! Missed that when setting this up, updated CL description.

[Justin Donnelly, 2014-11-21 19:15:25]

Sorry, traveling makes my brain stop working and I forgot about this.

Looks good in general but I have one big proposal.  The search button is also
dead.  In general I favor CLs as small as possible but I think overall we'd face
less work removing the chip and the button together.  The logic for URL hiding
is so finicky that I'll bet $1 that no matter how much effort we put into this
CL it'll introduce at least one bug. Then we'll fix those, do the search button
CL and introduce yet more bugs to chase down.  I think the band-aid-rip approach
would be better here - do it all at one go.

Regardless of what you think about that, here's a few other things we need to
deal with:

- We should clean out the experiments first. I can do this Monday unless mucking
with Finch configs sounds fun to you.
- This could be done in a separate CL, but there are a bunch of origin chip
image assets we need to delete at
chrome/app/theme/default_{100,200}_percent/common/chip_*.png.
- There are some changes to LocationIconView that you'll need to make.  (See
https://codereview.chromium.org/197623002/.)

https://codereview.chromium.org/731423002/diff/1/chrome/browser/ui/omnibox/om...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

https://codereview.chromium.org/731423002/diff/1/chrome/browser/ui/omnibox/om...
chrome/browser/ui/omnibox/omnibox_view.h:60: // Updates the placeholder text
with the value of GetHintText() if possible.
To avoid ambiguity, suggest replacing "if possible" with "if the show hint text
field trial is enabled."

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

https://codereview.chromium.org/731423002/diff/1/chrome/browser/ui/toolbar/to...
chrome/browser/ui/toolbar/toolbar_model_impl.cc:337: if
(!delegate_->InTabbedBrowser() || !ShouldDisplayURL() ||
This is the only place InTabbedBrowser is called. It should be removed from
ToolbarModelDelegate and its browser, Android and CrOS subclasses.

https://codereview.chromium.org/731423002/diff/1/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/731423002/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:1007: if (select_all)
If we get rid of the search button then these few lines can revert to their
original state, which is just the one line in the else clause.

See
https://codereview.chromium.org/111873004/diff/40001/chrome/browser/ui/views/...
for the original change.

https://codereview.chromium.org/731423002/diff/1/chrome/browser/ui/views/loca...
File chrome/browser/ui/views/location_bar/location_bar_view.h (left):

https://codereview.chromium.org/731423002/diff/1/chrome/browser/ui/views/loca...
chrome/browser/ui/views/location_bar/location_bar_view.h:541: int
starting_omnibox_leading_inset_;
These properties are no longer used.

[groby-ooo-7-16, 2014-11-21 21:41:38]

groby@chromium.org changed reviewers:
+ pkasting@chromium.org

[groby-ooo-7-16, 2014-11-21 21:45:17]

Added pkasting@ - since he'll be reviewing the whole mess :)

Peter, no review needed yet. But would you prefer removing search button as well
in one giant CL, or rather two separate CLs? Justin makes a good case for the
combo CL, but it's going to be a large-ish delta.

[Peter Kasting, 2014-11-21 21:49:03]

On 2014/11/21 21:45:17, groby wrote:
> Added pkasting@ - since he'll be reviewing the whole mess :)
> 
> Peter, no review needed yet. But would you prefer removing search button as
well
> in one giant CL, or rather two separate CLs? Justin makes a good case for the
> combo CL, but it's going to be a large-ish delta.

Doesn't matter to me.  If doing it in one CL is easy, do it.

[Justin Donnelly, 2014-11-24 22:56:06]

On 2014/11/21 19:15:25, Justin Donnelly wrote:
> - We should clean out the experiments first. I can do this Monday unless
mucking
> with Finch configs sounds fun to you.

FYI: https://critique.corp.google.com/#review/80667258

[groby-ooo-7-16, 2014-12-01 20:35:32]

Rebase to HEAD

[groby-ooo-7-16, 2014-12-01 21:01:40]

Disable unneeded test.

[groby-ooo-7-16, 2014-12-01 23:02:56]

Moved the asset removal into a separate CL, 

Why? Because crbug.com/340167 doesn't let me download any changes with binaries
on another machine, and I'm trying this on both OSX and Linux.

[groby-ooo-7-16, 2014-12-01 23:03:12]

Asset CL: https://codereview.chromium.org/757293003/

[groby-ooo-7-16, 2014-12-02 18:40:10]

LocationBarIcon has already been addressed by Peter in a previous CL. 

Search button removal to follow ASAP in a separate CL, but feel free to start
reviewing this CL already.

https://codereview.chromium.org/731423002/diff/1/chrome/browser/ui/omnibox/om...
File chrome/browser/ui/omnibox/omnibox_view.h (right):

https://codereview.chromium.org/731423002/diff/1/chrome/browser/ui/omnibox/om...
chrome/browser/ui/omnibox/omnibox_view.h:60: // Updates the placeholder text
with the value of GetHintText() if possible.
On 2014/11/21 19:15:24, Justin Donnelly wrote:
> To avoid ambiguity, suggest replacing "if possible" with "if the show hint
text
> field trial is enabled."

Slightly different wording, omitting "field trial", since that's an impl detail.

https://codereview.chromium.org/731423002/diff/1/chrome/browser/ui/views/fram...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/731423002/diff/1/chrome/browser/ui/views/fram...
chrome/browser/ui/views/frame/browser_view.cc:1007: if (select_all)
On 2014/11/21 19:15:24, Justin Donnelly wrote:
> If we get rid of the search button then these few lines can revert to their
> original state, which is just the one line in the else clause.
> 
> See
>
https://codereview.chromium.org/111873004/diff/40001/chrome/browser/ui/views/...
> for the original change.

We can get rid of them even without the search button, since it only depends on
WouldOmitDueToOriginChip.

Either way, I'll have a pipelined CL for the search button - I'm happy to submit
both combined into one, once they're reviewed, but I figured I make it a tad
easier on the reviewers :)

[groby-ooo-7-16, 2014-12-02 22:42:19]

If you'd rather review a single CL for OriginChip and SearchButton:
https://codereview.chromium.org/768413003/

[Justin Donnelly, 2014-12-03 16:08:51]

LGTM

Made some comments on the merged CL, but in general lgtm.

[Justin Donnelly, 2014-12-03 16:08:52]

LGTM

Made some comments on the merged CL, but in general lgtm.

[groby-ooo-7-16, 2015-01-10 01:40:06]

groby@chromium.org changed reviewers:
+ asvitkine@chromium.org, stevenjb@chromium.org

[groby-ooo-7-16, 2015-01-10 01:41:35]

groby@chromium.org changed reviewers:
+ kmadhusu@chromium.org

[groby-ooo-7-16, 2015-01-10 01:41:36]

kmadhusu: Please review changes in c/b/search
stevenb: Please review changes in c/b/chromeos
asvitkine: Please review changes in tools/metrics

[Alexei Svitkine (slow), 2015-01-12 16:15:21]

https://codereview.chromium.org/731423002/diff/270001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/731423002/diff/270001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:22710: - 
<owner>gbillock@chromium.org</owner>
Instead of deleting the histogram and action entry, mark them each with an
<obsolete> tag. (See examples for other ones within the files.)

https://codereview.chromium.org/731423002/diff/270001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:50071: -  <int value="-1392562498"
label="disable-origin-chip"/>
Keep these here as they are needed to decode data from old versions.

[stevenjb, 2015-01-12 16:49:28]

c/b/chromeos lgtm

[kmadhusu, 2015-01-12 17:52:32]

c/b/search lgtm

[groby-ooo-7-16, 2015-02-12 03:10:39]

asvitkine: PTAL tools/metrics
pkasting: This is the first of the remove OriginChip CLs. Please transfer LG
from original combined CL. (The combined one is just a merge, but feel free to
re-review ;)

Other reviewers: Only a merge to HEAD in your files. (Feel free to re-check if
you want to validate merge)

https://codereview.chromium.org/731423002/diff/270001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/731423002/diff/270001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:22710: - 
<owner>gbillock@chromium.org</owner>
On 2015/01/12 16:15:21, Alexei Svitkine wrote:
> Instead of deleting the histogram and action entry, mark them each with an
> <obsolete> tag. (See examples for other ones within the files.)

Done.

https://codereview.chromium.org/731423002/diff/270001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:50071: -  <int value="-1392562498"
label="disable-origin-chip"/>
On 2015/01/12 16:15:21, Alexei Svitkine wrote:
> Keep these here as they are needed to decode data from old versions.

Done.

[Peter Kasting, 2015-02-12 07:06:43]

Looks like the compile is broken.

Rubber-stamp LGTM once you fix the trybots.

[Alexei Svitkine (slow), 2015-02-12 13:39:12]

https://codereview.chromium.org/731423002/diff/310001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (left):

https://codereview.chromium.org/731423002/diff/310001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:9627: <action name="OriginChipPress">
Please mark it as <obsolete> instead.

[groby-ooo-7-16, 2015-02-12 23:35:23]

New patchsets have been uploaded after l-g-t-m from pkasting@chromium.org

[groby-ooo-7-16, 2015-02-13 02:50:31]

asvitkine: PTAL

https://codereview.chromium.org/731423002/diff/310001/tools/metrics/actions/a...
File tools/metrics/actions/actions.xml (left):

https://codereview.chromium.org/731423002/diff/310001/tools/metrics/actions/a...
tools/metrics/actions/actions.xml:9627: <action name="OriginChipPress">
On 2015/02/12 13:39:12, Alexei Svitkine wrote:
> Please mark it as <obsolete> instead.

Done.

[Alexei Svitkine (slow), 2015-02-13 06:13:47]

lgtm

[groby-ooo-7-16, 2015-02-13 06:38:40]

The CQ bit was checked by groby@chromium.org

[commit-bot: I haz the power, 2015-02-13 06:39:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/731423002/350001

[commit-bot: I haz the power, 2015-02-13 06:43:50]

Committed patchset #19 (id:350001)

[commit-bot: I haz the power, 2015-02-13 06:44:17]

Patchset 19 (id:??) landed as
https://crrev.com/95e99d3247de8e0f9e5d5dc9718a98a562f24c7b
Cr-Commit-Position: refs/heads/master@{#316179}