Adding support for mount point different from removable devices to MountLibrary

Adding support for mount point different from removable devices to MountLibrary
(work in progress..have to add GetMountPoints support)

tested on ChromeOS with these three patches: http://gerrit.chromium.org/gerrit/#change,4447
http://gerrit.chromium.org/gerrit/#change,4449
http://gerrit.chromium.org/gerrit/#change,4544

BUG=chromium-os:17673, chromium-os:17783
TEST=Made sure mounting devices still works and that 
     MountCompleted event gets through to 
     file_browser_event_router (checking ui and chrome logs
     for mount completed entries)
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=93754

[tbarzic, 2011-07-21 23:49:23]

http://codereview.chromium.org/7457001/diff/12001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/mount_library.cc (right):

http://codereview.chromium.org/7457001/diff/12001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.cc:283: LOG(WARNING)
<<"MountCompleted fired" <<error_code<<source_path<<mount_path;
this (and other not indented LOG's) will get removed before landing.

[zel, 2011-07-22 00:48:37]

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

http://codereview.chromium.org/7457001/diff/12001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:292:
LOG(WARNING) << "MountCompleted" << error_code << source_path;
space

http://codereview.chromium.org/7457001/diff/12001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_event_names.cc (right):

http://codereview.chromium.org/7457001/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_names.cc:29:
"fileBrowserPrivate.onMountCompleted";
where are extension_api.json changes for this event at?

[tbarzic, 2011-07-22 00:57:58]

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

http://codereview.chromium.org/7457001/diff/12001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:292:
LOG(WARNING) << "MountCompleted" << error_code << source_path;
On 2011/07/22 00:48:37, zel wrote:
> space

I will remove this line

http://codereview.chromium.org/7457001/diff/12001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_event_names.cc (right):

http://codereview.chromium.org/7457001/diff/12001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_event_names.cc:29:
"fileBrowserPrivate.onMountCompleted";
On 2011/07/22 00:48:37, zel wrote:
> where are extension_api.json changes for this event at?

added them

[tbarzic, 2011-07-22 01:00:07]

http://codereview.chromium.org/7457001/diff/18002/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/mount_library.h (right):

http://codereview.chromium.org/7457001/diff/18002/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.h:123: virtual const MountPoints&
mount_points() const = 0;
MountPointSet

[tbarzic, 2011-07-22 03:13:23]

http://codereview.chromium.org/7457001/diff/21003/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/mount_library.cc (right):

http://codereview.chromium.org/7457001/diff/21003/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.cc:189: LOG(WARNING)
<<"MountCompleted" <<error_code<<source_path<<mount_path;
to be removed before landing

http://codereview.chromium.org/7457001/diff/21003/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.cc:284: LOG(WARNING)
<<"MountCompleted fired" <<error_code<<source_path<<mount_path;
to be removed before landing

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

http://codereview.chromium.org/7457001/diff/21003/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:297:
LOG(WARNING) << "MountCompleted" << error_code << source_path;
to be removed before landing.

[zel, 2011-07-22 04:47:17]

http://codereview.chromium.org/7457001/diff/21003/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/mount_library.cc (right):

http://codereview.chromium.org/7457001/diff/21003/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.cc:288:
mount_points_.insert(mount_path);
see my comments about this data not being enough, you should save source_path,
type and mount_path here

http://codereview.chromium.org/7457001/diff/21003/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/mount_library.h (right):

http://codereview.chromium.org/7457001/diff/21003/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.h:101: typedef std::set<std::string>
MountPointSet;
this does not have enough information, in addition to mountPath (i hope this is
what string is here for) you should add

mountType
source

http://codereview.chromium.org/7457001/diff/21003/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7457001/diff/21003/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5263: "type": "string",
add:

"optional": true,

http://codereview.chromium.org/7457001/diff/21003/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5268: "enum": ["device", "file",
"network", "invalid"],
what's 'invalid'?

http://codereview.chromium.org/7457001/diff/21003/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5269: "description":"Type of the
mount."
space after :

