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

chrome/browser/system_monitor/image_capture_camera.mm

+// 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.
+
+#import "chrome/browser/system_monitor/image_capture_camera.h"
+
+#include "base/file_util.h"
+#include "base/system_monitor/system_monitor.h"
+#include "chrome/browser/system_monitor/disk_info_mac.h"
+#include "chrome/browser/system_monitor/media_storage_util.h"
+#include "content/public/browser/browser_thread.h"
+
+@implementation ImageCaptureCameraInterface
+
+- (id)init:(ICCameraDevice*)camera_device {
+  camera_ = camera_device;

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

init methods should be of the form:
  if ((self = [super init])) {
     ....
  }
  return self;

[Greg Billock, 2012/12/14 00:39:59]

Done.

+  camera_.delegate = self;
+  return self;
+}
+
+- (void)open {
+  [camera_ requestOpenSession];
+}
+
+- (void)close {
+  [camera_ requestCloseSession];
+  camera_.delegate = NULL;
+}
+
+- (void)setListener:(ImageCaptureDeviceListener*)listener {
+  listener_ = listener;
+}
+
+- (void)DownloadFile:(const std::string&)name
+      localPath:(const FilePath&)local_path {
+  // Find the file with that name and start download.
+  for (ICCameraItem* item in [camera_ mediaFiles]) {
+    std::string itemName = base::SysNSStringToUTF8([item name]);
+    if (itemName == name) {
+      NSMutableDictionary* options =
+          [NSMutableDictionary dictionaryWithCapacity:3];
+      NSString* fileURLString =

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

fileURLString and filename are confusing, one is the parent folder path and is
the name of a file.

maybe parentDirectoryPath and fileName

[Greg Billock, 2012/12/14 00:39:59]

Done.

+          [NSString stringWithUTF8String:local_path.DirName().value().c_str()];

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

base::mac::FilePathToNSString(), more below

[Greg Billock, 2012/12/14 00:39:59]

Ah ha! That's what I was wanting. Is there an NSURL version that goes all the
way?

On 2012/12/13 02:14:00, sail wrote:

+      [options setObject:[NSURL fileURLWithPath:fileURLString isDirectory:YES]
+               forKey:ICDownloadsDirectoryURL];

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

align colons, more below

[Greg Billock, 2012/12/14 00:39:59]

Done.

[Greg Billock, 2012/12/14 00:39:59]

Done.

+      NSString* filename =
+          [NSString stringWithUTF8String:local_path.BaseName().value().c_str()];
+      [options setObject:filename forKey:ICSaveAsFilename];
+      [options setObject:[NSNumber numberWithBool:YES] forKey:ICOverwrite];
+
+      [camera_ requestDownloadFile:(ICCameraFile*)item

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

no C style casts
should use base::mac::ObjCCastStrict<> here

[Greg Billock, 2012/12/14 00:39:59]

Done.

+               options:options
+               downloadDelegate:self
+               didDownloadSelector:
+                   @selector(didDownloadFile:error:options:contextInfo:)
+               contextInfo:NULL];
+      return;
+    }
+  }
+
+  if (listener_)
+    listener_->DownloadedFile(name, base::PLATFORM_FILE_ERROR_NOT_FOUND);
+}
+
+// Delegates for ICCameraDeviceDelegate
+
+- (void)cameraDevice:(ICCameraDevice*)camera didAddItem:(ICCameraItem*)item {
+  std::string name = base::SysNSStringToUTF8([item name]);
+  base::PlatformFileInfo info;
+  info.size = 0;

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

this is already initialized in the constructor, same with is_directory and
is_symbolic_link

[Greg Billock, 2012/12/14 00:39:59]

Done.

+  info.is_directory = false;
+  if ([[item UTI] isEqualToString:(NSString*)kUTTypeFolder])

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

no C style casts, use base::mac::CFToNSCast(kUTTypeFolder) instead

[Greg Billock, 2012/12/14 00:39:59]

Done.

+    info.is_directory = true;
+  else
+    info.size = [(ICCameraFile*)item fileSize];

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

no C style casts,

[Greg Billock, 2012/12/14 00:39:59]

Done.

+  info.is_symbolic_link = false;
+  info.last_modified =
+      base::Time::FromDoubleT([[item modificationDate] timeIntervalSince1970]);

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

maybe move this to a utility function above, NSDateToBaseTime()

[Greg Billock, 2012/12/14 00:39:59]

Done.

+  info.creation_time =
+      base::Time::FromDoubleT([[item creationDate] timeIntervalSince1970]);
+  info.last_accessed = info.last_modified;
+
+  if (listener_)
+    listener_->ItemAdded(name, info);
+}
+
+- (void)cameraDevice:(ICCameraDevice*)camera didAddItems:(NSArray*)items {
+  for (ICCameraItem* item in items)
+    [self cameraDevice:camera didAddItem:item];
+}
+
+- (void)didRemoveDevice:(ICDevice*)device {
+  device.delegate = NULL;
+  // Note: handled by ICDeviceBrowser::didRemoveDevice

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

It's not clear what the ready/open etc.. are used for. The comment isn't very
helpful either.

For example, if you didn't have these two line then what would happen?

[Greg Billock, 2012/12/14 00:39:59]

Yeah, I'm not sure yet what kind of error handling makes sense, or how we need
to relay it to the listener. I'll just take these out for now.

+  [device.userData setObject:[NSNumber numberWithBool:NO] forKey:@"ready"];
+  [device.userData setObject:[NSNumber numberWithBool:NO] forKey:@"open"];
+  if (listener_) {
+    listener_->DeviceRemoved();
+    [self setListener:nil];
+  }
+}
+
+// Notifies that a session was opened with the given device; potentially
+// with an error.
+- (void)device:(ICDevice*)device didOpenSessionWithError:(NSError*)error {
+  if (error == NULL)

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

Objective-C uses nil
in this caes this should just be if (!error)

[Greg Billock, 2012/12/14 00:39:59]

Done.

+    [device.userData setObject:[NSNumber numberWithBool:YES] forKey:@"open"];
+  else
+    [device.userData setObject:error forKey:@"error"];
+}
+
+// Notifies that the device is ready for commands (i.e. download images)

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

