Formatting feature initial commit for ChromeOS Tree

Formatting feature initial commit for ChromeOS Tree

Added formatting API for browser extension as well as event routing. 
Created a complete UI for formatting.

This code depends on the following changes to libcros:
http://gerrit.chromium.org/gerrit/#change,4446

BUG=chromium-os:4541, chromium-os:17071
TEST=Try to format removable media.


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

[sidor, 2011-07-21 00:24:10]

Hi,

Could you please review my code?

Thanks,
Szymon

[achuithb, 2011-07-21 00:58:21]

Some drive-by comments.

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

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:122:
CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
DCHECK?

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:139: std::string file_path = "";
Don't need = "", since the empty string is the default.
You can just do:
std::string device_path, file_path;

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:147: if (device_path.compare("")
== 0) {
Use device_path.empty()

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:155: if (it !=
to_be_formated_.end()) {
You can just do:
if (to_be_formated_.find(device_path_ != to_be_formated_.end())
You don't need the variable it.

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:162: // Formatting process
continues, after unmounting
Period at the end of comments please.

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:378: const std::string* file_path
= &(it->second);
I don't think this is safe. You should do
const std::string file_path = it->second;
It looks like you are deleting the string in the following line (unless I'm
missing something).

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:603: // to be unmounted
Period at the end of comments please.

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:604: PathMap to_be_formated_;
Maybe format_pending_?

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/extensi...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/extensi...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:371: FROM_HERE,
Is the indentation weird here?

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/extensi...
File chrome/browser/chromeos/extensions/file_browser_event_router.h (right):

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/extensi...
chrome/browser/chromeos/extensions/file_browser_event_router.h:93: void
OnDiskAdded(const chromeos::MountLibrary::Disk* disk);
These should all be virtual with OVERRIDE, I believe.

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/extensi...
chrome/browser/chromeos/extensions/file_browser_event_router.h:128: void
ShowFileBrowserNotification(const std::string& cathegory,
category

[sidor, 2011-07-21 18:24:15]

ALSO, THIS CODE DEPENDS ON CHROMEOS LIBCROS, SHOULD NOT BE COMMITED YET.

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

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:122:
CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
On 2011/07/21 00:58:21, achuith.bhandarkar wrote:
> DCHECK?

Done.

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:139: std::string file_path = "";
On 2011/07/21 00:58:21, achuith.bhandarkar wrote:
> Don't need = "", since the empty string is the default.
> You can just do:
> std::string device_path, file_path;

Done.

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:147: if (device_path.compare("")
== 0) {
On 2011/07/21 00:58:21, achuith.bhandarkar wrote:
> Use device_path.empty()

Done.

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:155: if (it !=
to_be_formated_.end()) {
On 2011/07/21 00:58:21, achuith.bhandarkar wrote:
> You can just do:
> if (to_be_formated_.find(device_path_ != to_be_formated_.end())
> You don't need the variable it.

Done.

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:162: // Formatting process
continues, after unmounting
On 2011/07/21 00:58:21, achuith.bhandarkar wrote:
> Period at the end of comments please.

Done.

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:378: const std::string* file_path
= &(it->second);
That's really weird. It intuitively it shouldn't work, but hmm after compilation
it works perfectly. Nevertheless I will change it according to what you wrote
just for the sanity sake.

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:603: // to be unmounted
On 2011/07/21 00:58:21, achuith.bhandarkar wrote:
> Period at the end of comments please.

Done.

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:604: PathMap to_be_formated_;
That doesn't sound correct. This is map holding the devices that are currently
being unmounted and will for which formatting process will start after removing
them from this map.

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/extensi...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/extensi...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:371: FROM_HERE,
Yup, I changed it, I added newline between this and 4000 to emphasis that these
belong to two different functions.

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/extensi...
File chrome/browser/chromeos/extensions/file_browser_event_router.h (right):

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/extensi...
chrome/browser/chromeos/extensions/file_browser_event_router.h:93: void
OnDiskAdded(const chromeos::MountLibrary::Disk* disk);
I didn't write this piece of code, I just added the functionality, I would
prefer not to change it because I am not sure about consequences.

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/extensi...
chrome/browser/chromeos/extensions/file_browser_event_router.h:128: void
ShowFileBrowserNotification(const std::string& cathegory,
On 2011/07/21 00:58:21, achuith.bhandarkar wrote:
> category

Done.

[sidor, 2011-07-21 19:12:33]

I added the dependency now it should be OK to commit it once it's ready.

[achuithb, 2011-07-21 22:02:32]

You should write some browser tests for the new extension functionality.

Also some unit tests for the mount library code.

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

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/cros/mo...
chrome/browser/chromeos/cros/mount_library.cc:604: PathMap to_be_formated_;
Isn't that what format_pending_ means? Format hasn't started yet?

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/extensi...
File chrome/browser/chromeos/extensions/file_browser_event_router.h (right):

http://codereview.chromium.org/7471024/diff/1/chrome/browser/chromeos/extensi...
chrome/browser/chromeos/extensions/file_browser_event_router.h:93: void
OnDiskAdded(const chromeos::MountLibrary::Disk* disk);
https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thr...

The suggestion is to modify all code to use OVERRIDE as appropriate as you come
across it. Your new functions should definitely have OVERRIDE, and the others
should too.

[achuithb, 2011-07-21 22:05:05]

Toni, could you please review the mount library code and also guide sidor on how
to write tests for the library?

[tbarzic, 2011-07-25 20:43:04]

http://codereview.chromium.org/7471024/diff/6001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/mount_library.cc (right):

http://codereview.chromium.org/7471024/diff/6001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/mount_library.cc:121: virtual void
FormatUnmountedDevice(const char* device_path) OVERRIDE {
Argument name should match name in MountLibrary.h

http://codereview.chromium.org/7471024/diff/6001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/mount_library.cc:129: }
If you leave this exposed through MountLibrary, checking that device (or some of
its children)is not mounted would probably be a good idea.

http://codereview.chromium.org/7471024/diff/6001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/mount_library.cc:137: DCHECK(device_mount_path);
Would it be possible to pass device_path to this method instead of mount_path?

http://codereview.chromium.org/7471024/diff/6001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/mount_library.cc:153: if
(to_be_formated_.find(device_path) != to_be_formated_.end()) {
I agree with achuith that format_pending_ would be a better naming

http://codereview.chromium.org/7471024/diff/6001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/mount_library.cc:396:
FireDeviceStatusUpdate(MOUNT_FORMATTING_STARTED,
Have you considered adding MOUNT_FORMATTING_FAILED event?
Or maybe add flag success to MOUNT_FORMATTING_FINISHED.
It is much cleaner than adding "!" in front of device_path.

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

http://codereview.chromium.org/7471024/diff/6001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/mount_library.h:124: // Formats device given its
file path, eg. /dev/*
dot at the end of the comment.

http://codereview.chromium.org/7471024/diff/6001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/mount_library.h:126: // Formats Device given its
mount path
I don't see that you are using FormatUnmountDevice outside MountLibrary, so it
would be a good idea to move it from the MountLibrary interface to
MountLibraryImpl. 
if you plan to use it in a future CL you can always move it back here.

http://codereview.chromium.org/7471024/diff/6001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/mount_library.h:127: virtual void
FormatMountedDevice(const char* device_mount_path) = 0;
I'd rename device_mount_path to mount_path.

http://codereview.chromium.org/7471024/diff/6001/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/7471024/diff/6001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:1374:
if(!confirm(str('FORMATTING_WARNING'))) return;
return should go to new line

http://codereview.chromium.org/7471024/diff/6001/tools/cros.DEPS/DEPS
File tools/cros.DEPS/DEPS (right):

http://codereview.chromium.org/7471024/diff/6001/tools/cros.DEPS/DEPS#newcode2
tools/cros.DEPS/DEPS:2: "chromium_git": "http://git.chromium.org/git",
You don't need this (your libcros change has already been picked up by
chrome)...

[sidor, 2011-07-25 21:40:07]

Will upload code shortly.

http://codereview.chromium.org/7471024/diff/6001/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/mount_library.cc (right):

http://codereview.chromium.org/7471024/diff/6001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/mount_library.cc:121: virtual void
FormatUnmountedDevice(const char* device_path) OVERRIDE {
On 2011/07/25 20:43:04, toni wrote:
> Argument name should match name in MountLibrary.h

Done.

http://codereview.chromium.org/7471024/diff/6001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/mount_library.cc:129: }
On 2011/07/25 20:43:04, toni wrote:
> If you leave this exposed through MountLibrary, checking that device (or some
of
> its children)is not mounted would probably be a good idea.

Done.

http://codereview.chromium.org/7471024/diff/6001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/mount_library.cc:137: DCHECK(device_mount_path);
Nope. UI, does not know the device path. At least my initial implementation.

http://codereview.chromium.org/7471024/diff/6001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/mount_library.cc:153: if
(to_be_formated_.find(device_path) != to_be_formated_.end()) {
On 2011/07/25 20:43:04, toni wrote:
> I agree with achuith that format_pending_ would be a better naming 

Done.

http://codereview.chromium.org/7471024/diff/6001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/mount_library.cc:396:
FireDeviceStatusUpdate(MOUNT_FORMATTING_STARTED,
Yes, I already talk about this with BenChan. There will be four event's:
- successfully started
- failed to start
- successfully finished
- failed to finish

We need four event's because they all means something different to the UI. Right
now it's encapsulated into two event's by use of !. 

I will change it as soon as you guys finish your changes to cors-disks (the
collisions we were talking about).

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

http://codereview.chromium.org/7471024/diff/6001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/mount_library.h:124: // Formats device given its
file path, eg. /dev/*
On 2011/07/25 20:43:04, toni wrote:
> dot at the end of the comment.

Done.

http://codereview.chromium.org/7471024/diff/6001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/mount_library.h:126: // Formats Device given its
mount path
Hmm.. I will use it really soon. I would really rather leave it there. I think I
will most probably use it this week, the only reason I didn't finished this
feature this last week is that it would overlap with your and BenChan work.

http://codereview.chromium.org/7471024/diff/6001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/mount_library.h:127: virtual void
FormatMountedDevice(const char* device_mount_path) = 0;
On 2011/07/25 20:43:04, toni wrote:
> I'd rename device_mount_path to mount_path.

Done.

http://codereview.chromium.org/7471024/diff/6001/chrome/browser/resources/fil...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/7471024/diff/6001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:1374:
if(!confirm(str('FORMATTING_WARNING'))) return;
True, and space after if, sorry for that.

http://codereview.chromium.org/7471024/diff/6001/tools/cros.DEPS/DEPS
File tools/cros.DEPS/DEPS (right):

http://codereview.chromium.org/7471024/diff/6001/tools/cros.DEPS/DEPS#newcode2
tools/cros.DEPS/DEPS:2: "chromium_git": "http://git.chromium.org/git",
On 2011/07/25 20:43:04, toni wrote:
> You don't need this (your libcros change has already been picked up by
> chrome)...

Done.

[tbarzic, 2011-07-26 22:49:50]

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

http://codereview.chromium.org/7471024/diff/13001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.cc:153: file_path =
it->second->file_path();
are you missing break here?

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

http://codereview.chromium.org/7471024/diff/13001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.h:126: // Formats Device given its
mount path
dot

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

http://codereview.chromium.org/7471024/diff/13001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:166:
OnDiskMounted(disk);
I added separate mount event handler, so you can remove these ;)

http://codereview.chromium.org/7471024/diff/13001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:359:
ShowFileBrowserNotification("FORMAT_FINISHED", device_path.substr(1, -1),
you don't need -1 here.

http://codereview.chromium.org/7471024/diff/13001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:376:
lib->MountPath(device_path.c_str());
MountLibrary interface has changed, this now has to be.
lib->MountPath(device_path.c_str(), 
               chromeos::MOUNT_TYPE_DEVICE,
               chromeos::MountPathOptions());

http://codereview.chromium.org/7471024/diff/13001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:381: const
std::string& category, const std::string& system_path,
Since you are not using category and system_path separately in this method,
maybe passing notification_id as parameter would be better idea than calculating
it inside.

http://codereview.chromium.org/7471024/diff/13001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:414:
ExtensionFileBrowserEventRouter::FindNotificationForPath(
I would change this name to FindNotificationForId

[sidor, 2011-07-28 00:20:51]

I will submit the code once it compiles nad works

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

http://codereview.chromium.org/7471024/diff/13001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.cc:153: file_path =
it->second->file_path();
On 2011/07/26 22:49:50, toni wrote:
> are you missing break here?

Done.

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

http://codereview.chromium.org/7471024/diff/13001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.h:126: // Formats Device given its
mount path
On 2011/07/26 22:49:50, toni wrote:
> dot

Done.

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

http://codereview.chromium.org/7471024/diff/13001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:166:
OnDiskMounted(disk);
On 2011/07/26 22:49:50, toni wrote:
> I added separate mount event handler, so you can remove these ;)

Done.

http://codereview.chromium.org/7471024/diff/13001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:359:
ShowFileBrowserNotification("FORMAT_FINISHED", device_path.substr(1, -1),
On 2011/07/26 22:49:50, toni wrote:
> you don't need -1 here.

Done.

http://codereview.chromium.org/7471024/diff/13001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:376:
lib->MountPath(device_path.c_str());
On 2011/07/26 22:49:50, toni wrote:
> MountLibrary interface has changed, this now has to be.
> lib->MountPath(device_path.c_str(), 
>                chromeos::MOUNT_TYPE_DEVICE,
>                chromeos::MountPathOptions());

Done.

http://codereview.chromium.org/7471024/diff/13001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:381: const
std::string& category, const std::string& system_path,
Nope, Look at the use of this function in code.

http://codereview.chromium.org/7471024/diff/13001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:414:
ExtensionFileBrowserEventRouter::FindNotificationForPath(
On 2011/07/26 22:49:50, toni wrote:
> I would change this name to FindNotificationForId

Done.

[sidor, 2011-07-29 17:17:55]

Confirmed that newest patch set is working


On 2011/07/28 00:20:51, sidor wrote:
> I will submit the code once it compiles nad works
> 
>
http://codereview.chromium.org/7471024/diff/13001/chrome/browser/chromeos/cro...
> File chrome/browser/chromeos/cros/mount_library.cc (right):
> 
>
http://codereview.chromium.org/7471024/diff/13001/chrome/browser/chromeos/cro...
> chrome/browser/chromeos/cros/mount_library.cc:153: file_path =
> it->second->file_path();
> On 2011/07/26 22:49:50, toni wrote:
> > are you missing break here?
> 
> Done.
> 
>
http://codereview.chromium.org/7471024/diff/13001/chrome/browser/chromeos/cro...
> File chrome/browser/chromeos/cros/mount_library.h (right):
> 
>
http://codereview.chromium.org/7471024/diff/13001/chrome/browser/chromeos/cro...
> chrome/browser/chromeos/cros/mount_library.h:126: // Formats Device given its
> mount path
> On 2011/07/26 22:49:50, toni wrote:
> > dot
> 
> Done.
> 
>
http://codereview.chromium.org/7471024/diff/13001/chrome/browser/chromeos/ext...
> File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):
> 
>
http://codereview.chromium.org/7471024/diff/13001/chrome/browser/chromeos/ext...
> chrome/browser/chromeos/extensions/file_browser_event_router.cc:166:
> OnDiskMounted(disk);
> On 2011/07/26 22:49:50, toni wrote:
> > I added separate mount event handler, so you can remove these ;)
> 
> Done.
> 
>
http://codereview.chromium.org/7471024/diff/13001/chrome/browser/chromeos/ext...
> chrome/browser/chromeos/extensions/file_browser_event_router.cc:359:
> ShowFileBrowserNotification("FORMAT_FINISHED", device_path.substr(1, -1),
> On 2011/07/26 22:49:50, toni wrote:
> > you don't need -1 here.
> 
> Done.
> 
>
http://codereview.chromium.org/7471024/diff/13001/chrome/browser/chromeos/ext...
> chrome/browser/chromeos/extensions/file_browser_event_router.cc:376:
> lib->MountPath(device_path.c_str());
> On 2011/07/26 22:49:50, toni wrote:
> > MountLibrary interface has changed, this now has to be.
> > lib->MountPath(device_path.c_str(), 
> >                chromeos::MOUNT_TYPE_DEVICE,
> >                chromeos::MountPathOptions());
> 
> Done.
> 
>
http://codereview.chromium.org/7471024/diff/13001/chrome/browser/chromeos/ext...
> chrome/browser/chromeos/extensions/file_browser_event_router.cc:381: const
> std::string& category, const std::string& system_path,
> Nope, Look at the use of this function in code.
> 
>
http://codereview.chromium.org/7471024/diff/13001/chrome/browser/chromeos/ext...
> chrome/browser/chromeos/extensions/file_browser_event_router.cc:414:
> ExtensionFileBrowserEventRouter::FindNotificationForPath(
> On 2011/07/26 22:49:50, toni wrote:
> > I would change this name to FindNotificationForId
> 
> Done.

[tbarzic, 2011-07-29 17:43:18]

You also have to edit 
chrome/browser/chromeos/cros/mock_mount_library.{cc, h}

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

http://codereview.chromium.org/7471024/diff/17001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.h:151: virtual void
FormatUnmountedDevice(const char* file_path) = 0;
These comments don't seem right...

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

http://codereview.chromium.org/7471024/diff/17001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:461: category +
system_path);
Doesn't this fit in one line

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

http://codereview.chromium.org/7471024/diff/17001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5767: "description": "Device
mount path."
Device's mount path.

[sidor, 2011-07-29 18:36:53]

Done.

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

http://codereview.chromium.org/7471024/diff/17001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.h:151: virtual void
FormatUnmountedDevice(const char* file_path) = 0;
On 2011/07/29 17:43:18, toni wrote:
> These comments don't seem right...

Done.

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

http://codereview.chromium.org/7471024/diff/17001/chrome/browser/chromeos/ext...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:461: category +
system_path);
Nope. One character :)
On 2011/07/29 17:43:18, toni wrote:
> Doesn't this fit in one line

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

http://codereview.chromium.org/7471024/diff/17001/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5767: "description": "Device
mount path."
On 2011/07/29 17:43:18, toni wrote:
> Device's mount path.

Done.

[tbarzic, 2011-07-29 19:25:03]

LGTM

On 2011/07/29 18:36:53, sidor wrote:
> Done.
> 
>
http://codereview.chromium.org/7471024/diff/17001/chrome/browser/chromeos/cro...
> File chrome/browser/chromeos/cros/mount_library.h (right):
> 
>
http://codereview.chromium.org/7471024/diff/17001/chrome/browser/chromeos/cro...
> chrome/browser/chromeos/cros/mount_library.h:151: virtual void
> FormatUnmountedDevice(const char* file_path) = 0;
> On 2011/07/29 17:43:18, toni wrote:
> > These comments don't seem right...
> 
> Done.
> 
>
http://codereview.chromium.org/7471024/diff/17001/chrome/browser/chromeos/ext...
> File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):
> 
>
http://codereview.chromium.org/7471024/diff/17001/chrome/browser/chromeos/ext...
> chrome/browser/chromeos/extensions/file_browser_event_router.cc:461: category
+
> system_path);
> Nope. One character :)
> On 2011/07/29 17:43:18, toni wrote:
> > Doesn't this fit in one line
> 
>
http://codereview.chromium.org/7471024/diff/17001/chrome/common/extensions/ap...
> File chrome/common/extensions/api/extension_api.json (right):
> 
>
http://codereview.chromium.org/7471024/diff/17001/chrome/common/extensions/ap...
> chrome/common/extensions/api/extension_api.json:5767: "description": "Device
> mount path."
> On 2011/07/29 17:43:18, toni wrote:
> > Device's mount path.
> 
> Done.

[sidor, 2011-07-29 19:26:02]

cool! Thanks for review and for help!

On 2011/07/29 19:25:03, toni wrote:
> LGTM
> 
> On 2011/07/29 18:36:53, sidor wrote:
> > Done.
> > 
> >
>
http://codereview.chromium.org/7471024/diff/17001/chrome/browser/chromeos/cro...
> > File chrome/browser/chromeos/cros/mount_library.h (right):
> > 
> >
>
http://codereview.chromium.org/7471024/diff/17001/chrome/browser/chromeos/cro...
> > chrome/browser/chromeos/cros/mount_library.h:151: virtual void
> > FormatUnmountedDevice(const char* file_path) = 0;
> > On 2011/07/29 17:43:18, toni wrote:
> > > These comments don't seem right...
> > 
> > Done.
> > 
> >
>
http://codereview.chromium.org/7471024/diff/17001/chrome/browser/chromeos/ext...
> > File chrome/browser/chromeos/extensions/file_browser_event_router.cc
(right):
> > 
> >
>
http://codereview.chromium.org/7471024/diff/17001/chrome/browser/chromeos/ext...
> > chrome/browser/chromeos/extensions/file_browser_event_router.cc:461:
category
> +
> > system_path);
> > Nope. One character :)
> > On 2011/07/29 17:43:18, toni wrote:
> > > Doesn't this fit in one line
> > 
> >
>
http://codereview.chromium.org/7471024/diff/17001/chrome/common/extensions/ap...
> > File chrome/common/extensions/api/extension_api.json (right):
> > 
> >
>
http://codereview.chromium.org/7471024/diff/17001/chrome/common/extensions/ap...
> > chrome/common/extensions/api/extension_api.json:5767: "description": "Device
> > mount path."
> > On 2011/07/29 17:43:18, toni wrote:
> > > Device's mount path.
> > 
> > Done.

[commit-bot: I haz the power, 2011-07-29 21:38:28]

Presubmit check for 7471024-21002 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
Found line ending with white spaces in:

***************
chrome/browser/chromeos/cros/mount_library.h, line 154
chrome/browser/chromeos/cros/mount_library.h, line 155
chrome/browser/chromeos/cros/mount_library.cc, line 674
chrome/browser/chromeos/extensions/file_browser_event_router.cc, line 429
***************

** Presubmit ERRORS **
Missing LGTM from an OWNER for: chrome/common/extensions/api/extension_api.json

Presubmit checks took 1.3s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[sidor.dev, 2011-07-29 22:43:49]

Hi,

I need a json file owner's LGTM to commit my code. Can you take a look at it?
It's just 11 lines (one function).

Thanks,
Szymon

[asargent_no_longer_on_chrome, 2011-07-29 23:14:19]

LGTM w/ a nit you should fix before committing (no need to send for re-review),

Also I had one question for you to consider about the API, but again no need to
hold this up since it could be addressed in a follow-up CL (if you decide you
want to change it).

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

http://codereview.chromium.org/7471024/diff/21002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:1271: if
(!args_->GetString(0, &volume_mount_path)) {
nit: you probably want to either CHECK(false) here and crash the whole browser,
or return early from the function and not go on to pass an empty string to
FormatMountedDevice below (since NOTREACHED only does something in Debug builds
and is elided in Release builds).

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

http://codereview.chromium.org/7471024/diff/21002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:279: : public
SyncExtensionFunction {
Note that if you wanted to have a callback, and wait until the format was
complete to pass back a success or failure message, you could make this inherit
from AsyncExtensionFunction instead of SyncExtensionFunction.

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

http://codereview.chromium.org/7471024/diff/21002/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5768: }
I'm sort of surprised not to see a callback here - do you not want to
communicate the success/failure of the method to callers?

[sidor.dev, 2011-07-29 23:28:27]

Thanks for review!

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

http://codereview.chromium.org/7471024/diff/21002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:1271: if
(!args_->GetString(0, &volume_mount_path)) {
On 2011/07/29 23:14:19, Antony Sargent wrote:
> nit: you probably want to either CHECK(false) here and crash the whole
browser,
> or return early from the function and not go on to pass an empty string to
> FormatMountedDevice below (since NOTREACHED only does something in Debug
builds
> and is elided in Release builds).
> 

Done.

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

http://codereview.chromium.org/7471024/diff/21002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:279: : public
SyncExtensionFunction {
Formatting is quite separated from UI, so callback is not needed here. But
thanks for suggestion!

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

http://codereview.chromium.org/7471024/diff/21002/chrome/common/extensions/ap...
chrome/common/extensions/api/extension_api.json:5768: }
Replied under other comment - in general it send's notifications outside UI and
the way it communicates with UI is via mount and unmount functions that already
have signals sent to UI implemented. But again thanks for advice!

On 2011/07/29 23:14:19, Antony Sargent wrote:
> I'm sort of surprised not to see a callback here - do you not want to
> communicate the success/failure of the method to callers?

[commit-bot: I haz the power, 2011-07-30 00:29:51]

Presubmit check for 7471024-28001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
Found line ending with white spaces in:

***************
chrome/browser/chromeos/cros/mount_library.h, line 154
chrome/browser/chromeos/cros/mount_library.h, line 155
chrome/browser/chromeos/cros/mount_library.cc, line 674
chrome/browser/chromeos/extensions/file_browser_event_router.cc, line 429
***************

Presubmit checks took 6.3s to calculate.

[commit-bot: I haz the power, 2011-07-30 00:42:34]

Presubmit check for 7471024-26015 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
Found line ending with white spaces in:

***************
chrome/browser/chromeos/cros/mount_library.h, line 154
chrome/browser/chromeos/cros/mount_library.h, line 155
chrome/browser/chromeos/cros/mount_library.cc, line 674
***************

Presubmit checks took 1.5s to calculate.

[commit-bot: I haz the power, 2011-07-30 01:44:20]

Change committed as 94812