[tbarzic, 2011-07-22 06:21:51]

http://codereview.chromium.org/7457001/diff/21003/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/mount_library.cc (right):

http://codereview.chromium.org/7457001/diff/21003/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.cc:288:
mount_points_.insert(mount_path);
On 2011/07/22 04:47:17, zel wrote:
> see my comments about this data not being enough, you should save source_path,
> type and mount_path here

Done.

http://codereview.chromium.org/7457001/diff/21003/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/mount_library.h (right):

http://codereview.chromium.org/7457001/diff/21003/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.h:101: typedef std::set<std::string>
MountPointSet;
On 2011/07/22 04:47:17, zel wrote:
> this does not have enough information, in addition to mountPath (i hope this
is
> what string is here for) you should add
> 
> mountType
> source

I know, I had in plan to add these (well, at least source) ;)

Done.

http://codereview.chromium.org/7457001/diff/21003/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7457001/diff/21003/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5263: "type": "string",
On 2011/07/22 04:47:17, zel wrote:
> add:
> 
> "optional": true,

Done.

http://codereview.chromium.org/7457001/diff/21003/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5268: "enum": ["device", "file",
"network", "invalid"],
On 2011/07/22 04:47:17, zel wrote:
> what's 'invalid'?

Type not set. But when I think about it, it should not be returned from mount
method, so it's better to remove it from here.

http://codereview.chromium.org/7457001/diff/21003/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5269: "description":"Type of the
mount."
On 2011/07/22 04:47:17, zel wrote:
> space after :

Done.

http://codereview.chromium.org/7457001/diff/21007/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/mount_library.cc (right):

http://codereview.chromium.org/7457001/diff/21007/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.cc:495: default: {
Will remove { }

[zel, 2011-07-22 06:56:34]

http://codereview.chromium.org/7457001/diff/19016/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/mount_library.cc (right):

