[Media Galleries] Add an ImageCaptureCore listener for Mac. (part 2)

chrome/browser/system_monitor/image_capture_device_browser_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_SYSTEM_MONITOR_IMAGE_CAPTURE_DEVICE_MAC_H_
+#define CHROME_BROWSER_SYSTEM_MONITOR_IMAGE_CAPTURE_DEVICE_MAC_H_
+
+#import <Foundation/Foundation.h>
+#import <ImageCaptureCore/ImageCaptureCore.h>
+
+#include "base/file_path.h"
+#include "base/platform_file.h"
+#include "base/string_util.h"
+#include "base/sys_string_conversions.h"
+
+// Client's use this listener interface to get notifications about
+// events happening as a particular ImageCapture device is interacted with.
+// Clients drive the interaction through the ImageCaptureDeviceBrowserMac
+// and the ImageCaptureCameraInterface classes, and get notifications of
+// events through this interface.
+class ImageCaptureDeviceListener {

[sail, 2012/12/12 20:53:20]

This isn't used anywhere in this patch. It's hard to review abstract code that
doesn't have any use yet.

[Greg Billock, 2012/12/13 00:31:58]

There's ImageCaptureCameraInterface:setListener, and that code uses the
interface. I wanted to separate out the MTP delegate and this class to make the
review smaller.

+ public:
+  virtual ~ImageCaptureDeviceListener() {}
+
+  // Get a notification that a particular item has been found on the device.
+  // These calls will come automatically after a new device is initialized.
+  virtual void ItemAdded(const std::string& name,
+                         const base::PlatformFileInfo& info) = 0;
+
+  // Called when there are no more items to retrieve.
+  virtual void NoMoreItems() = 0;
+
+  // Called upon completion of a file download request. The |path| is the
+  // requested download file. Note: in NOT_FOUND error case, can be called
+  // inline with the download request.
+  virtual void DownloadedFile(const std::string& name,
+                              base::PlatformFileError error) = 0;
+
+  // Called to let the client know the device is removed.
+  virtual void DeviceRemoved() = 0;
+};
+
+@class ImageCaptureCameraInterface;
+
+// This class is the surface for the Mac ICDeviceBrowser ImageCaptureCore API.
+// Owned by the ChromeBrowserParts and has browser process lifetime. Upon
+// creation, it gets a list of attached media volumes (asynchronously) which
+// it will eventually forward to the SystemMonitor as removable storage
+// notifications. It will also set up an ImageCaptureCore listener to be
+// told when new devices/volumes are discovered and existing ones are removed.
+@interface ImageCaptureDeviceBrowserMac :

[sail, 2012/12/12 20:53:20]

name isn't very descriptive. colon on the next line? same below

As per my earlier comment, I'd make this a C++ class and remove everything else
from this file.

[Greg Billock, 2012/12/13 00:31:58]

Suggestion? The ImageCapture API calls this the DeviceBrowser, so I was
basically wrapping that.

[sail, 2012/12/13 02:14:00]

How about:
  ImageCaptureCameraInterface -> SystemMonitorICDeviceBrowserDelegate

+    NSObject<ICDeviceBrowserDelegate> {
+ @private
+  ICDeviceBrowser* device_browser_;

[sail, 2012/12/12 20:53:20]

Objective-C classes should use camel case variable names, something like
deviceBrowser_.
Also, all variables should be scoped_nsobject. If it's a weak reference then add
a comment explaining that instead.

[Greg Billock, 2012/12/13 00:31:58]

OK. Do I need to make a scoped_nsobject for my cameras_ member? Or since that's
getting synthesized it takes care of it? The sample ImageCapture code I was
following used this method.

On 2012/12/12 20:53:20, sail wrote:

[sail, 2012/12/13 02:14:00]

Everything should be scoped_nsobject or a scoped_ptr, etc..
If it's a weak reference then it should have a comment about that (usually a //
weak is sufficient)

+  NSMutableArray* cameras_;
+}
+@property(retain) NSMutableArray* cameras;

[sail, 2012/12/12 20:53:20]

should be (nonatomic, retain)
newline before and after.

[Greg Billock, 2012/12/13 00:31:58]

Done.

+- (id)init;

[sail, 2012/12/12 20:53:20]

doesn't need to be public (since this derives from NSObject it already exposes
init).

[Greg Billock, 2012/12/13 00:31:58]

Done.

+- (void)close;
+- (ImageCaptureCameraInterface*)openDeviceByUUID:(std::string&)uuid;
++ (ImageCaptureDeviceBrowserMac*)Get;

[sail, 2012/12/12 20:53:20]

Objective-C method names should start with lower case (more below)
if this returns a singleton instance then maybe name this sharedInstance

[Greg Billock, 2012/12/13 00:31:58]

Done.

+@end

[sail, 2012/12/12 20:53:20]

newline before this

[Greg Billock, 2012/12/13 00:31:58]

Done.

+
+// Interface to a camera device found by ImageCaptureCore. This class manages a
+// session to the camera and provides the backing interactions to present the
+// media files on it to the filesystem delegate. FilePaths will be artificial,
+// like "/$device_id/" + name.
+@interface ImageCaptureCameraInterface :

[sail, 2012/12/12 20:53:20]

name isn't very descriptive

[Greg Billock, 2012/12/13 00:31:58]

The idea is this class is what you use to get images from the camera. Better
ideas?

[sail, 2012/12/13 02:14:00]

  ImageCaptureDeviceListener -> SystemMonitorICDeviceDelegate

+    NSObject<ICCameraDeviceDelegate, ICCameraDeviceDownloadDelegate> {
+ @private
+  ICCameraDevice* camera_;
+  ImageCaptureDeviceListener* listener_;
+}
+- (id)init:(ICCameraDevice*)camera_device;

[sail, 2012/12/12 20:53:20]

new line before this

[Greg Billock, 2012/12/13 00:31:58]

Done.

+- (void)open;
+- (void)close;
+- (void)setListener:(ImageCaptureDeviceListener*)listener;
+- (void)DidRenameDownloadFile:(const std::string&)name
+        withError:(bool)rename_error;

[sail, 2012/12/12 20:53:20]

colons should line up, same below

[Greg Billock, 2012/12/13 00:31:58]

Done.

+
+// Download the given |file| to the provided |local_path|. Completion notice
+// will be sent to the listener's DownloadedFile method.
+- (void)DownloadFile:(const std::string&)name
+      localPath:(const FilePath&)local_path;
+@end

[sail, 2012/12/12 20:53:20]

new line before this

[Greg Billock, 2012/12/13 00:31:58]

Done.

+
+#endif   // CHROME_BROWSER_SYSTEM_MONITOR_IMAGE_CAPTURE_DEVICE_MAC_H_