Dropping files into the root list.

Dropping files into the root list.

BUG=
TEST=

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

[Vladislav Kaznacheev, 2012-04-25 08:43:11]

https://chromiumcodereview.appspot.com/10226001/diff/1/chrome/browser/resourc...
File chrome/browser/resources/file_manager/js/directory_model.js (right):

https://chromiumcodereview.appspot.com/10226001/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/directory_model.js:56: /** @type
{Hash.<srting, boolead>} */
2 typos

https://chromiumcodereview.appspot.com/10226001/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/directory_model.js:157: return
this.offline_;
Lets use !navigator.onLine directly and remove the setter. There is no benefit
in passing it from FileManager

https://chromiumcodereview.appspot.com/10226001/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/directory_model.js:1069:
chrome.fileBrowserPrivate.getVolumeMetadata(roots[i].toURL(),
It looks like you are making this call for all roots while it only makes sense
for removable drives

https://chromiumcodereview.appspot.com/10226001/diff/1/chrome/browser/resourc...
File chrome/browser/resources/file_manager/js/file_transfer_controller.js
(right):

https://chromiumcodereview.appspot.com/10226001/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/file_transfer_controller.js:55:
attachDropTarget: function(list, opt_onlyIntoDirectories) {
Please comment on the second parameter (here or in onDrop_ JSDoc). The name is
not very obvious

https://chromiumcodereview.appspot.com/10226001/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/file_transfer_controller.js:318:
return;
return false?

[SeRya, 2012-04-25 10:15:15]

https://chromiumcodereview.appspot.com/10226001/diff/1/chrome/browser/resourc...
File chrome/browser/resources/file_manager/js/directory_model.js (right):

https://chromiumcodereview.appspot.com/10226001/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/directory_model.js:56: /** @type
{Hash.<srting, boolead>} */
On 2012/04/25 08:43:11, Vladislav Kaznacheev wrote:
> 2 typos

Done.

https://chromiumcodereview.appspot.com/10226001/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/directory_model.js:157: return
this.offline_;
On 2012/04/25 08:43:11, Vladislav Kaznacheev wrote:
> Lets use !navigator.onLine directly and remove the setter. There is no benefit
> in passing it from FileManager

Done.

https://chromiumcodereview.appspot.com/10226001/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/directory_model.js:1069:
chrome.fileBrowserPrivate.getVolumeMetadata(roots[i].toURL(),
On 2012/04/25 08:43:11, Vladislav Kaznacheev wrote:
> It looks like you are making this call for all roots while it only makes sense
> for removable drives

No, it's for removable only (I mentioned in JsDoc).

https://chromiumcodereview.appspot.com/10226001/diff/1/chrome/browser/resourc...
File chrome/browser/resources/file_manager/js/file_transfer_controller.js
(right):

https://chromiumcodereview.appspot.com/10226001/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/file_transfer_controller.js:55:
attachDropTarget: function(list, opt_onlyIntoDirectories) {
On 2012/04/25 08:43:11, Vladislav Kaznacheev wrote:
> Please comment on the second parameter (here or in onDrop_ JSDoc). The name is
> not very obvious

Done.

https://chromiumcodereview.appspot.com/10226001/diff/1/chrome/browser/resourc...
chrome/browser/resources/file_manager/js/file_transfer_controller.js:318:
return;
On 2012/04/25 08:43:11, Vladislav Kaznacheev wrote:
> return false?

Done.

[Vladislav Kaznacheev, 2012-04-25 11:08:04]

Two more comments (about the same problem really)

https://chromiumcodereview.appspot.com/10226001/diff/9006/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/directory_model.js (right):

https://chromiumcodereview.appspot.com/10226001/diff/9006/chrome/browser/reso...
chrome/browser/resources/file_manager/js/directory_model.js:57:
this.volumeReadOnlyStatus_ = {};
I am concerned that the default value is "false" (not readonly) which means that
before getVolumeMetadata returns the drive is actually writable and we can try
copying something there if we are quick.

https://chromiumcodereview.appspot.com/10226001/diff/9006/chrome/browser/reso...
chrome/browser/resources/file_manager/js/directory_model.js:1051:
status[roots[i].fullPath] = false;
How about setting 'true' originally?

[SeRya, 2012-04-25 11:50:52]

https://chromiumcodereview.appspot.com/10226001/diff/9006/chrome/browser/reso...
File chrome/browser/resources/file_manager/js/directory_model.js (right):

https://chromiumcodereview.appspot.com/10226001/diff/9006/chrome/browser/reso...
chrome/browser/resources/file_manager/js/directory_model.js:57:
this.volumeReadOnlyStatus_ = {};
On 2012/04/25 11:08:04, Vladislav Kaznacheev wrote:
> I am concerned that the default value is "false" (not readonly) which means
that
> before getVolumeMetadata returns the drive is actually writable and we can try
> copying something there if we are quick.

This place is not about default values. volumeReadOnlyStatus_ is empty and
rootList_ is empty as well. They both update synchronously so they must be
always consistent.

https://chromiumcodereview.appspot.com/10226001/diff/9006/chrome/browser/reso...
chrome/browser/resources/file_manager/js/directory_model.js:1051:
status[roots[i].fullPath] = false;
On 2012/04/25 11:08:04, Vladislav Kaznacheev wrote:
> How about setting 'true' originally? 

I would vote for 'false' because:
1. It better works in testing environment where getVolumeMetadata doesn't work.
2. In case something goes wrong it's better to let the user to perform
operations on an readonly volume and get an error then stop the user from
performing operations on a writable volume.

[Vladislav Kaznacheev, 2012-04-25 12:44:03]

lgtm