desktop-pwas: Set/unset app-specific prefs in TabInsertedAt/TabRemovedAt

WebContents in app windows require certain prefs to be set. This
happened in OpenApplication, but there are multiple ways for a
WebContents to end up in an app window and also a WebContents that
started in an app window could later move to regular tab. So, in some
cases, a WebContents could end up in an app window without the necessary
prefs or a WebContents could end up in a regular tab with app prefs set.

Before, when an app window was opened through OpenApplication,
OpenApplication would set app-specific prefs in the new app's
WebContents. Now, HostedAppBrowserController sets the prefs when the
WebContents is added to the tab strip. Before, nothing would unset the
app-specific prefs when a WebContents was moved from an app window.
Now, HostedAppBrowserController unsets the prefs when the WebContents
is detached.

Bug: 808901
Change-Id: I6b21e7e9090133755d8e45de7a216e4c5a5e1379
Reviewed-on: https://chromium-review.googlesource.com/958790
Commit-Queue: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544293}

chrome/browser/ui/extensions/application_launch.cc

@@ -29,13 +29,12 @@
 #include "chrome/browser/ui/extensions/app_launch_params.h"
 #include "chrome/browser/ui/extensions/extension_enable_flow.h"
 #include "chrome/browser/ui/extensions/extension_enable_flow_delegate.h"
+#include "chrome/browser/ui/extensions/hosted_app_browser_controller.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
 #include "chrome/browser/web_applications/web_app.h"
 #include "chrome/common/extensions/manifest_handlers/app_launch_info.h"
 #include "chrome/common/url_constants.h"
