Clean up the unused Suggest field trial and make GetActiveSuggestFieldTrialHash

Clean up the unused Suggest field trial and make GetActiveSuggestFieldTrialHash
use the dynamic field trials instead of the static one being deleted in this change.

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

[Bart N., 2013-04-11 01:16:12]

Mark and Steve, please take a look at your convenience. There is no rush with
this change as long as it makes sometime in M28.

Mark - I haven't cleaned up Disabled providers logic in this change yet, because
I wanted to first check with you if you need it in M28+.

[SteveT, 2013-04-11 13:46:27]

Couple quick things - might come by with more later.

THANK YOU for being diligent with cleaning up variation_ids.h :) !

https://codereview.chromium.org/13940005/diff/1/chrome/browser/autocomplete/s...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/13940005/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider.cc:34: #include
"chrome/browser/omnibox/omnibox_field_trial.h"
Can you check if this is still used?

https://codereview.chromium.org/13940005/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider.cc:433: }
That's odd... do these two braces need to be cleaned up?

[Mark P, 2013-04-11 18:17:13]

https://codereview.chromium.org/13940005/diff/1/chrome/browser/omnibox/omnibo...
File chrome/browser/omnibox/omnibox_field_trial.cc (right):

https://codereview.chromium.org/13940005/diff/1/chrome/browser/omnibox/omnibo...
chrome/browser/omnibox/omnibox_field_trial.cc:226: std::vector<uint32>*
field_trial_hashes) {
Please explicitly clear it first.

https://codereview.chromium.org/13940005/diff/1/chrome/browser/omnibox/omnibo...
chrome/browser/omnibox/omnibox_field_trial.cc:227: bool found = false;
Rather having a found variable, consider returning !empty().

Or consider whether you need to return anything at all.

[Bart N., 2013-04-11 19:34:53]

On 2013/04/11 18:17:13, Mark P wrote:
>
https://codereview.chromium.org/13940005/diff/1/chrome/browser/omnibox/omnibo...
> File chrome/browser/omnibox/omnibox_field_trial.cc (right):
> 
>
https://codereview.chromium.org/13940005/diff/1/chrome/browser/omnibox/omnibo...
> chrome/browser/omnibox/omnibox_field_trial.cc:226: std::vector<uint32>*
> field_trial_hashes) {
> Please explicitly clear it first.
> 
>
https://codereview.chromium.org/13940005/diff/1/chrome/browser/omnibox/omnibo...
> chrome/browser/omnibox/omnibox_field_trial.cc:227: bool found = false;
> Rather having a found variable, consider returning !empty().
> 
> Or consider whether you need to return anything at all.
I don't really need to return anything. Simplified.

[Bart N., 2013-04-11 19:35:07]

https://codereview.chromium.org/13940005/diff/1/chrome/browser/autocomplete/s...
File chrome/browser/autocomplete/search_provider.cc (right):

https://codereview.chromium.org/13940005/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider.cc:34: #include
"chrome/browser/omnibox/omnibox_field_trial.h"
On 2013/04/11 13:46:27, SteveT wrote:
> Can you check if this is still used?
Yes :) I still call OmniboxFieldTrial::GetActiveSuggestFieldTrialHashes.

https://codereview.chromium.org/13940005/diff/1/chrome/browser/autocomplete/s...
chrome/browser/autocomplete/search_provider.cc:433: }
On 2013/04/11 13:46:27, SteveT wrote:
> That's odd... do these two braces need to be cleaned up?
They are needed for if/for statements.

https://codereview.chromium.org/13940005/diff/1/chrome/browser/omnibox/omnibo...
File chrome/browser/omnibox/omnibox_field_trial.cc (right):

