Make code for the AutomaticProfileResetter more readabile (readability review)

chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h

Index: chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h
diff --git a/chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h b/chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h
index 94c73ccd1778df8f61970d24d1e5ed15efc58f07..54185bc47007d298dffc3238767fd6a8d57f981d 100644
--- a/chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h
+++ b/chrome/browser/profile_resetter/automatic_profile_resetter_delegate.h
@@ -2,6 +2,21 @@
 // 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_
 
@@ -28,7 +43,6 @@ 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() {}
@@ -174,16 +188,29 @@ class AutomaticProfileResetterDelegateImpl
       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_|
@@ -211,3 +238,4 @@ class 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.