TEST ONLY - DO NOT SUBMIT - FOR TRYBOTS

TEST ONLY - DO NOT SUBMIT - FOR TRYBOTS

This combines two CLs so the trybots can run them together.

BUG=

[Justin Donnelly, 2014-12-03 16:07:24]

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

[Justin Donnelly, 2014-12-03 16:07:25]

https://codereview.chromium.org/768413003/diff/1/chrome/browser/ui/cocoa/loca...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (left):

https://codereview.chromium.org/768413003/diff/1/chrome/browser/ui/cocoa/loca...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:562: const bool
meets_conditions =
I'm really sad to see these features ripped out. But on the other hand, I'm
thrilled to see some of the boolean gymnastics like this statement eliminated.

https://codereview.chromium.org/768413003/diff/1/chrome/browser/ui/cocoa/omni...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/768413003/diff/1/chrome/browser/ui/cocoa/omni...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:241: if
(OmniboxFieldTrial::DisplayHintTextWhenPossible()) {
It's probably worth double-checking that this field trial still works after your
change. I can't see how it wouldn't, the logic is trivial. But still.

https://codereview.chromium.org/768413003/diff/1/chrome/browser/ui/omnibox/om...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (left):

https://codereview.chromium.org/768413003/diff/1/chrome/browser/ui/omnibox/om...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:257: bool
url_replacement_enabled = !state || state->url_replacement_enabled;
One of the reasons I thought it would be helpful to combine the two changes is
that with both features gone, I'm pretty sure we don't need
url_replacement_enabled and all it's associated logic anymore. If you want to
keep the two changes separate and remove u_r_e in a third, that's fine, just
FYI.

Update: on second thought, possibly not. Is this still used for search term
replacement?

https://codereview.chromium.org/768413003/diff/1/chrome/browser/ui/toolbar/to...
File chrome/browser/ui/toolbar/toolbar_model_delegate.h (left):

https://codereview.chromium.org/768413003/diff/1/chrome/browser/ui/toolbar/to...
chrome/browser/ui/toolbar/toolbar_model_delegate.h:19: virtual bool
InTabbedBrowser() const = 0;
Looks like you're missing the corresponding change to toolbar_model_delegate.cc.

[Peter Kasting, 2014-12-04 00:23:18]

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

[Peter Kasting, 2014-12-04 00:23:19]

I haven't really reviewed, but here's a comment suggesting you do more work!  :D

https://codereview.chromium.org/768413003/diff/1/chrome/browser/ui/omnibox/om...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (left):

https://codereview.chromium.org/768413003/diff/1/chrome/browser/ui/omnibox/om...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:257: bool
url_replacement_enabled = !state || state->url_replacement_enabled;
On 2014/12/03 16:07:25, Justin Donnelly wrote:
> One of the reasons I thought it would be helpful to combine the two changes is
> that with both features gone, I'm pretty sure we don't need
> url_replacement_enabled and all it's associated logic anymore. If you want to
> keep the two changes separate and remove u_r_e in a third, that's fine, just
> FYI.
> 
> Update: on second thought, possibly not. Is this still used for search term
> replacement?

Yes, but given that the origin chip and search button were the mechanisms
proposed for enabling search term replacement, that's dead as well.  Unless
someone has a proposition for how to address the negative data we saw with it? 
I think this patch leaves the user without any flags to enable the feature, so I
wouldn't leave any of its plumbing either, unless we have some sort of set of
experiments in the pipe.

It makes me kind of sad, because search term replacement is something I
personally thought was interesting and viable, but AFAIK we haven't come up with
a way to do it that doesn't seem to measure as clearly bad.

I might check with dcblack to be sure that he agrees that this is dead.  If so,
I'd remove it as well -- doesn't matter what the combination of CLs is. 
Removing the search term replacement functionality entirely would probably allow
some API simplifications (assuming it amounts to reverting a lot of the changes
I originally did to make the underlying model amenable to search term
replacement; it's possible that some of those were good refactoring that should
stick around, though, and I'd have to peer through the original CLs to know).

[Justin Donnelly, 2014-12-04 00:54:43]

https://codereview.chromium.org/768413003/diff/1/chrome/browser/ui/omnibox/om...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (left):

https://codereview.chromium.org/768413003/diff/1/chrome/browser/ui/omnibox/om...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:257: bool
url_replacement_enabled = !state || state->url_replacement_enabled;
On 2014/12/04 00:23:19, Peter Kasting wrote:
> On 2014/12/03 16:07:25, Justin Donnelly wrote:
> > One of the reasons I thought it would be helpful to combine the two changes
is
> > that with both features gone, I'm pretty sure we don't need
> > url_replacement_enabled and all it's associated logic anymore. If you want
to
> > keep the two changes separate and remove u_r_e in a third, that's fine, just
> > FYI.
> > 
> > Update: on second thought, possibly not. Is this still used for search term
> > replacement?
> 
> Yes, but given that the origin chip and search button were the mechanisms
> proposed for enabling search term replacement, that's dead as well.  Unless
> someone has a proposition for how to address the negative data we saw with it?

> I think this patch leaves the user without any flags to enable the feature, so
I
> wouldn't leave any of its plumbing either, unless we have some sort of set of
> experiments in the pipe.
> 
> It makes me kind of sad, because search term replacement is something I
> personally thought was interesting and viable, but AFAIK we haven't come up
with
> a way to do it that doesn't seem to measure as clearly bad.
> 
> I might check with dcblack to be sure that he agrees that this is dead.  If
so,
> I'd remove it as well -- doesn't matter what the combination of CLs is. 
> Removing the search term replacement functionality entirely would probably
allow
> some API simplifications (assuming it amounts to reverting a lot of the
changes
> I originally did to make the underlying model amenable to search term
> replacement; it's possible that some of those were good refactoring that
should
> stick around, though, and I'd have to peer through the original CLs to know).

I think search term replacement is actually launched on mobile. If not, there's
some very similar type of behavior on Android.

[Peter Kasting, 2014-12-04 01:44:31]

https://codereview.chromium.org/768413003/diff/1/chrome/browser/ui/omnibox/om...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (left):

https://codereview.chromium.org/768413003/diff/1/chrome/browser/ui/omnibox/om...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:257: bool
url_replacement_enabled = !state || state->url_replacement_enabled;
On 2014/12/04 00:54:43, Justin Donnelly wrote:
> On 2014/12/04 00:23:19, Peter Kasting wrote:
> > On 2014/12/03 16:07:25, Justin Donnelly wrote:
> > > One of the reasons I thought it would be helpful to combine the two
changes
> is
> > > that with both features gone, I'm pretty sure we don't need
> > > url_replacement_enabled and all it's associated logic anymore. If you want
> to
> > > keep the two changes separate and remove u_r_e in a third, that's fine,
just
> > > FYI.
> > > 
> > > Update: on second thought, possibly not. Is this still used for search
term
> > > replacement?
> > 
> > Yes, but given that the origin chip and search button were the mechanisms
> > proposed for enabling search term replacement, that's dead as well.  Unless
> > someone has a proposition for how to address the negative data we saw with
it?
> 
> > I think this patch leaves the user without any flags to enable the feature,
so
> I
> > wouldn't leave any of its plumbing either, unless we have some sort of set
of
> > experiments in the pipe.
> > 
> > It makes me kind of sad, because search term replacement is something I
> > personally thought was interesting and viable, but AFAIK we haven't come up
> with
> > a way to do it that doesn't seem to measure as clearly bad.
> > 
> > I might check with dcblack to be sure that he agrees that this is dead.  If
> so,
> > I'd remove it as well -- doesn't matter what the combination of CLs is. 
> > Removing the search term replacement functionality entirely would probably
> allow
> > some API simplifications (assuming it amounts to reverting a lot of the
> changes
> > I originally did to make the underlying model amenable to search term
> > replacement; it's possible that some of those were good refactoring that
> should
> > stick around, though, and I'd have to peer through the original CLs to
know).
> 
> I think search term replacement is actually launched on mobile. If not,
there's
> some very similar type of behavior on Android.

I would be very surprised if this code is used on mobile -- AFAIK they have a
completely different omnibox implementation, and what with the various projects
going on internally (codenames and descriptions redacted here), that looks
likely to become more diverged rather than less.

Worth checking, though, so good to raise it.

[Justin Donnelly, 2014-12-17 19:09:13]

On 2014/12/04 01:44:31, Peter Kasting wrote:
> I would be very surprised if this code is used on mobile -- AFAIK they have a
> completely different omnibox implementation, and what with the various
projects
> going on internally (codenames and descriptions redacted here), that looks
> likely to become more diverged rather than less.
> 
> Worth checking, though, so good to raise it.

Yes, you're right. The query replacement in Clank is handled in
com/google/android/apps/chrome/omnibox/LocationBar.java and friends.

[Peter Kasting, 2015-01-09 09:42:13]

I was surprised to find this hadn't actually landed yet.  :(  Just hoping it
doesn't die.

[groby-ooo-7-16, 2015-01-09 18:49:09]

On 2015/01/09 09:42:13, Peter Kasting wrote:
> I was surprised to find this hadn't actually landed yet.  :(  Just hoping it
> doesn't die.

It's not dying. I was just on vacation :)

I'll address most comments this afternoon, with the exception of your "please do
more work" comment. Would you mind if that was a separate CL? (I don't mind
doing it, but it's a background task - it's easier to finish if I split it into
separate CLs)

[Peter Kasting, 2015-01-09 21:54:20]

On 2015/01/09 18:49:09, groby wrote:
> On 2015/01/09 09:42:13, Peter Kasting wrote:
> > I was surprised to find this hadn't actually landed yet.  :(  Just hoping it
> > doesn't die.
> 
> It's not dying. I was just on vacation :)
> 
> I'll address most comments this afternoon, with the exception of your "please
do
> more work" comment. Would you mind if that was a separate CL? (I don't mind
> doing it, but it's a background task - it's easier to finish if I split it
into
> separate CLs)

Yeah, that's fine.

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

Peter: In that case, it's just waiting for your LG on
http://crrev.com/749693005/ and http://crrev.com/731423002


(If you'd rather LG here, I'll TBR Justin who has approved the individual CLs
already)

https://codereview.chromium.org/768413003/diff/1/chrome/browser/ui/cocoa/loca...
File chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm (left):

https://codereview.chromium.org/768413003/diff/1/chrome/browser/ui/cocoa/loca...
chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm:562: const bool
meets_conditions =
On 2014/12/03 16:07:25, Justin Donnelly wrote:
> I'm really sad to see these features ripped out. But on the other hand, I'm
> thrilled to see some of the boolean gymnastics like this statement eliminated.

Acknowledged.

https://codereview.chromium.org/768413003/diff/1/chrome/browser/ui/cocoa/omni...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (right):

https://codereview.chromium.org/768413003/diff/1/chrome/browser/ui/cocoa/omni...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:241: if
(OmniboxFieldTrial::DisplayHintTextWhenPossible()) {
On 2014/12/03 16:07:25, Justin Donnelly wrote:
> It's probably worth double-checking that this field trial still works after
your
> change. I can't see how it wouldn't, the logic is trivial. But still.

DisplayHintText was introduced as part of origin chip, and is, AFAICT, not used
by anything else. I'll remove hint text as well, but if it's OK with you, a
separate CL after this landed. This is getting large :)

Filed as http://crbug.com/447809

https://codereview.chromium.org/768413003/diff/1/chrome/browser/ui/omnibox/om...
File chrome/browser/ui/omnibox/omnibox_edit_model.cc (left):

https://codereview.chromium.org/768413003/diff/1/chrome/browser/ui/omnibox/om...
chrome/browser/ui/omnibox/omnibox_edit_model.cc:257: bool
url_replacement_enabled = !state || state->url_replacement_enabled;
On 2014/12/04 01:44:31, Peter Kasting wrote:
> On 2014/12/04 00:54:43, Justin Donnelly wrote:
> > On 2014/12/04 00:23:19, Peter Kasting wrote:
> > > On 2014/12/03 16:07:25, Justin Donnelly wrote:
> > > > One of the reasons I thought it would be helpful to combine the two
> changes
> > is
> > > > that with both features gone, I'm pretty sure we don't need
> > > > url_replacement_enabled and all it's associated logic anymore. If you
want
> > to
> > > > keep the two changes separate and remove u_r_e in a third, that's fine,
> just
> > > > FYI.
> > > > 
> > > > Update: on second thought, possibly not. Is this still used for search
> term
> > > > replacement?
> > > 
> > > Yes, but given that the origin chip and search button were the mechanisms
> > > proposed for enabling search term replacement, that's dead as well. 
Unless
> > > someone has a proposition for how to address the negative data we saw with
> it?
> > 
> > > I think this patch leaves the user without any flags to enable the
feature,
> so
> > I
> > > wouldn't leave any of its plumbing either, unless we have some sort of set
> of
> > > experiments in the pipe.
> > > 
> > > It makes me kind of sad, because search term replacement is something I
> > > personally thought was interesting and viable, but AFAIK we haven't come
up
> > with
> > > a way to do it that doesn't seem to measure as clearly bad.
> > > 
> > > I might check with dcblack to be sure that he agrees that this is dead. 
If
> > so,
> > > I'd remove it as well -- doesn't matter what the combination of CLs is. 
> > > Removing the search term replacement functionality entirely would probably
> > allow
> > > some API simplifications (assuming it amounts to reverting a lot of the
> > changes
> > > I originally did to make the underlying model amenable to search term
> > > replacement; it's possible that some of those were good refactoring that
> > should
> > > stick around, though, and I'd have to peer through the original CLs to
> know).
> > 
> > I think search term replacement is actually launched on mobile. If not,
> there's
> > some very similar type of behavior on Android.
> 
> I would be very surprised if this code is used on mobile -- AFAIK they have a
> completely different omnibox implementation, and what with the various
projects
> going on internally (codenames and descriptions redacted here), that looks
> likely to become more diverged rather than less.
> 
> Worth checking, though, so good to raise it.

Filed as http://crbug.com/447810

https://codereview.chromium.org/768413003/diff/1/chrome/browser/ui/toolbar/to...
File chrome/browser/ui/toolbar/toolbar_model_delegate.h (left):

https://codereview.chromium.org/768413003/diff/1/chrome/browser/ui/toolbar/to...
chrome/browser/ui/toolbar/toolbar_model_delegate.h:19: virtual bool
InTabbedBrowser() const = 0;
On 2014/12/03 16:07:25, Justin Donnelly wrote:
> Looks like you're missing the corresponding change to
toolbar_model_delegate.cc.

There is no toolbar_model_delegate.cc - this is a pure interface :)

