Reduce visibility of methods in AutofillManager and AutofillDownload.

Also remove some obsolete code and simplify some of the code's expectations.

BUG=none
TEST=none


Review URL: http://codereview.chromium.org/8351027

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@107680 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 76 insertions, 138 deletions

diff --git a/chrome/browser/autofill/autofill_download.cc b/chrome/browser/autofill/autofill_download.cc
index 69f151d..529a77b 100644
--- a/chrome/browser/autofill/autofill_download.cc
+++ b/chrome/browser/autofill/autofill_download.cc
@@ -38,23 +38,22 @@ struct AutofillDownloadManager::FormRequestData {
   AutofillRequestType request_type;
 };
 
-AutofillDownloadManager::AutofillDownloadManager(Profile* profile)
+AutofillDownloadManager::AutofillDownloadManager(Profile* profile,
+                                                 Observer* observer)
     : profile_(profile),
-      observer_(NULL),
+      observer_(observer),
       max_form_cache_size_(kMaxFormCacheSize),
       next_query_request_(base::Time::Now()),
       next_upload_request_(base::Time::Now()),
       positive_upload_rate_(0),
       negative_upload_rate_(0),
       fetcher_id_for_unittest_(0) {
-  // |profile_| could be NULL in some unit-tests.
-  if (profile_) {
-    PrefService* preferences = profile_->GetPrefs();
-    positive_upload_rate_ =
-        preferences->GetDouble(prefs::kAutofillPositiveUploadRate);
-    negative_upload_rate_ =
-        preferences->GetDouble(prefs::kAutofillNegativeUploadRate);
-  }
+  DCHECK(observer_);
+  PrefService* preferences = profile_->GetPrefs();
+  positive_upload_rate_ =
+      preferences->GetDouble(prefs::kAutofillPositiveUploadRate);
+  negative_upload_rate_ =
+      preferences->GetDouble(prefs::kAutofillNegativeUploadRate);
 }
 
 AutofillDownloadManager::~AutofillDownloadManager() {
@@ -62,16 +61,6 @@ AutofillDownloadManager::~AutofillDownloadManager() {
                                       url_fetchers_.end());
 }
 
