fix external protocol handling for OOPIFs

chrome/browser/chrome_site_per_process_browsertest.cc

 // Copyright 2014 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/command_line.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
 #include "base/path_service.h"
 #include "base/strings/stringprintf.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/external_protocol/external_protocol_handler.h"
+#include "chrome/browser/external_protocol/external_protocol_handler_unittest.cc"

[alexmos, 2016/12/01 19:21:13]

Why do you need this?  Is this for FakeExternalProtocolHandlerWorker?    You can
only include .h files, so this won't work, but see my note below about avoiding
this.

[davidsac (gone - try alexmos), 2016/12/12 19:15:46]

Done.

+#include "chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/guest_view/browser/guest_view_manager_delegate.h"
 #include "components/guest_view/browser/test_guest_view_manager.h"
 #include "content/public/browser/interstitial_page.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_service.h"
@@ skipping 334 matching lines @@
       test_guest_view_manager()->WaitForSingleGuestCreated();
 
   // Now detach the frame and observe that the guest is destroyed.
   content::WebContentsDestroyedWatcher observer(guest_web_contents);
   EXPECT_TRUE(ExecuteScript(
       active_web_contents,
       "document.body.removeChild(document.querySelector('iframe'));"));
   observer.Wait();
   EXPECT_EQ(0U, test_guest_view_manager()->GetNumGuestsActive());
 }
+
+class RunsAtSomePointExternalProtocolHandlerDelegate

[alexmos, 2016/12/01 19:21:13]

Maybe name this MailtoExternalProtocolHandlerDelegate, which is a bit less vague
about what it's for?  Also, let's put a quick comment before the declaration
about what this class is for.  I.e., it's a helper class used to wait for a
mailto: external protocol request and verify that it succeeded.

[davidsac (gone - try alexmos), 2016/12/12 19:15:46]

Done.

