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

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 <sys/stat.h>
 
 #include <set>
@@ skipping 441 matching lines @@
 //
 // |callback| is run on the thread represented by |relay_proxy|.
 void OnTransferRegularFileCompleteForCopy(
     const FileOperationCallback& callback,
     scoped_refptr<base::MessageLoopProxy> relay_proxy,
     base::PlatformFileError error) {
   if (!callback.is_null())
     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,
                               base::PlatformFileError* error,
                               FilePath* file_path,
                               std::string* mime_type,
                               GDataFileType* file_type) {
   DCHECK(error);
   DCHECK(file_path);
   DCHECK(mime_type);
@@ skipping 2764 matching lines @@
 void GDataFileSystem::OnFileDownloaded(
     const GetFileFromCacheParams& params,
     GDataErrorCode status,
     const GURL& content_url,
     const FilePath& downloaded_file_path) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   // 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:
   // - The expected file size was incorrect and the file was larger
   // - There was an in-flight download operation and it used up space
   // - The disk became full for some user actions we cannot control
   //   (ex. the user might have downloaded a large file from a regular web site)
   //
   // If we don't have enough space, we return PLATFORM_FILE_ERROR_NO_SPACE,
   // and try to free up space, even if the file was downloaded successfully.
   bool* has_enough_space = new bool(false);
   PostBlockingPoolSequencedTaskAndReply(
       FROM_HERE,
       base::Bind(&GDataFileSystem::FreeDiskSpaceIfNeeded,
                  base::Unretained(this),
                  has_enough_space),
       base::Bind(&GDataFileSystem::OnFileDownloadedAndSpaceChecked,
                  ui_weak_ptr_,
                  params,
                  status,
                  content_url,
                  downloaded_file_path,
                  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,
     const GURL& content_url,
     const FilePath& downloaded_file_path,
     bool* has_enough_space) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   base::PlatformFileError error = GDataToPlatformError(status);
 
@@ skipping 1905 matching lines @@
     base::PlatformFileError error,
     const std::string& resource_id,
     const std::string& md5) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (!callback.is_null())
     callback.Run(error);
 }
 
 }  // namespace gdata