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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/gdata/gdata_file_system.h"
 
 #include <errno.h>
 
 #include <utility>
 
@@ skipping 1437 matching lines @@
            base::Bind(callback, base::PLATFORM_FILE_ERROR_FAILED, FilePath(),
                reinterpret_cast<GDataFileBase*>(NULL)));
     }
 
     return;
   }
 
   // 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,
         base::Bind(&GDataFileSystem::OnGetDocuments,
                    GetWeakPtrForCurrentThread(),
                    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));
   }
 }
 
 void GDataFileSystem::LoadRootFeedFromCache(
     const FilePath& search_file_path,
     bool load_from_server,
     scoped_refptr<base::MessageLoopProxy> proxy,
     const FindFileCallback& callback) {
   BrowserThread::PostBlockingPoolTask(FROM_HERE,
       base::Bind(&GDataFileSystem::LoadRootFeedOnIOThreadPool,
@@ skipping 469 matching lines @@
     // If the code above fails to parse a feed, any GDataFileBase instance
     // added to |file_by_url| is not managed by a GDataDirectory instance,
     // so we need to explicitly release them here.
     for (UrlToFileAndParentMap::iterator it = file_by_url.begin();
          it != file_by_url.end(); ++it) {
       delete it->second.first;
     }
     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);
+    if (dir)
+      dir->AddFile(file.release());
 
-    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)
     NotifyDirectoryChanged(root_->GetFilePath());
 
   return base::PLATFORM_FILE_OK;
 }
 
 void GDataFileSystem::NotifyCacheInitialized() {
   DVLOG(1) << "Cache initialized";
@@ skipping 1304 matching lines @@
   const bool posted = BrowserThread::PostBlockingPoolSequencedTask(
       sequence_token_name,
       from_here,
       base::Bind(&GDataFileSystem::RunTaskOnIOThreadPool,
                  base::Unretained(this),
                  task));
   DCHECK(posted);
 }
 
 }  // namespace gdata