i.e. -> e.g.
period at end

[Greg Billock, 2012/12/14 00:39:59]

Removed.

On 2012/12/13 02:14:00, sail wrote:

+- (void)deviceDidBecomeReady:(ICDevice*)device {
+  [device.userData setObject:[NSNumber numberWithBool:YES] forKey:@"ready"];
+}
+
+- (void)device:(ICDevice*)device didEncounterError:(NSError*)error {
+  [device.userData setObject:error forKey:@"error"];
+  [device.userData setObject:[NSNumber numberWithBool:NO] forKey:@"open"];
+}
+
+// All metadata is now loaded.
+- (void)deviceDidBecomeReadyWithCompleteContentCatalog:(ICDevice*)device {
+  if (device.type & ICDeviceTypeCamera) {

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

no braces

[Greg Billock, 2012/12/14 00:39:59]

Removed
On 2012/12/13 02:14:00, sail wrote:

+    [device.userData setValue:(id)kCFBooleanTrue forKey:@"complete"];

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

No C-style casts 

[Greg Billock, 2012/12/14 00:39:59]

Removed
On 2012/12/13 02:14:00, sail wrote:

+  }
+  if (listener_)
+    listener_->NoMoreItems();
+}
+
+// Delegates for ICCameraDeviceDownloadDelegate

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

not needed

[Greg Billock, 2012/12/14 00:39:59]

Done.

+
+void RenameFileAndReturn(const std::string& name,

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

move this to an anonymous namespace above

[Greg Billock, 2012/12/14 00:39:59]

Done.

+                         const FilePath& downloaded_filename,
+                         const FilePath& desired_filename,
+                         ImageCaptureCameraInterface* caller) {
+  int64 edsize = 0;

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

need to assert that this is on the file thread

[Greg Billock, 2012/12/14 00:39:59]

file_util does this assert

On 2012/12/13 02:14:00, sail wrote:

+  file_util::GetFileSize(downloaded_filename, &edsize);
+  bool error = file_util::ReplaceFile(downloaded_filename, desired_filename);
+  [caller DidRenameDownloadFile:name withError:error];

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

is the caller thread safe? do you want to do this on the UI thread

[Greg Billock, 2012/12/14 00:39:59]

I need to think more about this. The calling situation for the media galleries
is that the caller is on the browser blocking pool, but these ImageCapture
notifications come in on the UI thread. I think ultimately the caller needs to
be thread-safe, and whether I need to also add a mutex on listener for this
object or can get by with something else.

[sail, 2012/12/14 01:26:32]

This shouldn't be check in as is. If you'd like to commit this CL then remove
this line and add a TODO saying you need to notify the caller.

+}
+
+- (void)didDownloadFile:(ICCameraFile*)file error:(NSError*)error

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

one argument per line and line up colons

[Greg Billock, 2012/12/14 00:39:59]

Done.

+      options:(NSDictionary*)options contextInfo:(void*)contextInfo {
+  std::string name = base::SysNSStringToUTF8([file name]);
+  base::PlatformFileError err = base::PLATFORM_FILE_OK;
+
+  // ImageCapture does not save the file into the name we give it in the
+  // options. It picks a new name according to it's best lights, so we need
+  // to rename the file.
+  std::string savedFilename =
+      base::SysNSStringToUTF8([options objectForKey:ICSavedFilename]);
+  std::string saveAsFilename =
+      base::SysNSStringToUTF8([options objectForKey:ICSaveAsFilename]);
+  if (!error && (savedFilename != saveAsFilename)) {
+      FilePath save_dir(base::SysNSStringToUTF8(
+          [[options objectForKey:ICDownloadsDirectoryURL] path]));
+    FilePath saveAsPath = save_dir.Append(saveAsFilename);
+    FilePath savedPath = save_dir.Append(savedFilename);
+    // !!! should really pass a weak pointer or a refptr or something.

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

how about just retaining self here and releasing on callback

[Greg Billock, 2012/12/14 00:39:59]

That'd work. The other problem is the listener. I need to either add weak ptr
machinery for it or mutex-protect it.

+    content::BrowserThread::PostTask(content::BrowserThread::FILE, FROM_HERE,
+        base::Bind(&RenameFileAndReturn, name, savedPath, saveAsPath, self));
+    return;
+  }
+
+  if (error)

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

maybe just have an early return at the beginning, then you don't need the err
variable

[Greg Billock, 2012/12/14 00:39:59]

Done.

+    err = base::PLATFORM_FILE_ERROR_FAILED;
+
+  if (listener_)
+    listener_->DownloadedFile(name, err);
+}
+
+- (void)DidRenameDownloadFile:(const std::string&)name
+        withError:(bool)rename_error {
+  if (listener_) {
+    listener_->DownloadedFile(name,
+        rename_error ? base::PLATFORM_FILE_OK
+                     : base::PLATFORM_FILE_ERROR_FAILED);
+  }
+}
+
+@end  // ImageCaptureCameraInterface