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

chrome/browser/system_monitor/image_capture_device_manager_unittest.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 <Foundation/Foundation.h>
+#import <ImageCaptureCore/ImageCaptureCore.h>
+
+#include "base/file_path.h"
+#include "base/file_util.h"
+#include "base/files/scoped_temp_dir.h"
+#include "base/mac/foundation_util.h"
+#include "base/memory/weak_ptr.h"
+#include "base/message_loop.h"
+#include "base/system_monitor/system_monitor.h"
+#include "base/thread_task_runner_handle.h"
+#include "chrome/browser/system_monitor/image_capture_device.h"
+#include "chrome/browser/system_monitor/image_capture_device_manager.h"
+#include "content/public/test/test_browser_thread.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+const char kDeviceId[] = "id";
+const char kTestFileContents[] = "test";
+
+}  // namespace
+
+@interface MockICCameraDevice : ICCameraDevice {
+ @private
+  scoped_nsobject<NSMutableArray> allMediaFiles_;
+}
+
+- (void)addMediaFile:(ICCameraFile*)file;
+
+@end
+
+@implementation MockICCameraDevice
+
+- (NSString*)mountPoint {
+  return @"mountPoint";
+}
+
+- (NSString*)name {
+  return @"name";
+}
+
+- (NSString*)UUIDString {
+  return base::SysUTF8ToNSString(kDeviceId);
+}
+
+- (ICDeviceType)type {
+  return ICDeviceTypeCamera;
+}
+
+- (void)requestOpenSession {
+}
+
+- (void)requestCloseSession {
+}
+
+- (NSArray*)mediaFiles {
+  return allMediaFiles_;
+}
+
+- (void)addMediaFile:(ICCameraFile*)file {
+  if (!allMediaFiles_.get())
+    allMediaFiles_.reset([NSMutableArray arrayWithCapacity:1]);
+  [allMediaFiles_ addObject:file];
+}
+
+- (void)requestDownloadFile:(ICCameraFile*)file
+                    options:(NSDictionary*)options
+           downloadDelegate:(id<ICCameraDeviceDownloadDelegate>)downloadDelegate
+        didDownloadSelector:(SEL)selector
+                contextInfo:(void*)contextInfo {
+  FilePath saveDir(base::SysNSStringToUTF8(
+      [[options objectForKey:ICDownloadsDirectoryURL] path]));
+  std::string saveAsFilename =
+      base::SysNSStringToUTF8([options objectForKey:ICSaveAsFilename]);
+  // It appears that the ImageCapture library adds an extension to the requested
+  // filename. Do that here to require a rename.
+  saveAsFilename += ".jpg";
+  FilePath toBeSaved = saveDir.Append(saveAsFilename);

[sail, 2012/12/26 18:52:37]

maybe avoid a new local variable by doing saveDir = saveDir.Append()

[Greg Billock, 2013/01/03 20:59:55]

Could do, It isn't a directory though. I don't think it's that bad.

[sail, 2013/01/03 21:50:28]

Ah, ok to leave as is then.

+  ASSERT_EQ(static_cast<int>(strlen(kTestFileContents)),

[sail, 2012/12/26 18:52:37]

EXPECT_EQ ?

[Greg Billock, 2013/01/03 20:59:55]

This is more a low-level thing, so it if fails, it likely isn't this test, so I
think ASSERT is a better choice.

+            file_util::WriteFile(toBeSaved, kTestFileContents,

[sail, 2012/12/26 18:52:37]

could this be moved to the test? currently it's hard to follow the test because
it's not clear that a file is being created and then renamed.

if not could you add a comment to the test about the set of events that are
supposed to happen?

[Greg Billock, 2013/01/03 20:59:55]

It can't really be moved, since this mimics the behavior of the actual platform
object. Agreed it is hard to follow! I discovered it by trial and error. :-(
I'll add comments.

+                                 strlen(kTestFileContents)));
+
+  NSMutableDictionary* returnOptions =
+      [NSMutableDictionary dictionaryWithDictionary:options];
+  [returnOptions setObject:base::SysUTF8ToNSString(saveAsFilename)
+                    forKey:ICSavedFilename];
+
+  [downloadDelegate didDownloadFile:file
+                              error:nil
+                            options:returnOptions
+                        contextInfo:contextInfo];
+}
+
+@end
+
+@interface MockICCameraFile : ICCameraFile {
+ @private
+  scoped_nsobject<NSString> name_;
+  scoped_nsobject<NSDate> date_;
+}
+
+- (id)init:(NSString*)name;
+
+@end
+
+@implementation MockICCameraFile
+
+- (id)init:(NSString*)name {
+  if ((self = [super init])) {
+    name_.reset([NSString stringWithString:name]);
+    date_.reset([[NSDate dateWithNaturalLanguageString:@"12/12/12"] retain]);
+  }
+  return self;
+}
+
+- (NSString*)name {
+  return name_.get();
+}
+
+- (NSString*)UTI {
+  return base::mac::CFToNSCast(kUTTypeImage);
+}
+
+- (NSDate*)modificationDate {
+  return date_.get();
+}
+
+- (NSDate*)creationDate {
+  return date_.get();
+}
+
+- (off_t)fileSize {
+  return 1000;
+}
+
+@end
+
+class TestCameraListener
+    : public ImageCaptureDeviceListener,
+      public base::SupportsWeakPtr<TestCameraListener> {
+ public:
+  TestCameraListener() : completed_(false), removed_(false) {}
+  virtual ~TestCameraListener() {}
+
+  virtual void ItemAdded(const std::string& name,
+                         const base::PlatformFileInfo& info) OVERRIDE {
+    items_.push_back(name);
+  }
+
+  virtual void NoMoreItems() OVERRIDE {
+    completed_ = true;
+  }
+
+  virtual void DownloadedFile(const std::string& name,
+                              base::PlatformFileError error) OVERRIDE {
+    DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));

[sail, 2012/12/26 18:52:37]

EXPECT_TRUE ?

[Greg Billock, 2013/01/03 20:59:55]

Done.

+    downloads_.push_back(name);
+    last_error_ = error;
+  }
+
+  virtual void DeviceRemoved() OVERRIDE {
+    removed_ = true;
+  }
+
+  std::vector<std::string> items() { return items_; }
+  std::vector<std::string> downloads() { return downloads_; }
+  bool completed() { return completed_; }
+  bool removed() { return removed_; }
+  base::PlatformFileError last_error() { return last_error_; }
+
+ private:
+  std::vector<std::string> items_;
+  std::vector<std::string> downloads_;
+  bool completed_;
+  bool removed_;
+  base::PlatformFileError last_error_;

