Added logic that will return the result of the first chunk of root feed and continue fetching the r…

chrome/browser/chromeos/gdata/gdata_file_system.cc

Index: chrome/browser/chromeos/gdata/gdata_file_system.cc
diff --git a/chrome/browser/chromeos/gdata/gdata_file_system.cc b/chrome/browser/chromeos/gdata/gdata_file_system.cc
index 75f1ffd1925ffe36d7e4cc9508a652048bb6948d..4d0f673c8a5e94742ff0dd35658c9de9ba0ac3f3 100644
--- a/chrome/browser/chromeos/gdata/gdata_file_system.cc
+++ b/chrome/browser/chromeos/gdata/gdata_file_system.cc
@@ -1455,9 +1455,45 @@ void GDataFileSystem::OnGetDocuments(
   // Add the current feed to the list of collected feeds for this directory.
   feed_list->Append(data.release());
 
-  // Check if we need to collect more data to complete the directory list.
-  if (current_feed->GetNextFeedURL(&next_feed_url) &&
-      !next_feed_url.is_empty()) {
+  bool inital_read = false;

[satorux1, 2012/03/25 00:22:26]

nit: initial

[zel, 2012/03/25 02:06:41]

Done.

+  {
+    base::AutoLock lock(lock_);
+    inital_read = root_->origin() == UNINITIALIZED;
+  }
+
+  bool has_more_data = current_feed->GetNextFeedURL(&next_feed_url) &&
+                       !next_feed_url.is_empty();
+
+  if (!has_more_data || inital_read) {

[satorux1, 2012/03/25 00:22:26]

nit: make the order consistent with the code above:

 if (inital_read || !has_more_data)

Please also add some comment that why we are reporting to the callback when
initial_read is true. I know the intent, but it's not obvious from the code
alone.

[zel, 2012/03/25 02:06:41]

Done.

+    error = UpdateDirectoryWithDocumentFeed(feed_list.get(),
+                                            FROM_SERVER);
+    if (error != base::PLATFORM_FILE_OK) {
+      if (!callback.is_null()) {
+        proxy->PostTask(FROM_HERE,
+             base::Bind(callback, error, FilePath(),
+                 reinterpret_cast<GDataFileBase*>(NULL)));
+      }
+
+      return;
+    }
+
+    // If we had someone to report this too, then this retrieval was done in a
+    // context of search... so continue search.
+    if (!callback.is_null()) {
+      proxy->PostTask(FROM_HERE,
+          base::Bind(&GDataFileSystem::FindFileByPathOnCallingThread,
+                     GetWeakPtrForCurrentThread(),
+                     search_file_path,
+                     callback));
+    }
+  }
+
+  if (has_more_data) {
+    // Don't report to initial callback if we were fetching the first chunk of
+    // uninitialized root feed. Instead, just continue with entire feed fetch

[satorux1, 2012/03/25 00:22:26]

// uninitialized root feed.
->
// uninitialized root feed, because we already reported.

[zel, 2012/03/25 02:06:41]

Done.

+    // in backgorund.
+    const FindFileCallback continue_callback =
+        inital_read ? FindFileCallback() : callback;

[satorux1, 2012/03/25 00:22:26]

So we won't call the callback when the last chunk arrives. I found it a bit
confusing, so please explain what's gonna happen in the end, and why it's ok.

[zel, 2012/03/25 02:06:41]

The callback that returns results of find operation (i.e. directory content
read) will be triggered as soon as we have something to show. This could be when
we read the metadata file from the cache or when the first batch of content feed
arrives over the net. In both cases, if there is more feed data to be fetched to
complete the accurate picture about how FS really looks like, we will continue
that in the background. When the final feed fetching finishes, we will raise
signal directory change notification and client will be able to refresh their
views.

     // Kick of the remaining part of the feeds.
     documents_service_->GetDocuments(
         next_feed_url,
@@ -1466,32 +1502,11 @@ void GDataFileSystem::OnGetDocuments(
                    search_file_path,
                    base::Passed(&feed_list),
                    proxy,
-                   callback));
-    return;
-  }
-
-  error = UpdateDirectoryWithDocumentFeed(feed_list.get(), FROM_SERVER);
-  if (error != base::PLATFORM_FILE_OK) {
-    if (!callback.is_null()) {
-      proxy->PostTask(FROM_HERE,
-           base::Bind(callback, error, FilePath(),
-               reinterpret_cast<GDataFileBase*>(NULL)));
-    }
-
-    return;
-  }
-
-  scoped_ptr<base::Value> feed_list_value(feed_list.release());
-  SaveFeed(feed_list_value.Pass(), FilePath(kLastFeedFile));
-
-  // If we had someone to report this too, then this retrieval was done in a
-  // context of search... so continue search.
-  if (!callback.is_null()) {
-    proxy->PostTask(FROM_HERE,
-                    base::Bind(&GDataFileSystem::FindFileByPathOnCallingThread,
-                               GetWeakPtrForCurrentThread(),
-                               search_file_path,
-                               callback));
+                   continue_callback));
+  } else {
+    // Save completed feed in meta cache.
+    scoped_ptr<base::Value> feed_list_value(feed_list.release());
+    SaveFeed(feed_list_value.Pass(), FilePath(kLastFeedFile));
   }
 }
 
