This is UI side unreadable device support.

This is UI side unreadable device support. 

Mostly format manager code, that diplays and handles unreadalbe device. There's option to format them. One cannot access them.
Icons were added as well.

Depends on the following change in ChromeOS: http://gerrit.chromium.org/gerrit/#change,6406

BUG=chromium-os:19506
TEST=None.


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

[sidor, 2011-08-23 04:43:33]

Hi,

Toni, can you review my code?
Robert, can you take a look at file_manager.js code ad see id general flow is
ok?

Thanks in advance,
Szymon

[rginda, 2011-08-23 20:23:29]

http://codereview.chromium.org/7706028/diff/1/chrome/app/generated_resources.grd
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7706028/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:9147: This device cannot be opened, because
its filesystem was not recognised.
No comma, and use recognized (-ised is the British variant)

http://codereview.chromium.org/7706028/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:9150: This device cannot be opened, because
its filesystem is not supported on this machine.
No comma, and remove "on this machine".

http://codereview.chromium.org/7706028/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:9151: </message>
Please add these strings to mock_chrome too.

http://codereview.chromium.org/7706028/diff/1/chrome/browser/resources/file_m...
File chrome/browser/resources/file_manager/js/file_manager.js (left):

http://codereview.chromium.org/7706028/diff/1/chrome/browser/resources/file_m...
chrome/browser/resources/file_manager/js/file_manager.js:495: function
cacheEntryIconType(entry, successCallback) {
If this is going to be invoked as a method, it should be declared as one.

http://codereview.chromium.org/7706028/diff/1/chrome/browser/resources/file_m...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/7706028/diff/1/chrome/browser/resources/file_m...
chrome/browser/resources/file_manager/js/file_manager.js:320: function
normalize(x) {
normalizePath please.

http://codereview.chromium.org/7706028/diff/1/chrome/browser/resources/file_m...
chrome/browser/resources/file_manager/js/file_manager.js:673: var
isThisIconForThatEntry = value.bind(this);
Please just pass in the FileManager instance as a parameter.

[rginda, 2011-08-23 20:25:12]

http://codereview.chromium.org/7706028/diff/1/chrome/browser/resources/file_m...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/7706028/diff/1/chrome/browser/resources/file_m...
chrome/browser/resources/file_manager/js/file_manager.js:320: function
normalize(x) {
On 2011/08/23 20:23:30, rginda wrote:
> normalizePath please.

Actually, it would probably be cleaner to call this normalizeAbsolutePath, and
have it ensure that the path DOES start with a /.

[sidor, 2011-08-23 23:18:47]

iteration 2

http://codereview.chromium.org/7706028/diff/1/chrome/app/generated_resources.grd
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/7706028/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:9147: This device cannot be opened, because
its filesystem was not recognised.
On 2011/08/23 20:23:30, rginda wrote:
> No comma, and use recognized (-ised is the British variant)

Done.

http://codereview.chromium.org/7706028/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:9150: This device cannot be opened, because
its filesystem is not supported on this machine.
On 2011/08/23 20:23:30, rginda wrote:
> No comma, and remove "on this machine".

Done.

http://codereview.chromium.org/7706028/diff/1/chrome/app/generated_resources....
chrome/app/generated_resources.grd:9151: </message>
On 2011/08/23 20:23:30, rginda wrote:
> Please add these strings to mock_chrome too.

Done.

http://codereview.chromium.org/7706028/diff/1/chrome/browser/resources/file_m...
File chrome/browser/resources/file_manager/js/file_manager.js (left):

http://codereview.chromium.org/7706028/diff/1/chrome/browser/resources/file_m...
chrome/browser/resources/file_manager/js/file_manager.js:495: function
cacheEntryIconType(entry, successCallback) {
On 2011/08/23 20:23:30, rginda wrote:
> If this is going to be invoked as a method, it should be declared as one.

Done.

http://codereview.chromium.org/7706028/diff/1/chrome/browser/resources/file_m...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/7706028/diff/1/chrome/browser/resources/file_m...
chrome/browser/resources/file_manager/js/file_manager.js:320: function
normalize(x) {
So according to Toni, this should be removing / at the beginning because soon we
will have all path with / stripped at the beginning and this method would be
removed.

On 2011/08/23 20:25:12, rginda wrote:
> On 2011/08/23 20:23:30, rginda wrote:
> > normalizePath please.
> 
> Actually, it would probably be cleaner to call this normalizeAbsolutePath, and
> have it ensure that the path DOES start with a /.

http://codereview.chromium.org/7706028/diff/1/chrome/browser/resources/file_m...
chrome/browser/resources/file_manager/js/file_manager.js:320: function
normalize(x) {
On 2011/08/23 20:23:30, rginda wrote:
> normalizePath please.

Path name changed.

http://codereview.chromium.org/7706028/diff/1/chrome/browser/resources/file_m...
chrome/browser/resources/file_manager/js/file_manager.js:673: var
isThisIconForThatEntry = value.bind(this);
On 2011/08/23 20:23:30, rginda wrote:
> Please just pass in the FileManager instance as a parameter.

Done.

[tbarzic, 2011-08-24 03:59:44]

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

http://codereview.chromium.org/7706028/diff/7001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/mount_library.h:141: const char* special)
special data seems a bit vague.. maybe something like format_status, or
device_status..
Also, I'd rather make this enum, or even bool value  (can_be_formatted) if you
decide to go with unified error message (see comment in file_manager.js).

http://codereview.chromium.org/7706028/diff/7001/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

http://codereview.chromium.org/7706028/diff/7001/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:359:
mount_info.special_data != "unreadable_unknown_filesystem" &&
why not use mount_info.special_data.empty()

http://codereview.chromium.org/7706028/diff/7001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/7706028/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_file_browser_private_api.cc:1341:
chromeos::CrosLibrary::Get()->GetMountLibrary()->FormatUnmountedDevice(
are you sure you don't have to call FormatMountedDevice (to unmount "fake"
mounted path).. if not, be sure to remove entry for device from mount_points in
mount_lib

http://codereview.chromium.org/7706028/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_file_browser_private_api.cc:1384: mount_path
= "unmounted";
leave mount_path empty if volume has no mount_path...
or even leave mountPath in volume_info undefined

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

http://codereview.chromium.org/7706028/diff/7001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:1815: var
rescanDirectory = (event.status == 'success');
I think smth like rescanDirectoryNeeded would be clearer name..

http://codereview.chromium.org/7706028/diff/7001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:1817: if
(event.sourceUrl == mountPoints[i].sourceUrl &&
why don't you just add specialData field to the event?

http://codereview.chromium.org/7706028/diff/7001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:2537: return
this.showButter(str('UNKNOWN_FILESYSTEM_WARNING'));
do we really need different error messages for unknown and unsupported devices.
I think something like "Cannot open device which is not mounted." would be
sufficient.

[sidor, 2011-08-24 04:26:05]

Haven't made any actually changes to code(I don't have VPN access), but I
publish my comments because there's a lot of discussable things.

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

http://codereview.chromium.org/7706028/diff/7001/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/mount_library.h:141: const char* special)
So the idea is that there are some special condition related to the mount info.
That's why I created that field. In the future there might be more special
conditions. Making it enum is not a bad idea. Also if you think there's better
name for that field I am open to any suggestions.

http://codereview.chromium.org/7706028/diff/7001/chrome/browser/chromeos/exte...
File chrome/browser/chromeos/extensions/file_browser_event_router.cc (right):

http://codereview.chromium.org/7706028/diff/7001/chrome/browser/chromeos/exte...
chrome/browser/chromeos/extensions/file_browser_event_router.cc:359:
mount_info.special_data != "unreadable_unknown_filesystem" &&
On 2011/08/24 03:59:44, tbarzic wrote:
> why not use mount_info.special_data.empty()

hmm... I was thinking that in case of some other future values of special_data
we may want to actually open the tab.

http://codereview.chromium.org/7706028/diff/7001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_file_browser_private_api.cc (right):

http://codereview.chromium.org/7706028/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_file_browser_private_api.cc:1341:
chromeos::CrosLibrary::Get()->GetMountLibrary()->FormatUnmountedDevice(
On 2011/08/24 03:59:44, tbarzic wrote:
> are you sure you don't have to call FormatMountedDevice (to unmount "fake"
> mounted path).. if not, be sure to remove entry for device from mount_points
in
> mount_lib

I an not using this method at all, I just added it in case it's useful in the
future.

http://codereview.chromium.org/7706028/diff/7001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_file_browser_private_api.cc:1384: mount_path
= "unmounted";
On 2011/08/24 03:59:44, tbarzic wrote:
> leave mount_path empty if volume has no mount_path...
> or even leave mountPath in volume_info undefined

Will do this tomorrow.

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

http://codereview.chromium.org/7706028/diff/7001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:1815: var
rescanDirectory = (event.status == 'success');
Will do this tommorrow.

http://codereview.chromium.org/7706028/diff/7001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:1817: if
(event.sourceUrl == mountPoints[i].sourceUrl &&
On 2011/08/24 03:59:44, tbarzic wrote:
> why don't you just add specialData field to the event?

Because I need to perform check later on double click.

http://codereview.chromium.org/7706028/diff/7001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:2537: return
this.showButter(str('UNKNOWN_FILESYSTEM_WARNING'));
In the future I will add a message suggesting formatting to unknown device
message. For unsupported filesystem it seems a little bit to huge to suggest
formatting. That's what I established with Robert.

[sidor, 2011-08-24 18:44:23]

Ok the actual changes have been uploaded. I also factor out new
OnDirectoryAction fuction that is executed when directory is double clicked and
when use hits enter key.

[tbarzic, 2011-08-25 02:03:44]

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

http://codereview.chromium.org/7706028/diff/7001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:1817: if
(event.sourceUrl == mountPoints[i].sourceUrl &&
On 2011/08/24 04:26:06, sidor wrote:
> On 2011/08/24 03:59:44, tbarzic wrote:
> > why don't you just add specialData field to the event?
> 
> Because I need to perform check later on double click. 

yeah, I know..
What I ment is you could avoid search here by adding specialData field to both
event and mount point info

http://codereview.chromium.org/7706028/diff/7001/chrome/browser/resources/fil...
chrome/browser/resources/file_manager/js/file_manager.js:2537: return
this.showButter(str('UNKNOWN_FILESYSTEM_WARNING'));
On 2011/08/24 04:26:06, sidor wrote:
> In the future I will add a message suggesting formatting to unknown device
> message. For unsupported filesystem it seems a little bit to huge to suggest
> formatting. That's what I established with Robert.

ok

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

http://codereview.chromium.org/7706028/diff/11001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.h:144: MountCondition
mount_condition;
hm.. better than special_data :)
I would still consider making its name specific for data it holds a the moment
(if we decide to add some more meaning it shouldn't be a problem to change its
name)...
but I'm ok with what ever you decide..

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

http://codereview.chromium.org/7706028/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:1386: mount_path
= "";
you don't need else part... mount_path is set to "" by default

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

http://codereview.chromium.org/7706028/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:299: // Formats
Device given its file path.
If you don't use this (and don't plan to), I think it's better not to add it...
It won't be a problem to add it later if needed..

[sidor, 2011-08-25 18:02:13]

Done.

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

http://codereview.chromium.org/7706028/diff/11001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.h:144: MountCondition
mount_condition;
On 2011/08/25 02:03:44, tbarzic wrote:
> hm.. better than special_data :)
> I would still consider making its name specific for data it holds a the moment
> (if we decide to add some more meaning it shouldn't be a problem to change its
> name)...
> but I'm ok with what ever you decide..

Ok, I agree, but hmm... It's really hard to invent and a name that will be
specific but also not overdescriptive. We don't want
IsDeviceMountedEvenThoughFilesystemWasNotRecognised :D So I will leave it as
mountConditions. It shouldn't be too confusing because if somebody don't know
what it is, it's really easy to look up enum and see the possible values.

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

http://codereview.chromium.org/7706028/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.cc:1386: mount_path
= "";
On 2011/08/25 02:03:44, tbarzic wrote:
> you don't need else part... mount_path is set to "" by default

Yes, sorry for that. That what happens when you refactor :)

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

http://codereview.chromium.org/7706028/diff/11001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_file_browser_private_api.h:299: // Formats
Device given its file path.
On 2011/08/25 02:03:44, tbarzic wrote:
> If you don't use this (and don't plan to), I think it's better not to add
it...
> It won't be a problem to add it later if needed..

Done.

[tbarzic, 2011-08-25 18:17:53]

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

http://codereview.chromium.org/7706028/diff/11001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/mount_library.h:144: MountCondition
mount_condition;
On 2011/08/25 18:02:13, sidor wrote:
> On 2011/08/25 02:03:44, tbarzic wrote:
> > hm.. better than special_data :)
> > I would still consider making its name specific for data it holds a the
moment
> > (if we decide to add some more meaning it shouldn't be a problem to change
its
> > name)...
> > but I'm ok with what ever you decide..
> 
> Ok, I agree, but hmm... It's really hard to invent and a name that will be
> specific but also not overdescriptive. We don't want
> IsDeviceMountedEvenThoughFilesystemWasNotRecognised :D So I will leave it as
> mountConditions. It shouldn't be too confusing because if somebody don't know
> what it is, it's really easy to look up enum and see the possible values. 

Yeah, actually, when I think about it..this is quite good name ;)

[tbarzic, 2011-08-25 19:15:29]

On 2011/08/25 18:17:53, tbarzic wrote:
>
http://codereview.chromium.org/7706028/diff/11001/chrome/browser/chromeos/cro...
> File chrome/browser/chromeos/cros/mount_library.h (right):
> 
>
http://codereview.chromium.org/7706028/diff/11001/chrome/browser/chromeos/cro...
> chrome/browser/chromeos/cros/mount_library.h:144: MountCondition
> mount_condition;
> On 2011/08/25 18:02:13, sidor wrote:
> > On 2011/08/25 02:03:44, tbarzic wrote:
> > > hm.. better than special_data :)
> > > I would still consider making its name specific for data it holds a the
> moment
> > > (if we decide to add some more meaning it shouldn't be a problem to change
> its
> > > name)...
> > > but I'm ok with what ever you decide..
> > 
> > Ok, I agree, but hmm... It's really hard to invent and a name that will be
> > specific but also not overdescriptive. We don't want
> > IsDeviceMountedEvenThoughFilesystemWasNotRecognised :D So I will leave it as
> > mountConditions. It shouldn't be too confusing because if somebody don't
know
> > what it is, it's really easy to look up enum and see the possible values. 
> 
> Yeah, actually, when I think about it..this is quite good name ;)

