Use (Oobe)WebViewDialog as WebContentsDelegate for views-based oobe WebContents

Ensure that the OobeWebViewDialog is the WebContentsDelegate for the
WebContents used in views-based sign-in/OOBE. This fixes
camera access by SAML providers on the sign-in screen, because
the versions of CheckMediaAccessPermission/RequestMediaAccessPermission
forwarding to MediaCaptureDevicesDispatcher are invoked again.

Move focus/keyboard handling from OobeUIDialogDelegate to
OobeWebViewDialog to make changing the delegate unnecessary.

Background:
WebDialogView usually acts as the WebContentsDelegate for the
WebContents it embeds. CL:1178111 introduced the subclasss
OobeWebDialogView which customizes the WebContentsDelegate handling to
allow media requests.
CL:1197785 then exchanged the WebContents' delegate altogether to
implement keyboard/focus handling, orphaning the functions introduced in
CL:1178111. This CL moves the keyboard/focus handling to the
OobeWebDialogView and restores its function as WebContentsDelegate.

Bug: 900323
Test: out/Default/browser_tests --gtest_filter=*SAMLPolicy*Test*TestLoginMedia*
Change-Id: I6c12b8b08efaa71a9f9cbb93d674448cdccbdeaf
Reviewed-on: https://chromium-review.googlesource.com/c/1323075
Commit-Queue: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Quan Nguyen <qnnguyen@chromium.org>
Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606111}

chrome/browser/chromeos/login/saml/saml_browsertest.cc

@@ -7,6 +7,7 @@
 #include <string>
 #include <utility>
 
+#include "ash/public/cpp/ash_features.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
@@ -1497,10 +1498,37 @@ IN_PROC_BROWSER_TEST_F(SAMLPolicyTest, DISABLED_SAMLInterstitialNext) {
   session_start_waiter.Wait();
 }
 
