Implement install module verification metric.

chrome/browser/install_module_verifier_win.cc

Index: chrome/browser/install_module_verifier_win.cc
diff --git a/chrome/browser/install_module_verifier_win.cc b/chrome/browser/install_module_verifier_win.cc
new file mode 100644
index 0000000000000000000000000000000000000000..aac7e7d25a024af5a8bf300ce523de8af235f500
--- /dev/null
+++ b/chrome/browser/install_module_verifier_win.cc
@@ -0,0 +1,102 @@
+// Copyright (c) 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.
+
+#include "chrome/browser/install_module_verifier_win.h"
+
+#include <set>
+#include <string>
+#include "base/basictypes.h"
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/metrics/sparse_histogram.h"
+#include "base/strings/utf_string_conversions.h"
+#include "base/values.h"
+#include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/enumerate_modules_model_win.h"
+#include "chrome/browser/expected_install_modules_win.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+#include "content/public/browser/notification_service.h"
+#include "content/public/browser/notification_source.h"
+
+namespace {
+
+void OnModuleMatch(size_t module_id) {
+  UMA_HISTOGRAM_SPARSE_SLOWLY("InstallVerifier.ModuleMatch", module_id);
+}
+
+class InstallModuleVerifier : public content::NotificationObserver {
+ public:
+  InstallModuleVerifier() {
+    notification_registrar_.Add(this,
+                                chrome::NOTIFICATION_MODULE_LIST_ENUMERATED,
+                                content::NotificationService::AllSources());
+  }
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE {

[Finnur, 2013/09/20 14:23:14]

Maybe DCHECK that you got back type == NOTIFICATION_MODULE_LIST_ENUMERATED?

[erikwright (departed), 2013/09/24 17:33:51]

Done.

+    EnumerateModulesModel* model =
+        content::Source<EnumerateModulesModel>(source).ptr();
+    scoped_ptr<base::ListValue> module_list(model->GetModuleList());
+
+    if (module_list.get()) {
+      VerifyModules(module_list.get(), base::Bind(&OnModuleMatch));
+    }

[Finnur, 2013/09/20 14:23:14]

nit: No braces around single line ifs.

[erikwright (departed), 2013/09/24 17:33:51]

Done.

+    delete this;
+  }
+
+ private:
+  content::NotificationRegistrar notification_registrar_;
+  DISALLOW_COPY_AND_ASSIGN(InstallModuleVerifier);

[Finnur, 2013/09/20 14:23:14]

nit: Space before this line.

[erikwright (departed), 2013/09/24 17:33:51]

Done.

+};
+
+}  // namespace
+
+void BeginModuleVerification() {
+  scoped_ptr<base::ListValue> module_list(
+      EnumerateModulesModel::GetInstance()->GetModuleList());
+  if (module_list.get()) {
+    VerifyModules(module_list.get(),
+                           base::Bind(&OnModuleMatch));
+  } else {
+    // Will delete itself when scan results are available.
+    new InstallModuleVerifier();
+    EnumerateModulesModel::GetInstance()->ScanNow();
+  }
+}
+
+void VerifyModules(base::ListValue* module_list,

[sky, 2013/09/19 20:54:36]

Can this take a const base::ListValue& ?

[erikwright (departed), 2013/09/24 17:33:51]

Done.

+                   const base::Callback<void(size_t)>& delegate) {
+  DCHECK(module_list);
+
+  std::set<std::wstring> canonical_module_names;

[sky, 2013/09/19 20:54:36]

This code would be clearer if you broke this into two steps. One to build the
names the delegate is to run with, and then another to run on the delegate.

And a description here would be nice.

[erikwright (departed), 2013/09/24 17:33:51]

Let me know if the new factoring corresponds to what you had in mind. I added
header comments and an implementation comment. Let me know if you were looking
for something more.

+  for (size_t i = 0; i < module_list->GetSize(); ++i) {
+    base::DictionaryValue* module_dictionary = NULL;
+    if (!module_list->GetDictionary(i, &module_dictionary))
+      continue;
+    ModuleEnumerator::ModuleType module_type =
+        ModuleEnumerator::LOADED_MODULE;
+    if (!module_dictionary->GetInteger(
+            "type", reinterpret_cast<int*>(&module_type)) ||
+        module_type != ModuleEnumerator::LOADED_MODULE) {
+      continue;
+    }
+    std::string utf8_module_name;
+    if (!module_dictionary->GetString("name", &utf8_module_name))
+      continue;
+    std::wstring module_name = base::UTF8ToWide(utf8_module_name);

[Finnur, 2013/09/20 14:23:14]

nit: s/std::wstring/string16/g
Yes, it is the same thing on Windows, but more readable in my opinion (and more
portable, in case this gets copied somewhere).

[erikwright (departed), 2013/09/24 17:33:51]

Done.

[Finnur, 2013/09/25 12:54:22]

Hmmm, I didn't realize we had started using the base:: prefix everywhere for
string16. :/

It was also a bigger change than I thought it would be when I made the request. 

Anyway... Thanks for indulging me.

On 2013/09/24 17:33:51, erikwright wrote:

+
+    std::wstring canonical_module_name = CanonicalizeModuleName(module_name);
+    canonical_module_names.insert(canonical_module_name);
+  }
+  for (size_t module_index = 0; kExpectedInstallModules[module_index] != NULL;
+       ++module_index) {
+    if (canonical_module_names.find(kExpectedInstallModules[module_index]) !=
+        canonical_module_names.end()) {
+      delegate.Run(module_index);
+    }

[Finnur, 2013/09/20 14:23:14]

Nit: Single line -- no brace.

Also, don't you want EndsWith instead of find?

[erikwright (departed), 2013/09/24 17:33:51]

My understanding is that, if the condition is more than one line, we use braces.
No. The names are already converted to a canonical form ignoring path, case,
etc. So we have put them all in a set and for each expected value we check if
it's in the set.

[Finnur, 2013/09/25 12:54:22]

Oh, it is a *set*. I overlooked that part. I thought it was the string::find
function, not set::find. I was worried about "e.dll" matching with "chrome.dll"
(and I now realize I also meant to say compare() not EndsWith()...) :)
But, looks like we are good here.

As for the other question:
Yes, that's a very common misunderstanding. The rule said: "Curly braces are not
required for single-line conditionals or loop bodies, but are required for
single-statement bodies that span multiple lines."

Note the use of the word 'body' there and not 'conditional or body'. However,
after looking this up I see that at least the Blink style guide seems to be
relaxing this rule, so maybe that applies to Chromium as well. 

Anyway, the takeaway from this discussion for you should be: "Don't feel
compelled to add braces around this one-line body just because the conditional
is multi-line.

+  }
+}