Add Media device notification to SystemMonitor and Mac impl

Add Media device notification to SystemMonitor and Mac impl

BUG=110400
TEST=run chrome with -v=1, attach and a mass storage media device and observe the log entries for the attach and detach events.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=124288

[vandebo (ex-Chrome), 2012-02-08 00:41:30]

Tony noticed that there's already a device notification mechanism in
base/system_monitor.  Will Chan thinks it's reasonable to put our media stuff
here, so here's a new framework.  We can send it out for review once we get at
least one implementation.

[tpayne, 2012-02-08 16:20:30]

https://chromiumcodereview.appspot.com/9363008/diff/1/base/system_monitor/sys...
File base/system_monitor/system_monitor.cc (right):

https://chromiumcodereview.appspot.com/9363008/diff/1/base/system_monitor/sys...
base/system_monitor/system_monitor.cc:88: NotifyDevicesChanged();
At least on windows, this will end up getting called twice because windows
generates 2 separate events for attach/detach.

https://chromiumcodereview.appspot.com/9363008/diff/1/base/system_monitor/sys...
File base/system_monitor/system_monitor.h (right):

https://chromiumcodereview.appspot.com/9363008/diff/1/base/system_monitor/sys...
base/system_monitor/system_monitor.h:11: #include "base/file_path.h"
I thought forward declarations are preferred over including the .h file

[vandebo (ex-Chrome), 2012-02-08 18:37:01]

https://chromiumcodereview.appspot.com/9363008/diff/1/base/system_monitor/sys...
File base/system_monitor/system_monitor.cc (right):

https://chromiumcodereview.appspot.com/9363008/diff/1/base/system_monitor/sys...
base/system_monitor/system_monitor.cc:88: NotifyDevicesChanged();
On 2012/02/08 16:20:30, tpayne wrote:
> At least on windows, this will end up getting called twice because windows
> generates 2 separate events for attach/detach.

Yea, I realized that on my drive home.  Maybe we should make DeviceChanged more
specific - is it only USB devices that trigger it now?

https://chromiumcodereview.appspot.com/9363008/diff/1/base/system_monitor/sys...
File base/system_monitor/system_monitor.h (right):

https://chromiumcodereview.appspot.com/9363008/diff/1/base/system_monitor/sys...
base/system_monitor/system_monitor.h:11: #include "base/file_path.h"
On 2012/02/08 16:20:30, tpayne wrote:
> I thought forward declarations are preferred over including the .h file

Right, but the bind code, which is used by the observer code, needs the full
definition.

[tpayne, 2012-02-08 20:46:43]

https://chromiumcodereview.appspot.com/9363008/diff/1/base/system_monitor/sys...
File base/system_monitor/system_monitor_unittest.cc (right):

https://chromiumcodereview.appspot.com/9363008/diff/1/base/system_monitor/sys...
base/system_monitor/system_monitor_unittest.cc:152:
system_monitor.ProcessMediaDeviceAttached("media device", FilePath("path"));
FilePath("path") does not compile on Windows, which requires a wchar_t

[Lei Zhang, 2012-02-08 23:34:13]

https://chromiumcodereview.appspot.com/9363008/diff/1/base/system_monitor/sys...
File base/system_monitor/system_monitor_unittest.cc (right):

https://chromiumcodereview.appspot.com/9363008/diff/1/base/system_monitor/sys...
base/system_monitor/system_monitor_unittest.cc:152:
system_monitor.ProcessMediaDeviceAttached("media device", FilePath("path"));
On 2012/02/08 20:46:44, tpayne wrote:
> FilePath("path") does not compile on Windows, which requires a wchar_t

You need FILE_PATH_LITERAL here.

[vandebo (ex-Chrome), 2012-02-10 01:50:21]

Will, Chris, PTAL

[csilv, 2012-02-13 05:49:28]

lgtm

http://codereview.chromium.org/9363008/diff/13001/base/system_monitor/system_...
File base/system_monitor/system_monitor_mac.mm (right):

http://codereview.chromium.org/9363008/diff/13001/base/system_monitor/system_...
base/system_monitor/system_monitor_mac.mm:10: #import <Carbon/Carbon.h>
use #include instead, #import only for pure obj-c class headers.

[Lei Zhang, 2012-02-14 01:07:22]

http://codereview.chromium.org/9363008/diff/13001/base/system_monitor/system_...
File base/system_monitor/system_monitor.cc (right):

http://codereview.chromium.org/9363008/diff/13001/base/system_monitor/system_...
base/system_monitor/system_monitor.cc:14: static SystemMonitor* g_system_monitor
= NULL;
Should this be a LazyInstance?

[Lei Zhang, 2012-02-14 01:58:01]

http://codereview.chromium.org/9363008/diff/13001/base/system_monitor/system_...
File base/system_monitor/system_monitor.cc (right):

http://codereview.chromium.org/9363008/diff/13001/base/system_monitor/system_...
base/system_monitor/system_monitor.cc:150: void SystemMonitor::BatteryCheck() {
Also, this can be wrapped in #if defined(ENABLE_BATTERY_MONITORING)

[Lei Zhang, 2012-02-14 02:14:24]

One more round of nit-picking. :)

