[Win, MediaGallery] Enumerate and handle mtp device attach/detach events.

chrome/browser/system_monitor/portable_device_watcher_win.h

+// Copyright (c) 2012 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_BROWSER_SYSTEM_MONITOR_PORTABLE_DEVICE_WATCHER_WIN_H_
+#define CHROME_BROWSER_SYSTEM_MONITOR_PORTABLE_DEVICE_WATCHER_WIN_H_
+
+#include <portabledeviceapi.h>
+
+#include <map>
+#include <string>
+#include <vector>
+
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+#include "base/string16.h"
+#include "base/synchronization/cancellation_flag.h"
+#include "base/system_monitor/system_monitor.h"
+#include "content/public/browser/notification_observer.h"
+#include "content/public/browser/notification_registrar.h"
+
+namespace base {
+class SequencedTaskRunner;
+}
+
+class FilePath;
+
+namespace chrome {

[Peter Kasting, 2012/10/25 05:23:24]

Should this stuff really be in a namespace?  I thought we didn't use a chrome
namespace for things in chrome/ in general.

[kmadhusu, 2012/10/26 02:01:24]

I thought the other way around. Since this file is in chrome/, I added them in
chrome namespace. 

[Peter Kasting, 2012/10/26 02:14:49]

Almost no code in chrome uses a chrome namespace.  Please check with brettw on
the rules for using the chrome namespace.

[kmadhusu, 2012/10/26 22:23:26]

As per brettw@ comment, I am leaving this as it is.

[Peter Kasting, 2012/10/26 22:35:34]

I assume you're saying all the other code in system_monitor/ uses this
namespace?  If not, don't do it here either.

+
+// This class watches the portable device mount points and sends notifications
+// to base::SystemMonitor about the attached/detached Mtp devices. This is a
+// singleton class instantiated by RemovableDeviceNotificationsWindowWin.
+class PortableDeviceWatcherWin : public content::NotificationObserver {
+ public:
+  // Struct to store temporary and persistent storage object identifiers.
+  struct DeviceStorageInfo {
+    // Temporary identifier that uniquely identifies the object on the device.
+    // This need not be persistent across sessions.
+    // E.g.: s10001

[Peter Kasting, 2012/10/25 05:23:24]

Nit: Slightly clearer if you write this example inline, like "Temporary
identifier, e.g. "s10001", that uniquely identifies..." or maybe place the
example at the end of the sentence like "...on the device, e.g. "s10001"."  (2
places)

[kmadhusu, 2012/10/26 02:01:24]

Done.

+    string16 storage_object_id;
+
+    // Stores the persistent id of the storage object.
+    // E.g.: StorageSerial:<SID-{10001,D,31080448}>:<123456789>
+    std::string unique_id;

[Peter Kasting, 2012/10/25 05:23:24]

Nit: It's confusing to have the first string commented as "uniquely identifies"
and then call this string "unique_id".

Consider naming these "temporary_id" and "permanent_id" or some other clearer
naming scheme.

[kmadhusu, 2012/10/26 02:01:24]

Rephrased the comments.
Renamed storage_object_id => object_temporary_id
Renamed unique_id => object_persistent_id

+  };
+
+  // Struct to store attached mtp device details.
+  struct DeviceDetails {
+    // Device can have multiple data partitions. Therefore, store a list of
+    // device storage details.
+    std::vector<DeviceStorageInfo> storage_info_list;

[Peter Kasting, 2012/10/25 05:23:24]

Tiny nit: Does it make sense to put this after the name and location instead of
before?

[kmadhusu, 2012/10/26 02:01:24]

Done.

+
+    // Device name.
+    string16 name;
+
+    // Device interface path.
+    string16 location;
+  };
+
+  PortableDeviceWatcherWin();
+  virtual ~PortableDeviceWatcherWin();
+
+  // Must be called after the browser blocking pool is ready for use.

[Peter Kasting, 2012/10/25 05:23:24]

Nit: Document who is supposed to call this.

[kmadhusu, 2012/10/26 02:01:24]

Done.

+  void Init();
+
+  // Gets information about the mtp device specified by |device_path|. On
+  // success, returns true and fills in |location|, |unique_id|, |name| and
+  // |removable|.

[Peter Kasting, 2012/10/25 05:23:24]

Nit: Say whether these are all left untouched on failure.

[kmadhusu, 2012/10/26 02:01:24]

Done.

+  bool GetDeviceInfo(const FilePath& device_path,
+                     string16* location,
+                     std::string* unique_id,
+                     string16* name,
+                     bool* removable);
+
+  // Processes DEV_BROADCAST_DEVICEINTERFACE messages and triggers a
+  // SystemMonitor notification if appropriate.
+  void OnWindowMessage(UINT event_type, LPARAM data);
+
+ private:
+  friend class TestPortableDeviceWatcherWin;
+
+  // Key: MTP device storage unique id.
+  // Value: Metadata for the given storage.
+  typedef std::map<std::string, base::SystemMonitor::RemovableStorageInfo>
+      MtpStorageMap;
+
+  // List of Mtp device storage details.
+  typedef std::vector<DeviceStorageInfo> StorageInfoList;

[Peter Kasting, 2012/10/25 05:23:24]

Nit: This should be public, and you should use it in other files that want a
vector<DeviceStorageInfo> (e.g. the unittest), as well as in DeviceDetails. 
Also, it shouldn't use the word "List" if it's not a list; if for example you
named the struct "DeviceStorageObject" you could call this "StorageObjects".

Consider also having a vector<DeviceDetails> typedef, either public or private
depending on who would use it.

[kmadhusu, 2012/10/26 02:01:24]

Renamed DeviceStorageInfo => DeviceStorageObject
Renamed StorageInfoList => StorageObjects
Moved typedef to public section.

+
+  // Key: Mtp device plug and play ID string.
+  // Value: List of device storage objects.
+  typedef std::map<string16, StorageInfoList> MtpDeviceMap;
+
+  // Helpers to enumerate existing mtp storage devices.
+  virtual void EnumerateAttachedDevices();
+  virtual void OnDidEnumerateAttachedDevices(
+      std::vector<DeviceDetails> device_details_list);

[Peter Kasting, 2012/10/25 05:23:24]

Nit: const ref

[kmadhusu, 2012/10/26 02:01:24]

Changed to const ptr. I couldn't pass const ref to std::vector using
PostTaskAndReplyWithResult. It is complaining about the the return type
ambiguity..

[Peter Kasting, 2012/10/26 02:14:49]

Check with willchan, akalin, or some other knowledgeable person about this.  I
haven't used PostTaskAndReplyWithResult() enough offhand to know what the
solution is here but it seems like we ought to be able to make this work.

[kmadhusu, 2012/10/26 22:23:26]

Contacted willchan@ and akalin@. Looks like it is a bug in our implementation. I
guess they are  working on a fix. In the mean time, I will use PostTaskAndReply
function.

+
+  // Helpers to handle device attach event.
+  virtual void HandleDeviceAttachEvent(const string16& pnp_device_id);
+  virtual void OnHandleDeviceAttachEvent(DeviceDetails device_details);

[Peter Kasting, 2012/10/25 05:23:24]

Nit: const ref

[kmadhusu, 2012/10/26 02:01:24]

Changed to const ptr. I couldn't pass const ref to std::vector using
PostTaskAndReplyWithResult. It is complaining about the the return type
ambiguity.

+
+  //  Handles the detach event of the device specified by |pnp_device_id|.
+  void HandleDeviceDetachEvent(const string16& pnp_device_id);
+
+  // content::NotificationObserver implementation.
+  virtual void Observe(int type,
+                       const content::NotificationSource& source,
+                       const content::NotificationDetails& details) OVERRIDE;
+
+  // Stores attached mtp device storage details.

[Peter Kasting, 2012/10/25 05:23:24]

Nit: Don't say "stores" on data members.

[kmadhusu, 2012/10/26 02:01:24]

Fixed.

+  MtpStorageMap storage_map_;

[Peter Kasting, 2012/10/25 05:23:24]

Tiny nit: Should this go after the device_map_?

[kmadhusu, 2012/10/26 02:01:24]

Done.

+
+  // Stores attached mtp device details.
+  MtpDeviceMap device_map_;
+
+  // Stores a reference to worker pool thread. Mtp device tasks are posted on
+  // this thread.

[Peter Kasting, 2012/10/25 05:23:24]

Nit: How about:

  // The task runner used to execute tasks that may take a long time and thus
should not be performed on the UI thread.

Then note in the class-level comments that the cleass is created, destroyed, and
operates on the UI thread, except for long-running tasks it spins off to a task
runner.

[kmadhusu, 2012/10/26 02:01:24]

Done.

+  scoped_refptr<base::SequencedTaskRunner> media_task_runner_;
+
+  // Used to notify PortableDeviceWatcherWin about the shutdown sequence.

[Peter Kasting, 2012/10/25 05:23:24]

Used by whom?  And "the shutdown sequence" is vague.

Example of better comment: "Set by FooClass when it receives an XYZ_EVENT
notification.  PortableDeviceWatcherWin is expected to monitor this flag's state
in BarFunc() and respond by deleting itself."

But looking at the .cc file, I'm confused why you use this at all.  A
CancellationFlag is for communicating safely between threads.  It looks to me
like this thread is set and tested all on the UI thread.  You can just use a
normal bool in that case.  But better would be to not use a bool at all and
instead get whoever creates/owns this object to delete it on or before "app
termination".  Since all your blocking pool tasks are issued with weak pointers,
this should be safe to do at any time.

[kmadhusu, 2012/10/26 02:01:24]

Rephrased the comment. Removed cancellationFlag and used bool variable. By using
a bool variable, I can stop posting tasks on the blocking thread after the
application termination message is received.

[Peter Kasting, 2012/10/26 02:14:49]

But why does this class exist at all after app termination?

Shouldn't the correct way to stop posting tasks -- as well as immediately cancel
all outstanding ones -- be to delete this object entirely?  I thought I'd asked
this in another comment but I didn't see an answer.

[vandebo (ex-Chrome), 2012/10/26 20:32:10]

I'm with Peter here.  On shutdown, the owner of this class will be destroyed and
delete this class.  At that point, no new tasks will be posted and any
outstanding tasks will safely exit because of the WeakPointer.

[kmadhusu, 2012/10/26 22:23:26]

Done. This class no longer listens for application termination message. When the
application terminates, RemovableDeviceNotificationsWindowWin will destruct this
object.

+  base::CancellationFlag app_terminating_flag_;
+
+  // Handles registering notifications with the NotificationService.
+  // Used to listen for application termination message.

[Peter Kasting, 2012/10/25 05:23:24]

Nit: First sentence is unnecessary.  Second sentence should refer to an actual
notification (or more than one) and note precisely what will happen when it's
received, if this information (including notification name) has not been
previously listed in another comment.

[kmadhusu, 2012/10/26 02:01:24]

Done.

+  content::NotificationRegistrar registrar_;
+
+  // |media_task_runner_| tasks may take a longer time to complete the tasks.
+  // Used for creating callbacks.

[Peter Kasting, 2012/10/25 05:23:24]

Nit: How about just:

  // Used by |media_task_runner_| to create cancelable callbacks.

[kmadhusu, 2012/10/26 02:01:24]

Done.

+  base::WeakPtrFactory<PortableDeviceWatcherWin> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(PortableDeviceWatcherWin);
+};
+
+}  // namespace chrome
+
+#endif  // CHROME_BROWSER_SYSTEM_MONITOR_PORTABLE_DEVICE_WATCHER_WIN_H_