Switch from MountLibrary to CrosDisksLibrary

Switch from MountLibrary to DiskMountManager and CrosDisksLibrary

This patch is 2nd part of 3 splitted patches made from http://codereview.chromium.org/8386031


BUG=chromium-os:16556
TEST=Confirm that removable disk is correctly mounted


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

[satorux1, 2011-11-14 17:58:26]

Please update this patch. :)

[hashimoto, 2011-11-15 12:39:25]

New patch set.

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

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:331: const std::string
file_path = it->second;
Using const std::string& is wrong because |it| is erased in the next line.

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:1191: // The
third argument is simply ignored.
MountPathOptions declared in chromeos_mount.h is no longer used by
DiskMountManager because cros-disks does not care about that parameter.

[tbarzic, 2011-11-15 19:52:38]

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:1191: // The
third argument is simply ignored.
On 2011/11/15 12:39:25, hashimoto wrote:
> MountPathOptions declared in chromeos_mount.h is no longer used by
> DiskMountManager because cros-disks does not care about that parameter.

it's not yet used (this will be needed for network drives)..
I plan to add support for these options to cros disks client in a week or two,
so I'd prefer if you left options here (but I have nothing against adding it
back later ;) )

[satorux1, 2011-11-15 21:20:23]

Looks good overall. Minor issues only.

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/chromeos/chr...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/chromeos/chr...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:176: // Initialize the
disk mount manager
a period is missing.

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

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:331: const std::string
file_path = it->second;
On 2011/11/15 12:39:25, hashimoto wrote:
> Using const std::string& is wrong because |it| is erased in the next line.

oops. that was tricky! please add a comment just in case someone else tries to
add & back.

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:645: DiskMountManager*
disk_mount_manager) {
you can move the parameter to the previous line.

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:99:
disk_mount_manager->RemoveObserver(this);
nit.

chromeos::disks::DiskMountManager::GetInstance()
    ->RemoveObserver(this);

Would be simpler.

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:360:
disk->device_path().c_str(),
Please remove .c_str()

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:373:
disk_mount_manager->UnmountPath(disk->mount_path().c_str());
ditto.

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:428:
device_path.c_str(),
ditto.

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:997: void*
context) {
can you change this: void* -> std::string

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:1064: const
FilePathList& files, void* context) {
context is unused. can you remove?

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:1191: // The
third argument is simply ignored.
On 2011/11/15 19:52:38, tbarzic wrote:
> On 2011/11/15 12:39:25, hashimoto wrote:
> > MountPathOptions declared in chromeos_mount.h is no longer used by
> > DiskMountManager because cros-disks does not care about that parameter.
> 
> it's not yet used (this will be needed for network drives)..
> I plan to add support for these options to cros disks client in a week or two,
> so I'd prefer if you left options here (but I have nothing against adding it
> back later ;) )