http://codereview.chromium.org/9363008/diff/13001/base/system_monitor/system_...
File base/system_monitor/system_monitor.cc (right):

http://codereview.chromium.org/9363008/diff/13001/base/system_monitor/system_...
base/system_monitor/system_monitor.cc:9: #include "base/string_util.h"
not needed?

http://codereview.chromium.org/9363008/diff/13001/base/system_monitor/system_...
File base/system_monitor/system_monitor.h (right):

http://codereview.chromium.org/9363008/diff/13001/base/system_monitor/system_...
base/system_monitor/system_monitor.h:11: #include "base/file_path.h"
you can forward declare "class FilePath" and inclide this in system_monitor.cc.

http://codereview.chromium.org/9363008/diff/13001/base/system_monitor/system_...
base/system_monitor/system_monitor.h:99: // This is only implemented on Windows
currently.
Really? Is content::GamepadProvider() Windows-only?

[willchan no longer on Chromium, 2012-02-14 06:43:20]

I'm only going to review the interface and make sure the implementation isn't
doing something obviously silly that shouldn't happen in base/. But someone else
who is qualified to review the Mac implementation should do that. I assume csilv
is that person, right?

http://codereview.chromium.org/9363008/diff/13001/base/system_monitor/system_...
File base/system_monitor/system_monitor.h (right):

