Correctly track page scale changes due to navigation in the browser

In the browser process, we only consider page scale changes that the
renderer notifies the browser of. This leads to the following issue:
1) The user performs a pinch zoom, changing the page scale factor of
a page. The browser is notified of this.
2) A navigation to a new page happens. The browser still thinks the
WebContents is scaled.
3) A history navigation back to the original scaled page happens. Since
the browser never realized that the WebContents was on an unscaled page,
the browser discards the update from the renderer.
The HostZoomMap now lacks the page scale information for the first
page, and the browser will ignore attempts to reset the scale
(e.g. using Ctrl-0).

We now store page scale change notifications from the renderer in
content::PageImpl instead of manually tracking this in HostZoomMap
and per-WebContents state.

Note that the renderer persists page scale via history entry state,
which is a separate mechanism from this one where we want to know
the current page scale.

Bug: 1264958
Change-Id: I27260afba518954e87b2aff1027e3f06515435ad
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3258065
Commit-Queue: Kevin McNee <mcnee@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Mihai Sardarescu <msarda@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Reviewed-by: W. James MacLean <wjmaclean@chromium.org>
Cr-Commit-Position: refs/heads/main@{#940002}

chrome/browser/profiles/host_zoom_map_browsertest.cc

@@ -405,30 +405,3 @@ IN_PROC_BROWSER_TEST_F(HostZoomMapBrowserTest,
       new_default_zoom_level);
   EXPECT_EQ(new_default_zoom_level, child_host_zoom_map->GetDefaultZoomLevel());
 }
-
-// TODO(1115597): Flaky on linux and cros.
-#if defined(OS_LINUX) || defined(OS_CHROMEOS)
-#define MAYBE_PageScaleIsOneChanged DISABLED_PageScaleIsOneChanged
-#else
-#define MAYBE_PageScaleIsOneChanged PageScaleIsOneChanged
-#endif
-IN_PROC_BROWSER_TEST_F(HostZoomMapBrowserTest, MAYBE_PageScaleIsOneChanged) {
-  GURL test_url(url::kAboutBlankURL);
-  std::string test_host(test_url.host());
-
-  ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), test_url));
-  content::WebContents* web_contents =
-      browser()->tab_strip_model()->GetActiveWebContents();
-
-  ASSERT_TRUE(content::HostZoomMap::PageScaleFactorIsOne(web_contents));
-
-  ZoomLevelChangeObserver observer(browser()->profile());
-
-  web_contents->SetPageScale(1.5);
-  observer.BlockUntilZoomLevelForHostHasChanged(test_host);
-  EXPECT_FALSE(content::HostZoomMap::PageScaleFactorIsOne(web_contents));
-
-  web_contents->SetPageScale(1.f);
-  observer.BlockUntilZoomLevelForHostHasChanged(test_host);
-  EXPECT_TRUE(content::HostZoomMap::PageScaleFactorIsOne(web_contents));
-}

chrome/browser/profiles/off_the_record_profile_impl.cc

@@ -642,8 +642,6 @@ void OffTheRecordProfileImpl::OnParentZoomLevelChanged(
           change.host,
           change.zoom_level);
       return;
-    case HostZoomMap::PAGE_SCALE_IS_ONE_CHANGED:
-      return;
   }
 }
 

chrome/browser/ui/webui/settings/site_settings_handler.cc

@@ -1355,8 +1355,6 @@ void SiteSettingsHandler::SendZoomLevels() {
         // These are not stored in preferences and get cleared on next browser
         // start. Therefore, we don't care for them.
         continue;
-      case content::HostZoomMap::PAGE_SCALE_IS_ONE_CHANGED:
-        continue;
       case content::HostZoomMap::ZOOM_CHANGED_TEMPORARY_ZOOM:
         NOTREACHED();
     }

chrome/browser/ui/zoom/zoom_controller_browsertest.cc

@@ -6,8 +6,10 @@
 
 #include "base/macros.h"
 #include "base/process/kill.h"