-void AutofillDownloadManager::SetObserver(
-    AutofillDownloadManager::Observer* observer) {
-  if (observer) {
-    DCHECK(!observer_);
-    observer_ = observer;
-  } else {
-    observer_ = NULL;
-  }
-}
-
 bool AutofillDownloadManager::StartQueryRequest(
     const std::vector<FormStructure*>& forms,
     const AutofillMetrics& metric_logger) {
@@ -91,10 +80,9 @@ bool AutofillDownloadManager::StartQueryRequest(
 
   std::string query_data;
   if (CheckCacheForQueryRequest(request_data.form_signatures, &query_data)) {
-    VLOG(1) << "AutofillDownloadManager: query request has been retrieved from"
-            << "the cache";
-    if (observer_)
-      observer_->OnLoadedServerPredictions(query_data);
+    DVLOG(1) << "AutofillDownloadManager: query request has been retrieved from"
+             << "the cache";
+    observer_->OnLoadedServerPredictions(query_data);
     return true;
   }
 
@@ -107,7 +95,7 @@ bool AutofillDownloadManager::StartUploadRequest(
     const FieldTypeSet& available_field_types) {
   if (next_upload_request_ > base::Time::Now()) {
     // We are in back-off mode: do not do the request.
-    VLOG(1) << "AutofillDownloadManager: Upload request is throttled.";
+    DVLOG(1) << "AutofillDownloadManager: Upload request is throttled.";
     return false;
   }
 
@@ -117,7 +105,7 @@ bool AutofillDownloadManager::StartUploadRequest(
   if (form.upload_required() == UPLOAD_NOT_REQUIRED ||
       (form.upload_required() == USE_UPLOAD_RATES &&
        base::RandDouble() > upload_rate)) {
-    VLOG(1) << "AutofillDownloadManager: Upload request is ignored.";
+    DVLOG(1) << "AutofillDownloadManager: Upload request is ignored.";
     // If we ever need notification that upload was skipped, add it here.
     return false;
   }
@@ -134,25 +122,6 @@ bool AutofillDownloadManager::StartUploadRequest(
   return StartRequest(form_xml, request_data);
 }
 
-bool AutofillDownloadManager::CancelRequest(
-    const std::string& form_signature,
-    AutofillDownloadManager::AutofillRequestType request_type) {
-  for (std::map<content::URLFetcher*, FormRequestData>::iterator it =
-       url_fetchers_.begin();
-       it != url_fetchers_.end();
-       ++it) {
-    if (std::find(it->second.form_signatures.begin(),
-        it->second.form_signatures.end(), form_signature) !=
-        it->second.form_signatures.end() &&
-        it->second.request_type == request_type) {
-      delete it->first;
-      url_fetchers_.erase(it);
-      return true;
-    }
-  }
-  return false;
-}
-
 double AutofillDownloadManager::GetPositiveUploadRate() const {
   return positive_upload_rate_;
 }
@@ -167,7 +136,6 @@ void AutofillDownloadManager::SetPositiveUploadRate(double rate) {
   positive_upload_rate_ = rate;
   DCHECK_GE(rate, 0.0);
   DCHECK_LE(rate, 1.0);
-  DCHECK(profile_);
   PrefService* preferences = profile_->GetPrefs();
   preferences->SetDouble(prefs::kAutofillPositiveUploadRate, rate);
 }
@@ -178,7 +146,6 @@ void AutofillDownloadManager::SetNegativeUploadRate(double rate) {
   negative_upload_rate_ = rate;
   DCHECK_GE(rate, 0.0);
   DCHECK_LE(rate, 1.0);
-  DCHECK(profile_);
   PrefService* preferences = profile_->GetPrefs();
   preferences->SetDouble(prefs::kAutofillNegativeUploadRate, rate);
 }
@@ -307,23 +274,20 @@ void AutofillDownloadManager::OnURLFetchComplete(
       }
     }
 
-    LOG(WARNING) << "AutofillDownloadManager: " << type_of_request
-                 << " request has failed with response "
-                 << source->GetResponseCode();
-    if (observer_) {
-      observer_->OnServerRequestError(it->second.form_signatures[0],
-                                      it->second.request_type,
-                                      source->GetResponseCode());
-    }
+    DVLOG(1) << "AutofillDownloadManager: " << type_of_request
+             << " request has failed with response "
+             << source->GetResponseCode();
+    observer_->OnServerRequestError(it->second.form_signatures[0],
+                                    it->second.request_type,
+                                    source->GetResponseCode());
   } else {
-    VLOG(1) << "AutofillDownloadManager: " << type_of_request
-            << " request has succeeded";
+    DVLOG(1) << "AutofillDownloadManager: " << type_of_request
+             << " request has succeeded";
     std::string response_body;
     source->GetResponseAsString(&response_body);
     if (it->second.request_type == AutofillDownloadManager::REQUEST_QUERY) {
       CacheQueryRequest(it->second.form_signatures, response_body);
-      if (observer_)
-        observer_->OnLoadedServerPredictions(response_body);
+      observer_->OnLoadedServerPredictions(response_body);
     } else {
       double new_positive_upload_rate = 0;
       double new_negative_upload_rate = 0;
@@ -336,8 +300,7 @@ void AutofillDownloadManager::OnURLFetchComplete(
         SetNegativeUploadRate(new_negative_upload_rate);
       }
 
-      if (observer_)
-        observer_->OnUploadedPossibleFieldTypes();
+      observer_->OnUploadedPossibleFieldTypes();
     }
   }
   delete it->first;
diff --git a/chrome/browser/autofill/autofill_download.h b/chrome/browser/autofill/autofill_download.h
index ce5aa95..6144b90 100644
--- a/chrome/browser/autofill/autofill_download.h
+++ b/chrome/browser/autofill/autofill_download.h
@@ -37,33 +37,31 @@ class AutofillDownloadManager : public content::URLFetcherDelegate {
   };
 
   // An interface used to notify clients of AutofillDownloadManager.
-  // Notifications are *not* guaranteed to be called.
   class Observer {
    public:
     // Called when field type predictions are successfully received from the
-    // server.
-    // |response_xml| - server response.
+    // server.  |response_xml| contains the server response.
     virtual void OnLoadedServerPredictions(const std::string& response_xml) = 0;
+
+    // These notifications are used to help with testing.
     // Called when heuristic either successfully considered for upload and
     // not send or uploaded.
-    virtual void OnUploadedPossibleFieldTypes() = 0;
+    virtual void OnUploadedPossibleFieldTypes() {}
     // Called when there was an error during the request.
     // |form_signature| - the signature of the requesting form.
     // |request_type| - type of request that failed.
     // |http_error| - HTTP error code.
     virtual void OnServerRequestError(const std::string& form_signature,
                                       AutofillRequestType request_type,
-                                      int http_error) = 0;
+                                      int http_error) {}
+
    protected:
     virtual ~Observer() {}
   };
 
-  // |profile| can be NULL in unit-tests only.
-  explicit AutofillDownloadManager(Profile* profile);
-  virtual ~AutofillDownloadManager();
-
   // |observer| - observer to notify on successful completion or error.
-  void SetObserver(AutofillDownloadManager::Observer* observer);
+  AutofillDownloadManager(Profile* profile, Observer* observer);
+  virtual ~AutofillDownloadManager();
 
   // Starts a query request to Autofill servers. The observer is called with the
   // list of the fields of all requested forms.
@@ -82,14 +80,6 @@ class AutofillDownloadManager : public content::URLFetcherDelegate {
                           bool form_was_autofilled,
                           const FieldTypeSet& available_field_types);
 
-  // Cancels pending request.
-  // |form_signature| - signature of the form being cancelled. Warning:
-  // for query request if more than one form sent in the request, all other
-  // forms will be cancelled as well.
-  // |request_type| - type of the request.
-  bool CancelRequest(const std::string& form_signature,
-                     AutofillRequestType request_type);
-
  private:
   friend class AutofillDownloadTest;
   FRIEND_TEST_ALL_PREFIXES(AutofillDownloadTest, QueryAndUploadTest);
@@ -136,14 +126,18 @@ class AutofillDownloadManager : public content::URLFetcherDelegate {
   void SetPositiveUploadRate(double rate);
   void SetNegativeUploadRate(double rate);
 
-  // Profile for preference storage.
-  Profile* profile_;
+  // Profile for preference storage.  The pointer value is const, so this can
+  // only be set in the constructor.  Must not be null.
+  Profile* const profile_;  // 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.
+  AutofillDownloadManager::Observer* const observer_;  // WEAK
 
   // For each requested form for both query and upload we create a separate
   // request and save its info. As url fetcher is identified by its address
   // we use a map between fetchers and info.
   std::map<content::URLFetcher*, FormRequestData> url_fetchers_;
-  AutofillDownloadManager::Observer *observer_;
 
   // Cached QUERY requests.
   QueryRequestCache cached_forms_;
diff --git a/chrome/browser/autofill/autofill_download_unittest.cc b/chrome/browser/autofill/autofill_download_unittest.cc
index 325a3da..ad76e1f 100644
--- a/chrome/browser/autofill/autofill_download_unittest.cc
+++ b/chrome/browser/autofill/autofill_download_unittest.cc
@@ -64,7 +64,7 @@ class AutofillDownloadTest : public AutofillDownloadManager::Observer,
                              public testing::Test {
  public:
   AutofillDownloadTest()
-      : download_manager_(&profile_),
+      : download_manager_(&profile_, this),
         io_thread_(BrowserThread::IO) {
   }
 
@@ -73,13 +73,11 @@ class AutofillDownloadTest : public AutofillDownloadManager::Observer,
     options.message_loop_type = MessageLoop::TYPE_IO;
     io_thread_.StartWithOptions(options);
     profile_.CreateRequestContext();
-    download_manager_.SetObserver(this);
   }
 
   virtual void TearDown() {
     profile_.ResetRequestContext();
     io_thread_.Stop();
-    download_manager_.SetObserver(NULL);
   }
 
   void LimitCache(size_t cache_size) {
@@ -87,14 +85,15 @@ class AutofillDownloadTest : public AutofillDownloadManager::Observer,
   }
 
   // AutofillDownloadManager::Observer implementation.
-  virtual void OnLoadedServerPredictions(const std::string& response_xml) {
+  virtual void OnLoadedServerPredictions(
+      const std::string& response_xml) OVERRIDE {
     ResponseData response;
     response.response = response_xml;
     response.type_of_response = QUERY_SUCCESSFULL;
     responses_.push_back(response);
   }
 
-  virtual void OnUploadedPossibleFieldTypes() {
+  virtual void OnUploadedPossibleFieldTypes() OVERRIDE {
     ResponseData response;
     response.type_of_response = UPLOAD_SUCCESSFULL;
     responses_.push_back(response);
@@ -103,7 +102,7 @@ class AutofillDownloadTest : public AutofillDownloadManager::Observer,
   virtual void OnServerRequestError(
       const std::string& form_signature,
       AutofillDownloadManager::AutofillRequestType request_type,
-      int http_error) {
+      int http_error) OVERRIDE {
     ResponseData response;
     response.signature = form_signature;
     response.error = http_error;
@@ -113,7 +112,7 @@ class AutofillDownloadTest : public AutofillDownloadManager::Observer,
     responses_.push_back(response);
   }
 
-  enum TYPE_OF_RESPONSE {
+  enum ResponseType {
     QUERY_SUCCESSFULL,
     UPLOAD_SUCCESSFULL,
     REQUEST_QUERY_FAILED,
@@ -121,12 +120,12 @@ class AutofillDownloadTest : public AutofillDownloadManager::Observer,
   };
 
   struct ResponseData {
-    TYPE_OF_RESPONSE type_of_response;
+    ResponseType type_of_response;
     int error;
     std::string signature;
     std::string response;
-    ResponseData() : type_of_response(REQUEST_QUERY_FAILED), error(0) {
-    }
+
+    ResponseData() : type_of_response(REQUEST_QUERY_FAILED), error(0) {}
   };
   std::list<ResponseData> responses_;
 
diff --git a/chrome/browser/autofill/autofill_manager.cc b/chrome/browser/autofill/autofill_manager.cc
index 3fbbc99..0bc082a 100644
--- a/chrome/browser/autofill/autofill_manager.cc
+++ b/chrome/browser/autofill/autofill_manager.cc
@@ -208,7 +208,7 @@ AutofillManager::AutofillManager(TabContentsWrapper* tab_contents)
     : TabContentsObserver(tab_contents->tab_contents()),
       tab_contents_wrapper_(tab_contents),
       personal_data_(NULL),
-      download_manager_(tab_contents->profile()),
+      download_manager_(tab_contents->profile(), this),
       disable_download_manager_requests_(false),
       metric_logger_(new AutofillMetrics),
       has_logged_autofill_enabled_(false),
@@ -222,11 +222,9 @@ AutofillManager::AutofillManager(TabContentsWrapper* tab_contents)
   // |personal_data_| is NULL when using TestTabContents.
   personal_data_ = PersonalDataManagerFactory::GetForProfile(
       tab_contents->profile()->GetOriginalProfile());
-  download_manager_.SetObserver(this);
 }
 
 AutofillManager::~AutofillManager() {
-  download_manager_.SetObserver(NULL);
 }
 
 // static
@@ -618,15 +616,6 @@ void AutofillManager::OnLoadedServerPredictions(
   SendAutofillTypePredictions(form_structures_.get());
 }
 
-void AutofillManager::OnUploadedPossibleFieldTypes() {
-}
-
-void AutofillManager::OnServerRequestError(
-    const std::string& form_signature,
-    AutofillDownloadManager::AutofillRequestType request_type,
-    int http_error) {
-}
-
 bool AutofillManager::IsAutofillEnabled() const {
   Profile* profile = Profile::FromBrowserContext(
       const_cast<AutofillManager*>(this)->tab_contents()->browser_context());
@@ -736,7 +725,7 @@ AutofillManager::AutofillManager(TabContentsWrapper* tab_contents,
     : TabContentsObserver(tab_contents->tab_contents()),
       tab_contents_wrapper_(tab_contents),
       personal_data_(personal_data),
-      download_manager_(NULL),
+      download_manager_(tab_contents->profile(), this),
       disable_download_manager_requests_(true),
       metric_logger_(new AutofillMetrics),
       has_logged_autofill_enabled_(false),
diff --git a/chrome/browser/autofill/autofill_manager.h b/chrome/browser/autofill/autofill_manager.h
index 80c4ce0..e4fd014 100644
--- a/chrome/browser/autofill/autofill_manager.h
+++ b/chrome/browser/autofill/autofill_manager.h
@@ -54,51 +54,26 @@ class AutofillManager : public TabContentsObserver,
   // Registers our Enable/Disable Autofill pref.
   static void RegisterUserPrefs(PrefService* prefs);
 
-  // TabContentsObserver implementation.
-  virtual void DidNavigateMainFramePostCommit(
-      const content::LoadCommittedDetails& details,
-      const ViewHostMsg_FrameNavigate_Params& params);
-  virtual bool OnMessageReceived(const IPC::Message& message);
-
-  // AutofillDownloadManager::Observer implementation:
-  virtual void OnLoadedServerPredictions(const std::string& response_xml);
-  virtual void OnUploadedPossibleFieldTypes();
-  virtual void OnServerRequestError(
-      const std::string& form_signature,
-      AutofillDownloadManager::AutofillRequestType request_type,
-      int http_error);
-
-  // Returns the value of the AutofillEnabled pref.
-  virtual bool IsAutofillEnabled() const;
-
-  // Imports the form data, submitted by the user, into |personal_data_|.
-  void ImportFormData(const FormStructure& submitted_form);
-
-  // Uploads the form data to the Autofill server.
-  virtual void UploadFormData(const FormStructure& submitted_form);
-
-  // Reset cache.
-  void Reset();
-
  protected:
-  // For tests:
+  // Only test code should subclass AutofillManager.
 
   // The string/int pair is composed of the guid string and variant index
   // respectively.  The variant index is an index into the multi-valued item
   // (where applicable).
   typedef std::pair<std::string, size_t> GUIDPair;
 
+  // Test code should prefer to use this constructor.
   AutofillManager(TabContentsWrapper* tab_contents,
                   PersonalDataManager* personal_data);
 
-  void set_personal_data_manager(PersonalDataManager* personal_data) {
-    personal_data_ = personal_data;
-  }
+  // Returns the value of the AutofillEnabled pref.
+  virtual bool IsAutofillEnabled() const;
 
-  const AutofillMetrics* metric_logger() const { return metric_logger_.get(); }
-  void set_metric_logger(const AutofillMetrics* metric_logger);
+  // Uploads the form data to the Autofill server.
+  virtual void UploadFormData(const FormStructure& submitted_form);
 
-  ScopedVector<FormStructure>* form_structures() { return &form_structures_; }
+  // Reset cache.
+  void Reset();
 
   // Maps GUIDs to and from IDs that are used to identify profiles and credit
   // cards sent to and from the renderer process.
@@ -110,7 +85,22 @@ class AutofillManager : public TabContentsObserver,
   int PackGUIDs(const GUIDPair& cc_guid, const GUIDPair& profile_guid) const;
   void UnpackGUIDs(int id, GUIDPair* cc_guid, GUIDPair* profile_guid) const;
 
+  const AutofillMetrics* metric_logger() const { return metric_logger_.get(); }
+  void set_metric_logger(const AutofillMetrics* metric_logger);
+
+  ScopedVector<FormStructure>* form_structures() { return &form_structures_; }
+
  private:
+  // TabContentsObserver:
+  virtual void DidNavigateMainFramePostCommit(
+      const content::LoadCommittedDetails& details,
+      const ViewHostMsg_FrameNavigate_Params& params) OVERRIDE;
+  virtual bool OnMessageReceived(const IPC::Message& message) OVERRIDE;
+
+  // AutofillDownloadManager::Observer:
+  virtual void OnLoadedServerPredictions(
+      const std::string& response_xml) OVERRIDE;
+
   void OnFormSubmitted(const webkit_glue::FormData& form,
                        const base::TimeTicks& timestamp);
   void OnFormsSeen(const std::vector<webkit_glue::FormData>& forms,
@@ -217,6 +207,9 @@ class AutofillManager : public TabContentsObserver,
   // |submitted_form|.
   void DeterminePossibleFieldTypesForUpload(FormStructure* submitted_form);
 
+  // Imports the form data, submitted by the user, into |personal_data_|.
+  void ImportFormData(const FormStructure& submitted_form);
+
   // If |initial_interaction_timestamp_| is unset or is set to a later time than
   // |interaction_timestamp|, updates the cached timestamp.  The latter check is
   // needed because IPC messages can arrive out of order.