Remove search::GetSearchTerms since it always returns empty string

Remove search::GetSearchTerms since it always returns empty string
and update call sites accordingly.

This is the first of probably many follow-ups to https://codereview.chromium.org/2211983002/

TBRing a comment-only change in components/sessions/core/serialized_navigation_entry.cc
TBR=marja@chromium.org

BUG=627747
Committed: https://crrev.com/de23c50488c2558ba2633c0e4a2b09924bdd3704
Cr-Commit-Position: refs/heads/master@{#412473}

[Marc Treib, 2016-08-12 10:27:41]

Description was changed from

==========
Remove search::GetSearchTerms since it always returns empty string
and update call sites accordingly.

BUG=627747
==========

to

==========
Remove search::GetSearchTerms since it always returns empty string
and update call sites accordingly.

This is the first of probably many follow-ups to
https://codereview.chromium.org/2211983002/

BUG=627747
==========

[Marc Treib, 2016-08-12 10:29:58]

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

[Marc Treib, 2016-08-12 10:29:59]

Next part. PTAL!

[Peter Kasting, 2016-08-13 05:03:45]

LGTM

Most (though not all) of the comments below about additional items for removal
could be addressed in separate patches.  Just make sure whatever route you go
explicitly puts these all on the list -- the plumbing for search term
replacement, the origin chip, and all these related bits is huge and sprawling
and I'm trying to make sure we don't let anything slip through the cracks.

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/search/s...
File chrome/browser/search/search.cc (left):

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/search/s...
chrome/browser/search/search.cc:328:
GetInstantSupportStateFromNavigationEntry(*entry);
This was the only caller of this function, so it should be removed too.  In turn
that also implies removal of a bunch of related stuff (setter, conversion to
string, a key constant, maybe others).

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/search/s...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/search/s...
chrome/browser/search/search.cc:298: bool
IsQueryExtractionAllowedForURL(Profile* profile, const GURL& url) {
Eventually you'll want to rip out this, IsSuitableURLForInstant(), and every
other function that mentions "QueryExtraction", "SearchTermsReplacement", or the
like.

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/search/s...
File chrome/browser/search/search.h (right):

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/search/s...
chrome/browser/search/search.h:43: // support state of the page and does not
check for a privileged process.
I suspect you don't even need the second sentence, since those concepts aren't
really meaningful anymore for anything to have distinct behavior.

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(left):

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:314:
l10n_util::GetNSStringWithFixup(IDS_SHOW_URL_MAC);
This was the only use of this string, so it should be removed.

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest_helper.h
(right):

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest_helper.h:43:
MOCK_METHOD0(ShowURL, void());
You need to remove these two methods

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (left):

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:985: OmniboxView::ShowURL();
Remember to eventually rip out the other call chain that leads to this function
(which I think goes through LocationBarView::ShowURL()), and then rip out the
function itself.

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/ui/searc...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/ui/searc...
chrome/browser/ui/search/instant_controller.cc:38: // TODO(treib): Is it safe to
completely remove this? crbug.com/627747
Probably?  Not sure, you'd need to check with kmadhusu or someone else more
knowledgeable.  But at a glance it looks as if this is only really needed to
expose the search terms, which I think you're ripping out the readers of.

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_tab_helper.cc (left):

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_tab_helper.cc:481: origin =
SearchMode::ORIGIN_SEARCH;
This is the only non-unittest location that sets this mode or this origin, so
these values should be removed, along with anything that references them.

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:701: // When the user has
clicked and released to give us focus, select all.
Nit: Move this comment above the line above, and then combine the nested
conditionals.

https://codereview.chromium.org/2232863002/diff/20001/components/omnibox/brow...
File components/omnibox/browser/omnibox_edit_model.cc (left):

https://codereview.chromium.org/2232863002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:1513: return
OmniboxEventProto::SEARCH_RESULT_PAGE_DOING_SEARCH_TERM_REPLACEMENT;
This value should be changed to OBSOLETE in the proto file, and removed from
unittests.

https://codereview.chromium.org/2232863002/diff/20001/components/omnibox/brow...
File components/omnibox/browser/omnibox_edit_model.cc (right):

https://codereview.chromium.org/2232863002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:107: transition,
ui::PAGE_TRANSITION_TYPED)) {
You should check with mpearson as to whether we want to keep this histogram at
all.  It seems like we'd either want to rip it out entirely or else
update/rename it since we're ripping out half the logging, although in fairness
that code hasn't run for a while anyway so I guess this doesn't really change
things materially.

https://codereview.chromium.org/2232863002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:349: // If current text is not
composed of replaced search terms and
Nit: Remove everything in this line but the first word.

https://codereview.chromium.org/2232863002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:372: if ((sel_min != 0))
Nit: Extra ()

https://codereview.chromium.org/2232863002/diff/20001/components/toolbar/tool...
File components/toolbar/toolbar_model.h (right):

https://codereview.chromium.org/2232863002/diff/20001/components/toolbar/tool...
components/toolbar/toolbar_model.h:78: // TODO(treib,pkasting): Can this be
removed? crbug.com/627747
Yes, it can.

https://codereview.chromium.org/2232863002/diff/20001/components/toolbar/tool...
File components/toolbar/toolbar_model_impl.cc (left):

https://codereview.chromium.org/2232863002/diff/20001/components/toolbar/tool...
components/toolbar/toolbar_model_impl.cc:40: return search_terms;
This removal probably means we should rip out GetText() entirely and just
replace calls to it with calls to GetFormattedURL().

[Marc Treib, 2016-08-16 12:00:18]

Thanks, lots of useful comments! All either addressed or TODO'd.

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/search/s...
File chrome/browser/search/search.cc (left):

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/search/s...
chrome/browser/search/search.cc:328:
GetInstantSupportStateFromNavigationEntry(*entry);
On 2016/08/13 05:03:44, Peter Kasting wrote:
> This was the only caller of this function, so it should be removed too.  In
turn
> that also implies removal of a bunch of related stuff (setter, conversion to
> string, a key constant, maybe others).

Thanks! Done.

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/search/s...
File chrome/browser/search/search.cc (right):

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/search/s...
chrome/browser/search/search.cc:298: bool
IsQueryExtractionAllowedForURL(Profile* profile, const GURL& url) {
On 2016/08/13 05:03:44, Peter Kasting wrote:
> Eventually you'll want to rip out this, IsSuitableURLForInstant(), and every
> other function that mentions "QueryExtraction", "SearchTermsReplacement", or
the
> like.

Yup, probably. But as of now, it's still used in a bunch of places, and I'm
trying to be careful so that I don't break anything in the process

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/search/s...
File chrome/browser/search/search.h (right):

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/search/s...
chrome/browser/search/search.h:43: // support state of the page and does not
check for a privileged process.
On 2016/08/13 05:03:44, Peter Kasting wrote:
> I suspect you don't even need the second sentence, since those concepts aren't
> really meaningful anymore for anything to have distinct behavior.

Fair enough, removed.

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm
(left):

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_editor.mm:314:
l10n_util::GetNSStringWithFixup(IDS_SHOW_URL_MAC);
On 2016/08/13 05:03:44, Peter Kasting wrote:
> This was the only use of this string, so it should be removed.

Done. (I have a follow-up CL almost ready that'll remove the corresponding
non-MAC stuff too.)

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest_helper.h
(right):

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/location_bar/autocomplete_text_field_unittest_helper.h:43:
MOCK_METHOD0(ShowURL, void());
On 2016/08/13 05:03:44, Peter Kasting wrote:
> You need to remove these two methods

Done.

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/ui/cocoa...
File chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm (left):

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm:985: OmniboxView::ShowURL();
On 2016/08/13 05:03:44, Peter Kasting wrote:
> Remember to eventually rip out the other call chain that leads to this
function
> (which I think goes through LocationBarView::ShowURL()), and then rip out the
> function itself.

Yup, follow-up on the way!

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/ui/searc...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/ui/searc...
chrome/browser/ui/search/instant_controller.cc:38: // TODO(treib): Is it safe to
completely remove this? crbug.com/627747
On 2016/08/13 05:03:44, Peter Kasting wrote:
> Probably?  Not sure, you'd need to check with kmadhusu or someone else more
> knowledgeable.  But at a glance it looks as if this is only really needed to
> expose the search terms, which I think you're ripping out the readers of.

Yup, that's my feeling too, but just to be safe I decided to defer this to
another CL - it's at least conceivable that creating this transient entry has
some other side-effects that someone depends on.

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/ui/searc...
File chrome/browser/ui/search/search_tab_helper.cc (left):

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/ui/searc...
chrome/browser/ui/search/search_tab_helper.cc:481: origin =
SearchMode::ORIGIN_SEARCH;
On 2016/08/13 05:03:44, Peter Kasting wrote:
> This is the only non-unittest location that sets this mode or this origin, so
> these values should be removed, along with anything that references them.

Yup, on my list :)
I've added TODOs to the definitions.

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/2232863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:701: // When the user has
clicked and released to give us focus, select all.
On 2016/08/13 05:03:44, Peter Kasting wrote:
> Nit: Move this comment above the line above, and then combine the nested
> conditionals.

Done.

https://codereview.chromium.org/2232863002/diff/20001/components/omnibox/brow...
File components/omnibox/browser/omnibox_edit_model.cc (left):

https://codereview.chromium.org/2232863002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:1513: return
OmniboxEventProto::SEARCH_RESULT_PAGE_DOING_SEARCH_TERM_REPLACEMENT;
On 2016/08/13 05:03:44, Peter Kasting wrote:
> This value should be changed to OBSOLETE in the proto file, and removed from
> unittests.

Done.

https://codereview.chromium.org/2232863002/diff/20001/components/omnibox/brow...
File components/omnibox/browser/omnibox_edit_model.cc (right):

https://codereview.chromium.org/2232863002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:107: transition,
ui::PAGE_TRANSITION_TYPED)) {
On 2016/08/13 05:03:44, Peter Kasting wrote:
> You should check with mpearson as to whether we want to keep this histogram at
> all.  It seems like we'd either want to rip it out entirely or else
> update/rename it since we're ripping out half the logging, although in
fairness
> that code hasn't run for a while anyway so I guess this doesn't really change
> things materially.

Added a TODO for now.

https://codereview.chromium.org/2232863002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:349: // If current text is not
composed of replaced search terms and
On 2016/08/13 05:03:44, Peter Kasting wrote:
> Nit: Remove everything in this line but the first word.

Done.

https://codereview.chromium.org/2232863002/diff/20001/components/omnibox/brow...
components/omnibox/browser/omnibox_edit_model.cc:372: if ((sel_min != 0))
On 2016/08/13 05:03:44, Peter Kasting wrote:
> Nit: Extra ()

Done.

https://codereview.chromium.org/2232863002/diff/20001/components/toolbar/tool...
File components/toolbar/toolbar_model.h (right):

https://codereview.chromium.org/2232863002/diff/20001/components/toolbar/tool...
components/toolbar/toolbar_model.h:78: // TODO(treib,pkasting): Can this be
removed? crbug.com/627747
On 2016/08/13 05:03:44, Peter Kasting wrote:
> Yes, it can.

Thanks! I left a TODO for now, since removing it is not completely trivial and
this CL is big enough already.

https://codereview.chromium.org/2232863002/diff/20001/components/toolbar/tool...
File components/toolbar/toolbar_model_impl.cc (left):

https://codereview.chromium.org/2232863002/diff/20001/components/toolbar/tool...
components/toolbar/toolbar_model_impl.cc:40: return search_terms;
On 2016/08/13 05:03:45, Peter Kasting wrote:
> This removal probably means we should rip out GetText() entirely and just
> replace calls to it with calls to GetFormattedURL().

Done.

[Marc Treib, 2016-08-16 12:00:24]

The CQ bit was checked by treib@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 12:00:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-16 12:03:14]

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

