Reduces dependency from AutocompleteMatch to Profile.

Reduces dependency from AutocompleteMatch to Profile.

BUG=388510
R=hashimoto@chromium.org, blundell@chromium.org, pkasting@chromium.org
TBR=dtrainor@chromium.org
TEST=compile

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

[Jun Mukai, 2014-06-25 22:48:21]

hashimoto, blundell,

I don't want to increase the work load of pkasting unnecessarily. Could you
review the code first?

[hashimoto, 2014-06-26 00:04:41]

lgtm

https://codereview.chromium.org/354773002/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_match.h (right):

https://codereview.chromium.org/354773002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_match.h:180: // proper deduping.
|search_terms_data| is used to compute
I see no search_terms_data in this method.

[blundell, 2014-06-26 09:00:00]

lgtm

lgtm with question

https://codereview.chromium.org/354773002/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

https://codereview.chromium.org/354773002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_controller.cc:411:
TemplateURLServiceFactory::GetForProfile(profile_);
This class will eventually need to have the TemplateURLService passed in as
well. Is it worth it to just do that as part of this CL?

[Jun Mukai, 2014-06-26 21:01:19]

https://codereview.chromium.org/354773002/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_controller.cc (right):

https://codereview.chromium.org/354773002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_controller.cc:411:
TemplateURLServiceFactory::GetForProfile(profile_);
On 2014/06/26 09:00:00, blundell wrote:
> This class will eventually need to have the TemplateURLService passed in as
> well. Is it worth it to just do that as part of this CL?

Agree, it would be better to pass TemplateURLService as a constructor argument.
But AutoCompleteController is created several places (6-7 classes create and own
its own controller, apart from tests), so doing it would make the change even
bigger.

Possibly preparing AutocompleteControllerFactory would be a better approach, and
that should be done in another CL. Let me create that CL first.

https://codereview.chromium.org/354773002/diff/20001/chrome/browser/autocompl...
File chrome/browser/autocomplete/autocomplete_match.h (right):

https://codereview.chromium.org/354773002/diff/20001/chrome/browser/autocompl...
chrome/browser/autocomplete/autocomplete_match.h:180: // proper deduping.
|search_terms_data| is used to compute
On 2014/06/26 00:04:40, hashimoto wrote:
> I see no search_terms_data in this method.

Done.

[Jun Mukai, 2014-06-27 20:37:01]

Looked at the code and I feel like I can do the constructor parameter change of
AutocompleteController in the same CL. Please take another look.

[blundell, 2014-06-30 11:50:36]

LGTM

[Jun Mukai, 2014-06-30 21:42:29]

pkasting, could you review this?

[Peter Kasting, 2014-06-30 22:42:09]

https://codereview.chromium.org/354773002/diff/100001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_controller.h (right):

https://codereview.chromium.org/354773002/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_controller.h:56: TemplateURLService*
template_url_service);
Nit: Place this right after the Profile* argument

https://codereview.chromium.org/354773002/diff/100001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://codereview.chromium.org/354773002/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_match.cc:362: stripped_destination_url,
Nit: Indent 4, not 8

https://codereview.chromium.org/354773002/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_match.cc:417:
template_url_service->search_terms_data())) ?
It seems strange that this place unconditionally derefs |template_url_service|,
but GetTemplateURL() below NULL-checks.  I think one of these must be wrong.

[Jun Mukai, 2014-06-30 23:52:36]

https://codereview.chromium.org/354773002/diff/100001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_controller.h (right):

https://codereview.chromium.org/354773002/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_controller.h:56: TemplateURLService*
template_url_service);
On 2014/06/30 22:42:09, Peter Kasting wrote:
> Nit: Place this right after the Profile* argument

Done.

https://codereview.chromium.org/354773002/diff/100001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://codereview.chromium.org/354773002/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_match.cc:362: stripped_destination_url,
On 2014/06/30 22:42:09, Peter Kasting wrote:
> Nit: Indent 4, not 8

This is indent 4 from 'template_url->...' in the previous line, because these
are the parameters of ExtractSearchTermsFromURL. Forcing 4 columns left looks a
bit weird to me because these arguments and 'template_url->...' are at the same
level.

