Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
174053003:
Task Management - Handle Promises "Then" or "Catch" callback case. (Closed)
|
Created:
6 years, 10 months ago by robliao Modified:
6 years, 8 months ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionTask Management - Handle Promises "Then" or "Catch" callback case.
This CL will be merged into https://codereview.chromium.org/162273002/
when we are ready to submit.
Patch Set 1 #Patch Set 2 : Bug Fix #
Total comments: 3
Patch Set 3 : Rebase to r252941 #Patch Set 4 : CR Feedback #Patch Set 5 : Promise Tracking 2 #
Total comments: 15
Patch Set 6 : Modify Comments #
Total comments: 2
Patch Set 7 : Theoretical Bug Fix and CR Feedback #
Total comments: 13
Patch Set 8 : CR Feedback #Messages
Total messages: 24 (0 generated)
Added a bug fix and ping!
Rebase to r252941
lgtm
https://codereview.chromium.org/174053003/diff/90001/chrome/browser/resources... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/174053003/diff/90001/chrome/browser/resources... chrome/browser/resources/google_now/utility.js:504: this.__promiseId = promiseId; Still kinda confused in general by setting the same id to the promise calling then and the one resulting from then. Really we're tracking promise chains and not individual promises, right? Chained thens would all end up with the same id? Want to make sure I know what's going on here. In this case, can't multiple thens succeed and then one fail and catch is called which contradicts what is said farther down in the file about whenever then is called, catch isn't?
https://codereview.chromium.org/174053003/diff/90001/chrome/browser/resources... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/174053003/diff/90001/chrome/browser/resources... chrome/browser/resources/google_now/utility.js:504: this.__promiseId = promiseId; You're right in the general case (and we'll need to handle this later once we get promise chaining fully working). In our specific case, our thens do not return promises. This means that either all thens or all catches will execute. Once our thens accept promises, then it is possible to have a varying number of thens and catches in a single chain get called. On 2014/02/24 21:55:12, rgustafson wrote: > Still kinda confused in general by setting the same id to the promise calling > then and the one resulting from then. Really we're tracking promise chains and > not individual promises, right? Chained thens would all end up with the same id? > Want to make sure I know what's going on here. > > In this case, can't multiple thens succeed and then one fail and catch is called > which contradicts what is said farther down in the file about whenever then is > called, catch isn't?
https://codereview.chromium.org/174053003/diff/90001/chrome/browser/resources... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/174053003/diff/90001/chrome/browser/resources... chrome/browser/resources/google_now/utility.js:504: this.__promiseId = promiseId; Added a comment about this limitation. On 2014/02/24 22:31:13, robliao wrote: > You're right in the general case (and we'll need to handle this later once we > get promise chaining fully working). > > In our specific case, our thens do not return promises. > > This means that either all thens or all catches will execute. > > Once our thens accept promises, then it is possible to have a varying number of > thens and catches in a single chain get called. > > On 2014/02/24 21:55:12, rgustafson wrote: > > Still kinda confused in general by setting the same id to the promise calling > > then and the one resulting from then. Really we're tracking promise chains and > > not individual promises, right? Chained thens would all end up with the same > id? > > Want to make sure I know what's going on here. > > > > In this case, can't multiple thens succeed and then one fail and catch is > called > > which contradicts what is said farther down in the file about whenever then is > > called, catch isn't? >
lgtm
vadimt: Please provide owner approval for this patch.
On 2014/02/24 23:32:33, robliao wrote: > vadimt: Please provide owner approval for this patch. May be I didn't follow all nuances, so let me ask (before I really dive into the review): is this about the fact that for a promise, exactly 1 callback will be called, either THEN or CATCH? IF this is the case, I'm not super-happy about low-level wrapper infrastructure knowing about promises. I'd prefer you (again, if the above assumption is correct) to create a single callback wrapping THEN and CATCH, with the added boolean parameter telling whether this if THEN or CATCH. With this separate layer taking care of promises, we'd remove this specific knowledge from the wrapper, making layering clearer. wdyt?
The trick here is a promise can have multiple thens and multiple catch
statements like so:
promise
.then(do stuff)
.then(do stuff)
.catch(handle error)
.then(do stuff)
.then(do stuff)
.catch(handle error)
This CL makes a simplifying assumption that catches always happen at the end and
do stuff never returns promises, limiting us to
promise
.then(do stuff)
.then(do stuff)
.catch(handle error)
This is an either or situation: Either all the 'then' callbacks run or all the
'catch' callbacks run.
We can't do a simple wrap callback since we would overcount the number of
callbacks.
Having an object on the promise to handle counting this means we would need to
replicate a lot of the iteration logic the promise already handles.
Since the task manager understands counts, we talk to it in counts, and now we
talk in types of counts so it can determine how many callbacks are outstanding.
There will be future work to figure out the best way to handle the first case,
but since we don't do that now it will be done later.
On 2014/02/24 23:47:17, vadimt wrote:
> On 2014/02/24 23:32:33, robliao wrote:
> > vadimt: Please provide owner approval for this patch.
>
> May be I didn't follow all nuances, so let me ask (before I really dive into
the
> review): is this about the fact that for a promise, exactly 1 callback will be
> called, either THEN or CATCH?
>
> IF this is the case, I'm not super-happy about low-level wrapper
infrastructure
> knowing about promises. I'd prefer you (again, if the above assumption is
> correct) to create a single callback wrapping THEN and CATCH, with the added
> boolean parameter telling whether this if THEN or CATCH. With this separate
> layer taking care of promises, we'd remove this specific knowledge from the
> wrapper, making layering clearer.
>
> wdyt?
Refactored to not change the task manager.
https://codereview.chromium.org/174053003/diff/250001/chrome/browser/resource... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/174053003/diff/250001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:543: callbacksToClear[i](); It's not clear why we need to invoke all callbacks from a function named "clearXXX". Could you add a comment? Or may be, a better name for the function? https://codereview.chromium.org/174053003/diff/250001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:560: maybeCallback, sameTracker, otherTracker) { maybeCallback can be either function of undefined, correct? If so, can we be more specific both in the type of the argument, and with how we check that this is a function (!== undefined). This would help readability. https://codereview.chromium.org/174053003/diff/250001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:562: var handler = wrapper.wrapCallback(function() { Are you calling wrapCallback for each then and catch? If so, how do you avoid getting errors when, say then called, and catch didn't? Is this why you call all callbacks from the clearTracker()? (What I was suggesting was to call wrapCallback once per promise, and pass the result of it to all actual thens and catches.)
https://codereview.chromium.org/174053003/diff/250001/chrome/browser/resource... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/174053003/diff/250001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:543: callbacksToClear[i](); The comment says it clears the tracker in a manner consistent with the task manager. The task manager requires that all callbacks be called, and hence to properly clear it, we have to do so here. On 2014/02/25 22:36:49, vadimt wrote: > It's not clear why we need to invoke all callbacks from a function named > "clearXXX". Could you add a comment? Or may be, a better name for the function? https://codereview.chromium.org/174053003/diff/250001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:560: maybeCallback, sameTracker, otherTracker) { maybeCallback is *, as documented above. It can be a function, a promise, a value, a string, anything! On 2014/02/25 22:36:49, vadimt wrote: > maybeCallback can be either function of undefined, correct? > If so, can we be more specific both in the type of the argument, and with how we > check that this is a function (!== undefined). > This would help readability. https://codereview.chromium.org/174053003/diff/250001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:562: var handler = wrapper.wrapCallback(function() { This is why clearTracker calls all callbacks it the given callback tracker. There are a few reasons why each then/catch gets one callback: 1) I don't have to get into the business of resolving the value of each then/catch. I can leave that to the promise itself. This isn't as simple as returning the previous value. If the resolved value is a promise, for example, we have to do extra work. 2) We only care about if one then or one catch is called. This will correctly clear the other set and we don't have to iterate and rewrap callbacks ourselves for each then. 3) This lets the promise handle invocation of each then as it would normally do. On 2014/02/25 22:36:49, vadimt wrote: > Are you calling wrapCallback for each then and catch? > If so, how do you avoid getting errors when, say then called, and catch didn't? > Is this why you call all callbacks from the clearTracker()? > (What I was suggesting was to call wrapCallback once per promise, and pass the > result of it to all actual thens and catches.)
https://codereview.chromium.org/174053003/diff/250001/chrome/browser/resource... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/174053003/diff/250001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:562: var handler = wrapper.wrapCallback(function() { Also, calling wrapCallback once would also miscount the number of callbacks that could occur. If each promise only had one wrapCallback, but two thens were called, then the number of pendingCallbacks would be incorrect. The then callbacks would also need a wrappedCallback environment, but they can't use our single wrappedCallback since that would clear it from the pending callback. On 2014/02/25 22:43:14, robliao wrote: > This is why clearTracker calls all callbacks it the given callback tracker. > > There are a few reasons why each then/catch gets one callback: > 1) I don't have to get into the business of resolving the value of each > then/catch. I can leave that to the promise itself. This isn't as simple as > returning the previous value. If the resolved value is a promise, for example, > we have to do extra work. > 2) We only care about if one then or one catch is called. This will correctly > clear the other set and we don't have to iterate and rewrap callbacks ourselves > for each then. > 3) This lets the promise handle invocation of each then as it would normally do. > > On 2014/02/25 22:36:49, vadimt wrote: > > Are you calling wrapCallback for each then and catch? > > If so, how do you avoid getting errors when, say then called, and catch > didn't? > > Is this why you call all callbacks from the clearTracker()? > > (What I was suggesting was to call wrapCallback once per promise, and pass the > > result of it to all actual thens and catches.) >
https://codereview.chromium.org/174053003/diff/250001/chrome/browser/resource... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/174053003/diff/250001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:538: tracker.callbacks = []; It's worth adding a comment that this is done to actually prevent these callback from working. https://codereview.chromium.org/174053003/diff/250001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:543: callbacksToClear[i](); Cool, could you add that in the comment? https://codereview.chromium.org/174053003/diff/250001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:563: if (sameTracker.callbacks.length > 0) { If you place this check outside of the wrapped part, you can eliminate using the setTimeout() in clearTracker. https://codereview.chromium.org/174053003/diff/250001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:581: function handleThen(onResolved, onRejected) { 'handle' is not concrete enough. Add? Track? Register?
https://codereview.chromium.org/174053003/diff/250001/chrome/browser/resource... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/174053003/diff/250001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:538: tracker.callbacks = []; On 2014/02/25 23:20:22, vadimt wrote: > It's worth adding a comment that this is done to actually prevent these callback > from working. Done. https://codereview.chromium.org/174053003/diff/250001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:543: callbacksToClear[i](); On 2014/02/25 23:20:22, vadimt wrote: > Cool, could you add that in the comment? Added to the banner comment above. https://codereview.chromium.org/174053003/diff/250001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:563: if (sameTracker.callbacks.length > 0) { This check must be done in the wrapped part. It is critical in no-oping this callback. We don't know at this point if then or catch will be called. clearTracker should be responsible for handling the correctness conditions of calling within a callback. Most everywhere else, we assume that calls are safe within a callback. setTimeout is a common way of deferring execution. On 2014/02/25 23:20:22, vadimt wrote: > If you place this check outside of the wrapped part, you can eliminate using the > setTimeout() in clearTracker. https://codereview.chromium.org/174053003/diff/250001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:581: function handleThen(onResolved, onRejected) { We have precedence for this in Chromium (handleFocus, handleKeyDown, handleClick, etc.). This actually handles the then call. It takes care of registration and forwards it to the real then call. On 2014/02/25 23:20:22, vadimt wrote: > 'handle' is not concrete enough. Add? Track? Register?
https://codereview.chromium.org/174053003/diff/280002/chrome/browser/resource... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/174053003/diff/280002/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:543: setTimeout(function() { Please make sure setTimeout keeps event page loaded OR file a bug fix this. Otherwise, looks good.
https://codereview.chromium.org/174053003/diff/280002/chrome/browser/resource... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/174053003/diff/280002/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:543: setTimeout(function() { Cute trick with the promise. On 2014/02/26 00:31:46, vadimt wrote: > Please make sure setTimeout keeps event page loaded OR file a bug fix this. > Otherwise, looks good.
lgtm https://codereview.chromium.org/174053003/diff/290001/chrome/browser/resource... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/174053003/diff/290001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:543: originalThen.call(Promise.resolve(), function() { WOW!
skare and rgustafson: There have been some significant changes since you lg'ed. Feel free to take a look. This patch will be integrated into https://codereview.chromium.org/162273002/ and then getting on the CQ the late morning of Wed, February 26.
I still haven't actually reviewed for code correctness since everything changed... but submit if you wish. https://codereview.chromium.org/174053003/diff/290001/chrome/browser/resource... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/174053003/diff/290001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:470: * arguments or the 'catch' arguemnts will be processed, never both. arguments https://codereview.chromium.org/174053003/diff/290001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:493: * chained to the previous previous promise's resolution or rejection only one previous https://codereview.chromium.org/174053003/diff/290001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:496: * If p resolves, then the then calls will execute until all the 'then' use "'then' calls" to prevent confusion here https://codereview.chromium.org/174053003/diff/290001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:533: * by no-oping then first and then calling them. no-oping them https://codereview.chromium.org/174053003/diff/290001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:570: if (sameTracker.callbacks) { This check is already done. https://codereview.chromium.org/174053003/diff/290001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:608: // know which set of callbacks will not occur. I don't understand what this is saying at all. Change comment to make it more clear.
https://codereview.chromium.org/174053003/diff/290001/chrome/browser/resource... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/174053003/diff/290001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:470: * arguments or the 'catch' arguemnts will be processed, never both. On 2014/02/26 18:31:04, rgustafson wrote: > arguments Done. https://codereview.chromium.org/174053003/diff/290001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:493: * chained to the previous previous promise's resolution or rejection On 2014/02/26 18:31:04, rgustafson wrote: > only one previous Done. https://codereview.chromium.org/174053003/diff/290001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:496: * If p resolves, then the then calls will execute until all the 'then' On 2014/02/26 18:31:04, rgustafson wrote: > use "'then' calls" to prevent confusion here Done. https://codereview.chromium.org/174053003/diff/290001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:533: * by no-oping then first and then calling them. On 2014/02/26 18:31:04, rgustafson wrote: > no-oping them Done. https://codereview.chromium.org/174053003/diff/290001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:570: if (sameTracker.callbacks) { This check needs to be checked again since it's in a function that could be called later. sameTracker.callbacks can change in the meantime. On 2014/02/26 18:31:04, rgustafson wrote: > This check is already done. https://codereview.chromium.org/174053003/diff/290001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:608: // know which set of callbacks will not occur. On 2014/02/26 18:31:04, rgustafson wrote: > I don't understand what this is saying at all. Change comment to make it more > clear. Done.
Message was sent while issue was closed.
Merged into https://codereview.chromium.org/162273002/ |
