Implement polling part of DriveFileSyncService

Implement polling part of DriveFileSyncService


BUG=156041
TEST=manual

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=170904

[tzik, 2012-11-29 09:25:59]

Hi, Kinuko-san and Hiroki-san.
Could you take a early look to the CL?

I'll add some test and put parser part out to another patch.

[kinuko, 2012-11-29 11:40:46]

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_wapi_parser.h (right):

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_parser.h:389: static bool
OptionalChangestamp(const base::StringPiece& value, int64* result);
nit: over 80 chars

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_parser.h:440: int64 changestamp() const {
return changestamp_; }
Yes, I wanted to have this!

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:39:
"https://docs.google.com/feeds/default/private/full/";
Can we use GDataWapiUrlGenerator::GenerateDocumentListRootUrl()?

(And I think the URL should be given by DriveFileSyncClient and use the same
base URL?)

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:68: 
nit: extra empty line

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:585:
FOR_EACH_OBSERVER(Observer, observers_,
Can you comment why we do this here?

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1311: void
DriveFileSyncService::CancelRemoteChange(
Well I think we definitely need to change this method name.

Maybe... at least Cancel -> Remove?

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1334:
base::Bind(&EmptyStatusCallback));
Let's change this part per discussion!

[kinuko, 2012-11-29 11:47:43]

Polling part looks good.

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:215:
largest_synced_changestamp_(0),
We're not using this?

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1401: if
(changes->start_index() == 0 && changes->entries().empty()) {
nit: can you add a comment: 'If this set of changes is the first feed and its
empty, update the polling delay to wait longer.'

[nhiroki, 2012-11-30 06:23:53]

Looks good to me. It's only a trivial comment.

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1294: if
(entry.kind() == google_apis::ENTRY_KIND_FOLDER)
You may want to use "entry.is_folder()".

[tzik, 2012-12-03 10:00:08]

Thanks! Updated.

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/google_apis...
File chrome/browser/google_apis/gdata_wapi_parser.h (right):

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/google_apis...
chrome/browser/google_apis/gdata_wapi_parser.h:389: static bool
OptionalChangestamp(const base::StringPiece& value, int64* result);
On 2012/11/29 11:40:46, kinuko wrote:
> nit: over 80 chars

Done.

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:39:
"https://docs.google.com/feeds/default/private/full/";
On 2012/11/29 11:40:46, kinuko wrote:
> Can we use GDataWapiUrlGenerator::GenerateDocumentListRootUrl()?
> 
> (And I think the URL should be given by DriveFileSyncClient and use the same
> base URL?)

Done

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:68: 
On 2012/11/29 11:40:46, kinuko wrote:
> nit: extra empty line

Done.

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:215:
largest_synced_changestamp_(0),
On 2012/11/29 11:47:43, kinuko wrote:
> We're not using this?

Done. Removed.

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:585:
FOR_EACH_OBSERVER(Observer, observers_,
On 2012/11/29 11:40:46, kinuko wrote:
> Can you comment why we do this here?

Done.

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1294: if
(entry.kind() == google_apis::ENTRY_KIND_FOLDER)
On 2012/11/30 06:23:53, nhiroki wrote:
> You may want to use "entry.is_folder()".

Done.

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1311: void
DriveFileSyncService::CancelRemoteChange(
On 2012/11/29 11:40:46, kinuko wrote:
> Well I think we definitely need to change this method name.
> 
> Maybe... at least Cancel -> Remove?

Done.

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1334:
base::Bind(&EmptyStatusCallback));
On 2012/11/29 11:40:46, kinuko wrote:
> Let's change this part per discussion!

Done.

https://codereview.chromium.org/11421125/diff/3006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1401: if
(changes->start_index() == 0 && changes->entries().empty()) {
On 2012/11/29 11:47:43, kinuko wrote:
> nit: can you add a comment: 'If this set of changes is the first feed and its
> empty, update the polling delay to wait longer.'

Done.

[kinuko, 2012-12-03 13:10:34]

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_file_sync_client.cc (right):

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_client.cc:90:
GURL(google_apis::GDataWapiUrlGenerator::kBaseUrlForProduction)) {
nit: Should this be given from outside? It feels a bit weird to use
kBaseUrlForProduction for testing case (and ideally it should use the same base
url as the given drive_service)

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_client.cc:381:
replacements.SetQueryStr(std::string());
oh ok... it's cumbersome :(

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_client.cc:397: DVLOG(1) <<
*data;
nit: can you make this logging a bit more descriptive?

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:41: const int64
kDelayMultiplier = 2;
nit: if it's int we'll have 1, 2 or etc...? maybe it might make more sense if we
make it double?

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:585:
SchedulePolling();
I have a feeling that it might be ok to occasionally run this not only if we're
in REMOTE_SERVICE_OK but also in REMOTE_SERVICE_TEMPORARY_UNAVAILABLE?

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:585:
SchedulePolling();
in OnAuthenticated or OnNetworkConnected should we also call this (maybe after
resetting the polling_delay_ to min)?

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1330:
metadata_store_->MoveBatchSyncOriginToIncremental(url.origin());
Shouldn't we call SchedulePolling() but not kick the timer in SchedulePolling()
if incremental_sync_origins_.empty()?

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1361: }
Don't we call FetchChangesForIncrementalSync() repeatedly with minimum delay
while we have no incremental_sync_origins in this way?

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1378: GURL next_feed;
nit: can you move this right before line 1392?

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1396: AsWeakPtr(),
base::Passed(&token)));
nit: can we return here and remove else on line 1397?

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1401: // If this set
of changes is the first feed and its empty, update
nit: its empty -> it's empty

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1446: if
(!polling_timer_.IsRunning()) {
nit: maybe we can early return?

if (polling_timer_.IsRunning())
  return;

...

[tzik, 2012-12-04 04:59:50]

Updated. PTAL.

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_file_sync_client.cc (right):

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_client.cc:90:
GURL(google_apis::GDataWapiUrlGenerator::kBaseUrlForProduction)) {
On 2012/12/03 13:10:34, kinuko wrote:
> nit: Should this be given from outside? It feels a bit weird to use
> kBaseUrlForProduction for testing case (and ideally it should use the same
base
> url as the given drive_service)

Done.

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_client.cc:381:
replacements.SetQueryStr(std::string());
On 2012/12/03 13:10:34, kinuko wrote:
> oh ok... it's cumbersome :(

Ah, it seems not needed since we no longer use exact match.

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_client.cc:397: DVLOG(1) <<
*data;
On 2012/12/03 13:10:34, kinuko wrote:
> nit: can you make this logging a bit more descriptive?

Done. Removed.

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:41: const int64
kDelayMultiplier = 2;
On 2012/12/03 13:10:34, kinuko wrote:
> nit: if it's int we'll have 1, 2 or etc...? maybe it might make more sense if
we
> make it double?

Done.

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:585:
SchedulePolling();
On 2012/12/03 13:10:34, kinuko wrote:
> I have a feeling that it might be ok to occasionally run this not only if
we're
> in REMOTE_SERVICE_OK but also in REMOTE_SERVICE_TEMPORARY_UNAVAILABLE?

Done.

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:585:
SchedulePolling();
On 2012/12/03 13:10:34, kinuko wrote:
> in OnAuthenticated or OnNetworkConnected should we also call this (maybe after
> resetting the polling_delay_ to min)?

those patches are not landed yet.
let me do it later.

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1361: }
On 2012/12/03 13:10:34, kinuko wrote:
> Don't we call FetchChangesForIncrementalSync() repeatedly with minimum delay
> while we have no incremental_sync_origins in this way?

Done.

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1378: GURL next_feed;
On 2012/12/03 13:10:34, kinuko wrote:
> nit: can you move this right before line 1392?

Done.

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1396: AsWeakPtr(),
base::Passed(&token)));
On 2012/12/03 13:10:34, kinuko wrote:
> nit: can we return here and remove else on line 1397?

