Linux removable device notifications: notify about non-dcim devices.

Linux removable device notifications: notify about non-dcim devices.

Depends on https://chromiumcodereview.appspot.com/10882039/

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

[vandebo (ex-Chrome), 2012-08-29 00:30:59]

+kmadhusu@ because thestig@ has a lot on his plate at the moment.

[kmadhusu, 2012-08-29 01:12:01]

https://chromiumcodereview.appspot.com/10880078/diff/8001/chrome/browser/medi...
File chrome/browser/media_gallery/removable_device_notifications_linux.cc
(right):

https://chromiumcodereview.appspot.com/10880078/diff/8001/chrome/browser/medi...
chrome/browser/media_gallery/removable_device_notifications_linux.cc:390: if
(has_priority->second)
nit : {}

https://chromiumcodereview.appspot.com/10880078/diff/8001/chrome/browser/medi...
File
chrome/browser/media_gallery/removable_device_notifications_linux_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10880078/diff/8001/chrome/browser/medi...
chrome/browser/media_gallery/removable_device_notifications_linux_unittest.cc:316:
// Only |kDevice2| should be attached, since |kDevice1| has a bad path.
nit: kDevice1 => kDevice5

https://chromiumcodereview.appspot.com/10880078/diff/8001/chrome/browser/medi...
chrome/browser/media_gallery/removable_device_notifications_linux_unittest.cc:354:
.InSequence(mock_sequence);
We will also receive an attach and detach for kDevice5. Can you add those
checks?

https://chromiumcodereview.appspot.com/10880078/diff/8001/chrome/browser/medi...
chrome/browser/media_gallery/removable_device_notifications_linux_unittest.cc:572:
// kDevice5 -> kMountPointC
Why did you add a * in kMountPointB but not to kMountPointC?

https://chromiumcodereview.appspot.com/10880078/diff/8001/chrome/browser/medi...
chrome/browser/media_gallery/removable_device_notifications_linux_unittest.cc:658:
// kDevice5 -> kMountPointC
nit: kMountPointC * ???

[vandebo (ex-Chrome), 2012-08-29 06:10:13]

https://chromiumcodereview.appspot.com/10880078/diff/8001/chrome/browser/medi...
File chrome/browser/media_gallery/removable_device_notifications_linux.cc
(right):

https://chromiumcodereview.appspot.com/10880078/diff/8001/chrome/browser/medi...
chrome/browser/media_gallery/removable_device_notifications_linux.cc:390: if
(has_priority->second)
On 2012/08/29 01:12:02, kmadhusu wrote:
> nit : {}

Done.

https://chromiumcodereview.appspot.com/10880078/diff/8001/chrome/browser/medi...
File
chrome/browser/media_gallery/removable_device_notifications_linux_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10880078/diff/8001/chrome/browser/medi...
chrome/browser/media_gallery/removable_device_notifications_linux_unittest.cc:316:
// Only |kDevice2| should be attached, since |kDevice1| has a bad path.
On 2012/08/29 01:12:02, kmadhusu wrote:
> nit: kDevice1 => kDevice5

Done.

https://chromiumcodereview.appspot.com/10880078/diff/8001/chrome/browser/medi...
chrome/browser/media_gallery/removable_device_notifications_linux_unittest.cc:354:
.InSequence(mock_sequence);
On 2012/08/29 01:12:02, kmadhusu wrote:
> We will also receive an attach and detach for kDevice5. Can you add those
> checks?

No we won't, device5 is not removable.

https://chromiumcodereview.appspot.com/10880078/diff/8001/chrome/browser/medi...
chrome/browser/media_gallery/removable_device_notifications_linux_unittest.cc:572:
// kDevice5 -> kMountPointC
On 2012/08/29 01:12:02, kmadhusu wrote:
> Why did you add a * in kMountPointB but not to kMountPointC?

The mount points with *'s are the ones we've notified system monitor about.

[kmadhusu, 2012-08-29 16:46:19]

lgtm

