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

Issue 2498363002: Remove delegates from ArcAuthCodeFetcher and ArcAuthContext. (Closed)

Created:
4 years, 1 month ago by hidehiko
Modified:
4 years, 1 month ago
CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, khmel+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, khmel
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove delegates from ArcAuthCodeFetcher and ArcAuthContext. This CL replaces many "delegate" implementation by callbacks. Also, now ArcAuthContext's preparation is only for ArcAuthCodeFetcher, so it is done in the class. BUG=657687 BUG=b/31079732 TEST=Ran on test device. Ran bots. Committed: https://crrev.com/e4d65b979e7ccd7834db7d5c3037b1df9091f1d1 Cr-Commit-Position: refs/heads/master@{#432859}

Patch Set 1 #

Total comments: 11

Patch Set 2 : Address comments. #

Total comments: 2

Patch Set 3 : fix calback #

Patch Set 4 : shuffle method order #

Patch Set 5 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+165 lines, -225 lines) Patch
M chrome/browser/chromeos/BUILD.gn View 1 chunk +0 lines, -2 lines 0 comments Download
M chrome/browser/chromeos/arc/arc_auth_code_fetcher.h View 1 3 chunks +19 lines, -7 lines 0 comments Download
M chrome/browser/chromeos/arc/arc_auth_code_fetcher.cc View 1 7 chunks +39 lines, -27 lines 0 comments Download
D chrome/browser/chromeos/arc/arc_auth_code_fetcher_delegate.h View 1 chunk +0 lines, -23 lines 0 comments Download
M chrome/browser/chromeos/arc/arc_auth_context.h View 1 2 3 3 chunks +19 lines, -17 lines 0 comments Download
M chrome/browser/chromeos/arc/arc_auth_context.cc View 1 2 3 6 chunks +41 lines, -46 lines 0 comments Download
D chrome/browser/chromeos/arc/arc_auth_context_delegate.h View 1 chunk +0 lines, -21 lines 0 comments Download
M chrome/browser/chromeos/arc/arc_auth_service.h View 1 5 chunks +10 lines, -16 lines 0 comments Download
M chrome/browser/chromeos/arc/arc_auth_service.cc View 1 2 3 4 7 chunks +37 lines, -66 lines 0 comments Download

Messages

Total messages: 13 (4 generated)
hidehiko
Another preparation for ArcAuthService and ArcSessionManager separation. PTAL.
4 years, 1 month ago (2016-11-15 13:21:58 UTC) #2
Luis Héctor Chávez
https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc/arc_auth_code_fetcher.cc File chrome/browser/chromeos/arc/arc_auth_code_fetcher.cc (right): https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc/arc_auth_code_fetcher.cc#newcode82 chrome/browser/chromeos/arc/arc_auth_code_fetcher.cc:82: login_token_request_.reset( optional: can this be login_token_request_ = token_service->StartRequest(account_id, scoped, ...
4 years, 1 month ago (2016-11-15 23:27:38 UTC) #3
hidehiko
Thank you for review. PTAL. https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc/arc_auth_code_fetcher.cc File chrome/browser/chromeos/arc/arc_auth_code_fetcher.cc (right): https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc/arc_auth_code_fetcher.cc#newcode82 chrome/browser/chromeos/arc/arc_auth_code_fetcher.cc:82: login_token_request_.reset( On 2016/11/15 23:27:37, ...
4 years, 1 month ago (2016-11-16 01:00:34 UTC) #4
Luis Héctor Chávez
https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc/arc_auth_service.cc File chrome/browser/chromeos/arc/arc_auth_service.cc (right): https://codereview.chromium.org/2498363002/diff/1/chrome/browser/chromeos/arc/arc_auth_service.cc#newcode413 chrome/browser/chromeos/arc/arc_auth_service.cc:413: if (!auth_endpoint.empty()) { On 2016/11/16 01:00:34, hidehiko wrote: > ...
4 years, 1 month ago (2016-11-16 03:29:58 UTC) #5
hidehiko
Thank you for review. PTAL. Note: shuffling the method is done, too. https://codereview.chromium.org/2498363002/diff/20001/chrome/browser/chromeos/arc/arc_auth_context.cc File chrome/browser/chromeos/arc/arc_auth_context.cc ...
4 years, 1 month ago (2016-11-16 04:09:38 UTC) #6
Luis Héctor Chávez
lgtm
4 years, 1 month ago (2016-11-16 04:11:54 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2498363002/80001
4 years, 1 month ago (2016-11-17 11:59:19 UTC) #10
commit-bot: I haz the power
Committed patchset #5 (id:80001)
4 years, 1 month ago (2016-11-17 13:47:39 UTC) #11
commit-bot: I haz the power
4 years, 1 month ago (2016-11-17 13:50:34 UTC) #13
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/e4d65b979e7ccd7834db7d5c3037b1df9091f1d1
Cr-Commit-Position: refs/heads/master@{#432859}

Powered by Google App Engine
This is Rietveld 408576698