drive: Pass through AboutResouce from server among callbacks in ChangeListLoader.

drive: Pass through AboutResouce from server amond callbacks in ChangeListLoader.

This help us refactoring the pseudo tree structure with separate namespaces.
We would like to create a dummy "My Drive" root in ChangeListProcessor and use
it as "drive/root" instead of having it in DriveResourceMetadata.
This operation needs the root folder ID.
It also helps our migration to Drive API.

BUG=174233
TEST=unittests. Open Files App and verify Google Drive is available.


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

[Haruki Sato, 2013-03-15 06:42:19]

Sorry for the long delay...

Kinaba-san, could you take a look?
This change does 

Abe-san, This CL stores the root folder ID from the server in ChangeListLoader.
I guess this is related to Drive API migration as we chatted.
I am planning to use it for "My Drive" root in the coming CL.
Does it fit your purpose? Please let me know if you have any concern.

Thanks!

[Haruki Sato, 2013-03-15 06:43:19]

On 2013/03/15 06:42:19, Haruki Sato wrote:
> Sorry for the long delay...
> 
> Kinaba-san, could you take a look?
> This change does 
Ooops.
This change only passes the root folder ID and no actual effect for now.

> 
> Abe-san, This CL stores the root folder ID from the server in
ChangeListLoader.
> I guess this is related to Drive API migration as we chatted.
> I am planning to use it for "My Drive" root in the coming CL.
> Does it fit your purpose? Please let me know if you have any concern.
> 
> Thanks!

[hidehiko, 2013-03-15 06:57:44]

> Abe-san, This CL stores the root folder ID from the server in
ChangeListLoader.
> I guess this is related to Drive API migration as we chatted.
> I am planning to use it for "My Drive" root in the coming CL.
> Does it fit your purpose? Please let me know if you have any concern.
> 

Assuming the "My Drive" root will be created in ApplyFeeds() method correctly,
yes, it should work well for Drive API v2 migration, too, I believe. Thank you
for your work.

https://codereview.chromium.org/12588009/diff/5001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/change_list_loader.cc (right):

https://codereview.chromium.org/12588009/diff/5001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/change_list_loader.cc:302: drive_root_resource_id_
= about_resource->root_folder_id();
How about passing through AboutResource to L761 via arguments?

https://codereview.chromium.org/12588009/diff/5001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/change_list_processor.cc (right):

https://codereview.chromium.org/12588009/diff/5001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/change_list_processor.cc:64: const std::string
drive_root_resource_id)
Let's make this an argument of ApplyFeeds().
I think, the method can take AboutResource directly?

[Haruki Sato, 2013-03-18 04:20:25]

Per offline discussion with hidehiko, refactored the ChangeListLoader more to
simplify the arguments.

Could you take another look?

Thanks!

https://codereview.chromium.org/12588009/diff/5001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/change_list_loader.cc (right):

https://codereview.chromium.org/12588009/diff/5001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/change_list_loader.cc:302: drive_root_resource_id_
= about_resource->root_folder_id();
On 2013/03/15 06:57:44, hidehiko wrote:
> How about passing through AboutResource to L761 via arguments?

Done.

https://codereview.chromium.org/12588009/diff/5001/chrome/browser/chromeos/dr...
File chrome/browser/chromeos/drive/change_list_processor.cc (right):

https://codereview.chromium.org/12588009/diff/5001/chrome/browser/chromeos/dr...
chrome/browser/chromeos/drive/change_list_processor.cc:64: const std::string
drive_root_resource_id)
On 2013/03/15 06:57:44, hidehiko wrote:
> Let's make this an argument of ApplyFeeds().
> I think, the method can take AboutResource directly?

Done. Thank you for the advice!

[hidehiko, 2013-03-18 06:22:27]

https://codereview.chromium.org/12588009/diff/20004/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/change_list_loader.cc (right):

https://codereview.chromium.org/12588009/diff/20004/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/change_list_loader.cc:269:
DCHECK(about_resource.get());
Do you want to:
DCHECK_EQ(util::GDataToDriveFileError(status) == DRIVE_FILE_OK,
          about_resource.get() != NULL);
?

https://codereview.chromium.org/12588009/diff/20004/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/change_list_loader.cc:280:
base::Passed(about_resource.Pass())));
base::Passed(&about_resource) ? Ditto for below.

https://codereview.chromium.org/12588009/diff/20004/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/change_list_loader.h (right):

https://codereview.chromium.org/12588009/diff/20004/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/change_list_loader.h:278: // Resource ID for the
user's "My Drive" root. This must be retrieved from
Don't need?

https://codereview.chromium.org/12588009/diff/20004/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_test_util.cc (right):

https://codereview.chromium.org/12588009/diff/20004/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_test_util.cc:224:
about_resource->set_root_folder_id("fake_root");
Let's avoid hard-coded root id of the fake server.
Passing resource_id or AboutResource instance should be better.

