Add CrosDisksClient

Add CrosDisksClient

This patch is 1 of 3 splitted patches made from http://codereview.chromium.org/8386031
cros_disks_client.cc is made by mixing and refactoring chromeos_mount.cc of libcros and chrome/browser/chromeos/cros/mount_library.cc
Added code is still not used.

BUG=chromium-os:16556
TEST=build success


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

[satorux1, 2011-11-08 18:07:22]

This is a great piece of code, but I have a relatively large request.

The code in chrome/browser/chromeos/dbus is supposed to be just thin D-Bus
clients, but CrosDisksClient is much more intelligent than that. Good news is
that the thin D-Bus client part and the higher level class are already separate
in your patch: CrosDisksDBusProxy and CrosDisksClient.

I'd like you to split this to two parts:

- chrome/browser/chromeos/dbus/cros_disks_clients.{h,cc} -- this should just
contain thin D-Bus client code, nothing more than that. Hopefully,
CrosDisksDBusProxy is exactly the one. You can rename it to CrosDisksDBusClient.

- chrome/browser/chromeos/disks/cros_disks_manager.{h,cc} -- this implements
higher level operations on top of the D-Bus client. Hopefully, CrosDisksClient
is exactly the one. You can rename it to CrosDisksManager.

I know it's a little tedious to do this, but I think the separation will make
code cleaner.

