Adding new test for mount observer. Also changing the usb mounter to only rel...

Adding new test for mount observer. Also changing the usb mounter to only reload the correct tab, not the full browser.

BUG=chromium-os:2057
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=42696

[Paweł Hajdan Jr., 2010-03-25 06:47:02]

Drive-by with a minor test comment.

http://codereview.chromium.org/1238006/diff/1/3
File chrome/browser/chromeos/usb_mount_observer_browsertest.cc (right):

http://codereview.chromium.org/1238006/diff/1/3#newcode47
chrome/browser/chromeos/usb_mount_observer_browsertest.cc:47:
chromeos::MockMountLibrary* lib = new chromeos::MockMountLibrary();
Could you use scoped_ptr here?

[dhg, 2010-03-25 17:22:52]

Done.  Also updated to use GMock.

On 2010/03/25 06:47:02, Paweł Hajdan Jr. wrote:
> Drive-by with a minor test comment.
> 
> http://codereview.chromium.org/1238006/diff/1/3
> File chrome/browser/chromeos/usb_mount_observer_browsertest.cc (right):
> 
> http://codereview.chromium.org/1238006/diff/1/3#newcode47
> chrome/browser/chromeos/usb_mount_observer_browsertest.cc:47:
> chromeos::MockMountLibrary* lib = new chromeos::MockMountLibrary();
> Could you use scoped_ptr here?

[zel, 2010-03-25 17:30:12]

LGTM

few minor comments

http://codereview.chromium.org/1238006/diff/18001/19001
File chrome/browser/chromeos/usb_mount_observer.cc (right):

http://codereview.chromium.org/1238006/diff/18001/19001#newcode66
chrome/browser/chromeos/usb_mount_observer.cc:66: const
chromeos::MountLibrary::DiskVector& disks = obj->disks();
extra spaces?

http://codereview.chromium.org/1238006/diff/18001/19002
File chrome/browser/chromeos/usb_mount_observer_browsertest.cc (right):

http://codereview.chromium.org/1238006/diff/18001/19002#newcode46
chrome/browser/chromeos/usb_mount_observer_browsertest.cc:46:
observe->set_profile(browser()->profile());
There might be a better way to get a profile than this. You might want to ping
sky to see if there is a test or mock version of profile that we can use.

[dhg, 2010-03-25 17:34:28]

oh and done on the other one too.

On Thu, Mar 25, 2010 at 10:33 AM, David Garcia <dhg@chromium.org> wrote:
> re: profile. It doesn't work.  I tried the mock one.  There's a whole
> bunch of unimplemented functionality.  Crashes when you try to use it
> on a chromeos build.   Looks like its something with the
> desktop_notification stuff.  We should probably think about fixing it.
>   This was simpler and closer to real world anyway.
>
> On Thu, Mar 25, 2010 at 10:30 AM,  <zelidrag@chromium.org> wrote:
>> LGTM
>>
>> few minor comments
>>
>>
>> http://codereview.chromium.org/1238006/diff/18001/19001
>> File chrome/browser/chromeos/usb_mount_observer.cc (right):
>>
>> http://codereview.chromium.org/1238006/diff/18001/19001#newcode66
>> chrome/browser/chromeos/usb_mount_observer.cc:66: const
>> chromeos::MountLibrary::DiskVector& disks = obj->disks();
>> extra spaces?
>>
>> http://codereview.chromium.org/1238006/diff/18001/19002
>> File chrome/browser/chromeos/usb_mount_observer_browsertest.cc (right):
>>
>> http://codereview.chromium.org/1238006/diff/18001/19002#newcode46
>> chrome/browser/chromeos/usb_mount_observer_browsertest.cc:46:
>> observe->set_profile(browser()->profile());
>> There might be a better way to get a profile than this. You might want
>> to ping sky to see if there is a test or mock version of profile that we
>> can use.
>>
>> http://codereview.chromium.org/1238006
>>
>
>
>
> --
> David Garcia
> ---------------------
> I'm participating in Team In Training!  Check it out:
> http://pages.teamintraining.org/sj/AveGiant10/dgarciaijk
>



-- 
David Garcia
---------------------
I'm participating in Team In Training!  Check it out:
http://pages.teamintraining.org/sj/AveGiant10/dgarciaijk

[dhg, 2010-03-25 17:34:34]

re: profile. It doesn't work.  I tried the mock one.  There's a whole
bunch of unimplemented functionality.  Crashes when you try to use it
on a chromeos build.   Looks like its something with the
desktop_notification stuff.  We should probably think about fixing it.
   This was simpler and closer to real world anyway.

On Thu, Mar 25, 2010 at 10:30 AM,  <zelidrag@chromium.org> wrote:
> LGTM
>
> few minor comments
>
>
> http://codereview.chromium.org/1238006/diff/18001/19001
> File chrome/browser/chromeos/usb_mount_observer.cc (right):
>
> http://codereview.chromium.org/1238006/diff/18001/19001#newcode66
> chrome/browser/chromeos/usb_mount_observer.cc:66: const
> chromeos::MountLibrary::DiskVector& disks = obj->disks();
> extra spaces?
>
> http://codereview.chromium.org/1238006/diff/18001/19002
> File chrome/browser/chromeos/usb_mount_observer_browsertest.cc (right):
>
> http://codereview.chromium.org/1238006/diff/18001/19002#newcode46
> chrome/browser/chromeos/usb_mount_observer_browsertest.cc:46:
> observe->set_profile(browser()->profile());
> There might be a better way to get a profile than this. You might want
> to ping sky to see if there is a test or mock version of profile that we
> can use.
>
> http://codereview.chromium.org/1238006
>



-- 
David Garcia
---------------------
I'm participating in Team In Training!  Check it out:
http://pages.teamintraining.org/sj/AveGiant10/dgarciaijk

[Paweł Hajdan Jr., 2010-03-25 20:58:38]

After fixing the nit LGTM.

http://codereview.chromium.org/1238006/diff/24001/25002
File chrome/browser/chromeos/usb_mount_observer_browsertest.cc (right):

http://codereview.chromium.org/1238006/diff/24001/25002#newcode44
chrome/browser/chromeos/usb_mount_observer_browsertest.cc:44:
GURL("chrome://downloads"));
nit: Please use a constant from chrome/common/url_constants.h

[dhg, 2010-03-25 21:43:35]

Done
On 2010/03/25 20:58:38, Paweł Hajdan Jr. wrote:
> After fixing the nit LGTM.
> 
> http://codereview.chromium.org/1238006/diff/24001/25002
> File chrome/browser/chromeos/usb_mount_observer_browsertest.cc (right):
> 
> http://codereview.chromium.org/1238006/diff/24001/25002#newcode44
> chrome/browser/chromeos/usb_mount_observer_browsertest.cc:44:
> GURL("chrome://downloads"));
> nit: Please use a constant from chrome/common/url_constants.h