Let's add it back when needed. :) Hopefully this change will be checked this in
the by the end of this week.

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:1212:
std::string* context = new std::string(mount_type_str);
This void* context hack is ugly. I think you can remove it now.

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:1216: context));
can you pass mount_type_str directly?

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:1223: const
FilePathList& files, void* context) {
can you change this: void* -> const std::string&

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_apitest.cc
(right):

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_apitest.cc:124:
volumes_.clear();
STLDeleteValues() in base/stl_util.h can be used to replace the function.

[hashimoto, 2011-11-16 04:28:14]

New patch set.

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/chromeos/chr...
File chrome/browser/chromeos/chrome_browser_main_chromeos.cc (right):

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/chromeos/chr...
chrome/browser/chromeos/chrome_browser_main_chromeos.cc:176: // Initialize the
disk mount manager
On 2011/11/15 21:20:24, satorux1 wrote:
> a period is missing.

Done.

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

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:331: const std::string
file_path = it->second;
On 2011/11/15 21:20:24, satorux1 wrote:
> On 2011/11/15 12:39:25, hashimoto wrote:
> > Using const std::string& is wrong because |it| is erased in the next line.
> 
> oops. that was tricky! please add a comment just in case someone else tries to
> add & back.

Done.

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:645: DiskMountManager*
disk_mount_manager) {
On 2011/11/15 21:20:24, satorux1 wrote:
> you can move the parameter to the previous line.

Done.
But the line is too long to make the left brace fit into it.

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:99:
disk_mount_manager->RemoveObserver(this);
On 2011/11/15 21:20:24, satorux1 wrote:
> nit.
> 
> chromeos::disks::DiskMountManager::GetInstance()
>     ->RemoveObserver(this);
> 
> Would be simpler.

Done.

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:360:
disk->device_path().c_str(),
On 2011/11/15 21:20:24, satorux1 wrote:
> Please remove .c_str()

Done.

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:373:
disk_mount_manager->UnmountPath(disk->mount_path().c_str());
On 2011/11/15 21:20:24, satorux1 wrote:
> ditto.

Done.

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:428:
device_path.c_str(),
On 2011/11/15 21:20:24, satorux1 wrote:
> ditto.

Done.

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:997: void*
context) {
On 2011/11/15 21:20:24, satorux1 wrote:
> can you change this: void* -> std::string

Instead of replacing void* with std::string, I introduced callbacks to make it
easy to add parameters when MountPath gets another argument as tbarzic says.

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:1064: const
FilePathList& files, void* context) {
On 2011/11/15 21:20:24, satorux1 wrote:
> context is unused. can you remove?

Done.

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:1212:
std::string* context = new std::string(mount_type_str);
On 2011/11/15 21:20:24, satorux1 wrote:
> This void* context hack is ugly. I think you can remove it now.

Done.

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:1216: context));
On 2011/11/15 21:20:24, satorux1 wrote:
> can you pass mount_type_str directly?

