Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
21235008:
Authentication Manager for Google Now (Closed)
|
Created:
7 years, 4 months ago by robliao Modified:
7 years, 4 months ago CC:
chromium-reviews, arv+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@GeoSM-Diag Visibility:
Public. |
DescriptionAuthentication Manager for Google Now
There is currently no way to observe the sign-in state of the user in Chrome.
To work around this, the new authentication manager will poll for the
sign in state and notify any listeners about a change in the sign in state.
Calls for sign in state should correspondingly go through the authentication
manager since a check may already be in flight during a call to getAuthToken.
BUG=164227
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215115
Patch Set 1 #
Total comments: 22
Patch Set 2 : CR Feedback #Patch Set 3 : CR Feedback #
Total comments: 4
Patch Set 4 : Synced #Patch Set 5 : CR Feedback #
Total comments: 2
Patch Set 6 : Use undefined #
Total comments: 4
Patch Set 7 : Fixes #
Messages
Total messages: 16 (0 generated)
https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:196: function setAuthorization(request, callbackBoolean) { Why not move the whole setAuthorization into authenticationManager? https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility.js:286: instrumentApiFunction(chrome.identity, 'getAuthToken', 1); We instrument only functions with callbacks that are directly called by Chrome. For these ones, the callbacks will be invoked in the context of already instrumented Chrome callbacks. https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility.js:421: * @return {object} The Authentication Manager interface. {Object} https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility.js:430: var isSignedInRequests = []; Do we queue them for perf reasons? If so, no need to do this. I checked with identity guys: this API has great performance, and no caching of grouping is necessary. https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility.js:434: * @param {function(boolean, string)} onSuccess Called on completion. It's called even when getAuthToken fails, therefore, it cannot be called onSuccess. https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility.js:444: queuedCallback(signedIn, token); It would be enough to simply pass token. If it's undefined, we are not signed in. https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility.js:467: var listeners = []; Please add a TODO item to get rid of this and implement a chrome API instead. https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility.js:481: var lastReturnedSignedInState; Please explicitly assign null. No reasons to use undefined. https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility.js:485: (lastReturnedSignedInState != undefined)) { Please use !== to not deal with JS strangeness.
https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/background.js (right): https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/background.js:196: function setAuthorization(request, callbackBoolean) { I wanted to keep the HTTP portion out of the identity wrapper. Keeping it separate made the most sense. This also allows us to easily go back to chrome.identity should we get a notification mechanism from them. On 2013/07/30 23:27:23, vadimt wrote: > Why not move the whole setAuthorization into authenticationManager? https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility.js:286: instrumentApiFunction(chrome.identity, 'getAuthToken', 1); On 2013/07/30 23:27:23, vadimt wrote: > We instrument only functions with callbacks that are directly called by Chrome. > For these ones, the callbacks will be invoked in the context of already > instrumented Chrome callbacks. Done. https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility.js:421: * @return {object} The Authentication Manager interface. On 2013/07/30 23:27:23, vadimt wrote: > {Object} Done. https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility.js:430: var isSignedInRequests = []; Sounds good then. On 2013/07/30 23:27:23, vadimt wrote: > Do we queue them for perf reasons? If so, no need to do this. I checked with > identity guys: this API has great performance, and no caching of grouping is > necessary. https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility.js:434: * @param {function(boolean, string)} onSuccess Called on completion. In our case, getAuthToken can fail and we treat the answer as not signed in. This is a success in our case. On 2013/07/30 23:27:23, vadimt wrote: > It's called even when getAuthToken fails, therefore, it cannot be called > onSuccess. https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility.js:444: queuedCallback(signedIn, token); This was done to remove all of the !!'s that were around for the token. On 2013/07/30 23:27:23, vadimt wrote: > It would be enough to simply pass token. If it's undefined, we are not signed > in. https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility.js:467: var listeners = []; On 2013/07/30 23:27:23, vadimt wrote: > Please add a TODO item to get rid of this and implement a chrome API instead. Done. https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility.js:481: var lastReturnedSignedInState; On 2013/07/30 23:27:23, vadimt wrote: > Please explicitly assign null. No reasons to use undefined. Done. https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility.js:485: (lastReturnedSignedInState != undefined)) { On 2013/07/30 23:27:23, vadimt wrote: > Please use !== to not deal with JS strangeness. Done.
https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility.js:434: * @param {function(boolean, string)} onSuccess Called on completion. On 2013/07/30 23:58:03, Robert Liao wrote: > In our case, getAuthToken can fail and we treat the answer as not signed in. > This is a success in our case. > On 2013/07/30 23:27:23, vadimt wrote: > > It's called even when getAuthToken fails, therefore, it cannot be called > > onSuccess. > But onSuccess implies there is onFailure. If it's alone, whoever reads this code will be confused. If there is one callback, and it's called in all cases, the more appropriate name for it is neutral: 'callback'. See, for example, this: https://developer.chrome.com/extensions/alarms.html#method-getAll https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility.js:444: queuedCallback(signedIn, token); On 2013/07/30 23:58:03, Robert Liao wrote: > This was done to remove all of the !!'s that were around for the token. > On 2013/07/30 23:27:23, vadimt wrote: > > It would be enough to simply pass token. If it's undefined, we are not signed > > in. > But what you essentially do is passing a value and !!value. It's like passing a, b and a+b. We are ending up confusing the reader by such excessiveness. It's better to calculate !!value every time.
https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility.js:434: * @param {function(boolean, string)} onSuccess Called on completion. sgtm On 2013/07/31 02:29:49, vadimt wrote: > On 2013/07/30 23:58:03, Robert Liao wrote: > > In our case, getAuthToken can fail and we treat the answer as not signed in. > > This is a success in our case. > > On 2013/07/30 23:27:23, vadimt wrote: > > > It's called even when getAuthToken fails, therefore, it cannot be called > > > onSuccess. > > > > But onSuccess implies there is onFailure. If it's alone, whoever reads this code > will be confused. > If there is one callback, and it's called in all cases, the more appropriate > name for it is neutral: 'callback'. See, for example, this: > https://developer.chrome.com/extensions/alarms.html#method-getAll https://codereview.chromium.org/21235008/diff/1/chrome/browser/resources/goog... chrome/browser/resources/google_now/utility.js:444: queuedCallback(signedIn, token); On 2013/07/31 02:29:49, vadimt wrote: > On 2013/07/30 23:58:03, Robert Liao wrote: > > This was done to remove all of the !!'s that were around for the token. > > On 2013/07/30 23:27:23, vadimt wrote: > > > It would be enough to simply pass token. If it's undefined, we are not > signed > > > in. > > > > But what you essentially do is passing a value and !!value. > It's like passing a, b and a+b. > We are ending up confusing the reader by such excessiveness. It's better to > calculate !!value every time. Done.
https://codereview.chromium.org/21235008/diff/22001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/21235008/diff/22001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:430: * @param {function(string)} callback Called on completion. function(string=) https://codereview.chromium.org/21235008/diff/22001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:435: token = !chrome.runtime.lastError && token; This can make token boolean. We want it to be either string or undefined. Please use ternary operator.
https://codereview.chromium.org/21235008/diff/22001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/21235008/diff/22001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:430: * @param {function(string)} callback Called on completion. On 2013/07/31 19:49:54, vadimt wrote: > function(string=) Done. https://codereview.chromium.org/21235008/diff/22001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:435: token = !chrome.runtime.lastError && token; On 2013/07/31 19:49:54, vadimt wrote: > This can make token boolean. We want it to be either string or undefined. Please > use ternary operator. Done.
lgtm https://codereview.chromium.org/21235008/diff/31001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/21235008/diff/31001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:435: token = chrome.runtime.lastError ? null : token; I suggest using 'undefined' for 2 reasons: 1. getAuthToken returns undefined. You introduce complexity: now it can be string, undefined or null. 2. Without this, you have to change string= to ?string= See https://developers.google.com/closure/compiler/docs/js-for-compiler#types
xiyuan: Please provide owner approval for the files in this CL. Thanks!
https://codereview.chromium.org/21235008/diff/31001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/21235008/diff/31001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:435: token = chrome.runtime.lastError ? null : token; I'm convinced. On 2013/07/31 20:46:17, vadimt wrote: > I suggest using 'undefined' for 2 reasons: > 1. getAuthToken returns undefined. You introduce complexity: now it can be > string, undefined or null. > 2. Without this, you have to change string= to ?string= > > See https://developers.google.com/closure/compiler/docs/js-for-compiler#types
https://codereview.chromium.org/21235008/diff/38001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/21235008/diff/38001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:470: var lastReturnedSignedInState; Should we initialize lastReturnedSignedInState to null here? Otherwise, I don't see how it could ever be null. https://codereview.chromium.org/21235008/diff/38001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:485: } nit: no {} for one-liner.
https://codereview.chromium.org/21235008/diff/38001/chrome/browser/resources/... File chrome/browser/resources/google_now/utility.js (right): https://codereview.chromium.org/21235008/diff/38001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:470: var lastReturnedSignedInState; Hrm... a lost delta. This is supposed to be initialized to null. On 2013/07/31 21:17:02, xiyuan wrote: > Should we initialize lastReturnedSignedInState to null here? Otherwise, I don't > see how it could ever be null. https://codereview.chromium.org/21235008/diff/38001/chrome/browser/resources/... chrome/browser/resources/google_now/utility.js:485: } On 2013/07/31 21:17:02, xiyuan wrote: > nit: no {} for one-liner. Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/robliao@chromium.org/21235008/43001
Message was sent while issue was closed.
Change committed as 215115 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
