Eliminate AutofillWebDataService::FromBrowserContext().

chrome/browser/webdata/web_data_service_factory.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/webdata/web_data_service_factory.h"
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/profiles/incognito_helpers.h"
@@ skipping 123 matching lines @@
 
 scoped_refptr<WebDataService> WebDataServiceWrapper::GetWebData() {
   return web_data_.get();
 }
 
 scoped_refptr<TokenWebData> WebDataServiceWrapper::GetTokenWebData() {
   return token_web_data_.get();
 }
 
 // static
-scoped_refptr<AutofillWebDataService>
-AutofillWebDataService::FromBrowserContext(content::BrowserContext* context) {
-  // For this service, the implicit/explicit distinction doesn't
-  // really matter; it's just used for a DCHECK.  So we currently
-  // cheat and always say EXPLICIT_ACCESS.
-  WebDataServiceWrapper* wrapper =
-      WebDataServiceFactory::GetForProfile(
-          static_cast<Profile*>(context), Profile::EXPLICIT_ACCESS);
-  if (wrapper)
-    return wrapper->GetAutofillWebData();
-  // |wrapper| can be NULL in Incognito mode.
-  return scoped_refptr<AutofillWebDataService>(NULL);
-}
-
-// static
 scoped_refptr<TokenWebData> TokenWebData::FromBrowserContext(
     content::BrowserContext* context) {
   // For this service, the implicit/explicit distinction doesn't
   // really matter; it's just used for a DCHECK.  So we currently
   // cheat and always say EXPLICIT_ACCESS.
   WebDataServiceWrapper* wrapper =
       WebDataServiceFactory::GetForProfile(
           static_cast<Profile*>(context), Profile::EXPLICIT_ACCESS);
   if (wrapper)
     return wrapper->GetTokenWebData();
@@ skipping 23 matching lines @@
   // WebDataServiceFactory has no dependecies.
 }
 
 WebDataServiceFactory::~WebDataServiceFactory() {}
 
 // static
 WebDataServiceWrapper* WebDataServiceFactory::GetForProfile(
     Profile* profile, Profile::ServiceAccessType access_type) {
   // If |access_type| starts being used for anything other than this
   // DCHECK, we need to start taking it as a parameter to
-  // AutofillWebDataService::FromBrowserContext (see above).
+  // WebDataServiceFactory::GetAutofillWebDataForProfile (see below).
   DCHECK(access_type != Profile::IMPLICIT_ACCESS || !profile->IsOffTheRecord());
   return static_cast<WebDataServiceWrapper*>(
           GetInstance()->GetServiceForBrowserContext(profile, true));
 }
 
 // static
 WebDataServiceWrapper* WebDataServiceFactory::GetForProfileIfExists(
     Profile* profile, Profile::ServiceAccessType access_type) {
   // If |access_type| starts being used for anything other than this
   // DCHECK, we need to start taking it as a parameter to
-  // AutofillWebDataService::FromBrowserContext (see above).
+  // WebDataServiceFactory::GetAutofillWebDataForProfile (see below).
   DCHECK(access_type != Profile::IMPLICIT_ACCESS || !profile->IsOffTheRecord());
   return static_cast<WebDataServiceWrapper*>(
           GetInstance()->GetServiceForBrowserContext(profile, false));
 }
 
 // static
+scoped_refptr<AutofillWebDataService>
+WebDataServiceFactory::GetAutofillWebDataForProfile(Profile* profile) {

[Peter Kasting, 2013/11/15 01:30:59]

Nit: Indent 4

[blundell, 2013/11/19 14:05:37]

No indentation after return type:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
(in the "Some points to note" section).

On 2013/11/15 01:30:59, Peter Kasting wrote:

+  // For this service, the implicit/explicit distinction doesn't
+  // really matter; it's just used for a DCHECK.  So we currently
+  // cheat and always say EXPLICIT_ACCESS.

[Peter Kasting, 2013/11/15 01:30:59]

"Just a DCHECK" sounds kind of worrisome; DCHECKs usually indicate some state
that's supposed to hold.  If this distinction _truly_ doesn't matter, the
service shouldn't be DCHECKing it.  Otherwise, this function should take an
access parameter from the caller and pass it on to GetForProfile().

I realize you're merely moving this code, but since you're touching it, it
should be fixed.

If this would be super-hairy, it's OK to do it in a separate CL -- but I'd still
want to see it happen.

[Jói, 2013/11/15 13:25:34]

It would be OK to bring back the parameter and DCHECK now, since this code won't
be moving out of //chrome.

OTOH, all the DCHECK does is verify that the caller passed in a certain
parameter rather than a different parameter, and there is no code to handle what
would happen for a different value of the parameter.

[blundell, 2013/11/19 14:05:37]

Added in the parameter.

On 2013/11/15 13:25:34, Jói wrote:

+  WebDataServiceWrapper* wrapper =
+      WebDataServiceFactory::GetForProfile(profile, Profile::EXPLICIT_ACCESS);
+  if (wrapper)
+    return wrapper->GetAutofillWebData();
+  // |wrapper| can be NULL in Incognito mode.
+  return scoped_refptr<AutofillWebDataService>(NULL);

[Peter Kasting, 2013/11/15 01:30:59]

Nit: Simpler:

  // |wrapper| can be NULL in Incognito.
  return wrapper ?
      wrapper->GetAutofillWebData() :
      scoped_refptr<AutofillWebDataService>(NULL);

[blundell, 2013/11/19 14:05:37]

Done.

+}
+
+// static
 WebDataServiceFactory* WebDataServiceFactory::GetInstance() {
   return Singleton<WebDataServiceFactory>::get();
 }
 
 content::BrowserContext* WebDataServiceFactory::GetBrowserContextToUse(
     content::BrowserContext* context) const {
   return chrome::GetBrowserContextRedirectedInIncognito(context);
 }
 
 BrowserContextKeyedService* WebDataServiceFactory::BuildServiceInstanceFor(
     content::BrowserContext* profile) const {
   return new WebDataServiceWrapper(static_cast<Profile*>(profile));
 }
 
 bool WebDataServiceFactory::ServiceIsNULLWhileTesting() const {
   return true;
 }