Replace mountType by volumeType on fileBrowserPrivate APIs.

Replace mountType by volumeType on fileBrowserPrivate APIs.

This is API clean up. The mountType is being replaced by VolumeType. This CL
refactors private APIs about the flag.
Note that this is preparation to return Drive as a Volume.

BUG=279276
TEST=Ran unit_tests, browser_tests --gtest_filter="*FileSystemExtensionApiTest*:*FileManagerBrowserTest*:*FileBrowserPrivateApiTest*" and tested manually.
R=benwells@chromium.org, kinaba@chromium.org, satorux@chromium.org, yoshiki@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221978

[hidehiko, 2013-09-03 18:53:04]

Kazuhiro, could you review c/b/chromeos/extensions/file_manager/...?
Yoshiki, could you review c/b/resources/file_manager/js/volume_manager.js?
Ben, could you review file_browser_private.js and test.js?
Satoru, could you review chromeos/...?

Thank you for your review in advance,
- hidehiko

[satorux1, 2013-09-04 05:01:55]

https://codereview.chromium.org/23658009/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_manager/private_api_util.h (right):

https://codereview.chromium.org/23658009/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/private_api_util.h:30:
std::string VolumeTypeToString(VolumeType type);
Move this to volume_manager.h where VolumeType is defined? We don't want to 
dump even more random things in private_api_util.h. :P

[yoshiki, 2013-09-04 05:09:22]

volume_manager.js lgtm.

[hidehiko, 2013-09-04 05:13:24]

Thank you for your review. PTAL?

https://codereview.chromium.org/23658009/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_manager/private_api_util.h (right):

https://codereview.chromium.org/23658009/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/private_api_util.h:30:
std::string VolumeTypeToString(VolumeType type);
On 2013/09/04 05:01:55, satorux1 wrote:
> Move this to volume_manager.h where VolumeType is defined? We don't want to 
> dump even more random things in private_api_util.h. :P

This is rather mapping from VolumeType to "string represention of enum in
private API" than just a stringify the enum, so IMHO this is a part of private
API rather than VolumeManager, which will be split from private API eventually.
So, I'd like to keep this here.
Note that the name may be a bit confusing, but I followed the current API
implementation manner...

[satorux1, 2013-09-05 01:46:24]

LGTM with a request.

https://codereview.chromium.org/23658009/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_manager/private_api_util.h (right):

https://codereview.chromium.org/23658009/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/private_api_util.h:30:
std::string VolumeTypeToString(VolumeType type);
On 2013/09/04 05:13:25, hidehiko wrote:
> On 2013/09/04 05:01:55, satorux1 wrote:
> > Move this to volume_manager.h where VolumeType is defined? We don't want to 
> > dump even more random things in private_api_util.h. :P
> 
> This is rather mapping from VolumeType to "string represention of enum in
> private API" than just a stringify the enum, so IMHO this is a part of private
> API rather than VolumeManager, which will be split from private API
eventually.
> So, I'd like to keep this here.
> Note that the name may be a bit confusing, but I followed the current API
> implementation manner...

Oh I see. Then let's clarify by the function comment:

// Converts VolumeType to a string enum (XxxType) used in the private API.

Maybe also rename the function:

VolumeTypeToStringEnum()

[hidehiko, 2013-09-05 03:12:40]

Thank you for your review!

Kazuhiro, Ben, could you kindly review, too?

https://codereview.chromium.org/23658009/diff/1/chrome/browser/chromeos/exten...
File chrome/browser/chromeos/extensions/file_manager/private_api_util.h (right):

https://codereview.chromium.org/23658009/diff/1/chrome/browser/chromeos/exten...
chrome/browser/chromeos/extensions/file_manager/private_api_util.h:30:
std::string VolumeTypeToString(VolumeType type);
On 2013/09/05 01:46:24, satorux1 wrote:
> On 2013/09/04 05:13:25, hidehiko wrote:
> > On 2013/09/04 05:01:55, satorux1 wrote:
> > > Move this to volume_manager.h where VolumeType is defined? We don't want
to 
> > > dump even more random things in private_api_util.h. :P
> > 
> > This is rather mapping from VolumeType to "string represention of enum in
> > private API" than just a stringify the enum, so IMHO this is a part of
private
> > API rather than VolumeManager, which will be split from private API
> eventually.
> > So, I'd like to keep this here.
> > Note that the name may be a bit confusing, but I followed the current API
> > implementation manner...
> 
> Oh I see. Then let's clarify by the function comment:
> 
> // Converts VolumeType to a string enum (XxxType) used in the private API.
> 
> Maybe also rename the function:
> 
> VolumeTypeToStringEnum()

