Parameterize the PrefService that AutofillDownloadManager uses.

Parameterize the PrefService that AutofillDownloadManager uses.

Rather than AutofillDownloadManager obtaining the PrefService to use from
BrowserContext, have AutofillDownloadManager's creator supply it with the
PrefService to use. Incremental step toward abstracting BrowserContext
knowledge out of AutofillDownloadManager.

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

[blundell, 2013-11-04 10:49:43]

Dependent on https://codereview.chromium.org/52713006/ (just for the DEPS
restriction part). Thanks!

[Jói, 2013-11-04 10:52:29]

LGTM


On Mon, Nov 4, 2013 at 10:49 AM, <blundell@chromium.org> wrote:

> Reviewers: Jói, Ilya Sherman,
>
> Message:
> Dependent on https://codereview.chromium.org/52713006/ (just for the DEPS
> restriction part). Thanks!
>
> Description:
> Parameterize the PrefService that AutofillDownloadManager uses.
>
> Rather than AutofillDownloadManager obtaining the PrefService to use from
> BrowserContext, have AutofillDownloadManager's creator supply it with the
> PrefService to use. Incremental step toward abstracting BrowserContext
> knowledge out of AutofillDownloadManager.
>
> BUG=303050
>
> Please review this at https://codereview.chromium.org/49303005/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+30, -12 lines):
>   M chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc
>   M components/autofill/DEPS
>   M components/autofill/core/browser/DEPS
>   M components/autofill/core/browser/autofill_download.h
>   M components/autofill/core/browser/autofill_download.cc
>   M components/autofill/core/browser/autofill_download_unittest.cc
>   M components/autofill/core/browser/autofill_manager.cc
>
>
> Index: chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc
> diff --git a/chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc
> b/chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc
> index 057f0a7eab45e939be705b9479292236f201eed9..
> f442d3c5f33696da6e6656b6e834f62fc2b2eb1a 100644
> --- a/chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc
> +++ b/chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc
> @@ -4,11 +4,11 @@
>
>  #include "base/memory/scoped_ptr.h"
>  #include "base/memory/weak_ptr.h"
> -#include "base/prefs/testing_pref_service.h"
>  #include "base/strings/utf_string_conversions.h"
>  #include "chrome/browser/ui/autofill/autofill_popup_controller_impl.h"
>  #include "chrome/browser/ui/autofill/autofill_popup_view.h"
>  #include "chrome/test/base/chrome_render_view_host_test_harness.h"
> +#include "chrome/test/base/testing_pref_service_syncable.h"
>  #include "chrome/test/base/testing_profile.h"
>  #include "components/autofill/content/browser/autofill_driver_impl.h"
>  #include "components/autofill/core/browser/autofill_external_delegate.h"
> @@ -56,8 +56,12 @@ class MockAutofillManagerDelegate
>
>    virtual PrefService* GetPrefs() OVERRIDE { return &prefs_; }
>
> +  user_prefs::PrefRegistrySyncable* GetPrefRegistry() {
> +    return prefs_.registry();
> +  }
> +
>   private:
> -  TestingPrefServiceSimple prefs_;
> +  TestingPrefServiceSyncable prefs_;
>
>    DISALLOW_COPY_AND_ASSIGN(MockAutofillManagerDelegate);
>  };
> @@ -127,6 +131,8 @@ class AutofillPopupControllerUnitTest : public
> ChromeRenderViewHostTestHarness {
>    virtual void SetUp() OVERRIDE {
>      ChromeRenderViewHostTestHarness::SetUp();
>
> +    AutofillManager::RegisterProfilePrefs(manager_
> delegate_->GetPrefRegistry());
> +
>      AutofillDriverImpl::CreateForWebContentsAndDelegate(
>          web_contents(),
>          manager_delegate_.get(),
> Index: components/autofill/DEPS
> diff --git a/components/autofill/DEPS b/components/autofill/DEPS
> index e136eaf419bcbd793f3d273c7716923686401f92..
> 02b4339b90535d0c2db940f317625af14ccad1f7 100644
> --- a/components/autofill/DEPS
> +++ b/components/autofill/DEPS
> @@ -7,5 +7,8 @@ include_rules = [
>    # Autofill is a layered component; subdirectories must explicitly
> introduce
>    # the ability to use the content layer as appropriate.
>    "-components/autofill/content",
> +  # TODO(blundell): This subtraction can be eliminated once
> crbug.com/314560 is
> +  # fixed.
> +  "-components/user_prefs/user_prefs.h",
>    "-content/public/common",
>  ]
> Index: components/autofill/core/browser/DEPS
> diff --git a/components/autofill/core/browser/DEPS
> b/components/autofill/core/browser/DEPS
> index 01b9af29635be8c883262efd9fe117da19f09c72..
> 1ec7b000a8f39405283953056af91f2bae2e570d 100644
> --- a/components/autofill/core/browser/DEPS
> +++ b/components/autofill/core/browser/DEPS
> @@ -29,6 +29,7 @@ specific_include_rules = {
>      #
>      # Do not add to the list of temporarily-allowed dependencies below,
>      # and please do not introduce more #includes of these files.
> +    "!components/user_prefs/user_prefs.h",
>      "!content/public/browser/browser_thread.h",
>      "!content/public/test",
>
> Index: components/autofill/core/browser/autofill_download.cc
> diff --git a/components/autofill/core/browser/autofill_download.cc
> b/components/autofill/core/browser/autofill_download.cc
> index 9bf81e2c72bbcb71c64492e642ff9cee3f67eb06..
> 994db7161eac34ae63ffd210af2208777b1080a0 100644
> --- a/components/autofill/core/browser/autofill_download.cc
> +++ b/components/autofill/core/browser/autofill_download.cc
> @@ -18,7 +18,6 @@
>  #include "components/autofill/core/browser/autofill_xml_parser.h"
>  #include "components/autofill/core/browser/form_structure.h"
>  #include "components/autofill/core/common/autofill_pref_names.h"
> -#include "components/user_prefs/user_prefs.h"
>  #include "content/public/browser/browser_context.h"
>  #include "net/base/load_flags.h"
>  #include "net/http/http_response_headers.h"
> @@ -70,8 +69,10 @@ struct AutofillDownloadManager::FormRequestData {
>  };
>
>  AutofillDownloadManager::AutofillDownloadManager(BrowserContext* context,
> +                                                 PrefService*
> pref_service,
>                                                   Observer* observer)
>      : browser_context_(context),
> +      pref_service_(pref_service),
>        observer_(observer),
>        max_form_cache_size_(kMaxFormCacheSize),
>        next_query_request_(base::Time::Now()),
> @@ -80,11 +81,10 @@
AutofillDownloadManager::AutofillDownloadManager(BrowserContext*
> context,
>        negative_upload_rate_(0),
>        fetcher_id_for_unittest_(0) {
>    DCHECK(observer_);
> -  PrefService* preferences = user_prefs::UserPrefs::Get(
> browser_context_);
>    positive_upload_rate_ =
> -      preferences->GetDouble(prefs::kAutofillPositiveUploadRate);
> +      pref_service_->GetDouble(prefs::kAutofillPositiveUploadRate);
>    negative_upload_rate_ =
> -      preferences->GetDouble(prefs::kAutofillNegativeUploadRate);
> +      pref_service_->GetDouble(prefs::kAutofillNegativeUploadRate);
>  }
>
>  AutofillDownloadManager::~AutofillDownloadManager() {
> @@ -170,8 +170,7 @@ void
AutofillDownloadManager::SetPositiveUploadRate(double
> rate) {
>    positive_upload_rate_ = rate;
>    DCHECK_GE(rate, 0.0);
>    DCHECK_LE(rate, 1.0);
> -  PrefService* preferences = user_prefs::UserPrefs::Get(
> browser_context_);
> -  preferences->SetDouble(prefs::kAutofillPositiveUploadRate, rate);
> +  pref_service_->SetDouble(prefs::kAutofillPositiveUploadRate, rate);
>  }
>
>  void AutofillDownloadManager::SetNegativeUploadRate(double rate) {
> @@ -180,8 +179,7 @@ void
AutofillDownloadManager::SetNegativeUploadRate(double
> rate) {
>    negative_upload_rate_ = rate;
>    DCHECK_GE(rate, 0.0);
>    DCHECK_LE(rate, 1.0);
> -  PrefService* preferences = user_prefs::UserPrefs::Get(
> browser_context_);
> -  preferences->SetDouble(prefs::kAutofillNegativeUploadRate, rate);
> +  pref_service_->SetDouble(prefs::kAutofillNegativeUploadRate, rate);
>  }
>
>  bool AutofillDownloadManager::StartRequest(
> Index: components/autofill/core/browser/autofill_download.h
> diff --git a/components/autofill/core/browser/autofill_download.h
> b/components/autofill/core/browser/autofill_download.h
> index bba1d7197629883d732c07f2c56620984c8faee2..
> 23d2fad7affd58e3e6598ef80bae2036751673e3 100644
> --- a/components/autofill/core/browser/autofill_download.h
> +++ b/components/autofill/core/browser/autofill_download.h
> @@ -18,6 +18,8 @@
>  #include "components/autofill/core/browser/autofill_type.h"
>  #include "net/url_request/url_fetcher_delegate.h"
>
> +class PrefService;
> +
>  namespace content {
>  class BrowserContext;
>  }  // namespace content
> @@ -62,8 +64,10 @@ class AutofillDownloadManager : public
> net::URLFetcherDelegate {
>      virtual ~Observer() {}
>    };
>
> +  // |context| and |pref_service| must outlive this instance.
>    // |observer| - observer to notify on successful completion or error.
>    AutofillDownloadManager(content::BrowserContext* context,
> +                          PrefService* pref_service,
>                            Observer* observer);
>    virtual ~AutofillDownloadManager();
>
> @@ -136,6 +140,10 @@ class AutofillDownloadManager : public
> net::URLFetcherDelegate {
>    // constructor.  Must not be null.
>    content::BrowserContext* const browser_context_;  // WEAK
>
> +  // The pointer value is const, so this can only be set in the
> +  // constructor.  Must not be null, and must outlive this instance.
> +  PrefService* const pref_service_;  // WEAK
> +
>    // The observer to notify when server predictions are successfully
> received.
>    // The pointer value is const, so this can only be set in the
> constructor.
>    // Must not be null.
> Index: components/autofill/core/browser/autofill_download_unittest.cc
> diff --git a/components/autofill/core/browser/autofill_download_unittest.cc
> b/components/autofill/core/browser/autofill_download_unittest.cc
> index 834772f9d04e09e11926e7a43d423333deecd756..
> bbef25b708f32c27dac162071b3da5364b13d7f3 100644
> --- a/components/autofill/core/browser/autofill_download_unittest.cc
> +++ b/components/autofill/core/browser/autofill_download_unittest.cc
> @@ -64,7 +64,7 @@ class AutofillDownloadTest : public
> AutofillDownloadManager::Observer,
>                               public testing::Test {
>   public:
>    AutofillDownloadTest()
> -      : download_manager_(&profile_, this) {
> +      : download_manager_(&profile_, profile_.GetPrefs(), this) {
>    }
>
>    void LimitCache(size_t cache_size) {
> Index: components/autofill/core/browser/autofill_manager.cc
> diff --git a/components/autofill/core/browser/autofill_manager.cc
> b/components/autofill/core/browser/autofill_manager.cc
> index 4c76295dfdeae63d9fe86cda5bf10f1e60775339..
> 5e8539f393253226111be0ce4b20d6a9369352ef 100644
> --- a/components/autofill/core/browser/autofill_manager.cc
> +++ b/components/autofill/core/browser/autofill_manager.cc
> @@ -202,7 +202,9 @@ AutofillManager::AutofillManager(
>    if (enable_download_manager == ENABLE_AUTOFILL_DOWNLOAD_MANAGER) {
>      download_manager_.reset(
>          new AutofillDownloadManager(
> -            driver->GetWebContents()->GetBrowserContext(), this));
> +            driver->GetWebContents()->GetBrowserContext(),
> +            manager_delegate_->GetPrefs(),
> +            this));
>    }
>  }
>
>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ilya Sherman, 2013-11-05 00:17:03]

Tentative LGTM, but I'd like to make sure that we don't add unnecessary
complexity to the autofill_popup_controller_unittest.  Please either revert the
changes to that file; or if not possible, reply on this code review why they are
necessary.  If they are necessary feel free to go ahead and commit this CL
anyway; I can follow up with you on that change separately, so that your main
work isn't blocked.

https://codereview.chromium.org/49303005/diff/30001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc (right):

https://codereview.chromium.org/49303005/diff/30001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:64:
TestingPrefServiceSyncable prefs_;
Hmm, why is this change needed here, but not for the autofill_download_unittest
file?

https://codereview.chromium.org/49303005/diff/30001/components/autofill/core/...
File components/autofill/core/browser/autofill_download.h (right):

https://codereview.chromium.org/49303005/diff/30001/components/autofill/core/...
components/autofill/core/browser/autofill_download.h:144: // constructor.  Must
not be null, and must outlive this instance.
nit: No need to document what "const" means -- that should be clear from the
code to anyone sufficiently familiar with C++, and easy to research to anyone
who doesn't quite remember what "const" means in this context.  Please instead
document the role or purpose of this data member.  (I see that this was probably
copied from the above and below comments, but IMO those should be shortened as
well; this really is a language detail that ought to be safe to assume as common
knowledge.)

[blundell, 2013-11-05 15:26:53]

Hi Ilya,

Responded to your concern. I'm going to send this through the CQ, but happy to
follow up if you still have concerns.

Thanks,

Colin

https://codereview.chromium.org/49303005/diff/30001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc (right):

https://codereview.chromium.org/49303005/diff/30001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:64:
TestingPrefServiceSyncable prefs_;
It's needed here because this test is using a manually-constructed PrefService
that didn't have AutofillManager::RegisterProfilePrefs() called on its registry,
causing DCHECKs to go off when AutofillDownloadManager uses this PrefService
[note that prior to the refactoring in this CL, AutofillDownloadManager was not
using the MockAutofillManagerDelegate's pref service in this test). In
autofill_download_unittest, the PrefService being used is that of the Profile,
which has had proper registration done for its registry.

Eventually similar changes will have to happen for the core autofill unit tests
in order to get them shared by iOS. I'm not focusing on these challenges as part
of this first pass of getting the core autofill code running cleanly on iOS.
When that time comes, it might make sense to make a helper method in
autofill_test_common that can give the client a PrefService whose registry has
had the Autofil prefs registered.

On 2013/11/05 00:17:04, Ilya Sherman wrote:
> Hmm, why is this change needed here, but not for the
autofill_download_unittest
> file?

https://codereview.chromium.org/49303005/diff/30001/components/autofill/core/...
File components/autofill/core/browser/autofill_download.h (right):

https://codereview.chromium.org/49303005/diff/30001/components/autofill/core/...
components/autofill/core/browser/autofill_download.h:144: // constructor.  Must
not be null, and must outlive this instance.
On 2013/11/05 00:17:04, Ilya Sherman wrote:
> nit: No need to document what "const" means -- that should be clear from the
> code to anyone sufficiently familiar with C++, and easy to research to anyone
> who doesn't quite remember what "const" means in this context.  Please instead
> document the role or purpose of this data member.  (I see that this was
probably
> copied from the above and below comments, but IMO those should be shortened as
> well; this really is a language detail that ought to be safe to assume as
common
> knowledge.)

Done.

[commit-bot: I haz the power, 2013-11-05 15:29:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/49303005/150001

[commit-bot: I haz the power, 2013-11-05 16:28:32]

Retried try job too often on linux_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[blundell, 2013-11-05 17:30:09]

Hi Ilya,

It turns out that I had missed a test (AutocompleteHistoryManagerUnittest) that
also now needed the PrefService that it is using to have had Autofill
preferences registered with its associated registry. I went ahead and created
the helper function that I was discussing in autofill_common_test. What do you
think?

[Ilya Sherman, 2013-11-05 18:50:49]

Thanks, LGTM % nits.

https://codereview.chromium.org/49303005/diff/480001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc (right):

https://codereview.chromium.org/49303005/diff/480001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:14: #include
"components/autofill/core/browser/autofill_common_test.h"
nit: Completely orthogonal to your CL, but at some point we really should rename
this to something more like "autofill_test_common" or "autofill_test_utils",
since this file does not itself contain any test code.

https://codereview.chromium.org/49303005/diff/480001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:57:
MockAutofillManagerDelegate() :
nit: The colon should wrap with the initializer list, so it should be where the
'p' from 'prefs_' currently is.

https://codereview.chromium.org/49303005/diff/480001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:58:
prefs_(autofill::test::PrefServiceForTesting().Pass()) {
nit: I don't think ".Pass()" ought to be required here.  Does the code still
compile without it?

https://codereview.chromium.org/49303005/diff/480001/components/autofill/core...
File components/autofill/core/browser/autocomplete_history_manager_unittest.cc
(right):

https://codereview.chromium.org/49303005/diff/480001/components/autofill/core...
components/autofill/core/browser/autocomplete_history_manager_unittest.cc:87:
prefs_(test::PrefServiceForTesting().Pass()) {
Same comments here as in the other test file.

https://codereview.chromium.org/49303005/diff/480001/components/autofill/core...
File components/autofill/core/browser/autofill_common_test.cc (right):

https://codereview.chromium.org/49303005/diff/480001/components/autofill/core...
components/autofill/core/browser/autofill_common_test.cc:38:
builder.WithUserPrefs(new TestingPrefStore());
nit: Probably best to leave to a separate CL, but it would be nice to update the
PrefServiceBuilder API to accept a scoped_ptr rather than a raw pointer with
implicit ownership transfer.

https://codereview.chromium.org/49303005/diff/480001/components/autofill/core...
File components/autofill/core/browser/autofill_common_test.h (right):

https://codereview.chromium.org/49303005/diff/480001/components/autofill/core...
components/autofill/core/browser/autofill_common_test.h:26: // Returns a
PrefService* that can be used for Autofill-related testing in
nit: "PrefService*" -> "PrefService"

[commit-bot: I haz the power, 2013-11-06 10:31:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/49303005/460002

[blundell, 2013-11-06 10:37:20]

https://codereview.chromium.org/49303005/diff/480001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc (right):

https://codereview.chromium.org/49303005/diff/480001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:14: #include
"components/autofill/core/browser/autofill_common_test.h"
Strongly agreed. Filed crbug.com/315492.

On 2013/11/05 18:50:49, Ilya Sherman wrote:
> nit: Completely orthogonal to your CL, but at some point we really should
rename
> this to something more like "autofill_test_common" or "autofill_test_utils",
> since this file does not itself contain any test code.

https://codereview.chromium.org/49303005/diff/480001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:57:
MockAutofillManagerDelegate() :
On 2013/11/05 18:50:49, Ilya Sherman wrote:
> nit: The colon should wrap with the initializer list, so it should be where
the
> 'p' from 'prefs_' currently is.

Done.

https://codereview.chromium.org/49303005/diff/480001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:58:
prefs_(autofill::test::PrefServiceForTesting().Pass()) {
On 2013/11/05 18:50:49, Ilya Sherman wrote:
> nit: I don't think ".Pass()" ought to be required here.  Does the code still
> compile without it?

Done.

https://codereview.chromium.org/49303005/diff/480001/components/autofill/core...
File components/autofill/core/browser/autocomplete_history_manager_unittest.cc
(right):

https://codereview.chromium.org/49303005/diff/480001/components/autofill/core...
components/autofill/core/browser/autocomplete_history_manager_unittest.cc:87:
prefs_(test::PrefServiceForTesting().Pass()) {
On 2013/11/05 18:50:49, Ilya Sherman wrote:
> Same comments here as in the other test file.

Done.

https://codereview.chromium.org/49303005/diff/480001/components/autofill/core...
File components/autofill/core/browser/autofill_common_test.cc (right):

https://codereview.chromium.org/49303005/diff/480001/components/autofill/core...
components/autofill/core/browser/autofill_common_test.cc:38:
builder.WithUserPrefs(new TestingPrefStore());
Strongly agreed. I filed crbug.com/315499.

On 2013/11/05 18:50:49, Ilya Sherman wrote:
> nit: Probably best to leave to a separate CL, but it would be nice to update
the
> PrefServiceBuilder API to accept a scoped_ptr rather than a raw pointer with
> implicit ownership transfer.

https://codereview.chromium.org/49303005/diff/480001/components/autofill/core...
File components/autofill/core/browser/autofill_common_test.h (right):

https://codereview.chromium.org/49303005/diff/480001/components/autofill/core...
components/autofill/core/browser/autofill_common_test.h:26: // Returns a
PrefService* that can be used for Autofill-related testing in
On 2013/11/05 18:50:49, Ilya Sherman wrote:
> nit: "PrefService*" -> "PrefService"

Done.

[commit-bot: I haz the power, 2013-11-06 12:41:37]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-11-06 12:45:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/49303005/460002

[commit-bot: I haz the power, 2013-11-06 16:26:09]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-11-06 17:01:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/49303005/460002

[commit-bot: I haz the power, 2013-11-06 21:30:55]

Change committed as 233375