FWIW, bluetooth code follows this design. The thin clients are in
chrome/browser/chromeos/dbus, and the higher level classes are in
chrome/browser/chromeos/bluetooth.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/cros_disks_client.cc (right):

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:36: };
Add a blank line here?

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:41: enum MountEventType {
Please add some comment.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:51: class DiskInfo {
Class comment is missing.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:150: // Initialize |this| from
|response| given by the cros-disks service.
The parsing code in this function is relatively complex. Could you illustrate
the contents of |response| here? You can dump the contents of |response| by
response->ToString() and copy-paste the output here with some manual formatting.

Then, the code will be easier to understand.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:192: bool is_rotational,
is_optical;
 bool is_rotational = false;
 bool is_optiocal = false;

Let's make them initialized.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:270: const MountPathOptions&
options,
This parameter seems to be unused, which is also the case with
chromeos_mount.cc.

Can you remove this parameter? Otherwise, please rename it to |unused_options|.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:400: struct SignalEventTuple {
Please add some comment.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:537: ~CrosDisksClientImpl() {
Add virtual.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:709: void* user_data;
= NULL;

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:711: size_t
pending_callbacks_count;
= 0;

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/cros_disks_client.h (right):

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.h:21: enum MountType {
I know this is just inheriting from the original code, but since it's a good
opportunity, could you add some comment? Please also check other enum types.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.h:25: MOUNT_TYPE_NETWORK_STORAGE
Let's add a comma. It's a good practice, as it'll make it easier to add new
members.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.h:48: typedef
std::vector<std::pair<std::string, std::string> > MountPathOptions;
Why not map<string>? Maybe the order matters? Would be nice to add some comment
with some example of the contents.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.h:98: const std::string&
device_path() const { return device_path_; }
Please add a blank line between accessors. I think the code is a bit too dense
and hard to read.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.h:185: const MountPointInfo&
mount_info) = 0;
This may be obvious, but please could you add function comments for them?

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.h:194: virtual void
RequestMountInfoRefresh() = 0;
Function comment is missing. Please check other functions too.

[hashimoto, 2011-11-09 02:33:25]

New patch set reflecting comments.
Going to split cros_disks_client.cc in the next patch.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/cros_disks_client.cc (right):

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:36: };
On 2011/11/08 18:07:22, satorux1 wrote:
> Add a blank line here?

Done.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:41: enum MountEventType {
On 2011/11/08 18:07:22, satorux1 wrote:
> Please add some comment.

Done.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:51: class DiskInfo {
On 2011/11/08 18:07:22, satorux1 wrote:
> Class comment is missing.

Done.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:150: // Initialize |this| from
|response| given by the cros-disks service.
On 2011/11/08 18:07:22, satorux1 wrote:
> The parsing code in this function is relatively complex. Could you illustrate
> the contents of |response| here? You can dump the contents of |response| by
> response->ToString() and copy-paste the output here with some manual
formatting.
> 
> Then, the code will be easier to understand.

Done.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:192: bool is_rotational,
is_optical;
On 2011/11/08 18:07:22, satorux1 wrote:
>  bool is_rotational = false;
>  bool is_optiocal = false;
> 
> Let's make them initialized.

Done.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:270: const MountPathOptions&
options,
On 2011/11/08 18:07:22, satorux1 wrote:
> This parameter seems to be unused, which is also the case with
> chromeos_mount.cc.
> 
> Can you remove this parameter? Otherwise, please rename it to
|unused_options|.

Removed.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:400: struct SignalEventTuple {
On 2011/11/08 18:07:22, satorux1 wrote:
> Please add some comment.

Done.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:537: ~CrosDisksClientImpl() {
On 2011/11/08 18:07:22, satorux1 wrote:
> Add virtual.

Done.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.cc:709: void* user_data;
On 2011/11/08 18:07:22, satorux1 wrote:
> = NULL;

|data| and |pending_callbacks_count| are initialized by the ctor, you mean there
should be a default ctor?

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
File chrome/browser/chromeos/dbus/cros_disks_client.h (right):

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.h:21: enum MountType {
On 2011/11/08 18:07:22, satorux1 wrote:
> I know this is just inheriting from the original code, but since it's a good
> opportunity, could you add some comment? Please also check other enum types.

Done.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.h:25: MOUNT_TYPE_NETWORK_STORAGE
On 2011/11/08 18:07:22, satorux1 wrote:
> Let's add a comma. It's a good practice, as it'll make it easier to add new
> members.

Done.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.h:48: typedef
std::vector<std::pair<std::string, std::string> > MountPathOptions;
On 2011/11/08 18:07:22, satorux1 wrote:
> Why not map<string>? Maybe the order matters? Would be nice to add some
comment
> with some example of the contents.

Removed because it is no longer used.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.h:98: const std::string&
device_path() const { return device_path_; }
On 2011/11/08 18:07:22, satorux1 wrote:
> Please add a blank line between accessors. I think the code is a bit too dense
> and hard to read.

Done.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.h:185: const MountPointInfo&
mount_info) = 0;
On 2011/11/08 18:07:22, satorux1 wrote:
> This may be obvious, but please could you add function comments for them?

Done.

http://codereview.chromium.org/8499007/diff/2001/chrome/browser/chromeos/dbus...
chrome/browser/chromeos/dbus/cros_disks_client.h:194: virtual void
RequestMountInfoRefresh() = 0;
On 2011/11/08 18:07:22, satorux1 wrote:
> Function comment is missing. Please check other functions too.

Done.

[hashimoto, 2011-11-09 06:53:35]

CrosDisksClient is renamed to DiskMountManager and CrosDisksDBusProxy is now
named as CrosDisksClient to make it consistent with other dbus/*_client.h/cc

[satorux1, 2011-11-11 00:43:55]

Sorry for the belated response. I somehow didn't notice that the new version was
uploaded yesterday. 

The new patch looks pretty good. We are almost there!

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/cros_disks_client.cc (right):

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.cc:41: // It returns true when a
value is popped, false otherwise.
It returns -> Returns

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/cros_disks_client.h (right):

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.h:118: bool is_hidden() const {
return is_hidden_; }
function comment missing.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/mock_dbus_thread_manager.h (right):

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/mock_dbus_thread_manager.h:75: 
Please remove the two blank lines at the end.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
File chrome/browser/chromeos/disk_mount_manager.cc (right):

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.cc:46: virtual void
AddObserver(Observer* observer) OVERRIDE {
Add something like below for override functions.

// DiskMountManager override.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.cc:64:
CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
DCHECK

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.cc:398: void
OnMountEvent(MountEventType evt, std::string device_path) {
evt -> event? we eschew abbreviation like this.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.cc:454: void
FireDiskStatusUpdate(DiskMountManagerEventType evt, const Disk* disk) {
Fire -> Notify. This will match the comment and easier to understand.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.cc:490: return EmptyString();
As you are returning by value, you can just return "" instead of EmptyString().

from string_util.h:

// These should not be (snip)  return values for functions
// which return by value or outparam.                                           
                  
BASE_EXPORT const std::string& EmptyString();

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.cc:583: return EmptyString();
"";

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.cc:598: return EmptyString();
""

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
File chrome/browser/chromeos/disk_mount_manager.h (right):

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
chrome/browser/chromeos directly contains many files, and we probably don't want
to add more. Can you put this and related files under
chrome/browser/chromeos/disks?

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.h:29: // Condition of mounted
filesystem
period is missing.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.h:40: enum MountEvent {
Add a one-line comment?

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.h:43: };
Add a blank line here.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.h:93: // Total size of the device.
in bytes? in mega bytes? let's make it clear.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.h:94: uint64 total_size() const {
return total_size_; }
something like total_size_in_bytes() will be clearer.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.h:137: struct MountPointInfo {
Could you add a comment?

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.h:144: const MountType type,
MountCondition condition)
You might want to list parameters vertically

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.h:157: class Observer {
A brief class comment would be nice.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.h:175: // Adds an observer
period is missing. please check other places. sorry about being nit-picky.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/moc...
File chrome/browser/chromeos/mock_disk_mount_manager.cc (right):

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/moc...
chrome/browser/chromeos/mock_disk_mount_manager.cc:71: false));
The booleans are unreadable. Would be nice to add comment line 

false,  // is_parent

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/moc...
chrome/browser/chromeos/mock_disk_mount_manager.cc:102: false));
ditto.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/moc...
File chrome/browser/chromeos/mock_disk_mount_manager.h (right):

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/moc...
chrome/browser/chromeos/mock_disk_mount_manager.h:40: void
FireDeviceInsertEvents();
Fire -> Notify. Please add a function comment.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/moc...
chrome/browser/chromeos/mock_disk_mount_manager.h:46: void
AddObserverInternal(DiskMountManager::Observer* observer);
function comment

[hashimoto, 2011-11-11 08:00:53]

New patch set

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/cros_disks_client.cc (right):

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.cc:41: // It returns true when a
value is popped, false otherwise.
On 2011/11/11 00:43:55, satorux1 wrote:
> It returns -> Returns

Done.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/cros_disks_client.h (right):

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.h:118: bool is_hidden() const {
return is_hidden_; }
On 2011/11/11 00:43:55, satorux1 wrote:
> function comment missing.

Done.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/mock_dbus_thread_manager.h (right):

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/mock_dbus_thread_manager.h:75: 
On 2011/11/11 00:43:55, satorux1 wrote:
> Please remove the two blank lines at the end.

Done.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
File chrome/browser/chromeos/disk_mount_manager.cc (right):

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.cc:46: virtual void
AddObserver(Observer* observer) OVERRIDE {
On 2011/11/11 00:43:55, satorux1 wrote:
> Add something like below for override functions.
> 
> // DiskMountManager override.

Done.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.cc:64:
CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2011/11/11 00:43:55, satorux1 wrote:
> DCHECK

Done.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.cc:398: void
OnMountEvent(MountEventType evt, std::string device_path) {
On 2011/11/11 00:43:55, satorux1 wrote:
> evt -> event? we eschew abbreviation like this.

Done.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.cc:454: void
FireDiskStatusUpdate(DiskMountManagerEventType evt, const Disk* disk) {
On 2011/11/11 00:43:55, satorux1 wrote:
> Fire -> Notify. This will match the comment and easier to understand.

Done.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.cc:490: return EmptyString();
On 2011/11/11 00:43:55, satorux1 wrote:
> As you are returning by value, you can just return "" instead of
EmptyString().
> 
> from string_util.h:
> 
> // These should not be (snip)  return values for functions
> // which return by value or outparam.                                         
 
>                   
> BASE_EXPORT const std::string& EmptyString();

Done.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.cc:583: return EmptyString();
On 2011/11/11 00:43:55, satorux1 wrote:
> "";

Done.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.cc:598: return EmptyString();
On 2011/11/11 00:43:55, satorux1 wrote:
> ""

Done.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
File chrome/browser/chromeos/disk_mount_manager.h (right):

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2011/11/11 00:43:55, satorux1 wrote:
> chrome/browser/chromeos directly contains many files, and we probably don't
want
> to add more. Can you put this and related files under
> chrome/browser/chromeos/disks?

Done.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.h:29: // Condition of mounted
filesystem
On 2011/11/11 00:43:55, satorux1 wrote:
> period is missing.

Done.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.h:40: enum MountEvent {
On 2011/11/11 00:43:55, satorux1 wrote:
> Add a one-line comment?

Done.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.h:43: };
On 2011/11/11 00:43:55, satorux1 wrote:
> Add a blank line here.

Done.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.h:93: // Total size of the device.
On 2011/11/11 00:43:55, satorux1 wrote:
> in bytes? in mega bytes? let's make it clear.

Done.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.h:94: uint64 total_size() const {
return total_size_; }
On 2011/11/11 00:43:55, satorux1 wrote:
> something like total_size_in_bytes() will be clearer.

Done.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.h:137: struct MountPointInfo {
On 2011/11/11 00:43:55, satorux1 wrote:
> Could you add a comment?

Done.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.h:144: const MountType type,
MountCondition condition)
On 2011/11/11 00:43:55, satorux1 wrote:
> You might want to list parameters vertically

Done.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.h:157: class Observer {
On 2011/11/11 00:43:55, satorux1 wrote:
> A brief class comment would be nice.

Done.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disk_mount_manager.h:175: // Adds an observer
On 2011/11/11 00:43:55, satorux1 wrote:
> period is missing. please check other places. sorry about being nit-picky.

Done.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/moc...
File chrome/browser/chromeos/mock_disk_mount_manager.cc (right):

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/moc...
chrome/browser/chromeos/mock_disk_mount_manager.cc:71: false));
On 2011/11/11 00:43:55, satorux1 wrote:
> The booleans are unreadable. Would be nice to add comment line 
> 
> false,  // is_parent

Done.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/moc...
chrome/browser/chromeos/mock_disk_mount_manager.cc:102: false));
On 2011/11/11 00:43:55, satorux1 wrote:
> ditto.

Done.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/moc...
File chrome/browser/chromeos/mock_disk_mount_manager.h (right):

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/moc...
chrome/browser/chromeos/mock_disk_mount_manager.h:40: void
FireDeviceInsertEvents();
On 2011/11/11 00:43:55, satorux1 wrote:
> Fire -> Notify. Please add a function comment.

Done.

http://codereview.chromium.org/8499007/diff/11005/chrome/browser/chromeos/moc...
chrome/browser/chromeos/mock_disk_mount_manager.h:46: void
AddObserverInternal(DiskMountManager::Observer* observer);
On 2011/11/11 00:43:55, satorux1 wrote:
> function comment

Done.

[satorux1, 2011-11-11 19:01:47]

Looks pretty good, and almost there.

I think almost all issues I pointed out are inherited from the original code,
but I think it's a good opportunity to clean them up. However if you think it's
risky to do so, you can keep them as is. We don't want to introduce regressions.
It's up to you.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/cros_disks_client.cc (right):

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.cc:24: "sync"
you might want to add ,

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.cc:28: "force"
ditto.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.cc:82: virtual void Mount(const
std::string& source_path,
// CrosDisksClient override.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.cc:177: { "FormattingFinished",
FORMATTING_FINISHED },
ah, signal names are hard-coded. I'll add constants to service_constants.h
today. Please use the constants once these are added.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.cc:194: "MountCompleted",
ditto.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.cc:523: CrosDisksClient* impl;
= NULL; just in case.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.cc:525: impl = new
CrosDisksClientImpl(bus);
return new CrosDisksClientImpl(bus);

may be simpler.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/cros_disks_client.h (right):

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.h:22: // Enum describing types of
mount used by cros-disks
a period is missing. our style guide wants to see periods. please check other
places too.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Punctu...

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.h:69: // Device path. (e.g.
/sys/devices/pci0000:00/.../8:0:0:0/block/sdb/sdb1)
Great to see examples like this!

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.h:79: // Is disk into a drive
(i.e. /dev/sdb vs, /dev/sdb1).
into -> info. BTW, it wasn't clear for me which one is a drive. /dev/sdb or
/dev/sdb1?

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.h:85: // Is the disk on deveice
we booted the machien from.
machien -> machine

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.h:88: // Disk file path (e.g
/dev/sdb).
e.g -> e.g.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.h:95: std::string drive_label()
const { return drive_model_; }
(e.g. "TransMemory")

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
File chrome/browser/chromeos/disks/disk_mount_manager.cc (right):

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:23: const char*
kDeviceNotFound = "Device could not be found";
For an obscure reason I cannot explain, the following is slightly more
efficient:

const char kDeviceNotFound[] = "Device could not be found";

See http://udrepper.livejournal.com/13851.html

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:73: base::Bind(&DoNothing),
Since this class will likely be the only client of CrosDisksClient, I guess you
can remove the callback parameter, rather than passing a callback that does
nothing. That may be simpler.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:74:
base::Bind(&DiskMountManagerImpl::OnMountCompleted,
The function name may be misleading. OnMountFailed, may be clearer, as it's only
used when it failed.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:88: base::Bind(&DoNothing));
likewise, you might want to remove the parameter.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:97: uint64_t
total_size_uint64 = 0;
total_size_uint64 -> total_size_in_bytes

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:198: ++it) {
For vectors, the following is more common and concise:

for (size_t i = 0; i < devices_to_unmount.size(); ++i)

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:202:
weak_ptr_factory_.GetWeakPtr(), cb_data, true),
Does this compile? I think the last parameter (mount_path) is missing here.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:266: MountType type,
type -> mount_type?

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:329: const std::string
file_path = it->second;
You can make it a reference. 

const std::string&

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:378:
disks_.insert(std::pair<std::string, Disk*>(disk_info.device_path(),
disks_.insert(std::make_pair(disk_info.device_path(), disk));

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:462: return;
LOG(ERROR) << "Unknown event" << event;

may be useful to have before 'return'

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:471: // Make sure we run on
UI thread.
You can remove the comment. DCHECK(...) is clear about the intention.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:496: int failed =
(file_path[0] == '!') ? 1 : 0;
This is unsafe if file_path is empty. What about:

const int failed = StartsWithASCII(file_path, "!", false);

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:499: if
(it->second->file_path().compare(file_path.c_str() + failed) == 0) {
The following would be easier to read?

// Skip the leading '!' on the failure case.
if (it->second->file_path() == file_path.substr(failed)) {

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:516: if
(system_path.find(*it, 0) == 0)
The following would be clearer?

const std::string& prefix = *it;
if (StartsWithASCII(system_path, prefix, false))
  return prefix;

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:542: PathMap
formatting_pending_;
This looks like a map of device_path -> file_path. Could you add some comment on
it, with some example?

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
File chrome/browser/chromeos/disks/disk_mount_manager.h (right):

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.h:13: namespace chromeos {
Would be nice to introduce a sub namespace.

namespace chromeos {
namespace disks {

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.h:76: const std::string&
file_path() const { return file_path_; }
Like DiskInfo in cros_disks_client.h, can you add examples to these paths?

[hashimoto, 2011-11-14 13:52:44]

Thank you for your detailed review.
I agree that this is a good chance to make the code clean.

This is a new patch set.
Changes on chrome/chrome_tests.gypi and chrome/chrome_browser.gypi are due to
the rebase.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/cros_disks_client.cc (right):

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.cc:24: "sync"
On 2011/11/11 19:01:48, satorux1 wrote:
> you might want to add ,

Done.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.cc:28: "force"
On 2011/11/11 19:01:48, satorux1 wrote:
> ditto.

Done.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.cc:82: virtual void Mount(const
std::string& source_path,
On 2011/11/11 19:01:48, satorux1 wrote:
> // CrosDisksClient override.

Done.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.cc:177: { "FormattingFinished",
FORMATTING_FINISHED },
On 2011/11/11 19:01:48, satorux1 wrote:
> ah, signal names are hard-coded. I'll add constants to service_constants.h
> today. Please use the constants once these are added.

OK, Thanks!

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.cc:525: impl = new
CrosDisksClientImpl(bus);
On 2011/11/11 19:01:48, satorux1 wrote:
> return new CrosDisksClientImpl(bus);
> 
> may be simpler.

Done.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
File chrome/browser/chromeos/dbus/cros_disks_client.h (right):

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.h:22: // Enum describing types of
mount used by cros-disks
On 2011/11/11 19:01:48, satorux1 wrote:
> a period is missing. our style guide wants to see periods. please check other
> places too.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Punctu...

Done.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.h:79: // Is disk into a drive
(i.e. /dev/sdb vs, /dev/sdb1).
On 2011/11/11 19:01:48, satorux1 wrote:
> into -> info. BTW, it wasn't clear for me which one is a drive. /dev/sdb or
> /dev/sdb1?

Done.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.h:85: // Is the disk on deveice
we booted the machien from.
On 2011/11/11 19:01:48, satorux1 wrote:
> machien -> machine

Done.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.h:88: // Disk file path (e.g
/dev/sdb).
On 2011/11/11 19:01:48, satorux1 wrote:
> e.g -> e.g.

Done.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.h:95: std::string drive_label()
const { return drive_model_; }
On 2011/11/11 19:01:48, satorux1 wrote:
> (e.g. "TransMemory")

Done.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
File chrome/browser/chromeos/disks/disk_mount_manager.cc (right):

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:23: const char*
kDeviceNotFound = "Device could not be found";
On 2011/11/11 19:01:48, satorux1 wrote:
> For an obscure reason I cannot explain, the following is slightly more
> efficient:
> 
> const char kDeviceNotFound[] = "Device could not be found";
> 
> See http://udrepper.livejournal.com/13851.html
Done.

Thank you for the pointer.
Besides the efficiency, const correctness is another good point about using
const char[] instead of const char* because const char[] is not modifieable
while const char* is.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:73: base::Bind(&DoNothing),
On 2011/11/11 19:01:48, satorux1 wrote:
> Since this class will likely be the only client of CrosDisksClient, I guess
you
> can remove the callback parameter, rather than passing a callback that does
> nothing. That may be simpler.

See the comment in UnmountPath.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:74:
base::Bind(&DiskMountManagerImpl::OnMountCompleted,
On 2011/11/11 19:01:48, satorux1 wrote:
> The function name may be misleading. OnMountFailed, may be clearer, as it's
only
> used when it failed.

OnMountCompleted is also called by MountCompleted signal.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:88: base::Bind(&DoNothing));
On 2011/11/11 19:01:48, satorux1 wrote:
> likewise, you might want to remove the parameter.

Call for Unmount here does not require error handling, however, call for Unmount
in DiskMountManagerImpl::UnmountDeviceRecursive requires error handling.

Besides Unmount, some of CrosDisksClient's methods do not require error handling
at all (EnumerateAutoMountableDevices and GetDeviceProperties) and Mount does
not require handling of success case as you say.
But, it is highly context dependent if a method call requires success/error
handling or not, like the case of Unmount.
I prefer simple success-and-error-handler-pair pattern rather than having
mixture of success-handler-only, error-handler-only and
success-and-error-handlers method signatures.

I think we should do some refactoring at first to eliminate ugly error handling
like appending "!" to indicate failure.
After the refactoring is done, we can make the error handling paths simpler and
remove the unused CrosDisksClient's callback arguments so that we no longer need
to have DoNothing.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:97: uint64_t
total_size_uint64 = 0;
On 2011/11/11 19:01:48, satorux1 wrote:
> total_size_uint64 -> total_size_in_bytes

Done.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:198: ++it) {
On 2011/11/11 19:01:48, satorux1 wrote:
> For vectors, the following is more common and concise:
> 
> for (size_t i = 0; i < devices_to_unmount.size(); ++i)

Done.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:202:
weak_ptr_factory_.GetWeakPtr(), cb_data, true),
On 2011/11/11 19:01:48, satorux1 wrote:
> Does this compile? I think the last parameter (mount_path) is missing here.

It compiles because cros-disks returns the path as the response for a method
call.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:266: MountType type,
On 2011/11/11 19:01:48, satorux1 wrote:
> type -> mount_type?

Done.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:329: const std::string
file_path = it->second;
On 2011/11/11 19:01:48, satorux1 wrote:
> You can make it a reference. 
> 
> const std::string&

Done.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:378:
disks_.insert(std::pair<std::string, Disk*>(disk_info.device_path(),
On 2011/11/11 19:01:48, satorux1 wrote:
> disks_.insert(std::make_pair(disk_info.device_path(), disk));

Done.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:462: return;
On 2011/11/11 19:01:48, satorux1 wrote:
> LOG(ERROR) << "Unknown event" << event;
> 
> may be useful to have before 'return'

Done.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:471: // Make sure we run on
UI thread.
On 2011/11/11 19:01:48, satorux1 wrote:
> You can remove the comment. DCHECK(...) is clear about the intention.

Done.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:496: int failed =
(file_path[0] == '!') ? 1 : 0;
On 2011/11/11 19:01:48, satorux1 wrote:
> This is unsafe if file_path is empty. What about:
> 
> const int failed = StartsWithASCII(file_path, "!", false);

Done.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:499: if
(it->second->file_path().compare(file_path.c_str() + failed) == 0) {
On 2011/11/11 19:01:48, satorux1 wrote:
> The following would be easier to read?
> 
> // Skip the leading '!' on the failure case.
> if (it->second->file_path() == file_path.substr(failed)) {
> 

Done.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:516: if
(system_path.find(*it, 0) == 0)
On 2011/11/11 19:01:48, satorux1 wrote:
> The following would be clearer?
> 
> const std::string& prefix = *it;
> if (StartsWithASCII(system_path, prefix, false))
>   return prefix;

Why is the last argument of StartsWithASCII is false?
Shouldn't we treat file paths in case sensitive way?

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:542: PathMap
formatting_pending_;
On 2011/11/11 19:01:48, satorux1 wrote:
> This looks like a map of device_path -> file_path. Could you add some comment
on
> it, with some example?

Done.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
File chrome/browser/chromeos/disks/disk_mount_manager.h (right):

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.h:13: namespace chromeos {
On 2011/11/11 19:01:48, satorux1 wrote:
> Would be nice to introduce a sub namespace.
> 
> namespace chromeos {
> namespace disks {

Done.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.h:76: const std::string&
file_path() const { return file_path_; }
On 2011/11/11 19:01:48, satorux1 wrote:
> Like DiskInfo in cros_disks_client.h, can you add examples to these paths?

Done.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.h:85: const std::string&
parent_path() const { return parent_path_; }
parent_path is removed because it is never used by any code.

[satorux1, 2011-11-14 17:56:23]

LGTM with one request. Your thoughts on the error handling (using "!" prefix is
ugly), is right. Please put the comment in the code. Even better, you can file a
bug and add a URL to the bug in the comment.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
File chrome/browser/chromeos/disks/disk_mount_manager.cc (right):

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:88: base::Bind(&DoNothing));
On 2011/11/14 13:52:45, hashimoto wrote:
> On 2011/11/11 19:01:48, satorux1 wrote:
> > likewise, you might want to remove the parameter.
> 
> Call for Unmount here does not require error handling, however, call for
Unmount
> in DiskMountManagerImpl::UnmountDeviceRecursive requires error handling.
> 
> Besides Unmount, some of CrosDisksClient's methods do not require error
handling
> at all (EnumerateAutoMountableDevices and GetDeviceProperties) and Mount does
> not require handling of success case as you say.
> But, it is highly context dependent if a method call requires success/error
> handling or not, like the case of Unmount.
> I prefer simple success-and-error-handler-pair pattern rather than having
> mixture of success-handler-only, error-handler-only and
> success-and-error-handlers method signatures.
> 
> I think we should do some refactoring at first to eliminate ugly error
handling
> like appending "!" to indicate failure.
> After the refactoring is done, we can make the error handling paths simpler
and
> remove the unused CrosDisksClient's callback arguments so that we no longer
need
> to have DoNothing.

Thank you for the explanation. Your comment here looks valuable. Would be nice
to put this comment in the code.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:202:
weak_ptr_factory_.GetWeakPtr(), cb_data, true),
On 2011/11/14 13:52:45, hashimoto wrote:
> On 2011/11/11 19:01:48, satorux1 wrote:
> > Does this compile? I think the last parameter (mount_path) is missing here.
> 
> It compiles because cros-disks returns the path as the response for a method
> call.

Oh you are right. I got confused as base::Bind() on the next line had one more
parameter.

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:516: if
(system_path.find(*it, 0) == 0)
On 2011/11/14 13:52:45, hashimoto wrote:
> On 2011/11/11 19:01:48, satorux1 wrote:
> > The following would be clearer?
> > 
> > const std::string& prefix = *it;
> > if (StartsWithASCII(system_path, prefix, false))
> >   return prefix;
> 
> Why is the last argument of StartsWithASCII is false?
> Shouldn't we treat file paths in case sensitive way?

I misunderstood the meaning of the last parameter of StartsWithASCII(). This is
why boolean parameter like this is bad. I wouldn't be confused if it's an enum
like CASE_SENSITIVE or CASE_INSENSITIVE...

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
File chrome/browser/chromeos/disks/disk_mount_manager.h (right):

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.h:85: const std::string&
parent_path() const { return parent_path_; }
On 2011/11/14 13:52:45, hashimoto wrote:
> parent_path is removed because it is never used by any code.

Nice!

[hashimoto, 2011-11-15 01:55:18]

New patch set.
Going to commit this.

[hashimoto, 2011-11-15 02:23:55]

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
File chrome/browser/chromeos/disks/disk_mount_manager.cc (right):

http://codereview.chromium.org/8499007/diff/10023/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:88: base::Bind(&DoNothing));
On 2011/11/14 17:56:23, satorux1 wrote:
> On 2011/11/14 13:52:45, hashimoto wrote:
> > On 2011/11/11 19:01:48, satorux1 wrote:
> > > likewise, you might want to remove the parameter.
> > 
> > Call for Unmount here does not require error handling, however, call for
> Unmount
> > in DiskMountManagerImpl::UnmountDeviceRecursive requires error handling.
> > 
> > Besides Unmount, some of CrosDisksClient's methods do not require error
> handling
> > at all (EnumerateAutoMountableDevices and GetDeviceProperties) and Mount
does
> > not require handling of success case as you say.
> > But, it is highly context dependent if a method call requires success/error
> > handling or not, like the case of Unmount.
> > I prefer simple success-and-error-handler-pair pattern rather than having
> > mixture of success-handler-only, error-handler-only and
> > success-and-error-handlers method signatures.
> > 
> > I think we should do some refactoring at first to eliminate ugly error
> handling
> > like appending "!" to indicate failure.
> > After the refactoring is done, we can make the error handling paths simpler
> and
> > remove the unused CrosDisksClient's callback arguments so that we no longer
> need
> > to have DoNothing.
> 
> Thank you for the explanation. Your comment here looks valuable. Would be nice
> to put this comment in the code.

Done.

[commit-bot: I haz the power, 2011-11-15 04:06:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hashimoto@chromium.org/8499007/29003

[commit-bot: I haz the power, 2011-11-15 05:20:20]

Change committed as 110045