https://codereview.chromium.org/354773002/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_match.cc:417:
template_url_service->search_terms_data())) ?
On 2014/06/30 22:42:09, Peter Kasting wrote:
> It seems strange that this place unconditionally derefs
|template_url_service|,
> but GetTemplateURL() below NULL-checks.  I think one of these must be wrong.

Great catch. thanks.  Added NULL check on this method.

[Peter Kasting, 2014-06-30 23:55:46]

LGTM

https://codereview.chromium.org/354773002/diff/100001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://codereview.chromium.org/354773002/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_match.cc:362: stripped_destination_url,
On 2014/06/30 23:52:35, Jun Mukai wrote:
> On 2014/06/30 22:42:09, Peter Kasting wrote:
> > Nit: Indent 4, not 8
> 
> This is indent 4 from 'template_url->...' in the previous line, because these
> are the parameters of ExtractSearchTermsFromURL. Forcing 4 columns left looks
a
> bit weird to me because these arguments and 'template_url->...' are at the
same
> level.

I know, but that's how conditional indenting in most of Chrome works.

https://codereview.chromium.org/354773002/diff/120001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://codereview.chromium.org/354773002/diff/120001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_match.cc:414: if (template_url_service
== NULL)
Nit: Combine this with the conditional above

[Jun Mukai, 2014-07-01 00:08:01]

https://codereview.chromium.org/354773002/diff/100001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://codereview.chromium.org/354773002/diff/100001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_match.cc:362: stripped_destination_url,
On 2014/06/30 23:55:46, Peter Kasting wrote:
> On 2014/06/30 23:52:35, Jun Mukai wrote:
> > On 2014/06/30 22:42:09, Peter Kasting wrote:
> > > Nit: Indent 4, not 8
> > 
> > This is indent 4 from 'template_url->...' in the previous line, because
these
> > are the parameters of ExtractSearchTermsFromURL. Forcing 4 columns left
looks
> a
> > bit weird to me because these arguments and 'template_url->...' are at the
> same
> > level.
> 
> I know, but that's how conditional indenting in most of Chrome works.

Okay, fixed.

https://codereview.chromium.org/354773002/diff/120001/chrome/browser/autocomp...
File chrome/browser/autocomplete/autocomplete_match.cc (right):

https://codereview.chromium.org/354773002/diff/120001/chrome/browser/autocomp...
chrome/browser/autocomplete/autocomplete_match.cc:414: if (template_url_service
== NULL)
On 2014/06/30 23:55:46, Peter Kasting wrote:
> Nit: Combine this with the conditional above

Done.

[Jun Mukai, 2014-07-01 00:08:11]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-07-01 00:10:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mukai@chromium.org/354773002/140001

[commit-bot: I haz the power, 2014-07-01 04:36:51]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)

[commit-bot: I haz the power, 2014-07-01 04:49:06]

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

[commit-bot: I haz the power, 2014-07-01 04:49:07]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)

[Jun Mukai, 2014-07-01 17:35:24]

On 2014/07/01 04:49:07, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   android_clang_dbg on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
>   android_dbg on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)

Needed to edit auatocomplete_controller_android.cc as well. Done and let me push
to CQ w/ TBR=dtrainor.
The edit of the _android.cc is adding a new parameter to AutocompleteController.

[Jun Mukai, 2014-07-01 17:35:28]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-07-01 17:37:00]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mukai@chromium.org/354773002/180001

[commit-bot: I haz the power, 2014-07-01 21:36:01]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/20...)

[Jun Mukai, 2014-07-01 21:45:28]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-07-01 21:45:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mukai@chromium.org/354773002/200001

[commit-bot: I haz the power, 2014-07-02 00:09:20]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-02 00:11:45]

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

[commit-bot: I haz the power, 2014-07-02 00:11:46]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[Jun Mukai, 2014-07-02 00:45:48]

The CQ bit was checked by mukai@chromium.org

[commit-bot: I haz the power, 2014-07-02 00:46:28]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mukai@chromium.org/354773002/220001

[commit-bot: I haz the power, 2014-07-02 03:58:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-02 07:03:40]

Change committed as 280955

[jochen (gone - plz use gerrit), 2014-07-02 09:13:27]

A revert of this CL has been created in
https://codereview.chromium.org/363893002/ by jochen@chromium.org.

The reason for reverting is: AutocompleteProviderTest.GetDestinationURL crashes
on dbg bots.