Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
137743002:
Fix moving by dragging and CTRL-X CTRL-V. (Closed)
|
Created:
6 years, 11 months ago by mtomasz Modified:
6 years, 11 months ago Reviewers:
hirono CC:
chromium-reviews, rginda+watch_chromium.org, mtomasz+watch_chromium.org, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix moving by dragging and CTRL-X CTRL-V.
This was caused by a recent regression after refactoring. The volume manager was not available in the background file operation manager.
In this CL, the above problem is resolved by moving calls to the volume manager to the foreground.
TEST=Tested manually. Partly browser tests.
BUG=333180
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245456
Patch Set 1 #Patch Set 2 : Rewritten. #Patch Set 3 : Rewritten again. #
Total comments: 3
Patch Set 4 : Fixed nit and added missing callback() for errors. #Messages
Total messages: 12 (0 generated)
@hirono: PTAL. Thanks.
On 2014/01/14 04:43:25, mtomasz wrote: > @hirono: PTAL. Thanks. Because both the volume manager and the file operation manager are background stuff, it looks a bit strange to me that the volume check is done in the file transfer controller. In the current implementation, the foreground code obtain the volume information from the background, compare them, and take the result back to the background. How about adding the volume manager to the file operation manager instead of doing the check in the foreground?
On 2014/01/14 05:06:17, hirono wrote: > On 2014/01/14 04:43:25, mtomasz wrote: > > @hirono: PTAL. Thanks. > > Because both the volume manager and the file operation manager are background > stuff, > it looks a bit strange to me that the volume check is done in the file transfer > controller. > In the current implementation, the foreground code obtain the volume information > from the background, > compare them, and take the result back to the background. > How about adding the volume manager to the file operation manager instead of > doing the check in the foreground? Makes sense. I've rewritten as you suggested. PTAL. Thanks!
Thanks! Just one more and a nit. https://codereview.chromium.org/137743002/diff/130001/chrome/browser/resource... File chrome/browser/resources/file_manager/background/js/file_operation_manager.js (right): https://codereview.chromium.org/137743002/diff/130001/chrome/browser/resource... chrome/browser/resources/file_manager/background/js/file_operation_manager.js:1202: this.eventRouter_.sendProgressEvent( Can we send the event before the async call? It shows an item in the progress center. https://codereview.chromium.org/137743002/diff/130001/chrome/browser/resource... chrome/browser/resources/file_manager/background/js/file_operation_manager.js:1204: if (this.copyTasks_.length == 1) nit: ===
https://codereview.chromium.org/137743002/diff/130001/chrome/browser/resource... File chrome/browser/resources/file_manager/background/js/file_operation_manager.js (right): https://codereview.chromium.org/137743002/diff/130001/chrome/browser/resource... chrome/browser/resources/file_manager/background/js/file_operation_manager.js:1202: this.eventRouter_.sendProgressEvent( On 2014/01/15 02:29:41, hirono wrote: > Can we send the event before the async call? > It shows an item in the progress center. It seems that it is not safe. If we emit an event before initialization, then I'm getting a JS error in the countRemainingItems() call.
On 2014/01/15 08:01:02, mtomasz wrote: > https://codereview.chromium.org/137743002/diff/130001/chrome/browser/resource... > File > chrome/browser/resources/file_manager/background/js/file_operation_manager.js > (right): > > https://codereview.chromium.org/137743002/diff/130001/chrome/browser/resource... > chrome/browser/resources/file_manager/background/js/file_operation_manager.js:1202: > this.eventRouter_.sendProgressEvent( > On 2014/01/15 02:29:41, hirono wrote: > > Can we send the event before the async call? > > It shows an item in the progress center. > > It seems that it is not safe. If we emit an event before initialization, then > I'm getting a JS error in the countRemainingItems() call. Thank you for checking it. lgtm!
On 2014/01/15 10:04:03, hirono wrote: > On 2014/01/15 08:01:02, mtomasz wrote: > > > https://codereview.chromium.org/137743002/diff/130001/chrome/browser/resource... > > File > > chrome/browser/resources/file_manager/background/js/file_operation_manager.js > > (right): > > > > > https://codereview.chromium.org/137743002/diff/130001/chrome/browser/resource... > > > chrome/browser/resources/file_manager/background/js/file_operation_manager.js:1202: > > this.eventRouter_.sendProgressEvent( > > On 2014/01/15 02:29:41, hirono wrote: > > > Can we send the event before the async call? > > > It shows an item in the progress center. > > > > It seems that it is not safe. If we emit an event before initialization, then > > I'm getting a JS error in the countRemainingItems() call. > > Thank you for checking it. lgtm! I've added missing callback() calls in case of errors. Please look one more time at this change. Thanks!
On 2014/01/16 00:27:00, mtomasz wrote: > On 2014/01/15 10:04:03, hirono wrote: > > On 2014/01/15 08:01:02, mtomasz wrote: > > > > > > https://codereview.chromium.org/137743002/diff/130001/chrome/browser/resource... > > > File > > > > chrome/browser/resources/file_manager/background/js/file_operation_manager.js > > > (right): > > > > > > > > > https://codereview.chromium.org/137743002/diff/130001/chrome/browser/resource... > > > > > > chrome/browser/resources/file_manager/background/js/file_operation_manager.js:1202: > > > this.eventRouter_.sendProgressEvent( > > > On 2014/01/15 02:29:41, hirono wrote: > > > > Can we send the event before the async call? > > > > It shows an item in the progress center. > > > > > > It seems that it is not safe. If we emit an event before initialization, > then > > > I'm getting a JS error in the countRemainingItems() call. > > > > Thank you for checking it. lgtm! > > I've added missing callback() calls in case of errors. Please look one more time > at this change. Thanks! @hirono: Ping.
On 2014/01/17 04:17:11, mtomasz wrote: > On 2014/01/16 00:27:00, mtomasz wrote: > > On 2014/01/15 10:04:03, hirono wrote: > > > On 2014/01/15 08:01:02, mtomasz wrote: > > > > > > > > > > https://codereview.chromium.org/137743002/diff/130001/chrome/browser/resource... > > > > File > > > > > > chrome/browser/resources/file_manager/background/js/file_operation_manager.js > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/137743002/diff/130001/chrome/browser/resource... > > > > > > > > > > chrome/browser/resources/file_manager/background/js/file_operation_manager.js:1202: > > > > this.eventRouter_.sendProgressEvent( > > > > On 2014/01/15 02:29:41, hirono wrote: > > > > > Can we send the event before the async call? > > > > > It shows an item in the progress center. > > > > > > > > It seems that it is not safe. If we emit an event before initialization, > > then > > > > I'm getting a JS error in the countRemainingItems() call. > > > > > > Thank you for checking it. lgtm! > > > > I've added missing callback() calls in case of errors. Please look one more > time > > at this change. Thanks! > > @hirono: Ping. Sorry for missing it. lgtm!
On 2014/01/17 05:01:20, hirono wrote: > On 2014/01/17 04:17:11, mtomasz wrote: > > On 2014/01/16 00:27:00, mtomasz wrote: > > > On 2014/01/15 10:04:03, hirono wrote: > > > > On 2014/01/15 08:01:02, mtomasz wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/137743002/diff/130001/chrome/browser/resource... > > > > > File > > > > > > > > > chrome/browser/resources/file_manager/background/js/file_operation_manager.js > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/137743002/diff/130001/chrome/browser/resource... > > > > > > > > > > > > > > > chrome/browser/resources/file_manager/background/js/file_operation_manager.js:1202: > > > > > this.eventRouter_.sendProgressEvent( > > > > > On 2014/01/15 02:29:41, hirono wrote: > > > > > > Can we send the event before the async call? > > > > > > It shows an item in the progress center. > > > > > > > > > > It seems that it is not safe. If we emit an event before initialization, > > > then > > > > > I'm getting a JS error in the countRemainingItems() call. > > > > > > > > Thank you for checking it. lgtm! > > > > > > I've added missing callback() calls in case of errors. Please look one more > > time > > > at this change. Thanks! > > > > @hirono: Ping. > > Sorry for missing it. lgtm! Thx!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/137743002/190001
Message was sent while issue was closed.
Change committed as 245456 |
