Rewritten MountLibrary to work with non-blocking mount API calls in libcros....

Rewritten MountLibrary to work with non-blocking mount API calls in libcros.

BUG=chromium-os:13190
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=78612

[stevenjb, 2011-03-16 22:52:55]

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/cros/mo...
File chrome/browser/chromeos/cros/mount_library.cc (right):

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:122: // This method will receive
events that are caused by
partial comment

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:190: LOG(WARNING) << "Found disk "
<< device_path;
-> VLOG(1)

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:204: }
nit: { } not needed

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:207: }
nit: { } not needed

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:210: }
nit: { } not needed

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:216: disk->on_boot_device());
Is it valid for a Disk to have any empty path, mountpath, and/or systempath?
i.e. should there be an extra check here?

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:218: std::pair<std::string,
Disk*>(std::string(device_path), new_disk));
nit: second std::string conversion of device_path - maybe use local variable.

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:229: size_t device_len,
nit: devices_len

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:236: for (size_t i = 0; i <
device_len; i++) {
DCHECK(devices[i]) or if (!devices[i]) continue;

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:251: ++iter) {
++iter here and erase(iter++) below; remove this and add ++iter in else clause.

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:282: disks_.erase(iter++);
++ unnecessary

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/usb_mou...
File chrome/browser/chromeos/usb_mount_observer.cc (right):

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/usb_mou...
chrome/browser/chromeos/usb_mount_observer.cc:120: LOG(WARNING) << "Disk added:
" << disk->device_path();
-> VLOG(1)

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/usb_mou...
chrome/browser/chromeos/usb_mount_observer.cc:128: }
nit: unneeded {}

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/usb_mou...
chrome/browser/chromeos/usb_mount_observer.cc:142: LOG(WARNING) << "Disk
removed: " << disk->device_path();
VLOG(1)

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/usb_mou...
chrome/browser/chromeos/usb_mount_observer.cc:159: LOG(WARNING) << "Disk changed
: " << disk->device_path();
VLOG(1)

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/usb_mou...
chrome/browser/chromeos/usb_mount_observer.cc:189: LOG(WARNING) << "Device added
: " << device_path;
VLOG(1)

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/usb_mou...
chrome/browser/chromeos/usb_mount_observer.cc:199: LOG(WARNING) << "Device
scanned : " << device_path;
VLOG(1)

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/webui/i...
File chrome/browser/chromeos/webui/imageburner_ui.cc (right):

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/webui/i...
chrome/browser/chromeos/webui/imageburner_ui.cc:232: }
nit: extra { }

http://codereview.chromium.org/6674043/diff/1/chrome/browser/sessions/session...
File chrome/browser/sessions/session_restore.cc (right):

http://codereview.chromium.org/6674043/diff/1/chrome/browser/sessions/session...
chrome/browser/sessions/session_restore.cc:662: #endif
Maybe only do the DCHECK for non OS_CHROMEOS and make the logic the same for
both?

[xiyuan, 2011-03-17 00:23:32]

http://codereview.chromium.org/6674043/diff/7003/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/mount_library.h (right):

http://codereview.chromium.org/6674043/diff/7003/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/mount_library.h:55: const std::string system_path()
const { return system_path_; }
nit: return const std::string& ?

http://codereview.chromium.org/6674043/diff/7003/chrome/browser/chromeos/usb_...
File chrome/browser/chromeos/usb_mount_observer.cc (right):

http://codereview.chromium.org/6674043/diff/7003/chrome/browser/chromeos/usb_...
chrome/browser/chromeos/usb_mount_observer.cc:93: OnDiskAdded(disk);
nit: one less indent space.

http://codereview.chromium.org/6674043/diff/7003/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_restore.cc (right):

http://codereview.chromium.org/6674043/diff/7003/chrome/browser/sessions/sess...
chrome/browser/sessions/session_restore.cc:657: #if defined(OS_CHROMEOS)
Is this file used for non-chromeos build at all?

[zel, 2011-03-17 01:00:29]

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/cros/mo...
File chrome/browser/chromeos/cros/mount_library.cc (right):

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:122: // This method will receive
events that are caused by
On 2011/03/16 22:52:55, Steven Bennetts wrote:
> partial comment

