Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
136783003:
Files.app; Serializes event handler calls for particular URLs. (Closed)
|
Created:
6 years, 10 months ago by hirono Modified:
6 years, 10 months ago Reviewers:
mtomasz 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. |
DescriptionFiles.app; Serializes event handler calls for particular URLs.
Previously DriveSyncHandler#updateItem_ is called for 'in_progress' events and
DriveSyncHandler#removeItem_ is called for 'completed' events.
We should do updateItem_ and removeItem_ in this order but sometimes removeItem_
calls before updateItem_ is done becuase updateItem_ needs asynchronous
opeartion.
This CL serializes function calls of updateItem_ and removeItem_ by using
Promise.
BUG=339046
TEST=manually
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=247946
Patch Set 1 : #
Total comments: 10
Patch Set 2 : Fixed. #
Total comments: 2
Patch Set 3 : Fixed. #
Total comments: 2
Patch Set 4 : Fixed. #Messages
Total messages: 12 (0 generated)
PTAL the CL? Thanks!
some nits https://codereview.chromium.org/136783003/diff/20001/chrome/browser/resources... File chrome/browser/resources/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/136783003/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/background/js/drive_sync_handler.js:29: * Progressing file names. nit: Please update this comment, eg. that this is a map from a url to a promise of creating an item representing the sync operation. https://codereview.chromium.org/136783003/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/background/js/drive_sync_handler.js:30: * @type {Object.<string, Promise>} Map a file URL and promise. Hm. This solution is cool, but I'm a little bit concerned if the extra complexity is needed here. Another solution would be creating a big class-wide queue, and wrap updateItem and removeItem with it. This is very simple and often used. In Java this pattern is built in as 'synchronized methods'. But, I don't have very strong feelings here, so feel free to go with Premise if you prefer. WDYT? https://codereview.chromium.org/136783003/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/background/js/drive_sync_handler.js:118: }.bind(this)); Please print a warning to console that resolving failed. https://codereview.chromium.org/136783003/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/background/js/drive_sync_handler.js:141: console.error(error); Can you please handle this catch in updateItem_, where the resolving takes place, instead?
Thanks! https://codereview.chromium.org/136783003/diff/20001/chrome/browser/resources... File chrome/browser/resources/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/136783003/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/background/js/drive_sync_handler.js:29: * Progressing file names. On 2014/01/29 10:34:29, mtomasz wrote: > nit: Please update this comment, eg. that this is a map from a url to a promise > of creating an item representing the sync operation. Done. https://codereview.chromium.org/136783003/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/background/js/drive_sync_handler.js:30: * @type {Object.<string, Promise>} Map a file URL and promise. On 2014/01/29 10:34:29, mtomasz wrote: > Hm. This solution is cool, but I'm a little bit concerned if the extra > complexity is needed here. > > Another solution would be creating a big class-wide queue, and wrap updateItem > and removeItem with it. This is very simple and often used. In Java this pattern > is built in as 'synchronized methods'. > > But, I don't have very strong feelings here, so feel free to go with Premise if > you prefer. > > WDYT? Just do it simple! Thanks! https://codereview.chromium.org/136783003/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/background/js/drive_sync_handler.js:118: }.bind(this)); On 2014/01/29 10:34:29, mtomasz wrote: > Please print a warning to console that resolving failed. The error case of webkitResolveLocalFileSystemURL was handled by Promise, in this case. The constructor of promise takes a function having two arguments: onSuccess and onFailure. If onSuccess is called, the promise proceeds the following instructions, and if onFailure is called, the promise skips the following instructions until the catch method is called. https://codereview.chromium.org/136783003/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/background/js/drive_sync_handler.js:141: console.error(error); On 2014/01/29 10:34:29, mtomasz wrote: > Can you please handle this catch in updateItem_, where the resolving takes > place, instead? The position of the catch method was important because if resolving the entry failed at first, we should skip the following instructions until the sync completes. [Success] updateItem (with resolving URL) updateItem updateItem ... removeItem [Failure] updateItem (with resolving URL) updateItem - skipped updateItem - skipped ... - skipped removeItem - catch the error and output it. We usually add error checks at the head of functions when using AsyncUtil.Queue. Promise did the same thing automatically.
lgtm with nit https://codereview.chromium.org/136783003/diff/20001/chrome/browser/resources... File chrome/browser/resources/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/136783003/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/background/js/drive_sync_handler.js:118: }.bind(this)); On 2014/01/30 04:51:47, hirono wrote: > On 2014/01/29 10:34:29, mtomasz wrote: > > Please print a warning to console that resolving failed. > > The error case of webkitResolveLocalFileSystemURL was handled by Promise, in > this case. The constructor of promise takes a function having two arguments: > onSuccess and onFailure. > If onSuccess is called, the promise proceeds the following instructions, and if > onFailure is called, the promise skips the following instructions until the > catch method is called. My concern was, that the error in resolving was shown after removing, what is a little bit too late. I think we should report errors as close to the root cause as possible. https://codereview.chromium.org/136783003/diff/20001/chrome/browser/resources... chrome/browser/resources/file_manager/background/js/drive_sync_handler.js:141: console.error(error); On 2014/01/30 04:51:47, hirono wrote: > On 2014/01/29 10:34:29, mtomasz wrote: > > Can you please handle this catch in updateItem_, where the resolving takes > > place, instead? > > The position of the catch method was important because if resolving the entry > failed at first, we should skip the following instructions until the sync > completes. > > [Success] > updateItem (with resolving URL) > updateItem > updateItem > ... > removeItem > > [Failure] > updateItem (with resolving URL) > updateItem - skipped > updateItem - skipped > ... - skipped > removeItem - catch the error and output it. > > We usually add error checks at the head of functions when using AsyncUtil.Queue. > Promise did the same thing automatically. Interesting. What would happen if we called catch() in updateItem_? Like. new Promise().then(fun() { success }).catch(fun() { error }). Would removeItem() misbehave in such situation? The Pomise class is super cool, and it seems that we could use it in several places to simplify things, like that maybeFinish() methods with a callback counter inside, or AsyncUtil.Group() when dependencies are not needed. But on the other hand, the produced code can be sometimes difficult to understand. I need to read more about it. https://codereview.chromium.org/136783003/diff/60001/chrome/browser/resources... File chrome/browser/resources/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/136783003/diff/60001/chrome/browser/resources... chrome/browser/resources/file_manager/background/js/drive_sync_handler.js:112: this.queue_.run(function(callback) { nit: We need to resolve the entry only when creating. Can we reorganize the code to avoid resolving when it is not needed? If it makes the code more complex, I'm fine with the current solution.
> My concern was, that the error in resolving was shown after removing, what is
a
> little bit too late. I think we should report errors as close to the root
cause
> as possible.
I got the point. I was not care about it, but I agree with you.
And the problem is gone in current code.
> Interesting. What would happen if we called catch() in updateItem_? Like.
> new Promise().then(fun() { success }).catch(fun() { error }). Would
removeItem()
> misbehave in such situation?
In that case, the following then callbacks are invoked without an argument
because the
catch method get the promise back to the normal state but we cannot return the
correct entry at the catch callback.
this.items_[url] = new Promise().then(fun() { success }).catch(fun() { error });
// this.items_[url] would get back to the normal state.
But the catch method does not change 'this' object.
So we can write like this to output the error immediately.
this.items_[url] = new Promise().then(fun() { success });
this.items_[url].catch(fun() { error });
// The returned promise would get back to the normal state.
// But this.items_[url] itself is still error state.
// The following instructions to this.items_[url] would be skipped until catch.
Thanks! https://codereview.chromium.org/136783003/diff/60001/chrome/browser/resources... File chrome/browser/resources/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/136783003/diff/60001/chrome/browser/resources... chrome/browser/resources/file_manager/background/js/drive_sync_handler.js:112: this.queue_.run(function(callback) { On 2014/01/30 05:11:13, mtomasz wrote: > nit: We need to resolve the entry only when creating. Can we reorganize the code > to avoid resolving when it is not needed? If it makes the code more complex, I'm > fine with the current solution. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hirono@chromium.org/136783003/80001
LGTM with a tiny nit. Thx1 https://codereview.chromium.org/136783003/diff/80001/chrome/browser/resources... File chrome/browser/resources/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/136783003/diff/80001/chrome/browser/resources... chrome/browser/resources/file_manager/background/js/drive_sync_handler.js:124: console.warn('Resolving URL ' + status.fileUrl + 'is failed: ', error); nit: + 'is -> + ' is
CQ bit was unchecked on CL. Ignoring.
Thanks! https://codereview.chromium.org/136783003/diff/80001/chrome/browser/resources... File chrome/browser/resources/file_manager/background/js/drive_sync_handler.js (right): https://codereview.chromium.org/136783003/diff/80001/chrome/browser/resources... chrome/browser/resources/file_manager/background/js/drive_sync_handler.js:124: console.warn('Resolving URL ' + status.fileUrl + 'is failed: ', error); On 2014/01/30 06:38:06, mtomasz wrote: > nit: + 'is -> + ' is Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hirono@chromium.org/136783003/100001
Message was sent while issue was closed.
Change committed as 247946 |
