"Redirecting URLs to Packaged Apps" implementation: revised

chrome/browser/apps/app_browsertest.cc

 // Copyright 2013 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 "apps/launcher.h"
 #include "apps/native_app_window.h"
 #include "apps/shell_window.h"
 #include "apps/shell_window_registry.h"
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 617 matching lines @@
 IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, OpenLink) {
   ASSERT_TRUE(StartEmbeddedTestServer());
   content::WindowedNotificationObserver observer(
       chrome::NOTIFICATION_TAB_ADDED,
       content::Source<content::WebContentsDelegate>(browser()));
   LoadAndLaunchPlatformApp("open_link");
   observer.Wait();
   ASSERT_EQ(2, browser()->tab_strip_model()->count());
 }
 
+
+// Test that clicks on any type of links in a tab + window.open() launch
+// an app that has a matching handler in url_handlers.
+IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest,
+                       UrlHandlersNavigationsInTabIntercepted) {

[not at google - send to devlin, 2013/09/07 00:54:56]

can you put these tests in a different file? This one is already too big.

[sergeygs, 2013/09/09 09:55:36]

Done. If we start in this direction, it would be nice to split this file further
based on groups of tests (e.g. there is something like a dozen file_handlers
tests).

[not at google - send to devlin, 2013/09/09 19:01:13]

Absolutely.

+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kDisablePopupBlocking);
+
+  ASSERT_TRUE(StartEmbeddedTestServer());
+
+  InstallPlatformApp("url_handlers/handler");
+
+  const std::string href_root =
+      "/extensions/platform_apps/url_handlers/launching_tabs/";
+  const std::string hrefs[] = {
+      "click_link.html",
+      "click_blank_link.html",
+      "call_window_open.html",
+  };

[not at google - send to devlin, 2013/09/07 00:54:56]

might as well use const char*s here rather than std::string, and perhaps
StringPrintf below.

[sergeygs, 2013/09/09 09:55:36]

Done.

+
+  for (size_t i = 0; i < sizeof(hrefs) / sizeof(*hrefs); ++i) {

[not at google - send to devlin, 2013/09/07 00:54:56]

use arraysize(hrefs) or ARRAYSIZE_UNSAFE if that doesn't work.

[sergeygs, 2013/09/09 09:55:36]

Obsolete.

+    GURL url = embedded_test_server()->GetURL(href_root + hrefs[i]);
+
+    ui_test_utils::NavigateToURLWithDisposition(
+        browser(), url,
+        CURRENT_TAB,
+        ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
+
+    ExtensionTestMessageListener handler_listener("Handler launched", false);
+    ASSERT_TRUE(handler_listener.WaitUntilSatisfied());
+  }
+
+  // The above navigations click on 2 links (regular and target blank) and call
+  // 1 window.open(), all of which should be intercepted and launch the
+  // handler app.
+  ASSERT_EQ(3U, GetShellWindowCount());

[not at google - send to devlin, 2013/09/07 00:54:56]

asserting based on the number of shell windows open is a bit weak. better would
be to use chrome.test.sendMessage from the onLaunched event, intercept it from
here, then assert about the contents. rather than from the background page. the
test is driven from here; assertions should be made from here too.

if you make the apps dumb and bring the logic into here you can write more
complex tests. for example, you haven't tested subdomains not matching, http vs
https, incognito mode, POST requests, XHR...

[sergeygs, 2013/09/09 09:55:36]

chrome.test.sendMessage _is_ used here, see the end of the loop. The ASSERT_EQ
here is just to test that no extra windows have been opened.

Your point about driving and asserting in the same place is valid in general,
but here it can't be fully applied in any case. One of the reasons is that these
tests verify a declarative manifest entry: I need to have a specific URL pattern
in the handler app's manifest. It makes sense then to have a matching concrete
URL somewhere nearby, e.g. in the launcher app.

Also, this approach is used all around the tests: I just followed the suit.
Mentioning that not as a justification, but as an explanation.

[not at google - send to devlin, 2013/09/09 19:01:13]

Ah, I see. Thanks.
externally_connectable has the exact same problem - see those tests. Basically
there is a class called TestExtensionDir which lets you write your own manifest
files. See:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
Right. Agreed. Test cleanup is a slow process but particularly with this in its
own file, breaking untidy test idioms is not really an issue. Then further tests
which base themselves off your tests will be better.

+}
+
+// Test that a click on link in an app windows launches an app that has
+// a matching handler in url_handlers.
+IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest,
+                       UrlHandlersClickInAppIntercepted) {
+  ASSERT_TRUE(StartEmbeddedTestServer());
+
+  ExtensionTestMessageListener launcher_listener("Launcher launched", false);
+  ExtensionTestMessageListener handler_listener("Handler launched", false);
+
+  InstallPlatformApp("url_handlers/handler");
+  LoadAndLaunchPlatformApp("url_handlers/launcher");
+
+  ASSERT_TRUE(launcher_listener.WaitUntilSatisfied());
+  ASSERT_TRUE(handler_listener.WaitUntilSatisfied());
+
+  // The launcher clicks on a link, which gets intercepted and launches the
+  // handler.
+  ASSERT_EQ(2U, GetShellWindowCount());
+}
+
+// Test that a webview can navigate to URLs even when there are apps installed
+// that have matching url_handlers.
+IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest,
+                       UrlHandlersWebviewNavigationNotIntercepted) {
+  ASSERT_TRUE(StartEmbeddedTestServer());
+
+  ExtensionTestMessageListener handler_listener("Handler launched", false);
+
+  InstallPlatformApp("url_handlers/handler_with_webview");
+  LoadAndLaunchPlatformApp("url_handlers/launcher");
+
+  ASSERT_TRUE(handler_listener.WaitUntilSatisfied());
+  base::PlatformThread::Sleep(TestTimeouts::action_timeout());
+
+  // The launcher clicks on a link, which gets intercepted and launches the
+  // handler. The handler also redirects an embedded webview to the URL. The
+  // webview should just navigate without creating an endless loop of
+  // navigate-intercept-launch sequences with multiplying handler's windows.
+  // There should be 2 windows only: launcher's and handler's.
+  ASSERT_EQ(2U, GetShellWindowCount());
+}
+
 IN_PROC_BROWSER_TEST_F(PlatformAppBrowserTest, MutationEventsDisabled) {
   ASSERT_TRUE(RunPlatformAppTest("platform_apps/mutation_events")) << message_;
 }
 
 // Test that windows created with an id will remember and restore their
 // geometry when opening new windows.
 // Originally disabled due to flakiness (see http://crbug.com/155459)
 // but now because a regression breaks the test (http://crbug.com/160343).
 #if defined(TOOLKIT_GTK)
 #define MAYBE_ShellWindowRestorePosition DISABLED_ShellWindowRestorePosition
@@ skipping 463 matching lines @@
 
   while (!ContainsKey(opener_app_ids_, file_manager->id())) {
     content::RunAllPendingInMessageLoop();
   }
 }
 
 #endif  // defined(OS_CHROMEOS)
 
 
 }  // namespace extensions