Theme Preferences for Distilled Pages

components/dom_distiller/core/distilled_page_prefs.cc

+// Copyright 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 "components/dom_distiller/core/distilled_page_prefs.h"
+
+#include "base/bind.h"
+#include "base/memory/weak_ptr.h"
+#include "base/message_loop/message_loop.h"
+#include "base/observer_list.h"
+#include "base/prefs/pref_service.h"
+#include "components/pref_registry/pref_registry_syncable.h"
+
+namespace {
+
+// True if user prefers high-contrast (dark) appearance for distilled page.

[nyquist, 2014/07/07 19:12:48]

This comment needs to be updated.

[smaslo, 2014/07/08 19:58:10]

Done.

+const char kThemePref[] = "dom_distiller.theme";
+
+}
+
+namespace dom_distiller {
+
+DistilledPagePrefs::DistilledPagePrefs(PrefService* pref_service)
+    : pref_service_(pref_service),
+      weak_ptr_factory_(this) {
+}
+
+DistilledPagePrefs::~DistilledPagePrefs() {
+}
+
+void DistilledPagePrefs::AddObserver(Observer* obs) {
+  observers_.AddObserver(obs);
+}
+
+void DistilledPagePrefs::RemoveObserver(Observer* obs) {
+  observers_.RemoveObserver(obs);
+}
+
+// static
+void DistilledPagePrefs::RegisterProfilePrefs(
+    user_prefs::PrefRegistrySyncable* registry) {
+  registry->RegisterIntegerPref(kThemePref,
+      Theme::kLight,
+      user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
+}
+
+void DistilledPagePrefs::SetTheme(Theme new_theme) {
+  pref_service_->SetInteger(kThemePref, new_theme);
+  base::MessageLoop::current()->PostTask(FROM_HERE,
+      base::Bind(&DistilledPagePrefs::NotifyOnChangeTheme,
+          weak_ptr_factory_.GetWeakPtr(), new_theme));
+}
+
+void DistilledPagePrefs::NotifyOnChangeTheme(Theme new_theme) {
+  FOR_EACH_OBSERVER(Observer, observers_, OnChangeTheme(new_theme));
+}
+
+const DistilledPagePrefs::Theme DistilledPagePrefs::GetTheme() const {
+  return (Theme) pref_service_->GetInteger(kThemePref);

[nyquist, 2014/07/07 19:12:48]

This is an unsafe cast, since the values on disk could have been changed.

I think you must add an extra enum value "THEME_COUNT" or something like that
and then do bounds checking here.
If the bounds check fails, you probably want to reset the pref to the default
value to ensure the bounds checking will work next time.. (just extract out the
Theme::kLight from the registration to an anonymous const and use it here and
during registration).

See
https://code.google.com/p/chromium/codesearch#chromium/src/sync/internal_api/...
for example of a _COUNT style last enum value.

[smaslo, 2014/07/08 19:58:10]

Done.  I also added a check to SetTheme() to prevent a wrong value from being
stored and passed through NotifyOnChangeTheme().  I had to remove the const
specifier since the function will now change values.

On 2014/07/07 19:12:48, nyquist wrote:

+}
+
+}  // namespace dom_distiller
+