+// A specialization of SAMLPolicyTest which doesn't pass the command-line switch
+// forcing the WebUI login, thus allowing views-based login.
+class SAMLPolicyViewsBasedLoginTest : public SAMLPolicyTest {
+ public:
+  SAMLPolicyViewsBasedLoginTest() = default;
+  ~SAMLPolicyViewsBasedLoginTest() override = default;
+
+ protected:
+  // OobeBaseTest:
+  bool ShouldForceWebUiLogin() override {
+    // Allow the Views-based login to be used.
+    return false;
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(SAMLPolicyViewsBasedLoginTest);
+};
+
+IN_PROC_BROWSER_TEST_F(SAMLPolicyViewsBasedLoginTest,
+                       PRE_TestLoginMediaPermission) {
+  // Mark OOBE completed to go directly to the sign-in screen - this is
+  // currently needed to trigger the views-based login UI.
+  StartupUtils::MarkOobeCompleted();
+}
+
 // Ensure that the permission status of getUserMedia requests from SAML login
 // pages is controlled by the kLoginVideoCaptureAllowedUrls pref rather than the
 // underlying user content setting.
-IN_PROC_BROWSER_TEST_F(SAMLPolicyTest, TestLoginMediaPermission) {
+IN_PROC_BROWSER_TEST_F(SAMLPolicyViewsBasedLoginTest,
+                       TestLoginMediaPermission) {
+  EXPECT_TRUE(ash::features::IsViewsLoginEnabled());
   fake_saml_idp()->SetLoginHTMLTemplate("saml_login.html");
 
   const GURL url1("https://google.com");
@@ -1510,28 +1538,22 @@ IN_PROC_BROWSER_TEST_F(SAMLPolicyTest, TestLoginMediaPermission) {
   WaitForSigninScreen();
 
   content::WebContents* web_contents = GetLoginUI()->GetWebContents();
+  content::WebContentsDelegate* web_contents_delegate =
+      web_contents->GetDelegate();
 
   // Mic should always be blocked.
-  EXPECT_FALSE(
-      MediaCaptureDevicesDispatcher::GetInstance()->CheckMediaAccessPermission(
-          web_contents->GetMainFrame(), url1,
-          content::MEDIA_DEVICE_AUDIO_CAPTURE));
+  EXPECT_FALSE(web_contents_delegate->CheckMediaAccessPermission(
+      web_contents->GetMainFrame(), url1, content::MEDIA_DEVICE_AUDIO_CAPTURE));
 
   // Camera should be allowed if allowed by the whitelist, otherwise blocked.
-  EXPECT_TRUE(
-      MediaCaptureDevicesDispatcher::GetInstance()->CheckMediaAccessPermission(
-          web_contents->GetMainFrame(), url1,
-          content::MEDIA_DEVICE_VIDEO_CAPTURE));
+  EXPECT_TRUE(web_contents_delegate->CheckMediaAccessPermission(
+      web_contents->GetMainFrame(), url1, content::MEDIA_DEVICE_VIDEO_CAPTURE));
 
-  EXPECT_TRUE(
-      MediaCaptureDevicesDispatcher::GetInstance()->CheckMediaAccessPermission(
-          web_contents->GetMainFrame(), url2,
-          content::MEDIA_DEVICE_VIDEO_CAPTURE));
+  EXPECT_TRUE(web_contents_delegate->CheckMediaAccessPermission(
+      web_contents->GetMainFrame(), url2, content::MEDIA_DEVICE_VIDEO_CAPTURE));
 
-  EXPECT_FALSE(
-      MediaCaptureDevicesDispatcher::GetInstance()->CheckMediaAccessPermission(
-          web_contents->GetMainFrame(), url3,
-          content::MEDIA_DEVICE_VIDEO_CAPTURE));
+  EXPECT_FALSE(web_contents_delegate->CheckMediaAccessPermission(
+      web_contents->GetMainFrame(), url3, content::MEDIA_DEVICE_VIDEO_CAPTURE));
 
   // Camera should be blocked in the login screen, even if it's allowed via
   // content setting.
@@ -1542,10 +1564,8 @@ IN_PROC_BROWSER_TEST_F(SAMLPolicyTest, TestLoginMediaPermission) {
                                       CONTENT_SETTINGS_TYPE_MEDIASTREAM_CAMERA,
                                       std::string(), CONTENT_SETTING_ALLOW);
 
-  EXPECT_FALSE(
-      MediaCaptureDevicesDispatcher::GetInstance()->CheckMediaAccessPermission(
-          web_contents->GetMainFrame(), url3,
-          content::MEDIA_DEVICE_VIDEO_CAPTURE));
+  EXPECT_FALSE(web_contents_delegate->CheckMediaAccessPermission(
+      web_contents->GetMainFrame(), url3, content::MEDIA_DEVICE_VIDEO_CAPTURE));
 }
 
 }  // namespace chromeos

chrome/browser/chromeos/login/test/oobe_base_test.cc

@@ -142,7 +142,8 @@ void OobeBaseTest::TearDownOnMainThread() {
 void OobeBaseTest::SetUpCommandLine(base::CommandLine* command_line) {
   extensions::ExtensionApiTest::SetUpCommandLine(command_line);
 
-  command_line->AppendSwitch(ash::switches::kShowWebUiLogin);
+  if (ShouldForceWebUiLogin())
+    command_line->AppendSwitch(ash::switches::kShowWebUiLogin);
   command_line->AppendSwitch(chromeos::switches::kLoginManager);
   command_line->AppendSwitch(chromeos::switches::kForceLoginManagerInTests);
   if (!needs_background_networking_)
@@ -163,6 +164,10 @@ void OobeBaseTest::InitHttpsForwarders() {
       kGAIAHost, embedded_test_server()->base_url()));
 }
 
+bool OobeBaseTest::ShouldForceWebUiLogin() {
+  return true;
+}
+
 void OobeBaseTest::SimulateNetworkOffline() {
   NetworkPortalDetector::CaptivePortalState offline_state;
   offline_state.status = NetworkPortalDetector::CAPTIVE_PORTAL_STATUS_OFFLINE;

chrome/browser/chromeos/login/test/oobe_base_test.h

@@ -62,6 +62,12 @@ class OobeBaseTest : public extensions::ExtensionApiTest {
 
   virtual void InitHttpsForwarders();
 
+  // If this returns true (default), the |ash::switches::kShowWebUiLogin|
+  // command-line switch is passed to force the Web Ui Login.
+  // If this returns false, the switch is omitted so the views-based login may
+  // be used.
+  virtual bool ShouldForceWebUiLogin();
+
   // Network status control functions.
   void SimulateNetworkOffline();
   void SimulateNetworkOnline();

chrome/browser/chromeos/login/ui/oobe_ui_dialog_delegate.cc

@@ -22,7 +22,9 @@
 #include "ui/base/ui_base_features.h"
 #include "ui/display/display.h"
 #include "ui/display/screen.h"
+#include "ui/views/controls/webview/unhandled_keyboard_event_handler.h"
 #include "ui/views/controls/webview/web_dialog_view.h"
+#include "ui/views/focus/focus_manager.h"
 #include "ui/views/widget/widget.h"
 
 namespace chromeos {
@@ -44,7 +46,7 @@ class OobeWebDialogView : public views::WebDialogView {
                     WebContentsHandler* handler)
       : views::WebDialogView(context, delegate, handler) {}
 
-  // views::WebDialogView:
+  // content::WebContentsDelegate:
   void RequestMediaAccessPermission(
       content::WebContents* web_contents,
       const content::MediaStreamRequest& request,
@@ -60,6 +62,23 @@ class OobeWebDialogView : public views::WebDialogView {
     return MediaCaptureDevicesDispatcher::GetInstance()
         ->CheckMediaAccessPermission(render_frame_host, security_origin, type);
   }
+
+  bool TakeFocus(content::WebContents* source, bool reverse) override {
+    LoginScreenClient::Get()->login_screen()->FocusLoginShelf(reverse);
+    return true;
+  }
+
+  bool HandleKeyboardEvent(
+      content::WebContents* source,
+      const content::NativeWebKeyboardEvent& event) override {
+    return unhandled_keyboard_event_handler_.HandleKeyboardEvent(
+        event, GetFocusManager());
+  }
+
+ private:
+  views::UnhandledKeyboardEventHandler unhandled_keyboard_event_handler_;
+
+  DISALLOW_COPY_AND_ASSIGN(OobeWebDialogView);
 };
 
 class CaptivePortalDialogDelegate
@@ -201,8 +220,6 @@ OobeUIDialogDelegate::OobeUIDialogDelegate(
   extensions::ChromeExtensionWebContentsObserver::CreateForWebContents(
       dialog_view_->web_contents());
 
-  dialog_view_->web_contents()->SetDelegate(this);
-
   captive_portal_delegate_ = new CaptivePortalDialogDelegate(dialog_view_);
 
   GetOobeUI()->GetErrorScreen()->MaybeInitCaptivePortalWindowProxy(
@@ -315,19 +332,6 @@ gfx::NativeWindow OobeUIDialogDelegate::GetNativeWindow() const {
   return dialog_widget_ ? dialog_widget_->GetNativeWindow() : nullptr;
 }
 
-bool OobeUIDialogDelegate::TakeFocus(content::WebContents* source,
-                                     bool reverse) {
-  LoginScreenClient::Get()->login_screen()->FocusLoginShelf(reverse);
-  return true;
-}
-
-bool OobeUIDialogDelegate::HandleKeyboardEvent(
-    content::WebContents* source,
-    const content::NativeWebKeyboardEvent& event) {
-  return unhandled_keyboard_event_handler_.HandleKeyboardEvent(
-      event, dialog_widget_->GetFocusManager());
-}
-
 void OobeUIDialogDelegate::OnDisplayMetricsChanged(
     const display::Display& display,
     uint32_t changed_metrics) {

chrome/browser/chromeos/login/ui/oobe_ui_dialog_delegate.h

@@ -17,9 +17,7 @@
 #include "chrome/browser/ui/ash/tablet_mode_client_observer.h"
 #include "chrome/browser/ui/chrome_web_modal_dialog_manager_delegate.h"
 #include "components/web_modal/web_contents_modal_dialog_host.h"
-#include "content/public/browser/web_contents_delegate.h"
 #include "ui/display/display_observer.h"
-#include "ui/views/controls/webview/unhandled_keyboard_event_handler.h"
 #include "ui/web_dialogs/web_dialog_delegate.h"
 
 class TabletModeClient;
@@ -58,7 +56,6 @@ class CaptivePortalDialogDelegate;
 class OobeUIDialogDelegate : public display::DisplayObserver,
                              public TabletModeClientObserver,
                              public ui::WebDialogDelegate,
-                             public content::WebContentsDelegate,
                              public ChromeKeyboardControllerClient::Observer,
                              public CaptivePortalWindowProxy::Observer {
  public:
@@ -92,12 +89,6 @@ class OobeUIDialogDelegate : public display::DisplayObserver,
   OobeUI* GetOobeUI() const;
   gfx::NativeWindow GetNativeWindow() const;
 
-  // content::WebContentsDelegate:
-  bool TakeFocus(content::WebContents* source, bool reverse) override;
-  bool HandleKeyboardEvent(
-      content::WebContents* source,
-      const content::NativeWebKeyboardEvent& event) override;
-
  private:
   // display::DisplayObserver:
   void OnDisplayMetricsChanged(const display::Display& display,
@@ -154,8 +145,6 @@ class OobeUIDialogDelegate : public display::DisplayObserver,
   std::map<ui::Accelerator, std::string> accel_map_;
   ash::mojom::OobeDialogState state_ = ash::mojom::OobeDialogState::HIDDEN;
 
-  views::UnhandledKeyboardEventHandler unhandled_keyboard_event_handler_;
-
   // Whether the captive portal screen should be shown the next time the Gaia
   // dialog is opened.
   bool should_display_captive_portal_ = false;