Introduce ChromeZoomLevelPref, make zoom level prefs independent of profile prefs.

chrome/browser/ui/zoom/chrome_zoom_level_prefs.cc

+// Copyright (c) 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/ui/zoom/chrome_zoom_level_prefs.h"
+
+#include <sstream>
+
+#include "base/bind.h"
+#include "base/prefs/json_pref_store.h"
+#include "base/prefs/pref_filter.h"
+#include "base/prefs/pref_registry_simple.h"
+#include "base/prefs/pref_service_factory.h"
+#include "base/prefs/scoped_user_pref_update.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/values.h"
+#include "chrome/common/chrome_constants.h"
+#include "chrome/common/pref_names.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/host_zoom_map.h"
+#include "content/public/common/page_zoom.h"
+
+namespace chrome {
+
+std::string ChromeZoomLevelPrefs::GetHash(const base::FilePath& relative_path) {
+  size_t int_key =
+#if defined(COMPILER_MSVC)
+      BASE_HASH_NAMESPACE::hash_value(relative_path);
+#else
+      BASE_HASH_NAMESPACE::hash<base::FilePath>()(relative_path);
+#endif  // COMPILER
+  return base::SizeTToString(int_key);
+}
+
+ChromeZoomLevelPrefs::ChromeZoomLevelPrefs(PrefService* pref_service,
+                                           const base::FilePath& profile_path)
+    : pref_service_(pref_service),
+      profile_path_(profile_path),
+      host_zoom_map_(NULL) {
+  DCHECK(pref_service_);
+}
+
+ChromeZoomLevelPrefs::~ChromeZoomLevelPrefs() {
+}
+
+void ChromeZoomLevelPrefs::InitPrefsAndCopyToHostZoomMap(
+    const base::FilePath& partition_path,
+    content::HostZoomMap* host_zoom_map) {
+  DCHECK(!partition_path.empty());
+  DCHECK((partition_path == profile_path_) ||
+         profile_path_.IsParent(partition_path));
+  // This init function must be called only once.
+  DCHECK(!host_zoom_map_);
+  DCHECK(host_zoom_map);
+  host_zoom_map_ = host_zoom_map;
+
+  // Create a partition_key string with no '.'s in it. For the default
+  // StoragePartition, this string will always be "0".
+  base::FilePath partition_relative_path;
+  profile_path_.AppendRelativePath(partition_path, &partition_relative_path);
+  partition_key_ = GetHash(partition_relative_path);
+
+  // Initialize the default zoom level.
+  host_zoom_map_->SetDefaultZoomLevel(GetDefaultZoomLevelPref());
+
+  // Initialize the HostZoomMap with per-host zoom levels from the persisted
+  // zoom-level preference values.
+  const base::DictionaryValue* host_zoom_dictionaries =
+      pref_service_->GetDictionary(prefs::kPerHostZoomLevels);
+  const base::DictionaryValue* host_zoom_dictionary;

[battre, 2014/10/06 14:33:51]

= 0;

[wjmaclean, 2014/10/06 16:58:12]

Done.

+  if (host_zoom_dictionaries &&
+      host_zoom_dictionaries->GetDictionary(partition_key_,
+                                            &host_zoom_dictionary)) {
+    std::vector<std::string> keys_to_remove;
+    for (base::DictionaryValue::Iterator i(*host_zoom_dictionary); !i.IsAtEnd();
+         i.Advance()) {
+      const std::string& host(i.key());
+      double zoom_level = 0;
+
+      bool success = i.value().GetAsDouble(&zoom_level);
+      DCHECK(success);

[battre, 2014/10/06 14:33:51]

Preference files might be modified manually. I would not DCHECK on this.

[wjmaclean, 2014/10/06 16:58:13]

This was part of the legacy initialization code, though I see your point.
Updated.

+
+      if (host.empty() ||
+          content::ZoomValuesEqual(zoom_level,
+                                   host_zoom_map_->GetDefaultZoomLevel())) {
+        keys_to_remove.push_back(host);
+        continue;
+      }
+
+      host_zoom_map_->SetZoomLevelForHost(host, zoom_level);
+    }
+
+    // Sanitize prefs to remove entries that match the default zoom level and/or
+    // have an empty host.
+    {
+      DictionaryPrefUpdate update(pref_service_, prefs::kPerHostZoomLevels);
+      base::DictionaryValue* host_zoom_dictionaries = update.Get();
+      base::DictionaryValue* host_zoom_dictionary;
+      if (!host_zoom_dictionaries->GetDictionary(partition_key_,
+                                                 &host_zoom_dictionary)) {

[battre, 2014/10/06 14:33:51]

Is this branch at all possible? Doesn't line 72/72 prevent it?

[wjmaclean, 2014/10/06 16:58:13]

Done.

+        host_zoom_dictionary = new base::DictionaryValue();
+        host_zoom_dictionaries->Set(partition_key_, host_zoom_dictionary);
+      }

[battre, 2014/10/06 14:33:51]

I don't see a purpose for storing empty dictionaries. I think they get purged on
disk anyway.

[wjmaclean, 2014/10/06 16:58:12]

My experience from looking at prefs files is that empty dictionaries do get
stored.

I guess if we should only ever be removing here, then if we have a missing
sub-dictionary then something's wrong.

+
+      for (std::vector<std::string>::const_iterator it = keys_to_remove.begin();
+           it != keys_to_remove.end();
+           ++it) {

[battre, 2014/10/06 14:33:51]

nit: you can use C++11 syntax here

[wjmaclean, 2014/10/06 16:58:13]

Done.

+        host_zoom_dictionary->RemoveWithoutPathExpansion(*it, NULL);
+      }
+    }
+  }
+  zoom_subscription_ = host_zoom_map_->AddZoomLevelChangedCallback(base::Bind(
+      &ChromeZoomLevelPrefs::OnZoomLevelChanged, base::Unretained(this)));
+}
+
+void ChromeZoomLevelPrefs::SetDefaultZoomLevelPref(double level) {
+  if (host_zoom_map_->GetDefaultZoomLevel() == level)

[battre, 2014/10/06 14:33:51]

Don't compare zoom levels with ==. Use content::ZoomValuesEqual.

[wjmaclean, 2014/10/06 16:58:12]

Done.

+    return;
+
+  DictionaryPrefUpdate update(pref_service_, prefs::kDefaultZoomLevel);
+  update->SetDouble(partition_key_, level);
+  // For unregistered paths, OnDefaultZoomLevelChanged won't be called, so
+  // set this manually.
+  host_zoom_map_->SetDefaultZoomLevel(level);
+  default_zoom_changed_callbacks_.Notify();
+}
+
+double ChromeZoomLevelPrefs::GetDefaultZoomLevelPref() {
+  double default_zoom_level = 0.0;
+
+  const base::DictionaryValue* default_zoom_level_dictionary =
+      pref_service_->GetDictionary(prefs::kDefaultZoomLevel);
+  if (!default_zoom_level_dictionary->GetDouble(partition_key_,
+                                                &default_zoom_level)) {
+    // We will only get here if the default zoom level for this partition
+    // has never been set before.
+    SetDefaultZoomLevelPref(default_zoom_level);

[battre, 2014/10/06 14:33:51]

Why do you need this? I don't think that a getter should persist anything. Also
default values should not be persisted so that they can be changed later.

[wjmaclean, 2014/10/06 16:58:12]

I agree the getter should be const, but wasn't sure where else to put this. I've
removede it (see end of this comment).

I had though the default values would need to be persisted since the
per-partition default zoom levels cannot be registered (due to the prefix
property, see bauerb@'s review comments), which is (in my possibly limited
knowledge) the only other way to set a default value.

If it's an acceptable alternative, we can use the value on line 129 to act as
the (non-persisted) default value.

+  }
+  return default_zoom_level;
+}
+
+scoped_ptr<ChromeZoomLevelPrefs::DefaultZoomLevelSubscription>
+ChromeZoomLevelPrefs::RegisterDefaultZoomLevelCallback(
+    const base::Closure& callback) {
+  return default_zoom_changed_callbacks_.Add(callback);
+}
+
+void ChromeZoomLevelPrefs::OnZoomLevelChanged(
+    const content::HostZoomMap::ZoomLevelChange& change) {
+  if (change.mode != content::HostZoomMap::ZOOM_CHANGED_FOR_HOST)
+    return;
+  double level = change.zoom_level;
+  // Note: the following will create the dictionary if it doesn't already exist.
+  DictionaryPrefUpdate update(pref_service_, prefs::kPerHostZoomLevels);
+  base::DictionaryValue* host_zoom_dictionaries = update.Get();
+  DCHECK(host_zoom_dictionaries);
+
+  base::DictionaryValue* host_zoom_dictionary;

[battre, 2014/10/06 14:33:51]

= NULL;

[wjmaclean, 2014/10/06 16:58:12]

Done.

+  if (!host_zoom_dictionaries->GetDictionary(partition_key_,
+                                             &host_zoom_dictionary)) {
+    host_zoom_dictionary = new base::DictionaryValue();
+    host_zoom_dictionaries->Set(partition_key_, host_zoom_dictionary);

[battre, 2014/10/06 14:33:51]

Why?

[wjmaclean, 2014/10/06 16:58:12]

Won't we hit an error if we try to add an entry to a non-existent
sub-dictionary? The value of host_zoom_dictionary can be NULL (since the
sub-dictionaries aren't registered, and this sub-dictionary might never have
been accessed before).

I've modified the code to only create the sub-dictionary if we are doing an
insertion, as for a removal it should already exist.

[battre, 2014/10/08 11:27:10]

Acknowledged. - I misunderstood the code.

+  }
+
+  if (content::ZoomValuesEqual(level, host_zoom_map_->GetDefaultZoomLevel()))
+    host_zoom_dictionary->RemoveWithoutPathExpansion(change.host, NULL);
+  else
+    host_zoom_dictionary->SetDoubleWithoutPathExpansion(change.host, level);
+}
+
+}  // namespace chrome