|
|
Created:
6 years, 10 months ago by robliao Modified:
6 years, 10 months ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionConvert Google Now's Authentication Manager to use Promises
Also add support for Promises by tracking the "then" and "catch" callbacks.
The Task manager assumes that all callbacks will fire. With promises, this
is no longer the case. Either "then" or "catch" will call back, not always
both.
To account for this, the promise adapter will make sure all callbacks call
back while preserving when 'then' or 'catch' callbacks should execute.
To do this, it wraps all 'then' and 'catch' callbacks that check if
the actual callback should occur. Once a 'then' happens, it updates state
to make 'catch' callbacks a no-op and vice versa. The no-op'ed callbacks
are manually executed to update the task manager state.
BUG=164227
R=rgustafson@chromium.org, skare@chromium.org, vadimt@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=253683
Patch Set 1 #
Total comments: 6
Patch Set 2 : Deal with the Perils of Javascript Refactoring #Patch Set 3 : Fix Bug in Promises #
Total comments: 12
Patch Set 4 : CR Feedback #Patch Set 5 : CR Feedback #
Total comments: 2
Patch Set 6 : CR Feedback #
Total comments: 2
Patch Set 7 : CR Feedback #
Total comments: 3
Patch Set 8 : Rebase to r252941 #Patch Set 9 : Merging in https://codereview.chromium.org/174053003/ #
Messages
Total messages: 31 (0 generated)
https://codereview.chromium.org/162273002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/162273002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background.js:276: authenticationManager.removeToken(token, function() { No more callback. https://codereview.chromium.org/162273002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/utility.js (left): https://codereview.chromium.org/162273002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/utility.js:756: // Let Chrome now about a possible problem with the token. Keep the comment in, but now -> know. It's not immediately obvious why there's a get call after removing it. https://codereview.chromium.org/162273002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/162273002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/utility.js:787: isSignedIn(function(signedIn) { This doesn't have a parameter anymore. Needs more promise.
Ah, the perils of Javascript refactoring. https://codereview.chromium.org/162273002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/162273002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/background.js:276: authenticationManager.removeToken(token, function() { On 2014/02/13 21:31:40, rgustafson wrote: > No more callback. Done. https://codereview.chromium.org/162273002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/utility.js (left): https://codereview.chromium.org/162273002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/utility.js:756: // Let Chrome now about a possible problem with the token. On 2014/02/13 21:31:40, rgustafson wrote: > Keep the comment in, but now -> know. It's not immediately obvious why there's a > get call after removing it. Done. https://codereview.chromium.org/162273002/diff/1/chrome/browser/resources/goo... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/162273002/diff/1/chrome/browser/resources/goo... chrome/browser/resources/google_now/utility.js:787: isSignedIn(function(signedIn) { Indeed! Done. On 2014/02/13 21:31:40, rgustafson wrote: > This doesn't have a parameter anymore. Needs more promise.
Fixed a bug in promise instrumentation.
https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resource... File chrome/browser/resources/google_now/common_test_util.js (right): https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resource... chrome/browser/resources/google_now/common_test_util.js:115: promiseObj.catch = catchFunc; does promiseObj.catch = function(...) {} work? https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resource... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:440: * Add task tracking support to Promises.then. nit: Promises -> Promise here and below?
https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resource... File chrome/browser/resources/google_now/common_test_util.js (right): https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resource... chrome/browser/resources/google_now/common_test_util.js:115: promiseObj.catch = catchFunc; It does, but the named functions follows other non-prototype based 'classes' we have floating around in Google Now. On 2014/02/13 23:37:03, Travis Skare wrote: > does promiseObj.catch = function(...) {} work? https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resource... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:440: * Add task tracking support to Promises.then. On 2014/02/13 23:37:03, Travis Skare wrote: > nit: Promises -> Promise here and below? Done.
https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resource... File chrome/browser/resources/google_now/common_test_util.js (right): https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resource... chrome/browser/resources/google_now/common_test_util.js:103: // Promises use the function name catch to call back error handlers. (no action required, just a suggestion based on first reading) maybe put "catch" in quotes, like: // Promises call "catch" to ... // Promises use the function named "catch" https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resource... chrome/browser/resources/google_now/common_test_util.js:104: // We can't use catch since function or variable names cannot use the word or just "We can't use 'catch' since it's reserved" https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resource... chrome/browser/resources/google_now/common_test_util.js:115: promiseObj.catch = catchFunc; On 2014/02/13 23:43:20, robliao wrote: > It does, but the named functions follows other non-prototype based 'classes' we > have floating around in Google Now. > On 2014/02/13 23:37:03, Travis Skare wrote: > > does promiseObj.catch = function(...) {} work? > return {then: then, isPromise: true, catch: catchFunc} ?
I won't be surprised if "catch" is deprecated during EMCAScript 6 finalization. https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resource... File chrome/browser/resources/google_now/common_test_util.js (right): https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resource... chrome/browser/resources/google_now/common_test_util.js:103: // Promises use the function name catch to call back error handlers. On 2014/02/13 23:53:52, Travis Skare wrote: > (no action required, just a suggestion based on first reading) > maybe put "catch" in quotes, like: > // Promises call "catch" to ... > // Promises use the function named "catch" Done. https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resource... chrome/browser/resources/google_now/common_test_util.js:104: // We can't use catch since function or variable names cannot use the word On 2014/02/13 23:53:52, Travis Skare wrote: > or just "We can't use 'catch' since it's reserved" This isn't strictly true, since it works fine for foo.catch. Object initialization, function names, and variable names are the few instances I've found thus far that do not allow this. Variable references, function calls, on the other hand, are okay. https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resource... chrome/browser/resources/google_now/common_test_util.js:115: promiseObj.catch = catchFunc; On 2014/02/13 23:53:52, Travis Skare wrote: > On 2014/02/13 23:43:20, robliao wrote: > > It does, but the named functions follows other non-prototype based 'classes' > we > > have floating around in Google Now. > > On 2014/02/13 23:37:03, Travis Skare wrote: > > > does promiseObj.catch = function(...) {} work? > > > > return {then: then, isPromise: true, catch: catchFunc} ? This won't work since "catch" is reserved in object initialization.
https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resource... File chrome/browser/resources/google_now/common_test_util.js (right): https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resource... chrome/browser/resources/google_now/common_test_util.js:115: promiseObj.catch = catchFunc; On 2014/02/14 00:05:11, robliao wrote: > On 2014/02/13 23:53:52, Travis Skare wrote: > > On 2014/02/13 23:43:20, robliao wrote: > > > It does, but the named functions follows other non-prototype based 'classes' > > we > > > have floating around in Google Now. > > > On 2014/02/13 23:37:03, Travis Skare wrote: > > > > does promiseObj.catch = function(...) {} work? > > > > > > > return {then: then, isPromise: true, catch: catchFunc} ? > This won't work since "catch" is reserved in object initialization. will catch* you in person since I might have constructed the full parallel example wrong, but this seems ok? x = function() { return {catch: function(){return 'hi'} }}; (new x).catch() *sincere apologies
On 2014/02/14 00:19:25, Travis Skare wrote: > https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resource... > File chrome/browser/resources/google_now/common_test_util.js (right): > > https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resource... > chrome/browser/resources/google_now/common_test_util.js:115: promiseObj.catch = > catchFunc; > On 2014/02/14 00:05:11, robliao wrote: > > On 2014/02/13 23:53:52, Travis Skare wrote: > > > On 2014/02/13 23:43:20, robliao wrote: > > > > It does, but the named functions follows other non-prototype based > 'classes' > > > we > > > > have floating around in Google Now. > > > > On 2014/02/13 23:37:03, Travis Skare wrote: > > > > > does promiseObj.catch = function(...) {} work? > > > > > > > > > > return {then: then, isPromise: true, catch: catchFunc} ? > > This won't work since "catch" is reserved in object initialization. > > will catch* you in person since I might have constructed the full parallel > example wrong, but this seems ok? > x = function() { return {catch: function(){return 'hi'} }}; (new x).catch() > > *sincere apologies I figured out what's wrong. I was validating the expressions in the developer console. > {} undefined > {a: 1, b: 2, catch: 3} SyntaxError: Unexpected token : > var foo = {a: 1, b: 2, catch: 3} undefined > foo Object {a: 1, b: 2, catch: 3}. Will fix when I get back.
https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resource... File chrome/browser/resources/google_now/common_test_util.js (right): https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resource... chrome/browser/resources/google_now/common_test_util.js:115: promiseObj.catch = catchFunc; And done. On 2014/02/14 00:19:26, Travis Skare wrote: > On 2014/02/14 00:05:11, robliao wrote: > > On 2014/02/13 23:53:52, Travis Skare wrote: > > > On 2014/02/13 23:43:20, robliao wrote: > > > > It does, but the named functions follows other non-prototype based > 'classes' > > > we > > > > have floating around in Google Now. > > > > On 2014/02/13 23:37:03, Travis Skare wrote: > > > > > does promiseObj.catch = function(...) {} work? > > > > > > > > > > return {then: then, isPromise: true, catch: catchFunc} ? > > This won't work since "catch" is reserved in object initialization. > > will catch* you in person since I might have constructed the full parallel > example wrong, but this seems ok? > x = function() { return {catch: function(){return 'hi'} }}; (new x).catch() > > *sincere apologies >
lgtm
https://codereview.chromium.org/162273002/diff/290001/chrome/browser/resource... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/162273002/diff/290001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:776: // Let Chrome now about a possible problem with the token. know https://codereview.chromium.org/162273002/diff/340001/chrome/browser/resource... File chrome/browser/resources/google_now/common_test_util.js (right): https://codereview.chromium.org/162273002/diff/340001/chrome/browser/resource... chrome/browser/resources/google_now/common_test_util.js:92: result = asyncResult; asyncFailureResult?
https://codereview.chromium.org/162273002/diff/290001/chrome/browser/resource... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/162273002/diff/290001/chrome/browser/resource... chrome/browser/resources/google_now/utility.js:776: // Let Chrome now about a possible problem with the token. On 2014/02/14 02:12:07, rgustafson wrote: > know Done. https://codereview.chromium.org/162273002/diff/340001/chrome/browser/resource... File chrome/browser/resources/google_now/common_test_util.js (right): https://codereview.chromium.org/162273002/diff/340001/chrome/browser/resource... chrome/browser/resources/google_now/common_test_util.js:92: result = asyncResult; On 2014/02/14 02:12:07, rgustafson wrote: > asyncFailureResult? Done.
lgtm
vadimt: Please provide owner approval for this CL.
https://codereview.chromium.org/162273002/diff/310002/chrome/browser/resource... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/162273002/diff/310002/chrome/browser/resource... chrome/browser/resources/google_now/background.js:1088: authenticationManager.isSignedIn(), If isSignedIn() calls reject(), will .then still be called?
https://codereview.chromium.org/162273002/diff/310002/chrome/browser/resource... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/162273002/diff/310002/chrome/browser/resource... chrome/browser/resources/google_now/background.js:1088: authenticationManager.isSignedIn(), Nope. A reject here rejects the whole promise. The 'then' portions will not run. Since isSignedIn always resolves, it can't reject. On 2014/02/14 20:45:30, vadimt wrote: > If isSignedIn() calls reject(), will .then still be called?
lgtm https://codereview.chromium.org/162273002/diff/310002/chrome/browser/resource... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/162273002/diff/310002/chrome/browser/resource... chrome/browser/resources/google_now/background.js:1088: authenticationManager.isSignedIn(), On 2014/02/14 22:01:49, robliao wrote: > Nope. A reject here rejects the whole promise. The 'then' portions will not run. > > Since isSignedIn always resolves, it can't reject. > > On 2014/02/14 20:45:30, vadimt wrote: > > If isSignedIn() calls reject(), will .then still be called? > I see. It's getAuthToken() who can reject.
The CQ bit was checked by robliao@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/162273002/310002
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
Due to known issues with the presubmit regarding Promise.catch, this CL will be direct committed after the trybots finish.
Message was sent while issue was closed.
Committed patchset #7 manually as r252109 (presubmit successful).
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/170283015/ by robliao@chromium.org. The reason for reverting is: Promise.catch may not always be called back, breaking an assumption about the task tracking system..
Reopening
On 2014/02/26 21:20:23, robliao wrote: > Merged in https://codereview.chromium.org/174053003/ Like before, this CL contains a known presubmit bug due to Promises.catch: See the JavaScript style guide at http://www.chromium.org/developers/web-development-style-guide#TOC-JavaScript and if you have any feedback about the JavaScript PRESUBMIT check, contact tbreisacher@chromium.org or dbeam@chromium.org ** Presubmit Warnings ** Found JavaScript style violations in chrome\browser\resources\google_now\background.js: line 285: E0002: Missing space before "(" }).catch(function() { ^ Once the trybots succeed, this CL will be manually submitted.
Message was sent while issue was closed.
Committed patchset #9 manually as r253683 (presubmit successful). |