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

3 files changed, 71 insertions, 15 deletions

diff --git a/chrome/browser/ui/intents/web_intent_picker_controller.cc b/chrome/browser/ui/intents/web_intent_picker_controller.cc
index 0d1bf44..e3b7eb8 100644
--- a/chrome/browser/ui/intents/web_intent_picker_controller.cc
+++ b/chrome/browser/ui/intents/web_intent_picker_controller.cc
@@ -14,6 +14,7 @@
 #include "chrome/browser/intents/web_intents_registry_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/tabs/tab_strip_model.h"
+#include "chrome/browser/ui/browser_list.h"
 #include "chrome/browser/ui/browser_navigator.h"
 #include "chrome/browser/ui/intents/web_intent_picker.h"
 #include "chrome/browser/ui/intents/web_intent_picker_model.h"
@@ -128,6 +129,7 @@ WebIntentPickerController::WebIntentPickerController(
       picker_model_(new WebIntentPickerModel()),
       pending_async_count_(0),
       picker_shown_(false),
+      intents_dispatcher_(NULL),
       service_tab_(NULL) {
   content::NavigationController* controller =
       &wrapper->web_contents()->GetController();
@@ -142,7 +144,7 @@ WebIntentPickerController::~WebIntentPickerController() {
 
 void WebIntentPickerController::SetIntentsDispatcher(
     content::WebIntentsDispatcher* intents_dispatcher) {
-  intents_dispatcher_.reset(intents_dispatcher);
+  intents_dispatcher_ = intents_dispatcher;
   intents_dispatcher_->RegisterReplyNotification(
       base::Bind(&WebIntentPickerController::OnSendReturnMessage,
                  base::Unretained(this)));
@@ -230,7 +232,7 @@ void WebIntentPickerController::OnInlineDispositionWebContentsCreated(
 }
 
 void WebIntentPickerController::OnCancelled() {
-  if (!intents_dispatcher_.get())
+  if (!intents_dispatcher_)
     return;
 
   if (service_tab_) {
@@ -249,19 +251,32 @@ void WebIntentPickerController::OnClosing() {
   picker_ = NULL;
 }
 
-void WebIntentPickerController::OnSendReturnMessage() {
+void WebIntentPickerController::OnSendReturnMessage(
+    webkit_glue::WebIntentReplyType reply_type) {
   ClosePicker();
 
-  if (service_tab_) {
+  if (service_tab_ &&
+      reply_type != webkit_glue::WEB_INTENT_SERVICE_TAB_CLOSED) {
     int index = TabStripModel::kNoTab;
     Browser* browser = Browser::GetBrowserForController(
         &service_tab_->GetController(), &index);
     if (browser) {
       browser->tabstrip_model()->CloseTabContentsAt(
           index, TabStripModel::CLOSE_CREATE_HISTORICAL_TAB);
+
+      // Activate source tab.
+      Browser* source_browser =
+          BrowserList::FindBrowserWithWebContents(wrapper_->web_contents());
+      if (source_browser) {
+        int source_index =
+            source_browser->tabstrip_model()->GetIndexOfTabContents(wrapper_);
+        source_browser->ActivateTabAt(source_index, false);
+      }
     }
     service_tab_ = NULL;
   }
+
+  intents_dispatcher_ = NULL;
 }
 
 void WebIntentPickerController::OnWebIntentDataAvailable(
diff --git a/chrome/browser/ui/intents/web_intent_picker_controller.h b/chrome/browser/ui/intents/web_intent_picker_controller.h
index 1a8b329..bf0b7f0 100644
--- a/chrome/browser/ui/intents/web_intent_picker_controller.h
+++ b/chrome/browser/ui/intents/web_intent_picker_controller.h
@@ -15,6 +15,7 @@
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
 #include "webkit/glue/web_intent_data.h"
+#include "webkit/glue/web_intent_reply_data.h"
 
 class Browser;
 class GURL;
@@ -70,6 +71,7 @@ class WebIntentPickerController : public content::NotificationObserver,
   virtual void OnClosing() OVERRIDE;
 
  private:
+  friend class WebIntentPickerControllerTest;
   friend class WebIntentPickerControllerBrowserTest;
   friend class InvokingTabObserver;
   class WebIntentDataFetcher;
@@ -77,7 +79,7 @@ class WebIntentPickerController : public content::NotificationObserver,
 
   // Gets a notification when the return message is sent to the source tab,
   // so we can close the picker dialog or service tab.
-  void OnSendReturnMessage();
+  void OnSendReturnMessage(webkit_glue::WebIntentReplyType reply_type);
 
   // Exposed for tests only.
   void set_picker(WebIntentPicker* picker) { picker_ = picker; }
@@ -132,9 +134,10 @@ class WebIntentPickerController : public content::NotificationObserver,
   // case, a picker may be non-NULL before it is shown.
   bool picker_shown_;
 
-  // The routing object for the renderer which launched the intent.
-  // Contains the intent data and a way to signal back to the client page.
-  scoped_ptr<content::WebIntentsDispatcher> intents_dispatcher_;
+  // Weak pointer to the routing object for the renderer which launched the
+  // intent. Contains the intent data and a way to signal back to the
+  // client page.
+  content::WebIntentsDispatcher* intents_dispatcher_;
 
   // Weak pointer to the tab servicing the intent. Remembered in order to
   // close it when a reply is sent.
diff --git a/chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc b/chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc
index a4dc26c..1fa7028 100644
--- a/chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc
+++ b/chrome/browser/ui/intents/web_intent_picker_controller_browsertest.cc
@@ -17,6 +17,7 @@
 #include "chrome/browser/ui/intents/web_intent_picker_model_observer.h"
 #include "chrome/browser/ui/tab_contents/tab_contents_wrapper.h"
 #include "chrome/browser/webdata/web_data_service.h"
+#include "chrome/common/url_constants.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/web_contents.h"
@@ -94,7 +95,8 @@ class IntentsDispatcherMock : public content::WebIntentsDispatcher {
                                 const string16& data) OVERRIDE {
   }
 
-  virtual void RegisterReplyNotification(const base::Closure&) OVERRIDE {
+  virtual void RegisterReplyNotification(
+      const base::Callback<void(webkit_glue::WebIntentReplyType)>&) OVERRIDE {
   }
 
   webkit_glue::WebIntentData intent_;
@@ -127,8 +129,9 @@ class WebIntentPickerControllerBrowserTest : public InProcessBrowserTest {
     web_data_service_->AddWebIntentService(service);
   }
 
-  void OnSendReturnMessage() {
-    controller_->OnSendReturnMessage();
+  void OnSendReturnMessage(
+    webkit_glue::WebIntentReplyType reply_type) {
+    controller_->OnSendReturnMessage(reply_type);
   }
 
   void OnServiceChosen(size_t index, Disposition disposition) {
@@ -157,17 +160,17 @@ IN_PROC_BROWSER_TEST_F(WebIntentPickerControllerBrowserTest, ChooseService) {
   webkit_glue::WebIntentData intent;
   intent.action = ASCIIToUTF16("a");
   intent.type = ASCIIToUTF16("b");
-  IntentsDispatcherMock* host = new IntentsDispatcherMock(intent);
-  controller_->SetIntentsDispatcher(host);
+  IntentsDispatcherMock dispatcher(intent);
+  controller_->SetIntentsDispatcher(&dispatcher);
 
   OnServiceChosen(1, WebIntentPickerModel::DISPOSITION_WINDOW);
   ASSERT_EQ(2, browser()->tab_count());
   EXPECT_EQ(GURL(kServiceURL2),
             browser()->GetSelectedWebContents()->GetURL());
 
-  EXPECT_TRUE(host->dispatched_);
+  EXPECT_TRUE(dispatcher.dispatched_);
 
-  OnSendReturnMessage();
+  OnSendReturnMessage(webkit_glue::WEB_INTENT_REPLY_SUCCESS);
   ASSERT_EQ(1, browser()->tab_count());
 }
 
@@ -182,3 +185,38 @@ IN_PROC_BROWSER_TEST_F(WebIntentPickerControllerBrowserTest, OpenCancelOpen) {
   controller_->ShowDialog(browser(), kAction1, kType);
   OnCancelled();
 }
+
+IN_PROC_BROWSER_TEST_F(WebIntentPickerControllerBrowserTest,
+                       CloseTargetTabReturnToSource) {
+  AddWebIntentService(kAction1, kServiceURL1);
+
+  GURL original = browser()->GetSelectedWebContents()->GetURL();
+
+  // Open a new page, but keep focus on original.
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), GURL(chrome::kChromeUINewTabURL), NEW_BACKGROUND_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+  ASSERT_EQ(2, browser()->tab_count());
+  EXPECT_EQ(original, browser()->GetSelectedWebContents()->GetURL());
+
+  controller_->ShowDialog(browser(), kAction1, kType);
+  picker_.WaitForPendingAsync();
+  EXPECT_EQ(1, picker_.num_items_);
+
+  webkit_glue::WebIntentData intent;
+  intent.action = ASCIIToUTF16("a");
+  intent.type = ASCIIToUTF16("b");
+  IntentsDispatcherMock dispatcher(intent);
+  controller_->SetIntentsDispatcher(&dispatcher);
+
+  OnServiceChosen(0, WebIntentPickerModel::DISPOSITION_WINDOW);
+  ASSERT_EQ(3, browser()->tab_count());
+  EXPECT_EQ(GURL(kServiceURL1),
+            browser()->GetSelectedWebContents()->GetURL());
+
+  EXPECT_TRUE(dispatcher.dispatched_);
+
+  OnSendReturnMessage(webkit_glue::WEB_INTENT_REPLY_SUCCESS);
+  ASSERT_EQ(2, browser()->tab_count());
+  EXPECT_EQ(original, browser()->GetSelectedWebContents()->GetURL());
+}