VolumeTypeToStringEnum sounds better. Done.

[kinaba, 2013-09-05 05:51:38]

lgtm

[benwells, 2013-09-06 01:26:28]

https://codereview.chromium.org/23658009/diff/12001/chrome/common/extensions/...
File chrome/common/extensions/api/file_browser_private.json (right):

https://codereview.chromium.org/23658009/diff/12001/chrome/common/extensions/...
chrome/common/extensions/api/file_browser_private.json:287: "enum": ["drive",
"downloads", "removable", "archive"],
Is it possible to extract this into a shared volume type definition? It is
repeated three times.

[satorux1, 2013-09-06 01:57:36]

https://codereview.chromium.org/23658009/diff/12001/chrome/common/extensions/...
File chrome/common/extensions/api/file_browser_private.json (right):

https://codereview.chromium.org/23658009/diff/12001/chrome/common/extensions/...
chrome/common/extensions/api/file_browser_private.json:287: "enum": ["drive",
"downloads", "removable", "archive"],
On 2013/09/06 01:26:28, benwells wrote:
> Is it possible to extract this into a shared volume type definition? It is
> repeated three times.

I think he plans to unify MountCompletedEvent, VolumeMetadata, and
MountPointInfo, so the repeated definitions will go away shortly.

However, I agree that it's nicer to share the enum for now. Maybe define
something like in the "types" section:

  {
    "id": "VolumeType",
    "type": "string",
    "enum": [ ...]
  }


Then use "$ref": "VolumeType" instead of "type": "string"

[hidehiko, 2013-09-06 03:50:39]

Thank you for your review. PTAL?

https://codereview.chromium.org/23658009/diff/12001/chrome/common/extensions/...
File chrome/common/extensions/api/file_browser_private.json (right):

https://codereview.chromium.org/23658009/diff/12001/chrome/common/extensions/...
chrome/common/extensions/api/file_browser_private.json:287: "enum": ["drive",
"downloads", "removable", "archive"],
On 2013/09/06 01:57:37, satorux1 wrote:
> On 2013/09/06 01:26:28, benwells wrote:
> > Is it possible to extract this into a shared volume type definition? It is
> > repeated three times.
> 
> I think he plans to unify MountCompletedEvent, VolumeMetadata, and
> MountPointInfo, so the repeated definitions will go away shortly.
> 
> However, I agree that it's nicer to share the enum for now. Maybe define
> something like in the "types" section:
> 
>   {
>     "id": "VolumeType",
>     "type": "string",
>     "enum": [ ...]
>   }
> 
> 
> Then use "$ref": "VolumeType" instead of "type": "string"

Thank you for your comment.
Sharing sounds good to me, too.
I tried, but the code generator has a C++ ADL related bug, so it generates
un-compilable program. Considering the two of three will gone soon, I'd like to
keep this style for now. WDYT?

[satorux1, 2013-09-06 04:17:58]

LGTM

https://codereview.chromium.org/23658009/diff/12001/chrome/common/extensions/...
File chrome/common/extensions/api/file_browser_private.json (right):

https://codereview.chromium.org/23658009/diff/12001/chrome/common/extensions/...
chrome/common/extensions/api/file_browser_private.json:287: "enum": ["drive",
"downloads", "removable", "archive"],
On 2013/09/06 03:50:39, hidehiko wrote:
> On 2013/09/06 01:57:37, satorux1 wrote:
> > On 2013/09/06 01:26:28, benwells wrote:
> > > Is it possible to extract this into a shared volume type definition? It is
> > > repeated three times.
> > 
> > I think he plans to unify MountCompletedEvent, VolumeMetadata, and
> > MountPointInfo, so the repeated definitions will go away shortly.
> > 
> > However, I agree that it's nicer to share the enum for now. Maybe define
> > something like in the "types" section:
> > 
> >   {
> >     "id": "VolumeType",
> >     "type": "string",
> >     "enum": [ ...]
> >   }
> > 
> > 
> > Then use "$ref": "VolumeType" instead of "type": "string"
> 
> Thank you for your comment.
> Sharing sounds good to me, too.
> I tried, but the code generator has a C++ ADL related bug, so it generates
> un-compilable program. Considering the two of three will gone soon, I'd like
to
> keep this style for now. WDYT?