+    : public ExternalProtocolHandler::Delegate {
+ public:
+  bool has_triggered_external_protocol = false;

[alexmos, 2016/12/01 19:21:13]

Usually, this would go under private:, and the name would have an underscore
after it (has_triggered_external_protocol_).  Then, you'd expose this to callers
by a new public function has_triggered_external_protocol(), which just returns
the private field.

[davidsac (gone - try alexmos), 2016/12/12 19:15:46]

Done.

+  void RunExternalProtocolDialog(const GURL& url,
+                                 int render_process_host_id,
+                                 int routing_id,
+                                 ui::PageTransition page_transition,
+                                 bool has_user_gesture) override {}
+  scoped_refptr<shell_integration::DefaultProtocolClientWorker>

[alexmos, 2016/12/01 19:21:13]

Let's separate out individual functions with blank lines to make this easier to
read.

[davidsac (gone - try alexmos), 2016/12/12 19:15:46]

Done.

+  CreateShellWorker(
+      const shell_integration::DefaultWebClientWorkerCallback& callback,
+      const std::string& protocol) override {
+    return new FakeExternalProtocolHandlerWorker(

[alexmos, 2016/12/01 19:21:13]

Instead of this, can you just return "new
shell_integration::DefaultProtocolClientWorker(callback, protocol)", which is
what CreateShellWorker in external_protocol_handler.cc does when there's no
delegate?

[davidsac (gone - try alexmos), 2016/12/12 19:15:46]

Done.

+        callback, protocol, shell_integration::NOT_DEFAULT);
+  }
+  ExternalProtocolHandler::BlockState GetBlockState(
+      const std::string& scheme) override {
+    return ExternalProtocolHandler::GetBlockState(scheme);
+  }
+  void BlockRequest() override {}
+  void LaunchUrlWithoutSecurityCheck(
+      const GURL& url,
+      content::WebContents* web_contents) override {
+    if (!web_contents)
+      return;
+    has_triggered_external_protocol = true;

[alexmos, 2016/12/01 19:21:14]

Perhaps we should also save the |url| and |web_contents| similarly to this flag,
and have the test verify that they have expected values?

[davidsac (gone - try alexmos), 2016/12/12 19:15:46]

Done.

+  }
+  void FinishedProcessingCheck() override {}
+};
+
+IN_PROC_BROWSER_TEST_F(ChromeSitePerProcessTest,

[alexmos, 2016/12/01 19:21:14]

It's a good idea to describe the test in a comment before the declaration, and
include a bug reference so it's easy to get more context.  See other tests here
or site_per_process_browsertest.cc for examples.

[davidsac (gone - try alexmos), 2016/12/12 19:15:46]

Done.

+                       ExternalProtocolCrossSiteNavigation) {

[alexmos, 2016/12/01 19:21:13]

I think the test name should convey that an external protocol is invoked from a
cross-process subframe (cross-site navigation doesn't imply OOPIFs).  So,
something like LaunchExternalProtocolFromSubframe.

[davidsac (gone - try alexmos), 2016/12/12 19:15:46]

Done.

+  RunsAtSomePointExternalProtocolHandlerDelegate* delegate =

[alexmos, 2016/12/01 19:21:13]

No need to use new/delete; this can just be a stack variable.  I.e.,
"RunsAtSomePointExternalProtocolHandlerDelegate delegate;"

[davidsac (gone - try alexmos), 2016/12/12 19:15:46]

Done.

+      new RunsAtSomePointExternalProtocolHandlerDelegate();
+  ChromeResourceDispatcherHostDelegate::
+      SetExternalProtocolHandlerDelegateForTesting(delegate);
+
+  // Load an ordinary page to cause a new render view
+  // host to be created for the WebContents.

[alexmos, 2016/12/01 19:21:13]

I don't think these details are important to the test.  It's fine to just say
"Start at a simple a.com page" or omit this.

[davidsac (gone - try alexmos), 2016/12/12 19:15:46]

Done.

+  GURL start_url(embedded_test_server()->GetURL(
+      "a.com", "//cross_site_iframe_factory.html?a()"));

[alexmos, 2016/12/01 19:21:13]

No need to use cross_site_iframe_factory here, since you don't need to construct
any hierarchy for the first navigation.  In cases like this we often use
simpler/shorter files, such as /title1.html.

Also, no need for double /.

[davidsac (gone - try alexmos), 2016/12/12 19:15:46]

Done.

+
+  // TODO(davidsac):  this does not appear to return a boolean to check that it
+  // works???

[alexmos, 2016/12/01 19:21:14]

Yes, this is a known issue that I've been hoping to fix eventually as part of
https://crbug.com/425335.  No need to include this TODO.

[davidsac (gone - try alexmos), 2016/12/12 19:15:47]

Done.

+  ui_test_utils::NavigateToURL(browser(), start_url);
+
+  // Navigate to a page with a cross-site iframe that triggers a mailto:
+  // external protocol request.

[alexmos, 2016/12/01 19:21:13]

I'd make this comment more about why you couldn't just start the test with this
navigation.  I.e., let's explain that we didn't start at this URL because we
want to navigate the main frame to do a cross-process transfer, which will avoid
a situation where the OOPIF's swapped-out RenderViewHost and the main frame's
active RenderViewHost get the same routing IDs, because that masks the bug.

[davidsac (gone - try alexmos), 2016/12/12 19:15:46]

Done.

+  GURL mailto_url(
+      embedded_test_server()->GetURL("c.com", "/auto_mailto_main_frame.html"));

[alexmos, 2016/12/01 19:21:13]

You could avoid adding this new file by using an existing test file with an
iframe, such as /chrome/test/data/iframe.html.  I.e., you can navigate to
GetURL("b.com", "/iframe.html"), and then navigate the iframe to your c.com
mailto URL with NavigateIframeToURL (see OriginReplicationAllowsAccessToStorage
in this file for an example).

[davidsac (gone - try alexmos), 2016/12/12 19:15:46]

Done.

+
+  // TODO(davidsac):  this does not appear to return a boolean to check that it
+  // works???

[alexmos, 2016/12/01 19:21:13]

Same comment as for NavigateToURL above.

[davidsac (gone - try alexmos), 2016/12/12 19:15:46]

Done.

+  ui_test_utils::NavigateToURL(browser(), mailto_url);
+
+  EXPECT_TRUE(delegate->has_triggered_external_protocol);
+  delete delegate;
+}

[alexmos, 2016/12/01 19:21:13]

Since SetExternalProtocolHandlerDelegateForTesting sets a global flag, we want
to ensure other tests that run after this won't be affected by it, so how about
clearing it at the end the test with
SetExternalProtocolHandlerDelegateForTesting(nullptr)?

[davidsac (gone - try alexmos), 2016/12/12 19:15:46]

Done.