Done.

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1401: // If this set
of changes is the first feed and its empty, update
On 2012/12/03 13:10:34, kinuko wrote:
> nit: its empty -> it's empty

Done.

https://codereview.chromium.org/11421125/diff/2004/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1446: if
(!polling_timer_.IsRunning()) {
On 2012/12/03 13:10:34, kinuko wrote:
> nit: maybe we can early return?
> 
> if (polling_timer_.IsRunning())
>   return;
> 
> ...

Done.

[kinuko, 2012-12-04 05:18:30]

lgtm

Please update the issue description and BUG=, TEST= lines (TEST=manual is ok).

https://codereview.chromium.org/11421125/diff/2005/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):

https://codereview.chromium.org/11421125/diff/2005/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:568: if (state_ ==
REMOTE_SERVICE_TEMPORARY_UNAVAILABLE) {
Should we also check the same conditions as line 590-592?

Maybe we can just move the check into SchedulePolling?

https://codereview.chromium.org/11421125/diff/2005/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1382:
NotifyTaskDone(fileapi::SYNC_STATUS_FAILED, token.Pass());
GDataErrorCodeToSyncStatusCodeWrapper(error) ?

https://codereview.chromium.org/11421125/diff/2005/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1421:
kMinimumPollingDelaySeconds);
nit: this code is totally ok but just keeping polling_delay_ as secondes integer
might have been simpler

