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

Greg Billock

8 years ago (2012-12-12 18:07:01 UTC) #1

Greg Billock

On 2012/12/12 18:07:01, Greg Billock wrote: I'm not sure how to go about writing tests ...

8 years ago (2012-12-12 18:19:02 UTC) #2

Lei Zhang

Are you rolling https://codereview.chromium.org/11490010/ into this CL?

8 years ago (2012-12-12 20:27:40 UTC) #3

sail

Some high level comments: - the CL name needs to have "part x of y" ...

8 years ago (2012-12-12 20:53:20 UTC) #4

Greg Billock

No, I'll merge that when it gets submitted. On Wed, Dec 12, 2012 at 12:27 ...

8 years ago (2012-12-12 20:58:28 UTC) #5

Greg Billock

Thanks for the review. Improved the ObjC. Did I get it right? On where to ...

8 years ago (2012-12-13 00:31:58 UTC) #6

Thanks for the review. Improved the ObjC. Did I get it right?

On where to put it, I was thinking to just have these classes be ObjC outright,
as that followed the ImageCaptureCore example code I had the best. But wrapping
at least the browser in C++ for cleaner use in ChromeBrowserPartsMain seems like
a good goal. Any reason to prefer wrapping one place vs another?

https://codereview.chromium.org/11442057/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_main_mac.h (right):

https://codereview.chromium.org/11442057/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_main_mac.h:37: scoped_ptr<Inner> inner_;
On 2012/12/12 20:53:20, sail wrote:
> I think a better way to do this would be to expose a C++ class in
> chrome/browser/system_monitor. Make the Objective-C class internal to the
> implementation.

I've sketched that out separately -- I think we actually need to move these
notifications out of SystemMonitor. They aren't base:: namespaced notifications;
you have to know chrome:: code to handle them, so they don't belong in base::.

I tried working SystemMonitor into something that maintained list of
notification types, but it ends up being complex and relatively pointless -- why
not just get notifications from the sources themselves?

I think the best option is for things these BrowserMainParts use should be
available in BrowserContext. But that has its own problems....

https://codereview.chromium.org/11442057/diff/1/chrome/browser/system_monitor...
File chrome/browser/system_monitor/image_capture_device_browser_mac.h (right):

https://codereview.chromium.org/11442057/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/image_capture_device_browser_mac.h:21: class
ImageCaptureDeviceListener {
On 2012/12/12 20:53:20, sail wrote:
> This isn't used anywhere in this patch. It's hard to review abstract code that
> doesn't have any use yet.

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.

https://codereview.chromium.org/11442057/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/image_capture_device_browser_mac.h:51: @interface
ImageCaptureDeviceBrowserMac :
On 2012/12/12 20:53:20, sail wrote:
> name isn't very descriptive. colon on the next line? same below

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

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

https://codereview.chromium.org/11442057/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/image_capture_device_browser_mac.h:54:
ICDeviceBrowser* device_browser_;
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:
> 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.

https://codereview.chromium.org/11442057/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/image_capture_device_browser_mac.h:57:
@property(retain) NSMutableArray* cameras;
On 2012/12/12 20:53:20, sail wrote:
> should be (nonatomic, retain)
> newline before and after.

Done.

https://codereview.chromium.org/11442057/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/image_capture_device_browser_mac.h:58: - (id)init;
On 2012/12/12 20:53:20, sail wrote:
> doesn't need to be public (since this derives from NSObject it already exposes
> init).

Done.

https://codereview.chromium.org/11442057/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/image_capture_device_browser_mac.h:61: +
(ImageCaptureDeviceBrowserMac*)Get;
On 2012/12/12 20:53:20, sail wrote:
> Objective-C method names should start with lower case (more below)
> if this returns a singleton instance then maybe name this sharedInstance

Done.

https://codereview.chromium.org/11442057/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/image_capture_device_browser_mac.h:62: @end
On 2012/12/12 20:53:20, sail wrote:
> newline before this

Done.

https://codereview.chromium.org/11442057/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/image_capture_device_browser_mac.h:68: @interface
ImageCaptureCameraInterface :
On 2012/12/12 20:53:20, sail wrote:
> name isn't very descriptive

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

https://codereview.chromium.org/11442057/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/image_capture_device_browser_mac.h:74: -
(id)init:(ICCameraDevice*)camera_device;
On 2012/12/12 20:53:20, sail wrote:
> new line before this

Done.

https://codereview.chromium.org/11442057/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/image_capture_device_browser_mac.h:79:
withError:(bool)rename_error;
On 2012/12/12 20:53:20, sail wrote:
> colons should line up, same below

Done.

https://codereview.chromium.org/11442057/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/image_capture_device_browser_mac.h:85: @end
On 2012/12/12 20:53:20, sail wrote:
> new line before this

Done.

https://codereview.chromium.org/11442057/diff/1/chrome/browser/system_monitor...
File chrome/browser/system_monitor/image_capture_device_browser_mac.mm (right):

https://codereview.chromium.org/11442057/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/image_capture_device_browser_mac.mm:13:
ImageCaptureDeviceBrowserMac* g_image_capture_device_browser;
On 2012/12/12 20:53:20, sail wrote:
> It doesn't seem like you really need a singleton. Callers should just grab an
> instance from ChromeBrowserMainPartsMac.

Yes. I can't see where that's exposed, though. What am I missing?

https://codereview.chromium.org/11442057/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/image_capture_device_browser_mac.mm:45: return
[[ImageCaptureCameraInterface alloc] init:camera];
OK, I think I got this. Also changed the name to use "create".

On 2012/12/12 20:53:20, sail wrote:
> In Objective-C, the naming scheme implies the owner ship rule. If the name
> begins with create or new then the caler owns the returned object. Otherwise
the
> caller should retain the returned object if it keeps a reference to it.
> 
> In this case you should return an autoreleased object.

https://codereview.chromium.org/11442057/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/image_capture_device_browser_mac.mm:55: // Method
delegates for device added and removed
On 2012/12/12 20:53:20, sail wrote:
> don't need this

Done.

https://codereview.chromium.org/11442057/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/image_capture_device_browser_mac.mm:61: if
(addedDevice.type & ICDeviceTypeCamera) {
On 2012/12/12 20:53:20, sail wrote:
> should do early return instead

Done.

https://codereview.chromium.org/11442057/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/image_capture_device_browser_mac.mm:62:
ICCameraDevice* camera_device = (ICCameraDevice*)addedDevice;
On 2012/12/12 20:53:20, sail wrote:
> local variable names in Objective-C classes should be camel case

Done.

https://codereview.chromium.org/11442057/diff/1/chrome/browser/system_monitor...
chrome/browser/system_monitor/image_capture_device_browser_mac.mm:93: [self
willChangeValueForKey:@"cameras"];
On 2012/12/12 20:53:20, sail wrote:
> Are you planning on using key value observers? I'd recommend against that.

No. This is boilerplate from the ImageCapture sample app. I don't know enough
about Mac programming to know what's safe to remove. :-/ Is it OK to get rid of
this?

sail

https://codereview.chromium.org/11442057/diff/1/chrome/browser/system_monitor/image_capture_device_browser_mac.h File chrome/browser/system_monitor/image_capture_device_browser_mac.h (right): https://codereview.chromium.org/11442057/diff/1/chrome/browser/system_monitor/image_capture_device_browser_mac.h#newcode51 chrome/browser/system_monitor/image_capture_device_browser_mac.h:51: @interface ImageCaptureDeviceBrowserMac : On 2012/12/13 00:31:58, Greg Billock wrote: ...

8 years ago (2012-12-13 02:14:00 UTC) #7

Greg Billock

OK, Made a lot of ObjC improvements. Hopefully things are looking better now. Still a ...

8 years ago (2012-12-14 00:39:59 UTC) #8

sail

Looks pretty good! https://codereview.chromium.org/11442057/diff/13/chrome/browser/system_monitor/image_capture_camera.mm File chrome/browser/system_monitor/image_capture_camera.mm (right): https://codereview.chromium.org/11442057/diff/13/chrome/browser/system_monitor/image_capture_camera.mm#newcode140 chrome/browser/system_monitor/image_capture_camera.mm:140: [caller DidRenameDownloadFile:name withError:error]; On 2012/12/14 00:39:59, ...

8 years ago (2012-12-14 01:26:32 UTC) #9

Greg Billock

https://codereview.chromium.org/11442057/diff/13/chrome/browser/system_monitor/image_capture_device_browser_mac.mm File chrome/browser/system_monitor/image_capture_device_browser_mac.mm (right): https://codereview.chromium.org/11442057/diff/13/chrome/browser/system_monitor/image_capture_device_browser_mac.mm#newcode73 chrome/browser/system_monitor/image_capture_device_browser_mac.mm:73: chrome::DiskInfoMac info = chrome::DiskInfoMac::BuildDiskInfoFromICDevice( On 2012/12/14 01:26:32, sail wrote: ...

8 years ago (2012-12-14 17:29:22 UTC) #10

Greg Billock

Tests coming up. https://codereview.chromium.org/11442057/diff/13001/chrome/browser/system_monitor/image_capture_camera.h File chrome/browser/system_monitor/image_capture_camera.h (right): https://codereview.chromium.org/11442057/diff/13001/chrome/browser/system_monitor/image_capture_camera.h#newcode57 chrome/browser/system_monitor/image_capture_camera.h:57: - (id)initWithCameraDevice:(ICCameraDevice*)camera_device; On 2012/12/14 01:26:32, sail ...

8 years ago (2012-12-14 18:59:10 UTC) #11

sail

Everything looks really good! I think a really simple solution to the threading problem would ...

8 years ago (2012-12-14 19:27:32 UTC) #12

Greg Billock

Note there's now a test. Working on the second one... https://codereview.chromium.org/11442057/diff/13001/chrome/browser/system_monitor/image_capture_device_browser_mac.h File chrome/browser/system_monitor/image_capture_device_browser_mac.h (right): https://codereview.chromium.org/11442057/diff/13001/chrome/browser/system_monitor/image_capture_device_browser_mac.h#newcode24 ...

8 years ago (2012-12-14 22:03:56 UTC) #13

Note there's now a test. Working on the second one...

https://codereview.chromium.org/11442057/diff/13001/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_device_browser_mac.h (right):

https://codereview.chromium.org/11442057/diff/13001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_browser_mac.h:24: class
ImageCaptureDeviceBrowser {
Ah, hmmm. We haven't really followed that elsewhere here, and I worry it'd be
confusing. To me what these classes do is provide a wrapper on the ImageCapture
library, so that's the main feature of their name.

On 2012/12/14 19:27:32, sail wrote:
> On 2012/12/14 18:59:11, Greg Billock wrote:
> > On 2012/12/14 01:26:32, sail wrote:
> > > I think SystemMonitorICDeviceBrowser would be a better name
> > 
> > I suspect we'll end up not going through SystemMonitor at all. We do need to
> > resolve that approach, though.
> 
> Sorry, I wasn't referring to the base/system_monitor API. I was referring to
the
> module this code is in (chrome/browser/system_monitor). Ideally this should be
> named to reflect the module.

https://codereview.chromium.org/11442057/diff/14009/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_camera.h (right):

https://codereview.chromium.org/11442057/diff/14009/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_camera.h:59: // client supplies a
WeakPtr callback interface.
Oops! Editing here was imperfect. Just removed this.

I thought earlier about bouncing all the callbacks through a provided pool.
That's really obnoxious, though. You think it's more obnoxious than demanding
the client be thread-safe? Or is the expected convention that these callbacks
will come on the UI thread, so I should just trampoline them to there. The thing
is, that's doable, but a bit of work, and in practice will still mean the caller
needs to be thread-safe. That's why I thought it wasn't worth it.

I think we need a weakptr due to async callbacks, and if we thread-forward then
obviously the weakptr will be nice for that, so I'm happy with it, but my
instinct in tricky cases like this is usually to push complication out to
clients. (A thread adapter client would be easy to write, for instance, and then
isolates all that stuff away from the guts of this interface.)

On 2012/12/14 19:27:32, sail wrote:
> The reference to WeakPtr can be removed. WeakPtr isn't thread safe (it's just
> there for asynchronous callbacks).

https://codereview.chromium.org/11442057/diff/14009/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_camera.h:64: // Weak pointer to this
class' client.
On 2012/12/14 19:27:32, sail wrote:
> don't need this comment anymore

Done.

https://codereview.chromium.org/11442057/diff/14009/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_camera.mm (right):

https://codereview.chromium.org/11442057/diff/14009/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_camera.mm:51: CHECK(![camera_
delegate]);
On 2012/12/14 19:27:32, sail wrote:
> DCHECK ?