[Peter Kasting, 2015-01-10 03:54:28]

On 2015/01/10 00:54:06, groby wrote:
> Peter: In that case, it's just waiting for your LG on
> http://crrev.com/749693005/ and http://crrev.com/731423002
> 
> 
> (If you'd rather LG here, I'll TBR Justin who has approved the individual CLs
> already)

I think the part of this that's reviewable is finding bits that were supposed to
be removed but weren't.  Since this intentionally doesn't remove everything, I'm
not going to have any meaningful comments.

Rubber-stamp LGTM

[Justin Donnelly, 2015-01-12 15:56:21]

On 2015/01/10 00:54:06, groby wrote:
> There is no toolbar_model_delegate.cc - this is a pure interface :)

There was a file with this name at some point. If not here than maybe in another
directory as an implementation on some particular platform or for tests or
something, I can't recall what. But whatever it was is gone now so nevermind. :)

[Peter Kasting, 2015-02-12 00:58:10]

This still hasn't landed.  How come?

[groby-ooo-7-16, 2015-02-12 03:01:29]

Thanks for pinging.

This got stalled because 

a) There was more review work going on in the part changelists
http://crrev.com/731423002
http://crrev.com/749693005

b) Because I got sidetracked and let this linger - my apologies.

[Peter Kasting, 2015-02-20 01:17:18]

The individual pieces have landed.  Closing.