Allow Signin page to open other chrome:// URLs if login content in <webview>

chrome/browser/apps/guest_view/web_view_browsertest.cc

Index: chrome/browser/apps/guest_view/web_view_browsertest.cc
diff --git a/chrome/browser/apps/guest_view/web_view_browsertest.cc b/chrome/browser/apps/guest_view/web_view_browsertest.cc
index a7ef8be795ec7260c255e11b463a2f9916d3ed6a..9eaac9abeb61283b7fc29f97fe661fba9ad2f0f1 100644
--- a/chrome/browser/apps/guest_view/web_view_browsertest.cc
+++ b/chrome/browser/apps/guest_view/web_view_browsertest.cc
@@ -5,6 +5,7 @@
 #include "base/path_service.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
+#include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/apps/app_browsertest_util.h"
 #include "chrome/browser/chrome_content_browser_client.h"
 #include "chrome/browser/prerender/prerender_link_manager.h"
@@ -179,7 +180,8 @@ class MockWebContentsDelegate : public content::WebContentsDelegate {
  public:
   MockWebContentsDelegate()
       : requested_(false),
-        checked_(false) {}
+        checked_(false),
+        open_url_from_tab_(false) {}
   ~MockWebContentsDelegate() override {}
 
   void RequestMediaAccessPermission(
@@ -200,6 +202,15 @@ class MockWebContentsDelegate : public content::WebContentsDelegate {
     return true;
   }
 
+  content::WebContents* OpenURLFromTab(
+      content::WebContents* source,
+      const content::OpenURLParams& params) override {
+    open_url_from_tab_ = true;
+    if (open_url_message_loop_runner_.get())
+      open_url_message_loop_runner_->Quit();
+    return NULL;
+  }
+
   void WaitForRequestMediaPermission() {
     if (requested_)
       return;
@@ -214,11 +225,20 @@ class MockWebContentsDelegate : public content::WebContentsDelegate {
     check_message_loop_runner_->Run();
   }
 
+  void WaitForOpenURLFromTab() {
+    if (open_url_from_tab_)
+      return;
+    open_url_message_loop_runner_ = new content::MessageLoopRunner;
+    open_url_message_loop_runner_->Run();
+  }
+
  private:
   bool requested_;
   bool checked_;
+  bool open_url_from_tab_;

[Charlie Reis, 2015/02/19 22:56:28]

nit: Make past tense, e.g., did_open_url_from_tab_

[Fady Samuel, 2015/02/20 00:33:06]

Done.

   scoped_refptr<content::MessageLoopRunner> request_message_loop_runner_;
   scoped_refptr<content::MessageLoopRunner> check_message_loop_runner_;
+  scoped_refptr<content::MessageLoopRunner> open_url_message_loop_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(MockWebContentsDelegate);
 };
@@ -1831,6 +1851,76 @@ void WebViewTest::MediaAccessAPIAllowTestHelper(const std::string& test_name) {
   mock->WaitForRequestMediaPermission();
 }
 
+IN_PROC_BROWSER_TEST_F(WebViewTest, OpenURLFromTab_CurrentTab_Abort) {
+  LoadAppWithGuest("web_view/simple");
+
+  // Verify that OpenURLFromTab with a window disposition of CURRENT_TAB will
+  // navigate the current <webview>.
+  ExtensionTestMessageListener load_listener("WebViewTest.LOADSTOP", false);
+
+  // Navigating to a file URL is forbidden inside a <webview>.
+  content::OpenURLParams params(GURL("file://foo"),
+                                content::Referrer(),
+                                CURRENT_TAB,
+                                ui::PAGE_TRANSITION_AUTO_TOPLEVEL,
+                                true /* is_renderer_initiated */);
+  GetGuestWebContents()->GetDelegate()->OpenURLFromTab(
+      GetGuestWebContents(), params);
+
+  ASSERT_TRUE(load_listener.WaitUntilSatisfied());
+
+  // Verify that the <webview> ends up at about:blank.
+  EXPECT_EQ(GURL(url::kAboutBlankURL),
+            GetGuestWebContents()->GetLastCommittedURL());
+}
+
+IN_PROC_BROWSER_TEST_F(WebViewTest, OpenURLFromTab_NewWindow_Abort) {
+  LoadAppWithGuest("web_view/simple");
+
+  // Verify that OpenURLFromTab with a window disposition of NEW_BACKGROUND_TAB
+  // will trigger the <webview>'s New Window API.
+  ExtensionTestMessageListener new_window_listener(
+      "WebViewTest.NEWWINDOW", false);
+
+  // Navigating to a file URL is forbidden inside a <webview>.
+  content::OpenURLParams params(GURL("file://foo"),
+                                content::Referrer(),
+                                NEW_BACKGROUND_TAB,
+                                ui::PAGE_TRANSITION_AUTO_TOPLEVEL,
+                                true /* is_renderer_initiated */);
+  GetGuestWebContents()->GetDelegate()->OpenURLFromTab(
+      GetGuestWebContents(), params);
+
+  ASSERT_TRUE(new_window_listener.WaitUntilSatisfied());
+
+  content::WebContents* new_guest_web_contents=

[Charlie Reis, 2015/02/19 22:56:28]

nit: Comma before =

[Fady Samuel, 2015/02/20 00:33:06]

Done.

[Charlie Reis, 2015/02/20 00:37:59]

Ha!  Yes, I meant space, of course.

+      GetGuestViewManager()->GetLastGuestCreated();
+  // Verify that a new guest was created.

[Charlie Reis, 2015/02/19 22:56:28]

nit: Move comment above previous statement.

[Fady Samuel, 2015/02/20 00:33:06]

Done.

+  EXPECT_NE(GetGuestWebContents(), new_guest_web_contents);
+
+  // Verify that the new <webview> guest ends up at about:blank.
+  EXPECT_EQ(GURL(url::kAboutBlankURL),
+            new_guest_web_contents->GetLastCommittedURL());
+}
+
+IN_PROC_BROWSER_TEST_F(WebViewTest, ContextMenuLanguageSettings) {

[Charlie Reis, 2015/02/19 22:56:28]

nit: Let's add a comment about the test, saying that we expect this case to
succeed, unlike navigations initiated by the webview.

[Fady Samuel, 2015/02/20 00:33:06]

Done.

+  LoadAppWithGuest("web_view/context_menus/basic");
+
+  content::WebContents* guest_web_contents = GetGuestWebContents();
+  content::WebContents* embedder = GetEmbedderWebContents();
+  ASSERT_TRUE(embedder);
+  scoped_ptr<MockWebContentsDelegate> mock(new MockWebContentsDelegate());
+  embedder->SetDelegate(mock.get());
+
+  // Create and build our test context menu.
+  GURL page_url("http://www.google.com");
+  scoped_ptr<TestRenderViewContextMenu> menu(TestRenderViewContextMenu::Create(
+      guest_web_contents, page_url, GURL(), GURL()));
+  menu->ExecuteCommand(IDC_CONTENT_CONTEXT_LANGUAGE_SETTINGS, 0);
+
+  mock->WaitForOpenURLFromTab();

[Charlie Reis, 2015/02/19 22:56:28]

Can we verify something that shows the navigation succeeded?  Maybe add a
WebContentsAddedObserver and show that it gets to the language settings URL?

[Fady Samuel, 2015/02/20 00:33:06]

Done.

+}
+
 IN_PROC_BROWSER_TEST_F(WebViewTest, ContextMenusAPI_Basic) {
   LoadAppWithGuest("web_view/context_menus/basic");
 
@@ -1956,7 +2046,7 @@ IN_PROC_BROWSER_TEST_F(WebViewTest, ChromeVoxInjection) {
 #endif
 IN_PROC_BROWSER_TEST_F(WebViewTest, MAYBE_TearDownTest) {
   const extensions::Extension* extension =
-      LoadAndLaunchPlatformApp("web_view/teardown", "guest-loaded");
+      LoadAndLaunchPlatformApp("web_view/simple", "WebViewTest.LAUNCHED");
   extensions::AppWindow* window = NULL;
   if (!GetAppWindowCount())
     window = CreateAppWindow(extension);
@@ -1965,7 +2055,7 @@ IN_PROC_BROWSER_TEST_F(WebViewTest, MAYBE_TearDownTest) {
   CloseAppWindow(window);
 
   // Load the app again.
-  LoadAndLaunchPlatformApp("web_view/teardown", "guest-loaded");
+  LoadAndLaunchPlatformApp("web_view/simple", "WebViewTest.LAUNCHED");
 }
 
 // In following GeolocationAPIEmbedderHasNoAccess* tests, embedder (i.e. the