+#include "base/test/bind.h"
 #include "build/build_config.h"
 #include "build/chromeos_buildflags.h"
+#include "chrome/app/chrome_command_ids.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_commands.h"
@@ -20,11 +22,13 @@
 #include "components/prefs/pref_service.h"
 #include "components/zoom/test/zoom_test_utils.h"
 #include "components/zoom/zoom_observer.h"
+#include "content/public/browser/disallow_activation_reason.h"
 #include "content/public/browser/host_zoom_map.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
+#include "content/public/browser/render_widget_host_view.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/page_type.h"
 #include "content/public/test/browser_test.h"
@@ -43,6 +47,12 @@ class ZoomControllerBrowserTest : public InProcessBrowserTest {
   ZoomControllerBrowserTest() {}
   ~ZoomControllerBrowserTest() override {}
 
+  void SetUpOnMainThread() override {
+    InProcessBrowserTest::SetUpOnMainThread();
+    host_resolver()->AddRule("a.com", "127.0.0.1");
+    host_resolver()->AddRule("b.com", "127.0.0.1");
+  }
+
   void TestResetOnNavigation(ZoomController::ZoomMode zoom_mode) {
     DCHECK(zoom_mode == ZoomController::ZOOM_MODE_ISOLATED ||
            zoom_mode == ZoomController::ZOOM_MODE_MANUAL);
@@ -250,6 +260,86 @@ IN_PROC_BROWSER_TEST_F(ZoomControllerBrowserTest, NavigationResetsManualMode) {
   TestResetOnNavigation(ZoomController::ZOOM_MODE_MANUAL);
 }
 
+// Mac does not have touchscreen pinch.
+#if !defined(OS_MAC)
+// Ensure that when a history navigation restores the page scale factor from a
+// previous pinch zoom, the browser is notified of the page scale restoration.
+IN_PROC_BROWSER_TEST_F(ZoomControllerBrowserTest,
+                       RestoredPageScaleFromNavigation) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+
+  const GURL url_a(embedded_test_server()->GetURL("a.com", "/title1.html"));
+  const GURL url_b(embedded_test_server()->GetURL("b.com", "/title2.html"));
+
+  content::RenderFrameHostWrapper rfh_a(
+      ui_test_utils::NavigateToURL(browser(), url_a));
+  ASSERT_TRUE(rfh_a);
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  zoom::ZoomController* zoom_controller =
+      zoom::ZoomController::FromWebContents(web_contents);
+  EXPECT_TRUE(zoom_controller->PageScaleFactorIsOne());
+  EXPECT_FALSE(chrome::CanResetZoom(web_contents));
+  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_ZOOM_NORMAL));
+
+  // Perform a pinch zoom to change the page scale factor.
+  // The anchor is not important for this test, but we can't have it near the
+  // edge of the contents, otherwise the simulated pinch's touch events wouldn't
+  // be within the contents' bounds.
+  const gfx::Rect contents_rect = web_contents->GetContainerBounds();
+  const gfx::PointF anchor(contents_rect.width() / 2,
+                           contents_rect.height() / 2);
+  const float scale_change = 1.5;
+  base::RunLoop run_loop;
+  content::SimulateTouchscreenPinch(web_contents, anchor, scale_change,
+                                    run_loop.QuitClosure());
+  run_loop.Run();
+
+  // The page scale factor propagates from the compositor thread to the main
+  // thread to the browser process, so we'll roundtrip before checking the page
+  // scale from the browser side in order to avoid flakiness.
+  base::RepeatingClosure synchronize_threads =
+      base::BindLambdaForTesting([web_contents]() {
+        content::MainThreadFrameObserver observer(
+            web_contents->GetRenderWidgetHostView()->GetRenderWidgetHost());
+        observer.Wait();
+      });
+
+  synchronize_threads.Run();
+  EXPECT_FALSE(zoom_controller->PageScaleFactorIsOne());
+  EXPECT_TRUE(chrome::CanResetZoom(web_contents));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_ZOOM_NORMAL));
+
+  // Navigate to a different page.
+  ASSERT_TRUE(ui_test_utils::NavigateToURL(browser(), url_b));
+
+  // If the previous page was bfcached, evict it, in order to test the
+  // conditions that were the cause of https://crbug.com/1264958 (the page scale
+  // needs to apply to a new RenderFrameHost).
+  if (rfh_a) {
+    ASSERT_TRUE(rfh_a->IsInactiveAndDisallowActivation(
+        content::DisallowActivationReasonId::kForTesting));
+  }
+  ASSERT_TRUE(rfh_a.WaitUntilRenderFrameDeleted());
+
+  synchronize_threads.Run();
+  EXPECT_TRUE(zoom_controller->PageScaleFactorIsOne());
+  EXPECT_FALSE(chrome::CanResetZoom(web_contents));
+  EXPECT_FALSE(chrome::IsCommandEnabled(browser(), IDC_ZOOM_NORMAL));
+
+  // Navigate to the previous page which was pinch zoomed. The page scale will
+  // be restored in the renderer and the browser should be made aware of this.
+  ASSERT_TRUE(web_contents->GetController().CanGoBack());
+  web_contents->GetController().GoBack();
+  ASSERT_TRUE(content::WaitForLoadStop(web_contents));
+
+  synchronize_threads.Run();
+  EXPECT_FALSE(zoom_controller->PageScaleFactorIsOne());
+  EXPECT_TRUE(chrome::CanResetZoom(web_contents));
+  EXPECT_TRUE(chrome::IsCommandEnabled(browser(), IDC_ZOOM_NORMAL));
+}
+#endif  // !defined(OS_MAC)
+
 #if !BUILDFLAG(IS_CHROMEOS_ASH)
 // Regression test: crbug.com/438979.
 IN_PROC_BROWSER_TEST_F(ZoomControllerBrowserTest,

components/zoom/zoom_controller.cc

@@ -330,6 +330,7 @@ void ZoomController::DidFinishNavigation(
   // zoom level from the old one.
   UpdateState(std::string());
   DCHECK(!event_data_);
+  last_page_scale_factor_was_one_ = PageScaleFactorIsOne();
 }
 
 void ZoomController::WebContentsDestroyed() {
@@ -355,6 +356,16 @@ void ZoomController::RenderFrameHostChanged(
           &ZoomController::OnZoomLevelChanged, base::Unretained(this)));
 }
 