http://codereview.chromium.org/7457001/diff/19016/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.cc:285:
mount_points_.insert(std::pair<std::string, MountPointInfo>(mount_path,
nit: std::pair<std::string, MountPointInfo>  can be replaced with
MountPointMap::value_type.... slightly more readable IMO

http://codereview.chromium.org/7457001/diff/19016/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.cc:305: for (MountPointMap::iterator
it = mount_points_.begin();
this makes no sense, you have std::map for which you never use find(), instead
you drill down into all elements by source_path. why don't you simply do this:

1. MountPointInfo should have source, mount_point and type
2. MountPointMap should be <string(source_path), MountPointInfo> so you actually
get something out of the map here

[tbarzic, 2011-07-22 08:01:29]

http://codereview.chromium.org/7457001/diff/19016/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/mount_library.cc (right):

http://codereview.chromium.org/7457001/diff/19016/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.cc:285:
mount_points_.insert(std::pair<std::string, MountPointInfo>(mount_path,
On 2011/07/22 06:56:34, zel wrote:
> nit: std::pair<std::string, MountPointInfo>  can be replaced with
> MountPointMap::value_type.... slightly more readable IMO

Done.

http://codereview.chromium.org/7457001/diff/19016/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.cc:305: for (MountPointMap::iterator
it = mount_points_.begin();
On 2011/07/22 06:56:34, zel wrote:
> this makes no sense, you have std::map for which you never use find(), instead
> you drill down into all elements by source_path. why don't you simply do this:
> 
> 1. MountPointInfo should have source, mount_point and type
> 2. MountPointMap should be <string(source_path), MountPointInfo> so you
actually
> get something out of the map here

It would make more sense if we were using IsMountPoint in fileBrowser, but from
Oleg's CL it seems that is not the case...

http://codereview.chromium.org/7457001/diff/23020/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.cc:309: mount_points_.erase(it);
this should go into if bellow

[Oleg Eterevsky, 2011-07-22 13:34:52]

http://codereview.chromium.org/7457001/diff/23020/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/mount_library.cc (right):

http://codereview.chromium.org/7457001/diff/23020/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.cc:304: void OnUnmountPath(const
char* source_path,
Can we throw an event here also? Probably the same event as when mount is
completed, but with another type.

http://codereview.chromium.org/7457001/diff/23020/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/mount_library.h (right):

http://codereview.chromium.org/7457001/diff/23020/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.h:101: struct MountPointInfo {
Maybe add something like device_path to this structure to reference the file or
device that was mounted?

http://codereview.chromium.org/7457001/diff/23020/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.h:103: MountType mount_type;
Where do you declare MountType? Probably you could just move the declaration of
it and two functions from my CL.

[tbarzic, 2011-07-22 17:21:29]

http://codereview.chromium.org/7457001/diff/23020/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/mount_library.cc (right):

http://codereview.chromium.org/7457001/diff/23020/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.cc:304: void OnUnmountPath(const
char* source_path,
On 2011/07/22 13:34:53, Oleg wrote:
> Can we throw an event here also? Probably the same event as when mount is
> completed, but with another type.

Done.

http://codereview.chromium.org/7457001/diff/23020/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.cc:309:
mount_points_.erase(source_path);
On 2011/07/22 08:01:29, toni wrote:
> this should go into if bellow 

Done.

http://codereview.chromium.org/7457001/diff/23020/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/mount_library.h (right):

http://codereview.chromium.org/7457001/diff/23020/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.h:101: struct MountPointInfo {
On 2011/07/22 13:34:53, Oleg wrote:
> Maybe add something like device_path to this structure to reference the file
or
> device that was mounted?

Done.

http://codereview.chromium.org/7457001/diff/23020/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.h:103: MountType mount_type;
On 2011/07/22 13:34:53, Oleg wrote:
> Where do you declare MountType? Probably you could just move the declaration
of
> it and two functions from my CL.

It's in libcros...
I'll add MountTypeToString and MountTypeFromString to MountLib...

[zel, 2011-07-22 21:12:22]

http://codereview.chromium.org/7457001/diff/19024/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7457001/diff/19024/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5175: "id": "MountType",
remove this type, see below

http://codereview.chromium.org/7457001/diff/19024/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5258: "eventType": {
let's make eventType being just 'mount', 'unmount' and add
another field called 'status' that will have success/error values as:


  success, 
  error_unknown,
  error_internal,
  ...

and make sure you prefix all error conditions with error_*

http://codereview.chromium.org/7457001/diff/19024/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5275: "$ref": "MountType",
make this string and put that enum here as :

"type": "string",
"enum": ["device", "file", "network"],

[tbarzic, 2011-07-22 22:04:47]

http://codereview.chromium.org/7457001/diff/19024/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7457001/diff/19024/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5175: "id": "MountType",
On 2011/07/22 21:12:23, zel wrote:
> remove this type, see below

Done.

http://codereview.chromium.org/7457001/diff/19024/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5258: "eventType": {
On 2011/07/22 21:12:23, zel wrote:
> let's make eventType being just 'mount', 'unmount' and add
> another field called 'status' that will have success/error values as:
> 
> 
>   success, 
>   error_unknown,
>   error_internal,
>   ...
> 
> and make sure you prefix all error conditions with error_*

Done.

http://codereview.chromium.org/7457001/diff/19024/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5275: "$ref": "MountType",
On 2011/07/22 21:12:23, zel wrote:
> make this string and put that enum here as :
> 
> "type": "string",
> "enum": ["device", "file", "network"],

Done.

[zel, 2011-07-22 22:09:38]

almost there

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

http://codereview.chromium.org/7457001/diff/20038/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:63: return
"unknown";
prefix error events with error_*

http://codereview.chromium.org/7457001/diff/20038/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7457001/diff/20038/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5258: "enum": ["success",
"unknown", "internal", "unknown_filesystem",
please prefix all error enum with error_*

[tbarzic, 2011-07-22 22:15:14]

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

http://codereview.chromium.org/7457001/diff/20038/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:63: return
"unknown";
On 2011/07/22 22:09:38, zel wrote:
> prefix error events with error_*

Done.

http://codereview.chromium.org/7457001/diff/20038/chrome/common/extensions/ap...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/7457001/diff/20038/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5258: "enum": ["success",
"unknown", "internal", "unknown_filesystem",
On 2011/07/22 22:09:38, zel wrote:
> please prefix all error enum with error_*

Done.

[zel, 2011-07-22 22:24:06]

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

http://codereview.chromium.org/7457001/diff/24028/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:282:
mount_info->SetString("eventType", "mounting");
this does not match extension_api.json values

http://codereview.chromium.org/7457001/diff/24028/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:284:
mount_info->SetString("eventType", "unmounting");
same here

[tbarzic, 2011-07-22 22:27:41]

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

http://codereview.chromium.org/7457001/diff/24028/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:282:
mount_info->SetString("eventType", "mounting");
On 2011/07/22 22:24:06, zel wrote:
> this does not match extension_api.json values

Done.

http://codereview.chromium.org/7457001/diff/24028/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:284:
mount_info->SetString("eventType", "unmounting");
On 2011/07/22 22:24:06, zel wrote:
> same here

Done.

[zel, 2011-07-22 22:31:56]

LGTM

[tbarzic, 2011-07-22 23:37:17]

Merged Oleg's change into this one, so could you have another look..

[zel, 2011-07-22 23:50:42]

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

http://codereview.chromium.org/7457001/diff/17063/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:1092:
NOTREACHED();
change NOTREACHED() in this function to this instead:

return false;

http://codereview.chromium.org/7457001/diff/17063/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:1134:
SendResponse(true);
If this class derives from SyncExtensionFunction clase, remove SendResponse(),
because SyncExtensionFunction base class should call this method after RunImpl()

http://codereview.chromium.org/7457001/diff/17063/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:1151:
NOTREACHED();
replace NOTREACHED(); with
return false;

http://codereview.chromium.org/7457001/diff/17063/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:1170: if
(args_->GetSize() != 0) {
nit: {}

http://codereview.chromium.org/7457001/diff/17063/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:1187:
base::DictionaryValue *mount_info = new base::DictionaryValue();
make a helper function MountPointInfoToValue() for this chunk of code. it will
make the code more readable and we might need to do that on other places.

http://codereview.chromium.org/7457001/diff/17063/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:1271:
MountVolumeFunction::MountVolumeFunction() {
remove this one, it's commented out anyway

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

http://codereview.chromium.org/7457001/diff/17063/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:282: /*
remove this one, it's commented out anyway

[zel, 2011-07-22 23:51:22]

http://codereview.chromium.org/7457001/diff/17063/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_function_dispatcher.cc (right):

http://codereview.chromium.org/7457001/diff/17063/chrome/browser/extensions/e...
chrome/browser/extensions/extension_function_dispatcher.cc:351:
RegisterFunction<RemoveMountFunction>();
GetMountPointsFunction is missing

[zel, 2011-07-22 23:54:20]

http://codereview.chromium.org/7457001/diff/22049/chrome/browser/chromeos/ext...
File chrome/browser/chromeos/extensions/file_browser_event_router.h (right):

http://codereview.chromium.org/7457001/diff/22049/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.h:51: const
std::string& source_path,
pass MountPointInfo here instead of these 3 params

http://codereview.chromium.org/7457001/diff/22049/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.h:113: const
std::string& source_path,
pass MountPointInfo here instead of these 3 params

[rniwa-cr, 2011-07-23 04:18:38]

It appears that this broke InstallLocalized on Chromium OS and Linux:
http://build.chromium.org/p/chromium/builders/Linux%20Tests%20%28dbg%29%28sha...
http://build.chromium.org/p/chromium/builders/Linux%20Tests%20%28Views%20dbg%...

[rniwa-cr, 2011-07-23 04:42:20]

Sorry, I had to roll it out in
http://src.chromium.org/viewvc/chrome?view=rev&revision=93775 as there were
nobody else to take care of the tree.