chromeos: Stop calling gdata::GDataCache::GetCacheEntry on UI thread

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 ea62ae804e2908ac49edcd7a42b96ca5e7bd9d45..41beccdc40b089889b73686058aa2ef5d0b07f21 100644
--- a/chrome/browser/chromeos/gdata/gdata_file_system.cc
+++ b/chrome/browser/chromeos/gdata/gdata_file_system.cc
@@ -459,6 +459,14 @@ void OnTransferRegularFileCompleteForCopy(
     relay_proxy->PostTask(FROM_HERE, base::Bind(callback, error));
 }
 
+void GetCacheEntryOnBlockingPool(

[satorux1, 2012/06/13 23:03:03]

nit: function comment is missing.

[satorux1, 2012/06/14 00:41:47]

ping.

[hashimoto, 2012/06/14 01:17:53]

Done.

+    GDataCache* cache,
+    const std::string& resource_id,
+    const std::string& md5,
+    scoped_ptr<GDataCache::CacheEntry>* cache_entry) {
+  cache_entry->reset(cache->GetCacheEntry(resource_id, md5).release());
+}
+
 // Runs GetFileCallback with pointers dereferenced.
 // Used for PostTaskAndReply().
 void RunGetFileCallbackHelper(const GetFileCallback& callback,
@@ -3243,24 +3251,19 @@ void GDataFileSystem::OnFileDownloaded(
   // If user cancels download of a pinned-but-not-fetched file, mark file as
   // unpinned so that we do not sync the file again.
   if (status == GDATA_CANCELLED) {
-    bool pinning_cancelled = false;
-    {
-      // To access root_. Limit the scope as SetPinStateOnUIThread() will
-      // acquire the lock.
-      base::AutoLock lock(lock_);
-      // TODO(satorux): Should not call this on UI thread. crbug.com/131826.
-      scoped_ptr<GDataCache::CacheEntry> cache_entry = cache_->GetCacheEntry(
-          params.resource_id,
-          params.md5);
-      if (cache_entry.get() && cache_entry->IsPinned())
-        pinning_cancelled = true;
-    }
-    // TODO(hshi): http://crbug.com/127138 notify when file properties change.
-    // This allows file manager to clear the "Available offline" checkbox.
-    if (pinning_cancelled) {
-      SetPinStateOnUIThread(params.virtual_file_path, false,
-                            FileOperationCallback());
-    }
+    scoped_ptr<GDataCache::CacheEntry>* cache_entry =
+        new scoped_ptr<GDataCache::CacheEntry>();

[achuithb, 2012/06/13 21:37:11]

Why not 
GDataCache::CacheEntry* cache_entry = new GDataCache::CacheEntry();
There's no reason to used a scoped_ptr since you're deleting using base::Owned
and not the scoped_ptr dtor.

[satorux1, 2012/06/13 23:03:03]

I think hashimoto's intention was to clarify ownership of objects using
scoped_ptr<> for function parameters, which is usually a good thing to do.

However, I think it's rather confusing here, as new'ing scoped_ptr<> is unusual.
I guess using a regular pointer is easier to follow:


GDataCache::CacheEntry* cache_entry = NULL;
PostBlockingPoolSequencedTaskAndReply(
   ...
   base::Bind(&GetCacheEntryOnBlockingPool,
              ...
              &cache_entry),
   ...);

Then, GetCacheEntryOnBlockingPool() will take GDataCache::CacheEntry** to do:

  *cache_entry = cache->GetCacheEntry(resource_id, md5).release();

and UnpinIfPinned() will take a GDataCache::CacheEntry*.

[hashimoto, 2012/06/14 00:12:42]

We cannot use raw pointer here since NULL is passed to the reply task
(DoSomethingWithPtr) in the code below.

Foo* ptr = NULL;
PostTaskAndReply(FROM_HERE, base::Bind(&Getter, &ptr),
base::Bind(&DoSomethingWithPtr, base::Owned(ptr))

If we change DoSomethingWithPtr's signature to take Foo**, it can cause leak
since base::Owned(&ptr) does not delete ptr's pointee.

Foo* ptr = NULL;
PostTaskAndReply(FROM_HERE, base::Bind(&Getter, &ptr),
base::Bind(&DoSomethingWithPtr, base::Owned(&ptr))

I think using scoped_ptr is the most reasonable solution even if it's confusing
for readers.

[satorux1, 2012/06/14 00:23:56]

I'm confused. I thought we could delete "cache_entry" with base::Owned()?

GDataCache::CacheEntry* cache_entry = NULL;
PostBlockingPoolSequencedTaskAndReply(
    ...
    base::Bind(&GetCacheEntryOnBlockingPool,
               ...
               &cache_entry),
    base::Bind(&GDataFileSystem::UnpinIfPinned,
               ...
               base::Owned(cache_entry)));

I may be missing, but what's wrong with this?

[hashimoto, 2012/06/14 00:33:35]

In your code, UnpinIfPinned always receives NULL

[satorux1, 2012/06/14 00:37:57]

Oh you are absolutely right... Then, I'm fine with scoped_ptr<>.

You might want to add some comment here about why we are doing it this way.

[achuithb, 2012/06/14 00:45:51]

I think it would be preferable to copy CacheEntry instead. This object was
designed to be copied, and it's quite small.

void GetCacheEntryOnBlockingPool(
GDataCache* cache,
    const std::string& resource_id,
    const std::string& md5,
    GDataCache::CacheEntry* cache_entry) {
  *cache_entry = *(cache->GetCacheEntry(resource_id, md5));
}

[satorux1, 2012/06/14 00:48:11]

Ah, I like that!

[hashimoto, 2012/06/14 00:58:09]

The reason I was not copying the object here was that
GDataCache::GetCacheEntry's return type was scoped_ptr<CacheEntry>.
Then, why are we using scoped_ptr there after all?

Anyway, I'm happy to hear that I am allowed to copy CacheEntry.

[satorux1, 2012/06/14 01:00:09]

That's a very valid point... Maybe just a historical reason. Let's fix it in a
separate patch

[hashimoto, 2012/06/14 01:17:53]

Done, added the default constructor for GDataCache::CacheEntry.

+    PostBlockingPoolSequencedTaskAndReply(
+        FROM_HERE,
+        base::Bind(&GetCacheEntryOnBlockingPool,
+                   cache_,
+                   params.resource_id,
+                   params.md5,
+                   cache_entry),
+        base::Bind(&GDataFileSystem::UnpinIfPinned,
+                   ui_weak_ptr_,
+                   params.virtual_file_path,
+                   base::Owned(cache_entry)));
   }
 
   // At this point, the disk can be full or nearly full for several reasons:
@@ -3286,6 +3289,16 @@ void GDataFileSystem::OnFileDownloaded(
                  base::Owned(has_enough_space)));
 }
 
+void GDataFileSystem::UnpinIfPinned(
+    const FilePath& file_path,
+    scoped_ptr<GDataCache::CacheEntry>* cache_entry) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  // TODO(hshi): http://crbug.com/127138 notify when file properties change.
+  // This allows file manager to clear the "Available offline" checkbox.
+  if ((*cache_entry).get() && (*cache_entry)->IsPinned())
+    SetPinStateOnUIThread(file_path, false, FileOperationCallback());
+}
+
 void GDataFileSystem::OnFileDownloadedAndSpaceChecked(
     const GetFileFromCacheParams& params,
     GDataErrorCode status,