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

chrome/browser/system_monitor/image_capture_device_browser_mac_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.
+
+#include "chrome/browser/system_monitor/image_capture_device_browser_mac.h"
+
+#import <ImageCaptureCore/ImageCaptureCore.h>

[sail, 2012/12/17 03:16:03]

newline after system includes

[Greg Billock, 2012/12/17 23:20:41]

Done.

+#include "base/file_path.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 "chrome/browser/system_monitor/image_capture_camera.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+@interface MockICCameraDevice : ICCameraDevice {
+}
+
+@end
+
+@implementation MockICCameraDevice
+
+- (NSString*)mountPoint {
+  return @"mountPoint";
+}
+
+- (NSString*)name {
+  return @"name";
+}
+
+- (NSString*)UUIDString {
+  return @"id";

[sail, 2012/12/17 03:16:03]

make this a constant above in an anonymous namespace?

[Greg Billock, 2012/12/17 23:20:41]

Done.

+}
+
+- (ICDeviceType)type {
+  return ICDeviceTypeCamera;
+}
+
+- (void)requestOpenSession {
+}
+
+- (void)requestCloseSession {
+}
+
+@end
+
+@interface MockICCameraFile : ICCameraFile {
+@private

[sail, 2012/12/17 03:16:03]

space

[Greg Billock, 2012/12/17 23:20:41]

Done.

+  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(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) {}
+  ~TestCameraListener() {}

[sail, 2012/12/17 03:16:03]

virtual

[Greg Billock, 2012/12/17 23:20:41]

Done.

+
+  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 {
+    downloads_.push_back(name);
+  }
+
+  virtual void DeviceRemoved() OVERRIDE {
+    removed_ = true;
+  }
+
+  std::vector<std::string> items_;

[sail, 2012/12/17 03:16:03]

private and add accessors as needed

[Greg Billock, 2012/12/17 23:20:41]

I think that just adds noise to test-only objects.

On 2012/12/17 03:16:03, sail wrote:

[sail, 2012/12/17 23:55:03]

I've never seen a test do that. Only structs have public members. It's only one
line of code more to add an accessor.

[Greg Billock, 2012/12/19 00:05:09]

ok, sure.

On 2012/12/17 23:55:03, sail wrote:

+  std::vector<std::string> downloads_;
+  bool completed_;
+  bool removed_;
+};
+
+class ImageCaptureDeviceBrowserTest : public testing::Test {
+ public:
+  virtual void SetUp() OVERRIDE {
+    base::SystemMonitor::AllocateSystemIOPorts();
+    system_monitor_.reset(new base::SystemMonitor());
+  }
+
+  void AttachDevice(chrome::ImageCaptureDeviceBrowser* browser) {
+    ICCameraDevice* device = [MockICCameraDevice alloc];

[sail, 2012/12/17 03:16:03]

scoped_nsobject,
[[MockICCameraDevice alloc] init]
same in DeatchDevice

[Greg Billock, 2012/12/17 23:20:41]

Yes. Saw this looking through the test as well.

On 2012/12/17 03:16:03, sail wrote:

+    NSObject<ICDeviceBrowserDelegate>* delegate =
+        base::mac::ObjCCastStrict<NSObject<ICDeviceBrowserDelegate> >(
+            browser->device_browser_.get());

[sail, 2012/12/17 03:16:03]

change to use an accessor, same in DetachDevice

[Greg Billock, 2012/12/17 23:20:41]

Done.

+    [delegate deviceBrowser:nil didAddDevice:device moreComing:NO];
+  }
+
+  void DetachDevice(chrome::ImageCaptureDeviceBrowser* browser) {
+    ICCameraDevice* device = [MockICCameraDevice alloc];
+    NSObject<ICDeviceBrowserDelegate>* delegate =
+        base::mac::ObjCCastStrict<NSObject<ICDeviceBrowserDelegate> >(
+            browser->device_browser_.get());
+    [delegate deviceBrowser:nil didRemoveDevice:device moreGoing:NO];
+  }
+
+  MessageLoop message_loop_;

[sail, 2012/12/17 03:16:03]

protected

[Greg Billock, 2012/12/17 23:20:41]

Done.

+  scoped_ptr<base::SystemMonitor> system_monitor_;
+  TestCameraListener listener_;
+};
+
+TEST_F(ImageCaptureDeviceBrowserTest, TestAttachDetach) {
+  chrome::ImageCaptureDeviceBrowser browser;
+  AttachDevice(&browser);
+
+  std::vector<base::SystemMonitor::RemovableStorageInfo> devices =
+    system_monitor_->GetAttachedRemovableStorage();
+
+  ASSERT_EQ(1U, devices.size());
+  EXPECT_EQ("ic:id", devices[0].device_id);

[sail, 2012/12/17 03:16:03]

use a constant for "id"?

[Greg Billock, 2012/12/17 23:20:41]

Done.

+
+  DetachDevice(&browser);
+  devices = system_monitor_->GetAttachedRemovableStorage();
+  ASSERT_EQ(0U, devices.size());
+};
+
+TEST_F(ImageCaptureDeviceBrowserTest, OpenCamera) {
+  chrome::ImageCaptureDeviceBrowser browser;
+  AttachDevice(&browser);
+
+  EXPECT_FALSE(chrome::ImageCaptureDeviceBrowser::cameraInterfaceForUUID(
+      "nonexistent"));
+
+  scoped_nsobject<ImageCaptureCameraInterface> camera(
+      [chrome::ImageCaptureDeviceBrowser::cameraInterfaceForUUID("id") retain]);

[sail, 2012/12/17 03:16:03]

use a constant for "id"?

[Greg Billock, 2012/12/17 23:20:41]

Done.

+
+  [camera open];
+  [camera setListener:listener_.AsWeakPtr()];
+
+  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];

[sail, 2012/12/17 03:16:03]

add a test for DidRemoveDevice?
test that removing the camera and calling cameraInterfaceForUUID returns nil?

[Greg Billock, 2012/12/17 23:20:41]

Added. I still need a couple more tests for the camera object. Shall I add them
in follow-ups, though? This change is somewhat large as is...

On 2012/12/17 03:16:03, sail wrote:

[sail, 2012/12/17 23:55:03]

I think now would be better.

[Greg Billock, 2012/12/19 00:05:09]

Added. This was pretty low impact. Getting the downloads tested is going to take
more oomph. And I think some of our bots run on <10.8, and this download
delegate interface was introduced then, so I need to work around that.

On 2012/12/17 23:55:03, sail wrote:

+}