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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 11414253: DriveFileSyncService listens to OnAuthenticated event to recover from unavailable state (Closed)

Created:
8 years ago by nhiroki
Modified:
8 years ago
Reviewers:
kinuko, tzik
CC:
chromium-reviews, tzik+watch_chromium.org, kinuko+watch, calvinlo
Visibility:
Public.

Description

DriveFileSyncService listens to OnAuthenticated event to recover from unavailable state This patch includes: - Adding DriveFileSyncClientObserver to notify OnAuthenticated event - Invoking SyncFileSystemService::OnRemoteServiceStateUpdate when OnAuthenticated event happens. BUG=161436 TEST=manual (Launch test app -> Logout -> Login -> Receive notification) Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170900

Patch Set 1 #

Patch Set 2 : Fix #

Total comments: 4

Patch Set 3 : Review fix #

Total comments: 2

Patch Set 4 : Review fix #

Patch Set 5 : Rebase #

Total comments: 2

Patch Set 6 : Fix broken tests #

Patch Set 7 : Fix broken tests... #

Total comments: 2

Patch Set 8 : Rebase #

Patch Set 9 : Review fix #

Patch Set 10 : Rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+62 lines, -5 lines) Patch
M chrome/browser/sync_file_system/drive_file_sync_client.h View 1 2 3 4 5 6 7 8 9 5 chunks +21 lines, -1 line 0 comments Download
M chrome/browser/sync_file_system/drive_file_sync_client.cc View 1 2 3 4 5 6 7 8 9 3 chunks +19 lines, -0 lines 0 comments Download
M chrome/browser/sync_file_system/drive_file_sync_client_unittest.cc View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/sync_file_system/drive_file_sync_service.h View 1 2 3 4 5 6 7 8 9 4 chunks +5 lines, -3 lines 0 comments Download
M chrome/browser/sync_file_system/drive_file_sync_service.cc View 1 2 3 4 5 6 7 8 9 4 chunks +13 lines, -1 line 0 comments Download
M chrome/browser/sync_file_system/drive_file_sync_service_unittest.cc View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 17 (0 generated)
nhiroki
Could you review this? I manually checked this works well. I plan to add OnNetworkConnected ...
8 years ago (2012-11-30 05:30:46 UTC) #1
kinuko
https://codereview.chromium.org/11414253/diff/10/chrome/browser/sync_file_system/drive_file_sync_client.h File chrome/browser/sync_file_system/drive_file_sync_client.h (right): https://codereview.chromium.org/11414253/diff/10/chrome/browser/sync_file_system/drive_file_sync_client.h#newcode33 chrome/browser/sync_file_system/drive_file_sync_client.h:33: virtual void OnAuthenticated() {} I prefer leaving this pure ...
8 years ago (2012-11-30 06:26:50 UTC) #2
tzik
Can we implement DriveServiceObserver on DriveFileSyncService, and pass it to DriveFileSyncClient? I think it makes ...
8 years ago (2012-11-30 06:53:25 UTC) #3
nhiroki
On 2012/11/30 06:53:25, tzik wrote: > Can we implement DriveServiceObserver on DriveFileSyncService, and pass it ...
8 years ago (2012-11-30 08:26:46 UTC) #4
nhiroki
Updated. Please take another look, thanks! https://codereview.chromium.org/11414253/diff/10/chrome/browser/sync_file_system/drive_file_sync_client.h File chrome/browser/sync_file_system/drive_file_sync_client.h (right): https://codereview.chromium.org/11414253/diff/10/chrome/browser/sync_file_system/drive_file_sync_client.h#newcode33 chrome/browser/sync_file_system/drive_file_sync_client.h:33: virtual void OnAuthenticated() ...
8 years ago (2012-11-30 08:27:41 UTC) #5
kinuko
https://codereview.chromium.org/11414253/diff/12001/chrome/browser/sync_file_system/drive_file_sync_service.cc File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/11414253/diff/12001/chrome/browser/sync_file_system/drive_file_sync_service.cc#newcode379 chrome/browser/sync_file_system/drive_file_sync_service.cc:379: OnRemoteServiceStateUpdated(REMOTE_SERVICE_OK, "Authenticated")); DriveFileSyncService is also keeping the current state ...
8 years ago (2012-11-30 08:59:35 UTC) #6
nhiroki
Thanks! Updated. https://codereview.chromium.org/11414253/diff/12001/chrome/browser/sync_file_system/drive_file_sync_service.cc File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/11414253/diff/12001/chrome/browser/sync_file_system/drive_file_sync_service.cc#newcode379 chrome/browser/sync_file_system/drive_file_sync_service.cc:379: OnRemoteServiceStateUpdated(REMOTE_SERVICE_OK, "Authenticated")); On 2012/11/30 08:59:35, kinuko wrote: ...
8 years ago (2012-11-30 09:08:58 UTC) #7
kinuko
lgtm https://codereview.chromium.org/11414253/diff/2006/chrome/browser/sync_file_system/drive_file_sync_service.cc File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/11414253/diff/2006/chrome/browser/sync_file_system/drive_file_sync_service.cc#newcode505 chrome/browser/sync_file_system/drive_file_sync_service.cc:505: OnRemoteServiceStateUpdated(REMOTE_SERVICE_OK, "Authenticated")); nit: I'd use state_ instead.
8 years ago (2012-11-30 09:49:35 UTC) #8
nhiroki
Thanks for reviewing! https://codereview.chromium.org/11414253/diff/2006/chrome/browser/sync_file_system/drive_file_sync_service.cc File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/11414253/diff/2006/chrome/browser/sync_file_system/drive_file_sync_service.cc#newcode505 chrome/browser/sync_file_system/drive_file_sync_service.cc:505: OnRemoteServiceStateUpdated(REMOTE_SERVICE_OK, "Authenticated")); On 2012/11/30 09:49:35, kinuko ...
8 years ago (2012-11-30 10:43:36 UTC) #9
kinuko
https://codereview.chromium.org/11414253/diff/1003/chrome/browser/sync_file_system/drive_file_sync_service.cc File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/11414253/diff/1003/chrome/browser/sync_file_system/drive_file_sync_service.cc#newcode502 chrome/browser/sync_file_system/drive_file_sync_service.cc:502: state_ = REMOTE_SERVICE_OK; Hmm we're not defining strict state ...
8 years ago (2012-12-03 06:17:37 UTC) #10
tzik
lgtm
8 years ago (2012-12-03 06:19:51 UTC) #11
nhiroki
Updated https://codereview.chromium.org/11414253/diff/1003/chrome/browser/sync_file_system/drive_file_sync_service.cc File chrome/browser/sync_file_system/drive_file_sync_service.cc (right): https://codereview.chromium.org/11414253/diff/1003/chrome/browser/sync_file_system/drive_file_sync_service.cc#newcode502 chrome/browser/sync_file_system/drive_file_sync_service.cc:502: state_ = REMOTE_SERVICE_OK; On 2012/12/03 06:17:37, kinuko wrote: ...
8 years ago (2012-12-03 06:42:13 UTC) #12
kinuko
lgtm
8 years ago (2012-12-03 06:43:24 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nhiroki@chromium.org/11414253/6019
8 years ago (2012-12-03 06:48:28 UTC) #14
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) browser_tests
8 years ago (2012-12-03 11:52:39 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nhiroki@chromium.org/11414253/6019
8 years ago (2012-12-04 02:52:50 UTC) #16
commit-bot: I haz the power
8 years ago (2012-12-04 06:01:11 UTC) #17

Powered by Google App Engine
This is Rietveld 408576698