Sounds good.

[benwells, 2013-09-06 23:03:52]

lgtm

[commit-bot: I haz the power, 2013-09-08 16:22:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/23658009/23001

[commit-bot: I haz the power, 2013-09-08 16:22:25]

Failed to apply patch for
chrome/browser/chromeos/extensions/file_manager/event_router.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/extensions/file_manager/event_router.cc
  Hunk #1 FAILED at 16.
  1 out of 3 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/extensions/file_manager/event_router.cc.rej

Patch:       chrome/browser/chromeos/extensions/file_manager/event_router.cc
Index: chrome/browser/chromeos/extensions/file_manager/event_router.cc
diff --git a/chrome/browser/chromeos/extensions/file_manager/event_router.cc
b/chrome/browser/chromeos/extensions/file_manager/event_router.cc
index
5e024ccb323eab18ddf6cb87626bcf21a68adeca..b10e0e121bd913edaec0a4c61c68d3ea15d2c79d
100644
--- a/chrome/browser/chromeos/extensions/file_manager/event_router.cc
+++ b/chrome/browser/chromeos/extensions/file_manager/event_router.cc
@@ -16,6 +16,7 @@
 #include "chrome/browser/chromeos/drive/drive_integration_service.h"
 #include "chrome/browser/chromeos/drive/file_system_interface.h"
 #include "chrome/browser/chromeos/drive/file_system_util.h"
+#include "chrome/browser/chromeos/extensions/file_manager/private_api_util.h"
 #include "chrome/browser/chromeos/extensions/file_manager/volume_manager.h"
 #include "chrome/browser/chromeos/file_manager/app_id.h"
 #include "chrome/browser/chromeos/file_manager/desktop_notifications.h"
@@ -209,6 +210,23 @@ bool IsGooglePhotosInstalled(Profile *profile) {
   return false;
 }
 
+// This method is temporarily introduced to make a change step by step.
+// After mounting logic is moved to VolumeManager, this should be removed.
+std::string MountTypeToString(chromeos::MountType type) {
+  switch (type) {
+    case chromeos::MOUNT_TYPE_INVALID:
+      return "";
+    case chromeos::MOUNT_TYPE_DEVICE:
+      return
util::VolumeTypeToStringEnum(VOLUME_TYPE_REMOVABLE_DISK_PARTITION);
+    case chromeos::MOUNT_TYPE_ARCHIVE:
+      return util::VolumeTypeToStringEnum(VOLUME_TYPE_MOUNTED_ARCHIVE_FILE);
+    case chromeos::MOUNT_TYPE_GOOGLE_DRIVE:
+      return util::VolumeTypeToStringEnum(VOLUME_TYPE_GOOGLE_DRIVE);
+  }
+  NOTREACHED();
+  return "";
+}
+
 }  // namespace
 
 // Pass dummy value to JobInfo's constructor for make it default constructible.
@@ -690,8 +708,7 @@ void EventRouter::DispatchMountEvent(
       event == DiskMountManager::MOUNTING ? "mount" : "unmount");
   mount_info_value->SetString("status", MountErrorToString(error_code));
   mount_info_value->SetString(
-      "mountType",
-      DiskMountManager::MountTypeToString(mount_info.mount_type));
+      "volumeType", MountTypeToString(mount_info.mount_type));
 
   // Add sourcePath to the event.
   mount_info_value->SetString("sourcePath", mount_info.source_path);

[commit-bot: I haz the power, 2013-09-09 02:10:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hidehiko@chromium.org/23658009/30001

[hidehiko, 2013-09-09 06:22:04]

Committed patchset #5 manually as r221978 (presubmit successful).