+void ZoomController::OnPageScaleFactorChanged(float page_scale_factor) {
+  const bool is_one = page_scale_factor == 1.f;
+  if (is_one != last_page_scale_factor_was_one_) {
+    // We send a no-op zoom change to inform observers that PageScaleFactorIsOne
+    // has changed.
+    UpdateState(std::string());
+    last_page_scale_factor_was_one_ = is_one;
+  }
+}
+
 void ZoomController::OnZoomLevelChanged(
     const content::HostZoomMap::ZoomLevelChange& change) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ -404,19 +415,21 @@ void ZoomController::UpdateState(const std::string& host) {
 }
 
 void ZoomController::SetPageScaleFactorIsOneForTesting(bool is_one) {
-  int process_id = web_contents()
-                       ->GetMainFrame()
-                       ->GetRenderViewHost()
-                       ->GetProcess()
-                       ->GetID();
-  int view_id =
-      web_contents()->GetMainFrame()->GetRenderViewHost()->GetRoutingID();
-  host_zoom_map_->SetPageScaleFactorIsOneForView(process_id, view_id, is_one);
+  page_scale_factor_is_one_for_testing_ = is_one;
+
+  if (is_one != last_page_scale_factor_was_one_) {
+    // See OnPageScaleFactorChanged for why this is done.
+    UpdateState(std::string());
+    last_page_scale_factor_was_one_ = is_one;
+  }
 }
 
 bool ZoomController::PageScaleFactorIsOne() const {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  return content::HostZoomMap::PageScaleFactorIsOne(web_contents());
+  if (page_scale_factor_is_one_for_testing_.has_value())
+    return page_scale_factor_is_one_for_testing_.value();
+
+  return web_contents()->GetPrimaryPage().IsPageScaleFactorOne();
 }
 
 WEB_CONTENTS_USER_DATA_KEY_IMPL(ZoomController);

