test_shell_webblobregistry_impl.cc posts CString to other threads without

webkit/tools/test_shell/test_shell_webblobregistry_impl.cc

 // Copyright (c) 2010 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 "webkit/tools/test_shell/test_shell_webblobregistry_impl.h"
 
 #include "base/message_loop.h"
+#include "base/task.h"
 #include "googleurl/src/gurl.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebBlobData.h"
+#include "third_party/WebKit/Source/WebKit/chromium/public/WebCString.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebString.h"
 #include "third_party/WebKit/Source/WebKit/chromium/public/WebURL.h"
 #include "webkit/blob/blob_data.h"
 #include "webkit/blob/blob_storage_controller.h"
 
 using WebKit::WebBlobData;
 using WebKit::WebString;
 using WebKit::WebURL;
 
 namespace {
 
 MessageLoop* g_io_thread;
 webkit_blob::BlobStorageController* g_blob_storage_controller;
 
+// Copy WebURL object to safely pass it to a different thread.

[jianli, 2011/04/19 23:58:00]

Maybe better to add more comment like the following:
// Note: NewRunnableMethod does copy WebURL object, but not its underlying
WebCString object since it is ref-counted. To pass safely to another thread, we
need to make a copy of WebCString data.

[Dmitry Lomov, 2011/04/20 00:27:01]

Done.

+static WebURL GetWebURLCopy(const WebURL& source) {

[jianli, 2011/04/19 23:58:00]

I think you can remove "static".

[Dmitry Lomov, 2011/04/20 00:27:01]

Done.

+  // Creates a copy of WebCString with refcount 1.

[jianli, 2011/04/19 23:58:00]

I think this comment can be removed with the additional comment above.

[Dmitry Lomov, 2011/04/20 00:27:01]

Done.

+  const WebKit::WebCString spec(source.spec());
+  const url_parse::Parsed& parsed(source.parsed());
+  const bool is_valid = source.isValid();
+  return WebURL(spec, parsed, is_valid);
+}
+
 }  // namespace
 
 /* static */
 void TestShellWebBlobRegistryImpl::InitializeOnIOThread(
     webkit_blob::BlobStorageController* blob_storage_controller) {
   g_io_thread = MessageLoop::current();
   g_blob_storage_controller = blob_storage_controller;
 }
 
 /* static */
 void TestShellWebBlobRegistryImpl::Cleanup() {
   g_io_thread = NULL;
   g_blob_storage_controller = NULL;
 }
 
 TestShellWebBlobRegistryImpl::TestShellWebBlobRegistryImpl() {
 }
 
 void TestShellWebBlobRegistryImpl::registerBlobURL(
     const WebURL& url, WebBlobData& data) {
   DCHECK(g_io_thread);
-  // Note: BlobData is not refcounted thread safe.
-  scoped_refptr<webkit_blob::BlobData> blob_data(
+  CancelableTask* task;
+  {
+    scoped_refptr<webkit_blob::BlobData> blob_data(
       new webkit_blob::BlobData(data));
-  g_io_thread->PostTask(
-      FROM_HERE,
+    WebURL url_copy = ::GetWebURLCopy(url);
+    task =
       NewRunnableMethod(
-          this, &TestShellWebBlobRegistryImpl::DoRegisterBlobUrl, url,
-          blob_data));
+          this, &TestShellWebBlobRegistryImpl::DoRegisterBlobUrl, url_copy,
+          blob_data);
+  }
+  g_io_thread->PostTask(FROM_HERE, task);
 }
 
 void TestShellWebBlobRegistryImpl::registerBlobURL(
     const WebURL& url, const WebURL& src_url) {
   DCHECK(g_io_thread);
-  g_io_thread->PostTask(
-      FROM_HERE,
+  CancelableTask* task;
+  {
+    WebURL url_copy = ::GetWebURLCopy(url);
+    WebURL src_url_copy = ::GetWebURLCopy(src_url);
+    task =
       NewRunnableMethod(this,
                         &TestShellWebBlobRegistryImpl::DoRegisterBlobUrlFrom,
-                        url,
-                        src_url));
+                        url_copy,

[jianli, 2011/04/19 23:58:00]

You can put GetWebURLCopy(url) here directly. Doing so will also remove the need
of {}.

Also no need to add "::".

[Dmitry Lomov, 2011/04/20 00:27:01]

Done.

+                        src_url_copy);
+  }
+  g_io_thread->PostTask(FROM_HERE, task);
 }
 
 void TestShellWebBlobRegistryImpl::unregisterBlobURL(const WebURL& url) {
   DCHECK(g_io_thread);
-  g_io_thread->PostTask(
-      FROM_HERE,
+  CancelableTask* task;
+  {
+    WebURL url_copy = ::GetWebURLCopy(url);
+    task =
       NewRunnableMethod(this,
                         &TestShellWebBlobRegistryImpl::DoUnregisterBlobUrl,
-                        url));
+                        url_copy);
+  }
+  g_io_thread->PostTask(FROM_HERE, task);
 }
 
 void TestShellWebBlobRegistryImpl::DoRegisterBlobUrl(
     const GURL& url, webkit_blob::BlobData* blob_data) {
   DCHECK(g_blob_storage_controller);
   g_blob_storage_controller->RegisterBlobUrl(url, blob_data);
 }
 
 void TestShellWebBlobRegistryImpl::DoRegisterBlobUrlFrom(
     const GURL& url, const GURL& src_url) {
   DCHECK(g_blob_storage_controller);
   g_blob_storage_controller->RegisterBlobUrlFrom(url, src_url);
 }
 
 void TestShellWebBlobRegistryImpl::DoUnregisterBlobUrl(const GURL& url) {
   DCHECK(g_blob_storage_controller);
   g_blob_storage_controller->UnregisterBlobUrl(url);
 }