Prepare HistoryProvider for componentization

chrome/browser/autocomplete/autocomplete_controller.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/autocomplete/autocomplete_controller.h"
 
 #include <set>
 #include <string>
 
 #include "base/format_macros.h"
@@ skipping 171 matching lines @@
       done_(true),
       in_start_(false),
       template_url_service_(template_url_service) {
   provider_types &= ~OmniboxFieldTrial::GetDisabledProviderTypes();
   if (provider_types & AutocompleteProvider::TYPE_BOOKMARK)
     providers_.push_back(new BookmarkProvider(profile));
   if (provider_types & AutocompleteProvider::TYPE_BUILTIN)
     providers_.push_back(new BuiltinProvider());
   if (provider_types & AutocompleteProvider::TYPE_HISTORY_QUICK) {
     providers_.push_back(new HistoryQuickProvider(
+        scoped_ptr<AutocompleteProviderClient>(
+            new ChromeAutocompleteProviderClient(profile)),
         profile, InMemoryURLIndexFactory::GetForProfile(profile)));

[Peter Kasting, 2015/06/12 16:20:22]

It feels weird that right now these classes take both a profile and an
AutocompleteProviderClient that itself takes a profile.

What if this file remained unchanged and these providers themselves constructed
the AutocompleteProviderClients?  When at some point the Profile dependencies
need to be eliminated from these classes as well, we could move the construction
here.  That seems cleaner for now, at least.

[blundell, 2015/06/12 16:46:59]

I'd prefer keep this structure even though it's kind of odd as I plan on
continuing the componentization such that the Profile dependencies will be
eliminated from the History*Provider subclasses in the near future. However, if
you feel strongly about it I can change to what you suggest.

[Peter Kasting, 2015/06/12 17:02:20]

How near is near future?  If it's in the next week or so, this is fine.  Longer
than that and I worry it could get sidelined, in which case the other would be
nice.

This makes me think of a different question.  Should there really be separate
ChromeAutocompleteProviderClient objects for each provider, owned by the
provider?  It seems like a better model would be that the provider would take a
raw pointer to one, which would be a shared client owned by whoever owned the
provider (in this case, the AutocompleteController).  Especially as more
providers get converted, this reduces both boiler plate and overhead.

[blundell, 2015/06/12 19:30:19]

"Near future" is within a couple of weeks *unless* there's an unexpected
blocker, so I take your point :).

But I think that your suggested model is actually the right one. OK with making
that change now and having these two classes take in both a Profile* and a
Client* for the transition period?

[blundell, 2015/06/15 08:51:33]

Changed to that model.

   }
   if (provider_types & AutocompleteProvider::TYPE_HISTORY_URL) {
-    history_url_provider_ = new HistoryURLProvider(this, profile);
+    history_url_provider_ = new HistoryURLProvider(
+        scoped_ptr<AutocompleteProviderClient>(
+            new ChromeAutocompleteProviderClient(profile)),
+        this, profile);
     providers_.push_back(history_url_provider_);
   }
   // "Tab to search" can be used on all platforms other than Android.
 #if !defined(OS_ANDROID)
   if (provider_types & AutocompleteProvider::TYPE_KEYWORD) {
     keyword_provider_ = new KeywordProvider(this, template_url_service);
 #if defined(ENABLE_EXTENSIONS)
     keyword_provider_->set_extensions_delegate(
         scoped_ptr<KeywordExtensionsDelegate>(
             new KeywordExtensionsDelegateImpl(profile, keyword_provider_)));
@@ skipping 461 matching lines @@
   expire_timer_.Stop();
   stop_timer_.Stop();
   done_ = true;
   if (clear_result && !result_.empty()) {
     result_.Reset();
     // NOTE: We pass in false since we're trying to only clear the popup, not
     // touch the edit... this is all a mess and should be cleaned up :(
     NotifyChanged(false);
   }
 }