[commit-bot: I haz the power, 2016-08-16 12:03:15]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Marc Treib, 2016-08-16 14:27:05]

Description was changed from

==========
Remove search::GetSearchTerms since it always returns empty string
and update call sites accordingly.

This is the first of probably many follow-ups to
https://codereview.chromium.org/2211983002/

BUG=627747
==========

to

==========
Remove search::GetSearchTerms since it always returns empty string
and update call sites accordingly.

This is the first of probably many follow-ups to
https://codereview.chromium.org/2211983002/

TBRing a comment-only change in
components/sessions/core/serialized_navigation_entry.cc
TBR=marja@chromium.org

BUG=627747
==========

[Marc Treib, 2016-08-16 14:27:25]

The CQ bit was checked by treib@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-16 14:27:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Marc Treib, 2016-08-16 14:27:59]

treib@chromium.org changed reviewers:
+ isherman@chromium.org

[Marc Treib, 2016-08-16 14:28:00]

+isherman for components/metrics/proto/omnibox_event.proto, PTAL!

[commit-bot: I haz the power, 2016-08-16 14:47:49]

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

[commit-bot: I haz the power, 2016-08-16 14:47:50]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Ilya Sherman, 2016-08-16 20:39:40]

components/metrics/proto/omnibox_event.proto lgtm