@@ -1981,30 +1996,39 @@ base::PlatformFileError GDataFileSystem::UpdateDirectoryWithDocumentFeed(
     return error;
   }
 
+  scoped_ptr<GDataRootDirectory> orphans(new GDataRootDirectory(NULL));

[satorux1, 2012/03/25 00:22:26]

This part doesn't seem to be related to the directory refresh business. Is this
just a general bug fix? If so, would be nice to file a bug and add BUG= to it,
for the record.

[zel, 2012/03/25 02:06:41]

Done.

[zel, 2012/03/25 03:22:10]

My unit tests convinced me that this on was still needed at the end to do the
proper cleanup. It's wired properly in the loop below.

   for (UrlToFileAndParentMap::iterator it = file_by_url.begin();
        it != file_by_url.end(); ++it) {
     scoped_ptr<GDataFileBase> file(it->second.first);
     GURL parent_url = it->second.second;
     GDataDirectory* dir = root_.get();
     if (!parent_url.is_empty()) {
-      UrlToFileAndParentMap::iterator find_iter = file_by_url.find(parent_url);
+      UrlToFileAndParentMap::const_iterator find_iter =
+          file_by_url.find(parent_url);
       if (find_iter == file_by_url.end()) {
-        LOG(WARNING) << "Found orphaned file '" << file->file_name()
-                     << "' with non-existing parent folder of "
-                     << parent_url.spec();
+        DVLOG(1) << "Found orphaned kitten file '" << file->file_name()

[satorux1, 2012/03/25 00:22:26]

Does "kitten file" mean files shared by someone else, that we were talking
about? Please add some comment.

[zel, 2012/03/25 02:06:41]

These are files that have parents that don't belong to the current user. Shared
is one example, might be others. In Kan's case, these are really kitten images
:)

...removed 'kitten' part.

+                 << "' with non-existing parent folder of "
+                 << parent_url.spec();
+        dir = NULL;
       } else {
-        dir = find_iter->second.first->AsGDataDirectory();
+        dir = find_iter->second.first ?
+              find_iter->second.first->AsGDataDirectory() : NULL;
         if (!dir) {
-          LOG(WARNING) << "Found orphaned file '" << file->file_name()
-                       << "' pointing to non directory parent "
-                       << parent_url.spec();
-          dir = root_.get();
+          DVLOG(1) << "Found orphaned file '" << file->file_name()
+                   << "' pointing to non directory parent "
+                   << parent_url.spec();
+          dir = NULL;
         }
       }
     }
-    DCHECK(dir);
-
-    dir->AddFile(file.release());
+    if (dir)
+      dir->AddFile(file.release());
+
+    // |file| instance is gone now, deleted by either this scoped_ptr (orphans)

[satorux1, 2012/03/25 00:22:26]

I'm confused. |orphans| doesn't seem to be used to collect orphans.

[zel, 2012/03/25 02:06:41]

orphans was supposed to be removed. done.

+    // or taken over by |dir|. So, let's mark it as deleted in the map so we
+    // don't end up with other children file items of this instance trying to
+    // add themselves as chikdren.

[satorux1, 2012/03/25 00:22:26]

children

[zel, 2012/03/25 02:06:41]

Done.

+    it->second.first = NULL;
   }
 
   if (should_nofify)