https://codereview.chromium.org/13940005/diff/1/chrome/browser/omnibox/omnibo...
chrome/browser/omnibox/omnibox_field_trial.cc:226: std::vector<uint32>*
field_trial_hashes) {
On 2013/04/11 18:17:14, Mark P wrote:
> Please explicitly clear it first.

Done.

https://codereview.chromium.org/13940005/diff/1/chrome/browser/omnibox/omnibo...
chrome/browser/omnibox/omnibox_field_trial.cc:227: bool found = false;
On 2013/04/11 18:17:14, Mark P wrote:
> Rather having a found variable, consider returning !empty().
> 
> Or consider whether you need to return anything at all.
Yes, much better now.

[Mark P, 2013-04-11 21:45:59]

lgtm

but please wait for Steve to circle back before you submit.

--mark

[SteveT, 2013-04-12 14:03:40]

lgtm - thanks for cleaning this up!

[Bart N., 2013-04-12 17:20:34]

Mark and Steve, thanks for the quick review! Submitting...

[commit-bot: I haz the power, 2013-04-12 17:20:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartn@chromium.org/13940005/9001

[commit-bot: I haz the power, 2013-04-12 17:20:54]

Presubmit check for 13940005-9001 failed and returned exit status 1.

INFO:root:Found 5 file(s).

Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/omnibox/omnibox_field_trial.cc
    chrome/browser/omnibox/omnibox_field_trial.h

Presubmit checks took 1.1s to calculate.

[Bart N., 2013-04-12 17:27:47]

On 2013/04/12 17:20:54, I haz the power (commit-bot) wrote:
> Presubmit check for 13940005-9001 failed and returned exit status 1.
> 
> INFO:root:Found 5 file(s).
> 
> Running presubmit commit checks ...
> Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
> Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
> 
> ** Presubmit ERRORS **
> Missing LGTM from an OWNER for these files:
>     chrome/browser/omnibox/omnibox_field_trial.cc
>     chrome/browser/omnibox/omnibox_field_trial.h
> 
> Presubmit checks took 1.1s to calculate.
Mark - is it intentional we don't have OWNERS in omnibox/*? Do you want me to
add it and include you?

[Bart N., 2013-04-12 17:28:44]

On 2013/04/12 17:27:47, Bart N. wrote:
> On 2013/04/12 17:20:54, I haz the power (commit-bot) wrote:
> > Presubmit check for 13940005-9001 failed and returned exit status 1.
> > 
> > INFO:root:Found 5 file(s).
> > 
> > Running presubmit commit checks ...
> > Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
> > Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
> > 
> > ** Presubmit ERRORS **
> > Missing LGTM from an OWNER for these files:
> >     chrome/browser/omnibox/omnibox_field_trial.cc
> >     chrome/browser/omnibox/omnibox_field_trial.h
> > 
> > Presubmit checks took 1.1s to calculate.
> Mark - is it intentional we don't have OWNERS in omnibox/*? Do you want me to
> add it and include you?
Or perhaps just amend chrome/browser/OWNERS with a file level permission.

[Mark P, 2013-04-12 18:04:54]

On 2013/04/12 17:27:47, Bart N. wrote:
> On 2013/04/12 17:20:54, I haz the power (commit-bot) wrote:
> > Presubmit check for 13940005-9001 failed and returned exit status 1.
> > 
> > INFO:root:Found 5 file(s).
> > 
> > Running presubmit commit checks ...
> > Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
> > Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
> > 
> > ** Presubmit ERRORS **
> > Missing LGTM from an OWNER for these files:
> >     chrome/browser/omnibox/omnibox_field_trial.cc
> >     chrome/browser/omnibox/omnibox_field_trial.h
> > 
> > Presubmit checks took 1.1s to calculate.
> Mark - is it intentional we don't have OWNERS in omnibox/*? Do you want me to
> add it and include you?

It is not intentional.  Please create another changelist that copies the OWNERS
file from autocomplete to omnibox.  Then we just need an appropriate person to
approve that change...

I you don't get to this by mid-afternoon, I'll do it for you.

--mark

[Bart N., 2013-04-12 18:16:10]

On 2013/04/12 18:04:54, Mark P wrote:
> On 2013/04/12 17:27:47, Bart N. wrote:
> > On 2013/04/12 17:20:54, I haz the power (commit-bot) wrote:
> > > Presubmit check for 13940005-9001 failed and returned exit status 1.
> > > 
> > > INFO:root:Found 5 file(s).
> > > 
> > > Running presubmit commit checks ...
> > > Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
> > > Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
> > > 
> > > ** Presubmit ERRORS **
> > > Missing LGTM from an OWNER for these files:
> > >     chrome/browser/omnibox/omnibox_field_trial.cc
> > >     chrome/browser/omnibox/omnibox_field_trial.h
> > > 
> > > Presubmit checks took 1.1s to calculate.
> > Mark - is it intentional we don't have OWNERS in omnibox/*? Do you want me
to
> > add it and include you?
> 
> It is not intentional.  Please create another changelist that copies the
OWNERS
> file from autocomplete to omnibox.  Then we just need an appropriate person to
> approve that change...
> 
> I you don't get to this by mid-afternoon, I'll do it for you.
> 
> --mark
Sent out 13951013 for review.

[Bart N., 2013-04-12 22:14:15]

On 2013/04/12 18:16:10, Bart N. wrote:
> On 2013/04/12 18:04:54, Mark P wrote:
> > On 2013/04/12 17:27:47, Bart N. wrote:
> > > On 2013/04/12 17:20:54, I haz the power (commit-bot) wrote:
> > > > Presubmit check for 13940005-9001 failed and returned exit status 1.
> > > > 
> > > > INFO:root:Found 5 file(s).
> > > > 
> > > > Running presubmit commit checks ...
> > > > Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
> > > > Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
> > > > 
> > > > ** Presubmit ERRORS **
> > > > Missing LGTM from an OWNER for these files:
> > > >     chrome/browser/omnibox/omnibox_field_trial.cc
> > > >     chrome/browser/omnibox/omnibox_field_trial.h
> > > > 
> > > > Presubmit checks took 1.1s to calculate.
> > > Mark - is it intentional we don't have OWNERS in omnibox/*? Do you want me
> to
> > > add it and include you?
> > 
> > It is not intentional.  Please create another changelist that copies the
> OWNERS
> > file from autocomplete to omnibox.  Then we just need an appropriate person
to
> > approve that change...
> > 
> > I you don't get to this by mid-afternoon, I'll do it for you.
> > 
> > --mark
> Sent out 13951013 for review.
Giving one more shot, since now the OWNERS is in place :)

[commit-bot: I haz the power, 2013-04-12 22:14:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bartn@chromium.org/13940005/9001

[commit-bot: I haz the power, 2013-04-13 02:04:09]

Change committed as 194074