Done.

https://codereview.chromium.org/11442057/diff/14009/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_device_browser_mac.mm (right):

https://codereview.chromium.org/11442057/diff/14009/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_browser_mac.mm:97:
didRemoveDevice:(ICDevice*)device moreGoing:(BOOL)moreGoing {
On 2012/12/14 19:27:32, sail wrote:
> one argument per line

Done.

https://codereview.chromium.org/11442057/diff/14009/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_browser_mac.mm:102:
base::mac::ObjCCastStrict<ICCameraDevice>(device);
On 2012/12/14 19:27:32, sail wrote:
> It looks like you don't need this cast anymore. You can just call UUIDString
on
> ICDevice directly.

Done.

sail

https://codereview.chromium.org/11442057/diff/13001/chrome/browser/system_monitor/image_capture_device_browser_mac.h File chrome/browser/system_monitor/image_capture_device_browser_mac.h (right): https://codereview.chromium.org/11442057/diff/13001/chrome/browser/system_monitor/image_capture_device_browser_mac.h#newcode24 chrome/browser/system_monitor/image_capture_device_browser_mac.h:24: class ImageCaptureDeviceBrowser { On 2012/12/14 22:03:56, Greg Billock wrote: ...

8 years ago (2012-12-14 22:14:48 UTC) #14

https://codereview.chromium.org/11442057/diff/13001/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_device_browser_mac.h (right):

https://codereview.chromium.org/11442057/diff/13001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_browser_mac.h:24: class
ImageCaptureDeviceBrowser {
On 2012/12/14 22:03:56, Greg Billock wrote:
> Ah, hmmm. We haven't really followed that elsewhere here, and I worry it'd be
> confusing. To me what these classes do is provide a wrapper on the
ImageCapture
> library, so that's the main feature of their name.
> 
> On 2012/12/14 19:27:32, sail wrote:
> > On 2012/12/14 18:59:11, Greg Billock wrote:
> > > On 2012/12/14 01:26:32, sail wrote:
> > > > I think SystemMonitorICDeviceBrowser would be a better name
> > > 
> > > I suspect we'll end up not going through SystemMonitor at all. We do need
to
> > > resolve that approach, though.
> > 
> > Sorry, I wasn't referring to the base/system_monitor API. I was referring to
> the
> > module this code is in (chrome/browser/system_monitor). Ideally this should
be
> > named to reflect the module.
> 

In ui/cocoa/* we normally name the class after the feature it's implementing and
not the API its using.

For example, instead of MyTableDelegate it would be TaskManagerTableDelegate.
This helps avoid confusion where we have lots of table delegates but they all do
different things.

https://codereview.chromium.org/11442057/diff/14009/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_camera.h (right):

https://codereview.chromium.org/11442057/diff/14009/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_camera.h:59: // client supplies a
WeakPtr callback interface.
On 2012/12/14 22:03:56, Greg Billock wrote:
> Oops! Editing here was imperfect. Just removed this.
> 
> I thought earlier about bouncing all the callbacks through a provided pool.
> That's really obnoxious, though. You think it's more obnoxious than demanding
> the client be thread-safe? Or is the expected convention that these callbacks
> will come on the UI thread, so I should just trampoline them to there. The
thing
> is, that's doable, but a bit of work, and in practice will still mean the
caller
> needs to be thread-safe. That's why I thought it wasn't worth it.
> 
> I think we need a weakptr due to async callbacks, and if we thread-forward
then
> obviously the weakptr will be nice for that, so I'm happy with it, but my
> instinct in tricky cases like this is usually to push complication out to
> clients. (A thread adapter client would be easy to write, for instance, and
then
> isolates all that stuff away from the guts of this interface.)
> 
> On 2012/12/14 19:27:32, sail wrote:
> > The reference to WeakPtr can be removed. WeakPtr isn't thread safe (it's
just
> > there for asynchronous callbacks).
> 

WeakPtr is not thread safe. That means you can't pass it to RenameFileAndReturn.

By having RenameFileAndReturn perform the callback on the UI thread you
encapsulate all the threading issues into a single function.

sail

https://codereview.chromium.org/11442057/diff/24003/chrome/browser/system_monitor/image_capture_device_browser_mac.h File chrome/browser/system_monitor/image_capture_device_browser_mac.h (right): https://codereview.chromium.org/11442057/diff/24003/chrome/browser/system_monitor/image_capture_device_browser_mac.h#newcode36 chrome/browser/system_monitor/image_capture_device_browser_mac.h:36: friend class ::ImageCaptureDeviceBrowserTest; It would be better to add ...

8 years ago (2012-12-17 03:16:03 UTC) #15

Greg Billock

https://codereview.chromium.org/11442057/diff/24003/chrome/browser/system_monitor/image_capture_device_browser_mac.h File chrome/browser/system_monitor/image_capture_device_browser_mac.h (right): https://codereview.chromium.org/11442057/diff/24003/chrome/browser/system_monitor/image_capture_device_browser_mac.h#newcode36 chrome/browser/system_monitor/image_capture_device_browser_mac.h:36: friend class ::ImageCaptureDeviceBrowserTest; On 2012/12/17 03:16:03, sail wrote: > ...

8 years ago (2012-12-17 23:20:41 UTC) #16

sail

Only nits left, everything else looks good. Also, what do you think of changing the ...

8 years ago (2012-12-17 23:55:03 UTC) #17

Greg Billock

> Also, what do you think of changing the names to SystemMonitorICDeviceBrowserDelegate and SystemMonitorICDeviceDelegate? I ...

8 years ago (2012-12-19 00:05:08 UTC) #18

> Also, what do you think of changing the names to
SystemMonitorICDeviceBrowserDelegate and SystemMonitorICDeviceDelegate?

I prefer spelling out "ImageCapture" and not including any ref to
"SystemMonitor", away from which I'm hoping we move. :-)

Lei, what do you think?

https://codereview.chromium.org/11442057/diff/24003/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_device_browser_mac_unittest.mm
(right):

https://codereview.chromium.org/11442057/diff/24003/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_browser_mac_unittest.mm:114:
std::vector<std::string> items_;
ok, sure.

On 2012/12/17 23:55:03, sail wrote:
> On 2012/12/17 23:20:41, Greg Billock wrote:
> > I think that just adds noise to test-only objects.
> > 
> > On 2012/12/17 03:16:03, sail wrote:
> > > private and add accessors as needed
> > 
> 
> 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.

https://codereview.chromium.org/11442057/diff/24003/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_browser_mac_unittest.mm:191:
[camera close];
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:
> On 2012/12/17 23:20:41, Greg Billock wrote:
> > 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:
> > > add a test for DidRemoveDevice?
> > > test that removing the camera and calling cameraInterfaceForUUID returns
> nil?
> > 
> 
> I think now would be better.

https://codereview.chromium.org/11442057/diff/30001/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_device_browser_mac.h (right):

https://codereview.chromium.org/11442057/diff/30001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_browser_mac.h:36: // Return a
weak pointer to the internal ImageCapture interface protocol.
On 2012/12/17 23:55:03, sail wrote:
> Return -> Returns

Done.

https://codereview.chromium.org/11442057/diff/30001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_browser_mac.h:37: //
Currently useful for unit tests.
On 2012/12/17 23:55:03, sail wrote:
> probably don't need

Done.

https://codereview.chromium.org/11442057/diff/30001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_browser_mac.h:41: //  friend
class ::ImageCaptureDeviceBrowserTest;
Oops! yes.
On 2012/12/17 23:55:03, sail wrote:
> remove?

https://codereview.chromium.org/11442057/diff/30001/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_device_browser_mac_unittest.mm
(right):

https://codereview.chromium.org/11442057/diff/30001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_browser_mac_unittest.mm:179:
retain]);
On 2012/12/17 23:55:03, sail wrote:
> indent 3 more spaces

Done.

Lei Zhang

I'm cool with spelling out ImageCapture, especially in the external bits that go into browser ...

8 years ago (2012-12-19 00:30:52 UTC) #19

sail

https://codereview.chromium.org/11442057/diff/43001/chrome/browser/system_monitor/image_capture_camera.mm File chrome/browser/system_monitor/image_capture_camera.mm (right): https://codereview.chromium.org/11442057/diff/43001/chrome/browser/system_monitor/image_capture_camera.mm#newcode14 chrome/browser/system_monitor/image_capture_camera.mm:14: // This continuation runs on the file thread to ...

8 years ago (2012-12-19 01:18:54 UTC) #20

sail

On 2012/12/19 00:05:08, Greg Billock wrote: > > Also, what do you think of changing ...

8 years ago (2012-12-19 01:20:15 UTC) #21

Greg Billock

On 2012/12/19 01:20:15, sail wrote: > On 2012/12/19 00:05:08, Greg Billock wrote: > > > ...

8 years ago (2012-12-19 16:14:38 UTC) #22

Greg Billock

I think the remaining issue is using WeakPtr handoff vs continuations to do the rename. ...

8 years ago (2012-12-19 17:20:08 UTC) #23

I think the remaining issue is using WeakPtr handoff vs continuations to do the
rename. I prefer just having one listener weak ptr there rather than
coordinating through the manager (which means it'll need to support weak ptr as
well), but if there's some flaw I'm not seeing, let me know.

https://codereview.chromium.org/11442057/diff/43001/chrome/browser/chrome_bro...
File chrome/browser/chrome_browser_main_mac.mm (right):

https://codereview.chromium.org/11442057/diff/43001/chrome/browser/chrome_bro...
chrome/browser/chrome_browser_main_mac.mm:26: #include
"chrome/browser/system_monitor/image_capture_device_browser_mac.h"
On 2012/12/19 00:30:52, Lei Zhang wrote:
> nit: move up 1 line.

Done.

https://codereview.chromium.org/11442057/diff/43001/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_camera.mm (right):

https://codereview.chromium.org/11442057/diff/43001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_camera.mm:14: // This continuation
runs on the file thread to rename a saved file if needed.
On 2012/12/19 01:18:54, sail wrote:
> On 2012/12/19 00:30:52, Lei Zhang wrote:
> > Ading
> DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::FILE));
> > makes for even better documentation.
> 
> +1

Done.

https://codereview.chromium.org/11442057/diff/43001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_camera.mm:18:
base::WeakPtr<ImageCaptureDeviceListener> listener) {
On 2012/12/19 01:18:54, sail wrote:
> As per my earlier comment this won't work.
> See the earlier comment for a much better way to do this.

How so? I agree bouncing between the threads will work too, but I like this
better -- requires no additional handoff between this continuation and the
caller. It relies on thread safety in the listener, but that's already pretty
much a certain requirement given other factors.

https://codereview.chromium.org/11442057/diff/43001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_camera.mm:20:
file_util::GetFileSize(downloaded_filename, &edsize);
On 2012/12/19 00:30:52, Lei Zhang wrote:
> We are getting the file size but not using it.

Thanks. This was leftover from logging trying to figure out what was going on
with the downloaded file when I was discovering it invented its own name.

https://codereview.chromium.org/11442057/diff/43001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_camera.mm:21: bool error =
file_util::ReplaceFile(downloaded_filename, desired_filename);
On 2012/12/19 00:30:52, Lei Zhang wrote:
> |error| should be renamed 'succeeded'.

Done.

https://codereview.chromium.org/11442057/diff/43001/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_device_browser_mac.h (right):

https://codereview.chromium.org/11442057/diff/43001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_browser_mac.h:10: #include
"base/file_path.h"
On 2012/12/19 00:30:52, Lei Zhang wrote:
> nit: remove unused headers.

Done.

sail

On 2012/12/19 17:20:08, Greg Billock wrote: > I think the remaining issue is using WeakPtr ...

8 years ago (2012-12-19 17:49:36 UTC) #24

Greg Billock

ok. Reading weak_ptr.h file comment I see it isn't happy with this usage. I'd originally ...

8 years ago (2012-12-19 18:24:02 UTC) #25

sail

On 2012/12/19 18:24:02, Greg Billock wrote: > ok. Reading weak_ptr.h file comment I see it ...

8 years ago (2012-12-19 18:27:40 UTC) #26

sail

Also, you removed the call to BuildDiskInfoFromICDevice but you didn't update disk_info_mac.h and disk_info_mac.mm.

8 years ago (2012-12-19 18:29:43 UTC) #27

Greg Billock

On 2012/12/19 18:29:43, sail wrote: > Also, you removed the call to BuildDiskInfoFromICDevice but you ...

8 years ago (2012-12-19 20:22:24 UTC) #28

sail

https://codereview.chromium.org/11442057/diff/49003/chrome/browser/system_monitor/image_capture_device.h File chrome/browser/system_monitor/image_capture_device.h (right): https://codereview.chromium.org/11442057/diff/49003/chrome/browser/system_monitor/image_capture_device.h#newcode55 chrome/browser/system_monitor/image_capture_device.h:55: // Note on thread-safety. This class ends up involving ...

8 years ago (2012-12-19 20:41:39 UTC) #29

Greg Billock

https://codereview.chromium.org/11442057/diff/49003/chrome/browser/system_monitor/image_capture_device.mm File chrome/browser/system_monitor/image_capture_device.mm (right): https://codereview.chromium.org/11442057/diff/49003/chrome/browser/system_monitor/image_capture_device.mm#newcode226 chrome/browser/system_monitor/image_capture_device.mm:226: [self release]; On 2012/12/19 20:41:39, sail wrote: > this ...

8 years ago (2012-12-19 21:19:17 UTC) #30

sail

https://codereview.chromium.org/11442057/diff/49003/chrome/browser/system_monitor/image_capture_device.mm File chrome/browser/system_monitor/image_capture_device.mm (right): https://codereview.chromium.org/11442057/diff/49003/chrome/browser/system_monitor/image_capture_device.mm#newcode226 chrome/browser/system_monitor/image_capture_device.mm:226: [self release]; On 2012/12/19 21:19:17, Greg Billock wrote: > ...

8 years ago (2012-12-19 21:32:48 UTC) #31

Greg Billock

https://codereview.chromium.org/11442057/diff/49003/chrome/browser/system_monitor/image_capture_device.mm File chrome/browser/system_monitor/image_capture_device.mm (right): https://codereview.chromium.org/11442057/diff/49003/chrome/browser/system_monitor/image_capture_device.mm#newcode226 chrome/browser/system_monitor/image_capture_device.mm:226: [self release]; On 2012/12/19 21:32:49, sail wrote: > On ...

8 years ago (2012-12-19 22:08:55 UTC) #32

sail

https://codereview.chromium.org/11442057/diff/49003/chrome/browser/system_monitor/image_capture_device.mm File chrome/browser/system_monitor/image_capture_device.mm (right): https://codereview.chromium.org/11442057/diff/49003/chrome/browser/system_monitor/image_capture_device.mm#newcode226 chrome/browser/system_monitor/image_capture_device.mm:226: [self release]; On 2012/12/19 22:08:55, Greg Billock wrote: > ...

8 years ago (2012-12-19 22:52:05 UTC) #33

Greg Billock

The alternative you've suggested still conflicts in the same way with setListener as the present ...

8 years ago (2012-12-19 23:35:15 UTC) #34

The alternative you've suggested still conflicts in the same way with
setListener as the present state does, so switching to it buys us nothing.
The reason is that setListener can be (and is) called by a thread which is
neither the UI nor the FILE thread. That's what the existing comment says.

I'm still concerned I've got a problem here, but I don't think your
solution works. Going to look some more....



On Wed, Dec 19, 2012 at 2:52 PM, <sail@chromium.org> wrote:

>
> https://codereview.chromium.**org/11442057/diff/49003/**
>
chrome/browser/system_monitor/**image_capture_device.mm<https://codereview.chromium.org/11442057/diff/49003/chrome/browser/system_monitor/image_capture_device.mm>
> File
chrome/browser/system_monitor/**image_capture_device.mm<http://image_capture_device.mm>(right):
>
> https://codereview.chromium.**org/11442057/diff/49003/**
>
chrome/browser/system_monitor/**image_capture_device.mm#**newcode226<https://codereview.chromium.org/11442057/diff/49003/chrome/browser/system_monitor/image_capture_device.mm#newcode226>
>
chrome/browser/system_monitor/**image_capture_device.mm:226<http://image_capture_device.mm:226>:
> [self
> release];
> On 2012/12/19 22:08:55, Greg Billock wrote:
>
>> On 2012/12/19 21:32:49, sail wrote:
>> > On 2012/12/19 21:19:17, Greg Billock wrote:
>> > > On 2012/12/19 20:41:39, sail wrote:
>> > > > this isn't a good idea on the file thread
>> > >
>> > > Really? are the NSObject refcounters not threadsafe? Will creation
>>
> be safe,
>
>> > > then? Because that's likely done in neither the UI nor FILE
>>
> threads.
>
>> >
>> > The release may cause the object to be deleted. This means that the
>>
> classes
>
>> > destructor and all member variables need to be thread safe which
>>
> they are not.
>
>  Docs specifically say it is safe to delete WeakPtr on another thread.
>>
> Which
>
>> member variable is the concern?
>>
>
>  I'm still a bit worried that there's a hole here where WeakPtr can be
>> invalidated one place and then called, but I think I've read it
>>
> carefully enough
>
>> now that I'm sure that won't happen. Still, it is a bit subtle. I
>>
> currently
>
>> believe that if the caller manages itself on a single thread, the way
>>
> WeakPtr
>
>> wants to be, that this class is OK.
>>
>
> I don't think you are correct.
>
> Basically WeakPtr<Foo> is the same as Foo* except that it does some
> magic when Foo is deleted.
>
> So instead of WeakPtr<Foo> my_ptr, lets imagine you just have Foo*
> my_ptr. In this function you are referencing my_ptr at the same time
> another function (lets say setListener) might be referencing it as well.
>
> To fix this you either have to make this class thread safe or do a (very
> very very simple) modification I suggested 6 days ago.
>
>
https://codereview.chromium.**org/11442057/<https://codereview.chromium.org/1...
>

Greg Billock

Looking carefully at weak_ptr.h, I think the thread behavior is more iffy than the comments ...

8 years ago (2012-12-20 00:06:48 UTC) #35

Looking carefully at weak_ptr.h, I think the thread behavior is more iffy
than the comments say -- the copy constructor uses get(), so if that's not
thread safe (it isn't), then while you can copy on another thread and get a
valid object, you shouldn't do it unless that thread is sequenced with the
originating thread of the WeakPtr anyway. :-(

I think the goal of having the interface object interact in the provided
worker pool is a good one, but I need to find a better way to do it.
Nothing we have so far accomplishes that.


On Wed, Dec 19, 2012 at 3:35 PM, Greg Billock <gbillock@chromium.org> wrote:

> The alternative you've suggested still conflicts in the same way with
> setListener as the present state does, so switching to it buys us nothing.
> The reason is that setListener can be (and is) called by a thread which is
> neither the UI nor the FILE thread. That's what the existing comment says.
>
> I'm still concerned I've got a problem here, but I don't think your
> solution works. Going to look some more....
>
>
>
> On Wed, Dec 19, 2012 at 2:52 PM, <sail@chromium.org> wrote:
>
>>
>> https://codereview.chromium.**org/11442057/diff/49003/**
>>
chrome/browser/system_monitor/**image_capture_device.mm<https://codereview.chromium.org/11442057/diff/49003/chrome/browser/system_monitor/image_capture_device.mm>
>> File
chrome/browser/system_monitor/**image_capture_device.mm<http://image_capture_device.mm>(right):
>>
>> https://codereview.chromium.**org/11442057/diff/49003/**
>>
chrome/browser/system_monitor/**image_capture_device.mm#**newcode226<https://codereview.chromium.org/11442057/diff/49003/chrome/browser/system_monitor/image_capture_device.mm#newcode226>
>>
chrome/browser/system_monitor/**image_capture_device.mm:226<http://image_capture_device.mm:226>:
>> [self
>> release];
>> On 2012/12/19 22:08:55, Greg Billock wrote:
>>
>>> On 2012/12/19 21:32:49, sail wrote:
>>> > On 2012/12/19 21:19:17, Greg Billock wrote:
>>> > > On 2012/12/19 20:41:39, sail wrote:
>>> > > > this isn't a good idea on the file thread
>>> > >
>>> > > Really? are the NSObject refcounters not threadsafe? Will creation
>>>
>> be safe,
>>
>>> > > then? Because that's likely done in neither the UI nor FILE
>>>
>> threads.
>>
>>> >
>>> > The release may cause the object to be deleted. This means that the
>>>
>> classes
>>
>>> > destructor and all member variables need to be thread safe which
>>>
>> they are not.
>>
>>  Docs specifically say it is safe to delete WeakPtr on another thread.
>>>
>> Which
>>
>>> member variable is the concern?
>>>
>>
>>  I'm still a bit worried that there's a hole here where WeakPtr can be
>>> invalidated one place and then called, but I think I've read it
>>>
>> carefully enough
>>
>>> now that I'm sure that won't happen. Still, it is a bit subtle. I
>>>
>> currently
>>
>>> believe that if the caller manages itself on a single thread, the way
>>>
>> WeakPtr
>>
>>> wants to be, that this class is OK.
>>>
>>
>> I don't think you are correct.
>>
>> Basically WeakPtr<Foo> is the same as Foo* except that it does some
>> magic when Foo is deleted.
>>
>> So instead of WeakPtr<Foo> my_ptr, lets imagine you just have Foo*
>> my_ptr. In this function you are referencing my_ptr at the same time
>> another function (lets say setListener) might be referencing it as well.
>>
>> To fix this you either have to make this class thread safe or do a (very
>> very very simple) modification I suggested 6 days ago.
>>
>>
https://codereview.chromium.**org/11442057/<https://codereview.chromium.org/1...
>>
>
>

Greg Billock

OK, Took out using the source pool, and now just uses the UI thread. I ...

8 years ago (2012-12-20 00:59:07 UTC) #36

OK, Took out using the source pool, and now just uses the UI thread. I
didn't add in DCHECKs for this in this patch, as it'll make the test more
painful.


On Wed, Dec 19, 2012 at 4:06 PM, Greg Billock <gbillock@chromium.org> wrote:

> Looking carefully at weak_ptr.h, I think the thread behavior is more iffy
> than the comments say -- the copy constructor uses get(), so if that's not
> thread safe (it isn't), then while you can copy on another thread and get a
> valid object, you shouldn't do it unless that thread is sequenced with the
> originating thread of the WeakPtr anyway. :-(
>
> I think the goal of having the interface object interact in the provided
> worker pool is a good one, but I need to find a better way to do it.
> Nothing we have so far accomplishes that.
>
>
> On Wed, Dec 19, 2012 at 3:35 PM, Greg Billock <gbillock@chromium.org>wrote:
>
>> The alternative you've suggested still conflicts in the same way with
>> setListener as the present state does, so switching to it buys us nothing.
>> The reason is that setListener can be (and is) called by a thread which is
>> neither the UI nor the FILE thread. That's what the existing comment says.
>>
>> I'm still concerned I've got a problem here, but I don't think your
>> solution works. Going to look some more....
>>
>>
>>
>> On Wed, Dec 19, 2012 at 2:52 PM, <sail@chromium.org> wrote:
>>
>>>
>>> https://codereview.chromium.**org/11442057/diff/49003/**
>>>
chrome/browser/system_monitor/**image_capture_device.mm<https://codereview.chromium.org/11442057/diff/49003/chrome/browser/system_monitor/image_capture_device.mm>
>>> File
chrome/browser/system_monitor/**image_capture_device.mm<http://image_capture_device.mm>(right):
>>>
>>> https://codereview.chromium.**org/11442057/diff/49003/**
>>>
chrome/browser/system_monitor/**image_capture_device.mm#**newcode226<https://codereview.chromium.org/11442057/diff/49003/chrome/browser/system_monitor/image_capture_device.mm#newcode226>
>>>
chrome/browser/system_monitor/**image_capture_device.mm:226<http://image_capture_device.mm:226>:
>>> [self
>>> release];
>>> On 2012/12/19 22:08:55, Greg Billock wrote:
>>>
>>>> On 2012/12/19 21:32:49, sail wrote:
>>>> > On 2012/12/19 21:19:17, Greg Billock wrote:
>>>> > > On 2012/12/19 20:41:39, sail wrote:
>>>> > > > this isn't a good idea on the file thread
>>>> > >
>>>> > > Really? are the NSObject refcounters not threadsafe? Will creation
>>>>
>>> be safe,
>>>
>>>> > > then? Because that's likely done in neither the UI nor FILE
>>>>
>>> threads.
>>>
>>>> >
>>>> > The release may cause the object to be deleted. This means that the
>>>>
>>> classes
>>>
>>>> > destructor and all member variables need to be thread safe which
>>>>
>>> they are not.
>>>
>>>  Docs specifically say it is safe to delete WeakPtr on another thread.
>>>>
>>> Which
>>>
>>>> member variable is the concern?
>>>>
>>>
>>>  I'm still a bit worried that there's a hole here where WeakPtr can be
>>>> invalidated one place and then called, but I think I've read it
>>>>
>>> carefully enough
>>>
>>>> now that I'm sure that won't happen. Still, it is a bit subtle. I
>>>>
>>> currently
>>>
>>>> believe that if the caller manages itself on a single thread, the way
>>>>
>>> WeakPtr
>>>
>>>> wants to be, that this class is OK.
>>>>
>>>
>>> I don't think you are correct.
>>>
>>> Basically WeakPtr<Foo> is the same as Foo* except that it does some
>>> magic when Foo is deleted.
>>>
>>> So instead of WeakPtr<Foo> my_ptr, lets imagine you just have Foo*
>>> my_ptr. In this function you are referencing my_ptr at the same time
>>> another function (lets say setListener) might be referencing it as well.
>>>
>>> To fix this you either have to make this class thread safe or do a (very
>>> very very simple) modification I suggested 6 days ago.
>>>
>>>
https://codereview.chromium.**org/11442057/<https://codereview.chromium.org/1...
>>>
>>
>>
>

sail

https://codereview.chromium.org/11442057/diff/52004/chrome/browser/system_monitor/image_capture_device.mm File chrome/browser/system_monitor/image_capture_device.mm (right): https://codereview.chromium.org/11442057/diff/52004/chrome/browser/system_monitor/image_capture_device.mm#newcode27 chrome/browser/system_monitor/image_capture_device.mm:27: scoped_ptr<base::PlatformFileError> result_deleter(result); I think there's a memory leak here. ...

8 years ago (2012-12-20 01:08:24 UTC) #37

Greg Billock

https://codereview.chromium.org/11442057/diff/52004/chrome/browser/system_monitor/image_capture_device.mm File chrome/browser/system_monitor/image_capture_device.mm (right): https://codereview.chromium.org/11442057/diff/52004/chrome/browser/system_monitor/image_capture_device.mm#newcode27 chrome/browser/system_monitor/image_capture_device.mm:27: scoped_ptr<base::PlatformFileError> result_deleter(result); On 2012/12/20 01:08:24, sail wrote: > I ...

8 years ago (2012-12-20 16:31:52 UTC) #38

sail

https://codereview.chromium.org/11442057/diff/52004/chrome/browser/system_monitor/image_capture_device.mm File chrome/browser/system_monitor/image_capture_device.mm (right): https://codereview.chromium.org/11442057/diff/52004/chrome/browser/system_monitor/image_capture_device.mm#newcode27 chrome/browser/system_monitor/image_capture_device.mm:27: scoped_ptr<base::PlatformFileError> result_deleter(result); On 2012/12/20 16:31:52, Greg Billock wrote: > ...

8 years ago (2012-12-20 18:34:53 UTC) #39

Greg Billock

https://codereview.chromium.org/11442057/diff/52004/chrome/browser/system_monitor/image_capture_device.mm File chrome/browser/system_monitor/image_capture_device.mm (right): https://codereview.chromium.org/11442057/diff/52004/chrome/browser/system_monitor/image_capture_device.mm#newcode150 chrome/browser/system_monitor/image_capture_device.mm:150: - (void)didDownloadFile:(ICCameraFile*)file On 2012/12/20 18:34:54, sail wrote: > now ...

8 years ago (2012-12-20 23:35:26 UTC) #40

sail

https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm File chrome/browser/system_monitor/image_capture_device_manager_unittest.mm (right): https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode159 chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:159: downloads_.push_back(name); check the thread ID here? https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode193 chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:193: ICCameraDevice* ...

8 years ago (2012-12-21 02:47:10 UTC) #41

Greg Billock

https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm File chrome/browser/system_monitor/image_capture_device_manager_unittest.mm (right): https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode159 chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:159: downloads_.push_back(name); On 2012/12/21 02:47:10, sail wrote: > check the ...

8 years ago (2012-12-21 20:05:37 UTC) #42

https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
(right):

https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:159:
downloads_.push_back(name);
On 2012/12/21 02:47:10, sail wrote:
> check the thread ID here?

Done.

https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:294:
[base::mac::ObjCCastStrict<MockICCameraDevice>(device)
I wanted to do this, but when I did, the call to 
[delegate deviceBrowser:nil didAddDevice:device moreComing:NO]; above
fails, presumably because the compile-time type id is different, and so doesn't
bind to the right message handler? I wrestled with that for a while, but in the
end, just went back to this. It seems like there must be a better way, though.

On 2012/12/21 02:47:10, sail wrote:
> should remove the cast and have AttachedDevice return a MockICCameraDevice
type.

https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:301:
ASSERT_EQ(0U, listener_.downloads().size());
On 2012/12/21 02:47:10, sail wrote:
> expect_eq

Done.

https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:304:
localPath:temp_dir.path().Append("tempfile")];
On 2012/12/21 02:47:10, sail wrote:
> make tempfile a const above this line?

Done.

https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:310:
[camera downloadFile:std::string("pic1")
Done, kinda. I can't figure out how to make @"pic1" use the constant. All I get
is double-deletes, which is a pretty shocking outcome for changing the init
method to take a copy and passing in [NSString stringWithUTF8String:kxxx]. I
guess I don't understand this stuff as well as I thought. :-/

On 2012/12/21 02:47:10, sail wrote:
> make pic1 a constant above this line

https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:320:
EXPECT_EQ("test", std::string(file_contents, 4));
On 2012/12/21 02:47:10, sail wrote:
> make "test" a constant at the top of this file?

Done.

sail

https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm File chrome/browser/system_monitor/image_capture_device_manager_unittest.mm (right): https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode294 chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:294: [base::mac::ObjCCastStrict<MockICCameraDevice>(device) On 2012/12/21 20:05:38, Greg Billock wrote: > I ...

7 years, 12 months ago (2012-12-26 18:52:37 UTC) #43

Greg Billock

On Wed, Dec 26, 2012 at 10:52 AM, <sail@chromium.org> wrote: > > https://codereview.chromium.**org/11442057/diff/53011/** > chrome/browser/system_monitor/**image_capture_device_manager_**unittest.mm<https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm> ...

7 years, 12 months ago (2012-12-26 20:42:11 UTC) #44

On Wed, Dec 26, 2012 at 10:52 AM, <sail@chromium.org> wrote:

>
> https://codereview.chromium.**org/11442057/diff/53011/**
>
chrome/browser/system_monitor/**image_capture_device_manager_**unittest.mm<https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm>
> File
>
chrome/browser/system_monitor/**image_capture_device_manager_**unittest.mm<http://image_capture_device_manager_unittest.mm>
> (right):
>
> https://codereview.chromium.**org/11442057/diff/53011/**
> chrome/browser/system_monitor/**image_capture_device_manager_**
>
unittest.mm#newcode294<https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode294>
> chrome/browser/system_monitor/**image_capture_device_manager_**
> unittest.mm:294 <http://image_capture_device_manager_unittest.mm:294>:
> [base::mac::ObjCCastStrict<**MockICCameraDevice>(device)
> On 2012/12/21 20:05:38, Greg Billock wrote:
>
>> I wanted to do this, but when I did, the call to
>> [delegate deviceBrowser:nil didAddDevice:device moreComing:NO]; above
>> fails, presumably because the compile-time type id is different, and
>>
> so doesn't
>
>> bind to the right message handler? I wrestled with that for a while,
>>
> but in the
>
>> end, just went back to this. It seems like there must be a better way,
>>
> though.
>
>  On 2012/12/21 02:47:10, sail wrote:
>> > should remove the cast and have AttachedDevice return a
>>
> MockICCameraDevice
>
>> type.
>>
>
>
> Hm.. that's really weird, it shouldn't fail. What's the compile error
> you get?
>

No compile error -- just no runtime message delivery.


>
> https://codereview.chromium.**org/11442057/diff/56001/**
>
chrome/browser/system_monitor/**image_capture_device.mm<https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_monitor/image_capture_device.mm>
> File
chrome/browser/system_monitor/**image_capture_device.mm<http://image_capture_device.mm>(right):
>
> https://codereview.chromium.**org/11442057/diff/56001/**
>
chrome/browser/system_monitor/**image_capture_device.mm#**newcode27<https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_monitor/image_capture_device.mm#newcode27>
>
>
chrome/browser/system_monitor/**image_capture_device.mm:27<http://image_capture_device.mm:27>
> :
> scoped_ptr<base::**PlatformFileError> result_deleter(result);
> DCHECK that this is called on the UI thread
>
> https://codereview.chromium.**org/11442057/diff/56001/**
>
chrome/browser/system_monitor/**image_capture_device.mm#**newcode176<https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_monitor/image_capture_device.mm#newcode176>
>
chrome/browser/system_monitor/**image_capture_device.mm:176<http://image_capture_device.mm:176>:
> //
> ImageCapture did not save the file into the name we give it in the
> give -> gave
>
> https://codereview.chromium.**org/11442057/diff/56001/**
>
chrome/browser/system_monitor/**image_capture_device_manager.h<https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_monitor/image_capture_device_manager.h>
> File chrome/browser/system_monitor/**image_capture_device_manager.h
> (right):
>
> https://codereview.chromium.**org/11442057/diff/56001/**
>
chrome/browser/system_monitor/**image_capture_device_manager.**h#newcode10<https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_monitor/image_capture_device_manager.h#newcode10>
> chrome/browser/system_monitor/**image_capture_device_manager.**h:10:
> #include <string>
> no new line before this, new line after this
>
> https://codereview.chromium.**org/11442057/diff/56001/**
>
chrome/browser/system_monitor/**image_capture_device_manager.**mm<https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_monitor/image_capture_device_manager.mm>
> File
chrome/browser/system_monitor/**image_capture_device_manager.**mm<http://image_capture_device_manager.mm>
> (right):
>
> https://codereview.chromium.**org/11442057/diff/56001/**
>
chrome/browser/system_monitor/**image_capture_device_manager.**mm#newcode9<https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_monitor/image_capture_device_manager.mm#newcode9>
>
chrome/browser/system_monitor/**image_capture_device_manager.**mm:9<http://image_capture_device_manager.mm:9>:
> #import
> "chrome/browser/system_**monitor/image_capture_device.**h"
> not alphabetically ordered
>
> https://codereview.chromium.**org/11442057/diff/56001/**
> chrome/browser/system_monitor/**image_capture_device_manager.**
>
mm#newcode52<https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_monitor/image_capture_device_manager.mm#newcode52>
>
chrome/browser/system_monitor/**image_capture_device_manager.**mm:52<http://image_capture_device_manager.mm:52>
> :
> deviceBrowser_.get().**browsedDeviceTypeMask =
> [deviceBrowser setBrowsedDeviceTypeMask:]
>
> https://codereview.chromium.**org/11442057/diff/56001/**
> chrome/browser/system_monitor/**image_capture_device_manager.**
>
mm#newcode129<https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_monitor/image_capture_device_manager.mm#newcode129>
>
chrome/browser/system_monitor/**image_capture_device_manager.**mm:129<http://image_capture_device_manager.mm:129>
> :
> base::mac::ObjCCastStrict<**ImageCaptureDeviceManagerImpl>**(
> I don't think you need this cast
>
> https://codereview.chromium.**org/11442057/diff/56001/**
>
chrome/browser/system_monitor/**image_capture_device_manager_**unittest.mm<https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm>
> File
>
chrome/browser/system_monitor/**image_capture_device_manager_**unittest.mm<http://image_capture_device_manager_unittest.mm>
> (right):
>
> https://codereview.chromium.**org/11442057/diff/56001/**
> chrome/browser/system_monitor/**image_capture_device_manager_**
>
unittest.mm#newcode84<https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode84>
> chrome/browser/system_monitor/**image_capture_device_manager_**
> unittest.mm:84 <http://image_capture_device_manager_unittest.mm:84>:
> FilePath toBeSaved = saveDir.Append(saveAsFilename)**;
> maybe avoid a new local variable by doing saveDir = saveDir.Append()
>
> https://codereview.chromium.**org/11442057/diff/56001/**
> chrome/browser/system_monitor/**image_capture_device_manager_**
>
unittest.mm#newcode85<https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode85>
> chrome/browser/system_monitor/**image_capture_device_manager_**
> unittest.mm:85 <http://image_capture_device_manager_unittest.mm:85>:
> ASSERT_EQ(static_cast<int>(**strlen(kTestFileContents)),
> EXPECT_EQ ?
>
> https://codereview.chromium.**org/11442057/diff/56001/**
> chrome/browser/system_monitor/**image_capture_device_manager_**
>
unittest.mm#newcode86<https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode86>
> chrome/browser/system_monitor/**image_capture_device_manager_**
> unittest.mm:86 <http://image_capture_device_manager_unittest.mm:86>:
> file_util::WriteFile(**toBeSaved, kTestFileContents,
> 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?
>
> https://codereview.chromium.**org/11442057/diff/56001/**
> chrome/browser/system_monitor/**image_capture_device_manager_**
>
unittest.mm#newcode162<https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode162>
> chrome/browser/system_monitor/**image_capture_device_manager_**
> unittest.mm:162 <http://image_capture_device_manager_unittest.mm:162>:
> DCHECK(content::BrowserThread:**:CurrentlyOn(content::**
> BrowserThread::UI));
> EXPECT_TRUE ?
>
> https://codereview.chromium.**org/11442057/diff/56001/**
> chrome/browser/system_monitor/**image_capture_device_manager_**
>
unittest.mm#newcode182<https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode182>
> chrome/browser/system_monitor/**image_capture_device_manager_**
> unittest.mm:182 <http://image_capture_device_manager_unittest.mm:182>:
> base::PlatformFileError last_error_;
> needs to be initialized in constructor
>
> https://codereview.chromium.**org/11442057/diff/56001/**
> chrome/browser/system_monitor/**image_capture_device_manager_**
>
unittest.mm#newcode197<https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode197>
> chrome/browser/system_monitor/**image_capture_device_manager_**
> unittest.mm:197 <http://image_capture_device_manager_unittest.mm:197>:
>
> ICCameraDevice* device = [MockICCameraDevice alloc];
> should be
> MockICCameraDevice* device = [[[MockICCameraDevice alloc] init]
> autorelease];
>
> https://codereview.chromium.**org/11442057/diff/56001/**
> chrome/browser/system_monitor/**image_capture_device_manager_**
>
unittest.mm#newcode238<https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode238>
> chrome/browser/system_monitor/**image_capture_device_manager_**
> unittest.mm:238 <http://image_capture_device_manager_unittest.mm:238>:
> scoped_nsobject<**ImageCaptureDevice> camera(
> you don't even need the scoped_nsobject here, you could just do:
>   ImageCaptureDevcie* camera =
> chrome::**ImageCaptureDeviceManager::**deviceForUUID(kDeviceId)
>
> (not a big deal)
>
> https://codereview.chromium.**org/11442057/diff/56001/**
> chrome/browser/system_monitor/**image_capture_device_manager_**
>
unittest.mm#newcode270<https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode270>
> chrome/browser/system_monitor/**image_capture_device_manager_**
> unittest.mm:270 <http://image_capture_device_manager_unittest.mm:270>:
> scoped_nsobject<**ImageCaptureDevice> camera(
> same as above, no need for scoped_nsobject + retain
>
> https://codereview.chromium.**org/11442057/diff/56001/**
> chrome/browser/system_monitor/**image_capture_device_manager_**
>
unittest.mm#newcode289<https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode289>
> chrome/browser/system_monitor/**image_capture_device_manager_**
> unittest.mm:289 <http://image_capture_device_manager_unittest.mm:289>:
> scoped_nsobject<**ImageCaptureDevice> camera(
> same as above, no need for scoped_nsobject + retain
>
> https://codereview.chromium.**org/11442057/diff/56001/**
> chrome/browser/system_monitor/**image_capture_device_manager_**
>
unittest.mm#newcode316<https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode316>
> chrome/browser/system_monitor/**image_capture_device_manager_**
> unittest.mm:316 <http://image_capture_device_manager_unittest.mm:316>:
> [camera downloadFile:std::string(**kTestFileName) localPath:temp_file];
> instead of creating std::string here just change the type of
> kTestFileName to an std::string?
>
>
https://codereview.chromium.**org/11442057/<https://codereview.chromium.org/1...
>

sail

https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm File chrome/browser/system_monitor/image_capture_device_manager_unittest.mm (right): https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode294 chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:294: [base::mac::ObjCCastStrict<MockICCameraDevice>(device) > No compile error -- just no runtime ...

7 years, 12 months ago (2012-12-26 20:48:47 UTC) #45

Greg Billock

https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm File chrome/browser/system_monitor/image_capture_device_manager_unittest.mm (right): https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode294 chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:294: [base::mac::ObjCCastStrict<MockICCameraDevice>(device) OK, it must have been some other simultaneous ...

7 years, 11 months ago (2013-01-03 20:59:55 UTC) #46

https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
(right):

https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:294:
[base::mac::ObjCCastStrict<MockICCameraDevice>(device)
OK, it must have been some other simultaneous change I made that messed it up.
It works fine.

On 2012/12/26 20:48:47, sail wrote:
> > No compile error -- just no runtime message delivery.
> 
> There's no strong typing on method dispatch. If doing:
> MockICCameraDevice* foo = ...
> vs
> ICCameraDevcie* foo = ...
> changes things then there's some other problem.
> 
> In this case -[ImageCaptureDeviceManagerImpl
> deviceBrowser:didAddDevice:moreComing:] didn't get called right? I would try
> several things:
>   - EXPECT_TRUE(delegate)
>   - EXPECT_TRUE(device)
>   - printf in deviceBrowser:ddiAddDevice:

https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_device.mm (right):

https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device.mm:27:
scoped_ptr<base::PlatformFileError> result_deleter(result);
On 2012/12/26 18:52:37, sail wrote:
> DCHECK that this is called on the UI thread

Done.

https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device.mm:176: // ImageCapture did
not save the file into the name we give it in the
On 2012/12/26 18:52:37, sail wrote:
> give -> gave

Done.

https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_device_manager.h (right):

https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager.h:10: #include
<string>
On 2012/12/26 18:52:37, sail wrote:
> no new line before this, new line after this

Done.

https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_device_manager.mm (right):

https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager.mm:9: #import
"chrome/browser/system_monitor/image_capture_device.h"
On 2012/12/26 18:52:37, sail wrote:
> not alphabetically ordered

Done.

https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager.mm:52:
deviceBrowser_.get().browsedDeviceTypeMask =
On 2012/12/26 18:52:37, sail wrote:
> [deviceBrowser setBrowsedDeviceTypeMask:]

Done.

https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager.mm:129:
base::mac::ObjCCastStrict<ImageCaptureDeviceManagerImpl>(
On 2012/12/26 18:52:37, sail wrote:
> I don't think you need this cast

Done.

https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
(right):

https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:84:
FilePath toBeSaved = saveDir.Append(saveAsFilename);
On 2012/12/26 18:52:37, sail wrote:
> maybe avoid a new local variable by doing saveDir = saveDir.Append()

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

https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:85:
ASSERT_EQ(static_cast<int>(strlen(kTestFileContents)),
On 2012/12/26 18:52:37, sail wrote:
> EXPECT_EQ ?

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.

https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:86:
file_util::WriteFile(toBeSaved, kTestFileContents,
On 2012/12/26 18:52:37, sail wrote:
> 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?

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.

https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:162:
DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
On 2012/12/26 18:52:37, sail wrote:
> EXPECT_TRUE ?

Done.

https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:182:
base::PlatformFileError last_error_;
On 2012/12/26 18:52:37, sail wrote:
> needs to be initialized in constructor

Done.

https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:197:
ICCameraDevice* device = [MockICCameraDevice alloc];
On 2012/12/26 18:52:37, sail wrote:
> should be
> MockICCameraDevice* device = [[[MockICCameraDevice alloc] init] autorelease];

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.

https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:238:
scoped_nsobject<ImageCaptureDevice> camera(
On 2012/12/26 18:52:37, sail wrote:
> you don't even need the scoped_nsobject here, you could just do:
>   ImageCaptureDevcie* camera =
> chrome::ImageCaptureDeviceManager::deviceForUUID(kDeviceId)
> 
> (not a big deal)

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

https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:316:
[camera downloadFile:std::string(kTestFileName) localPath:temp_file];
On 2012/12/26 18:52:37, sail wrote:
> instead of creating std::string here just change the type of kTestFileName to
an
> std::string?

Done.

sail

https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm File chrome/browser/system_monitor/image_capture_device_manager_unittest.mm (right): https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode294 chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:294: [base::mac::ObjCCastStrict<MockICCameraDevice>(device) On 2013/01/03 20:59:55, Greg Billock wrote: > OK, ...

7 years, 11 months ago (2013-01-03 21:50:28 UTC) #47

https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
(right):

https://codereview.chromium.org/11442057/diff/53011/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:294:
[base::mac::ObjCCastStrict<MockICCameraDevice>(device)
On 2013/01/03 20:59:55, Greg Billock wrote:
> OK, it must have been some other simultaneous change I made that messed it up.
> It works fine.
> 
> On 2012/12/26 20:48:47, sail wrote:
> > > No compile error -- just no runtime message delivery.
> > 
> > There's no strong typing on method dispatch. If doing:
> > MockICCameraDevice* foo = ...
> > vs
> > ICCameraDevcie* foo = ...
> > changes things then there's some other problem.
> > 
> > In this case -[ImageCaptureDeviceManagerImpl
> > deviceBrowser:didAddDevice:moreComing:] didn't get called right? I would try
> > several things:
> >   - EXPECT_TRUE(delegate)
> >   - EXPECT_TRUE(device)
> >   - printf in deviceBrowser:ddiAddDevice:
> 

Ahh cool.

https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
(right):

https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:84:
FilePath toBeSaved = saveDir.Append(saveAsFilename);
On 2013/01/03 20:59:55, Greg Billock wrote:
> On 2012/12/26 18:52:37, sail wrote:
> > maybe avoid a new local variable by doing saveDir = saveDir.Append()
> 
> Could do, It isn't a directory though. I don't think it's that bad.

Ah, ok to leave as is then.

https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:197:
ICCameraDevice* device = [MockICCameraDevice alloc];
On 2013/01/03 20:59:55, Greg Billock wrote:
> On 2012/12/26 18:52:37, sail wrote:
> > should be
> > MockICCameraDevice* device = [[[MockICCameraDevice alloc] init]
autorelease];
> 
> Making this change makes the registrations fail, presumably because of the
> autorelease? 

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.

> 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.

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.

https://codereview.chromium.org/11442057/diff/71003/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_device_manager.mm (right):

https://codereview.chromium.org/11442057/diff/71003/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager.mm:103: [cameras_
removeObject:device];
At this point device may be deleted. You can do something like this to make sure
it stays alive:
  scoped_nsobject<IDDevice> deviceKeepAlive([device retain]);

https://codereview.chromium.org/11442057/diff/71003/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
(right):

https://codereview.chromium.org/11442057/diff/71003/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:290: new
content::TestBrowserThread(
this needs to be indented 2 more spaces (thus the next line also needs to be
indented).

https://codereview.chromium.org/11442057/diff/71003/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:306:
[[MockICCameraFile alloc] init:@"pic1"]);
this can be base::SysUTF8ToNSString(kTestFileName)

https://codereview.chromium.org/11442057/diff/71003/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:315:
FilePath temp_file = temp_dir.path().Append("tempfile");
high level comments about what you're testing would be good

Greg Billock

https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm File chrome/browser/system_monitor/image_capture_device_manager_unittest.mm (right): https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode197 chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:197: ICCameraDevice* device = [MockICCameraDevice alloc]; On 2013/01/03 21:50:28, sail ...

7 years, 11 months ago (2013-01-03 23:09:42 UTC) #48

https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
(right):

https://codereview.chromium.org/11442057/diff/56001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:197:
ICCameraDevice* device = [MockICCameraDevice alloc];
On 2013/01/03 21:50:28, sail wrote:
> On 2013/01/03 20:59:55, Greg Billock wrote:
> > On 2012/12/26 18:52:37, sail wrote:
> > > should be
> > > MockICCameraDevice* device = [[[MockICCameraDevice alloc] init]
> autorelease];
> > 
> > Making this change makes the registrations fail, presumably because of the
> > autorelease? 
> 
> 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.
> 
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/11442057/diff/71003/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_device_manager.mm (right):

https://codereview.chromium.org/11442057/diff/71003/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager.mm:103: [cameras_
removeObject:device];
On 2013/01/03 21:50:28, sail wrote:
> At this point device may be deleted. You can do something like this to make
sure
> it stays alive:
>   scoped_nsobject<IDDevice> deviceKeepAlive([device retain]);

I just need it to get the UUIDString. I'll do that above.

https://codereview.chromium.org/11442057/diff/71003/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
(right):

https://codereview.chromium.org/11442057/diff/71003/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:290: new
content::TestBrowserThread(
On 2013/01/03 21:50:28, sail wrote:
> this needs to be indented 2 more spaces (thus the next line also needs to be
> indented).

Done.

https://codereview.chromium.org/11442057/diff/71003/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:306:
[[MockICCameraFile alloc] init:@"pic1"]);
On 2013/01/03 21:50:28, sail wrote:
> this can be base::SysUTF8ToNSString(kTestFileName)

Done. But I think it ends up that using literals is more legible than all this
fiddling with the constant.

https://codereview.chromium.org/11442057/diff/71003/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:315:
FilePath temp_file = temp_dir.path().Append("tempfile");
On 2013/01/03 21:50:28, sail wrote:
> high level comments about what you're testing would be good

Done.

sail

https://codereview.chromium.org/11442057/diff/71003/chrome/browser/system_monitor/image_capture_device_manager.mm File chrome/browser/system_monitor/image_capture_device_manager.mm (right): https://codereview.chromium.org/11442057/diff/71003/chrome/browser/system_monitor/image_capture_device_manager.mm#newcode103 chrome/browser/system_monitor/image_capture_device_manager.mm:103: [cameras_ removeObject:device]; On 2013/01/03 23:09:42, Greg Billock wrote: > ...

7 years, 11 months ago (2013-01-04 00:00:36 UTC) #49

sail

Here's the patch I applied to get all the tests to pass on my machine: ...

7 years, 11 months ago (2013-01-04 01:43:02 UTC) #50

Here's the patch I applied to get all the tests to pass on my machine:

diff --git
a/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
b/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
index 5154da1..787bfb4 100644
--- a/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
+++ b/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
@@ -25,6 +25,11 @@ const char kTestFileContents[] = "test";
 
 }  // namespace
 
+// Private ICCamerDevice method needed to properly initialize the object.
+@interface NSObject (PrivateAPIICCamerDevice)
+- (id)initWithDictionary:(id)properties;
+@end
+
 @interface MockICCameraDevice : ICCameraDevice {
  @private
   scoped_nsobject<NSMutableArray> allMediaFiles_;
@@ -36,6 +41,12 @@ const char kTestFileContents[] = "test";
 
 @implementation MockICCameraDevice
 
+- (id)init {
+  if ((self = [super initWithDictionary:[NSDictionary dictionary]])) {
+  }
+  return self;
+}
+
 - (NSString*)mountPoint {
   return @"mountPoint";
 }
@@ -116,7 +127,7 @@ const char kTestFileContents[] = "test";
 
 - (id)init:(NSString*)name {
   if ((self = [super init])) {
-    name_.reset([NSString stringWithString:name]);
+    name_.reset([name retain]);
     date_.reset([[NSDate dateWithNaturalLanguageString:@"12/12/12"] retain]);
   }
   return self;
@@ -201,9 +212,7 @@ class ImageCaptureDeviceManagerTest : public testing::Test {
   MockICCameraDevice* AttachDevice(
       chrome::ImageCaptureDeviceManager* manager) {
     // Ownership will be passed to the device browser delegate.
-    MockICCameraDevice* device = [MockICCameraDevice alloc];
-    //[device init];  <--- causes the MockICCameraDevice to throw on exit
-    //[device autorelease];
+    MockICCameraDevice* device = [[MockICCameraDevice alloc] init];
 LOG(INFO) << "camera is " << device;
     id<ICDeviceBrowserDelegate> delegate = manager->device_browser();
     [delegate deviceBrowser:nil didAddDevice:device moreComing:NO];
@@ -306,12 +315,8 @@ LOG(INFO) << "camera=" << camera.get();
 
   std::string kTestFileName("pic1");
 
-  //scoped_nsobject<MockICCameraFile> picture1(
-  //    [[MockICCameraFile alloc]
-  //        init:base::SysUTF8ToNSString(kTestFileName)]); <--- is the cause of
the double free
-  scoped_nsobject<MockICCameraFile> picture1(
-      [[MockICCameraFile alloc] init:@"pic1"]);
-LOG(INFO) << "pic1 = " << picture1.get();
+  scoped_nsobject<MockICCameraFile> picture1([[MockICCameraFile alloc]
+      init:base::SysUTF8ToNSString(kTestFileName)]);
   [device addMediaFile:picture1];
   [camera cameraDevice:nil didAddItem:picture1];
 
@@ -345,4 +350,6 @@ LOG(INFO) << "pic1 = " << picture1.get();
                                    strlen(kTestFileContents)));
   EXPECT_EQ(kTestFileContents,
             std::string(file_contents, strlen(kTestFileContents)));
+
+  [camera didRemoveDevice:device];
 }

Greg Billock

On 2013/01/04 01:43:02, sail wrote: > Here's the patch I applied to get all the ...

7 years, 11 months ago (2013-01-04 16:26:00 UTC) #51

On 2013/01/04 01:43:02, sail wrote:
> Here's the patch I applied to get all the tests to pass on my machine:
> 
> diff --git
> a/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
> b/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
> index 5154da1..787bfb4 100644
> --- a/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
> +++ b/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
> @@ -25,6 +25,11 @@ const char kTestFileContents[] = "test";
>  
>  }  // namespace
>  
> +// Private ICCamerDevice method needed to properly initialize the object.
> +@interface NSObject (PrivateAPIICCamerDevice)
> +- (id)initWithDictionary:(id)properties;
> +@end
> +
>  @interface MockICCameraDevice : ICCameraDevice {
>   @private
>    scoped_nsobject<NSMutableArray> allMediaFiles_;
> @@ -36,6 +41,12 @@ const char kTestFileContents[] = "test";
>  
>  @implementation MockICCameraDevice
>  
> +- (id)init {
> +  if ((self = [super initWithDictionary:[NSDictionary dictionary]])) {
> +  }
> +  return self;
> +}
> +
>  - (NSString*)mountPoint {
>    return @"mountPoint";
>  }
> @@ -116,7 +127,7 @@ const char kTestFileContents[] = "test";
>  
>  - (id)init:(NSString*)name {
>    if ((self = [super init])) {
> -    name_.reset([NSString stringWithString:name]);
> +    name_.reset([name retain]);
>      date_.reset([[NSDate dateWithNaturalLanguageString:@"12/12/12"] retain]);
>    }
>    return self;
> @@ -201,9 +212,7 @@ class ImageCaptureDeviceManagerTest : public testing::Test
{
>    MockICCameraDevice* AttachDevice(
>        chrome::ImageCaptureDeviceManager* manager) {
>      // Ownership will be passed to the device browser delegate.
> -    MockICCameraDevice* device = [MockICCameraDevice alloc];
> -    //[device init];  <--- causes the MockICCameraDevice to throw on exit
> -    //[device autorelease];
> +    MockICCameraDevice* device = [[MockICCameraDevice alloc] init];
>  LOG(INFO) << "camera is " << device;
>      id<ICDeviceBrowserDelegate> delegate = manager->device_browser();
>      [delegate deviceBrowser:nil didAddDevice:device moreComing:NO];
> @@ -306,12 +315,8 @@ LOG(INFO) << "camera=" << camera.get();
>  
>    std::string kTestFileName("pic1");
>  
> -  //scoped_nsobject<MockICCameraFile> picture1(
> -  //    [[MockICCameraFile alloc]
> -  //        init:base::SysUTF8ToNSString(kTestFileName)]); <--- is the cause
of
> the double free
> -  scoped_nsobject<MockICCameraFile> picture1(
> -      [[MockICCameraFile alloc] init:@"pic1"]);
> -LOG(INFO) << "pic1 = " << picture1.get();
> +  scoped_nsobject<MockICCameraFile> picture1([[MockICCameraFile alloc]
> +      init:base::SysUTF8ToNSString(kTestFileName)]);
>    [device addMediaFile:picture1];
>    [camera cameraDevice:nil didAddItem:picture1];
>  
> @@ -345,4 +350,6 @@ LOG(INFO) << "pic1 = " << picture1.get();
>                                     strlen(kTestFileContents)));
>    EXPECT_EQ(kTestFileContents,
>              std::string(file_contents, strlen(kTestFileContents)));
> +
> +  [camera didRemoveDevice:device];
>  }

OK, applied pretty much this and it is repaired. :-)

Nico, added you for base/mac owners. Can you have a look? I think this is pretty
much ready now, right?

Greg Billock

https://codereview.chromium.org/11442057/diff/71003/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm File chrome/browser/system_monitor/image_capture_device_manager_unittest.mm (right): https://codereview.chromium.org/11442057/diff/71003/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode306 chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:306: [[MockICCameraFile alloc] init:@"pic1"]); On 2013/01/04 00:00:36, sail wrote: > ...

7 years, 11 months ago (2013-01-04 16:26:10 UTC) #52

sail

Looks good, just a few nits left. https://codereview.chromium.org/11442057/diff/71004/chrome/browser/system_monitor/image_capture_device_manager.h File chrome/browser/system_monitor/image_capture_device_manager.h (right): https://codereview.chromium.org/11442057/diff/71004/chrome/browser/system_monitor/image_capture_device_manager.h#newcode19 chrome/browser/system_monitor/image_capture_device_manager.h:19: // This ...

7 years, 11 months ago (2013-01-04 18:58:56 UTC) #53

Greg Billock

https://codereview.chromium.org/11442057/diff/71004/chrome/browser/system_monitor/image_capture_device_manager.h File chrome/browser/system_monitor/image_capture_device_manager.h (right): https://codereview.chromium.org/11442057/diff/71004/chrome/browser/system_monitor/image_capture_device_manager.h#newcode19 chrome/browser/system_monitor/image_capture_device_manager.h:19: // This wrapper basically allows the owner to have ...

7 years, 11 months ago (2013-01-04 21:05:50 UTC) #54

sail

https://codereview.chromium.org/11442057/diff/71004/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm File chrome/browser/system_monitor/image_capture_device_manager_unittest.mm (right): https://codereview.chromium.org/11442057/diff/71004/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode216 chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:216: MockICCameraDevice* device = [[MockICCameraDevice alloc] init]; On 2013/01/04 21:05:50, ...

7 years, 11 months ago (2013-01-04 21:21:43 UTC) #55

https://codereview.chromium.org/11442057/diff/71004/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
(right):

https://codereview.chromium.org/11442057/diff/71004/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:216:
MockICCameraDevice* device = [[MockICCameraDevice alloc] init];
On 2013/01/04 21:05:50, Greg Billock wrote:
> Looks good. Done. But autorelease() causes a crash; used release() instead,
> which may have been what you meant.
> 
> On 2013/01/04 18:58:56, sail wrote:
> > autorelease (sorry this was missing from my patch)
> > 
> > or even better:
> > scoped_nsobject<MockICCameraDevice> device([[MockICCameraDevice alloc]
init]);
> > ...
> > return device.autorelease();
> 

I applied the following patch on my machine and I don't see a crash:
diff --git
a/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
b/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
index 8e11f4d..397afbd 100644
--- a/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
+++ b/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
@@ -213,10 +213,10 @@ class ImageCaptureDeviceManagerTest : public testing::Test
{
       chrome::ImageCaptureDeviceManager* manager) {
     // Ownership will be passed to the device browser delegate.
     scoped_nsobject<MockICCameraDevice> device(
-        [[[MockICCameraDevice alloc] init] autorelease]);
+        [[MockICCameraDevice alloc] init]);
     id<ICDeviceBrowserDelegate> delegate = manager->device_browser();
     [delegate deviceBrowser:nil didAddDevice:device moreComing:NO];
-    return device.release();
+    return device.autorelease();
   }

   void DetachDevice(chrome::ImageCaptureDeviceManager* manager,

https://codereview.chromium.org/11442057/diff/83001/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_device_manager.h (right):

https://codereview.chromium.org/11442057/diff/83001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager.h:19: // Upon
creation, begins monitoring the ImageCapture API for any attached
begins monitoring for any attached devices using the ImageCapture API

https://codereview.chromium.org/11442057/diff/83001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager.h:21: // USB cards)
using the SystemMonitor, and makes them available using
no comma after SystemMonitor ?

Greg Billock

https://codereview.chromium.org/11442057/diff/71004/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm File chrome/browser/system_monitor/image_capture_device_manager_unittest.mm (right): https://codereview.chromium.org/11442057/diff/71004/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode216 chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:216: MockICCameraDevice* device = [[MockICCameraDevice alloc] init]; Oh; I'd forgotten ...

7 years, 11 months ago (2013-01-04 22:29:25 UTC) #56

https://codereview.chromium.org/11442057/diff/71004/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
(right):

https://codereview.chromium.org/11442057/diff/71004/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:216:
MockICCameraDevice* device = [[MockICCameraDevice alloc] init];
Oh; I'd forgotten to take out my autorelease there. I assume scoped_nsobject is
doing the refcount? Every time I feel like I've figured this out, it turns out I
was wrong. I thought when I read scoped_nsobject, it specifically said it does
*not* inc the refcount.

On 2013/01/04 21:21:43, sail wrote:
> On 2013/01/04 21:05:50, Greg Billock wrote:
> > Looks good. Done. But autorelease() causes a crash; used release() instead,
> > which may have been what you meant.
> > 
> > On 2013/01/04 18:58:56, sail wrote:
> > > autorelease (sorry this was missing from my patch)
> > > 
> > > or even better:
> > > scoped_nsobject<MockICCameraDevice> device([[MockICCameraDevice alloc]
> init]);
> > > ...
> > > return device.autorelease();
> > 
> 
> I applied the following patch on my machine and I don't see a crash:
> diff --git
> a/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
> b/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
> index 8e11f4d..397afbd 100644
> --- a/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
> +++ b/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm
> @@ -213,10 +213,10 @@ class ImageCaptureDeviceManagerTest : public
testing::Test
> {
>        chrome::ImageCaptureDeviceManager* manager) {
>      // Ownership will be passed to the device browser delegate.
>      scoped_nsobject<MockICCameraDevice> device(
> -        [[[MockICCameraDevice alloc] init] autorelease]);
> +        [[MockICCameraDevice alloc] init]);
>      id<ICDeviceBrowserDelegate> delegate = manager->device_browser();
>      [delegate deviceBrowser:nil didAddDevice:device moreComing:NO];
> -    return device.release();
> +    return device.autorelease();
>    }
>  
>    void DetachDevice(chrome::ImageCaptureDeviceManager* manager,

https://codereview.chromium.org/11442057/diff/83001/chrome/browser/system_mon...
File chrome/browser/system_monitor/image_capture_device_manager.h (right):

https://codereview.chromium.org/11442057/diff/83001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager.h:19: // Upon
creation, begins monitoring the ImageCapture API for any attached
On 2013/01/04 21:21:43, sail wrote:
> begins monitoring for any attached devices using the ImageCapture API

Done.

https://codereview.chromium.org/11442057/diff/83001/chrome/browser/system_mon...
chrome/browser/system_monitor/image_capture_device_manager.h:21: // USB cards)
using the SystemMonitor, and makes them available using
sure

On 2013/01/04 21:21:43, sail wrote:
> no comma after SystemMonitor ?

Greg Billock

On 2013/01/05 01:11:22, Lei Zhang wrote: > +mark for base/ Nico and/or Mark, can you ...

7 years, 11 months ago (2013-01-07 22:00:06 UTC) #59

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/11442057/86001

7 years, 11 months ago (2013-01-07 23:30:43 UTC) #62

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/11442057/83004

7 years, 11 months ago (2013-01-08 16:41:55 UTC) #64

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/11442057/85008

7 years, 11 months ago (2013-01-08 17:35:31 UTC) #65

commit-bot: I haz the power

Step "update" is always a major failure. Look at the try server FAQ for more ...

7 years, 11 months ago (2013-01-08 18:50:42 UTC) #66

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/11442057/113001

7 years, 11 months ago (2013-01-08 22:05:05 UTC) #67

commit-bot: I haz the power

Retried try job too often on mac_rel for step(s) unit_tests

7 years, 11 months ago (2013-01-09 03:25:01 UTC) #68

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/11442057/114015

7 years, 11 months ago (2013-01-09 21:15:24 UTC) #69

sail

https://codereview.chromium.org/11442057/diff/120007/chrome/browser/chrome_browser_main_mac.mm File chrome/browser/chrome_browser_main_mac.mm (right): https://codereview.chromium.org/11442057/diff/120007/chrome/browser/chrome_browser_main_mac.mm#newcode287 chrome/browser/chrome_browser_main_mac.mm:287: image_capture_device_manager_.reset(new chrome::ImageCaptureDeviceManager); I think this should only be created ...

7 years, 11 months ago (2013-01-10 18:34:31 UTC) #71

Greg Billock

10.6 is still showing ~30% in some surveys (NetMarketShare). Do you think it's not worth ...

7 years, 11 months ago (2013-01-10 19:24:10 UTC) #72

10.6 is still showing ~30% in some surveys (NetMarketShare). Do you think
it's not worth the effort? Or just disable until we can figure out what the
ImageCapture library is doing on it?


On Thu, Jan 10, 2013 at 10:34 AM, <sail@chromium.org> wrote:

>
> https://codereview.chromium.**org/11442057/diff/120007/**
>
chrome/browser/chrome_browser_**main_mac.mm<https://codereview.chromium.org/11442057/diff/120007/chrome/browser/chrome_browser_main_mac.mm>
> File
chrome/browser/chrome_browser_**main_mac.mm<http://chrome_browser_main_mac.mm>(right):
>
> https://codereview.chromium.**org/11442057/diff/120007/**
>
chrome/browser/chrome_browser_**main_mac.mm#newcode287<https://codereview.chromium.org/11442057/diff/120007/chrome/browser/chrome_browser_main_mac.mm#newcode287>
>
chrome/browser/chrome_browser_**main_mac.mm:287<http://chrome_browser_main_mac.mm:287>
> :
> image_capture_device_manager_.**reset(new
> chrome::**ImageCaptureDeviceManager);
> I think this should only be created for 10.7 and higher
>
> https://codereview.chromium.**org/11442057/diff/120007/**
>
chrome/browser/system_monitor/**image_capture_device.mm<https://codereview.chromium.org/11442057/diff/120007/chrome/browser/system_monitor/image_capture_device.mm>
> File
chrome/browser/system_monitor/**image_capture_device.mm<http://image_capture_device.mm>(right):
>
> https://codereview.chromium.**org/11442057/diff/120007/**
>
chrome/browser/system_monitor/**image_capture_device.mm#**newcode160<https://codereview.chromium.org/11442057/diff/120007/chrome/browser/system_monitor/image_capture_device.mm#newcode160>
>
chrome/browser/system_monitor/**image_capture_device.mm:160<http://image_capture_device.mm:160>:
> if (listener_
> && !base::mac::IsOSLionOrLater())
> the 10.6 specific code should be removed
>
> https://codereview.chromium.**org/11442057/diff/120007/**
>
chrome/browser/system_monitor/**image_capture_device_manager_**unittest.mm<https://codereview.chromium.org/11442057/diff/120007/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm>
> File
>
chrome/browser/system_monitor/**image_capture_device_manager_**unittest.mm<http://image_capture_device_manager_unittest.mm>
> (right):
>
> https://codereview.chromium.**org/11442057/diff/120007/**
> chrome/browser/system_monitor/**image_capture_device_manager_**
>
unittest.mm#newcode33<https://codereview.chromium.org/11442057/diff/120007/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode33>
> chrome/browser/system_monitor/**image_capture_device_manager_**
> unittest.mm:33 <http://image_capture_device_manager_unittest.mm:33>:
> - (CGFloat)backingScaleFactor;
> don't need this and the one below
>
>
https://codereview.chromium.**org/11442057/<https://codereview.chromium.org/1...
>

sail

On 2013/01/10 19:24:10, Greg Billock wrote: > 10.6 is still showing ~30% in some surveys ...

7 years, 11 months ago (2013-01-10 20:18:53 UTC) #73

Greg Billock

OK, took out the 10.6 support. We can assess that later. I think I've gotten ...

7 years, 11 months ago (2013-01-10 21:42:15 UTC) #74

Greg Billock

On 2013/01/10 21:47:14, sail wrote: > new changes LGTM! ok, here we go, CQ roulette. ...

7 years, 11 months ago (2013-01-10 21:50:31 UTC) #76

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/11442057/123002

7 years, 11 months ago (2013-01-10 21:51:51 UTC) #77

sail

https://chromiumcodereview.appspot.com/11442057/diff/123002/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm File chrome/browser/system_monitor/image_capture_device_manager_unittest.mm (right): https://chromiumcodereview.appspot.com/11442057/diff/123002/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode21 chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:21: #if !defined(MAC_OS_X_VERSION_10_6) || \ I think this should be ...

7 years, 11 months ago (2013-01-11 03:38:15 UTC) #79

Greg Billock

https://chromiumcodereview.appspot.com/11442057/diff/123002/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm File chrome/browser/system_monitor/image_capture_device_manager_unittest.mm (right): https://chromiumcodereview.appspot.com/11442057/diff/123002/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode21 chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:21: #if !defined(MAC_OS_X_VERSION_10_6) || \ On 2013/01/11 03:38:15, sail wrote: ...

7 years, 11 months ago (2013-01-11 16:17:21 UTC) #80

Greg Billock

https://chromiumcodereview.appspot.com/11442057/diff/123002/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm File chrome/browser/system_monitor/image_capture_device_manager_unittest.mm (right): https://chromiumcodereview.appspot.com/11442057/diff/123002/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode24 chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:24: @interface ICCameraDeviceDelegate (SnowLeopardAPI) On 2013/01/11 03:38:15, sail wrote: > ...

7 years, 11 months ago (2013-01-11 16:56:11 UTC) #81

Greg Billock

https://chromiumcodereview.appspot.com/11442057/diff/123002/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm File chrome/browser/system_monitor/image_capture_device_manager_unittest.mm (right): https://chromiumcodereview.appspot.com/11442057/diff/123002/chrome/browser/system_monitor/image_capture_device_manager_unittest.mm#newcode21 chrome/browser/system_monitor/image_capture_device_manager_unittest.mm:21: #if !defined(MAC_OS_X_VERSION_10_6) || \ Oh, drat. I see. I ...

7 years, 11 months ago (2013-01-11 17:13:55 UTC) #82

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/11442057/127004

7 years, 11 months ago (2013-01-11 18:05:36 UTC) #83

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/11442057/153001

7 years, 11 months ago (2013-01-15 00:30:08 UTC) #85

Message was sent while issue was closed.

Change committed as 176797

Issue 11442057: [Media Galleries] Add an ImageCaptureCore listener for Mac. (part 2) (Closed)

Description

Patch Set 1 #

Patch Set 2 : Correct base cl #

Patch Set 3 : Better ObjC #

Patch Set 4 : Lots of fixes #

Patch Set 5 : Use weak ptr for listener. #

Patch Set 6 : Device browser test #

Patch Set 7 : Review comments #

Patch Set 8 : Add test for ImageCaptureCameraInterface #

Patch Set 9 : Add accessor for device browser #

Patch Set 10 : Add removal test #

Patch Set 11 : Rename classes #

Patch Set 12 : Pool-delegate all listener callbacks #

Patch Set 13 : Delete unused factory #

Patch Set 14 : Switch to only use the UI thread. #

Patch Set 15 : Add test for downloadFile #

Patch Set 16 : Use constants #

Patch Set 17 : Casts and comments #

Patch Set 18 : Take out header #

Patch Set 19 : Remove include line #

Patch Set 20 : Using init, but now double-free'ing #

Patch Set 21 : Mark spots where doing what seems sensible is leading to test crashes #

Patch Set 22 : Add overloads to get the mock camera device initialized properly #

Patch Set 23 : Fix comment #

Patch Set 24 : Get autorelease right #

Patch Set 25 : Take out mountPoint #

Patch Set 26 : Fix paren #

Patch Set 27 : Rebase to head #

Patch Set 28 : Allocate array, handle deviceDidBecomeReady #

Patch Set 29 : Add declarations for 10.8 methods #

Patch Set 30 : Don't run ImageCapture on 10.6 #

Patch Set 31 : Fix 10.6 declarations #

Patch Set 32 : Ifdef fix #

Patch Set 33 : Add casts for 10.6 #

Patch Set 34 : Repair patch #

Patch Set 35 : the sequel #

Patch Set 36 : Don't use second cast #

Messages