Introduce a couple of abstract bases for WebDataService.

chrome/browser/autofill/autocomplete_history_manager.cc

Index: chrome/browser/autofill/autocomplete_history_manager.cc
diff --git a/chrome/browser/autofill/autocomplete_history_manager.cc b/chrome/browser/autofill/autocomplete_history_manager.cc
index 042e83783667e2055044fb29b0fe0ac13d914264..fb01eefd48193562e750c1f9cb5388ca8ba0954a 100644
--- a/chrome/browser/autofill/autocomplete_history_manager.cc
+++ b/chrome/browser/autofill/autocomplete_history_manager.cc
@@ -13,7 +13,6 @@
 #include "chrome/browser/autofill/autofill_external_delegate.h"
 #include "chrome/browser/autofill/credit_card.h"
 #include "chrome/browser/profiles/profile.h"
-#include "chrome/browser/webdata/web_data_service_factory.h"
 #include "chrome/common/autofill_messages.h"
 #include "chrome/common/pref_names.h"
 #include "content/public/browser/render_view_host.h"
@@ -116,8 +115,7 @@ AutocompleteHistoryManager::AutocompleteHistoryManager(
       external_delegate_(NULL) {
   profile_ = Profile::FromBrowserContext(web_contents->GetBrowserContext());
   // May be NULL in unit tests.
-  web_data_service_ = WebDataServiceFactory::GetForProfile(
-      profile_, Profile::EXPLICIT_ACCESS);
+  web_data_service_ = WebDataServiceBase::ForProfile(profile_);

[erikwright (departed), 2012/09/04 17:43:50]

Why go through WebDataServiceBase? Why not have
AutofillWebDataService::ForProfile and have the detail that this is implemented
by WebDataService be hidden?

[Jói, 2012/09/06 01:17:27]

As discussed, there are common bits in WebDataServiceBase that any consumer of
the service would use (currently just the typedef of Handle, and the
CancelRequest method).

Since we can't multiply inherit concrete base classes, having AutofillWebData
also be a refcounted object seems out of the question.  That leaves a few
choices:

a) The current approach, where knowing how to get an AutofillWebData is apparent
by looking at the AutofillWebData interface and seeing the FromService method
(i.e. you see that you need a WebDataServiceBase).  This is error prone because
you might forget to keep the reference to WebDataserviceBase for as long as you
keep the AutofillWebData pointer.

b) The current approach, except on its head; instead of
AutofillWebData::ForService(), you have an accessor named GetAutofillWebData()
on WebDataServiceBase that returns an AutofillWebData*.  This has the same
limitation w.r.t. the reference, although you are perhaps slightly less likely
to forget to keep the WebDataServiceBase reference.

c) Have WebDataService implement WebDataServiceBase and AutofillWebData as it
does today, but using either approach (a) or (b), instead of getting an
AutofillWebData*, you are returned a scoped_ptr<AutofillWebData> which instead
of actually pointing to an underlying WebDataService object, points to an
AutofillWebDataRef object that inherits from AutofillWebData, contains an inner
scoped_refptr<WebDataService> to ensure lifetime, and implements all methods of
AutofillWebData by delegating to the inner WebDataService.

Approach (c) is probably the safest, and it leaves flexibility in whether the
underlying implementation is one big class (as today) or split up (as may be
desired in the future).  Its drawback is that it is a bit more implementation
because it uses delegation rather than inheritance, but that is probably fine.

All in favor?

The last outstanding question, which I'm relatively agnostic on, is whether to
have an accessor on WebDataServiceBase to retrieve AutofillWebData, or whether
to have a ForService factory on AutofillWebData.  I lean slightly to the latter,
since it decouples things a bit more, but am willing to be persuaded of the
former.

   autofill_enabled_.Init(
       prefs::kAutofillEnabled, PrefServiceBase::ForProfile(profile_), NULL);
 }
@@ -138,7 +136,7 @@ bool AutocompleteHistoryManager::OnMessageReceived(
 }
 
 void AutocompleteHistoryManager::OnWebDataServiceRequestDone(
-    WebDataService::Handle h,
+    WebDataServiceBase::Handle h,
     const WDTypedResult* result) {
   DCHECK(pending_query_handle_);
   pending_query_handle_ = 0;
@@ -184,8 +182,8 @@ void AutocompleteHistoryManager::OnGetAutocompleteSuggestions(
     return;
   }
 
-  if (web_data_service_.get()) {
-    pending_query_handle_ = web_data_service_->GetFormValuesForElementName(
+  if (autofill_data()) {
+    pending_query_handle_ = autofill_data()->GetFormValuesForElementName(
         name, prefix, kMaxAutocompleteMenuItems, this);
   }
 }
@@ -220,14 +218,14 @@ void AutocompleteHistoryManager::OnFormSubmitted(const FormData& form) {
     }
   }
 
-  if (!values.empty() && web_data_service_.get())
-    web_data_service_->AddFormFields(values);
+  if (!values.empty() && autofill_data())
+    autofill_data()->AddFormFields(values);
 }
 
 void AutocompleteHistoryManager::OnRemoveAutocompleteEntry(
     const string16& name, const string16& value) {
-  if (web_data_service_.get())
-    web_data_service_->RemoveFormValueForElementName(name, value);
+  if (autofill_data())
+    autofill_data()->RemoveFormValueForElementName(name, value);
 }
 
 void AutocompleteHistoryManager::SetExternalDelegate(
@@ -238,7 +236,7 @@ void AutocompleteHistoryManager::SetExternalDelegate(
 AutocompleteHistoryManager::AutocompleteHistoryManager(
     WebContents* web_contents,
     Profile* profile,
-    WebDataService* wds)
+    WebDataServiceBase* wds)
     : content::WebContentsObserver(web_contents),
       profile_(profile),
       web_data_service_(wds),
@@ -252,7 +250,7 @@ AutocompleteHistoryManager::AutocompleteHistoryManager(
 void AutocompleteHistoryManager::CancelPendingQuery() {
   if (pending_query_handle_) {
     SendSuggestions(NULL);
-    if (web_data_service_.get())
+    if (web_data_service_)
       web_data_service_->CancelRequest(pending_query_handle_);
     pending_query_handle_ = 0;
   }