-#include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
-#include "content/public/common/renderer_preferences.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/browser/extension_system.h"
@@ -219,8 +218,8 @@ WebContents* OpenApplicationWindow(const AppLaunchParams& params,
   Navigate(&nav_params);
 
   WebContents* web_contents = nav_params.target_contents;
-  web_contents->GetMutableRendererPrefs()->can_accept_load_drops = false;
-  web_contents->GetRenderViewHost()->SyncRendererPrefs();
+  extensions::HostedAppBrowserController::SetAppPrefsForWebContents(
+      web_contents);
 
   browser->window()->Show();
 

chrome/browser/ui/extensions/hosted_app_browser_controller.cc

@@ -22,7 +22,9 @@
 #include "components/url_formatter/url_formatter.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/navigation_entry.h"
+#include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/renderer_preferences.h"
 #include "extensions/browser/extension_registry.h"
 #include "extensions/common/extension.h"
 #include "ui/gfx/favicon_size.h"
@@ -81,6 +83,14 @@ bool HostedAppBrowserController::IsForExperimentalHostedAppBrowser(
 }
 
 // static
+void HostedAppBrowserController::SetAppPrefsForWebContents(
+    content::WebContents* web_contents) {
+  auto* rvh = web_contents->GetRenderViewHost();
+
+  web_contents->GetMutableRendererPrefs()->can_accept_load_drops = false;
+  rvh->SyncRendererPrefs();
+}
+
 base::string16 HostedAppBrowserController::FormatUrlOrigin(const GURL& url) {
   return url_formatter::FormatUrl(
       url.GetOrigin(),
@@ -96,9 +106,13 @@ HostedAppBrowserController::HostedAppBrowserController(Browser* browser)
     : SiteEngagementObserver(SiteEngagementService::Get(browser->profile())),
       browser_(browser),
       extension_id_(
-          web_app::GetExtensionIdFromApplicationName(browser->app_name())) {}
+          web_app::GetExtensionIdFromApplicationName(browser->app_name())) {
+  browser_->tab_strip_model()->AddObserver(this);
+}
 
-HostedAppBrowserController::~HostedAppBrowserController() {}
+HostedAppBrowserController::~HostedAppBrowserController() {
+  browser_->tab_strip_model()->RemoveObserver(this);
+}
 
 bool HostedAppBrowserController::IsForInstalledPwa(
     content::WebContents* web_contents) const {
@@ -228,4 +242,19 @@ void HostedAppBrowserController::OnEngagementEvent(
                             SiteEngagementService::ENGAGEMENT_LAST);
 }
 
+void HostedAppBrowserController::TabInsertedAt(TabStripModel* tab_strip_model,
+                                               content::WebContents* contents,
+                                               int index,
+                                               bool foreground) {
+  HostedAppBrowserController::SetAppPrefsForWebContents(contents);
+}
+
+void HostedAppBrowserController::TabDetachedAt(content::WebContents* contents,
+                                               int index) {
+  auto* rvh = contents->GetRenderViewHost();
+
+  contents->GetMutableRendererPrefs()->can_accept_load_drops = true;
+  rvh->SyncRendererPrefs();
+}
+
 }  // namespace extensions

chrome/browser/ui/extensions/hosted_app_browser_controller.h

@@ -11,6 +11,7 @@
 #include "base/optional.h"
 #include "base/strings/string16.h"
 #include "chrome/browser/engagement/site_engagement_observer.h"
+#include "chrome/browser/ui/tabs/tab_strip_model_observer.h"
 #include "third_party/skia/include/core/SkColor.h"
 
 class Browser;
@@ -26,7 +27,8 @@ extern const char kPwaWindowEngagementTypeHistogram[];
 class Extension;
 
 // Class to encapsulate logic to control the browser UI for hosted apps.
-class HostedAppBrowserController : public SiteEngagementObserver {
+class HostedAppBrowserController : public SiteEngagementObserver,
+                                   public TabStripModelObserver {
  public:
   // Indicates whether |browser| is a hosted app browser.
   static bool IsForHostedApp(const Browser* browser);
@@ -34,6 +36,9 @@ class HostedAppBrowserController : public SiteEngagementObserver {
   // Returns whether |browser| uses the experimental hosted app experience.
   static bool IsForExperimentalHostedAppBrowser(const Browser* browser);
 
+  // Functions to set preferences that are unique to app windows.
+  static void SetAppPrefsForWebContents(content::WebContents* web_contents);
+
   // Renders |url|'s origin as Unicode.
   static base::string16 FormatUrlOrigin(const GURL& url);
 
@@ -80,6 +85,13 @@ class HostedAppBrowserController : public SiteEngagementObserver {
                          double score,
                          SiteEngagementService::EngagementType type) override;
 
+  // TabStripModelObserver overrides.
+  void TabInsertedAt(TabStripModel* tab_strip_model,
+                     content::WebContents* contents,
+                     int index,
+                     bool foreground) override;
+  void TabDetachedAt(content::WebContents* contents, int index) override;
+
  private:
   Browser* const browser_;
   const std::string extension_id_;

chrome/browser/ui/extensions/hosted_app_browsertest.cc

@@ -36,6 +36,7 @@
 #include "content/public/common/content_features.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/context_menu_params.h"
+#include "content/public/common/renderer_preferences.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "content/public/test/test_utils.h"
@@ -99,6 +100,16 @@ void NavigateAndCheckForLocationBar(Browser* browser,
       browser->hosted_app_controller()->ShouldShowLocationBar());
 }
 
+void CheckWebContentsHasAppPrefs(content::WebContents* web_contents) {
+  content::RendererPreferences* prefs = web_contents->GetMutableRendererPrefs();
+  EXPECT_FALSE(prefs->can_accept_load_drops);
+}
+
+void CheckWebContentsDoesNotHaveAppPrefs(content::WebContents* web_contents) {
+  content::RendererPreferences* prefs = web_contents->GetMutableRendererPrefs();
+  EXPECT_TRUE(prefs->can_accept_load_drops);
+}
+
 }  // namespace
 
 // Parameters are {app_type, desktop_pwa_flag}. |app_type| controls whether it
@@ -108,7 +119,7 @@ class HostedAppTest
     : public ExtensionBrowserTest,
       public ::testing::WithParamInterface<std::tuple<AppType, bool>> {
  public:
-  HostedAppTest() : app_browser_(nullptr) {}
+  HostedAppTest() : app_browser_(nullptr), app_(nullptr) {}
   ~HostedAppTest() override {}
 
   void SetUp() override {
@@ -131,15 +142,15 @@ class HostedAppTest
   }
 
   void SetupApp(const base::FilePath& app_folder) {
-    const Extension* app = InstallExtensionWithSourceAndFlags(
+    app_ = InstallExtensionWithSourceAndFlags(
         app_folder, 1, extensions::Manifest::INTERNAL,
         app_type_ == AppType::BOOKMARK_APP
             ? extensions::Extension::FROM_BOOKMARK
             : extensions::Extension::NO_FLAGS);
-    ASSERT_TRUE(app);
+    ASSERT_TRUE(app_);
 
     // Launch it in a window.
-    app_browser_ = LaunchAppBrowser(app);
+    app_browser_ = LaunchAppBrowser(app_);
     ASSERT_TRUE(app_browser_);
     ASSERT_TRUE(app_browser_ != browser());
   }
@@ -173,6 +184,7 @@ class HostedAppTest
   }
 
   Browser* app_browser_;
+  const extensions::Extension* app_;
 
   AppType app_type() const { return app_type_; }
 
@@ -246,6 +258,46 @@ IN_PROC_BROWSER_TEST_P(HostedAppTest, CtrlClickLink) {
       url);
 }
 
+// Tests that the WebContents of an app window launched using OpenApplication
+// has the correct prefs.
+IN_PROC_BROWSER_TEST_P(HostedAppTest, WebContentsPrefsOpenApplication) {
+  SetupApp("https_app");
+  CheckWebContentsHasAppPrefs(
+      app_browser_->tab_strip_model()->GetActiveWebContents());
+}
+
+// Tests that the WebContents of an app window launched using
+// ReparentWebContentsIntoAppBrowser has the correct prefs.
+IN_PROC_BROWSER_TEST_P(HostedAppTest, WebContentsPrefsReparentWebContents) {
+  SetupApp("https_app");
+
+  content::WebContents* current_tab =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  CheckWebContentsDoesNotHaveAppPrefs(current_tab);
+
+  ReparentWebContentsIntoAppBrowser(current_tab, app_);
+  ASSERT_NE(browser(), chrome::FindLastActive());
+
+  CheckWebContentsHasAppPrefs(
+      chrome::FindLastActive()->tab_strip_model()->GetActiveWebContents());
+}
+
+// Tests that the WebContents of a regular browser window launched using
+// OpenInChrome has the correct prefs.
+IN_PROC_BROWSER_TEST_P(HostedAppTest, WebContentsPrefsOpenInChrome) {
+  SetupApp("https_app");
+
+  content::WebContents* app_contents =
+      app_browser_->tab_strip_model()->GetActiveWebContents();
+  CheckWebContentsHasAppPrefs(app_contents);
+
+  chrome::OpenInChrome(app_browser_);
+  ASSERT_EQ(browser(), chrome::FindLastActive());
+
+  CheckWebContentsDoesNotHaveAppPrefs(
+      browser()->tab_strip_model()->GetActiveWebContents());
+}
+
 // Check that the location bar is shown correctly.
 IN_PROC_BROWSER_TEST_P(HostedAppTest, ShouldShowLocationBar) {
   SetupApp("https_app");