https://chromiumcodereview.appspot.com/10880078/diff/8001/chrome/browser/medi...
File
chrome/browser/media_gallery/removable_device_notifications_linux_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10880078/diff/8001/chrome/browser/medi...
chrome/browser/media_gallery/removable_device_notifications_linux_unittest.cc:316:
// Only |kDevice2| should be attached, since |kDevice1| has a bad path.
On 2012/08/29 06:10:13, vandebo wrote:
> On 2012/08/29 01:12:02, kmadhusu wrote:
> > nit: kDevice1 => kDevice5
> 
> Done.

Only |kDevice2| should be attached, since |kDevice5| has a bad path.

https://chromiumcodereview.appspot.com/10880078/diff/8001/chrome/browser/medi...
chrome/browser/media_gallery/removable_device_notifications_linux_unittest.cc:354:
.InSequence(mock_sequence);
On 2012/08/29 06:10:13, vandebo wrote:
> On 2012/08/29 01:12:02, kmadhusu wrote:
> > We will also receive an attach and detach for kDevice5. Can you add those
> > checks?
> 
> No we won't, device5 is not removable.

For readability, we should have better names for these test devices. You can do
that change in a separate CL.

https://chromiumcodereview.appspot.com/10880078/diff/8001/chrome/browser/medi...
chrome/browser/media_gallery/removable_device_notifications_linux_unittest.cc:382:
// kDevice2 -> kMountPointB *
nit: It took a while for me to understand what "*" means in the mount point
info. May be you want to add a note somewhere.

[vandebo (ex-Chrome), 2012-08-29 17:04:39]

https://chromiumcodereview.appspot.com/10880078/diff/8001/chrome/browser/medi...
File
chrome/browser/media_gallery/removable_device_notifications_linux_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10880078/diff/8001/chrome/browser/medi...
chrome/browser/media_gallery/removable_device_notifications_linux_unittest.cc:316:
// Only |kDevice2| should be attached, since |kDevice1| has a bad path.
On 2012/08/29 16:46:19, kmadhusu wrote:
> On 2012/08/29 06:10:13, vandebo wrote:
> > On 2012/08/29 01:12:02, kmadhusu wrote:
> > > nit: kDevice1 => kDevice5
> > 
> > Done.
> 
> Only |kDevice2| should be attached, since |kDevice5| has a bad path.

Oops, fixed.

https://chromiumcodereview.appspot.com/10880078/diff/8001/chrome/browser/medi...
chrome/browser/media_gallery/removable_device_notifications_linux_unittest.cc:354:
.InSequence(mock_sequence);
On 2012/08/29 16:46:19, kmadhusu wrote:
> On 2012/08/29 06:10:13, vandebo wrote:
> > On 2012/08/29 01:12:02, kmadhusu wrote:
> > > We will also receive an attach and detach for kDevice5. Can you add those
> > > checks?
> > 
> > No we won't, device5 is not removable.
> 
> For readability, we should have better names for these test devices. You can
do
> that change in a separate CL. 

Done.

https://chromiumcodereview.appspot.com/10880078/diff/8001/chrome/browser/medi...
chrome/browser/media_gallery/removable_device_notifications_linux_unittest.cc:382:
// kDevice2 -> kMountPointB *
On 2012/08/29 16:46:19, kmadhusu wrote:
> nit: It took a while for me to understand what "*" means in the mount point
> info. May be you want to add a note somewhere.

Done.

[kmadhusu, 2012-08-29 17:18:04]

lgtm. Thanks for fixing nits.

[Lei Zhang, 2012-08-29 20:27:21]

lgtm++

[commit-bot: I haz the power, 2012-08-29 21:34:31]

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

[commit-bot: I haz the power, 2012-08-29 23:11:53]

Failed to apply patch for
chrome/browser/media_gallery/removable_device_notifications_linux.cc:
While running patch -p1 --forward --force;
patching file
chrome/browser/media_gallery/removable_device_notifications_linux.cc
Hunk #1 FAILED at 385.
Hunk #2 FAILED at 422.
Hunk #3 FAILED at 493.
3 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/media_gallery/removable_device_notifications_linux.cc.rej

[commit-bot: I haz the power, 2012-08-30 13:20:50]

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

[commit-bot: I haz the power, 2012-08-30 15:41:22]

Change committed as 154155