Done.

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:204: }
On 2011/03/16 22:52:55, Steven Bennetts wrote:
> nit: { } not needed

Done.

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:207: }
On 2011/03/16 22:52:55, Steven Bennetts wrote:
> nit: { } not needed

Done.

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:210: }
On 2011/03/16 22:52:55, Steven Bennetts wrote:
> nit: { } not needed

Done.

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:216: disk->on_boot_device());
On 2011/03/16 22:52:55, Steven Bennetts wrote:
> Is it valid for a Disk to have any empty path, mountpath, and/or systempath?
> i.e. should there be an extra check here?

Yes, mount path can be empty. We actually kick off mount operation when we
detect this state.

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:218: std::pair<std::string,
Disk*>(std::string(device_path), new_disk));
On 2011/03/16 22:52:55, Steven Bennetts wrote:
> nit: second std::string conversion of device_path - maybe use local variable.

Done.

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:229: size_t device_len,
On 2011/03/16 22:52:55, Steven Bennetts wrote:
> nit: devices_len

Done.

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:236: for (size_t i = 0; i <
device_len; i++) {
On 2011/03/16 22:52:55, Steven Bennetts wrote:
> DCHECK(devices[i]) or if (!devices[i]) continue;

Done.

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:251: ++iter) {
On 2011/03/16 22:52:55, Steven Bennetts wrote:
> ++iter here and erase(iter++) below; remove this and add ++iter in else
clause.

Good catch. Done.

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:282: disks_.erase(iter++);
On 2011/03/16 22:52:55, Steven Bennetts wrote:
> ++ unnecessary

Done.

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/usb_mou...
File chrome/browser/chromeos/usb_mount_observer.cc (right):

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/usb_mou...
chrome/browser/chromeos/usb_mount_observer.cc:120: LOG(WARNING) << "Disk added:
" << disk->device_path();
On 2011/03/16 22:52:55, Steven Bennetts wrote:
> -> VLOG(1)

Done.

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/usb_mou...
chrome/browser/chromeos/usb_mount_observer.cc:128: }
On 2011/03/16 22:52:55, Steven Bennetts wrote:
> nit: unneeded {}

Done.

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/usb_mou...
chrome/browser/chromeos/usb_mount_observer.cc:142: LOG(WARNING) << "Disk
removed: " << disk->device_path();
On 2011/03/16 22:52:55, Steven Bennetts wrote:
> VLOG(1)

Done.

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/usb_mou...
chrome/browser/chromeos/usb_mount_observer.cc:159: LOG(WARNING) << "Disk changed
: " << disk->device_path();
On 2011/03/16 22:52:55, Steven Bennetts wrote:
> VLOG(1)

Done.

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/usb_mou...
chrome/browser/chromeos/usb_mount_observer.cc:189: LOG(WARNING) << "Device added
: " << device_path;
On 2011/03/16 22:52:55, Steven Bennetts wrote:
> VLOG(1)

Done.

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/usb_mou...
chrome/browser/chromeos/usb_mount_observer.cc:199: LOG(WARNING) << "Device
scanned : " << device_path;
On 2011/03/16 22:52:55, Steven Bennetts wrote:
> VLOG(1)

Done.

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/webui/i...
File chrome/browser/chromeos/webui/imageburner_ui.cc (right):

http://codereview.chromium.org/6674043/diff/1/chrome/browser/chromeos/webui/i...
chrome/browser/chromeos/webui/imageburner_ui.cc:232: }
On 2011/03/16 22:52:55, Steven Bennetts wrote:
> nit: extra { }

Done.

http://codereview.chromium.org/6674043/diff/1/chrome/browser/sessions/session...
File chrome/browser/sessions/session_restore.cc (right):

http://codereview.chromium.org/6674043/diff/1/chrome/browser/sessions/session...
chrome/browser/sessions/session_restore.cc:662: #endif
On 2011/03/16 22:52:55, Steven Bennetts wrote:
> Maybe only do the DCHECK for non OS_CHROMEOS and make the logic the same for
> both?

This solves the crash that affects our debugging, DCHECK won't do the trick
here.

http://codereview.chromium.org/6674043/diff/7003/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/mount_library.h (right):

