[Win] Add reporting of total number of modules loaded in browser process.

chrome/browser/enumerate_modules_model_win.h

Index: chrome/browser/enumerate_modules_model_win.h
diff --git a/chrome/browser/enumerate_modules_model_win.h b/chrome/browser/enumerate_modules_model_win.h
index d40a81e7d67f09cbd07ff338f5294b04c8f869fa..66e2e78beebc4c751d74304db3ae5765e1b86d82 100644
--- a/chrome/browser/enumerate_modules_model_win.h
+++ b/chrome/browser/enumerate_modules_model_win.h
@@ -8,10 +8,12 @@
 #include <utility>
 #include <vector>
 
+#include "base/callback.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/singleton.h"
+#include "base/observer_list_threadsafe.h"
 #include "base/strings/string16.h"
 #include "base/timer/timer.h"
 #include "content/public/browser/browser_thread.h"
@@ -115,17 +117,6 @@ class ModuleEnumerator : public base::RefCountedThreadSafe<ModuleEnumerator> {
   // A vector typedef of all modules enumerated.
   typedef std::vector<Module> ModulesVector;
 
-  // A structure we populate with the blacklist entries.
-  struct BlacklistEntry {
-    const char* filename;
-    const char* location;
-    const char* desc_or_signer;
-    const char* version_from;  // Version where conflict started.
-    const char* version_to;    // First version that works.
-    OperatingSystem os;  // Bitmask, representing what OS this entry applies to.
-    RecommendedAction help_tip;
-  };
-
   // A static function that normalizes the module information in the |module|
   // struct. Module information needs to be normalized before comparing against
   // the blacklist. This is because the same module can be described in many
@@ -135,10 +126,6 @@ class ModuleEnumerator : public base::RefCountedThreadSafe<ModuleEnumerator> {
   // against the blacklist.
   static void NormalizeModule(Module* module);
 
-  // A static function that checks whether |module| has been |blacklisted|.
-  static ModuleStatus Match(const Module& module,
-                            const BlacklistEntry& blacklisted);
-
   explicit ModuleEnumerator(EnumerateModulesModel* observer);
 
   // Start scanning the loaded module list (if a scan is not already in
@@ -158,9 +145,6 @@ class ModuleEnumerator : public base::RefCountedThreadSafe<ModuleEnumerator> {
   friend class base::RefCountedThreadSafe<ModuleEnumerator>;
   ~ModuleEnumerator();
 
-  // The (currently) hard coded blacklist of known bad modules.
-  static const BlacklistEntry kModuleBlacklist[];
-
   // This function does the actual file scanning work on the FILE thread (or
   // block the main thread when in limited_mode). It enumerates all loaded
   // modules in the process and other modules of interest, such as the
@@ -205,10 +189,6 @@ class ModuleEnumerator : public base::RefCountedThreadSafe<ModuleEnumerator> {
   // based on the |path_mapping_| vector.
   void CollapsePath(Module* module);
 
-  // Takes each module in the |enumerated_modules_| vector and matches it
-  // against a fixed blacklist of bad and suspected bad modules.
-  void MatchAgainstBlacklist();
-
   // This function executes on the UI thread when the scanning and matching
   // process is done. It notifies the observer.
   void ReportBack();
@@ -218,8 +198,10 @@ class ModuleEnumerator : public base::RefCountedThreadSafe<ModuleEnumerator> {
   base::string16 GetSubjectNameFromDigitalSignature(
       const base::FilePath& filename);
 
+  void ReportThirdPartyMetrics();

[grt (UTC plus 2), 2016/06/16 18:27:53]

please add doc comment

[chrisha, 2016/06/17 20:48:03]

Done.

+
   // The typedef for the vector that maps a regular file path to %env_var%.
-  typedef std::vector< std::pair<base::string16, base::string16> > PathMapping;
+  typedef std::vector<std::pair<base::string16, base::string16>> PathMapping;
 
   // The vector of paths to %env_var%, used to account for differences in
   // where people keep there files, c:\windows vs. d:\windows, etc.
@@ -229,7 +211,7 @@ class ModuleEnumerator : public base::RefCountedThreadSafe<ModuleEnumerator> {
   // interest).
   ModulesVector* enumerated_modules_;
 
-  // The observer, who needs to be notified when we are done.
+  // The observers, who need to be notified when the scan is complete.
   EnumerateModulesModel* observer_;

[grt (UTC plus 2), 2016/06/16 18:27:53]

why is the pointer to the model called "observer_"?

[chrisha, 2016/06/17 20:48:02]

The implementation of the enumeration is done by this helper class
(ModuleEnumerator), and it notifies the EnumerateModulesModel when it has
completed. The 'observer' is passed in via the constructor.

This could stand to be cleaned up, but I was hoping to avoid refactoring the
whole world.

 
   // See limited_mode below.
@@ -264,8 +246,37 @@ class EnumerateModulesModel {
     ACTION_BOUNDARY, // Must be the last value.
   };
 
+  // Observer class used to receive the list of modules when enumeration is
+  // finished.
+  class Observer {

[grt (UTC plus 2), 2016/06/16 18:27:53]

the following suggestions are for consistency with observers in content/public
that do not carry behavior

[chrisha, 2016/06/17 20:48:03]

Done.

+   public:
+    Observer() {}

[grt (UTC plus 2), 2016/06/16 18:27:53]

omit the ctor

[chrisha, 2016/06/17 20:48:02]

Done.

+    virtual ~Observer() {}

[grt (UTC plus 2), 2016/06/16 18:27:53]

move dtor into "protected:" block i think " = default;" is a good choice here as
well

[chrisha, 2016/06/17 20:48:03]

Done.

+
+    // Invoked when EnumerateModulesModel has completed a scan of modules.
+    virtual void OnScanCompleted(bool limited_mode);

[grt (UTC plus 2), 2016/06/16 18:27:53]

inline the empty methods with "{}"

[chrisha, 2016/06/17 20:48:03]

Done.

+
+    // Invoked when a user has acknowledge incompatible modules found in a
+    // module scan.
+    virtual void OnConflictsAcknowledged();
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(Observer);

[grt (UTC plus 2), 2016/06/16 18:27:53]

omit this

[chrisha, 2016/06/17 20:48:03]

Done.

+  };
+
   static EnumerateModulesModel* GetInstance();
 
+  // Adds an |observer| to the enumerator. May be called from any thread. The
+  // callback will be invoked on the thread on which AddObserver was called.
+  void AddObserver(Observer* observer) {
+    observers_->AddObserver(observer);
+  }
+
+  // Removes an |observer| from the enumerator. May be called from any thread.
+  void RemoveObserver(Observer* observer) {
+    observers_->RemoveObserver(observer);
+  }
+
   // Returns true if we should show the conflict notification. The conflict
   // notification is only shown once during the lifetime of the process.
   bool ShouldShowConflictWarning() const;
@@ -306,9 +317,11 @@ class EnumerateModulesModel {
   // Gets the whole module list as a ListValue.
   base::ListValue* GetModuleList() const;
 
-  // Gets the Help Center URL for the first *notable* conflict module that we've
-  // elected to notify the user about.
-  GURL GetFirstNotableConflict();
+  // The user will be taken to this site when the conflict bubble or app menu

[grt (UTC plus 2), 2016/06/16 18:27:53]

nit:
  // Returns the site to which the user should be taken when...
as per https://google.github.io/styleguide/cppguide.html#Function_Comments

[chrisha, 2016/06/17 20:48:03]

Done.

+  // item is clicked. For now this simply opens chrome://conflicts, which
+  // contains detailed information about conflicts. Returns an empty URL if
+  // there are no conficts.
+  GURL GetConflictUrl();
 
  private:
   friend struct base::DefaultSingletonTraits<EnumerateModulesModel>;
@@ -320,11 +333,6 @@ class EnumerateModulesModel {
   // Called on the UI thread when the helper class is done scanning.
   void DoneScanning();
 
-  // Constructs a Help Center article URL for help with a particular module.
-  // The module must have the SEE_LINK attribute for |recommended_action| set,
-  // otherwise this returns a blank string.
-  GURL ConstructHelpCenterUrl(const ModuleEnumerator::Module& module) const;
-
   // The vector containing all the modules enumerated. Will be normalized and
   // any bad modules will be marked.
   ModuleEnumerator::ModulesVector enumerated_modules_;
@@ -360,6 +368,8 @@ class EnumerateModulesModel {
   // found during last scan.
   int suspected_bad_modules_detected_;
 
+  scoped_refptr<base::ObserverListThreadSafe<Observer>> observers_;
+
   DISALLOW_COPY_AND_ASSIGN(EnumerateModulesModel);
 };