[sail, 2012/12/26 18:52:37]

needs to be initialized in constructor

[Greg Billock, 2013/01/03 20:59:55]

Done.

+};
+
+class ImageCaptureDeviceManagerTest : public testing::Test {
+ public:
+  virtual void SetUp() OVERRIDE {
+    base::SystemMonitor::AllocateSystemIOPorts();
+    system_monitor_.reset(new base::SystemMonitor());
+    ui_thread_.reset(new content::TestBrowserThread(
+        content::BrowserThread::UI, &message_loop_));
+  }
+
+  ICCameraDevice* AttachDevice(
+      chrome::ImageCaptureDeviceManager* manager) {
+    // Ownership will be passed to the device browser delegate.
+    ICCameraDevice* device = [MockICCameraDevice alloc];

[sail, 2012/12/26 18:52:37]

should be
MockICCameraDevice* device = [[[MockICCameraDevice alloc] init] autorelease];

[Greg Billock, 2013/01/03 20:59:55]

Making this change makes the registrations fail, presumably because of the
autorelease? I thought that since the delegate takes ownership, I should just
use alloc, and since there's no init receiver, it wasn't necessary to invoke.

[sail, 2013/01/03 21:50:28]

I did a quick audit of the code and couldn't find anyplace that would cause this
to crash.

There was a small error in -[ImageCaptureDeviceManagerImpl
deviceBrowser:didRemoveDevice:...] but that wouldn't cause a crash in this case.

Could you apply this change and show me steps to reproduce the crash? I can try
to figure out what's going on.
Alloc should always be followed by an init.
ICCameraDevice and ICDevice don't have a designated initializer. NSObject's
designated initializer is -init which is what you should call here.

[Greg Billock, 2013/01/03 23:09:42]

It didn't crash; it just leaves no registered devices, presumably because when
the autorelease runs, it deletes itself from all containers.

Added the init call.

+    id<ICDeviceBrowserDelegate> delegate = manager->device_browser();
+    [delegate deviceBrowser:nil didAddDevice:device moreComing:NO];
+    return device;
+  }
+
+  void DetachDevice(chrome::ImageCaptureDeviceManager* manager,
+                    ICCameraDevice* device) {
+    id<ICDeviceBrowserDelegate> delegate = manager->device_browser();
+    [delegate deviceBrowser:nil didRemoveDevice:device moreGoing:NO];
+  }
+
+ protected:
+  MessageLoopForUI message_loop_;
+  scoped_ptr<content::TestBrowserThread> ui_thread_;
+  scoped_ptr<base::SystemMonitor> system_monitor_;
+  TestCameraListener listener_;
+};
+
+TEST_F(ImageCaptureDeviceManagerTest, TestAttachDetach) {
+  chrome::ImageCaptureDeviceManager manager;
+  ICCameraDevice* device = AttachDevice(&manager);
+
+  std::vector<base::SystemMonitor::RemovableStorageInfo> devices =
+    system_monitor_->GetAttachedRemovableStorage();
+
+  ASSERT_EQ(1U, devices.size());
+  EXPECT_EQ(std::string("ic:") + kDeviceId, devices[0].device_id);
+
+  DetachDevice(&manager, device);
+  devices = system_monitor_->GetAttachedRemovableStorage();
+  ASSERT_EQ(0U, devices.size());
+};
+
+TEST_F(ImageCaptureDeviceManagerTest, OpenCamera) {
+  chrome::ImageCaptureDeviceManager manager;
+  ICCameraDevice* device = AttachDevice(&manager);
+
+  EXPECT_FALSE(chrome::ImageCaptureDeviceManager::deviceForUUID(
+      "nonexistent"));
+
+  scoped_nsobject<ImageCaptureDevice> camera(

[sail, 2012/12/26 18:52:37]

you don't even need the scoped_nsobject here, you could just do:
  ImageCaptureDevcie* camera =
chrome::ImageCaptureDeviceManager::deviceForUUID(kDeviceId)

(not a big deal)

[Greg Billock, 2013/01/03 20:59:55]

deviceForUUID is creating a new one, though. Won't that then leak?

+      [chrome::ImageCaptureDeviceManager::deviceForUUID(kDeviceId)
+          retain]);
+
+  [camera setListener:listener_.AsWeakPtr()];
+  [camera open];
+
+  scoped_nsobject<MockICCameraFile> picture1(
+      [[MockICCameraFile alloc] init:@"pic1"]);
+  [camera cameraDevice:nil didAddItem:picture1];
+  scoped_nsobject<MockICCameraFile> picture2(
+      [[MockICCameraFile alloc] init:@"pic2"]);
+  [camera cameraDevice:nil didAddItem:picture2];
+  ASSERT_EQ(2U, listener_.items().size());
+  EXPECT_EQ("pic1", listener_.items()[0]);
+  EXPECT_EQ("pic2", listener_.items()[1]);
+  EXPECT_FALSE(listener_.completed());
+
+  [camera deviceDidBecomeReadyWithCompleteContentCatalog:nil];
+  ASSERT_EQ(2U, listener_.items().size());
+  EXPECT_TRUE(listener_.completed());
+
+  [camera close];
+  DetachDevice(&manager, device);
+  EXPECT_FALSE(chrome::ImageCaptureDeviceManager::deviceForUUID(
+      kDeviceId));
+}
+
+TEST_F(ImageCaptureDeviceManagerTest, RemoveCamera) {
+  chrome::ImageCaptureDeviceManager manager;
+  ICCameraDevice* device = AttachDevice(&manager);
+
+  scoped_nsobject<ImageCaptureDevice> camera(

[sail, 2012/12/26 18:52:37]

same as above, no need for scoped_nsobject + retain

+      [chrome::ImageCaptureDeviceManager::deviceForUUID(kDeviceId)
+          retain]);
+
+  [camera setListener:listener_.AsWeakPtr()];
+  [camera open];
+
+  [camera didRemoveDevice:device];
+  EXPECT_TRUE(listener_.removed());
+}
+
+TEST_F(ImageCaptureDeviceManagerTest, DownloadFile) {
+  scoped_ptr<content::TestBrowserThread> file_thread_(
+    new content::TestBrowserThread(
+        content::BrowserThread::FILE, &message_loop_));
+
+  chrome::ImageCaptureDeviceManager manager;
+  ICCameraDevice* device = AttachDevice(&manager);
+
+  scoped_nsobject<ImageCaptureDevice> camera(

[sail, 2012/12/26 18:52:37]

same as above, no need for scoped_nsobject + retain

+      [chrome::ImageCaptureDeviceManager::deviceForUUID(kDeviceId)
+          retain]);
+
+  [camera setListener:listener_.AsWeakPtr()];
+  [camera open];
+
+  const char kTestFileName[] = "pic1";
+
+  scoped_nsobject<MockICCameraFile> picture1(
+      [[MockICCameraFile alloc] init:@"pic1"]);
+  [base::mac::ObjCCastStrict<MockICCameraDevice>(device)
+      addMediaFile:picture1];
+  [camera cameraDevice:nil didAddItem:picture1];
+
+  base::ScopedTempDir temp_dir;
+  ASSERT_TRUE(temp_dir.CreateUniqueTempDir());
+
+  EXPECT_EQ(0U, listener_.downloads().size());
+
+  FilePath temp_file = temp_dir.path().Append("tempfile");
+  [camera downloadFile:std::string("nonexistent") localPath:temp_file];
+  message_loop_.RunUntilIdle();
+  ASSERT_EQ(1U, listener_.downloads().size());
+  EXPECT_EQ("nonexistent", listener_.downloads()[0]);
+  EXPECT_EQ(base::PLATFORM_FILE_ERROR_NOT_FOUND, listener_.last_error());
+
+  [camera downloadFile:std::string(kTestFileName) localPath:temp_file];

[sail, 2012/12/26 18:52:37]

instead of creating std::string here just change the type of kTestFileName to an
std::string?

[Greg Billock, 2013/01/03 20:59:55]

Done.

+  message_loop_.RunUntilIdle();
+
+  ASSERT_EQ(2U, listener_.downloads().size());
+  EXPECT_EQ(kTestFileName, listener_.downloads()[1]);
+  ASSERT_EQ(base::PLATFORM_FILE_OK, listener_.last_error());
+  char file_contents[5];
+  ASSERT_EQ(4, file_util::ReadFile(temp_file, file_contents,
+                                   strlen(kTestFileContents)));
+  EXPECT_EQ(kTestFileContents,
+            std::string(file_contents, strlen(kTestFileContents)));
+}