http://codereview.chromium.org/6674043/diff/7003/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/mount_library.h:55: const std::string system_path()
const { return system_path_; }
On 2011/03/17 00:23:32, xiyuan wrote:
> nit: return const std::string& ?

Done.

http://codereview.chromium.org/6674043/diff/7003/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_restore.cc (right):

http://codereview.chromium.org/6674043/diff/7003/chrome/browser/sessions/sess...
chrome/browser/sessions/session_restore.cc:657: #if defined(OS_CHROMEOS)
On 2011/03/17 00:23:32, xiyuan wrote:
> Is this file used for non-chromeos build at all?

Yes it is.

[stevenjb, 2011-03-17 01:30:37]

LGTM, but note comment about DEPS change.

http://codereview.chromium.org/6674043/diff/1/chrome/browser/sessions/session...
File chrome/browser/sessions/session_restore.cc (right):

http://codereview.chromium.org/6674043/diff/1/chrome/browser/sessions/session...
chrome/browser/sessions/session_restore.cc:662: #endif
On 2011/03/17 01:00:30, zel wrote:
> On 2011/03/16 22:52:55, Steven Bennetts wrote:
> > Maybe only do the DCHECK for non OS_CHROMEOS and make the logic the same for
> > both?
> 
> This solves the crash that affects our debugging, DCHECK won't do the trick
> here.
I meant to suggest leaving the DCHECK in for non-chromeos only, but have the
extra if() check for both.
This is fine as-is though.

http://codereview.chromium.org/6674043/diff/1014/tools/cros.DEPS/DEPS
File tools/cros.DEPS/DEPS (right):

http://codereview.chromium.org/6674043/diff/1014/tools/cros.DEPS/DEPS#newcode7
tools/cros.DEPS/DEPS:7: Var("chromium_git") + "/cros.git@3b12cd",
Note: the trybots will fail if you don't push the DEPS changes first (the patch
is applied after gclient sync). I'm not sure about the other builders.

[zel, 2011-03-17 01:37:20]

http://codereview.chromium.org/6674043/diff/1014/tools/cros.DEPS/DEPS
File tools/cros.DEPS/DEPS (right):

http://codereview.chromium.org/6674043/diff/1014/tools/cros.DEPS/DEPS#newcode7
tools/cros.DEPS/DEPS:7: Var("chromium_git") + "/cros.git@3b12cd",
On 2011/03/17 01:30:37, Steven Bennetts wrote:
> Note: the trybots will fail if you don't push the DEPS changes first (the
patch
> is applied after gclient sync). I'm not sure about the other builders.

Yes, i've just noticed that. I will push this first then.

[xiyuan, 2011-03-17 01:54:03]

LGTM

http://codereview.chromium.org/6674043/diff/7003/chrome/browser/sessions/sess...
File chrome/browser/sessions/session_restore.cc (right):

http://codereview.chromium.org/6674043/diff/7003/chrome/browser/sessions/sess...
chrome/browser/sessions/session_restore.cc:657: #if defined(OS_CHROMEOS)
On 2011/03/17 01:00:30, zel wrote:
> On 2011/03/17 00:23:32, xiyuan wrote:
> > Is this file used for non-chromeos build at all?
> 
> Yes it is.

Yep. Sorry I misread the file name and thought it was under chromeos dir.

http://codereview.chromium.org/6674043/diff/7006/chrome/browser/chromeos/usb_...
File chrome/browser/chromeos/usb_mount_observer.cc (right):

http://codereview.chromium.org/6674043/diff/7006/chrome/browser/chromeos/usb_...
chrome/browser/chromeos/usb_mount_observer.cc:94: OnDiskAdded(disk);
nit: one less indent space.

[zel, 2011-03-17 01:57:46]

http://codereview.chromium.org/6674043/diff/7006/chrome/browser/chromeos/usb_...
File chrome/browser/chromeos/usb_mount_observer.cc (right):

http://codereview.chromium.org/6674043/diff/7006/chrome/browser/chromeos/usb_...
chrome/browser/chromeos/usb_mount_observer.cc:94: OnDiskAdded(disk);
On 2011/03/17 01:54:03, xiyuan wrote:
> nit: one less indent space.

Done.

[stevenjb, 2011-03-17 21:32:04]

Changes through patch set 9 LGTM.