Done.

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:1223: const
FilePathList& files, void* context) {
On 2011/11/15 21:20:24, satorux1 wrote:
> can you change this: void* -> const std::string&

Done.

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_apitest.cc
(right):

http://codereview.chromium.org/8497007/diff/10004/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_apitest.cc:124:
volumes_.clear();
On 2011/11/15 21:20:24, satorux1 wrote:
> STLDeleteValues() in base/stl_util.h can be used to replace the function.

Done.

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:56: 
Introduced anonymous namespace to make locally used variables have internal
linkage.

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:307: class
RequestLocalFileSystemFunction::LocalFileSystemCallbackDispatcher
LocalFileSystemCallbackDispatcher and ExecuteTasksFileSystemCallbackDispatcher
became internal classes to avoid contaminating the global namespace.

[satorux1, 2011-11-16 06:46:06]

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

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.cc:479: typedef
std::map<std::string, dbus::MessageReader*> properties_t;
Type name should look like PropertiesMap

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

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:646: ) {
Hmm, this looks uglier than before.. Please change it back.

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:56: 
On 2011/11/16 04:28:15, hashimoto wrote:
> Introduced anonymous namespace to make locally used variables have internal
> linkage.

+1

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:307: class
RequestLocalFileSystemFunction::LocalFileSystemCallbackDispatcher
On 2011/11/16 04:28:15, hashimoto wrote:
> LocalFileSystemCallbackDispatcher and ExecuteTasksFileSystemCallbackDispatcher
> became internal classes to avoid contaminating the global namespace.

+1

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:996:
GetLocalPathsCallback callback) {
This function does something a bit tricky. Would be nice to give a bit more
descriptive name, like GetLocalPathsOnFileThreadAndRunCallbackOnUIThread().

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:1001: this,
Did you mean FileBrowserFunction::GetLocalPathsOnFileThread ?

I'm confused. Why does this compile? |this| is a  FileBrowserFunction object,
but it's bound with GetLocalPathsOnFileThread() function in SelectFileFunction,
which is a sub class of  FileBrowserFunction? If this compiles, this may be a
bug in base::Bind()?

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.h (right):

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:136: // Convert
virtual paths to local paths
period is missing. looking at the implementation, this function does something a
bit tricky. Please describe a bit more details about what the function does.

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:143: void
GetLocalPathsOnFileThread(const UrlList& file_urls,
function comment is missing.

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:160: void
GetLocalPathsResponseOnUIThread(const FilePathList& files);
ditto.

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:178: void
GetLocalPathsResponseOnUIThread(const std::string& internal_task_id,
ditto.

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:197: void
GetLocalPathsResponseOnUIThread(const FilePathList& files);
ditto

[hashimoto, 2011-11-16 07:45:32]

New patch set

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

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/chromeos/dbu...
chrome/browser/chromeos/dbus/cros_disks_client.cc:479: typedef
std::map<std::string, dbus::MessageReader*> properties_t;
On 2011/11/16 06:46:06, satorux1 wrote:
> Type name should look like PropertiesMap

Done.

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

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/chromeos/dis...
chrome/browser/chromeos/disks/disk_mount_manager.cc:646: ) {
On 2011/11/16 06:46:06, satorux1 wrote:
> Hmm, this looks uglier than before.. Please change it back.

Done.

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:996:
GetLocalPathsCallback callback) {
On 2011/11/16 06:46:06, satorux1 wrote:
> This function does something a bit tricky. Would be nice to give a bit more
> descriptive name, like GetLocalPathsOnFileThreadAndRunCallbackOnUIThread().
> 

Done.

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:1001: this,
On 2011/11/16 06:46:06, satorux1 wrote:
> Did you mean FileBrowserFunction::GetLocalPathsOnFileThread ?
> 
> I'm confused. Why does this compile? |this| is a  FileBrowserFunction object,
> but it's bound with GetLocalPathsOnFileThread() function in
SelectFileFunction,
> which is a sub class of  FileBrowserFunction? If this compiles, this may be a
> bug in base::Bind()?

Thank you for finding this mistake, fixed.

BTW, this line seems to compile because
SelectFileFunction::GetLocalPathsOnFileThread is resolved to
FileBrowserFunction::GetLocalPathsOnFileThread by the compiler and hence its
type is void (FileBrowser::Function::*)(const UrlList&, GetLocalPathsCallback).
I have no knowledge about if this behavior is defined by the C++ standard, but
gcc seems to work in this way. (http://ideone.com/Lp5vj)

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.h (right):

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:136: // Convert
virtual paths to local paths
On 2011/11/16 06:46:06, satorux1 wrote:
> period is missing. looking at the implementation, this function does something
a
> bit tricky. Please describe a bit more details about what the function does.

Done.

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:143: void
GetLocalPathsOnFileThread(const UrlList& file_urls,
On 2011/11/16 06:46:06, satorux1 wrote:
> function comment is missing.

Done.

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:160: void
GetLocalPathsResponseOnUIThread(const FilePathList& files);
On 2011/11/16 06:46:06, satorux1 wrote:
> ditto.

Done.

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:178: void
GetLocalPathsResponseOnUIThread(const std::string& internal_task_id,
On 2011/11/16 06:46:06, satorux1 wrote:
> ditto.

Done.

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:197: void
GetLocalPathsResponseOnUIThread(const FilePathList& files);
On 2011/11/16 06:46:06, satorux1 wrote:
> ditto

Done.

[satorux1, 2011-11-16 18:01:09]

LGTM

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/8497007/diff/10009/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:1001: this,
On 2011/11/16 07:45:32, hashimoto wrote:
> On 2011/11/16 06:46:06, satorux1 wrote:
> > Did you mean FileBrowserFunction::GetLocalPathsOnFileThread ?
> > 
> > I'm confused. Why does this compile? |this| is a  FileBrowserFunction
object,
> > but it's bound with GetLocalPathsOnFileThread() function in
> SelectFileFunction,
> > which is a sub class of  FileBrowserFunction? If this compiles, this may be
a
> > bug in base::Bind()?
> 
> Thank you for finding this mistake, fixed.
> 
> BTW, this line seems to compile because
> SelectFileFunction::GetLocalPathsOnFileThread is resolved to
> FileBrowserFunction::GetLocalPathsOnFileThread by the compiler and hence its
> type is void (FileBrowser::Function::*)(const UrlList&,
GetLocalPathsCallback).
> I have no knowledge about if this behavior is defined by the C++ standard, but
> gcc seems to work in this way. (http://ideone.com/Lp5vj)

Heh, interesting. Learned something new...

[commit-bot: I haz the power, 2011-11-17 06:51:39]

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

[commit-bot: I haz the power, 2011-11-17 07:49:57]

Change committed as 110469