Allow platform apps to respond to Web Intents

chrome/browser/extensions/platform_app_browsertest.cc

Index: chrome/browser/extensions/platform_app_browsertest.cc
diff --git a/chrome/browser/extensions/platform_app_browsertest.cc b/chrome/browser/extensions/platform_app_browsertest.cc
index 2ee706f3418114ec0380b07cd15162582dd3a783..10ada03cdb5ea4c4cbf80e3cb82be20aa635fa0e 100644
--- a/chrome/browser/extensions/platform_app_browsertest.cc
+++ b/chrome/browser/extensions/platform_app_browsertest.cc
@@ -2,11 +2,15 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include "base/bind.h"
+#include "base/utf_string_conversions.h"
 #include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/automation/automation_util.h"
 #include "chrome/browser/tab_contents/render_view_context_menu.h"
+#include "chrome/browser/extensions/extension_browsertest.h"
 #include "chrome/browser/extensions/extension_test_message_listener.h"
 #include "chrome/browser/extensions/platform_app_browsertest_util.h"
+#include "chrome/browser/extensions/platform_app_launcher.h"
 #include "chrome/browser/extensions/shell_window_registry.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_tabstrip.h"
@@ -15,6 +19,9 @@
 #include "chrome/common/chrome_notification_types.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/render_process_host.h"
+#include "content/public/browser/web_intents_dispatcher.h"
+#include "googleurl/src/gurl.h"
+#include "webkit/glue/web_intent_data.h"
 
 using content::WebContents;
 
@@ -43,6 +50,52 @@ class PlatformAppContextMenu : public RenderViewContextMenu {
   virtual void PlatformCancel() {}
 };
 
+// State holder for the LaunchReply test. This provides an WebIntentsDispatcher
+// that will, when used to launch a Web Intent, will return its reply via this
+// class. The result may then be waited on via WaitUntilReply().
+class LaunchReplyHandler {
+ public:
+  explicit LaunchReplyHandler(webkit_glue::WebIntentData& data)
+      : data_(data),
+        replied_(false),
+        weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
+    intents_dispatcher_ = content::WebIntentsDispatcher::Create(data);
+    intents_dispatcher_->RegisterReplyNotification(base::Bind(
+        &LaunchReplyHandler::OnReply, weak_ptr_factory_.GetWeakPtr()));
+  }
+
+ content::WebIntentsDispatcher* intents_dispatcher() {

[benwells, 2012/08/29 12:19:21]

nit: indenting

[thorogood, 2012/08/30 02:37:42]

Done.

+    return intents_dispatcher_;
+  }
+
+  // Waits until a reply to this Web Intent is provided via the
+  // WebIntentsDispatcher.
+  bool WaitUntilReply() {
+    if (replied_)
+      return true;
+    waiting_ = true;

[benwells, 2012/08/29 12:19:21]

I am not sure if the threading model in tests is the same as in normal runtime;
if it is there is a race here.

E.g. Thread A calls WaitUntilReply, and executes past the if (replied_) check
which is false. Then we switch to thread B which calls OnReply. It sets replied
and sees that we're not waiting so doesn't call quit. Then thread A starts again
and waits forever.

[thorogood, 2012/08/30 02:37:42]

Naively, I tend to actually agree with you -- I've however duplicated this idiom
from some other test, and in fact it seems like a lot of tests do basically the
same thing.

jeremya@ has had a look at the code with me, and he believes that this isn't an
issue. The callback that I'm waiting for is a SyncExtensionFunction, which only
runs on the UI thread, which means that it can only run in this test during a
call to content::RunMessageLoop().

The if (replied_) bit is thus basically just to make calls here idempotent: it
doesn't imply that we're trying to deal with thread race conditions.

[benwells, 2012/08/30 04:29:07]

Ah ok, if both functions always run in the same thread it isn't a problem.

+    content::RunMessageLoop();
+    waiting_ = false;
+    return replied_;
+  }
+
+ private:
+  void OnReply(webkit_glue::WebIntentReplyType reply) {
+    // Note that the ReplyNotification registered on WebIntentsDispatcher does
+    // not include the result data: this is reserved for the source page (which
+    // we don't care about).
+    replied_ = true;
+    if (waiting_)
+      MessageLoopForUI::current()->Quit();
+  }
+
+  webkit_glue::WebIntentData data_;
+  bool replied_;
+  bool waiting_;
+  content::WebIntentsDispatcher* intents_dispatcher_;
+  base::WeakPtrFactory<LaunchReplyHandler> weak_ptr_factory_;
+};
+
 }  // namespace
 
 // Tests that CreateShellWindow doesn't crash if you close it straight away.
@@ -59,6 +112,35 @@ IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, OnLaunchedEvent) {
   ASSERT_TRUE(RunPlatformAppTest("platform_apps/launch")) << message_;
 }
 
+// Tests that platform apps can reply to "launch" events that contain a Web

[benwells, 2012/08/29 12:19:21]

It's great that this is tested! It would be even better if we tested:
- postFailure
- postSuccess
- sending some data in the reply.

I'd be OK with that in a follow up patch.

[thorogood, 2012/08/30 02:37:42]

Thanks! Yep, I'll happily follow this up.

+// Intent. This test does not test the mechanics of invoking a Web Intent
+// from a source page, and short-circuits to LaunchPlatformAppWithWebIntent.
+IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, LaunchReply) {
+  FilePath path = test_data_dir_.AppendASCII("platform_apps/launch_reply");
+  const extensions::Extension* extension = LoadExtension(path);
+  ASSERT_TRUE(extension) << "Failed to load extension.";
+
+  webkit_glue::WebIntentData data(
+      UTF8ToUTF16("http://webintents.org/view"),
+      UTF8ToUTF16("text/plain"),
+      UTF8ToUTF16("irrelevant unserialized string data"));
+  LaunchReplyHandler handler(data);
+
+  // Navigate to a boring page: we don't care what it is, but we require some
+  // source WebContents to launch the Web Intent "from".
+  ui_test_utils::NavigateToURL(browser(), GURL("about:blank"));
+  WebContents* web_contents = chrome::GetActiveWebContents(browser());
+  ASSERT_TRUE(web_contents);
+
+  extensions::LaunchPlatformAppWithWebIntent(
+      browser()->profile(),
+      extension,
+      handler.intents_dispatcher(),
+      web_contents);
+
+  ASSERT_TRUE(handler.WaitUntilReply());
+}
+
 // Tests that platform apps cannot use certain disabled window properties, but
 // can override them and then use them.
 IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, DisabledWindowProperties) {