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 {
+
+// Path to the integer corresponding to user's preference theme.
+const char kThemePref[] = "dom_distiller.theme";
+
+}
+
+namespace dom_distiller {
+
+DistilledPagePrefs::DistilledPagePrefs(PrefService* pref_service)
+    : pref_service_(pref_service),
+      weak_ptr_factory_(this) {
+}
+
+DistilledPagePrefs::~DistilledPagePrefs() {
+}
+
+// static
+void DistilledPagePrefs::RegisterProfilePrefs(
+    user_prefs::PrefRegistrySyncable* registry) {
+  registry->RegisterIntegerPref(kThemePref,
+      Theme::kLight,
+      user_prefs::PrefRegistrySyncable::SYNCABLE_PREF);
+}
+
+void DistilledPagePrefs::SetTheme(Theme new_theme) {
+  if (new_theme >= Theme::kThemeCount) {

[nyquist, 2014/07/08 22:26:47]

When a parameter is already an enum you don't have to bounds check it, so this
last edit should be unnecessary.

[smaslo, 2014/07/09 19:42:25]

Done.

+    new_theme = Theme::kLight;
+  }
+  pref_service_->SetInteger(kThemePref, new_theme);
+  base::MessageLoop::current()->PostTask(FROM_HERE,
+      base::Bind(&DistilledPagePrefs::NotifyOnChangeTheme,
+          weak_ptr_factory_.GetWeakPtr(), new_theme));
+}
+
+const DistilledPagePrefs::Theme DistilledPagePrefs::GetTheme() {
+  if (pref_service_->GetInteger(kThemePref) >= Theme::kThemeCount) {

[nyquist, 2014/07/08 22:26:48]

You also need to check for negative values. How about read the value into an
int, and then do bounds checking on it?
foobar >= 0 && foobar < THEME_COUNT

[smaslo, 2014/07/09 19:42:25]

Done.

+    SetTheme(Theme::kLight);

[nyquist, 2014/07/08 22:26:48]

I think you could:
1) read to local variable
2) bounds check
  - if OOB, do like you do now, and then just return Theme::kLight inside the
if.
3) return a cast of the local variable.

[smaslo, 2014/07/09 19:42:25]

Done.

+  }
+  return (Theme) pref_service_->GetInteger(kThemePref);

[nyquist, 2014/07/08 22:26:48]

I don't think you want to read from disk again here (in case someone is wrong
with reading files)

[smaslo, 2014/07/09 19:42:25]

Done.

+}
+
+void DistilledPagePrefs::AddObserver(Observer* obs) {
+  observers_.AddObserver(obs);
+}
+
+void DistilledPagePrefs::RemoveObserver(Observer* obs) {
+  observers_.RemoveObserver(obs);
+}
+
+void DistilledPagePrefs::NotifyOnChangeTheme(Theme new_theme) {
+  FOR_EACH_OBSERVER(Observer, observers_, OnChangeTheme(new_theme));
+}
+
+}  // namespace dom_distiller
+