http://codereview.chromium.org/9363008/diff/13001/base/system_monitor/system_...
base/system_monitor/system_monitor.h:163: const std::string& name, const
FilePath& path);
Move to the next line as per chromium style guide
(http://www.chromium.org/developers/coding-style#TOC-Code-formatting) which
says:
For function declarations and definitions, put each argument on a separate line
unless everything fits on one line. You should only break this rule when the
arguments have some logical association like "int x, int y" or "char* str, int
str_len" where putting a few arguments together makes semantic sense.  Do NOT
collapse function arguments together just because you have "too many"

[vandebo (ex-Chrome), 2012-02-14 21:23:23]

On 2012/02/14 06:43:20, willchan wrote:
> I'm only going to review the interface and make sure the implementation isn't
> doing something obviously silly that shouldn't happen in base/. But someone
else
> who is qualified to review the Mac implementation should do that. I assume
csilv
> is that person, right?

Yes, I added csilv as a mac reviewer.

http://codereview.chromium.org/9363008/diff/13001/base/system_monitor/system_...
File base/system_monitor/system_monitor.cc (right):

http://codereview.chromium.org/9363008/diff/13001/base/system_monitor/system_...
base/system_monitor/system_monitor.cc:9: #include "base/string_util.h"
On 2012/02/14 02:14:24, Lei Zhang wrote:
> not needed?

Done.

http://codereview.chromium.org/9363008/diff/13001/base/system_monitor/system_...
base/system_monitor/system_monitor.cc:14: static SystemMonitor* g_system_monitor
= NULL;
On 2012/02/14 01:07:22, Lei Zhang wrote:
> Should this be a LazyInstance?

SystemMonitor is setup in BrowserMainLoop, so there aren't any thread safety
issues.

http://codereview.chromium.org/9363008/diff/13001/base/system_monitor/system_...
base/system_monitor/system_monitor.cc:150: void SystemMonitor::BatteryCheck() {
On 2012/02/14 01:58:01, Lei Zhang wrote:
> Also, this can be wrapped in #if defined(ENABLE_BATTERY_MONITORING)

It could be, but I'm not sure there's any gain to be had.

http://codereview.chromium.org/9363008/diff/13001/base/system_monitor/system_...
File base/system_monitor/system_monitor.h (right):

http://codereview.chromium.org/9363008/diff/13001/base/system_monitor/system_...
base/system_monitor/system_monitor.h:11: #include "base/file_path.h"
On 2012/02/14 02:14:24, Lei Zhang wrote:
> you can forward declare "class FilePath" and inclide this in
system_monitor.cc.

I tried that, but the observer code needs the full definition.

http://codereview.chromium.org/9363008/diff/13001/base/system_monitor/system_...
base/system_monitor/system_monitor.h:99: // This is only implemented on Windows
currently.
On 2012/02/14 02:14:24, Lei Zhang wrote:
> Really? Is content::GamepadProvider() Windows-only?

The only caller of ProcessDevicesChanged is in the windows code.  We may
register for the callback on all platforms, but it only happens on Windows.

http://codereview.chromium.org/9363008/diff/13001/base/system_monitor/system_...
base/system_monitor/system_monitor.h:163: const std::string& name, const
FilePath& path);
On 2012/02/14 06:43:20, willchan wrote:
> Move to the next line as per chromium style guide
> (http://www.chromium.org/developers/coding-style#TOC-Code-formatting) which
> says:
> For function declarations and definitions, put each argument on a separate
line
> unless everything fits on one line. You should only break this rule when the
> arguments have some logical association like "int x, int y" or "char* str, int
> str_len" where putting a few arguments together makes semantic sense.  Do NOT
> collapse function arguments together just because you have "too many"

Done.

http://codereview.chromium.org/9363008/diff/13001/base/system_monitor/system_...
File base/system_monitor/system_monitor_mac.mm (right):

http://codereview.chromium.org/9363008/diff/13001/base/system_monitor/system_...
base/system_monitor/system_monitor_mac.mm:10: #import <Carbon/Carbon.h>
On 2012/02/13 05:49:28, csilv wrote:
> use #include instead, #import only for pure obj-c class headers.

Done.

[tpayne, 2012-02-15 00:56:30]

http://codereview.chromium.org/9363008/diff/24002/base/system_monitor/system_...
File base/system_monitor/system_monitor_unittest.cc (right):

http://codereview.chromium.org/9363008/diff/24002/base/system_monitor/system_...
base/system_monitor/system_monitor_unittest.cc:93: class DevicesChangedTest :
public SystemMonitor::DevicesChangedObserver {
As mentioned on chat, this would be better using gmock. I'd prefer to see it in
someplace like base/testing so it could be reused in other tests.

[vandebo (ex-Chrome), 2012-02-15 22:28:25]

https://chromiumcodereview.appspot.com/9363008/diff/24002/base/system_monitor...
File base/system_monitor/system_monitor_unittest.cc (right):

https://chromiumcodereview.appspot.com/9363008/diff/24002/base/system_monitor...
base/system_monitor/system_monitor_unittest.cc:93: class DevicesChangedTest :
public SystemMonitor::DevicesChangedObserver {
On 2012/02/15 00:56:30, tpayne wrote:
> As mentioned on chat, this would be better using gmock. I'd prefer to see it
in
> someplace like base/testing so it could be reused in other tests.

Done.

[Lei Zhang, 2012-02-15 22:55:45]

LGTM with comments below:

Please forward declare FilePath if possible, per in-person discussion.

https://chromiumcodereview.appspot.com/9363008/diff/13001/base/system_monitor...
File base/system_monitor/system_monitor.cc (right):

https://chromiumcodereview.appspot.com/9363008/diff/13001/base/system_monitor...
base/system_monitor/system_monitor.cc:150: void SystemMonitor::BatteryCheck() {
On 2012/02/14 21:23:23, vandebo wrote:
> On 2012/02/14 01:58:01, Lei Zhang wrote:
> > Also, this can be wrapped in #if defined(ENABLE_BATTERY_MONITORING)
> 
> It could be, but I'm not sure there's any gain to be had.

It's just dead code on non-Windows platforms. No point in having it.

[vandebo (ex-Chrome), 2012-02-15 23:13:36]

will, LG?

https://chromiumcodereview.appspot.com/9363008/diff/13001/base/system_monitor...
File base/system_monitor/system_monitor.cc (right):

https://chromiumcodereview.appspot.com/9363008/diff/13001/base/system_monitor...
base/system_monitor/system_monitor.cc:150: void SystemMonitor::BatteryCheck() {
On 2012/02/15 22:55:45, Lei Zhang wrote:
> On 2012/02/14 21:23:23, vandebo wrote:
> > On 2012/02/14 01:58:01, Lei Zhang wrote:
> > > Also, this can be wrapped in #if defined(ENABLE_BATTERY_MONITORING)
> > 
> > It could be, but I'm not sure there's any gain to be had.
> 
> It's just dead code on non-Windows platforms. No point in having it.

Fine. Done.

https://chromiumcodereview.appspot.com/9363008/diff/13001/base/system_monitor...
File base/system_monitor/system_monitor.h (right):

https://chromiumcodereview.appspot.com/9363008/diff/13001/base/system_monitor...
base/system_monitor/system_monitor.h:11: #include "base/file_path.h"
On 2012/02/14 21:23:23, vandebo wrote:
> On 2012/02/14 02:14:24, Lei Zhang wrote:
> > you can forward declare "class FilePath" and inclide this in
> system_monitor.cc.
> 
> I tried that, but the observer code needs the full definition.

I think I was trying to declare it in the base namespace.  It works now.

[Lei Zhang, 2012-02-15 23:43:36]

http://codereview.chromium.org/9363008/diff/33005/base/system_monitor/system_...
File base/system_monitor/system_monitor.cc (right):

http://codereview.chromium.org/9363008/diff/33005/base/system_monitor/system_...
base/system_monitor/system_monitor.cc:151: #if
defined(ENABLE_BATTERY_MONITORING)
I meant putting the #if around the entire method. Also in the header files.

[willchan no longer on Chromium, 2012-02-15 23:52:48]

lgtm

[tpayne, 2012-02-17 20:41:11]

http://codereview.chromium.org/9363008/diff/31006/base/base.gyp
File base/base.gyp (right):

http://codereview.chromium.org/9363008/diff/31006/base/base.gyp#newcode224
base/base.gyp:224: 'system_monitor/mock_devices_changed_observer.cc',
These should be added to test_support_base. Personally, I think they'd be better
off in base/test/ than in a production code directory.

[vandebo (ex-Chrome), 2012-02-25 00:50:13]

avi, Most of the new mac code is moved into content/browser/  PTAL

http://codereview.chromium.org/9363008/diff/31006/base/base.gyp
File base/base.gyp (right):

http://codereview.chromium.org/9363008/diff/31006/base/base.gyp#newcode224
base/base.gyp:224: 'system_monitor/mock_devices_changed_observer.cc',
On 2012/02/17 20:41:11, tpayne wrote:
> These should be added to test_support_base. Personally, I think they'd be
better
> off in base/test/ than in a production code directory.

Done.

[Avi (use Gerrit), 2012-02-25 22:13:00]

I'm not entirely convinced content is the place where this belongs; including
John.

For my edification during review, can you post a sample dictionary returned by
ICACopyObjectPropertyDictionary? Right now I only have my iPhone handy as an
image capture device and that's not mass-storage so I'm seeing a different set
of keys than you are.

http://codereview.chromium.org/9363008/diff/37001/base/system_monitor/system_...
File base/system_monitor/system_monitor_mac.mm (right):

http://codereview.chromium.org/9363008/diff/37001/base/system_monitor/system_...
base/system_monitor/system_monitor_mac.mm:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
Why is this file in the CL if it's not otherwise modified?

http://codereview.chromium.org/9363008/diff/37001/content/browser/mac/media_d...
File content/browser/mac/media_device_notifications.mm (right):

http://codereview.chromium.org/9363008/diff/37001/content/browser/mac/media_d...
content/browser/mac/media_device_notifications.mm:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
File-wide comments:

- I do not want to see CFRelease scattered all over the place. Use
scoped_cfobject.
- Why are you using CFStringCompare with kCFCompareEqualTo? Why not CFEqual?
- This is .mm, Objective-C++. When appropriate, save yourself pain, NSToCFCast
the objects, and use ObjC to make your life easier.
- Please use appropriate constants rather than hard-coding strings.

http://codereview.chromium.org/9363008/diff/37001/content/browser/mac/media_d...
content/browser/mac/media_device_notifications.mm:28: // For now, we only
support mass storage media devices.
That's a huge limitation; e.g. iPhones are an ICA device that isn't
mass-storage. That's probably worth a TODO.

http://codereview.chromium.org/9363008/diff/37001/content/browser/mac/media_d...
content/browser/mac/media_device_notifications.mm:30: device_properties,
CFSTR("volume"));
Is there a constant you can use?

http://codereview.chromium.org/9363008/diff/37001/content/browser/mac/media_d...
content/browser/mac/media_device_notifications.mm:35: *location =
FilePath("/Volumes/").Append(base::SysCFStringRefToUTF8(volume));
This scares me because this is almost correct and yet wrong. In the dictionary
that you get back, do you get any media identifiers? Use the Disk Arbitration
utils or something else to convert it to a mount path. Assuming that the mount
path is going to be /Volumes/volumename is a dangerous assumption.

http://codereview.chromium.org/9363008/diff/37001/content/browser/mac/media_d...
content/browser/mac/media_device_notifications.mm:38: device_properties,
CFSTR("ICAUserAssignedDeviceNameKey"));
kICAUserAssignedDeviceNameKey. Constants, please!

http://codereview.chromium.org/9363008/diff/37001/content/browser/mac/media_d...
content/browser/mac/media_device_notifications.mm:41: device_properties,
CFSTR("ifil"));
kICAPropertyImageFilename?

http://codereview.chromium.org/9363008/diff/37001/content/browser/mac/media_d...
content/browser/mac/media_device_notifications.mm:71:
CFNumberGetValue(device_number_object, kCFNumberIntType, &device_number);
Objective C will likely be nice here.

http://codereview.chromium.org/9363008/diff/37001/content/browser/mac/media_d...
content/browser/mac/media_device_notifications.mm:89: CFArrayRef
events_of_interest =
Does ICARegisterForEventNotificationPB own this? Does
ICARegisterForEventNotification take a ref? Are we going to be leaking this?

[vandebo (ex-Chrome), 2012-02-28 00:51:33]

On 2012/02/25 22:13:00, Avi wrote:
> I'm not entirely convinced content is the place where this belongs; including
> John.

I'm not sure this is the perfect place for it either.  The equivalent windows
code is in ontent/browser/system_message_window_win.*  This is effectively an
implementation for something in base that requires (as far as I can tell) access
to the window manager.


> For my edification during review, can you post a sample dictionary returned by
> ICACopyObjectPropertyDictionary? Right now I only have my iPhone handy as an
> image capture device and that's not mass-storage so I'm seeing a different set
> of keys than you are.

Sure...
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN"
"http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
  <key>DADeviceModel</key>
  <string>USB  SD Reader</string>
  <key>ICAContentCatalogPercentCompletedKey</key>
  <integer>100</integer>
  <key>ICADeviceTypeKey</key>
  <string>camera</string>
  <key>ICAExecutableArchitectureKey</key>
  <integer>16777223</integer>
  <key>ICANumberOfDownloadableObjectsKey</key>
  <integer>602</integer>
  <key>ICAObjectInTemporaryStoreKey</key>
  <string>NO</string>
  <key>ICAUserAssignedDeviceNameKey</key>
  <string>TPAYNE A77</string>
  <key>UUIDString</key>
  <string>00000000-0000-0000-0064-69736B327331</string>
  <key>autolaunchApplicationPath</key>
  <string></string>
  <key>bsdName</key>

  <string>disk2s1</string>
  <key>capa</key>
  <array>
    <integer>1684368433</integer>
    <integer>1819238756</integer>
    <integer>1701471587</integer>
  </array>
  <key>clients</key>
  <array>
   
<string>/Volumes/Data/home/vandebo/chrome/src/out/Debug/Chromium.app/Contents/Versions/18.0.1026.0/Chromium
Helper.app</string>
   
<string>/Volumes/Data/home/vandebo/chrome/src/out/Debug/Chromium.app</string>
  </array>
  <key>data</key>
  <array>
    <dict>
      <key>9003</key>
      <string>2012:02:02 08:50:22</string>
      <key>9004</key>
      <string>2012:02:02 08:50:22</string>
      <key>ICAObjectInTemporaryStoreKey</key>
      <string>NO</string>
      <key>PTPObjH</key>
      <integer>0</integer>
      <key>datP</key>
      <integer>16777393</integer>
      <key>file</key>
      <integer>1768776039</integer>
      <key>icao</key>
      <integer>1073741881</integer>
      <key>ifil</key>
      <string>DSC09239.ARW</string>
      <key>isiz</key>
      <integer>24739840</integer>
      <key>lock</key>
      <integer>0</integer>
      <key>raw</key>
      <integer>1</integer>
      <key>thuP</key>
      <integer>16777389</integer>
      <key>tsiz</key>
      <integer>1</integer>
    </dict>
...
  </array>
  <key>tsiz</key>
  <integer>9228</integer>
  <key>volume</key>
  <string>TPAYNE A77</string>
</dict>
</plist>

Today I don't have a device to test the changes with, but it looks right.  I'm
sure there will be other style comments to address anyway, so sending out now.

http://codereview.chromium.org/9363008/diff/37001/base/system_monitor/system_...
File base/system_monitor/system_monitor_mac.mm (right):

http://codereview.chromium.org/9363008/diff/37001/base/system_monitor/system_...
base/system_monitor/system_monitor_mac.mm:1: // Copyright (c) 2012 The Chromium
Authors. All rights reserved.
On 2012/02/25 22:13:00, Avi wrote:
> Why is this file in the CL if it's not otherwise modified?

Oops, it was modified in a previous patch set.  Fixed.

http://codereview.chromium.org/9363008/diff/37001/content/browser/mac/media_d...
File content/browser/mac/media_device_notifications.mm (right):

http://codereview.chromium.org/9363008/diff/37001/content/browser/mac/media_d...
content/browser/mac/media_device_notifications.mm:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2012/02/25 22:13:00, Avi wrote:
> File-wide comments:
> 
> - I do not want to see CFRelease scattered all over the place. Use
> scoped_cfobject.

ScopedCFTypeRef?  Done.

> - Why are you using CFStringCompare with kCFCompareEqualTo? Why not CFEqual?

I don't know any better - not a Mac programmer.  Please educate me where
appropriate.

> - This is .mm, Objective-C++. When appropriate, save yourself pain, NSToCFCast
> the objects, and use ObjC to make your life easier.
> - Please use appropriate constants rather than hard-coding strings.

http://codereview.chromium.org/9363008/diff/37001/content/browser/mac/media_d...
content/browser/mac/media_device_notifications.mm:28: // For now, we only
support mass storage media devices.
On 2012/02/25 22:13:00, Avi wrote:
> That's a huge limitation; e.g. iPhones are an ICA device that isn't
> mass-storage. That's probably worth a TODO.

Changed to a todo.  Supporting all of OSX's media devices is on the roadmap. 
Supporting just mass storage was a simple thing to do to unblock another effort.

http://codereview.chromium.org/9363008/diff/37001/content/browser/mac/media_d...
content/browser/mac/media_device_notifications.mm:30: device_properties,
CFSTR("volume"));
On 2012/02/25 22:13:00, Avi wrote:
> Is there a constant you can use?

Not that I'm aware of.  "volume" does not appear in
/Volumes/Leopard/Developer/SDKs/MacOSX10.5.sdk/System/Library/Frameworks/Carbon.framework/Versions/A/Frameworks/ImageCapture.framework/*

http://codereview.chromium.org/9363008/diff/37001/content/browser/mac/media_d...
content/browser/mac/media_device_notifications.mm:35: *location =
FilePath("/Volumes/").Append(base::SysCFStringRefToUTF8(volume));
On 2012/02/25 22:13:00, Avi wrote:
> This scares me because this is almost correct and yet wrong. In the dictionary
> that you get back, do you get any media identifiers? Use the Disk Arbitration
> utils or something else to convert it to a mount path. Assuming that the mount
> path is going to be /Volumes/volumename is a dangerous assumption.

Done.

http://codereview.chromium.org/9363008/diff/37001/content/browser/mac/media_d...
content/browser/mac/media_device_notifications.mm:38: device_properties,
CFSTR("ICAUserAssignedDeviceNameKey"));
On 2012/02/25 22:13:00, Avi wrote:
> kICAUserAssignedDeviceNameKey. Constants, please!

Done. (Added my own, there doesn't seem to be one in the system header)

http://codereview.chromium.org/9363008/diff/37001/content/browser/mac/media_d...
content/browser/mac/media_device_notifications.mm:41: device_properties,
CFSTR("ifil"));
On 2012/02/25 22:13:00, Avi wrote:
> kICAPropertyImageFilename? 

It's the wrong type.  Changed to a constant though.

http://codereview.chromium.org/9363008/diff/37001/content/browser/mac/media_d...
content/browser/mac/media_device_notifications.mm:71:
CFNumberGetValue(device_number_object, kCFNumberIntType, &device_number);
On 2012/02/25 22:13:00, Avi wrote:
> Objective C will likely be nice here.

Done.

http://codereview.chromium.org/9363008/diff/37001/content/browser/mac/media_d...
content/browser/mac/media_device_notifications.mm:89: CFArrayRef
events_of_interest =
On 2012/02/25 22:13:00, Avi wrote:
> Does ICARegisterForEventNotificationPB own this? Does
> ICARegisterForEventNotification take a ref? Are we going to be leaking this?

This sample app implies that ICARegisterForEventNotification takes ownership:
https://developer.apple.com/library/mac/#samplecode/SampleScannerApp/Listings...

http://codereview.chromium.org/9363008/diff/44001/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/9363008/diff/44001/base/mac/foundation_util.h#...
base/mac/foundation_util.h:102: TYPE_NAME_FOR_CF_TYPE_DECL(CFURL);
I'm not sure If this is the right thing, but was needed for it to compile.

[Avi (use Gerrit), 2012-02-28 01:26:35]

On 2012/02/28 00:51:33, vandebo wrote:
> I'm not sure this is the perfect place for it either.  The equivalent windows
> code is in ontent/browser/system_message_window_win.*

Eeew. Same weird feelings about that class.

[Avi (use Gerrit), 2012-02-28 01:26:45]

http://codereview.chromium.org/9363008/diff/37001/content/browser/mac/media_d...
File content/browser/mac/media_device_notifications.mm (right):

http://codereview.chromium.org/9363008/diff/37001/content/browser/mac/media_d...
content/browser/mac/media_device_notifications.mm:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2012/02/28 00:51:33, vandebo wrote:
> > - Why are you using CFStringCompare with kCFCompareEqualTo? Why not CFEqual?
> 
> I don't know any better - not a Mac programmer.  Please educate me where
> appropriate.

No problem. If all you're trying to do is do equality comparisons, CFEqual is
simpler and shorter.

http://codereview.chromium.org/9363008/diff/37001/content/browser/mac/media_d...
content/browser/mac/media_device_notifications.mm:89: CFArrayRef
events_of_interest =
On 2012/02/28 00:51:33, vandebo wrote:
> This sample app implies that ICARegisterForEventNotification takes ownership

You got ICA sample code to compile?! I tried two different ones and neither
compiled.

But the sample code you bring proves the opposite. -[NSArray arrayWithObjects:]
returns an autoreleased array, so if the Apple sample code works, it shows that
ICARegisterForEventNotification retains the array itself and does not take over
the ownership.

http://codereview.chromium.org/9363008/diff/44001/base/mac/foundation_util.h
File base/mac/foundation_util.h (right):

http://codereview.chromium.org/9363008/diff/44001/base/mac/foundation_util.h#...
base/mac/foundation_util.h:102: TYPE_NAME_FOR_CF_TYPE_DECL(CFURL);
On 2012/02/28 00:51:33, vandebo wrote:
> I'm not sure If this is the right thing, but was needed for it to compile.

Yes, this is fine.

http://codereview.chromium.org/9363008/diff/44001/content/browser/mac/media_d...
File content/browser/mac/media_device_notifications.mm (right):

http://codereview.chromium.org/9363008/diff/44001/content/browser/mac/media_d...
content/browser/mac/media_device_notifications.mm:21:
base::mac::NSToCFCast(@"ICAUserAssignedDeviceNameKey");
CFSTR() here and the next two lines.

http://codereview.chromium.org/9363008/diff/44001/content/browser/mac/media_d...
content/browser/mac/media_device_notifications.mm:25: CFStringRef
CopyMountPointFromBSDName(const char* bsd_name) {
It's weird that this doesn't take a CF argument but returns one. Have it do the
base::SysCFStringRefToUTF8(device).c_str() conversion inside here and spare the
caller.

And yes, this is _exactly_ what I was looking for; thanks!

http://codereview.chromium.org/9363008/diff/44001/content/browser/mac/media_d...
content/browser/mac/media_device_notifications.mm:46: return
CFURLCopyFileSystemPath(mount_point, kCFURLComponentPath);
You mean kCFURLPOSIXPathStyle. (kCFURLComponentPath is a CFURLComponentType, not
a CFURLPathStyle.)

http://codereview.chromium.org/9363008/diff/44001/content/browser/mac/media_d...
content/browser/mac/media_device_notifications.mm:58: CHECK_EQ(ret, noErr);
Everywhere else, if a null is returned you return false from this function. Why
does this one deserve the severity of CHECK? (Also, move to the line after the
call to ICACopyObjectPropertyDictionary.)

http://codereview.chromium.org/9363008/diff/44001/content/browser/mac/media_d...
content/browser/mac/media_device_notifications.mm:92: return;
A little confused; why the return here? If you don't want to deal with this
notification type, why register for it?

http://codereview.chromium.org/9363008/diff/44001/content/browser/mac/media_d...
content/browser/mac/media_device_notifications.mm:120: NULL];
nil (not NULL) is used when referring to an ObjC object.

[jam, 2012-02-28 01:48:07]

On 2012/02/28 00:51:33, vandebo wrote:
> On 2012/02/25 22:13:00, Avi wrote:
> > I'm not entirely convinced content is the place where this belongs;
including
> > John.
> 
> I'm not sure this is the perfect place for it either.  The equivalent windows
> code is in ontent/browser/system_message_window_win.*  This is effectively an
> implementation for something in base that requires (as far as I can tell)
access
> to the window manager.

If this is webintent related, then it should be in content (as opposed to
chrome) because web platform features belong there.

[Lei Zhang, 2012-02-28 02:14:13]

On 2012/02/28 01:48:07, John Abd-El-Malek wrote:
> On 2012/02/28 00:51:33, vandebo wrote:
> > On 2012/02/25 22:13:00, Avi wrote:
> > > I'm not entirely convinced content is the place where this belongs;
> including
> > > John.
> > 
> > I'm not sure this is the perfect place for it either.  The equivalent
windows
> > code is in ontent/browser/system_message_window_win.*  This is effectively
an
> > implementation for something in base that requires (as far as I can tell)
> access
> > to the window manager.
> 
> If this is webintent related, then it should be in content (as opposed to
> chrome) because web platform features belong there.

content/browser/system_message_window_win.* is currently used in conjunction
with content/browser/gamepad/*win*.

[vandebo (ex-Chrome), 2012-02-28 18:31:25]

On 2012/02/28 01:26:35, Avi wrote:
> On 2012/02/28 00:51:33, vandebo wrote:
> > I'm not sure this is the perfect place for it either.  The equivalent
windows
> > code is in ontent/browser/system_message_window_win.*
> 
> Eeew. Same weird feelings about that class.

We could move that code and this new code to content/browser/system_monitor/ ?

[vandebo (ex-Chrome), 2012-02-28 18:31:59]

On 2012/02/28 01:48:07, John Abd-El-Malek wrote:
> On 2012/02/28 00:51:33, vandebo wrote:
> > On 2012/02/25 22:13:00, Avi wrote:
> > > I'm not entirely convinced content is the place where this belongs;
> including
> > > John.
> > 
> > I'm not sure this is the perfect place for it either.  The equivalent
windows
> > code is in ontent/browser/system_message_window_win.*  This is effectively
an
> > implementation for something in base that requires (as far as I can tell)
> access
> > to the window manager.
> 
> If this is webintent related, then it should be in content (as opposed to
> chrome) because web platform features belong there.

webintents will use this notification, yes.

[vandebo (ex-Chrome), 2012-02-28 18:35:04]

Thanks for the comments and your patience.

https://chromiumcodereview.appspot.com/9363008/diff/37001/content/browser/mac...
File content/browser/mac/media_device_notifications.mm (right):

https://chromiumcodereview.appspot.com/9363008/diff/37001/content/browser/mac...
content/browser/mac/media_device_notifications.mm:89: CFArrayRef
events_of_interest =
On 2012/02/28 01:26:46, Avi wrote:
> On 2012/02/28 00:51:33, vandebo wrote:
> > This sample app implies that ICARegisterForEventNotification takes ownership
> 
> You got ICA sample code to compile?! I tried two different ones and neither
> compiled.
> 
> But the sample code you bring proves the opposite. -[NSArray
arrayWithObjects:]
> returns an autoreleased array, so if the Apple sample code works, it shows
that
> ICARegisterForEventNotification retains the array itself and does not take
over
> the ownership.

I didn't try compiling the sample code, just used it as a reference.  I guess
it's good to know that they don't necessarily work/correctly.  As you probably
saw, I switched to the array construction style in the sample app (because it
was shorter), so ownership should be ok now.

https://chromiumcodereview.appspot.com/9363008/diff/44001/content/browser/mac...
File content/browser/mac/media_device_notifications.mm (right):

https://chromiumcodereview.appspot.com/9363008/diff/44001/content/browser/mac...
content/browser/mac/media_device_notifications.mm:21:
base::mac::NSToCFCast(@"ICAUserAssignedDeviceNameKey");
On 2012/02/28 01:26:46, Avi wrote:
> CFSTR() here and the next two lines.

Done.

https://chromiumcodereview.appspot.com/9363008/diff/44001/content/browser/mac...
content/browser/mac/media_device_notifications.mm:25: CFStringRef
CopyMountPointFromBSDName(const char* bsd_name) {
On 2012/02/28 01:26:46, Avi wrote:
> It's weird that this doesn't take a CF argument but returns one. Have it do
the
> base::SysCFStringRefToUTF8(device).c_str() conversion inside here and spare
the
> caller.
> 
> And yes, this is _exactly_ what I was looking for; thanks!

Done.

https://chromiumcodereview.appspot.com/9363008/diff/44001/content/browser/mac...
content/browser/mac/media_device_notifications.mm:46: return
CFURLCopyFileSystemPath(mount_point, kCFURLComponentPath);
On 2012/02/28 01:26:46, Avi wrote:
> You mean kCFURLPOSIXPathStyle. (kCFURLComponentPath is a CFURLComponentType,
not
> a CFURLPathStyle.)

Done.

https://chromiumcodereview.appspot.com/9363008/diff/44001/content/browser/mac...
content/browser/mac/media_device_notifications.mm:58: CHECK_EQ(ret, noErr);
On 2012/02/28 01:26:46, Avi wrote:
> Everywhere else, if a null is returned you return false from this function.
Why
> does this one deserve the severity of CHECK? (Also, move to the line after the
> call to ICACopyObjectPropertyDictionary.)

Done.

https://chromiumcodereview.appspot.com/9363008/diff/44001/content/browser/mac...
content/browser/mac/media_device_notifications.mm:92: return;
On 2012/02/28 01:26:46, Avi wrote:
> A little confused; why the return here? If you don't want to deal with this
> notification type, why register for it?

A conversion bug, from CFStringCompare.  I would have caught it anyway when I
had a chance to test today.

https://chromiumcodereview.appspot.com/9363008/diff/44001/content/browser/mac...
content/browser/mac/media_device_notifications.mm:120: NULL];
On 2012/02/28 01:26:46, Avi wrote:
> nil (not NULL) is used when referring to an ObjC object.

Done.

[Avi (use Gerrit), 2012-02-28 19:19:59]

lgtm

https://chromiumcodereview.appspot.com/9363008/diff/49001/content/browser/mac...
File content/browser/mac/media_device_notifications.mm (right):

https://chromiumcodereview.appspot.com/9363008/diff/49001/content/browser/mac...
content/browser/mac/media_device_notifications.mm:78:
path(CopyMountPointFromBSDName(device));
Can this fit on one line?

[vandebo (ex-Chrome), 2012-02-28 19:24:45]

Thanks

https://chromiumcodereview.appspot.com/9363008/diff/49001/content/browser/mac...
File content/browser/mac/media_device_notifications.mm (right):

https://chromiumcodereview.appspot.com/9363008/diff/49001/content/browser/mac...
content/browser/mac/media_device_notifications.mm:78:
path(CopyMountPointFromBSDName(device));
On 2012/02/28 19:19:59, Avi wrote:
> Can this fit on one line?

Nope, two characters too long.

[tpayne, 2012-02-28 19:31:42]

https://chromiumcodereview.appspot.com/9363008/diff/49001/base/system_monitor...
File base/system_monitor/system_monitor.h (right):

https://chromiumcodereview.appspot.com/9363008/diff/49001/base/system_monitor...
base/system_monitor/system_monitor.h:104: // if triggered.
typo (is)

[vandebo (ex-Chrome), 2012-02-28 20:36:10]

https://chromiumcodereview.appspot.com/9363008/diff/49001/base/system_monitor...
File base/system_monitor/system_monitor.h (right):

https://chromiumcodereview.appspot.com/9363008/diff/49001/base/system_monitor...
base/system_monitor/system_monitor.h:104: // if triggered.
On 2012/02/28 19:31:42, tpayne wrote:
> typo (is)

Done.

[commit-bot: I haz the power, 2012-02-28 21:18:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/9363008/57002

[commit-bot: I haz the power, 2012-02-28 22:29:59]

Try job failure for 9363008-57002 (retry) on linux_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2012-02-28 22:31:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/9363008/57002

[commit-bot: I haz the power, 2012-02-29 00:25:17]

Try job failure for 9363008-57002 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-02-29 00:27:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/9363008/57002

[commit-bot: I haz the power, 2012-02-29 05:20:13]

Try job failure for 9363008-57002 (retry) (retry) on mac_rel for step
"browser_tests".
It's a second try, previously, steps "safe_browsing_tests, browser_tests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[Lei Zhang, 2012-02-29 06:17:13]

On 2012/02/29 05:20:13, I haz the power (commit-bot) wrote:
> Try job failure for 9363008-57002 (retry) (retry) on mac_rel for step
> "browser_tests".
> It's a second try, previously, steps "safe_browsing_tests, browser_tests"
> failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

Doh. Well, there's your chance to fix the legit lint errors from the base/
files.

[vandebo (ex-Chrome), 2012-02-29 19:56:09]

On 2012/02/29 06:17:13, Lei Zhang wrote:
> Doh. Well, there's your chance to fix the legit lint errors from the base/
> files.

Done.  CQ gets one more try - then it's a dcommit.

[commit-bot: I haz the power, 2012-02-29 19:56:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/9363008/61003

[commit-bot: I haz the power, 2012-02-29 20:27:50]

Try job failure for 9363008-61003 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-02-29 20:33:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vandebo@chromium.org/9363008/65002

[commit-bot: I haz the power, 2012-02-29 22:33:43]

Try job failure for 9363008-65002 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[jam, 2012-03-01 02:03:30]

btw those are the only two files in content/browser/mac now. do we still want to
keep that directory or should we move these to content/browser?

[vandebo (ex-Chrome), 2012-03-02 18:29:05]

On 2012/03/01 02:03:30, John Abd-El-Malek wrote:
> btw those are the only two files in content/browser/mac now. do we still want
to
> keep that directory or should we move these to content/browser?

FYI - I tried to put this code in content to get rid of an oserr -600.  The
change got reverted because it popped up anyway, so the mac code will probably
land it base once I figure out the source of the -600 errors.