[Marc Treib, 2016-08-17 06:53:11]

The CQ bit was checked by treib@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 06:53:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 08:04:40]

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

[commit-bot: I haz the power, 2016-08-17 08:04:41]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2016-08-17 08:05:27]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-08-17 08:05:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2232863002/#ps80001
(title: "Android")

[commit-bot: I haz the power, 2016-08-17 08:05:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 08:10:02]

Description was changed from

==========
Remove search::GetSearchTerms since it always returns empty string
and update call sites accordingly.

This is the first of probably many follow-ups to
https://codereview.chromium.org/2211983002/

TBRing a comment-only change in
components/sessions/core/serialized_navigation_entry.cc
TBR=marja@chromium.org

BUG=627747
==========

to

==========
Remove search::GetSearchTerms since it always returns empty string
and update call sites accordingly.

This is the first of probably many follow-ups to
https://codereview.chromium.org/2211983002/

TBRing a comment-only change in
components/sessions/core/serialized_navigation_entry.cc
TBR=marja@chromium.org

BUG=627747
==========

[commit-bot: I haz the power, 2016-08-17 08:10:03]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-08-17 08:14:25]

Description was changed from

==========
Remove search::GetSearchTerms since it always returns empty string
and update call sites accordingly.

This is the first of probably many follow-ups to
https://codereview.chromium.org/2211983002/

TBRing a comment-only change in
components/sessions/core/serialized_navigation_entry.cc
TBR=marja@chromium.org

BUG=627747
==========

to

==========
Remove search::GetSearchTerms since it always returns empty string
and update call sites accordingly.

This is the first of probably many follow-ups to
https://codereview.chromium.org/2211983002/

TBRing a comment-only change in
components/sessions/core/serialized_navigation_entry.cc
TBR=marja@chromium.org

BUG=627747

Committed: https://crrev.com/de23c50488c2558ba2633c0e4a2b09924bdd3704
Cr-Commit-Position: refs/heads/master@{#412473}
==========

[commit-bot: I haz the power, 2016-08-17 08:14:26]

Patchset 5 (id:??) landed as
https://crrev.com/de23c50488c2558ba2633c0e4a2b09924bdd3704
Cr-Commit-Position: refs/heads/master@{#412473}