[Haruki Sato, 2013-03-18 07:06:41]

https://codereview.chromium.org/12588009/diff/20004/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/change_list_loader.cc (right):

https://codereview.chromium.org/12588009/diff/20004/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/change_list_loader.cc:269:
DCHECK(about_resource.get());
On 2013/03/18 06:22:27, hidehiko wrote:
> Do you want to:
> DCHECK_EQ(util::GDataToDriveFileError(status) == DRIVE_FILE_OK,
>           about_resource.get() != NULL);
> ?

Done. Thanks!
This actually is rather a debugging code...

https://codereview.chromium.org/12588009/diff/20004/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/change_list_loader.cc:280:
base::Passed(about_resource.Pass())));
On 2013/03/18 06:22:27, hidehiko wrote:
> base::Passed(&about_resource) ? Ditto for below.

Done. Thanks.

https://codereview.chromium.org/12588009/diff/20004/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/change_list_loader.h (right):

https://codereview.chromium.org/12588009/diff/20004/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/change_list_loader.h:278: // Resource ID for the
user's "My Drive" root. This must be retrieved from
On 2013/03/18 06:22:27, hidehiko wrote:
> Don't need?

Done. Thanks.

https://codereview.chromium.org/12588009/diff/20004/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_test_util.cc (right):

https://codereview.chromium.org/12588009/diff/20004/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_test_util.cc:224:
about_resource->set_root_folder_id("fake_root");
On 2013/03/18 06:22:27, hidehiko wrote:
> Let's avoid hard-coded root id of the fake server.
> Passing resource_id or AboutResource instance should be better.

Done. Thanks. This is what you meant, right?

[hidehiko, 2013-03-18 07:09:50]

lgtm with nitpicks.

https://codereview.chromium.org/12588009/diff/22002/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_test_util.cc (right):

https://codereview.chromium.org/12588009/diff/22002/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_test_util.cc:205: std::string
root_resource_id,
const std::string& ?

https://codereview.chromium.org/12588009/diff/22002/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_test_util.h (right):

https://codereview.chromium.org/12588009/diff/22002/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_test_util.h:170: std::string
toor_resource_id,
s/toor/root/ ?

[Haruki Sato, 2013-03-18 07:31:17]

Kinaba-san, could you OWNER review?

Thanks.

https://codereview.chromium.org/12588009/diff/22002/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_test_util.cc (right):

https://codereview.chromium.org/12588009/diff/22002/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_test_util.cc:205: std::string
root_resource_id,
On 2013/03/18 07:09:50, hidehiko wrote:
> const std::string& ?

Done. Thanks!

https://codereview.chromium.org/12588009/diff/22002/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_test_util.h (right):

https://codereview.chromium.org/12588009/diff/22002/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_test_util.h:170: std::string
toor_resource_id,
On 2013/03/18 07:09:50, hidehiko wrote:
> s/toor/root/ ?

Done. qwerty magic!

[kinaba, 2013-03-18 09:46:31]

lgtm

[commit-bot: I haz the power, 2013-03-18 09:57:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/12588009/31003

[commit-bot: I haz the power, 2013-03-20 13:59:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/12588009/31003

[commit-bot: I haz the power, 2013-03-20 13:59:07]

Failed to apply patch for chrome/browser/chromeos/drive/change_list_loader.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/chromeos/drive/change_list_loader.cc
  Hunk #1 FAILED at 264.
  Hunk #2 FAILED at 302.
  Hunk #3 succeeded at 262 (offset -57 lines).
  Hunk #4 succeeded at 445 (offset -26 lines).
  Hunk #5 succeeded at 459 with fuzz 1 (offset -26 lines).
  Hunk #6 succeeded at 694 (offset -39 lines).
  Hunk #7 FAILED at 747.
  3 out of 7 hunks FAILED -- saving rejects to file
chrome/browser/chromeos/drive/change_list_loader.cc.rej

Patch:       chrome/browser/chromeos/drive/change_list_loader.cc
Index: chrome/browser/chromeos/drive/change_list_loader.cc
diff --git a/chrome/browser/chromeos/drive/change_list_loader.cc
b/chrome/browser/chromeos/drive/change_list_loader.cc
index
93b24c01bb96c486374174dcf7481c2313c4642d..cb0354604775cca02665274131886bb9e7d3857c
100644
--- a/chrome/browser/chromeos/drive/change_list_loader.cc
+++ b/chrome/browser/chromeos/drive/change_list_loader.cc
@@ -264,30 +264,28 @@ void
ChangeListLoader::LoadFromServerIfNeededAfterGetAbout(
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
   DCHECK(refreshing_);
-
-  int64 remote_changestamp = 0;
-  if (util::GDataToDriveFileError(status) == DRIVE_FILE_OK) {
-    DCHECK(about_resource);
-    remote_changestamp = about_resource->largest_change_id();
-  }
+  DCHECK_EQ(util::GDataToDriveFileError(status) == DRIVE_FILE_OK,
+            about_resource.get() != NULL);
 
   resource_metadata_->GetLargestChangestamp(
       base::Bind(&ChangeListLoader::CompareChangestampsAndLoadIfNeeded,
                  weak_ptr_factory_.GetWeakPtr(),
                  directory_fetch_info,
                  callback,
-                 remote_changestamp));
+                 base::Passed(&about_resource)));
 }
 
 void ChangeListLoader::CompareChangestampsAndLoadIfNeeded(
     const DirectoryFetchInfo& directory_fetch_info,
     const FileOperationCallback& callback,
-    int64 remote_changestamp,
+    scoped_ptr<google_apis::AboutResource> about_resource,
     int64 local_changestamp) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!callback.is_null());
   DCHECK(refreshing_);
 