components/zoom/zoom_controller.h

@@ -15,6 +15,7 @@
 #include "content/public/browser/host_zoom_map.h"
 #include "content/public/browser/web_contents_observer.h"
 #include "content/public/browser/web_contents_user_data.h"
+#include "third_party/abseil-cpp/absl/types/optional.h"
 
 class ZoomControllerTest;
 
@@ -159,6 +160,7 @@ class ZoomController : public content::WebContentsObserver,
   void WebContentsDestroyed() override;
   void RenderFrameHostChanged(content::RenderFrameHost* old_host,
                               content::RenderFrameHost* new_host) override;
+  void OnPageScaleFactorChanged(float page_scale_factor) override;
 
  protected:
   // Protected for testing.
@@ -201,6 +203,13 @@ class ZoomController : public content::WebContentsObserver,
 
   base::CallbackListSubscription zoom_subscription_;
 
+  // Whether the page scale factor was one the last time we notified our
+  // observers of a change to PageScaleFactorIsOne.
+  bool last_page_scale_factor_was_one_ = true;
+
+  // If set, this value is returned in PageScaleFactorIsOne.
+  absl::optional<bool> page_scale_factor_is_one_for_testing_;
+
   WEB_CONTENTS_USER_DATA_KEY_DECL();
 };
 

content/browser/devtools/render_frame_devtools_agent_host.cc

@@ -276,7 +276,7 @@ void RenderFrameDevToolsAgentHost::SetFrameTreeNode(
                  ? WebContentsImpl::FromFrameTreeNode(frame_tree_node_)
                  : nullptr;
   if (wc)
-    page_scale_factor_ = wc->page_scale_factor();
+    page_scale_factor_ = wc->GetPrimaryPage().page_scale_factor();
   WebContentsObserver::Observe(wc);
 }
 

content/browser/host_zoom_map_impl.cc

@@ -104,14 +104,6 @@ double HostZoomMap::GetZoomLevel(WebContents* web_contents) {
       static_cast<WebContentsImpl*>(web_contents));
 }
 
-bool HostZoomMap::PageScaleFactorIsOne(WebContents* web_contents) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  HostZoomMapImpl* host_zoom_map = static_cast<HostZoomMapImpl*>(
-      HostZoomMap::GetForWebContents(web_contents));
-  return host_zoom_map->PageScaleFactorIsOneForWebContents(
-      static_cast<WebContentsImpl*>(web_contents));
-}
-
 void HostZoomMap::SetZoomLevel(WebContents* web_contents, double level) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   HostZoomMapImpl* host_zoom_map = static_cast<HostZoomMapImpl*>(
@@ -392,36 +384,6 @@ void HostZoomMapImpl::SetZoomLevelForWebContents(
   }
 }
 