[nhiroki, 2012-12-04 05:55:31]

LGTM

https://codereview.chromium.org/11421125/diff/1040/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):

https://codereview.chromium.org/11421125/diff/1040/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:597: // Notify
observer of the right timing to run ProcessRemoteChange.
"Notify observers in the right timing to run ProcessRemoteChange." ?

I think "the right timing" is somewhat vague.

[tzik, 2012-12-04 06:32:32]

https://codereview.chromium.org/11421125/diff/2005/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):

https://codereview.chromium.org/11421125/diff/2005/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:568: if (state_ ==
REMOTE_SERVICE_TEMPORARY_UNAVAILABLE) {
On 2012/12/04 05:18:30, kinuko wrote:
> Should we also check the same conditions as line 590-592?
> 
> Maybe we can just move the check into SchedulePolling?

Done.

https://codereview.chromium.org/11421125/diff/2005/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1382:
NotifyTaskDone(fileapi::SYNC_STATUS_FAILED, token.Pass());
On 2012/12/04 05:18:30, kinuko wrote:
> GDataErrorCodeToSyncStatusCodeWrapper(error) ?

Done.

https://codereview.chromium.org/11421125/diff/2005/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:1421:
kMinimumPollingDelaySeconds);
On 2012/12/04 05:18:30, kinuko wrote:
> nit: this code is totally ok but just keeping polling_delay_ as secondes
integer
> might have been simpler

Done.

https://codereview.chromium.org/11421125/diff/1040/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):

https://codereview.chromium.org/11421125/diff/1040/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:597: // Notify
observer of the right timing to run ProcessRemoteChange.
On 2012/12/04 05:55:31, nhiroki wrote:
> "Notify observers in the right timing to run ProcessRemoteChange." ?
> 
> I think "the right timing" is somewhat vague.

Done.

https://codereview.chromium.org/11421125/diff/1040/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:597: // Notify
observer of the right timing to run ProcessRemoteChange.
On 2012/12/04 05:55:31, nhiroki wrote:
> "Notify observers in the right timing to run ProcessRemoteChange." ?
> 
> I think "the right timing" is somewhat vague.

Done.

[kinuko, 2012-12-04 06:59:17]

ok, still lgtm

https://codereview.chromium.org/11421125/diff/6006/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):

https://codereview.chromium.org/11421125/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:105: void
set_callback(const base::Closure& callback) {
nit: set_completion_callback(), completion_callback() to make them a bit
clearer?

https://codereview.chromium.org/11421125/diff/6006/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/sync_file_system_service.cc (right):

https://codereview.chromium.org/11421125/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/sync_file_system_service.cc:457: DVLOG(1) <<
"OnRemoteChangeAvailable: " << pending_changes;
nit: can you also update this log msg?

[nhiroki, 2012-12-04 07:06:01]

Still LGTM too!

https://codereview.chromium.org/11421125/diff/6006/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_file_sync_client.h (right):

https://codereview.chromium.org/11421125/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_client.h:158: // Convers
|resource_id| to corresponing resource link.
nit: s/Convers/Converts/

[tzik, 2012-12-04 07:06:09]

https://codereview.chromium.org/11421125/diff/6006/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_file_sync_service.cc (right):

https://codereview.chromium.org/11421125/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_service.cc:105: void
set_callback(const base::Closure& callback) {
On 2012/12/04 06:59:17, kinuko wrote:
> nit: set_completion_callback(), completion_callback() to make them a bit
> clearer?

Done.

https://codereview.chromium.org/11421125/diff/6006/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/sync_file_system_service.cc (right):

https://codereview.chromium.org/11421125/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/sync_file_system_service.cc:457: DVLOG(1) <<
"OnRemoteChangeAvailable: " << pending_changes;
On 2012/12/04 06:59:17, kinuko wrote:
> nit: can you also update this log msg?

Done.

[tzik, 2012-12-04 07:07:46]

https://codereview.chromium.org/11421125/diff/6006/chrome/browser/sync_file_s...
File chrome/browser/sync_file_system/drive_file_sync_client.h (right):

https://codereview.chromium.org/11421125/diff/6006/chrome/browser/sync_file_s...
chrome/browser/sync_file_system/drive_file_sync_client.h:158: // Convers
|resource_id| to corresponing resource link.
On 2012/12/04 07:06:01, nhiroki wrote:
> nit: s/Convers/Converts/

Done.

[commit-bot: I haz the power, 2012-12-04 07:08:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tzik@chromium.org/11421125/5011