Make code for the AutomaticProfileResetter more readabile (readability review)

chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h

 // Copyright 2013 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.
 
+// Declares a delegate that interacts with the rest of the browser on behalf of
+// the AutomaticProfileResetter.
+//
+// The reason for this separation is to facilitate unit testing. On one hand,
+// factoring out the implementation for each interaction step (encapsulated by
+// one method of the delegate) allows it to be tested in itself, independently.
+// On the other hand, it also becomes easier to verify that the state machine

[Peter Kasting, 2014/05/28 18:46:32]

"On one/the other hand" implies two arguments that counter each other.  In this
case you're just giving two arguments that both suggest factoring things out is
a good idea, so don't use this idiom.

[engedy, 2014/05/30 14:38:47]

Fixed. Thanks for pointing this out!

+// inside AutomaticProfileResetter works correctly: by mocking out interaction
+// methods in the delegate, we can effectively mock out the entire rest of the
+// browser, allowing us to only simulate the scenarios that are of interest for
+// testing the state machine.
+//
+// The delegate is normally instantiated by AutomaticProfileResetter internally,
+// while a mock implementation can be injected during unit tests.
+
 #ifndef CHROME_BROWSER_PROFILE_RESETTER_AUTOMATIC_PROFILE_RESETTER_DELEGATE_H_
 #define CHROME_BROWSER_PROFILE_RESETTER_AUTOMATIC_PROFILE_RESETTER_DELEGATE_H_
 
 #include "base/basictypes.h"
 #include "base/callback_forward.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/profile_resetter/profile_resetter.h"
 #include "chrome/browser/search_engines/template_url_service_observer.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "extensions/common/one_shot_event.h"
 
 class BrandcodeConfigFetcher;
 class GlobalErrorService;
 class Profile;
 class ResettableSettingsSnapshot;
 class TemplateURLService;
 
 namespace base {
 class DictionaryValue;
 class ListValue;
 }
 
 // Defines the interface for the delegate that will interact with the rest of
 // the browser on behalf of the AutomaticProfileResetter.
-// The primary reason for this separation is to facilitate unit testing.
 class AutomaticProfileResetterDelegate {
  public:
   virtual ~AutomaticProfileResetterDelegate() {}
 
   // Requests the module enumerator to start scanning for loaded modules now, if
   // it has not done so already.
   virtual void EnumerateLoadedModulesIfNeeded() = 0;
 
   // Requests |ready_callback| to be posted on the UI thread once the module
   // enumerator has finished scanning for loaded modules.
@@ skipping 125 matching lines @@
   void OnBrandcodedDefaultsFetched();
 
   // Called back by the ProfileResetter once resetting the profile settings has
   // been completed. If |old_settings_snapshot| is non-NULL, will compare it to
   // the new settings and send the differences to Google for analysis. Finally,
   // will post |user_callback|.
   void OnProfileSettingsResetCompleted(
       const base::Closure& user_callback,
       scoped_ptr<ResettableSettingsSnapshot> old_settings_snapshot);
 
+  // The profile that this delegate operates on.
   Profile* profile_;
+
+  // Shortcuts to |profile_| keyed services, to reduce boilerplate. These may be
+  // NULL in unit tests that do not initialize the respective service(s).
   GlobalErrorService* global_error_service_;
   TemplateURLService* template_url_service_;
 
+  // Helper to asynchronously download the default settings for the current
+  // distribution channel (identified by brand code). Instantiated on-demand.
   scoped_ptr<BrandcodeConfigFetcher> brandcoded_config_fetcher_;
+
+  // Once |brandcoded_defaults_fetched_event_| has fired, this will contain the
+  // brandcoded default settings, or empty settings for a non-branded build.
   scoped_ptr<BrandcodedDefaultSettings> brandcoded_defaults_;
 
+  // Overwritten to avoid resetting aspects that will not work in unit tests.
   const ProfileResetter::ResettableFlags resettable_aspects_;
+
+  // The profile resetter to perform the actual reset. Instantiated on-demand.
   scoped_ptr<ProfileResetter> profile_resetter_;
 
+  // Manages registrations/unregistrations for notifications.
   content::NotificationRegistrar registrar_;
 
   // The list of modules found. Even when |modules_have_been_enumerated_event_|
   // is signaled, this may still be NULL.
   scoped_ptr<base::ListValue> module_list_;
 
   // This event is signaled once module enumeration has been attempted. If
   // during construction, EnumerateModulesModel can supply a non-empty list of
   // modules, module enumeration has clearly already happened, so the event will
   // be signaled immediately; otherwise, when EnumerateLoadedModulesIfNeeded()
   // is called, it will ask the model to scan the modules, and then signal the
   // event once this process is completed.
   extensions::OneShotEvent modules_have_been_enumerated_event_;
 
   // This event is signaled once the TemplateURLService has loaded.  If the
   // TemplateURLService was already loaded prior to the creation of this class,
   // the event will be signaled during construction.
   extensions::OneShotEvent template_url_service_ready_event_;
 
   // This event is signaled once brandcoded default settings have been fetched,
   // or once it has been established that this is not a branded build.
   extensions::OneShotEvent brandcoded_defaults_fetched_event_;
 
   DISALLOW_COPY_AND_ASSIGN(AutomaticProfileResetterDelegateImpl);
 };
 
 #endif  // CHROME_BROWSER_PROFILE_RESETTER_AUTOMATIC_PROFILE_RESETTER_DELEGATE_H_
+

[Peter Kasting, 2014/05/28 18:46:32]

Nit: Don't add a trailing blank line

[engedy, 2014/05/30 14:38:47]

Done.