Remove search::IsQueryExtractionEnabled

Remove search::IsQueryExtractionEnabled

BUG=627747, 627483
Committed: https://crrev.com/c4fec3722b90b214511eb24745fa172c916f5902
Cr-Commit-Position: refs/heads/master@{#411587}

[Marc Treib, 2016-08-09 10:23:42]

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

[Marc Treib, 2016-08-09 10:23:43]

This is the (first) big one! There's more that can be removed, but I figured
this CL was big enough.

pkasting: Please look at c/b/ui minus c/b/ui/search.

kmadhusu: Please look at anything "search", and overall sanity.

Thanks!

https://codereview.chromium.org/2211983002/diff/120001/chrome/browser/ui/sear...
File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (left):

https://codereview.chromium.org/2211983002/diff/120001/chrome/browser/ui/sear...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:331:
chrome::NOTIFICATION_INSTANT_TAB_SUPPORT_DETERMINED,
This notification doesn't get sent anymore for SRPs - without query extraction,
we don't even bother checking for instant support.

https://codereview.chromium.org/2211983002/diff/120001/chrome/browser/ui/sear...
File chrome/browser/ui/search/instant_extended_interactive_uitest.cc (right):

https://codereview.chromium.org/2211983002/diff/120001/chrome/browser/ui/sear...
chrome/browser/ui/search/instant_extended_interactive_uitest.cc:531:
IN_PROC_BROWSER_TEST_F(InstantExtendedTest, DISABLED_NavigateBackToNTP) {
Since parts of this test got removed, it might be worth a try to re-enable it,
but I won't do that in the same CL.

https://codereview.chromium.org/2211983002/diff/120001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_model_unittest.cc (right):

https://codereview.chromium.org/2211983002/diff/120001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_model_unittest.cc:196:
TEST_F(ToolbarModelTest, ShouldDisplayURL) {
I'm not sure how much value this test still has with all the query extraction
parts removed. It still tests that URLs are displayed properly, but now the only
special case is that "http://" gets removed.

[Peter Kasting, 2016-08-09 20:55:13]

LGTM

https://codereview.chromium.org/2211983002/diff/120001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_model_unittest.cc (right):

https://codereview.chromium.org/2211983002/diff/120001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_model_unittest.cc:196:
TEST_F(ToolbarModelTest, ShouldDisplayURL) {
On 2016/08/09 10:23:43, Marc Treib wrote:
> I'm not sure how much value this test still has with all the query extraction
> parts removed. It still tests that URLs are displayed properly, but now the
only
> special case is that "http://" gets removed.

I would keep the file, but I'd rip out PopupToolbarModelTest entirely, remove
the last four test item entries, remove the TemplateURLServiceFactory an
SetGoogleBaseURL() stuff from SetUp(), remove the last couple blocks of
NavigateAndCheckText() that toggle whether editing is in progress, remove the
bits about faking a secure connection, see if NavigateAndCheckText() and
NavigateAndCheckElided() can share code somehow, etc.

https://codereview.chromium.org/2211983002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (left):

https://codereview.chromium.org/2211983002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:1104: select_all_position
+ 1, IDS_SHOW_URL, IDS_SHOW_URL);
Make sure that removing IDS_SHOW_URL and all its handling code is on your list
of stuff to do next, so that doesn't end up orphaned.

(There's also IDS_SHOW_URL_MAC; I think it works similarly so I'm not really
sure why they need a string, but that should be ripped out at the same time
probably.)

[Marc Treib, 2016-08-10 12:53:40]

Thanks Peter!

Ping @ Kausalya - could you take a look please? Thanks!

https://codereview.chromium.org/2211983002/diff/120001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_model_unittest.cc (right):

https://codereview.chromium.org/2211983002/diff/120001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_model_unittest.cc:196:
TEST_F(ToolbarModelTest, ShouldDisplayURL) {
On 2016/08/09 20:55:13, Peter Kasting wrote:
> On 2016/08/09 10:23:43, Marc Treib wrote:
> > I'm not sure how much value this test still has with all the query
extraction
> > parts removed. It still tests that URLs are displayed properly, but now the
> only
> > special case is that "http://" gets removed.
> 
> I would keep the file, but I'd rip out PopupToolbarModelTest entirely, remove
> the last four test item entries, remove the TemplateURLServiceFactory an
> SetGoogleBaseURL() stuff from SetUp(), remove the last couple blocks of
> NavigateAndCheckText() that toggle whether editing is in progress, remove the
> bits about faking a secure connection, see if NavigateAndCheckText() and
> NavigateAndCheckElided() can share code somehow, etc.

Done, mostly (except for sharing code between the two NavigateAnd... methods -
they could potentially share the first few lines, but IMO not worth it).

Out of curiosity: What was the purpose of the PopupToolbarModelTest? The only
difference to ToolbarModelTest was the "Browser::TYPE_POPUP" part; not sure what
that means or why it would make any difference for the test?

https://codereview.chromium.org/2211983002/diff/120001/chrome/browser/ui/view...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (left):

https://codereview.chromium.org/2211983002/diff/120001/chrome/browser/ui/view...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:1104: select_all_position
+ 1, IDS_SHOW_URL, IDS_SHOW_URL);
On 2016/08/09 20:55:13, Peter Kasting wrote:
> Make sure that removing IDS_SHOW_URL and all its handling code is on your list
> of stuff to do next, so that doesn't end up orphaned.
> 
> (There's also IDS_SHOW_URL_MAC; I think it works similarly so I'm not really
> sure why they need a string, but that should be ripped out at the same time
> probably.)

Yup, that's already on my list :)

[kmadhusu, 2016-08-10 20:19:53]

Changes LGTM.

Thanks.

https://codereview.chromium.org/2211983002/diff/140001/components/search/sear...
File components/search/search.cc (left):

https://codereview.chromium.org/2211983002/diff/140001/components/search/sear...
components/search/search.cc:59: const char kEnableQueryExtractionFlagName[] =
"query_extraction";
Some of the expired finch trials have references to this flag name (go/uervm).
Please update those references accordingly.

[Marc Treib, 2016-08-11 09:58:52]

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

[commit-bot: I haz the power, 2016-08-11 09:59:03]

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-11 09:59:14]

https://codereview.chromium.org/2211983002/diff/140001/components/search/sear...
File components/search/search.cc (left):

https://codereview.chromium.org/2211983002/diff/140001/components/search/sear...
components/search/search.cc:59: const char kEnableQueryExtractionFlagName[] =
"query_extraction";
On 2016/08/10 20:19:52, kmadhusu wrote:
> Some of the expired finch trials have references to this flag name (go/uervm).
> Please update those references accordingly.

AFAIK we don't have a policy of updating historic field trials?
I mean, I can update them if you want, but TBH I don't see the point?

[commit-bot: I haz the power, 2016-08-11 11:34:16]

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

[commit-bot: I haz the power, 2016-08-11 11:34:18]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2016-08-11 11:37:28]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-08-11 11:37: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/2211983002/#ps140001
(title: "remove more tests")

[Marc Treib, 2016-08-11 11:37:29]

The CQ bit was unchecked by treib@chromium.org

[Marc Treib, 2016-08-11 11:39:06]

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

[Marc Treib, 2016-08-11 11:39:07]

+sdefresne for ios/ approval - just deleting some unused code. PTAL, thanks!

[Marc Treib, 2016-08-11 11:42:29]

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

[Marc Treib, 2016-08-11 11:42:30]

+tnagel for c/b/policy approval. PTAL, thanks!

https://codereview.chromium.org/2211983002/diff/140001/chrome/browser/policy/...
File chrome/browser/policy/policy_browsertest.cc (left):

https://codereview.chromium.org/2211983002/diff/140001/chrome/browser/policy/...
chrome/browser/policy/policy_browsertest.cc:1194:
IN_PROC_BROWSER_TEST_F(PolicyTest, ReplaceSearchTerms) {
This (mostly) tested a feature that was never launched and is being removed - I
moved the one thing that still applies, the kInstantURL part, into the
DefaultSearchProvider test.

[Thiemo Nagel, 2016-08-11 11:58:53]

On 2016/08/11 11:42:30, Marc Treib wrote:
> +tnagel for c/b/policy approval. PTAL, thanks!

chrome/browser/policy/policy_browsertest.cc rubberstamp lgtm

[sdefresne, 2016-08-11 18:32:48]

ios/ lgtm

[kmadhusu, 2016-08-11 22:17:12]

lgtm

https://codereview.chromium.org/2211983002/diff/140001/components/search/sear...
File components/search/search.cc (left):

https://codereview.chromium.org/2211983002/diff/140001/components/search/sear...
components/search/search.cc:59: const char kEnableQueryExtractionFlagName[] =
"query_extraction";
On 2016/08/11 09:59:14, Marc Treib wrote:
> On 2016/08/10 20:19:52, kmadhusu wrote:
> > Some of the expired finch trials have references to this flag name
(go/uervm).
> > Please update those references accordingly.
> 
> AFAIK we don't have a policy of updating historic field trials?
> I mean, I can update them if you want, but TBH I don't see the point?

Its been a long time since I updated the field trial. So, I am not familiar
about the current policy. If we can't update them, that's fine to me.

[Marc Treib, 2016-08-12 09:49:37]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2016-08-12 09:49:37]

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/2211983002/#ps140001
(title: "remove more tests")

[commit-bot: I haz the power, 2016-08-12 09:49:49]

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-12 09:53:49]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-08-12 09:55:38]