-void HostZoomMapImpl::SetPageScaleFactorIsOneForView(int render_process_id,
-                                                     int render_view_id,
-                                                     bool is_one) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  view_page_scale_factors_are_one_[RenderViewKey(render_process_id,
-                                                 render_view_id)] = is_one;
-  HostZoomMap::ZoomLevelChange change;
-  change.mode = HostZoomMap::PAGE_SCALE_IS_ONE_CHANGED;
-  zoom_level_changed_callbacks_.Notify(change);
-}
-
-bool HostZoomMapImpl::PageScaleFactorIsOneForWebContents(
-    WebContentsImpl* web_contents_impl) const {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (!web_contents_impl->GetRenderViewHost()->GetProcess())
-    return true;
-
-  const auto it = view_page_scale_factors_are_one_.find(RenderViewKey(
-      web_contents_impl->GetRenderViewHost()->GetProcess()->GetID(),
-      web_contents_impl->GetRenderViewHost()->GetRoutingID()));
-  return it != view_page_scale_factors_are_one_.end() ? it->second : true;
-}
-
-void HostZoomMapImpl::ClearPageScaleFactorIsOneForView(int render_process_id,
-                                                       int render_view_id) {
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  view_page_scale_factors_are_one_.erase(
-      RenderViewKey(render_process_id, render_view_id));
-}
-
 bool HostZoomMapImpl::UsesTemporaryZoomLevel(int render_process_id,
                                              int render_view_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ -519,7 +481,6 @@ void HostZoomMapImpl::WillCloseRenderView(int render_process_id,
                                           int render_view_id) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   ClearTemporaryZoomLevel(render_process_id, render_view_id);
-  ClearPageScaleFactorIsOneForView(render_process_id, render_view_id);
 }
 
 HostZoomMapImpl::~HostZoomMapImpl() {
@@ -551,4 +512,4 @@ jdouble JNI_HostZoomMapImpl_GetZoomLevel(
 }
 #endif
 
-}  // namespace content
+}  // namespace content

content/browser/host_zoom_map_impl.h

@@ -30,10 +30,6 @@ class CONTENT_EXPORT HostZoomMapImpl : public HostZoomMap {
   ~HostZoomMapImpl() override;
 
   // HostZoomMap implementation:
-  void SetPageScaleFactorIsOneForView(
-      int render_process_id, int render_view_id, bool is_one) override;
-  void ClearPageScaleFactorIsOneForView(
-      int render_process_id, int render_view_id) override;
   void CopyFrom(HostZoomMap* copy) override;
   double GetZoomLevelForHostAndScheme(const std::string& scheme,
                                       const std::string& host) override;
@@ -65,9 +61,6 @@ class CONTENT_EXPORT HostZoomMapImpl : public HostZoomMap {
   // be a temporary zoom level, depending on UsesTemporaryZoomLevel().
   double GetZoomLevelForWebContents(WebContentsImpl* web_contents_impl);
 
-  bool PageScaleFactorIsOneForWebContents(
-      WebContentsImpl* web_contents_impl) const;
-
   // Sets the zoom level for this WebContents. If this WebContents is using
   // a temporary zoom level, then level is only applied to this WebContents.
   // Otherwise, the level will be applied on a host level.
@@ -107,7 +100,6 @@ class CONTENT_EXPORT HostZoomMapImpl : public HostZoomMap {
   };
 
   typedef std::map<RenderViewKey, double> TemporaryZoomLevels;
-  typedef std::map<RenderViewKey, bool> ViewPageScaleFactorsAreOne;
 
   double GetZoomLevelForHost(const std::string& host) const;
 
@@ -133,9 +125,6 @@ class CONTENT_EXPORT HostZoomMapImpl : public HostZoomMap {
   SchemeHostZoomLevels scheme_host_zoom_levels_;
   double default_zoom_level_;
 
-  // Page scale factor data for each renderer.
-  ViewPageScaleFactorsAreOne view_page_scale_factors_are_one_;
-
   TemporaryZoomLevels temporary_zoom_levels_;
 
   base::Clock* clock_;

content/browser/renderer_host/page_impl.cc

@@ -73,6 +73,10 @@ base::WeakPtr<Page> PageImpl::GetWeakPtr() {
   return weak_factory_.GetWeakPtr();
 }
 
+bool PageImpl::IsPageScaleFactorOne() {
+  return page_scale_factor_ == 1.f;
+}
+
 void PageImpl::OnFirstVisuallyNonEmptyPaint() {
   did_first_visually_non_empty_paint_ = true;
   delegate_.OnFirstVisuallyNonEmptyPaint(*this);