Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2204223005:
Blimp OAuth2 token retreival on application start up. (Closed)
|
Created:
4 years, 4 months ago by xingliu Modified:
4 years, 4 months ago CC:
chromium-reviews, anandc+watch-blimp_chromium.org, maniscalco+watch-blimp_chromium.org, sriramsr+watch-blimp_chromium.org, gcasto+watch-blimp_chromium.org, nyquist+watch-blimp_chromium.org, marcinjb+watch-blimp_chromium.org, jessicag+watch-blimp_chromium.org, kmarshall+watch-blimp_chromium.org, lethalantidote+watch-blimp_chromium.org, dtrainor+watch-blimp_chromium.org, shaktisahu+watch-blimp_chromium.org, khushalsagar+watch-blimp_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionBlimp OAuth2 token retreival on application start up.
Previously Blimp runs in its own app, and we have our own token source
implementation.
This cl implement the OAuth2 token flow in c++.
There is a known issue that connect must be called after account seeding in android layer, or token request can be flaky since account seeding might cause request canceled.
BUG=624457
Committed: https://crrev.com/f748905541816e1fede06d2ad01b96075e727bc4
Cr-Commit-Position: refs/heads/master@{#412923}
Patch Set 1 #Patch Set 2 : Add deps file. #
Total comments: 16
Patch Set 3 : Move authenticator to c++. #Patch Set 4 : Build rules for dummy core. #Patch Set 5 : Unit test of authenticator in c++. #Patch Set 6 : Add two line in unittest. #Patch Set 7 : Fix build config. #
Total comments: 38
Patch Set 8 : Rename to IdentitySource, fix based on reviews. #Patch Set 9 : Fix compiling without blimp flag. #Patch Set 10 : Rebase to resolve conflicts. #Patch Set 11 : Fix the unittest just moved to session. #Patch Set 12 : Add some deps only for gn deps check script. #
Total comments: 42
Patch Set 13 : Fixes based on code review. #Patch Set 14 : More fixes based on review. #Patch Set 15 : Hooked up auth token to assignment source code in BlimpContextImpl. #Patch Set 16 : Renamed BlimpError to AuthError. #Patch Set 17 : Fix unittests compiling. #
Total comments: 12
Patch Set 18 : Fixes for code review. #Patch Set 19 : Fix test code gn file, also rename some vars #Patch Set 20 : Minor fix. #
Total comments: 6
Patch Set 21 : Fixes for nits. #Messages
Total messages: 98 (74 generated)
Description was changed from ========== Blimp OAuth2 token retreival on application start up. Previously Blimp runs in its own app, and we have our own token source implementation. This cl implement the OAuth2 token flow with sync component, and put the code inside Blimp. In Chrome, we have one function to trigger token flow via BlimpClientContext on application start up. Inside Blimp, UI can use Authenticator to get the token. This cl also move PreferencesUtil to core/common, to avoid circular dependency issue. Potential improvment is to implement whole thing in c++, but not sure if this is the plan for 0.6. BUG=624457 ========== to ========== Blimp OAuth2 token retreival on application start up. Previously Blimp runs in its own app, and we have our own token source implementation. This cl implement the OAuth2 token flow with sync component, and put the code inside Blimp. In Chrome, we have one function to trigger token flow via BlimpClientContext on application start up. Inside Blimp, UI can use Authenticator to get the token. This cl also move PreferencesUtil to core/common, to avoid circular dependency issue. Potential improvment is to implement whole thing in c++, but not sure if this is the plan for 0.6. BUG=624457 ==========
xingliu@chromium.org changed reviewers: + dtrainor@chromium.org, nyquist@chromium.org
OAuth token retrieval. This CL only gets the token, no sign in or UI work included.
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
https://codereview.chromium.org/2204223005/diff/20001/blimp/client/core/BUILD.gn File blimp/client/core/BUILD.gn (right): https://codereview.chromium.org/2204223005/diff/20001/blimp/client/core/BUILD... blimp/client/core/BUILD.gn:169: android_library("auth_java") { Hmm I wonder if we should just merge all core java files to one target. Something to think about. I think that's how most other android/java folders work in Chrome IIRC. https://codereview.chromium.org/2204223005/diff/20001/blimp/client/core/andro... File blimp/client/core/android/java/src/org/chromium/blimp/core/auth/Authenticator.java (right): https://codereview.chromium.org/2204223005/diff/20001/blimp/client/core/andro... blimp/client/core/android/java/src/org/chromium/blimp/core/auth/Authenticator.java:48: public void tokenUnavailable(boolean isTransientError) { Will this be handled in the future CL? https://codereview.chromium.org/2204223005/diff/20001/blimp/client/public/and... File blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContext.java (right): https://codereview.chromium.org/2204223005/diff/20001/blimp/client/public/and... blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContext.java:47: void connect(String clientToken); Should this be exposed in the public interface? What do you think about making the Authenticator hold a BlimpClientContextImpl (or some callback interface) and calling something like onAuthTokenReceived() there? https://codereview.chromium.org/2204223005/diff/20001/blimp/client/public/and... blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContext.java:52: void getAuthToken(); Shouldn't the publicly exposed method just be connect()? getAuthToken is an implementation detail for how the BlimpClientContext actually sets up the connection right? wdyt?
https://codereview.chromium.org/2204223005/diff/20001/blimp/client/core/BUILD.gn File blimp/client/core/BUILD.gn (right): https://codereview.chromium.org/2204223005/diff/20001/blimp/client/core/BUILD... blimp/client/core/BUILD.gn:169: android_library("auth_java") { On 2016/08/05 15:49:09, David Trainor wrote: > Hmm I wonder if we should just merge all core java files to one target. > Something to think about. I think that's how most other android/java folders > work in Chrome IIRC. My idea with the directory structure would be to have one BUILD.gn file for each directory. Since this lives in an 'auth' subpackage of core, it should also live in an auth subdirectory under core, so //blimp/client/core/auth/... https://codereview.chromium.org/2204223005/diff/20001/blimp/client/core/andro... File blimp/client/core/android/java/src/org/chromium/blimp/core/auth/Authenticator.java (right): https://codereview.chromium.org/2204223005/diff/20001/blimp/client/core/andro... blimp/client/core/android/java/src/org/chromium/blimp/core/auth/Authenticator.java:40: accountManagerHelper.getAuthToken(signin.getSignedInUser(), SCOPE, Since this does everything directly inside of blimp, I wonder if we should just move this to native code instead? You could ask the delegate to get an OAuth2TokenService. It should be able to get one through ProfileOAuth2TokenServiceFactory::GetForProfile(Profile* profile). That way we don't have to have any of this Java-code. https://codereview.chromium.org/2204223005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2204223005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:938: mBlimpClientContextDelegate.getBlimpClientContext().getAuthToken(); It feels more natural if this call was "connect()" instead, which then would trigger the auth fetching code, which would lead to a call with a token to an internal connect-method.
https://codereview.chromium.org/2204223005/diff/20001/blimp/client/core/BUILD.gn File blimp/client/core/BUILD.gn (right): https://codereview.chromium.org/2204223005/diff/20001/blimp/client/core/BUILD... blimp/client/core/BUILD.gn:169: android_library("auth_java") { On 2016/08/05 17:05:07, nyquist wrote: > On 2016/08/05 15:49:09, David Trainor wrote: > > Hmm I wonder if we should just merge all core java files to one target. > > Something to think about. I think that's how most other android/java folders > > work in Chrome IIRC. > > My idea with the directory structure would be to have one BUILD.gn file for each > directory. Since this lives in an 'auth' subpackage of core, it should also live > in an auth subdirectory under core, so //blimp/client/core/auth/... Also, to the point of that's how many other targets do it; most of those targets also have a single top-level BUILD.gn for all of their code, which usually matches their old setup with GYP-files. With //blimp/client/core for now being heavily split up into multiple GN files, and the Java-files mostly being JNI-passthrough-classes, I still feel like it's helpful to have the android-specific JNI-bridges and Java files in those directories.
Discussion on review feedback. https://codereview.chromium.org/2204223005/diff/20001/blimp/client/core/andro... File blimp/client/core/android/java/src/org/chromium/blimp/core/auth/Authenticator.java (right): https://codereview.chromium.org/2204223005/diff/20001/blimp/client/core/andro... blimp/client/core/android/java/src/org/chromium/blimp/core/auth/Authenticator.java:48: public void tokenUnavailable(boolean isTransientError) { On 2016/08/05 15:49:09, David Trainor wrote: > Will this be handled in the future CL? Should we show some error message and give user an option to trigger the authentication flow again in something like snackbar or alert dialog? https://codereview.chromium.org/2204223005/diff/20001/blimp/client/public/and... File blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContext.java (right): https://codereview.chromium.org/2204223005/diff/20001/blimp/client/public/and... blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContext.java:47: void connect(String clientToken); On 2016/08/05 15:49:09, David Trainor wrote: > Should this be exposed in the public interface? What do you think about making > the Authenticator hold a BlimpClientContextImpl (or some callback interface) and > calling something like onAuthTokenReceived() there? I think that's better, will discuss it with Tommy. If we put get token function inside Blimp, then connect in the BlimpClientContext interface is probably not needed. If get token is put in chrome (in delegate), then connect in the interface is needed. https://codereview.chromium.org/2204223005/diff/20001/blimp/client/public/and... blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContext.java:52: void getAuthToken(); On 2016/08/05 15:49:09, David Trainor wrote: > Shouldn't the publicly exposed method just be connect()? getAuthToken is an > implementation detail for how the BlimpClientContext actually sets up the > connection right? wdyt? Oh, this function is probably badly named, what it does here is try to query the token on application start up, for user who has already enable blimp and open clank. If we put get token function inside Blimp, some function for early start up(maybe something like initialize() ) in BlimpClientContext interface is probably needed. If we can put get token function in chrome(in delegate), so external can directly call and internal can call through the delegate, then this function in BlimpClientContext interface is not needed. But connect() is needed.
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Blimp OAuth2 token retreival on application start up. Previously Blimp runs in its own app, and we have our own token source implementation. This cl implement the OAuth2 token flow with sync component, and put the code inside Blimp. In Chrome, we have one function to trigger token flow via BlimpClientContext on application start up. Inside Blimp, UI can use Authenticator to get the token. This cl also move PreferencesUtil to core/common, to avoid circular dependency issue. Potential improvment is to implement whole thing in c++, but not sure if this is the plan for 0.6. BUG=624457 ========== to ========== Blimp OAuth2 token retreival on application start up. Previously Blimp runs in its own app, and we have our own token source implementation. This cl implement the OAuth2 token flow in c++. There is a known issue that connect must be called after account seeding in android layer, or token request can be flaky since account seeding might cause request canceled. BUG=624457 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Recent several patches includes: #1. Moved token retrieval code to c++, into authenticator.cc. #2. Unit test in authenticator_unittest.cc for Authenticator class. #3. Fixed call site of connect() in Java, this has to be after onSystemAccountsSeedingComplete due to the way Clank using native code. https://codereview.chromium.org/2204223005/diff/20001/blimp/client/core/BUILD.gn File blimp/client/core/BUILD.gn (right): https://codereview.chromium.org/2204223005/diff/20001/blimp/client/core/BUILD... blimp/client/core/BUILD.gn:169: android_library("auth_java") { On 2016/08/05 17:09:31, nyquist wrote: > On 2016/08/05 17:05:07, nyquist wrote: > > On 2016/08/05 15:49:09, David Trainor wrote: > > > Hmm I wonder if we should just merge all core java files to one target. > > > Something to think about. I think that's how most other android/java > folders > > > work in Chrome IIRC. > > > > My idea with the directory structure would be to have one BUILD.gn file for > each > > directory. Since this lives in an 'auth' subpackage of core, it should also > live > > in an auth subdirectory under core, so //blimp/client/core/auth/... > > Also, to the point of that's how many other targets do it; most of those targets > also have a single top-level BUILD.gn for all of their code, which usually > matches their old setup with GYP-files. With //blimp/client/core for now being > heavily split up into multiple GN files, and the Java-files mostly being > JNI-passthrough-classes, I still feel like it's helpful to have the > android-specific JNI-bridges and Java files in those directories. For java files, will have circular deps issue between UI and context. Currently just try to fix it by moving PreferencesUtil.java to common. https://codereview.chromium.org/2204223005/diff/20001/blimp/client/core/andro... File blimp/client/core/android/java/src/org/chromium/blimp/core/auth/Authenticator.java (right): https://codereview.chromium.org/2204223005/diff/20001/blimp/client/core/andro... blimp/client/core/android/java/src/org/chromium/blimp/core/auth/Authenticator.java:40: accountManagerHelper.getAuthToken(signin.getSignedInUser(), SCOPE, On 2016/08/05 17:05:07, nyquist wrote: > Since this does everything directly inside of blimp, I wonder if we should just > move this to native code instead? You could ask the delegate to get an > OAuth2TokenService. It should be able to get one through > ProfileOAuth2TokenServiceFactory::GetForProfile(Profile* profile). > That way we don't have to have any of this Java-code. Done. https://codereview.chromium.org/2204223005/diff/20001/blimp/client/public/and... File blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContext.java (right): https://codereview.chromium.org/2204223005/diff/20001/blimp/client/public/and... blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContext.java:47: void connect(String clientToken); On 2016/08/05 17:17:00, xingliu wrote: > On 2016/08/05 15:49:09, David Trainor wrote: > > Should this be exposed in the public interface? What do you think about > making > > the Authenticator hold a BlimpClientContextImpl (or some callback interface) > and > > calling something like onAuthTokenReceived() there? > > I think that's better, will discuss it with Tommy. > > If we put get token function inside Blimp, then connect in the > BlimpClientContext interface is probably not needed. > > If get token is put in chrome (in delegate), then connect in the interface is > needed. Done. Now token is handled in c++. No exposure in Java. https://codereview.chromium.org/2204223005/diff/20001/blimp/client/public/and... blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContext.java:52: void getAuthToken(); On 2016/08/05 15:49:09, David Trainor wrote: > Shouldn't the publicly exposed method just be connect()? getAuthToken is an > implementation detail for how the BlimpClientContext actually sets up the > connection right? wdyt? Removed. https://codereview.chromium.org/2204223005/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2204223005/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:938: mBlimpClientContextDelegate.getBlimpClientContext().getAuthToken(); On 2016/08/05 17:05:07, nyquist wrote: > It feels more natural if this call was "connect()" instead, which then would > trigger the auth fetching code, which would lead to a call with a token to an > internal connect-method. Done, now it's connect(), and fixed the call site of this to a callback in AccountTracker Serivce.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/BUIL... File blimp/client/core/BUILD.gn (right): https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/BUIL... blimp/client/core/BUILD.gn:94: source_set("auth") { Could we move this into //blimp/client/core/session ? https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/andr... File blimp/client/core/android/java/src/org/chromium/blimp/core/common/PreferencesUtil.java (right): https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/andr... blimp/client/core/android/java/src/org/chromium/blimp/core/common/PreferencesUtil.java:12: public class PreferencesUtil { Is this move related to this CL? https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... File blimp/client/core/authenticator.cc (right): https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... blimp/client/core/authenticator.cc:11: const char kOAuth2TokenScope[] = This should be in an anonymous namespace. https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... blimp/client/core/authenticator.cc:33: return; For now, maybe at least log? But I'm thinking that maybe that this should inform the BlimpClientContextDelegate about this state? https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... blimp/client/core/authenticator.cc:56: // TODO(xingliu): Alert the user in UI. Like above, I think this could call out to the delegate for now. Then maybe you could move this TODO up to the ChromeBlimpClientContextDelegate in //chrome? https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... blimp/client/core/authenticator.cc:64: void Authenticator::OnRefreshTokenAvailable(const std::string& account_id) { Is there any requirement to when this is created? As in, when you're adding yourself as an observer, will you automatically get a callback, if the call has already gone out? https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... blimp/client/core/authenticator.cc:65: if (account_id != account_id_) I think we should be consistent about using { or not. In chromium in general we do not use them, but for //blimp we should use {} to be consistent for one-liners like this. https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... blimp/client/core/authenticator.cc:87: } else { I think something happened to the indent here? You should be able to just run 'git cl format' before you upload, and such things magically go away :-) https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... File blimp/client/core/authenticator.h (right): https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... blimp/client/core/authenticator.h:18: class Authenticator : public OAuth2TokenService::Consumer, I think we want this to live in //blimp/client/core/session https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... blimp/client/core/authenticator.h:27: // Start Blimp authentication by requesting OAuth2 token from google. Capitalize Google here and below. https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... blimp/client/core/authenticator.h:49: std::unique_ptr<IdentityProvider> identity_provider_; #include <memory> https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... blimp/client/core/authenticator.h:51: // OAuth2 token request. This class should take ownership of the request. std::unique_ptr implies ownership. Maybe instead focus on when it's created and for how long it lives? Is this for the one in-flight? https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... blimp/client/core/authenticator.h:55: std::string account_id_; #include <string> https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/blim... File blimp/client/core/blimp_client_context_impl.cc (right): https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/blim... blimp/client/core/blimp_client_context_impl.cc:65: authenticator_->Connect(); If there is no authenticator_ if you have the creation of the IdentityProvider in the delegate, you'd be able to create on-the-fly to ensure that you always have it here. https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/blim... File blimp/client/core/blimp_client_context_impl.h (right): https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/blim... blimp/client/core/blimp_client_context_impl.h:39: virtual void Connect(); It kind of feels like this should be part of the public API of BlimpClientContext? The C++ BlimpClientContextImplAndroid could still just call this like now. https://codereview.chromium.org/2204223005/diff/120001/blimp/client/public/bl... File blimp/client/public/blimp_client_context.h (right): https://codereview.chromium.org/2204223005/diff/120001/blimp/client/public/bl... blimp/client/public/blimp_client_context.h:55: virtual void CreateAuthenticator( Could you move this to the BlimpClientContextDelegate instead? Something like: std::unique_ptr<IdentityProvider> CreateIdentityProvider()? https://codereview.chromium.org/2204223005/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/blimp/ChromeBlimpClientContextDelegate.java (right): https://codereview.chromium.org/2204223005/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/blimp/ChromeBlimpClientContextDelegate.java:40: public static ChromeBlimpClientContextDelegate createAndSetDelegateForContext(Profile profile) { As it looks like now, this in fact also triggers a connection to the blimp engine. Would we want to have that as a separate method maybe? And maybe let the "onSeedingComplete" just set a flag that it's ready? And only if the caller has called Connect() AND the seeding is complete do we connect, and if either happens first, we'll wait for the other call? WDYT? https://codereview.chromium.org/2204223005/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/blimp/ChromeBlimpClientContextDelegate.java:83: .addSystemAccountsSeededListener(this); If this class is instantiated late (after accounts have been seeded), will the callback still trigger?
https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... File blimp/client/core/authenticator.h (right): https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... blimp/client/core/authenticator.h:16: // Authenticator handles OAuth2 token request and forward user sign in state Could we come up with a different name than 'Authenticator' that's even more descriptive? I'm not sure what's a good name yet, but Authenticator has a very specific purpose in the Android platform. See https://developer.android.com/training/sync-adapters/creating-authenticator.html
Thanks for the great review. Fixes based on the review. https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/BUIL... File blimp/client/core/BUILD.gn (right): https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/BUIL... blimp/client/core/BUILD.gn:94: source_set("auth") { On 2016/08/10 01:20:33, nyquist wrote: > Could we move this into //blimp/client/core/session ? Done. https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/andr... File blimp/client/core/android/java/src/org/chromium/blimp/core/common/PreferencesUtil.java (right): https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/andr... blimp/client/core/android/java/src/org/chromium/blimp/core/common/PreferencesUtil.java:12: public class PreferencesUtil { On 2016/08/10 01:20:33, nyquist wrote: > Is this move related to this CL? Done. Removed. https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... File blimp/client/core/authenticator.cc (right): https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... blimp/client/core/authenticator.cc:11: const char kOAuth2TokenScope[] = On 2016/08/10 01:20:33, nyquist wrote: > This should be in an anonymous namespace. Done. https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... blimp/client/core/authenticator.cc:33: return; On 2016/08/10 01:20:33, nyquist wrote: > For now, maybe at least log? > But I'm thinking that maybe that this should inform the > BlimpClientContextDelegate about this state? Added VLOG, moved this to delegate's OnError https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... blimp/client/core/authenticator.cc:56: // TODO(xingliu): Alert the user in UI. On 2016/08/10 01:20:33, nyquist wrote: > Like above, I think this could call out to the delegate for now. Then maybe you > could move this TODO up to the ChromeBlimpClientContextDelegate in //chrome? Done. https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... blimp/client/core/authenticator.cc:64: void Authenticator::OnRefreshTokenAvailable(const std::string& account_id) { On 2016/08/10 01:20:33, nyquist wrote: > Is there any requirement to when this is created? As in, when you're adding > yourself as an observer, will you automatically get a callback, if the call has > already gone out? This is added only when no refresh token. If we have refresh token, then directly start to fetch instead of listening this. No automatic callback in this case. https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... blimp/client/core/authenticator.cc:65: if (account_id != account_id_) On 2016/08/10 01:20:33, nyquist wrote: > I think we should be consistent about using { or not. In chromium in general we > do not use them, but for //blimp we should use {} to be consistent for > one-liners like this. Done. https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... blimp/client/core/authenticator.cc:87: } else { On 2016/08/10 01:20:33, nyquist wrote: > I think something happened to the indent here? You should be able to just run > 'git cl format' before you upload, and such things magically go away :-) Oh, that's cool. https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... File blimp/client/core/authenticator.h (right): https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... blimp/client/core/authenticator.h:16: // Authenticator handles OAuth2 token request and forward user sign in state On 2016/08/10 16:48:56, nyquist wrote: > Could we come up with a different name than 'Authenticator' that's even more > descriptive? I'm not sure what's a good name yet, but Authenticator has a very > specific purpose in the Android platform. See > https://developer.android.com/training/sync-adapters/creating-authenticator.html Renamed to IdentitySource, also Authenticator is probably too general for this class. https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... blimp/client/core/authenticator.h:18: class Authenticator : public OAuth2TokenService::Consumer, On 2016/08/10 01:20:33, nyquist wrote: > I think we want this to live in //blimp/client/core/session Done. https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... blimp/client/core/authenticator.h:27: // Start Blimp authentication by requesting OAuth2 token from google. On 2016/08/10 01:20:33, nyquist wrote: > Capitalize Google here and below. Done. https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... blimp/client/core/authenticator.h:49: std::unique_ptr<IdentityProvider> identity_provider_; On 2016/08/10 01:20:33, nyquist wrote: > #include <memory> Done. https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... blimp/client/core/authenticator.h:51: // OAuth2 token request. This class should take ownership of the request. On 2016/08/10 01:20:33, nyquist wrote: > std::unique_ptr implies ownership. Maybe instead focus on when it's created and > for how long it lives? Is this for the one in-flight? I'll change the comment. Yes, it's the actual request on the fly. https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/auth... blimp/client/core/authenticator.h:55: std::string account_id_; On 2016/08/10 01:20:33, nyquist wrote: > #include <string> Done. https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/blim... File blimp/client/core/blimp_client_context_impl.cc (right): https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/blim... blimp/client/core/blimp_client_context_impl.cc:65: authenticator_->Connect(); On 2016/08/10 01:20:33, nyquist wrote: > If there is no authenticator_ if you have the creation of the IdentityProvider > in the delegate, you'd be able to create on-the-fly to ensure that you always > have it here. Done. Now do lazy init and create IdentityProvider in the delegate. https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/blim... File blimp/client/core/blimp_client_context_impl.h (right): https://codereview.chromium.org/2204223005/diff/120001/blimp/client/core/blim... blimp/client/core/blimp_client_context_impl.h:39: virtual void Connect(); On 2016/08/10 01:20:33, nyquist wrote: > It kind of feels like this should be part of the public API of > BlimpClientContext? The C++ BlimpClientContextImplAndroid could still just call > this like now. Oh, this is my bad. Moved to public interface. https://codereview.chromium.org/2204223005/diff/120001/blimp/client/public/bl... File blimp/client/public/blimp_client_context.h (right): https://codereview.chromium.org/2204223005/diff/120001/blimp/client/public/bl... blimp/client/public/blimp_client_context.h:55: virtual void CreateAuthenticator( On 2016/08/10 01:20:34, nyquist wrote: > Could you move this to the BlimpClientContextDelegate instead? Something like: > std::unique_ptr<IdentityProvider> CreateIdentityProvider()? Done. This is great idea. https://codereview.chromium.org/2204223005/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/blimp/ChromeBlimpClientContextDelegate.java (right): https://codereview.chromium.org/2204223005/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/blimp/ChromeBlimpClientContextDelegate.java:40: public static ChromeBlimpClientContextDelegate createAndSetDelegateForContext(Profile profile) { On 2016/08/10 01:20:34, nyquist wrote: > As it looks like now, this in fact also triggers a connection to the blimp > engine. > Would we want to have that as a separate method maybe? > > And maybe let the "onSeedingComplete" just set a flag that it's ready? > And only if the caller has called Connect() AND the seeding is complete do we > connect, and if either happens first, we'll wait for the other call? > WDYT? Maybe a single line of start() or init(), which call createAndSetDelegateForContext() and connect(), is better ? Lots of other components do that I think. If we do nothing for connect call before account seeding, then the code is not robust since either account seeding changed or call site of first connect changed, this might be broken. And also we need to find a call site for the later connect, then we will have the same issue of whether the account seeding is done or not. If the later call depends on user interaction, I guess it's a bad user experience, since user may see different behavior each time loads Clank. https://codereview.chromium.org/2204223005/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/blimp/ChromeBlimpClientContextDelegate.java:83: .addSystemAccountsSeededListener(this); On 2016/08/10 01:20:34, nyquist wrote: > If this class is instantiated late (after accounts have been seeded), will the > callback still trigger? Yes, I think so. It has state check, and will directly trigger callback if state check passed. In AccountTrackerService.addSystemAccountsSeededListener.
Sorry that I forgot to submit a patch after a rebase.
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
xingliu@chromium.org changed reviewers: + rogerta@chromium.org
To rogerta: Please help review gaia related code, blimp/client/core/session/identity_source.h blimp/client/core/session/identity_source.cc chrome/browser/android/blimp/chrome_blimp_client_context_delegate.cc
this looks great! awesome that you could do this in C++! https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/andr... File blimp/client/core/android/blimp_client_context_impl_android.cc (right): https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/andr... blimp/client/core/android/blimp_client_context_impl_android.cc:84: Connect(); Could you add BlimpClientContextImpl:: prefix here for clarity? https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/blim... File blimp/client/core/blimp_client_context_impl.cc (right): https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/blim... blimp/client/core/blimp_client_context_impl.cc:75: void BlimpClientContextImpl::Connect() { Nit: You see if it makes sense to order these functions like in the header? https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/blim... File blimp/client/core/blimp_client_context_impl.h (right): https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/blim... blimp/client/core/blimp_client_context_impl.h:90: std::unique_ptr<IdentitySource> identity_source_; Could you explain what this is used for? https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/dumm... File blimp/client/core/dummy_blimp_client_context.cc (right): https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/dumm... blimp/client/core/dummy_blimp_client_context.cc:48: void DummyBlimpClientContext::Connect() {} Do we want to add NOTREACHED() here as well, to ensure that //chrome code doesn't call this if Blimp isn't compiled in? https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/sess... File blimp/client/core/session/identity_source.cc (right): https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/sess... blimp/client/core/session/identity_source.cc:64: void IdentitySource::OnRefreshTokenAvailable(const std::string& account_id) { Should this also reset |token_request_|? https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/sess... File blimp/client/core/session/identity_source_unittest.cc (right): https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/sess... blimp/client/core/session/identity_source_unittest.cc:13: #include "testing/gmock/include/gmock/gmock.h" Is this used here? https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/sess... blimp/client/core/session/identity_source_unittest.cc:22: class MockIdentitySource : public IdentitySource { Does this need to be an inner class, or could you just declare it above IdentitySourceTest? https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/sess... blimp/client/core/session/identity_source_unittest.cc:57: private: DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/sess... blimp/client/core/session/identity_source_unittest.cc:64: IdentitySourceTest() {} = default here and below? https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/sess... blimp/client/core/session/identity_source_unittest.cc:91: FakeOAuth2TokenServiceDelegate* mock_delegate = Could you make this name a little bit more specific, especially since there are multiple delegates in play here. https://codereview.chromium.org/2204223005/diff/220001/blimp/client/public/an... File blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContext.java (right): https://codereview.chromium.org/2204223005/diff/220001/blimp/client/public/an... blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContext.java:44: * Start Blimp authentication. This must be called after Maybe put "Start authentication flow and connection to engine." on its own line, and ensure that it's the same as in C++? The next sentence is Android-specific, so that can stay here only. https://codereview.chromium.org/2204223005/diff/220001/blimp/client/public/an... blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContext.java:45: * AccountTrackerService.onSystemAccountsSeedingComplete, since Clank may asynchronously seed change "Clank" to "the embedder" https://codereview.chromium.org/2204223005/diff/220001/blimp/client/public/bl... File blimp/client/public/blimp_client_context.h (right): https://codereview.chromium.org/2204223005/diff/220001/blimp/client/public/bl... blimp/client/public/blimp_client_context.h:58: virtual void Connect(const std::string& client_auth_token) = 0; Should we remove this method now, and make it only part of the BlimpClientContextImpl interface? https://codereview.chromium.org/2204223005/diff/220001/blimp/client/public/bl... blimp/client/public/blimp_client_context.h:60: // Start authentication flow. ... "and connection to engine."? https://codereview.chromium.org/2204223005/diff/220001/blimp/client/public/bl... File blimp/client/public/blimp_client_context_delegate.h (right): https://codereview.chromium.org/2204223005/diff/220001/blimp/client/public/bl... blimp/client/public/blimp_client_context_delegate.h:22: enum BlimpError { Do we want to specify that this is Identity-related for now? Or is the plan to use this for all kinds of errors going forward? https://codereview.chromium.org/2204223005/diff/220001/blimp/client/public/bl... blimp/client/public/blimp_client_context_delegate.h:46: virtual void OnError(BlimpClientContextDelegate::BlimpError error) = 0; Similarly to the BlimpError enum, do we want to have this as a more specific error-type? https://codereview.chromium.org/2204223005/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/blimp/ChromeBlimpClientContextDelegate.java (right): https://codereview.chromium.org/2204223005/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/blimp/ChromeBlimpClientContextDelegate.java:26: // Blimp client context reference associated with this delegate. Could you use JavaDoc comment standard? And maybe just "{@link BlimpClientContext} associated with this delegate." https://codereview.chromium.org/2204223005/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/blimp/ChromeBlimpClientContextDelegate.java:81: AccountTrackerService.get(ContextUtils.getApplicationContext()) Could you add a comment that explains that the listener will be called even if the system accounts have already been seeded? https://codereview.chromium.org/2204223005/diff/220001/chrome/browser/android... File chrome/browser/android/blimp/chrome_blimp_client_context_delegate.cc (right): https://codereview.chromium.org/2204223005/diff/220001/chrome/browser/android... chrome/browser/android/blimp/chrome_blimp_client_context_delegate.cc:42: return base::WrapUnique<IdentityProvider>(new ProfileIdentityProvider( Is it so that MakeUnique can't be used because the unique_ptr type is the base class of ProfileIdentityProvider? https://codereview.chromium.org/2204223005/diff/220001/chrome/browser/android... chrome/browser/android/blimp/chrome_blimp_client_context_delegate.cc:50: switch (error) { Should this have a default statement? https://codereview.chromium.org/2204223005/diff/220001/chrome/browser/android... File chrome/browser/android/blimp/chrome_blimp_client_context_delegate.h (right): https://codereview.chromium.org/2204223005/diff/220001/chrome/browser/android... chrome/browser/android/blimp/chrome_blimp_client_context_delegate.h:41: std::unique_ptr<IdentityProvider> CreateIdentityProvider() override; No empty line before this or the next method, as they're part of the BlimpClientContextDelegate implementation.
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the review, fixed couple of issues based on the review. https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/andr... File blimp/client/core/android/blimp_client_context_impl_android.cc (right): https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/andr... blimp/client/core/android/blimp_client_context_impl_android.cc:84: Connect(); On 2016/08/12 05:47:04, nyquist wrote: > Could you add BlimpClientContextImpl:: prefix here for clarity? Done. https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/sess... File blimp/client/core/session/identity_source.cc (right): https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/sess... blimp/client/core/session/identity_source.cc:64: void IdentitySource::OnRefreshTokenAvailable(const std::string& account_id) { On 2016/08/12 05:47:04, nyquist wrote: > Should this also reset |token_request_|? Investigate the code a little bit, I think we shouldn't do this here. Request(OAuth2TokenService::RequestImpl) wraps Consumer(implemented by IdentitySource) which gives token callbacks, reset it here will eat the OnGetTokenFailure callback if a refresh happens in the middle of the request. https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/sess... File blimp/client/core/session/identity_source_unittest.cc (right): https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/sess... blimp/client/core/session/identity_source_unittest.cc:13: #include "testing/gmock/include/gmock/gmock.h" On 2016/08/12 05:47:04, nyquist wrote: > Is this used here? Done. https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/sess... blimp/client/core/session/identity_source_unittest.cc:22: class MockIdentitySource : public IdentitySource { On 2016/08/12 05:47:04, nyquist wrote: > Does this need to be an inner class, or could you just declare it above > IdentitySourceTest? Done. https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/sess... blimp/client/core/session/identity_source_unittest.cc:57: private: On 2016/08/12 05:47:04, nyquist wrote: > DISALLOW_COPY_AND_ASSIGN? Done. https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/sess... blimp/client/core/session/identity_source_unittest.cc:64: IdentitySourceTest() {} On 2016/08/12 05:47:04, nyquist wrote: > = default here and below? Done. https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/sess... blimp/client/core/session/identity_source_unittest.cc:91: FakeOAuth2TokenServiceDelegate* mock_delegate = On 2016/08/12 05:47:04, nyquist wrote: > Could you make this name a little bit more specific, especially since there are > multiple delegates in play here. Done. https://codereview.chromium.org/2204223005/diff/220001/chrome/browser/android... File chrome/browser/android/blimp/chrome_blimp_client_context_delegate.cc (right): https://codereview.chromium.org/2204223005/diff/220001/chrome/browser/android... chrome/browser/android/blimp/chrome_blimp_client_context_delegate.cc:42: return base::WrapUnique<IdentityProvider>(new ProfileIdentityProvider( On 2016/08/12 05:47:05, nyquist wrote: > Is it so that MakeUnique can't be used because the unique_ptr type is the base > class of ProfileIdentityProvider? Yes, the reason to use abstract class here is mainly for testing, to use FakeIdentityProvider. Also external classes don't need to know other things from ProfileIdentityProvider but the base class. https://codereview.chromium.org/2204223005/diff/220001/chrome/browser/android... chrome/browser/android/blimp/chrome_blimp_client_context_delegate.cc:50: switch (error) { On 2016/08/12 05:47:05, nyquist wrote: > Should this have a default statement? Done. https://codereview.chromium.org/2204223005/diff/220001/chrome/browser/android... File chrome/browser/android/blimp/chrome_blimp_client_context_delegate.h (right): https://codereview.chromium.org/2204223005/diff/220001/chrome/browser/android... chrome/browser/android/blimp/chrome_blimp_client_context_delegate.h:41: std::unique_ptr<IdentityProvider> CreateIdentityProvider() override; On 2016/08/12 05:47:05, nyquist wrote: > No empty line before this or the next method, as they're part of the > BlimpClientContextDelegate implementation. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Pass the token to assignment source code in context impl. Fixes for reviews. https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/blim... File blimp/client/core/blimp_client_context_impl.cc (right): https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/blim... blimp/client/core/blimp_client_context_impl.cc:75: void BlimpClientContextImpl::Connect() { On 2016/08/12 05:47:04, nyquist wrote: > Nit: You see if it makes sense to order these functions like in the header? Done. https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/blim... File blimp/client/core/blimp_client_context_impl.h (right): https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/blim... blimp/client/core/blimp_client_context_impl.h:90: std::unique_ptr<IdentitySource> identity_source_; On 2016/08/12 05:47:04, nyquist wrote: > Could you explain what this is used for? Done. https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/dumm... File blimp/client/core/dummy_blimp_client_context.cc (right): https://codereview.chromium.org/2204223005/diff/220001/blimp/client/core/dumm... blimp/client/core/dummy_blimp_client_context.cc:48: void DummyBlimpClientContext::Connect() {} On 2016/08/12 05:47:04, nyquist wrote: > Do we want to add NOTREACHED() here as well, to ensure that //chrome code > doesn't call this if Blimp isn't compiled in? Done. https://codereview.chromium.org/2204223005/diff/220001/blimp/client/public/an... File blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContext.java (right): https://codereview.chromium.org/2204223005/diff/220001/blimp/client/public/an... blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContext.java:44: * Start Blimp authentication. This must be called after On 2016/08/12 05:47:05, nyquist wrote: > Maybe put "Start authentication flow and connection to engine." on its own line, > and ensure that it's the same as in C++? > > The next sentence is Android-specific, so that can stay here only. Done. https://codereview.chromium.org/2204223005/diff/220001/blimp/client/public/an... blimp/client/public/android/java/src/org/chromium/blimp_public/BlimpClientContext.java:45: * AccountTrackerService.onSystemAccountsSeedingComplete, since Clank may asynchronously seed On 2016/08/12 05:47:05, nyquist wrote: > change "Clank" to "the embedder" Done. https://codereview.chromium.org/2204223005/diff/220001/blimp/client/public/bl... File blimp/client/public/blimp_client_context.h (right): https://codereview.chromium.org/2204223005/diff/220001/blimp/client/public/bl... blimp/client/public/blimp_client_context.h:58: virtual void Connect(const std::string& client_auth_token) = 0; On 2016/08/12 05:47:05, nyquist wrote: > Should we remove this method now, and make it only part of the > BlimpClientContextImpl interface? Done, rename the function, and pass it as base::Callback to IdentitySource. https://codereview.chromium.org/2204223005/diff/220001/blimp/client/public/bl... blimp/client/public/blimp_client_context.h:60: // Start authentication flow. On 2016/08/12 05:47:05, nyquist wrote: > ... "and connection to engine."? Done. https://codereview.chromium.org/2204223005/diff/220001/blimp/client/public/bl... File blimp/client/public/blimp_client_context_delegate.h (right): https://codereview.chromium.org/2204223005/diff/220001/blimp/client/public/bl... blimp/client/public/blimp_client_context_delegate.h:22: enum BlimpError { On 2016/08/12 05:47:05, nyquist wrote: > Do we want to specify that this is Identity-related for now? Or is the plan to > use this for all kinds of errors going forward? Not sure, I guess this is errors to the embedder. https://codereview.chromium.org/2204223005/diff/220001/blimp/client/public/bl... blimp/client/public/blimp_client_context_delegate.h:46: virtual void OnError(BlimpClientContextDelegate::BlimpError error) = 0; On 2016/08/12 05:47:05, nyquist wrote: > Similarly to the BlimpError enum, do we want to have this as a more specific > error-type? Can I put the errors in public? So the delegate and internal Blimp can share them. https://codereview.chromium.org/2204223005/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/blimp/ChromeBlimpClientContextDelegate.java (right): https://codereview.chromium.org/2204223005/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/blimp/ChromeBlimpClientContextDelegate.java:26: // Blimp client context reference associated with this delegate. On 2016/08/12 05:47:05, nyquist wrote: > Could you use JavaDoc comment standard? > And maybe just "{@link BlimpClientContext} associated with this delegate." Done. https://codereview.chromium.org/2204223005/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/blimp/ChromeBlimpClientContextDelegate.java:81: AccountTrackerService.get(ContextUtils.getApplicationContext()) On 2016/08/12 05:47:05, nyquist wrote: > Could you add a comment that explains that the listener will be called even if > the system accounts have already been seeded? Done.
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
this mostly looks good! awesome! https://codereview.chromium.org/2204223005/diff/320001/blimp/client/core/sess... File blimp/client/core/session/identity_source.cc (right): https://codereview.chromium.org/2204223005/diff/320001/blimp/client/core/sess... blimp/client/core/session/identity_source.cc:56: token_callback_.Run(access_token); Should this reset the callback? https://codereview.chromium.org/2204223005/diff/320001/blimp/client/core/sess... blimp/client/core/session/identity_source.cc:61: void IdentitySource::OnGetTokenFailure( Should this also reset the callback? https://codereview.chromium.org/2204223005/diff/320001/blimp/client/core/sess... File blimp/client/core/session/identity_source.h (right): https://codereview.chromium.org/2204223005/diff/320001/blimp/client/core/sess... blimp/client/core/session/identity_source.h:32: // Start Blimp authentication by requesting OAuth2 token from Google. What happens if I call Connect() multiple times? Possibly with different callbacks? https://codereview.chromium.org/2204223005/diff/320001/blimp/client/core/sess... blimp/client/core/session/identity_source.h:61: // Callback to BlimpClientContextImpl after OAuth2 token is fetched. I don't think you need to mention BlimpClientContextImpl here, as this class is able to stand on its own :-) https://codereview.chromium.org/2204223005/diff/320001/blimp/client/core/sess... File blimp/client/core/session/identity_source_unittest.cc (right): https://codereview.chromium.org/2204223005/diff/320001/blimp/client/core/sess... blimp/client/core/session/identity_source_unittest.cc:85: IdentitySource::TokenCallback token_callback; Should this test ensure that the callback is invoked on success, and not invoked on failure? https://codereview.chromium.org/2204223005/diff/320001/chrome/browser/android... File chrome/browser/android/blimp/chrome_blimp_client_context_delegate.cc (right): https://codereview.chromium.org/2204223005/diff/320001/chrome/browser/android... chrome/browser/android/blimp/chrome_blimp_client_context_delegate.cc:62: VLOG(1) << "Unknown Blimp error."; I don't think this should ever happen, so maybe LOG(WARNING) ?
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the review, please help to take a look. Also @Roger, friendly ping, please help review: blimp/client/DEPS. https://codereview.chromium.org/2204223005/diff/320001/blimp/client/core/sess... File blimp/client/core/session/identity_source.cc (right): https://codereview.chromium.org/2204223005/diff/320001/blimp/client/core/sess... blimp/client/core/session/identity_source.cc:56: token_callback_.Run(access_token); On 2016/08/16 18:38:05, nyquist wrote: > Should this reset the callback? Removed token callback as parameter here, instead it sets the callback right after the ctor of identity source. Also changed the ctor of IdentitySource, to hide some construction details. Now IdentitySource is coupled with BlimpClientContextDelegate. https://codereview.chromium.org/2204223005/diff/320001/blimp/client/core/sess... blimp/client/core/session/identity_source.cc:61: void IdentitySource::OnGetTokenFailure( On 2016/08/16 18:38:05, nyquist wrote: > Should this also reset the callback? Same as above. https://codereview.chromium.org/2204223005/diff/320001/blimp/client/core/sess... File blimp/client/core/session/identity_source.h (right): https://codereview.chromium.org/2204223005/diff/320001/blimp/client/core/sess... blimp/client/core/session/identity_source.h:32: // Start Blimp authentication by requesting OAuth2 token from Google. On 2016/08/16 18:38:05, nyquist wrote: > What happens if I call Connect() multiple times? Possibly with different > callbacks? This is a good point. Added a bool to filter duplicate connect attempts. https://codereview.chromium.org/2204223005/diff/320001/blimp/client/core/sess... blimp/client/core/session/identity_source.h:61: // Callback to BlimpClientContextImpl after OAuth2 token is fetched. On 2016/08/16 18:38:06, nyquist wrote: > I don't think you need to mention BlimpClientContextImpl here, as this class is > able to stand on its own :-) Done. https://codereview.chromium.org/2204223005/diff/320001/blimp/client/core/sess... File blimp/client/core/session/identity_source_unittest.cc (right): https://codereview.chromium.org/2204223005/diff/320001/blimp/client/core/sess... blimp/client/core/session/identity_source_unittest.cc:85: IdentitySource::TokenCallback token_callback; On 2016/08/16 18:38:06, nyquist wrote: > Should this test ensure that the callback is invoked on success, and not invoked > on failure? Fixed, also move a couple of fake objects into MockBlimpClientContextDelegate. Also it tests duplicate connect calls, to ensure all the callbacks working correctly. https://codereview.chromium.org/2204223005/diff/320001/chrome/browser/android... File chrome/browser/android/blimp/chrome_blimp_client_context_delegate.cc (right): https://codereview.chromium.org/2204223005/diff/320001/chrome/browser/android... chrome/browser/android/blimp/chrome_blimp_client_context_delegate.cc:62: VLOG(1) << "Unknown Blimp error."; On 2016/08/16 18:38:06, nyquist wrote: > I don't think this should ever happen, so maybe LOG(WARNING) ? Done.
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm blimp/client/DEPS
lgtm % tommy. https://codereview.chromium.org/2204223005/diff/380001/blimp/client/core/sess... File blimp/client/core/session/identity_source_unittest.cc (right): https://codereview.chromium.org/2204223005/diff/380001/blimp/client/core/sess... blimp/client/core/session/identity_source_unittest.cc:122: DCHECK_EQ(auth.Succeeded(), 0); Can we add a way to reset the counts so it's easier to list exact expectations for each subtest here?
lgtm % nit and dtrainor@ https://codereview.chromium.org/2204223005/diff/380001/blimp/client/core/sess... File blimp/client/core/session/identity_source.h (right): https://codereview.chromium.org/2204223005/diff/380001/blimp/client/core/sess... blimp/client/core/session/identity_source.h:33: void Connect(); Nit: Could you add a comment similar to |is_fetching_token_|?
https://codereview.chromium.org/2204223005/diff/380001/blimp/client/core/sess... File blimp/client/core/session/identity_source.h (right): https://codereview.chromium.org/2204223005/diff/380001/blimp/client/core/sess... blimp/client/core/session/identity_source.h:36: void SetTokenCallback(const TokenCallback& callback); Could you move this to become a constructor argument?
Fixed all nits issues. Will put the CL into commit queue soon. https://codereview.chromium.org/2204223005/diff/380001/blimp/client/core/sess... File blimp/client/core/session/identity_source.h (right): https://codereview.chromium.org/2204223005/diff/380001/blimp/client/core/sess... blimp/client/core/session/identity_source.h:33: void Connect(); On 2016/08/18 17:43:52, nyquist wrote: > Nit: Could you add a comment similar to |is_fetching_token_|? Done. https://codereview.chromium.org/2204223005/diff/380001/blimp/client/core/sess... blimp/client/core/session/identity_source.h:36: void SetTokenCallback(const TokenCallback& callback); On 2016/08/18 17:46:46, nyquist wrote: > Could you move this to become a constructor argument? Done. https://codereview.chromium.org/2204223005/diff/380001/blimp/client/core/sess... File blimp/client/core/session/identity_source_unittest.cc (right): https://codereview.chromium.org/2204223005/diff/380001/blimp/client/core/sess... blimp/client/core/session/identity_source_unittest.cc:122: DCHECK_EQ(auth.Succeeded(), 0); On 2016/08/18 17:27:06, David Trainor wrote: > Can we add a way to reset the counts so it's easier to list exact expectations > for each subtest here? Done.
The CQ bit was checked by xingliu@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by xingliu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org, dtrainor@chromium.org, rogerta@chromium.org Link to the patchset: https://codereview.chromium.org/2204223005/#ps400001 (title: "Fixes for nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Blimp OAuth2 token retreival on application start up. Previously Blimp runs in its own app, and we have our own token source implementation. This cl implement the OAuth2 token flow in c++. There is a known issue that connect must be called after account seeding in android layer, or token request can be flaky since account seeding might cause request canceled. BUG=624457 ========== to ========== Blimp OAuth2 token retreival on application start up. Previously Blimp runs in its own app, and we have our own token source implementation. This cl implement the OAuth2 token flow in c++. There is a known issue that connect must be called after account seeding in android layer, or token request can be flaky since account seeding might cause request canceled. BUG=624457 ==========
Message was sent while issue was closed.
Committed patchset #21 (id:400001)
Message was sent while issue was closed.
Description was changed from ========== Blimp OAuth2 token retreival on application start up. Previously Blimp runs in its own app, and we have our own token source implementation. This cl implement the OAuth2 token flow in c++. There is a known issue that connect must be called after account seeding in android layer, or token request can be flaky since account seeding might cause request canceled. BUG=624457 ========== to ========== Blimp OAuth2 token retreival on application start up. Previously Blimp runs in its own app, and we have our own token source implementation. This cl implement the OAuth2 token flow in c++. There is a known issue that connect must be called after account seeding in android layer, or token request can be flaky since account seeding might cause request canceled. BUG=624457 Committed: https://crrev.com/f748905541816e1fede06d2ad01b96075e727bc4 Cr-Commit-Position: refs/heads/master@{#412923} ==========
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/f748905541816e1fede06d2ad01b96075e727bc4 Cr-Commit-Position: refs/heads/master@{#412923} |
