[Media Galleries] Filesystem interface for Mac PTP/MTP devices using ImageCaptureCore (part 3)

chrome/browser/media_gallery/mac/mtp_device_delegate_impl_mac.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_MEDIA_GALLERY_MTP_DEVICE_DELEGATE_IMPL_MAC_H_
+#define CHROME_BROWSER_MEDIA_GALLERY_MTP_DEVICE_DELEGATE_IMPL_MAC_H_
+
+#include <vector>

[sail, 2012/12/26 21:30:43]

newline after system includes

[Greg Billock, 2013/01/03 22:41:32]

Done.

+#include "base/file_path.h"
+#include "base/hash_tables.h"
+#include "base/memory/weak_ptr.h"
+#include "base/platform_file.h"
+#include "base/synchronization/waitable_event.h"
+#include "webkit/fileapi/file_system_file_util.h"
+#include "webkit/fileapi/media/mtp_device_delegate.h"
+
+namespace base {
+class SequencedTaskRunner;
+}
+
+namespace chrome {
+
+class CameraDeviceInterface;

[sail, 2012/12/26 21:30:43]

make this an inner class? (you can still forward declare inner classes I think)

[Greg Billock, 2013/01/03 22:41:32]

Could. Why do you think that's better?

On 2012/12/26 21:30:43, sail wrote:

[sail, 2013/01/03 23:10:43]

Currently this class is in the public name space. Making it an inner class
pollutes the public name space less.

[Greg Billock, 2013/01/04 18:05:05]

It's in chrome namespace, but that's not a lot better. :-) I'm only resisting
because the outer class name is a bear, and makes the definition a serious
mouthful.

[sail, 2013/01/08 20:16:41]

I'd really like this not to be public.

[Greg Billock, 2013/01/16 01:15:29]

Moved this and shortened the name.

On 2013/01/08 20:16:41, sail wrote:

+
+// Delegate for presenting an Image Capture device through the filesystem
+// API. The synthetic filesystem will be rooted at the constructed location,
+// and names of non-directories notified through the ItemAdded call will be

[sail, 2012/12/26 21:30:43]

"non-directories notified through" is a bit confusing. How about just "names of
files will appear as children of that directory".

[Greg Billock, 2013/01/03 22:41:32]

Done.

+// all appear as children of that directory.
+class MTPDeviceDelegateImplMac : public fileapi::MTPDeviceDelegate {
+ public:
+  MTPDeviceDelegateImplMac(const FilePath::StringType& location,
+                           base::SequencedTaskRunner* media_task_runner);
+  // Destructed via CancelPendingTasksAndDeleteDelegate. Do not call directly.

[sail, 2012/12/26 21:30:43]

make the destructor private and remove comment?

[Greg Billock, 2013/01/03 22:41:32]

Done.

+  virtual ~MTPDeviceDelegateImplMac();
+
+  // MTPDeviceDelegate:
+  virtual base::PlatformFileError GetFileInfo(
+      const FilePath& file_path,
+      base::PlatformFileInfo* file_info) OVERRIDE;
+  virtual scoped_ptr<fileapi::FileSystemFileUtil::AbstractFileEnumerator>
+      CreateFileEnumerator(const FilePath& root, bool recursive) OVERRIDE;
+  virtual base::PlatformFileError CreateSnapshotFile(
+      const FilePath& device_file_path,
+      const FilePath& local_path,
+      base::PlatformFileInfo* file_info) OVERRIDE;
+  virtual void CancelPendingTasksAndDeleteDelegate() OVERRIDE;
+
+  // Forward delegates for ImageCaptureDeviceListener
+  virtual void ItemAdded(const std::string& name,
+                         const base::PlatformFileInfo& info);
+  virtual void NoMoreItems();
+  virtual void DownloadedFile(const std::string& name,
+                              base::PlatformFileError error);
+
+  // Implementation returned by |CreateFileEnumerator()|. Must be deleted
+  // before CancelPendingTasksAndDeleteDelegate is called.
+  class Enumerator :
+      public fileapi::FileSystemFileUtil::AbstractFileEnumerator {
+   public:
+    explicit Enumerator(MTPDeviceDelegateImplMac* delegate);
+    virtual ~Enumerator();
+
+    virtual FilePath Next() OVERRIDE;
+    virtual int64 Size() OVERRIDE;
+    virtual base::Time LastModifiedTime() OVERRIDE;
+    virtual bool IsDirectory() OVERRIDE;
+
+    // Called by the delegate to signal any waiters that the received items
+    // list has changed state.
+    void ItemsChanged();
+
+   private:
+    MTPDeviceDelegateImplMac* delegate_;
+    size_t position_;
+    base::WaitableEvent wait_for_items_;
+  };
+
+  // GetFile and HasAllFiles called by enumerators.
+  FilePath GetFile(size_t index);
+  bool HasAllFiles();

[sail, 2012/12/26 21:30:43]

rename to received_all_files() ?

[Greg Billock, 2013/01/03 22:41:32]

Done.

+  void RemoveEnumerator(Enumerator* enumerator);
+
+ private:
+  std::string device_id_;
+  FilePath root_path_;
+
+  // Stores a reference to worker pool thread. All requests and response of file
+  // operations are posted on |media_task_runner_|. All callbacks from the
+  // camera will come through this task runner as well.
+  scoped_refptr<base::SequencedTaskRunner> media_task_runner_;
+
+  // Interface object for the camera underlying this MTP session.
+  scoped_ptr<CameraDeviceInterface> camera_interface_;
+
+  // Stores a map from filename to file metadata received from the camera.
+  base::hash_map<FilePath::StringType, base::PlatformFileInfo> file_info_;

[sail, 2012/12/26 21:30:43]

use std::map<> unless there's a reason to use hash_map

[Greg Billock, 2013/01/03 22:41:32]

There could be tons of files.

[sail, 2013/01/03 23:10:43]

Unless you can show a difference between hash_map and std::map for this use case
please use std::map.

[Greg Billock, 2013/01/04 18:05:05]

The difference is O(n) access vs O(1) access. Is there something about the base
hashtable header I don't know? It looks to me like the intended way to use
hashtables to make the compilers all happy.

[sail, 2013/01/08 20:16:41]

Could you provide a reference for this?

[Greg Billock, 2013/01/16 01:15:29]

log-n, so only a factor of 10 or so worse, not 1000. That still strikes me as a
big hit.

On 2013/01/08 20:16:41, sail wrote:

+
+  // Notifications for all the files we're waiting on downloads for.

[sail, 2012/12/26 21:30:43]

Events for pending downloads?

[Greg Billock, 2013/01/03 22:41:32]

Done.

+  base::hash_map<std::string, base::WaitableEvent*> download_events_;
+
+  // Resulting error codes for downloads.
+  base::hash_map<std::string, base::PlatformFileError> download_errors_;
+
+  // List of files received from the camera.
+  std::vector<FilePath::StringType> file_paths_;

[sail, 2012/12/26 21:30:43]

can this be vector<FilePath> instead?

[Greg Billock, 2013/01/03 22:41:32]

Done.

+
+  // Weak pointer to the enumerator which may be listening for files to come in.
+  Enumerator* enumerator_;
+
+  // Set to true when all file metadata has been received from the camera.
+  bool received_all_files_;
+
+  DISALLOW_COPY_AND_ASSIGN(MTPDeviceDelegateImplMac);
+};
+
+}  // namespace chrome
+
+#endif  // CHROME_BROWSER_MEDIA_GALLERY_MTP_DEVICE_DELEGATE_IMPL_MAC_H_