Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1618193003:
arc: Pass auth token from Chrome to ARC instance. (Closed)
|
Created:
4 years, 11 months ago by khmel Modified:
3 years, 9 months ago CC:
chromium-reviews, sadrul, dzhioev+watch_chromium.org, achuith+watch_chromium.org, oshima+watch_chromium.org, kalyank, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptionarc: Fetch and pass auth code from Chrome to ARC instance.
This supports communication via Google api to get an auth
code for GMS Core client and pass it to ARC instance. There
are two types of getting the auth code. Firstly, we try to
get it silently and secondary, in case when an additional
input is required from the user WebView dialog is shown.
Changed logic of starting ARC instance. Now its start is
controlled by ArcAuthService after communication with
Google auth servers.
BUG=571146
TEST=Provide additional unit_tests.
TEST=logcat shows that auth code is passed to ARC instance.
Both modes are tested: silent and WebView based (can
be forced by re-imaging)
Committed: https://crrev.com/0d5f977b77bc8d3c9f9dd78724fe7eecd6066e73
Cr-Commit-Position: refs/heads/master@{#372222}
Patch Set 1 #
Total comments: 2
Patch Set 2 : minor formating #
Total comments: 12
Patch Set 3 : Rebased, refactored OptInManager to ArcAuthService #
Total comments: 13
Patch Set 4 : rename token->code #Patch Set 5 : Refactored. Added LSO UI #
Total comments: 13
Patch Set 6 : use gaia_auth_fetcher. comments addressed #Patch Set 7 : fix accidental change #
Total comments: 28
Patch Set 8 : comments addressed #Patch Set 9 : rebased #
Total comments: 33
Patch Set 10 : #Patch Set 11 : Use DCHEC_NE/EQ #
Total comments: 1
Patch Set 12 : update BUILD.gn #Messages
Total messages: 42 (14 generated)
khmel@chromium.org changed reviewers: + elijahtaylor@chromium.org, lhchavez@chromium.org
This is first CL to enable opt-in workflow. PTAL https://codereview.chromium.org/1618193003/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_opt_in_manager_impl.cc (right): https://codereview.chromium.org/1618193003/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_opt_in_manager_impl.cc:27: static bool CookiePartsContains(const std::vector<std::string>& parts, Works with cookies here and at some places below are made similar to GaiaAuthFetcher: https://chromium.googlesource.com/chromium/chromium/+/483602073c62fc84068f547... However it is not possible to reuse GaiaAuthFetcher because it does not work with arbitrary clients. https://codereview.chromium.org/1618193003/diff/1/chrome/browser/ui/ash/chrom... File chrome/browser/ui/ash/chrome_shell_delegate.h (left): https://codereview.chromium.org/1618193003/diff/1/chrome/browser/ui/ash/chrom... chrome/browser/ui/ash/chrome_shell_delegate.h:97: class ArcSessionObserver : public ash::ShellObserver { We had 2 points to handle user login. I removed this code and use user_session_manager.cc.
lhchavez@google.com changed reviewers: + lhchavez@google.com
https://codereview.chromium.org/1618193003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_opt_in_manager_impl.h (right): https://codereview.chromium.org/1618193003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_opt_in_manager_impl.h:24: class ArcOptInManagerImpl : public ArcOptInManager, You should be able to s/ArcOptInManager/ArcService/ https://codereview.chromium.org/1618193003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_opt_in_manager_impl.h:50: Profile* profile_ = nullptr; If you end up taking the Profile* in the constructor, make this Profile* const profile_; https://codereview.chromium.org/1618193003/diff/20001/components/arc/arc_serv... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/1618193003/diff/20001/components/arc/arc_serv... components/arc/arc_service_manager.cc:81: arc_opt_in_manager_->SetProfile(profile); Is it possible to construct the ArcOptInManager with the Profile* whenever it's ready and then add it to the service manager instead of having to do this in two steps? https://codereview.chromium.org/1618193003/diff/20001/components/arc/arc_serv... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1618193003/diff/20001/components/arc/arc_serv... components/arc/arc_service_manager.h:34: scoped_ptr<ArcAuthService> auth_service, Please rebase, this interface changed. Now you need to .AddService(make_scoped_ptr(...)) instead of modifying this class. https://codereview.chromium.org/1618193003/diff/20001/components/arc/opt_in/a... File components/arc/opt_in/arc_opt_in_manager.h (right): https://codereview.chromium.org/1618193003/diff/20001/components/arc/opt_in/a... components/arc/opt_in/arc_opt_in_manager.h:16: class ArcOptInManager { The only implementation that exists is ArcOptInManagerImpl. You should be able to remove this completely and rename ArcOptInManagerImpl to ArcOptInManager. https://codereview.chromium.org/1618193003/diff/20001/components/arc/opt_in/a... components/arc/opt_in/arc_opt_in_manager.h:35: virtual State state() const = 0; Only inlined trivial functions can be lowercase. Either have an inline definition or make this GetState().
also, s/Crome/Chrome/ on the issue title/commit message? :P
Description was changed from
==========
arc: Pass auth token form Crome to ARC instance.
This supports silen communication via google api to get auth token
for ARC instance. Fetched auth token is passed to ARC. Changed
logic of starting ARC instance. Now its start is controlled by
opt-in manager after communication with google auth servers.
BUG=571146
TEST=Provide additional unit_tests.
TEST=logcat shows that auth token is passed to ARC instance.
ARC starts on token fetched. ARC starts when token is not
fetched (this is temporary solution until UI for login
is ready).
==========
to
==========
arc: Pass auth token form Chrome to ARC instance.
This supports silen communication via google api to get auth token
for ARC instance. Fetched auth token is passed to ARC. Changed
logic of starting ARC instance. Now its start is controlled by
opt-in manager after communication with google auth servers.
BUG=571146
TEST=Provide additional unit_tests.
TEST=logcat shows that auth token is passed to ARC instance.
ARC starts on token fetched. ARC starts when token is not
fetched (this is temporary solution until UI for login
is ready).
==========
Description was changed from
==========
arc: Pass auth token form Chrome to ARC instance.
This supports silen communication via google api to get auth token
for ARC instance. Fetched auth token is passed to ARC. Changed
logic of starting ARC instance. Now its start is controlled by
opt-in manager after communication with google auth servers.
BUG=571146
TEST=Provide additional unit_tests.
TEST=logcat shows that auth token is passed to ARC instance.
ARC starts on token fetched. ARC starts when token is not
fetched (this is temporary solution until UI for login
is ready).
==========
to
==========
arc: Pass auth token from Chrome to ARC instance.
This supports silent communication via google api to get auth token
for ARC instance. Fetched auth token is passed to ARC. Changed
logic of starting ARC instance. Now its start is controlled by
opt-in manager after communication with google auth servers.
BUG=571146
TEST=Provide additional unit_tests.
TEST=logcat shows that auth token is passed to ARC instance.
ARC starts on token fetched. ARC starts when token is not
fetched (this is temporary solution until UI for login
is ready).
==========
Hi Luis, Thanks for review. I rebased the code and moved ArcOptInManager to the existing ArcAuthService. This simplifies a little bit. PTAL. https://codereview.chromium.org/1618193003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_opt_in_manager_impl.h (right): https://codereview.chromium.org/1618193003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_opt_in_manager_impl.h:24: class ArcOptInManagerImpl : public ArcOptInManager, On 2016/01/22 22:18:33, lhc(google) wrote: > You should be able to s/ArcOptInManager/ArcService/ I moved code to ArcAuthService. It seems the right place in order not to multiply classes with the same functionality. https://codereview.chromium.org/1618193003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_opt_in_manager_impl.h:50: Profile* profile_ = nullptr; On 2016/01/22 22:18:33, lhc(google) wrote: > If you end up taking the Profile* in the constructor, make this Profile* const > profile_; Actually profile_ can be and should be changed. ArcOptInManager must be available all the time and there are several classes will observer opt in state change (UI for opt in as an example). https://codereview.chromium.org/1618193003/diff/20001/components/arc/arc_serv... File components/arc/arc_service_manager.cc (right): https://codereview.chromium.org/1618193003/diff/20001/components/arc/arc_serv... components/arc/arc_service_manager.cc:81: arc_opt_in_manager_->SetProfile(profile); On 2016/01/22 22:18:33, lhc(google) wrote: > Is it possible to construct the ArcOptInManager with the Profile* whenever it's > ready and then add it to the service manager instead of having to do this in two > steps? I answered in previous comment. https://codereview.chromium.org/1618193003/diff/20001/components/arc/arc_serv... File components/arc/arc_service_manager.h (right): https://codereview.chromium.org/1618193003/diff/20001/components/arc/arc_serv... components/arc/arc_service_manager.h:34: scoped_ptr<ArcAuthService> auth_service, On 2016/01/22 22:18:33, lhc(google) wrote: > Please rebase, this interface changed. Now you need to > .AddService(make_scoped_ptr(...)) instead of modifying this class. Rebased. Also moved code to ArcAuthService. https://codereview.chromium.org/1618193003/diff/20001/components/arc/opt_in/a... File components/arc/opt_in/arc_opt_in_manager.h (right): https://codereview.chromium.org/1618193003/diff/20001/components/arc/opt_in/a... components/arc/opt_in/arc_opt_in_manager.h:16: class ArcOptInManager { On 2016/01/22 22:18:33, lhc(google) wrote: > The only implementation that exists is ArcOptInManagerImpl. You should be able > to remove this completely and rename ArcOptInManagerImpl to ArcOptInManager. Done. https://codereview.chromium.org/1618193003/diff/20001/components/arc/opt_in/a... components/arc/opt_in/arc_opt_in_manager.h:35: virtual State state() const = 0; On 2016/01/22 22:18:33, lhc(google) wrote: > Only inlined trivial functions can be lowercase. Either have an inline > definition or make this GetState(). Done.
haven't looked in great detail yet, but wanted to get early comments out https://codereview.chromium.org/1618193003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1618193003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:24: "1070009224336-sdh77n7uot3oc99ais00jmuft6sk2fg9.apps.googleusercontent.com"; what client ID is this? https://codereview.chromium.org/1618193003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:37: if (base::LowerCaseEqualsASCII(*it, part)) I'm no expert, but there is disagreement on whether cookie names are case-sensitive or not: http://stackoverflow.com/a/11312272 https://codereview.chromium.org/1618193003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:63: DCHECK(g_arc_auth_service->thread_checker_.CalledOnValidThread()); I wonder if this pointer should be managed by ArcAuthService at all. Since this class is owned by the ArcServiceManager, can we maybe just make a call to ArcServiceMAnager here and avoid holding this weak reference? https://codereview.chromium.org/1618193003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:133: "?scope=%s&client_id=%s&email=%s", GaiaConstants::kOAuth1LoginScope, please make sure to get approval from an OWNER from where this scope constant and GaiaUrls comes from https://codereview.chromium.org/1618193003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:149: net::URLFetcher::Create(CreateURL(profile_), net::URLFetcher::GET, this); is it ok to have this code enabled in Chromium? Is this API and client ID specific to Chrome? https://codereview.chromium.org/1618193003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:194: ArcBridgeService::Get()->HandleStartup(); I think we shouldn't start the bridge in this case, and just log an error https://codereview.chromium.org/1618193003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/1618193003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.h:73: // Overrides AuthHost. For security reason this token can be used only I'm confused by the token/code terminology. IIUC we are only getting Auth codes to pass to ARC. If that's the case, I think mixing "Token" here confuses things. Can you clarify the difference in comments or fix the code?
Hi Elijah, Thanks for review. PTAL https://codereview.chromium.org/1618193003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1618193003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:24: "1070009224336-sdh77n7uot3oc99ais00jmuft6sk2fg9.apps.googleusercontent.com"; On 2016/01/26 01:02:14, elijahtaylor1 wrote: > what client ID is this? This is GMS Core Client id. I renamed for better reading https://codereview.chromium.org/1618193003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:37: if (base::LowerCaseEqualsASCII(*it, part)) On 2016/01/26 01:02:14, elijahtaylor1 wrote: > I'm no expert, but there is disagreement on whether cookie names are > case-sensitive or not: http://stackoverflow.com/a/11312272 I mentioned earlier that some code is taken from https://code.google.com/p/chromium/codesearch#chromium/src/google_apis/gaia/g... There cookies are case-insensetive https://codereview.chromium.org/1618193003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:63: DCHECK(g_arc_auth_service->thread_checker_.CalledOnValidThread()); On 2016/01/26 01:02:14, elijahtaylor1 wrote: > I wonder if this pointer should be managed by ArcAuthService at all. Since this > class is owned by the ArcServiceManager, can we maybe just make a call to > ArcServiceMAnager here and avoid holding this weak reference? After recent ArcServiceManager refactoring, it contains only ArcService interfaces array. It is impossible now to get from ArcService ArcAuthService. Luis, could you please confirm. https://codereview.chromium.org/1618193003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:133: "?scope=%s&client_id=%s&email=%s", GaiaConstants::kOAuth1LoginScope, On 2016/01/26 01:02:14, elijahtaylor1 wrote: > please make sure to get approval from an OWNER from where this scope constant > and GaiaUrls comes from Ok, I will add someone. https://codereview.chromium.org/1618193003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:149: net::URLFetcher::Create(CreateURL(profile_), net::URLFetcher::GET, this); On 2016/01/26 01:02:14, elijahtaylor1 wrote: > is it ok to have this code enabled in Chromium? Is this API and client ID > specific to Chrome? There is a tons of usage URLFetcher under chrome/browser folder. There is also usage of GaiaAuthFetcher that does similar but for Chrome client id. It seems I do nothing unusual here. https://codereview.chromium.org/1618193003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service.cc:194: ArcBridgeService::Get()->HandleStartup(); On 2016/01/26 01:02:14, elijahtaylor1 wrote: > I think we shouldn't start the bridge in this case, and just log an error This is done temporarily, until we have UI that shows LSO page to user to enter a password (comes as separate CL). For example with fresh image and first login, current google api does not return cookies and asks user to enter password additionally.
Hi, Originally I decided to split CL into two parts, one for low-level parts and second for UI that handles LSO page. However, this was inconvenient and introduced several stubs and made the code less readable. Considered pros/cons I decided to join them in one CLs. So this CLs does following: On user login, it automatically assumes that ARC is opted-in by default because there is no current UI (chrome notification, dialog, app list page, settings) to handle opt-in process (current CL has to go first). ArcAuthFetcher is created, and it sends a request to google apis to request GMS Core Auth code. Following results are possible * Auth code is automatically fetched from cookies. We start ARC and pass the auth code to it. * Request completed successfully, but no cookies are returned. That indicates that we need to start Web dialog and ask for required input from a user. (final LSO page is not implemented by server team, and we need to use programmatic_auth service so far) * Request failed for some reason. At this moment do nothing. If additional UI is required we create WebView based dialog and display LSO page (user needs to enter a password as it currently implemented via programmatic_auth service). Each time when a page is completed, we check the presence of auth cookies. If auth code is extracted from cookies, we start ARC and pass the auth code to it. PTAL
Description was changed from
==========
arc: Pass auth token from Chrome to ARC instance.
This supports silent communication via google api to get auth token
for ARC instance. Fetched auth token is passed to ARC. Changed
logic of starting ARC instance. Now its start is controlled by
opt-in manager after communication with google auth servers.
BUG=571146
TEST=Provide additional unit_tests.
TEST=logcat shows that auth token is passed to ARC instance.
ARC starts on token fetched. ARC starts when token is not
fetched (this is temporary solution until UI for login
is ready).
==========
to
==========
arc: Fetch and pass auth code from Chrome to ARC instance.
This supports communication via Google api to get an auth
code for GMS Core client and pass it to ARC instance. There
are two types of getting the auth code. Firstly, we try to
get it silently and secondary, in case when an additional
input is required from the user WebView dialog is shown.
Changed logic of starting ARC instance. Now its start is
controlled by ArcAuthService after communication with
Google auth servers.
BUG=571146
TEST=Provide additional unit_tests.
TEST=logcat shows that auth code is passed to ARC instance.
Both modes are tested: silent and WebView based (can
be forced by re-imaging)
==========
khmel@chromium.org changed reviewers: + oshima@chromium.org, xiyuan@chromium.org
PTAL https://codereview.chromium.org/1618193003/diff/80001/chrome/browser/ui/ash/c... File chrome/browser/ui/ash/chrome_shell_delegate.h (left): https://codereview.chromium.org/1618193003/diff/80001/chrome/browser/ui/ash/c... chrome/browser/ui/ash/chrome_shell_delegate.h:21: #if defined(OS_CHROMEOS) This is just revert previous ARC changes, not user_manager_session is used.
https://codereview.chromium.org/1618193003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_fetcher.cc (right): https://codereview.chromium.org/1618193003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_fetcher.cc:61: auth_fetcher_->SetRequestContext(profile_->GetRequestContext()); Looks like the class does not need the whole |profile_|. Can we just keep a reference of net::URLRequestContextGetter* instead? https://codereview.chromium.org/1618193003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_fetcher.h (right): https://codereview.chromium.org/1618193003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_fetcher.h:24: class ArcAuthFetcher : public net::URLFetcherDelegate { Is it possible to extend GaiaAuthFetcher to support what we are trying to do with the class? It looks like we can reuse GaiaAuthFetcher::StartCookieForOAuthLoginTokenExchangeWithDeviceId [1]. The difference is that we need to be able to pass a different client id and needs to stop after auth code is fetched. [1] https://code.google.com/p/chromium/codesearch#chromium/src/google_apis/gaia/g... https://codereview.chromium.org/1618193003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service_unittest.cc (right): https://codereview.chromium.org/1618193003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service_unittest.cc:36: base::ScopedTempDir temp_dir; ScopedTempDir would auto delete the temp dir when it goes out of scope. We should not be using it like this because the returned TestingProfile might still need access to the dir. https://codereview.chromium.org/1618193003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service_unittest.cc:89: std::string rt_cookie_; Data bembers are always declared last, i.e. move these after FakeURLFetcherCreator(). https://codereview.chromium.org/1618193003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/1618193003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:1149: arc::ArcAuthService::Get()->SetProfile(profile); nit: Prefer to have something similar to ArcServiceManager::OnPrimaryUserProfilePrepared and wrap the call in it. https://codereview.chromium.org/1618193003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:1793: arc::ArcAuthService::Get()->SetProfile(nullptr); nit: Similarly, prefer to have something like ArcServiceManager::Shutdown to wrap this. It is not obvious that SetProfile(nullptr) would call ArcBridgeService::Get()->Shutdown().
PTAL https://codereview.chromium.org/1618193003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_fetcher.cc (right): https://codereview.chromium.org/1618193003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_fetcher.cc:61: auth_fetcher_->SetRequestContext(profile_->GetRequestContext()); On 2016/01/26 23:37:05, xiyuan wrote: > Looks like the class does not need the whole |profile_|. Can we just keep a > reference of net::URLRequestContextGetter* instead? Good point! Thanks https://codereview.chromium.org/1618193003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_fetcher.h (right): https://codereview.chromium.org/1618193003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_fetcher.h:24: class ArcAuthFetcher : public net::URLFetcherDelegate { On 2016/01/26 23:37:05, xiyuan wrote: > Is it possible to extend GaiaAuthFetcher to support what we are trying to do > with the class? It looks like we can reuse > GaiaAuthFetcher::StartCookieForOAuthLoginTokenExchangeWithDeviceId [1]. The > difference is that we need to be able to pass a different client id and needs to > stop after auth code is fetched. > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/google_apis/gaia/g... Honestly speaking I was thinking about this possibility and once you mention it here did it. Please take a look into separate CL (https://codereview.chromium.org/1644713002/). https://codereview.chromium.org/1618193003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_auth_service_unittest.cc (right): https://codereview.chromium.org/1618193003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service_unittest.cc:36: base::ScopedTempDir temp_dir; On 2016/01/26 23:37:05, xiyuan wrote: > ScopedTempDir would auto delete the temp dir when it goes out of scope. We > should not be using it like this because the returned TestingProfile might still > need access to the dir. Yes, overlooked this. Thanks. https://codereview.chromium.org/1618193003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_auth_service_unittest.cc:89: std::string rt_cookie_; On 2016/01/26 23:37:05, xiyuan wrote: > Data bembers are always declared last, i.e. move these after > FakeURLFetcherCreator(). Done. https://codereview.chromium.org/1618193003/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/login/session/user_session_manager.cc (right): https://codereview.chromium.org/1618193003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:1149: arc::ArcAuthService::Get()->SetProfile(profile); On 2016/01/26 23:37:05, xiyuan wrote: > nit: Prefer to have something similar to > ArcServiceManager::OnPrimaryUserProfilePrepared and wrap the call in it. Sounds better, thanks https://codereview.chromium.org/1618193003/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/login/session/user_session_manager.cc:1793: arc::ArcAuthService::Get()->SetProfile(nullptr); On 2016/01/26 23:37:05, xiyuan wrote: > nit: Similarly, prefer to have something like ArcServiceManager::Shutdown to > wrap this. It is not obvious that SetProfile(nullptr) would call > ArcBridgeService::Get()->Shutdown(). Done.
https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_fetcher.cc (right): https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_fetcher.cc:8: #include "google_apis/gaia/gaia_auth_fetcher.h" nit: not needed since we included it in header https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_fetcher.cc:53: } Seems we don't really need OnClientOAuthSuccess. Remove it or insert an empty line below. https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_fetcher.h (right): https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_fetcher.h:34: }; nit: add protected: virtual ~Delegate() {} just in case the derived class is not putting a proper one. https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_fetcher.h:52: Delegate* delegate_; nit: Delegate* const since it should never change. https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:90: state_ = State::DISABLE; nit: We have multiple places using state_ then FOR_EACH_OBSERVER(...). Can we create a SetState for it? https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.h:32: FETCHING_CODE, // ARC is allowed, receiving auth_2 code. nit: strip one space between "receiving" and "auth_2" https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.h:41: }; nit: add a virtual ~Observer() {} https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service_unittest.cc (right): https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_unittest.cc:37: EXPECT_TRUE(base::CreateDirectory(path)); These two lines are not really needed since we only do it once in SetUp. temp_dir_.CreateUniqueTempDir should already guarantee that we get an empty dir. Think we can get rid of them and maybe move the whole function into SetUp. https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_unittest.cc:140: // Empty profile disables opt-in. nit: Update comment? https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_ui.cc (right): https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_ui.cc:84: if (source->GetLoadState().state == net::LoadState::LOAD_STATE_IDLE) A couple of questions: - Is LOAD_STATE_IDLE used here to detect when page loading is stopped? I seem not able to get that from [1]. Maybe use source->IsLoading(), which might be a better signal. - Does this mean that we kick start a ArcAuthFetcher on every page load during the web auth flow? Is there any better signal we can use, e.g. would we get some fixed landing URL at the end of flow? ==== [1] https://code.google.com/p/chromium/codesearch#chromium/src/net/base/load_stat... https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_ui.cc:86: new ArcAuthFetcher(browser_context_->GetRequestContext(), this)); nit: need {} when the branch has more than one line. https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_ui.h (right): https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_ui.h:14: } // namespace content nit: // namespace xxx could be omitted in header https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_ui.h:30: }; nit: virtual ~Delegate() {} https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_ui.h:60: Delegate* delegate_; nit: Delegate* const
PTAL https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_fetcher.cc (right): https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_fetcher.cc:8: #include "google_apis/gaia/gaia_auth_fetcher.h" On 2016/01/27 23:56:05, xiyuan wrote: > nit: not needed since we included it in header Done. https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_fetcher.cc:53: } On 2016/01/27 23:56:05, xiyuan wrote: > Seems we don't really need OnClientOAuthSuccess. Remove it or insert an empty > line below. Yes, this is redundant. I added this check for gaia_auth_fetcher. unit tests. Safe to remove. https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_fetcher.h (right): https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_fetcher.h:34: }; On 2016/01/27 23:56:05, xiyuan wrote: > nit: add > > protected: > virtual ~Delegate() {} > > just in case the derived class is not putting a proper one. Done. https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_fetcher.h:52: Delegate* delegate_; On 2016/01/27 23:56:05, xiyuan wrote: > nit: Delegate* const since it should never change. Done. https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:90: state_ = State::DISABLE; On 2016/01/27 23:56:05, xiyuan wrote: > nit: We have multiple places using state_ then FOR_EACH_OBSERVER(...). Can we > create a SetState for it? > Done https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.h:32: FETCHING_CODE, // ARC is allowed, receiving auth_2 code. On 2016/01/27 23:56:05, xiyuan wrote: > nit: strip one space between "receiving" and "auth_2" Done. https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.h:41: }; On 2016/01/27 23:56:05, xiyuan wrote: > nit: add a virtual ~Observer() {} Done. https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service_unittest.cc (right): https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_unittest.cc:37: EXPECT_TRUE(base::CreateDirectory(path)); On 2016/01/27 23:56:05, xiyuan wrote: > These two lines are not really needed since we only do it once in SetUp. > temp_dir_.CreateUniqueTempDir should already guarantee that we get an empty dir. > Think we can get rid of them and maybe move the whole function into SetUp. Makes sense, thanks https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_unittest.cc:140: // Empty profile disables opt-in. On 2016/01/27 23:56:05, xiyuan wrote: > nit: Update comment? Done. https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_ui.cc (right): https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_ui.cc:84: if (source->GetLoadState().state == net::LoadState::LOAD_STATE_IDLE) On 2016/01/27 23:56:05, xiyuan wrote: > A couple of questions: > > - Is LOAD_STATE_IDLE used here to detect when page loading is stopped? I seem > not able to get that from [1]. Maybe use source->IsLoading(), which might be a > better signal. > > - Does this mean that we kick start a ArcAuthFetcher on every page load during > the web auth flow? Is there any better signal we can use, e.g. would we get some > fixed landing URL at the end of flow? > > ==== > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/net/base/load_stat... Thanks for good hints! Yes, LOAD_STATE_IDLE appears frequently and we do a lot of unnecessary work. IsLoading check is much better and we also can check only pages that can contain required cookies and ignore all intermediate steps. https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_ui.cc:86: new ArcAuthFetcher(browser_context_->GetRequestContext(), this)); On 2016/01/27 23:56:05, xiyuan wrote: > nit: need {} when the branch has more than one line. Done. https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_ui.h (right): https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_ui.h:14: } // namespace content On 2016/01/27 23:56:05, xiyuan wrote: > nit: // namespace xxx could be omitted in header Done. https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_ui.h:30: }; On 2016/01/27 23:56:05, xiyuan wrote: > nit: virtual ~Delegate() {} Done. https://codereview.chromium.org/1618193003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_ui.h:60: Delegate* delegate_; On 2016/01/27 23:56:05, xiyuan wrote: > nit: Delegate* const Done.
lgtm https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_fetcher.cc (right): https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_fetcher.cc:8: #include "google_apis/gaia/gaia_auth_fetcher.h" nit: The header is still here. :) https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_ui.cc (right): https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_ui.cc:93: return; nit: wrap with {} since conditions span more than 1 line.
https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_fetcher.cc (right): https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_fetcher.cc:42: "", /* session_index */ std::string() same for other ""s. https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_fetcher.h (right): https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_fetcher.h:3: // found in the LICENSE file. Can this be in components/arc instead? Looks like all you need is BrowserContext, not Profile. https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_fetcher.h:36: virtual ~Delegate() {} = default; same for other default ctor/dtor. https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:18: ArcAuthService* g_arc_auth_service = nullptr; optional: you don't need g_ because this is file scope. g_ is for global. https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:58: std::string ArcAuthService::GetAuthCode() { GetAndResetAutoCode ? https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:112: DCHECK(auth_ui_ == nullptr); DCHEK(!auth_ui_) https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:119: DCHECK(state_ != State::ENABLE); DCHECK_EQ. same for others https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:134: auth_code_ = ""; auth_code_.clear() https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:160: DCHECK(auth_ui_ != nullptr); DCHECK(auth_ui_) https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.h:59: void SetAuthCodeAndStartArc(const std::string auth_code); std::string& https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service_unittest.cc (right): https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_unittest.cc:90: rt_cookie_ = ""; .clear() https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_ui.h (right): https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_ui.h:40: // overrides ui::WebDialogDelegate. // ui::WebDialogDelegate: https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_ui.h:55: // Overrides ArcAuthFetcher::Delegate. ditto
components/arc lgtm, overall design lgtm but left details to xiyuan and oshima
Hi Mitsuru, Thanks for your review. I moved ArcAuthService to components/arc/auth and did other style updates. PTAL https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_fetcher.cc (right): https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_fetcher.cc:8: #include "google_apis/gaia/gaia_auth_fetcher.h" On 2016/01/28 16:18:07, xiyuan wrote: > nit: The header is still here. :) :) My fault. Thanks https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_fetcher.cc:42: "", /* session_index */ On 2016/01/28 17:46:40, oshima wrote: > std::string() > > same for other ""s. Done. https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_fetcher.h (right): https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_fetcher.h:3: // found in the LICENSE file. On 2016/01/28 17:46:40, oshima wrote: > Can this be in components/arc instead? Looks like all you need is > BrowserContext, not Profile. Yes, we can move. https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_fetcher.h:36: virtual ~Delegate() {} On 2016/01/28 17:46:40, oshima wrote: > = default; > > same for other default ctor/dtor. Done. https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:18: ArcAuthService* g_arc_auth_service = nullptr; On 2016/01/28 17:46:40, oshima wrote: > optional: you don't need g_ because this is file scope. g_ is for global. Done. https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:18: ArcAuthService* g_arc_auth_service = nullptr; On 2016/01/28 17:46:40, oshima wrote: > optional: you don't need g_ because this is file scope. g_ is for global. Done. https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:58: std::string ArcAuthService::GetAuthCode() { On 2016/01/28 17:46:40, oshima wrote: > GetAndResetAutoCode ? Yes, more clear https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:112: DCHECK(auth_ui_ == nullptr); On 2016/01/28 17:46:41, oshima wrote: > DCHEK(!auth_ui_) Done. https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:119: DCHECK(state_ != State::ENABLE); On 2016/01/28 17:46:40, oshima wrote: > DCHECK_EQ. same for others I cannot use DCHECK_EQ(NE) with State. It cannot be compiled: In file included from ../../base/observer_list.h:14: ../../base/logging.h:523:23: error: invalid operands to binary expression ... https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:134: auth_code_ = ""; On 2016/01/28 17:46:40, oshima wrote: > auth_code_.clear() Done. https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:160: DCHECK(auth_ui_ != nullptr); On 2016/01/28 17:46:40, oshima wrote: > DCHECK(auth_ui_) Done. https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.h (right): https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.h:59: void SetAuthCodeAndStartArc(const std::string auth_code); On 2016/01/28 17:46:41, oshima wrote: > std::string& Done. https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service_unittest.cc (right): https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_unittest.cc:90: rt_cookie_ = ""; On 2016/01/28 17:46:41, oshima wrote: > .clear() Done. https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_ui.cc (right): https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_ui.cc:93: return; On 2016/01/28 16:18:07, xiyuan wrote: > nit: wrap with {} since conditions span more than 1 line. Done. https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_ui.h (right): https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_ui.h:40: // overrides ui::WebDialogDelegate. On 2016/01/28 17:46:41, oshima wrote: > // ui::WebDialogDelegate: Done. https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_ui.h:55: // Overrides ArcAuthFetcher::Delegate. On 2016/01/28 17:46:41, oshima wrote: > ditto Done.
https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:119: DCHECK(state_ != State::ENABLE); On 2016/01/28 19:24:02, khmel wrote: > On 2016/01/28 17:46:40, oshima wrote: > > DCHECK_EQ. same for others > > I cannot use DCHECK_EQ(NE) with State. It cannot be compiled: > In file included from ../../base/observer_list.h:14: > ../../base/logging.h:523:23: error: invalid operands to binary expression > ... that's probably because you use enum class (instead of enum) and don't have operator<< for that class. I still recommend to use _NE/EQ because it'll give the current value. (I'm fine with either way)
PTAL https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1618193003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:119: DCHECK(state_ != State::ENABLE); On 2016/01/28 20:39:54, oshima wrote: > On 2016/01/28 19:24:02, khmel wrote: > > On 2016/01/28 17:46:40, oshima wrote: > > > DCHECK_EQ. same for others > > > > I cannot use DCHECK_EQ(NE) with State. It cannot be compiled: > > In file included from ../../base/observer_list.h:14: > > ../../base/logging.h:523:23: error: invalid operands to binary expression > > ... > > that's probably because you use enum class (instead of enum) and don't have > operator<< for that class. I still recommend to use _NE/EQ because it'll give > the current value. (I'm fine with either way) Yes, you are right. This works fine. Thanks for help!
lgtm https://codereview.chromium.org/1618193003/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/1618193003/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service.cc:26: const char kStateEnable[] = "ENABLE"; While I prefer this way, IIRC, you could have just print int value using static_cast<int>(enum_value). It's up to you.
khmel@chromium.org changed reviewers: + zelidrag@chromium.org
Adding Zel to review components/arc/auth/DEPS. PTAL
lgtm
The CQ bit was checked by khmel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from elijahtaylor@chromium.org, xiyuan@chromium.org Link to the patchset: https://codereview.chromium.org/1618193003/#ps200001 (title: "Use DCHEC_NE/EQ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1618193003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1618193003/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
looks like you need to update DEPS
On 2016/01/28 23:20:47, oshima wrote: > looks like you need to update DEPS It seems forgot about BUILD.gn. Will try one more time.
The CQ bit was checked by khmel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from elijahtaylor@chromium.org, xiyuan@chromium.org, oshima@chromium.org, zelidrag@chromium.org Link to the patchset: https://codereview.chromium.org/1618193003/#ps220001 (title: "update BUILD.gn")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1618193003/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1618193003/220001
Message was sent while issue was closed.
Description was changed from
==========
arc: Fetch and pass auth code from Chrome to ARC instance.
This supports communication via Google api to get an auth
code for GMS Core client and pass it to ARC instance. There
are two types of getting the auth code. Firstly, we try to
get it silently and secondary, in case when an additional
input is required from the user WebView dialog is shown.
Changed logic of starting ARC instance. Now its start is
controlled by ArcAuthService after communication with
Google auth servers.
BUG=571146
TEST=Provide additional unit_tests.
TEST=logcat shows that auth code is passed to ARC instance.
Both modes are tested: silent and WebView based (can
be forced by re-imaging)
==========
to
==========
arc: Fetch and pass auth code from Chrome to ARC instance.
This supports communication via Google api to get an auth
code for GMS Core client and pass it to ARC instance. There
are two types of getting the auth code. Firstly, we try to
get it silently and secondary, in case when an additional
input is required from the user WebView dialog is shown.
Changed logic of starting ARC instance. Now its start is
controlled by ArcAuthService after communication with
Google auth servers.
BUG=571146
TEST=Provide additional unit_tests.
TEST=logcat shows that auth code is passed to ARC instance.
Both modes are tested: silent and WebView based (can
be forced by re-imaging)
==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from
==========
arc: Fetch and pass auth code from Chrome to ARC instance.
This supports communication via Google api to get an auth
code for GMS Core client and pass it to ARC instance. There
are two types of getting the auth code. Firstly, we try to
get it silently and secondary, in case when an additional
input is required from the user WebView dialog is shown.
Changed logic of starting ARC instance. Now its start is
controlled by ArcAuthService after communication with
Google auth servers.
BUG=571146
TEST=Provide additional unit_tests.
TEST=logcat shows that auth code is passed to ARC instance.
Both modes are tested: silent and WebView based (can
be forced by re-imaging)
==========
to
==========
arc: Fetch and pass auth code from Chrome to ARC instance.
This supports communication via Google api to get an auth
code for GMS Core client and pass it to ARC instance. There
are two types of getting the auth code. Firstly, we try to
get it silently and secondary, in case when an additional
input is required from the user WebView dialog is shown.
Changed logic of starting ARC instance. Now its start is
controlled by ArcAuthService after communication with
Google auth servers.
BUG=571146
TEST=Provide additional unit_tests.
TEST=logcat shows that auth code is passed to ARC instance.
Both modes are tested: silent and WebView based (can
be forced by re-imaging)
Committed: https://crrev.com/0d5f977b77bc8d3c9f9dd78724fe7eecd6066e73
Cr-Commit-Position: refs/heads/master@{#372222}
==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/0d5f977b77bc8d3c9f9dd78724fe7eecd6066e73 Cr-Commit-Position: refs/heads/master@{#372222} |
