[Win] Create ModuleWatcher.

chrome/common/conflicts/module_watcher_win.h

+// Copyright 2016 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.
+
+#ifndef CHROME_COMMON_CONFLICTS_MODULE_WATCHER_WIN_H_
+#define CHROME_COMMON_CONFLICTS_MODULE_WATCHER_WIN_H_
+
+#include "base/callback.h"
+#include "chrome/common/conflicts/module_event_win.mojom.h"
+
+namespace conflicts {

[grt (UTC plus 2), 2016/11/08 14:07:52]

don't put this in a namespace; see
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/pcgYLSJsii8/fYtyX....

[chrisha, 2016/11/08 21:45:21]

Done.

+
+class ModuleWatcherTest;
+
+// This class observes modules as they are loaded and unloaded into a process'

[grt (UTC plus 2), 2016/11/08 14:07:52]

process's

[chrisha, 2016/11/08 21:45:22]

Oh sweet jebus, you just opened a can of worms:

http://www.grammarbook.com/punctuation/apostro.asp

(Although your suggestion is a commonly used approach, I've always been taught
to end the word with an apostrophe.)

+// address space. It works by installing a DllNotification callback, and

[grt (UTC plus 2), 2016/11/08 14:07:52]

is this documenting an implementation detail, or does a consumer need to know
this? please move any parts of this comment that relate to the implementation
out of the .h and into the .cc file somewhere.

[chrisha, 2016/11/08 21:45:21]

Done.

+// also enumerating all currently loaded modules at the time of its creation.
+// It doesn't really make sense to create more than one of these in a process,
+// although it's entirely possible.
+//
+// This class is safe to be created on any thread. Similarly, it is safe to be
+// destroyed on any thread, independent of the thread on which the instance was
+// created. The only constraint is that the constructor and destructor do not
+// run concurrently on two separate threads (a bad idea anyways).

[grt (UTC plus 2), 2016/11/08 14:07:52]

are you saying i shouldn't use this foot gun i have here? :-)

[chrisha, 2016/11/08 21:45:22]

Acknowledged.

+class ModuleWatcher {
+ public:
+  using OnModuleEventCallback =
+      base::Callback<void(const mojom::ModuleEvent& event)>;
+
+  // Creates and starts a watcher. This enumerates all loaded modules

[grt (UTC plus 2), 2016/11/08 14:07:52]

does this mean that |callback| is invoked synchronously during construction of
the object? if so, please make that clear.

[chrisha, 2016/11/08 21:45:22]

Done.

+  // synchronously on the current thread, and then registers a DllNotification
+  // callback for continued notifications. Notifications originating from the

[grt (UTC plus 2), 2016/11/08 14:07:52]

is this saying that |callback| may be invoked from any thread? if so, please
make that clear. the fact that DllNotifications can arrive on any thread sounds
like an implementation detail to me.

[chrisha, 2016/11/08 21:45:22]

Done.

+  // DllNotification API can arrive from any thread. Note that it is possible to
+  // receive two modifications for some modules as the initial loaded module
+  // enumeration races with the installation of the callback. In this case both
+  // a MODULE_LOADED and a MODULE_ALREADY_LOADED event will be received for the
+  // same module. Since the callback is installed first no modules can be
+  // missed, however. This can be called on any thread.
+  explicit ModuleWatcher(const OnModuleEventCallback& callback);
+
+  // Destructor. Unregisters the DllNotification callback if it was successfully

[grt (UTC plus 2), 2016/11/08 14:07:52]

"Destructor" is obvious, remove it.

[chrisha, 2016/11/08 21:45:21]

Done.

+  // registered in the constructor. This need not be called on the same thread
+  // as the constructor as the OS provides synchronization via the
+  // DllNotification API. This can be called on any thread.
+  ~ModuleWatcher();
+
+ protected:
+  // Used as a bridge between the operating system notifications and this class.
+  class Bridge;
+
+  // The Bridge requires access to Notify.
+  friend class Bridge;
+  // For unittesting.
+  friend class ModuleWatcherTest;
+
+  // Registers a DllNotification callback with the OS. Returns true on success,
+  // false otherwise. Modifies |dll_notification_cookie_|. Can be called on any
+  // thread. Invoked by the constructor.
+  bool RegisterDllNotificationCallback();
+
+  // Removes the installed DllNotification callback. Returns true on success,
+  // false otherwise. Modifies |dll_notification_cookie_|. Can be called on any
+  // thread. Invoked by the destructor.
+  bool UnregisterDllNotificationCallback();
+
+  // Enumerates all currently loaded modules, synchonously invoking callbacks on
+  // the current thread. Returns true on success, false otherwise. Can be
+  // called on any thread. Invoked by the constructor.

[grt (UTC plus 2), 2016/11/08 14:07:52]

"Invoked by the constructor." is it necessary to document that here? the doc
should explain what the function does, not who is doing it.

[chrisha, 2016/11/08 21:45:21]

Done.

+  bool EnumerateAlreadyLoadedModules();
+
+  // Invoked by ModuleWatcher::Bridge. Dispatches the notification via

[grt (UTC plus 2), 2016/11/08 14:07:52]

"Invoked by..." same comment

[chrisha, 2016/11/08 21:45:22]

Done.

+  // |callback_|.
+  void Notify(const mojom::ModuleEvent& event);
+
+ private:
+  // The current callback. Can end up being invoked on any thread.
+  OnModuleEventCallback callback_;
+  // Used by the DllNotification mechanism.
+  void* dll_notification_cookie_;

[grt (UTC plus 2), 2016/11/08 14:07:52]

 = nullptr; here rather than in the ctor

[chrisha, 2016/11/08 21:45:21]

I'm not 100% a fan of that, as it leaves construction details in several
places...

I see the current thinking is to "encourage" its use, and this drawback is
highlighted. But I'm pretty ambivalent.

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(ModuleWatcher);
+};
+
+}  // namespace conflicts
+
+#endif  // CHROME_COMMON_CONFLICTS_MODULE_WATCHER_WIN_H_