Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(801)

Issue 162273002: Convert Google Now's Authentication Manager to use Promises (Closed)

Created:
6 years, 10 months ago by robliao
Modified:
6 years, 10 months ago
Reviewers:
vadimt, rgustafson, skare_
CC:
chromium-reviews, arv+watch_chromium.org
Visibility:
Public.

Description

Convert 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/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+243 lines, -60 lines) Patch
M chrome/browser/resources/google_now/background.js View 1 2 3 4 5 6 7 5 chunks +5 lines, -20 lines 0 comments Download
M chrome/browser/resources/google_now/background_unittest.gtestjs View 4 chunks +12 lines, -14 lines 0 comments Download
M chrome/browser/resources/google_now/common_test_util.js View 1 2 3 4 5 6 1 chunk +22 lines, -3 lines 0 comments Download
M chrome/browser/resources/google_now/utility.js View 1 2 3 4 5 6 7 8 3 chunks +204 lines, -23 lines 0 comments Download

Messages

Total messages: 31 (0 generated)
robliao
6 years, 10 months ago (2014-02-13 00:38:13 UTC) #1
rgustafson
https://codereview.chromium.org/162273002/diff/1/chrome/browser/resources/google_now/background.js File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/162273002/diff/1/chrome/browser/resources/google_now/background.js#newcode276 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/google_now/utility.js File chrome/browser/resources/google_now/utility.js ...
6 years, 10 months ago (2014-02-13 21:31:40 UTC) #2
robliao
Ah, the perils of Javascript refactoring. https://codereview.chromium.org/162273002/diff/1/chrome/browser/resources/google_now/background.js File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/162273002/diff/1/chrome/browser/resources/google_now/background.js#newcode276 chrome/browser/resources/google_now/background.js:276: authenticationManager.removeToken(token, function() { ...
6 years, 10 months ago (2014-02-13 21:41:32 UTC) #3
robliao
Fixed a bug in promise instrumentation.
6 years, 10 months ago (2014-02-13 23:23:17 UTC) #4
skare_
https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resources/google_now/common_test_util.js File chrome/browser/resources/google_now/common_test_util.js (right): https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resources/google_now/common_test_util.js#newcode115 chrome/browser/resources/google_now/common_test_util.js:115: promiseObj.catch = catchFunc; does promiseObj.catch = function(...) {} work? ...
6 years, 10 months ago (2014-02-13 23:37:03 UTC) #5
robliao
https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resources/google_now/common_test_util.js File chrome/browser/resources/google_now/common_test_util.js (right): https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resources/google_now/common_test_util.js#newcode115 chrome/browser/resources/google_now/common_test_util.js:115: promiseObj.catch = catchFunc; It does, but the named functions ...
6 years, 10 months ago (2014-02-13 23:43:20 UTC) #6
skare_
https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resources/google_now/common_test_util.js File chrome/browser/resources/google_now/common_test_util.js (right): https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resources/google_now/common_test_util.js#newcode103 chrome/browser/resources/google_now/common_test_util.js:103: // Promises use the function name catch to call ...
6 years, 10 months ago (2014-02-13 23:53:52 UTC) #7
robliao
I won't be surprised if "catch" is deprecated during EMCAScript 6 finalization. https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resources/google_now/common_test_util.js File chrome/browser/resources/google_now/common_test_util.js ...
6 years, 10 months ago (2014-02-14 00:05:11 UTC) #8
skare_
https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resources/google_now/common_test_util.js File chrome/browser/resources/google_now/common_test_util.js (right): https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resources/google_now/common_test_util.js#newcode115 chrome/browser/resources/google_now/common_test_util.js:115: promiseObj.catch = catchFunc; On 2014/02/14 00:05:11, robliao wrote: > ...
6 years, 10 months ago (2014-02-14 00:19:25 UTC) #9
robliao
On 2014/02/14 00:19:25, Travis Skare wrote: > https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resources/google_now/common_test_util.js > File chrome/browser/resources/google_now/common_test_util.js (right): > > https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resources/google_now/common_test_util.js#newcode115 ...
6 years, 10 months ago (2014-02-14 00:33:22 UTC) #10
robliao
https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resources/google_now/common_test_util.js File chrome/browser/resources/google_now/common_test_util.js (right): https://codereview.chromium.org/162273002/diff/140001/chrome/browser/resources/google_now/common_test_util.js#newcode115 chrome/browser/resources/google_now/common_test_util.js:115: promiseObj.catch = catchFunc; And done. On 2014/02/14 00:19:26, Travis ...
6 years, 10 months ago (2014-02-14 01:57:04 UTC) #11
skare_
lgtm
6 years, 10 months ago (2014-02-14 02:03:29 UTC) #12
rgustafson
https://codereview.chromium.org/162273002/diff/290001/chrome/browser/resources/google_now/utility.js File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/162273002/diff/290001/chrome/browser/resources/google_now/utility.js#newcode776 chrome/browser/resources/google_now/utility.js:776: // Let Chrome now about a possible problem with ...
6 years, 10 months ago (2014-02-14 02:12:07 UTC) #13
robliao
https://codereview.chromium.org/162273002/diff/290001/chrome/browser/resources/google_now/utility.js File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/162273002/diff/290001/chrome/browser/resources/google_now/utility.js#newcode776 chrome/browser/resources/google_now/utility.js:776: // Let Chrome now about a possible problem with ...
6 years, 10 months ago (2014-02-14 02:28:25 UTC) #14
rgustafson
lgtm
6 years, 10 months ago (2014-02-14 02:32:14 UTC) #15
robliao
vadimt: Please provide owner approval for this CL.
6 years, 10 months ago (2014-02-14 02:33:27 UTC) #16
vadimt
https://codereview.chromium.org/162273002/diff/310002/chrome/browser/resources/google_now/background.js File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/162273002/diff/310002/chrome/browser/resources/google_now/background.js#newcode1088 chrome/browser/resources/google_now/background.js:1088: authenticationManager.isSignedIn(), If isSignedIn() calls reject(), will .then still be ...
6 years, 10 months ago (2014-02-14 20:45:30 UTC) #17
robliao
https://codereview.chromium.org/162273002/diff/310002/chrome/browser/resources/google_now/background.js File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/162273002/diff/310002/chrome/browser/resources/google_now/background.js#newcode1088 chrome/browser/resources/google_now/background.js:1088: authenticationManager.isSignedIn(), Nope. A reject here rejects the whole promise. ...
6 years, 10 months ago (2014-02-14 22:01:48 UTC) #18
vadimt
lgtm https://codereview.chromium.org/162273002/diff/310002/chrome/browser/resources/google_now/background.js File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/162273002/diff/310002/chrome/browser/resources/google_now/background.js#newcode1088 chrome/browser/resources/google_now/background.js:1088: authenticationManager.isSignedIn(), On 2014/02/14 22:01:49, robliao wrote: > Nope. ...
6 years, 10 months ago (2014-02-14 22:05:09 UTC) #19
robliao
The CQ bit was checked by robliao@chromium.org
6 years, 10 months ago (2014-02-14 22:08:47 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/162273002/310002
6 years, 10 months ago (2014-02-14 22:09:24 UTC) #21
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 10 months ago (2014-02-14 22:43:45 UTC) #22
commit-bot: I haz the power
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_presubmit&number=50451
6 years, 10 months ago (2014-02-14 22:43:47 UTC) #23
robliao
Due to known issues with the presubmit regarding Promise.catch, this CL will be direct committed ...
6 years, 10 months ago (2014-02-15 00:00:17 UTC) #24
robliao
Committed patchset #7 manually as r252109 (presubmit successful).
6 years, 10 months ago (2014-02-19 22:20:49 UTC) #25
robliao
A revert of this CL has been created in https://codereview.chromium.org/170283015/ by robliao@chromium.org. The reason for ...
6 years, 10 months ago (2014-02-19 23:17:55 UTC) #26
robliao
Reopening
6 years, 10 months ago (2014-02-19 23:32:57 UTC) #27
robliao
6 years, 10 months ago (2014-02-24 18:27:53 UTC) #28
robliao
Merged in https://codereview.chromium.org/174053003/
6 years, 10 months ago (2014-02-26 21:20:23 UTC) #29
robliao
On 2014/02/26 21:20:23, robliao wrote: > Merged in https://codereview.chromium.org/174053003/ Like before, this CL contains a ...
6 years, 10 months ago (2014-02-26 21:23:01 UTC) #30
robliao
6 years, 10 months ago (2014-02-27 02:16:33 UTC) #31
Message was sent while issue was closed.
Committed patchset #9 manually as r253683 (presubmit successful).

Powered by Google App Engine
This is Rietveld 408576698