as far as I'm concerned, LGTM

[rginda, 2011-08-25 21:05:39]

http://codereview.chromium.org/7706028/diff/16003/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/7706028/diff/16003/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/file_manager.js:1276: cacheFunction =
this.cacheEntryIconType;
This should be bound.

http://codereview.chromium.org/7706028/diff/16003/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/file_manager.js:2540:
FileManager.prototype.OnDirectoryAction = function(entry) {
Lowercase 'o'

[sidor, 2011-08-26 00:49:14]

Done.

http://codereview.chromium.org/7706028/diff/16003/chrome/browser/resources/fi...
File chrome/browser/resources/file_manager/js/file_manager.js (right):

http://codereview.chromium.org/7706028/diff/16003/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/file_manager.js:1276: cacheFunction =
this.cacheEntryIconType;
On 2011/08/25 21:05:39, rginda wrote:
> This should be bound.

Done.

http://codereview.chromium.org/7706028/diff/16003/chrome/browser/resources/fi...
chrome/browser/resources/file_manager/js/file_manager.js:2540:
FileManager.prototype.OnDirectoryAction = function(entry) {
On 2011/08/25 21:05:39, rginda wrote:
> Lowercase 'o'

Done.

[commit-bot: I haz the power, 2011-08-26 19:32:26]

Presubmit check for 7706028-23001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  chrome/browser/resources/file_manager/js/mock_chrome.js, line 284, 107 chars \
  chrome/browser/resources/file_manager/js/mock_chrome.js, line 285, 110 chars \
  chrome/browser/resources/file_manager/js/mock_chrome.js, line 286, 112 chars

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

Presubmit checks took 4.0s to calculate.

[sidor, 2011-08-26 19:47:21]

Hi,

I need mtgl of the owner of manifest file. Could you take a look?

Thanks,
Szymon

[asargent_no_longer_on_chrome, 2011-08-26 20:17:42]

extension_api.json LGTM

[commit-bot: I haz the power, 2011-08-26 20:27:25]

Presubmit check for 7706028-23001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
Found lines longer than 80 characters (first 5 shown).
  chrome/browser/resources/file_manager/js/mock_chrome.js, line 284, 107 chars \
  chrome/browser/resources/file_manager/js/mock_chrome.js, line 285, 110 chars \
  chrome/browser/resources/file_manager/js/mock_chrome.js, line 286, 112 chars

Presubmit checks took 1.5s to calculate.

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

[commit-bot: I haz the power, 2011-08-26 22:52:01]

Can't apply patch for file
chrome/browser/resources/file_manager/js/file_manager.js.
While running patch -p1 --forward --force;
patching file chrome/browser/resources/file_manager/js/file_manager.js
Hunk #11 FAILED at 1825.
Hunk #16 succeeded at 3022 (offset 51 lines).
1 out of 16 hunks FAILED -- saving rejects to file
chrome/browser/resources/file_manager/js/file_manager.js.rej

[commit-bot: I haz the power, 2011-08-27 02:45:59]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux, revision is 98533, job name
was 7706028-32001 (retry) (retry) (retry) (retry).

[commit-bot: I haz the power, 2011-08-27 04:56:25]

Change committed as 98548