Description was changed from

==========
Remove search::IsQueryExtractionEnabled

BUG=627747,627483
==========

to

==========
Remove search::IsQueryExtractionEnabled

BUG=627747,627483

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

[commit-bot: I haz the power, 2016-08-12 09:55:40]

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

[Peter Kasting, 2016-08-20 02:21:19]

Apparently I never sent this draft message!  It's not really important and
requires no followup, but sending it anyway for posterity.

https://codereview.chromium.org/2211983002/diff/120001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_model_unittest.cc (right):

https://codereview.chromium.org/2211983002/diff/120001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_model_unittest.cc:196:
TEST_F(ToolbarModelTest, ShouldDisplayURL) {
On 2016/08/10 12:53:40, Marc Treib wrote:
> On 2016/08/09 20:55:13, Peter Kasting wrote:
> > On 2016/08/09 10:23:43, Marc Treib wrote:
> > > I'm not sure how much value this test still has with all the query
> extraction
> > > parts removed. It still tests that URLs are displayed properly, but now
the
> > only
> > > special case is that "http://" gets removed.
> > 
> > I would keep the file, but I'd rip out PopupToolbarModelTest entirely,
remove
> > the last four test item entries, remove the TemplateURLServiceFactory an
> > SetGoogleBaseURL() stuff from SetUp(), remove the last couple blocks of
> > NavigateAndCheckText() that toggle whether editing is in progress, remove
the
> > bits about faking a secure connection, see if NavigateAndCheckText() and
> > NavigateAndCheckElided() can share code somehow, etc.
> 
> Done, mostly (except for sharing code between the two NavigateAnd... methods -
> they could potentially share the first few lines, but IMO not worth it).
> 
> Out of curiosity: What was the purpose of the PopupToolbarModelTest? The only
> difference to ToolbarModelTest was the "Browser::TYPE_POPUP" part; not sure
what
> that means or why it would make any difference for the test?

The location bar works differently in popup windows (e.g. is not editable).  The
test verified none of those differences affected this sort of case.  That
distinction isn't meaningful enough, with most of this file gone, to stick
around.