Merge 120433 - Fix up Dispatcher ownership issues.

Make the picker controller not assume a single WebIntentsDispatcher. Make the picker controller switch tabs to the source tab when a window intent finishes. Add documentation about ownership of WebIntentsDispatcher.
Add reply flag to the dispatcher, avoiding tab double-close


R=binji@chromium.org
BUG=111444
TEST=WebIntentPickerControllerTest.*


Review URL: http://codereview.chromium.org/9301023

TBR=gbillock@chromium.org
Review URL: https://chromiumcodereview.appspot.com/9362024

git-svn-id: svn://svn.chromium.org/chrome/branches/1025/src@121070 0039d316-1c4b-4281-b951-d872f2087c98

5 files changed, 38 insertions, 21 deletions

diff --git a/content/browser/intents/intent_injector.cc b/content/browser/intents/intent_injector.cc
index ae41fc6..7bab732 100644
--- a/content/browser/intents/intent_injector.cc
+++ b/content/browser/intents/intent_injector.cc
@@ -94,6 +94,9 @@ void IntentInjector::OnReply(webkit_glue::WebIntentReplyType reply_type,
     NOTREACHED();
 
   if (intents_dispatcher_) {
-    intents_dispatcher_->SendReplyMessage(reply_type, data);
+    // Ensure we only call SendReplyMessage once.
+    content::WebIntentsDispatcher* intents_dispatcher = intents_dispatcher_;
+    intents_dispatcher_ = NULL;
+    intents_dispatcher->SendReplyMessage(reply_type, data);
   }
 }
diff --git a/content/browser/intents/web_intents_dispatcher_impl.cc b/content/browser/intents/web_intents_dispatcher_impl.cc
index 35b2c4c..a896aa6 100644
--- a/content/browser/intents/web_intents_dispatcher_impl.cc
+++ b/content/browser/intents/web_intents_dispatcher_impl.cc
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -38,17 +38,19 @@ void WebIntentsDispatcherImpl::SendReplyMessage(
     const string16& data) {
   intent_injector_ = NULL;
 
-  if (!web_contents())
-    return;
+  if (web_contents()) {
+    Send(new IntentsMsg_WebIntentReply(
+        routing_id(), reply_type, data, intent_id_));
+  }
 
-  Send(new IntentsMsg_WebIntentReply(
-      routing_id(), reply_type, data, intent_id_));
   if (!reply_notifier_.is_null())
-    reply_notifier_.Run();
+    reply_notifier_.Run(reply_type);
+
+  delete this;
 }
 
 void WebIntentsDispatcherImpl::RegisterReplyNotification(
-    const base::Closure& closure) {
+    const base::Callback<void(webkit_glue::WebIntentReplyType)>& closure) {
   reply_notifier_ = closure;
 }
 
diff --git a/content/browser/intents/web_intents_dispatcher_impl.h b/content/browser/intents/web_intents_dispatcher_impl.h
index bfd4655..bed4e71 100644
--- a/content/browser/intents/web_intents_dispatcher_impl.h
+++ b/content/browser/intents/web_intents_dispatcher_impl.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -35,7 +35,9 @@ class WebIntentsDispatcherImpl : public content::WebIntentsDispatcher,
   virtual void DispatchIntent(content::WebContents* destination_tab) OVERRIDE;
   virtual void SendReplyMessage(webkit_glue::WebIntentReplyType reply_type,
                                 const string16& data) OVERRIDE;
-  virtual void RegisterReplyNotification(const base::Closure& closure) OVERRIDE;
+  virtual void RegisterReplyNotification(
+      const base::Callback<void(webkit_glue::WebIntentReplyType)>&
+          closure) OVERRIDE;
 
   // content::WebContentsObserver implementation.
   virtual void WebContentsDestroyed(content::WebContents* tab) OVERRIDE;
@@ -51,7 +53,7 @@ class WebIntentsDispatcherImpl : public content::WebIntentsDispatcher,
   IntentInjector* intent_injector_;
 
   // A callback to be notified when SendReplyMessage is called.
-  base::Closure reply_notifier_;
+  base::Callback<void(webkit_glue::WebIntentReplyType)> reply_notifier_;
 
   DISALLOW_COPY_AND_ASSIGN(WebIntentsDispatcherImpl);
 };
diff --git a/content/public/browser/web_contents_delegate.h b/content/public/browser/web_contents_delegate.h
index 9739429..7af4874 100644
--- a/content/public/browser/web_contents_delegate.h
+++ b/content/public/browser/web_contents_delegate.h
@@ -349,8 +349,8 @@ class CONTENT_EXPORT WebContentsDelegate {
                                      const string16& title,
                                      const string16& disposition) {}
 
-  // Web Intents notification handler. Takes ownership of the
-  // |intents_dispatcher|.
+  // Web Intents notification handler. See WebIntentsDispatcher for
+  // documentation of callee responsibility for the dispatcher.
   virtual void WebIntentDispatch(WebContents* tab,
                                  WebIntentsDispatcher* intents_dispatcher);
 
diff --git a/content/public/browser/web_intents_dispatcher.h b/content/public/browser/web_intents_dispatcher.h
index 33b10e7..ccfdf52 100644
--- a/content/public/browser/web_intents_dispatcher.h
+++ b/content/public/browser/web_intents_dispatcher.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -9,6 +9,7 @@
 #include "webkit/glue/web_intent_reply_data.h"
 
 namespace webkit_glue {
+enum WebIntentReplyType;
 struct WebIntentData;
 }
 
@@ -17,12 +18,19 @@ namespace content {
 class WebContents;
 
 // This class is the coordinator for dispatching web intents and seeing that
-// return messages are sent to the correct invoking context. TheWebContents
+// return messages are sent to the correct invoking context. The WebContents
 // for the invoking context will create one of these for each intent and hand
-// ownership to the client WebContentsDelegate code. The WebContentsDelegate
+// a pointer to the client WebContentsDelegate code. The WebContentsDelegate
 // code can then read the intent data, create UI to pick the service, and
-// create a new context for that service. At that point, it should call
-// DispatchIntent, which will connect the object to the new context.
+// create a new context for that service.
+//
+// At that point, it should call DispatchIntent, which will connect the object
+// to the new context. If anything goes wrong, the client should call
+// SendReplyMessage with an error. That will self-delete the object.
+//
+// At that point, before the client may use the object again, it must register a
+// reply notification, so it can avoid referencing the dispatcher after other
+// code calls SendReplyMessage.
 class CONTENT_EXPORT WebIntentsDispatcher {
  public:
   virtual ~WebIntentsDispatcher() {}
@@ -30,16 +38,18 @@ class CONTENT_EXPORT WebIntentsDispatcher {
   // Get the intent data being dispatched.
   virtual const webkit_glue::WebIntentData& GetIntent() = 0;
 
-  // Attach the intent to a new context in which the service is loaded.
+  // Attach the intent to a new context in which the service page is loaded.
   virtual void DispatchIntent(WebContents* web_contents) = 0;
 
   // Return a success or failure message to the source context which invoked
-  // the intent.
+  // the intent. Deletes the object; it should not be used after this call
+  // returns. Calls the reply notification, if registered.
   virtual void SendReplyMessage(webkit_glue::WebIntentReplyType reply_type,
                                 const string16& data) = 0;
 
   // Register a callback to be notified when SendReplyMessage is called.
-  virtual void RegisterReplyNotification(const base::Closure& closure) = 0;
+  virtual void RegisterReplyNotification(
+      const base::Callback<void(webkit_glue::WebIntentReplyType)>& closure) = 0;
 };
 
 }  // namespace content