Allow platform apps to respond to Web Intents

chrome/browser/extensions/platform_app_browsertest.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 "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"
 #include "chrome/browser/ui/extensions/application_launch.h"
 #include "chrome/browser/ui/extensions/shell_window.h"
 #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;
 
 namespace extensions {
 
 namespace {
 // Non-abstract RenderViewContextMenu class.
 class PlatformAppContextMenu : public RenderViewContextMenu {
  public:
   PlatformAppContextMenu(WebContents* web_contents,
                          const content::ContextMenuParams& params)
       : RenderViewContextMenu(web_contents, params) {}
 
   bool HasCommandWithId(int command_id) {
     return menu_model_.GetIndexOfCommandId(command_id) != -1;
   }
 
  protected:
   // These two functions implement pure virtual methods of
   // RenderViewContextMenu.
   virtual bool GetAcceleratorForCommandId(int command_id,
                                           ui::Accelerator* accelerator) {
     return false;
   }
   virtual void PlatformInit() {}
   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.
 // LauncherPlatformAppBrowserTest relies on this behaviour, but is only run for
 // ash, so we test that it works here.
 IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, CreateAndCloseShellWindow) {
   const Extension* extension = LoadAndLaunchPlatformApp("minimal");
   ShellWindow* window = CreateShellWindow(extension);
   CloseShellWindow(window);
 }
 
 // Tests that platform apps received the "launch" event when launched.
 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) {
   ASSERT_TRUE(RunPlatformAppTest("platform_apps/disabled_window_properties"))
       << message_;
 }
 
 IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, EmptyContextMenu) {
   ExtensionTestMessageListener launched_listener("Launched", false);
   LoadAndLaunchPlatformApp("minimal");
@@ skipping 275 matching lines @@
   LoadAndLaunchPlatformApp("open_link");
   observer.Wait();
   ASSERT_EQ(2, browser()->tab_count());
 }
 
 IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, MutationEventsDisabled) {
   ASSERT_TRUE(RunPlatformAppTest("platform_apps/mutation_events")) << message_;
 }
 
 }  // namespace extensions