|
|
Created:
6 years, 3 months ago by iseki Modified:
6 years, 3 months ago Reviewers:
hirono CC:
chromium-reviews, extensions-reviews_chromium.org, nkostylev+watch_chromium.org, yoshiki+watch_chromium.org, rginda+watch_chromium.org, mtomasz+watch_chromium.org, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd notification before getMultiProfileShareEntries_.
* getMultiProfileShareEntries_ is very slow.
BUG=409711
TEST=manually
1.Copy the large directory in the drive of user A.
2.Paste the A's large directory into the drive of user B.
3.Confirm notification is displayed.
Committed: https://crrev.com/fe9b1a17bd8576ce2a42d233d668c469c0379c91
Cr-Commit-Position: refs/heads/master@{#295235}
Patch Set 1 #
Total comments: 20
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #
Total comments: 12
Patch Set 4 : #
Messages
Total messages: 14 (2 generated)
iseki@chromium.org changed reviewers: + hirono@chromium.org
Please take a look.
https://codereview.chromium.org/571343002/diff/1/chrome/app/chromeos_strings.... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/571343002/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:501: Check items are shared. Could you confirm with @bengold if the message is correct? Maybe we can just show "Copying..." message. https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/background/js/file_operation_manager.js (right): https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/background/js/file_operation_manager.js:1186: * if the operation is "copy") false. Please update JSDoc. https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/background/js/file_operation_manager.js:1234: * @private ditto. https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/foreground/js/file_manager.js (right): https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/file_manager.js:2823: for (var i=0; I think the for loop should be placed at the out of the if loop. FileTransferController adds an item regardress of the existence of ProgressCenterPanel. WDYT? https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/file_manager.js:2824: i < this.fileTransferController_.pendingTaskId.length; ++i) { nit: 1. Please insert spaces: "var i = 0;". 2. Also fix the indent at #2824. 3. Please put the third statement "++i" at the new line as well as the second line. https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/foreground/js/file_transfer_controller.js (right): https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:23: * used to share files from another profile. Please update JSDoc. https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:55: this.pendingTaskId = []; nit: pendingTaskIDs ? https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:89: * @type {int} We don't use int. Instead, please use number. https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:395: this.pendingTaskId.splice(this.pendingTaskId.find(taskId), 1); Array does not have 'find' method.
Thank you for your review. https://codereview.chromium.org/571343002/diff/1/chrome/app/chromeos_strings.... File chrome/app/chromeos_strings.grdp (right): https://codereview.chromium.org/571343002/diff/1/chrome/app/chromeos_strings.... chrome/app/chromeos_strings.grdp:501: Check items are shared. On 2014/09/16 08:12:56, hirono wrote: > Could you confirm with @bengold if the message is correct? > Maybe we can just show "Copying..." message. Done. https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/background/js/file_operation_manager.js (right): https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/background/js/file_operation_manager.js:1186: * if the operation is "copy") false. On 2014/09/16 08:12:56, hirono wrote: > Please update JSDoc. Done. https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/background/js/file_operation_manager.js:1234: * @private On 2014/09/16 08:12:56, hirono wrote: > ditto. Done. https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/foreground/js/file_manager.js (right): https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/file_manager.js:2823: for (var i=0; On 2014/09/16 08:12:56, hirono wrote: > I think the for loop should be placed at the out of the if loop. > FileTransferController adds an item regardress of the existence of > ProgressCenterPanel. > WDYT? Done. https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/file_manager.js:2824: i < this.fileTransferController_.pendingTaskId.length; ++i) { On 2014/09/16 08:12:56, hirono wrote: > nit: > > 1. Please insert spaces: "var i = 0;". > 2. Also fix the indent at #2824. > 3. Please put the third statement "++i" at the new line as well as the second > line. > Done. https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/foreground/js/file_transfer_controller.js (right): https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:23: * used to share files from another profile. On 2014/09/16 08:12:56, hirono wrote: > Please update JSDoc. Done. https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:55: this.pendingTaskId = []; On 2014/09/16 08:12:56, hirono wrote: > nit: pendingTaskIDs ? Done. https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:89: * @type {int} On 2014/09/16 08:12:56, hirono wrote: > We don't use int. Instead, please use number. Done. https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:395: this.pendingTaskId.splice(this.pendingTaskId.find(taskId), 1); On 2014/09/16 08:12:56, hirono wrote: > Array does not have 'find' method. Done.
https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/foreground/js/file_manager.js (right): https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/file_manager.js:2824: i < this.fileTransferController_.pendingTaskId.length; ++i) { On 2014/09/16 08:44:20, iseki wrote: > On 2014/09/16 08:12:56, hirono wrote: > > nit: > > > > 1. Please insert spaces: "var i = 0;". > > 2. Also fix the indent at #2824. > > 3. Please put the third statement "++i" at the new line as well as the second > > line. > > > > Done. Please also handle 2. and 3. https://codereview.chromium.org/571343002/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/file_operation_manager.js (right): https://codereview.chromium.org/571343002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/file_operation_manager.js:1187: * @param {string} taskId It is used at progressCenter. @param {string=} opt_taskId ? Please mention about: 1. If the corresponding item has already created at another places, we need to specify the ID of the item. 2. If we don't specify the ID, the class generates new ID. https://codereview.chromium.org/571343002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/file_operation_manager.js:1255: if (taskId) nit: We can make it more simple. task.taskId = taskId || this.generateTaskId_(); https://codereview.chromium.org/571343002/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/file_transfer_controller.js (right): https://codereview.chromium.org/571343002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:362: item.message = strf('COPY_PROGRESS_SUMMARY'); Sorry for changing the comment. But IDS_FILE_BROWSER_COPY_FILE_NAME or IDS_FILE_BROWSER_COPY_ITEMS_REMAINING is better here. https://codereview.chromium.org/571343002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:1103: return 'file-operation-from-transfer-controller' + this.taskIdCounter_++; This function should generate unique ID among the windows.
Thank you for your review :) https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/foreground/js/file_manager.js (right): https://codereview.chromium.org/571343002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/file_manager.js:2824: i < this.fileTransferController_.pendingTaskId.length; ++i) { On 2014/09/16 09:01:31, hirono wrote: > On 2014/09/16 08:44:20, iseki wrote: > > On 2014/09/16 08:12:56, hirono wrote: > > > nit: > > > > > > 1. Please insert spaces: "var i = 0;". > > > 2. Also fix the indent at #2824. > > > 3. Please put the third statement "++i" at the new line as well as the > second > > > line. > > > > > > > Done. > > Please also handle 2. and 3. Done. https://codereview.chromium.org/571343002/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/file_operation_manager.js (right): https://codereview.chromium.org/571343002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/file_operation_manager.js:1187: * @param {string} taskId It is used at progressCenter. On 2014/09/16 09:01:31, hirono wrote: > @param {string=} opt_taskId ? > > Please mention about: > > 1. If the corresponding item has already created at another places, we need to > specify the ID of the item. > 2. If we don't specify the ID, the class generates new ID. Done. https://codereview.chromium.org/571343002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/file_operation_manager.js:1255: if (taskId) On 2014/09/16 09:01:31, hirono wrote: > nit: We can make it more simple. > task.taskId = taskId || this.generateTaskId_(); Done. https://codereview.chromium.org/571343002/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/file_transfer_controller.js (right): https://codereview.chromium.org/571343002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:362: item.message = strf('COPY_PROGRESS_SUMMARY'); On 2014/09/16 09:01:31, hirono wrote: > Sorry for changing the comment. > But IDS_FILE_BROWSER_COPY_FILE_NAME or IDS_FILE_BROWSER_COPY_ITEMS_REMAINING is > better here. > Done. https://codereview.chromium.org/571343002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:1103: return 'file-operation-from-transfer-controller' + this.taskIdCounter_++; On 2014/09/16 09:01:31, hirono wrote: > This function should generate unique ID among the windows. Done.
https://codereview.chromium.org/571343002/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/file_operation_manager.js (right): https://codereview.chromium.org/571343002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/file_operation_manager.js:1192: sourceEntries, targetEntry, isMove, optTaskId) { nit: The type name should be string= since it is optional. nit: Start the optional variable name with opt_. https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon... https://codereview.chromium.org/571343002/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/file_manager.js (right): https://codereview.chromium.org/571343002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_manager.js:2824: ++i) { nit: Usually i++ in JavaScript. https://codereview.chromium.org/571343002/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/file_transfer_controller.js (right): https://codereview.chromium.org/571343002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:53: * The array of pending task id. id -> ID? http://eow.alc.co.jp/search?q=id https://codereview.chromium.org/571343002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:89: * Task id counter. id -> ID? https://codereview.chromium.org/571343002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:362: item.message = strf('COPY_FILE_NAME', result.entries[0].name || ''); Please use IDS_FILE_BROWSER_COPY_ITEMS_REMAINING for multiple files , and use IDS_FILE_BROWSER_COPY_FILE_NAME for a single file. https://code.google.com/p/chromium/codesearch#chromium/src/ui/file_manager/fi... https://codereview.chromium.org/571343002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:1103: return 'paste-from-transfer-controller' + this.taskIdCounter_++; If the app opens multiple windows, each window generates the ID 'paste-from-transfer-controller0'. The IDs are not unique.
Thank you for your review. https://codereview.chromium.org/571343002/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/file_operation_manager.js (right): https://codereview.chromium.org/571343002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/file_operation_manager.js:1192: sourceEntries, targetEntry, isMove, optTaskId) { On 2014/09/17 03:33:20, hirono wrote: > nit: The type name should be string= since it is optional. > nit: Start the optional variable name with opt_. > https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon... Done. https://codereview.chromium.org/571343002/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/file_manager.js (right): https://codereview.chromium.org/571343002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_manager.js:2824: ++i) { On 2014/09/17 03:33:20, hirono wrote: > nit: Usually i++ in JavaScript. Done. https://codereview.chromium.org/571343002/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/file_transfer_controller.js (right): https://codereview.chromium.org/571343002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:53: * The array of pending task id. On 2014/09/17 03:33:20, hirono wrote: > id -> ID? > http://eow.alc.co.jp/search?q=id Done. https://codereview.chromium.org/571343002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:89: * Task id counter. On 2014/09/17 03:33:20, hirono wrote: > id -> ID? Done. https://codereview.chromium.org/571343002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:362: item.message = strf('COPY_FILE_NAME', result.entries[0].name || ''); On 2014/09/17 03:33:20, hirono wrote: > Please use IDS_FILE_BROWSER_COPY_ITEMS_REMAINING for multiple files , and use > IDS_FILE_BROWSER_COPY_FILE_NAME for a single file. > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/file_manager/fi... Done. https://codereview.chromium.org/571343002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:1103: return 'paste-from-transfer-controller' + this.taskIdCounter_++; On 2014/09/17 03:33:20, hirono wrote: > If the app opens multiple windows, each window generates the ID > 'paste-from-transfer-controller0'. The IDs are not unique. Done.
lgtm. Could you add cancelCallback to the progress item at the separated patch? Thank you!
The CQ bit was checked by iseki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/571343002/60001
Thanks! I file the bug about it and start this bug.
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as cefd7b6ec2a8a00a453a571f767393bfeeb5f9c4
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/fe9b1a17bd8576ce2a42d233d668c469c0379c91 Cr-Commit-Position: refs/heads/master@{#295235} |