Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
609223002:
Add filter to prevent move operation in the same directory. (Closed)
DescriptionAdd filter to prevent move operation in the same directory.
BUG=418014
TEST=manually
1. Open Files.app. Put a file name "test.txt" under /Downloads.
2. Start dragging the file, and drop to the blank area of the same
directory (/Downloads).
3. Confirm to not start move operation.
Committed: https://crrev.com/55f3eb10a5eb1e7a933607687fa036f57d634198
Cr-Commit-Position: refs/heads/master@{#297354}
Patch Set 1 #
Total comments: 10
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #
Total comments: 12
Patch Set 4 : #
Total comments: 4
Patch Set 5 : #
Total comments: 4
Patch Set 6 : #
Total comments: 12
Patch Set 7 : #
Total comments: 2
Patch Set 8 : #
Messages
Total messages: 22 (2 generated)
iseki@chromium.org changed reviewers: + hirono@chromium.org
Please take a look.
https://codereview.chromium.org/609223002/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/foreground/js/file_transfer_controller.js (right): https://codereview.chromium.org/609223002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:359: var url = result.entries[i].toURL().slice(0, destinationUrl.length); Could you handle the following case? Dest: /A/B Source: /A/BB/C https://codereview.chromium.org/609223002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:366: if (entries.length == 0) We usually use ===. https://codereview.chromium.org/609223002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:372: item.type = ProgressItemType.COPY; Is it OK to use ProgressItemType.COPY for toMove == true case? https://codereview.chromium.org/609223002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:374: item.message = strf('COPY_FILE_NAME', result.entries[0].name); Please update result.entries https://codereview.chromium.org/609223002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:376: item.message = strf('COPY_ITEMS_REMAINING', result.entries.length); ditto.
Thank you for your review :) https://codereview.chromium.org/609223002/diff/1/ui/file_manager/file_manager... File ui/file_manager/file_manager/foreground/js/file_transfer_controller.js (right): https://codereview.chromium.org/609223002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:359: var url = result.entries[i].toURL().slice(0, destinationUrl.length); On 2014/09/29 04:42:10, hirono wrote: > Could you handle the following case? > > Dest: /A/B > Source: /A/BB/C Done. https://codereview.chromium.org/609223002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:366: if (entries.length == 0) On 2014/09/29 04:42:10, hirono wrote: > We usually use ===. Done. https://codereview.chromium.org/609223002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:372: item.type = ProgressItemType.COPY; On 2014/09/29 04:42:10, hirono wrote: > Is it OK to use ProgressItemType.COPY for toMove == true case? Done. https://codereview.chromium.org/609223002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:374: item.message = strf('COPY_FILE_NAME', result.entries[0].name); On 2014/09/29 04:42:10, hirono wrote: > Please update result.entries Done. https://codereview.chromium.org/609223002/diff/1/ui/file_manager/file_manager... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:376: item.message = strf('COPY_ITEMS_REMAINING', result.entries.length); On 2014/09/29 04:42:10, hirono wrote: > ditto. Done.
https://codereview.chromium.org/609223002/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/file_transfer_controller.js (right): https://codereview.chromium.org/609223002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:358: for (var i = 0; i < result.entries.length; ++i) { nit: i++ for consistency. https://codereview.chromium.org/609223002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:361: if (url.isDirectory) 1. result.entries[i].isDirectory? 2. Do we need to check if it is directory or not? https://codereview.chromium.org/609223002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:377: if (toMove) { How about combining the if statement with the previous one. https://codereview.chromium.org/609223002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:385: if (result.entries.length === 1) It works, but please use entries instead of result.entries, because we uses entries at the following lines.
Thank you for your review! https://codereview.chromium.org/609223002/diff/20001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/file_transfer_controller.js (right): https://codereview.chromium.org/609223002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:358: for (var i = 0; i < result.entries.length; ++i) { On 2014/09/29 07:11:43, hirono wrote: > nit: i++ for consistency. Done. https://codereview.chromium.org/609223002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:361: if (url.isDirectory) On 2014/09/29 07:11:43, hirono wrote: > 1. result.entries[i].isDirectory? > > 2. Do we need to check if it is directory or not? Done. https://codereview.chromium.org/609223002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:377: if (toMove) { On 2014/09/29 07:11:43, hirono wrote: > How about combining the if statement with the previous one. Done. https://codereview.chromium.org/609223002/diff/20001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:385: if (result.entries.length === 1) On 2014/09/29 07:11:43, hirono wrote: > It works, but please use entries instead of result.entries, because we uses > entries at the following lines. Done.
https://codereview.chromium.org/609223002/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/file_transfer_controller.js (right): https://codereview.chromium.org/609223002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:350: var entries = new Array(); Please use [] instead of Array constructor. https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon... https://codereview.chromium.org/609223002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:354: util.URLsToEntries(sourceURLs).then(function(result) { Maybe we can filter out sourceURLs directly before resolving entries. https://codereview.chromium.org/609223002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:376: item.message = strf('COPY_FILE_NAME', entries[0].name); MOVE_FILE_NAME https://codereview.chromium.org/609223002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:378: item.message = strf('COPY_ITEMS_REMAINING', entries.length); ditto. https://codereview.chromium.org/609223002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:382: item.message = strf('MOVE_FILE_NAME', entries[0].name); COPY_FILE_NAME https://codereview.chromium.org/609223002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:384: item.message = strf('MOVE_ITEMS_REMAINING', entries.length); ditto.
Thank you for your review :D https://codereview.chromium.org/609223002/diff/40001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/file_transfer_controller.js (right): https://codereview.chromium.org/609223002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:350: var entries = new Array(); On 2014/09/29 07:30:12, hirono wrote: > Please use [] instead of Array constructor. > https://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showon... Done. https://codereview.chromium.org/609223002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:354: util.URLsToEntries(sourceURLs).then(function(result) { On 2014/09/29 07:30:12, hirono wrote: > Maybe we can filter out sourceURLs directly before resolving entries. Done. https://codereview.chromium.org/609223002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:376: item.message = strf('COPY_FILE_NAME', entries[0].name); On 2014/09/29 07:30:12, hirono wrote: > MOVE_FILE_NAME Done. https://codereview.chromium.org/609223002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:378: item.message = strf('COPY_ITEMS_REMAINING', entries.length); On 2014/09/29 07:30:12, hirono wrote: > ditto. Done. https://codereview.chromium.org/609223002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:382: item.message = strf('MOVE_FILE_NAME', entries[0].name); On 2014/09/29 07:30:12, hirono wrote: > COPY_FILE_NAME Done. https://codereview.chromium.org/609223002/diff/40001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:384: item.message = strf('MOVE_ITEMS_REMAINING', entries.length); On 2014/09/29 07:30:12, hirono wrote: > ditto. Done.
Sorry, I didn't upload the patch.
https://codereview.chromium.org/609223002/diff/60001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/file_transfer_controller.js (right): https://codereview.chromium.org/609223002/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:354: var filteredSourceURLs = []; Could you share the logic with FileOperationManager? The filtering procedures should not be out of sync. https://codereview.chromium.org/609223002/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:371: return; Just returning from Promise chain does not cancel following chain. Please use return Promise.reject('ABORT'); as well as the line #397.
Thank you for your review :) https://codereview.chromium.org/609223002/diff/60001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/file_transfer_controller.js (right): https://codereview.chromium.org/609223002/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:354: var filteredSourceURLs = []; On 2014/09/29 08:45:04, hirono wrote: > Could you share the logic with FileOperationManager? > The filtering procedures should not be out of sync. Done. https://codereview.chromium.org/609223002/diff/60001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:371: return; On 2014/09/29 08:45:04, hirono wrote: > Just returning from Promise chain does not cancel following chain. Please use > return Promise.reject('ABORT'); as well as the line #397. Done.
https://codereview.chromium.org/609223002/diff/80001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/file_operation_manager.js (right): https://codereview.chromium.org/609223002/diff/80001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/file_operation_manager.js:1200: return Promise.all(promises).then(concatArrays); Promise.all returns rejected promise when even one promise in the array is rejected. https://codereview.chromium.org/609223002/diff/80001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/file_transfer_controller.js (right): https://codereview.chromium.org/609223002/diff/80001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:357: result.entries, destinationEntry, toMove); Indent should be 4 from the head of 'return' at the previous line.
Thank you for your review:) https://codereview.chromium.org/609223002/diff/80001/ui/file_manager/file_man... File ui/file_manager/file_manager/background/js/file_operation_manager.js (right): https://codereview.chromium.org/609223002/diff/80001/ui/file_manager/file_man... ui/file_manager/file_manager/background/js/file_operation_manager.js:1200: return Promise.all(promises).then(concatArrays); On 2014/09/29 12:05:29, hirono wrote: > Promise.all returns rejected promise when even one promise in the array is > rejected. Done. https://codereview.chromium.org/609223002/diff/80001/ui/file_manager/file_man... File ui/file_manager/file_manager/foreground/js/file_transfer_controller.js (right): https://codereview.chromium.org/609223002/diff/80001/ui/file_manager/file_man... ui/file_manager/file_manager/foreground/js/file_transfer_controller.js:357: result.entries, destinationEntry, toMove); On 2014/09/29 12:05:29, hirono wrote: > Indent should be 4 from the head of 'return' at the previous line. Done.
https://codereview.chromium.org/609223002/diff/100001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/file_operation_manager.js (right): https://codereview.chromium.org/609223002/diff/100001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_manager.js:1181: * Filter the entry in the same directory nit: Filter -> Filters We usually add 's' to function comments. https://codereview.chromium.org/609223002/diff/100001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_manager.js:1187: * if the operation is "copy") false. Please add @return {Promise} and its description. https://codereview.chromium.org/609223002/diff/100001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_manager.js:1195: return arrays.filter(function(element) { return element; }); nit: Please do "return !!element;". It explicitly shows the function returns a boolean value. https://codereview.chromium.org/609223002/diff/100001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_manager.js:1209: resolve(); nit: Please do "resolve(null);" to show the promise fulfilled with "Entry" type (it's nullable). https://codereview.chromium.org/609223002/diff/100001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_manager.js:1212: return entry; This 'then' call is needed? https://codereview.chromium.org/609223002/diff/100001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_manager.js:1241: }.bind(this)); Please add .catch(function(error) { console.error(error.stack || error); }); Exceptions thrown in Promise are never reported so we need to call console.error explicitly.
Thank you for your review :D https://codereview.chromium.org/609223002/diff/100001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/file_operation_manager.js (right): https://codereview.chromium.org/609223002/diff/100001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_manager.js:1181: * Filter the entry in the same directory On 2014/09/30 01:51:55, hirono wrote: > nit: Filter -> Filters > We usually add 's' to function comments. Done. https://codereview.chromium.org/609223002/diff/100001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_manager.js:1187: * if the operation is "copy") false. On 2014/09/30 01:51:55, hirono wrote: > Please add @return {Promise} and its description. Done. https://codereview.chromium.org/609223002/diff/100001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_manager.js:1195: return arrays.filter(function(element) { return element; }); On 2014/09/30 01:51:55, hirono wrote: > nit: Please do "return !!element;". It explicitly shows the function returns a > boolean value. Done. https://codereview.chromium.org/609223002/diff/100001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_manager.js:1209: resolve(); On 2014/09/30 01:51:55, hirono wrote: > nit: Please do "resolve(null);" to show the promise fulfilled with "Entry" type > (it's nullable). Done. https://codereview.chromium.org/609223002/diff/100001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_manager.js:1212: return entry; On 2014/09/30 01:51:55, hirono wrote: > This 'then' call is needed? Done. https://codereview.chromium.org/609223002/diff/100001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_manager.js:1241: }.bind(this)); On 2014/09/30 01:51:55, hirono wrote: > Please add > > .catch(function(error) { > console.error(error.stack || error); > }); > > Exceptions thrown in Promise are never reported so we need to call console.error > explicitly. Done.
Last one! https://codereview.chromium.org/609223002/diff/120001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/file_operation_manager.js (right): https://codereview.chromium.org/609223002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_manager.js:1207: entry.getParent(function(inParentEntry) { getParent takes error callback as the second argument. Please also filter out the entry for the error case.
Thank you for your review! https://codereview.chromium.org/609223002/diff/120001/ui/file_manager/file_ma... File ui/file_manager/file_manager/background/js/file_operation_manager.js (right): https://codereview.chromium.org/609223002/diff/120001/ui/file_manager/file_ma... ui/file_manager/file_manager/background/js/file_operation_manager.js:1207: entry.getParent(function(inParentEntry) { On 2014/09/30 02:36:10, hirono wrote: > getParent takes error callback as the second argument. Please also filter out > the entry for the error case. Done.
lgtm!
The CQ bit was checked by iseki@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/609223002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as 6df9a50f34eb0eb8384d0d478d8dfe971fea5081
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/55f3eb10a5eb1e7a933607687fa036f57d634198 Cr-Commit-Position: refs/heads/master@{#297354} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
