Implement Linux media notifier.

Implement Linux media notifier.

BUG=110400
TEST=included.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=125361

[Lei Zhang, 2012-03-01 01:30:04]

Still figuring out this unit test business, but here's the CL which I think is
mostly good to go.

[Lei Zhang, 2012-03-01 08:21:11]

PTAL

[vandebo (ex-Chrome), 2012-03-02 23:47:35]

http://codereview.chromium.org/9560008/diff/6001/content/browser/media_device...
File content/browser/media_device_notifications_linux.cc (right):

http://codereview.chromium.org/9560008/diff/6001/content/browser/media_device...
content/browser/media_device_notifications_linux.cc:135: if
(ProcessInotifyEvent(event))
maybe add a DCHECK(bytes_processed + event_size <= bytes_read)

http://codereview.chromium.org/9560008/diff/6001/content/browser/media_device...
content/browser/media_device_notifications_linux.cc:176: std::set<MountPoint>
seen_mount_points;
This copies the mount points (strings), MountPoint* ?

http://codereview.chromium.org/9560008/diff/6001/content/browser/media_device...
content/browser/media_device_notifications_linux.cc:186: for
(MountDevices::const_iterator deviceiter = devices.begin();
Only one device at a mount point is accessible.  Effectively, when we mount over
a device, the old one is removed and the new one is attached...

I'm not sure we really need to handle these cases though...

http://codereview.chromium.org/9560008/diff/6001/content/browser/media_device...
content/browser/media_device_notifications_linux.cc:225: for (size_t i = 0; i <
arraysize(kKnownFileSystems); ++i) {
Maybe put known file systems in a set, and use set::find ?

http://codereview.chromium.org/9560008/diff/6001/content/browser/media_device...
content/browser/media_device_notifications_linux.cc:229: new_mtab[entry.mnt_dir]
= MountDevices();
Only one device is accessible at a time, do we need a set?

http://codereview.chromium.org/9560008/diff/6001/content/browser/media_device...
content/browser/media_device_notifications_linux.cc:259: void
MediaDeviceNotificationsLinux::AddNewMediaDevice(
nit: AddNewDevice, we don't know if it's a media device yet.

http://codereview.chromium.org/9560008/diff/6001/content/browser/media_device...
content/browser/media_device_notifications_linux.cc:263: // device. Mac OS X
behaves this way.
Actually Mac does something more magic (I haven't figure out what yet), just a
DCIM directory isn't enough.

[Lei Zhang, 2012-03-03 02:28:07]

Fixed the easy bits. Will address remaining comments in next patch set.

http://codereview.chromium.org/9560008/diff/6001/content/browser/media_device...
File content/browser/media_device_notifications_linux.cc (right):

http://codereview.chromium.org/9560008/diff/6001/content/browser/media_device...
content/browser/media_device_notifications_linux.cc:135: if
(ProcessInotifyEvent(event))
On 2012/03/02 23:47:35, vandebo wrote:
> maybe add a DCHECK(bytes_processed + event_size <= bytes_read)

Done.

http://codereview.chromium.org/9560008/diff/6001/content/browser/media_device...
content/browser/media_device_notifications_linux.cc:225: for (size_t i = 0; i <
arraysize(kKnownFileSystems); ++i) {
On 2012/03/02 23:47:35, vandebo wrote:
> Maybe put known file systems in a set, and use set::find ?

Done.

http://codereview.chromium.org/9560008/diff/6001/content/browser/media_device...
content/browser/media_device_notifications_linux.cc:259: void
MediaDeviceNotificationsLinux::AddNewMediaDevice(
On 2012/03/02 23:47:35, vandebo wrote:
> nit: AddNewDevice, we don't know if it's a media device yet.

Done.

http://codereview.chromium.org/9560008/diff/6001/content/browser/media_device...
content/browser/media_device_notifications_linux.cc:263: // device. Mac OS X
behaves this way.
On 2012/03/02 23:47:35, vandebo wrote:
> Actually Mac does something more magic (I haven't figure out what yet), just a
> DCIM directory isn't enough.

I added a TODO for you.

[Lei Zhang, 2012-03-05 23:14:32]

PTAL at patch set 4.

http://codereview.chromium.org/9560008/diff/6001/content/browser/media_device...
File content/browser/media_device_notifications_linux.cc (right):

http://codereview.chromium.org/9560008/diff/6001/content/browser/media_device...
content/browser/media_device_notifications_linux.cc:176: std::set<MountPoint>
seen_mount_points;
On 2012/03/02 23:47:35, vandebo wrote:
> This copies the mount points (strings), MountPoint* ?

This code has been removed.

http://codereview.chromium.org/9560008/diff/6001/content/browser/media_device...
content/browser/media_device_notifications_linux.cc:186: for
(MountDevices::const_iterator deviceiter = devices.begin();
On 2012/03/02 23:47:35, vandebo wrote:
> Only one device at a mount point is accessible.  Effectively, when we mount
over
> a device, the old one is removed and the new one is attached...
> 
> I'm not sure we really need to handle these cases though...

I've changed the code so the last device mounted at a given mount point is the
one that counts. A device that is mounted in multiple places is only considered
once, at its last mount point.

[jam, 2012-03-05 23:31:52]

https://chromiumcodereview.appspot.com/9560008/diff/23002/content/browser/med...
File content/browser/media_device_notifications_linux.h (right):

https://chromiumcodereview.appspot.com/9560008/diff/23002/content/browser/med...
content/browser/media_device_notifications_linux.h:28: public
base::RefCounted<MediaDeviceNotificationsLinux> {
i think you want RefCountedThreadSafe?

[Lei Zhang, 2012-03-05 23:44:35]

Also removed some Mac device notifier bits that sneaked in.

https://chromiumcodereview.appspot.com/9560008/diff/23002/content/browser/med...
File content/browser/media_device_notifications_linux.h (right):

https://chromiumcodereview.appspot.com/9560008/diff/23002/content/browser/med...
content/browser/media_device_notifications_linux.h:28: public
base::RefCounted<MediaDeviceNotificationsLinux> {
On 2012/03/05 23:31:54, John Abd-El-Malek wrote:
> i think you want RefCountedThreadSafe?

Done.

[vandebo (ex-Chrome), 2012-03-06 00:41:40]

http://codereview.chromium.org/9560008/diff/23002/content/browser/browser_mai...
File content/browser/browser_main_loop.cc (right):

http://codereview.chromium.org/9560008/diff/23002/content/browser/browser_mai...
content/browser/browser_main_loop.cc:605: const FilePath
kDefaultMtabPath("/etc/mtab");
Random thought - does this work on CrOS?  It should, but crazy stuff might be
going on.

http://codereview.chromium.org/9560008/diff/23002/content/browser/media_devic...
File content/browser/media_device_notifications_linux.cc (right):

http://codereview.chromium.org/9560008/diff/23002/content/browser/media_devic...
content/browser/media_device_notifications_linux.cc:74: inotify_fd_ =
inotify_init();
It'd be great if we could reuse FilePathWatcher

http://codereview.chromium.org/9560008/diff/23002/content/browser/media_devic...
content/browser/media_device_notifications_linux.cc:160: bool
MediaDeviceNotificationsLinux::ProcessInotifyEvent(inotify_event* event) {
nit: InotifyEventIsInteresting ?

http://codereview.chromium.org/9560008/diff/23002/content/browser/media_devic...
content/browser/media_device_notifications_linux.cc:205: mtab_ = new_mtab;
I think if you update mtab_ in place (i.e. add and remove entries from it as you
go along).  You can just discard new_mtab here, but you also wouldn't need
|device_id_map_| - you could just store the device id in the MountMap value.

http://codereview.chromium.org/9560008/diff/23002/content/browser/media_devic...
content/browser/media_device_notifications_linux.cc:224: for (MountMap::iterator
it = new_mtab.begin(); it != new_mtab.end(); ++it) {
This is n^2.  Should we just do a dev -> point mapping first and then copy it
into a point -> dev mapping?

http://codereview.chromium.org/9560008/diff/23002/content/browser/media_devic...
File content/browser/media_device_notifications_linux_unittest.cc (right):

http://codereview.chromium.org/9560008/diff/23002/content/browser/media_devic...
content/browser/media_device_notifications_linux_unittest.cc:54: class
DummyDeviceChangeObserver
You should use MockDevicesChangedObserver

http://codereview.chromium.org/9560008/diff/23002/content/browser/media_devic...
content/browser/media_device_notifications_linux_unittest.cc:106: mtab_file_ =
test_dir.AppendASCII("foo");
why not mtab ?

[Lei Zhang, 2012-03-06 19:00:24]

https://chromiumcodereview.appspot.com/9560008/diff/23002/content/browser/bro...
File content/browser/browser_main_loop.cc (right):

https://chromiumcodereview.appspot.com/9560008/diff/23002/content/browser/bro...
content/browser/browser_main_loop.cc:605: const FilePath
kDefaultMtabPath("/etc/mtab");
On 2012/03/06 00:41:40, vandebo wrote:
> Random thought - does this work on CrOS?  It should, but crazy stuff might be
> going on.

It should based on our observations on a Chromebook in dev mode.

https://chromiumcodereview.appspot.com/9560008/diff/23002/content/browser/med...
File content/browser/media_device_notifications_linux.cc (right):

https://chromiumcodereview.appspot.com/9560008/diff/23002/content/browser/med...
content/browser/media_device_notifications_linux.cc:74: inotify_fd_ =
inotify_init();
On 2012/03/06 00:41:40, vandebo wrote:
> It'd be great if we could reuse FilePathWatcher

Done.

https://chromiumcodereview.appspot.com/9560008/diff/23002/content/browser/med...
content/browser/media_device_notifications_linux.cc:160: bool
MediaDeviceNotificationsLinux::ProcessInotifyEvent(inotify_event* event) {
On 2012/03/06 00:41:40, vandebo wrote:
> nit: InotifyEventIsInteresting ?

Code got removed.

https://chromiumcodereview.appspot.com/9560008/diff/23002/content/browser/med...
content/browser/media_device_notifications_linux.cc:205: mtab_ = new_mtab;
On 2012/03/06 00:41:40, vandebo wrote:
> I think if you update mtab_ in place (i.e. add and remove entries from it as
you
> go along).  You can just discard new_mtab here, but you also wouldn't need
> |device_id_map_| - you could just store the device id in the MountMap value.

Done.

https://chromiumcodereview.appspot.com/9560008/diff/23002/content/browser/med...
content/browser/media_device_notifications_linux.cc:224: for (MountMap::iterator
it = new_mtab.begin(); it != new_mtab.end(); ++it) {
On 2012/03/06 00:41:40, vandebo wrote:
> This is n^2.  Should we just do a dev -> point mapping first and then copy it
> into a point -> dev mapping?

Sure, it's not that trivial though. We still need to keep track of the ordering
for the mount entries, otherwise during the copy, we won't know the ordering for
any given mount point.

https://chromiumcodereview.appspot.com/9560008/diff/23002/content/browser/med...
File content/browser/media_device_notifications_linux_unittest.cc (right):

https://chromiumcodereview.appspot.com/9560008/diff/23002/content/browser/med...
content/browser/media_device_notifications_linux_unittest.cc:54: class
DummyDeviceChangeObserver
On 2012/03/06 00:41:40, vandebo wrote:
> You should use MockDevicesChangedObserver

Done.

https://chromiumcodereview.appspot.com/9560008/diff/23002/content/browser/med...
content/browser/media_device_notifications_linux_unittest.cc:106: mtab_file_ =
test_dir.AppendASCII("foo");
On 2012/03/06 00:41:40, vandebo wrote:
> why not mtab ?

The name is arbitrary. I changed it to "test_mtab".

[vandebo (ex-Chrome), 2012-03-06 19:36:14]

LGTM

https://chromiumcodereview.appspot.com/9560008/diff/35003/content/browser/med...
File content/browser/media_device_notifications_linux.cc (right):

https://chromiumcodereview.appspot.com/9560008/diff/35003/content/browser/med...
content/browser/media_device_notifications_linux.cc:180: bool
MediaDeviceNotificationsLinux::IsNewDeviceAMediaDevice(
nit: IsMediaDevice?

https://chromiumcodereview.appspot.com/9560008/diff/35003/content/browser/med...
content/browser/media_device_notifications_linux.cc:192: *device_id =
current_device_id_++;
nit: maybe set device_id in AddNewDevice and return the new device id?

[Lei Zhang, 2012-03-06 20:23:16]

jam: OWNERS review please.

https://chromiumcodereview.appspot.com/9560008/diff/35003/content/browser/med...
File content/browser/media_device_notifications_linux.cc (right):

https://chromiumcodereview.appspot.com/9560008/diff/35003/content/browser/med...
content/browser/media_device_notifications_linux.cc:180: bool
MediaDeviceNotificationsLinux::IsNewDeviceAMediaDevice(
On 2012/03/06 19:36:14, vandebo wrote:
> nit: IsMediaDevice?

Done.

https://chromiumcodereview.appspot.com/9560008/diff/35003/content/browser/med...
content/browser/media_device_notifications_linux.cc:192: *device_id =
current_device_id_++;
On 2012/03/06 19:36:14, vandebo wrote:
> nit: maybe set device_id in AddNewDevice and return the new device id?

I had it like that before, but having an explicit IsMediaDevice() method seems
more readable.

[jam, 2012-03-06 21:21:37]

http://codereview.chromium.org/9560008/diff/33011/content/browser/browser_mai...
File content/browser/browser_main_loop.cc (right):

http://codereview.chromium.org/9560008/diff/33011/content/browser/browser_mai...
content/browser/browser_main_loop.cc:7: #include <string>
nit: no need. i understand you're adding it for IWYU, but this is such a base
include file

http://codereview.chromium.org/9560008/diff/33011/content/browser/browser_mai...
content/browser/browser_main_loop.cc:603: BrowserThread::FILE, FROM_HERE,
nit: it's nicer if this class can do its thread initialization itself, to keep
BrowserMainLoop small and not have to worry about implementation details. this
class can just call an Init() method that internally does this

[Lei Zhang, 2012-03-06 21:36:50]

https://chromiumcodereview.appspot.com/9560008/diff/33011/content/browser/bro...
File content/browser/browser_main_loop.cc (right):

https://chromiumcodereview.appspot.com/9560008/diff/33011/content/browser/bro...
content/browser/browser_main_loop.cc:7: #include <string>
On 2012/03/06 21:21:38, John Abd-El-Malek wrote:
> nit: no need. i understand you're adding it for IWYU, but this is such a base
> include file

Done.

https://chromiumcodereview.appspot.com/9560008/diff/33011/content/browser/bro...
content/browser/browser_main_loop.cc:603: BrowserThread::FILE, FROM_HERE,
On 2012/03/06 21:21:38, John Abd-El-Malek wrote:
> nit: it's nicer if this class can do its thread initialization itself, to keep
> BrowserMainLoop small and not have to worry about implementation details. this
> class can just call an Init() method that internally does this

Done.

[jam, 2012-03-06 22:22:11]

https://chromiumcodereview.appspot.com/9560008/diff/33011/content/browser/bro...
File content/browser/browser_main_loop.cc (right):

https://chromiumcodereview.appspot.com/9560008/diff/33011/content/browser/bro...
content/browser/browser_main_loop.cc:603: BrowserThread::FILE, FROM_HERE,
On 2012/03/06 21:36:50, Lei Zhang wrote:
> On 2012/03/06 21:21:38, John Abd-El-Malek wrote:
> > nit: it's nicer if this class can do its thread initialization itself, to
keep
> > BrowserMainLoop small and not have to worry about implementation details.
this
> > class can just call an Init() method that internally does this
> 
> Done.

you have do it in in a separate method, i.e. after the calling code has a
reference. otherwise if the task runs on the file thread before the constructor
returns, the object will be deleted.

if you try this in debug builds, it'll crash right away because the PostTask
will do release/addref first

[Lei Zhang, 2012-03-06 23:23:42]

https://chromiumcodereview.appspot.com/9560008/diff/33011/content/browser/bro...
File content/browser/browser_main_loop.cc (right):

https://chromiumcodereview.appspot.com/9560008/diff/33011/content/browser/bro...
content/browser/browser_main_loop.cc:603: BrowserThread::FILE, FROM_HERE,
On 2012/03/06 22:22:11, John Abd-El-Malek wrote:
> On 2012/03/06 21:36:50, Lei Zhang wrote:
> > On 2012/03/06 21:21:38, John Abd-El-Malek wrote:
> > > nit: it's nicer if this class can do its thread initialization itself, to
> keep
> > > BrowserMainLoop small and not have to worry about implementation details.
> this
> > > class can just call an Init() method that internally does this
> > 
> > Done.
> 
> you have do it in in a separate method, i.e. after the calling code has a
> reference. otherwise if the task runs on the file thread before the
constructor
> returns, the object will be deleted.
> 
> if you try this in debug builds, it'll crash right away because the PostTask
> will do release/addref first

Oh thanks for reminding me about that. Fixed.

https://chromiumcodereview.appspot.com/9560008/diff/37003/content/browser/med...
File content/browser/media_device_notifications_linux.cc (right):

https://chromiumcodereview.appspot.com/9560008/diff/37003/content/browser/med...
content/browser/media_device_notifications_linux.cc:124: if (mount_point ==
old_mount_device_and_id.first)
Typo, whoops. Horray for unit tests.

[jam, 2012-03-07 01:38:20]

lgtm

[commit-bot: I haz the power, 2012-03-07 01:42:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/9560008/22020

[commit-bot: I haz the power, 2012-03-07 01:46:28]

Try job failure for 9560008-22020 on linux_clang for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_clan...

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

[commit-bot: I haz the power, 2012-03-07 06:01:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/9560008/35012

[commit-bot: I haz the power, 2012-03-07 07:34:53]

Change committed as 125361