Simplify ZoomController by removing DefaultZoomLevel prefs machinery.

In preparation for moving to multiple HostZoomMaps (one-per
StoragePartition), it is helpful to simplify ZoomController by removing
the prefs machinery for DefaultZoomLevel, and instead rely on getting
this value from the HostZoomMap.

The calls to ZoomController::UpdateState() that used to be triggered
directly by the prefs changes still occur, though by a roundtrip from
PrefsTabHelper via IPC to RenderViewImpl and then by IPC back to
the HostZoomMap, which in turn notifies its observers. This only happens
in cases where the change in DefaultZoomLevel actually causes the
RenderViewImpl to change its zoom, so it is a subtle change in behaviour
from before.

BUG=335317

Review URL: https://codereview.chromium.org/508263002

Cr-Commit-Position: refs/heads/master@{#292920}

chrome/browser/ui/zoom/zoom_controller.cc

@@ -4,17 +4,10 @@
 
 #include "chrome/browser/ui/zoom/zoom_controller.h"
 
-#include "base/prefs/pref_service.h"
-#include "chrome/browser/chrome_notification_types.h"
-#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/zoom/zoom_event_manager.h"
 #include "chrome/browser/ui/zoom/zoom_observer.h"
-#include "chrome/common/pref_names.h"
 #include "content/public/browser/host_zoom_map.h"
 #include "content/public/browser/navigation_entry.h"
-#include "content/public/browser/notification_details.h"
-#include "content/public/browser/notification_service.h"
-#include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
@@ -31,19 +24,14 @@ ZoomController::ZoomController(content::WebContents* web_contents)
       zoom_mode_(ZOOM_MODE_DEFAULT),
       zoom_level_(1.0),
       browser_context_(web_contents->GetBrowserContext()) {
-  Profile* profile =
-      Profile::FromBrowserContext(web_contents->GetBrowserContext());
-  default_zoom_level_.Init(
-      prefs::kDefaultZoomLevel,
-      profile->GetPrefs(),
-      base::Bind(
-          &ZoomController::UpdateState, base::Unretained(this), std::string()));
-  zoom_level_ = default_zoom_level_.GetValue();
-
-  zoom_subscription_ = content::HostZoomMap::GetDefaultForBrowserContext(
-      browser_context_)->AddZoomLevelChangedCallback(
-          base::Bind(&ZoomController::OnZoomLevelChanged,
-                     base::Unretained(this)));
+  // TODO(wjmaclean) Make calls to HostZoomMap::GetDefaultForBrowserContext()
+  // refer to the webcontents-specific HostZoomMap when that becomes available.
+  content::HostZoomMap* host_zoom_map =
+      content::HostZoomMap::GetDefaultForBrowserContext(browser_context_);
+  zoom_level_ = host_zoom_map->GetDefaultZoomLevel();
+
+  zoom_subscription_ = host_zoom_map->AddZoomLevelChangedCallback(
+      base::Bind(&ZoomController::OnZoomLevelChanged, base::Unretained(this)));
 
   UpdateState(std::string());
 }
@@ -51,15 +39,14 @@ ZoomController::ZoomController(content::WebContents* web_contents)
 ZoomController::~ZoomController() {}
 
 bool ZoomController::IsAtDefaultZoom() const {
-  return content::ZoomValuesEqual(GetZoomLevel(),
-                                  default_zoom_level_.GetValue());
+  return content::ZoomValuesEqual(GetZoomLevel(), GetDefaultZoomLevel());
 }
 
 int ZoomController::GetResourceForZoomLevel() const {
   if (IsAtDefaultZoom())
     return IDR_ZOOM_NORMAL;
-  return GetZoomLevel() > default_zoom_level_.GetValue() ? IDR_ZOOM_PLUS
-                                                         : IDR_ZOOM_MINUS;
+  return GetZoomLevel() > GetDefaultZoomLevel() ? IDR_ZOOM_PLUS
+                                                : IDR_ZOOM_MINUS;
 }
 
 void ZoomController::AddObserver(ZoomObserver* observer) {
@@ -227,7 +214,7 @@ void ZoomController::SetZoomMode(ZoomMode new_mode) {
       // the zoom level is handled independently.
       if (zoom_mode_ != ZOOM_MODE_DISABLED) {
         zoom_map->SetTemporaryZoomLevel(
-            render_process_id, render_view_id, default_zoom_level_.GetValue());
+            render_process_id, render_view_id, GetDefaultZoomLevel());
         zoom_level_ = original_zoom_level;
       } else {
         // When we don't call any HostZoomMap set functions, we send the event
@@ -241,7 +228,7 @@ void ZoomController::SetZoomMode(ZoomMode new_mode) {
     case ZOOM_MODE_DISABLED: {
       // The page needs to be zoomed back to default before disabling the zoom
       zoom_map->SetTemporaryZoomLevel(
-          render_process_id, render_view_id, default_zoom_level_.GetValue());
+          render_process_id, render_view_id, GetDefaultZoomLevel());
       break;
     }
   }

chrome/browser/ui/zoom/zoom_controller.h

@@ -70,6 +70,15 @@ class ZoomController : public content::WebContentsObserver,
 
   ZoomMode zoom_mode() const { return zoom_mode_; }
 
+  // Convenience method to get default zoom level. Implemented here for
+  // inlining.
+  double GetDefaultZoomLevel() const {
+    // TODO(wjmaclean) Make this refer to the webcontents-specific HostZoomMap
+    // when that becomes available.
+    return content::HostZoomMap::GetDefaultForBrowserContext(browser_context_)->
+        GetDefaultZoomLevel();
+  }
+
   // Convenience method to quickly check if the tab's at default zoom.
   bool IsAtDefaultZoom() const;
 
@@ -142,9 +151,6 @@ class ZoomController : public content::WebContentsObserver,
   // Current zoom level.
   double zoom_level_;
 
-  // Used to access the default zoom level preference.
-  DoublePrefMember default_zoom_level_;
-
   scoped_ptr<ZoomChangedEventData> event_data_;
 
   // Keeps track of the extension (if any) that initiated the last zoom change

chrome/browser/ui/zoom/zoom_controller_browsertest.cc

@@ -4,17 +4,58 @@
 
 #include "chrome/browser/ui/zoom/zoom_controller.h"
 
+#include "base/prefs/pref_service.h"
 #include "base/process/kill.h"
+#include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/tabs/tab_strip_model.h"
+#include "chrome/common/pref_names.h"
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test_utils.h"
+#include "testing/gmock/include/gmock/gmock.h"
 
 typedef InProcessBrowserTest ZoomControllerBrowserTest;
 
+bool operator==(const ZoomController::ZoomChangedEventData& lhs,
+                const ZoomController::ZoomChangedEventData& rhs) {
+  return lhs.web_contents == rhs.web_contents &&
+         lhs.old_zoom_level == rhs.old_zoom_level &&
+         lhs.new_zoom_level == rhs.new_zoom_level &&
+         lhs.zoom_mode == rhs.zoom_mode &&
+         lhs.can_show_bubble == rhs.can_show_bubble;
+}
+
+class ZoomChangedWatcher : public ZoomObserver {
+ public:
+  ZoomChangedWatcher(
+      content::WebContents* web_contents,
+      const ZoomController::ZoomChangedEventData& expected_event_data)
+      : web_contents_(web_contents),
+        expected_event_data_(expected_event_data),
+        message_loop_runner_(new content::MessageLoopRunner) {
+    ZoomController::FromWebContents(web_contents)->AddObserver(this);
+  }
+  virtual ~ZoomChangedWatcher() {}
+
+  void Wait() { message_loop_runner_->Run(); }
+
+  virtual void OnZoomChanged(
+      const ZoomController::ZoomChangedEventData& event_data) OVERRIDE {
+    if (event_data == expected_event_data_)
+      message_loop_runner_->Quit();
+  }
+
+ private:
+  content::WebContents* web_contents_;
+  ZoomController::ZoomChangedEventData expected_event_data_;
+  scoped_refptr<content::MessageLoopRunner> message_loop_runner_;
+
+  DISALLOW_COPY_AND_ASSIGN(ZoomChangedWatcher);
+};
+
 // TODO(wjmaclean): Enable this on Android when we can kill the process there.
 #if defined(OS_ANDROID)
 #define MAYBE_CrashedTabsDoNotChangeZoom DISABLED_CrashedTabsDoNotChangeZoom
@@ -47,3 +88,25 @@ IN_PROC_BROWSER_TEST_F(ZoomControllerBrowserTest,
   zoom_controller->SetZoomLevel(new_zoom_level);
   EXPECT_FLOAT_EQ(old_zoom_level, zoom_controller->GetZoomLevel());
 }
+
+IN_PROC_BROWSER_TEST_F(ZoomControllerBrowserTest, OnPreferenceChanged) {
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  double new_default_zoom_level = 1.0;
+  // Since this page uses the default zoom level, the changes to the default
+  // zoom level will change the zoom level for this web_contents.
+  ZoomController::ZoomChangedEventData zoom_change_data(
+      web_contents,
+      new_default_zoom_level,
+      new_default_zoom_level,
+      ZoomController::ZOOM_MODE_DEFAULT,
+      true);
+  ZoomChangedWatcher zoom_change_watcher(web_contents, zoom_change_data);
+  // TODO(wjmaclean): Convert this to call partition-specific zoom level prefs
+  // when they become available.
+  browser()->profile()->GetPrefs()->SetDouble(prefs::kDefaultZoomLevel,
+                                              new_default_zoom_level);
+  // Because this test relies on a round-trip IPC to/from the renderer process,
+  // we need to wait for it to propagate.
+  zoom_change_watcher.Wait();
+}

chrome/browser/ui/zoom/zoom_controller_unittest.cc

@@ -65,23 +65,6 @@ TEST_F(ZoomControllerTest, DidNavigateMainFrame) {
                                          content::FrameNavigateParams());
 }
 
-TEST_F(ZoomControllerTest, OnPreferenceChanged) {
-  double zoom_level = zoom_controller_->GetZoomLevel();
-  // Note that while the change in the default zoom level triggers an event,
-  // the current zoom level for this web contents does not change since the
-  // default zoom level in HostZoomMap is not updated.
-  // TODO(wjmaclean) Make sure changes to the default zoom level in preferences
-  // propagate to HostZoomMap. http://crbug.com/391484
-  ZoomController::ZoomChangedEventData zoom_change_data(
-      web_contents(),
-      zoom_level,
-      zoom_level,
-      ZoomController::ZOOM_MODE_DEFAULT,
-      false);
-  EXPECT_CALL(zoom_observer_, OnZoomChanged(zoom_change_data)).Times(1);
-  profile()->GetPrefs()->SetDouble(prefs::kDefaultZoomLevel, 110.0);
-}
-
 TEST_F(ZoomControllerTest, Observe) {
   double new_zoom_level = 110.0;
   // When the event is initiated from HostZoomMap, the old zoom level is not