+  int64 remote_changestamp =
+      about_resource ? about_resource->largest_change_id() : 0;
   if (remote_changestamp > 0 && local_changestamp >= remote_changestamp) {
     if (local_changestamp > remote_changestamp) {
       LOG(WARNING) << "Cached client feed is fresher than server, client = "
@@ -302,16 +300,22 @@ void ChangeListLoader::CompareChangestampsAndLoadIfNeeded(
   }
 
   int64 start_changestamp = local_changestamp > 0 ? local_changestamp + 1 : 0;
+  if (start_changestamp == 0 && !about_resource.get()) {
+    // Full update needs AboutResource. If this is a full update, we should
just
+    // give up.
+    callback.Run(DRIVE_FILE_ERROR_FAILED);
+    return;
+  }
 
   // TODO(satorux): Use directory_fetch_info to start "fast-fetch" here.
-  LoadChangeListFromServer(start_changestamp,
-                           remote_changestamp,
+  LoadChangeListFromServer(about_resource.Pass(),
+                           start_changestamp,
                            callback);
 }
 
 void ChangeListLoader::LoadChangeListFromServer(
+    scoped_ptr<google_apis::AboutResource> about_resource,
     int64 start_changestamp,
-    int64 remote_changestamp,
     const FileOperationCallback& callback) {
   scoped_ptr<LoadFeedParams> load_params(new LoadFeedParams);
   load_params->start_changestamp = start_changestamp;
@@ -319,8 +323,8 @@ void ChangeListLoader::LoadChangeListFromServer(
       load_params.Pass(),
       base::Bind(&ChangeListLoader::UpdateMetadataFromFeedAfterLoadFromServer,
                  weak_ptr_factory_.GetWeakPtr(),
+                 base::Passed(&about_resource),
                  start_changestamp != 0,  // is_delta_feed
-                 remote_changestamp,
                  callback));
 }
 
@@ -471,8 +475,8 @@ void ChangeListLoader::SearchFromServer(
 }
 
 void ChangeListLoader::UpdateMetadataFromFeedAfterLoadFromServer(
+    scoped_ptr<google_apis::AboutResource> about_resource,
     bool is_delta_feed,
-    int64 feed_changestamp,
     const FileOperationCallback& callback,
     const ScopedVector<google_apis::ResourceList>& feed_list,
     DriveFileError error) {
@@ -485,9 +489,9 @@ void
ChangeListLoader::UpdateMetadataFromFeedAfterLoadFromServer(
     return;
   }
 
-  UpdateFromFeed(feed_list,
+  UpdateFromFeed(about_resource.Pass(),
+                 feed_list,
                  is_delta_feed,
-                 feed_changestamp,
                  base::Bind(&ChangeListLoader::OnUpdateFromFeed,
                             weak_ptr_factory_.GetWeakPtr(),
                             callback));
@@ -733,9 +737,9 @@ void ChangeListLoader::SaveFileSystem() {
 }
 
 void ChangeListLoader::UpdateFromFeed(
+    scoped_ptr<google_apis::AboutResource> about_resource,
     const ScopedVector<google_apis::ResourceList>& feed_list,
     bool is_delta_feed,
-    int64 root_feed_changestamp,
     const base::Closure& update_finished_callback) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK(!update_finished_callback.is_null());
@@ -747,9 +751,9 @@ void ChangeListLoader::UpdateFromFeed(
   const bool should_notify_changed_directories = is_delta_feed;
 
   change_list_processor_->ApplyFeeds(
+      about_resource.Pass(),
       feed_list,
       is_delta_feed,
-      root_feed_changestamp,
       base::Bind(&ChangeListLoader::NotifyDirectoryChanged,
                  weak_ptr_factory_.GetWeakPtr(),
                  should_notify_changed_directories,

[commit-bot: I haz the power, 2013-03-20 14:34:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haruki@chromium.org/12588009/52001

[commit-bot: I haz the power, 2013-03-20 17:11:59]

Change committed as 189323