Introduce incognito preference settings.

chrome/browser/extensions/extension_pref_value_map.cc

+// Copyright (c) 2011 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/extensions/extension_pref_value_map.h"
+
+#include "base/scoped_ptr.h"
+#include "base/stl_util-inl.h"
+#include "base/values.h"
+#include "chrome/browser/prefs/pref_value_map.h"
+
+ExtensionPrefValueMap::ExtensionPrefValueMap()
+    : initialization_complete_(false) {
+}
+
+ExtensionPrefValueMap::~ExtensionPrefValueMap() {
+  STLDeleteValues(&entries_);

[Mattias Nissler (ping if slow), 2011/01/05 12:08:07]

Hm, let's put an entries_.clear() here just to be on the safe side :)

[battre, 2011/01/05 20:23:08]

Done.

+}
+
+void ExtensionPrefValueMap::SetExtensionPref(const std::string& ext_id,
+                                             const std::string& key,
+                                             bool incognito,
+                                             Value* value) {
+  scoped_ptr<Value> scoped_value(value);
+  PrefValueMap* prefs = GetExtensionPrefValueMap(ext_id, incognito);
+
+  Value* oldValue = NULL;
+  prefs->GetValue(key, &oldValue);
+
+  bool modified = !Value::Equals(oldValue, scoped_value.get());
+  if (!modified)

[Mattias Nissler (ping if slow), 2011/01/05 12:08:07]

no need for the modified local, just inline the condition.

[battre, 2011/01/05 20:23:08]

Gone. (due to following comment)

+    return;
+
+  prefs->SetValue(key, scoped_value.release());

[Mattias Nissler (ping if slow), 2011/01/05 12:08:07]

You can just do the following:

if (prefs->SetValue(key, value))
  NotifyPrefValueChanged(key)

[battre, 2011/01/05 20:23:08]

Done.

+  NotifyPrefValueChanged(key);
+}
+
+void ExtensionPrefValueMap::RemoveExtensionPref(const std::string& ext_id,
+                                                const std::string& key,
+                                                bool incognito) {
+  PrefValueMap* prefs = GetExtensionPrefValueMap(ext_id, incognito);
+  prefs->RemoveValue(key);

[Mattias Nissler (ping if slow), 2011/01/05 12:08:07]

RemoveValue returns a bool indicating whether there actually was a change. So
make the NotifyPrefValueChanged(key) call only when RemoveValue returns true?

[battre, 2011/01/05 20:23:08]

Done.

+  NotifyPrefValueChanged(key);
+}
+
+void ExtensionPrefValueMap::OnInitializationCompleted() {
+  DCHECK(!initialization_complete_);

[Mattias Nissler (ping if slow), 2011/01/05 12:08:07]

It seems overkill to have that flag only for the sake of being able to DCHECK.

[battre, 2011/01/05 20:23:08]

Done.

+  initialization_complete_ = true;
+  NotifyInitializationCompleted();

[Mattias Nissler (ping if slow), 2011/01/05 12:08:07]

Why not just inline the NotifyInitializationCompleted code here?

[battre, 2011/01/05 20:23:08]

Done.

+}
+
+void ExtensionPrefValueMap::RegisterExtension(const std::string& ext_id,
+                                              const base::Time& install_time,
+                                              bool is_enabled) {
+  if (entries_.find(ext_id) != entries_.end()) {
+    LOG(WARNING) << "Extension " << ext_id << " registered multiple times.";

[Mattias Nissler (ping if slow), 2011/01/05 12:08:07]

Make this a NOTREACHED()?

[battre, 2011/01/05 20:23:08]

Cannot be NOTREACHED. An extension can be reinstalled in which case it should
have empty preferences.

I have removed the warning.

+    UnregisterExtension(ext_id);
+  }
+  entries_[ext_id] = new ExtensionEntry;
+  entries_[ext_id]->install_time = install_time;
+  entries_[ext_id]->enabled = is_enabled;
+}
+
+void ExtensionPrefValueMap::UnregisterExtension(const std::string& ext_id) {
+  if (entries_.find(ext_id) == entries_.end()) {
+    LOG(WARNING) << "Extension " << ext_id << " unregistered without"
+                 << " being registered.";

[Mattias Nissler (ping if slow), 2011/01/05 12:08:07]

NOTREACHED() again?

[battre, 2011/01/05 20:23:08]

Everytime the extension black list is updated, all their configuration values
are deleted. I have removed the warning as unregistering an extension twice is
not really critical in any way.

+    return;
+  }
+  std::set<std::string> keys;  // keys set by this extension
+  GetExtensionControlledKeys(ext_id, &keys);

[Mattias Nissler (ping if slow), 2011/01/05 12:08:07]

It seems this function should really operate on ExtensionEntry and not make
another lookup...

[battre, 2011/01/05 20:23:08]

Done.

+
+  delete entries_[ext_id];

[Mattias Nissler (ping if slow), 2011/01/05 12:08:07]

If you do the find() call above, can we use that iterator for the delete and
erase() calls instead of looking it up again?

[battre, 2011/01/05 20:23:08]

Done.

+  entries_.erase(ext_id);
+
+  NotifyPrefValueChanged(keys);
+}
+
+void ExtensionPrefValueMap::UpdateExtensionsState(const std::string& ext_id,
+                                                  bool is_enabled) {
+  CHECK(entries_.find(ext_id) != entries_.end());
+  if (entries_[ext_id]->enabled == is_enabled)
+    return;
+  std::set<std::string> keys;  // keys set by this extension
+  GetExtensionControlledKeys(ext_id, &keys);
+  entries_[ext_id]->enabled = is_enabled;
+  NotifyPrefValueChanged(keys);
+}
+
+PrefValueMap* ExtensionPrefValueMap::GetExtensionPrefValueMap(
+    const std::string& ext_id,
+    bool incognito) {
+  CHECK(entries_[ext_id]);
+  PrefValueMap* prefs = incognito ? &(entries_[ext_id]->inc_preferences)
+                                  : &(entries_[ext_id]->reg_preferences);

[Mattias Nissler (ping if slow), 2011/01/05 12:08:07]

Again, three lookups where one would suffice. Why not do it along the lines of
the const variant below?

[battre, 2011/01/05 20:23:08]

I prefer the visual clarity of a [] lookup over some obscure "i->second", but I
don't care too much.

Done.

+  return prefs;
+}
+
+const PrefValueMap* ExtensionPrefValueMap::GetExtensionPrefValueMap(
+    const std::string& ext_id,
+    bool incognito) const {
+  ExtensionEntryMap::const_iterator i = entries_.find(ext_id);
+  CHECK(i != entries_.end());
+  if (incognito)
+    return &(i->second->inc_preferences);
+  else
+    return &(i->second->reg_preferences);
+}
+

[Mattias Nissler (ping if slow), 2011/01/05 12:08:07]

excess newline

[battre, 2011/01/05 20:23:08]

Done.

+
+void ExtensionPrefValueMap::GetExtensionControlledKeys(
+    const std::string& ext_id, std::set<std::string>* out) const {

[Mattias Nissler (ping if slow), 2011/01/05 12:08:07]

I think you should break the line after the comma.

[Mattias Nissler (ping if slow), 2011/01/05 12:08:07]

Do you actually require ordering here? If not, I'd swap the std::set for a
std::vector, which is more efficient.

[battre, 2011/01/05 20:23:08]

Done.

[battre, 2011/01/05 20:23:08]

I don't require ordering but uniqueness. An extension might set pref1 and pref2
in reg_prefs and pref2 and pref3 in inc_prefs. We want to retrieve {pref1,
pref2, pref3}.

+  PrefValueMap::const_iterator i;
+
+  const PrefValueMap* reg_prefs = GetExtensionPrefValueMap(ext_id, false);
+  for (i = reg_prefs->begin(); i != reg_prefs->end(); ++i)
+    out->insert(i->first);
+
+  const PrefValueMap* inc_prefs = GetExtensionPrefValueMap(ext_id, true);
+  for (i = inc_prefs->begin(); i != inc_prefs->end(); ++i)
+    out->insert(i->first);
+}
+
+const Value* ExtensionPrefValueMap::GetEffectivePrefValue(
+    const std::string& key,
+    bool incognito) const {
+  Value *winner = NULL;
+  base::Time winners_install_time = base::Time();

[Mattias Nissler (ping if slow), 2011/01/05 12:08:07]

no need for the assignment here, the default constructor will be called anyway
:)

[battre, 2011/01/05 20:23:08]

right... one a Java programmer by heart you never change your habits. :-)

Done.

+
+  ExtensionEntryMap::const_iterator i;
+  for (i = entries_.begin(); i != entries_.end(); ++i) {
+    const std::string& ext_id = i->first;
+    const base::Time& install_time = i->second->install_time;
+    const bool enabled = i->second->enabled;
+
+    if (!enabled)

[Mattias Nissler (ping if slow), 2011/01/05 12:08:07]

inline i->second->enabled?

[battre, 2011/01/05 20:23:08]

I prefer my variant: The first three lines extract the relevant information from
some obscure iterator syntax. It should not make any difference at runtime.

If you disagree, I could do a
const ExtensionEntry& entry = i->second;
instead of
const base::Time& install_time = i->second->install_time;
const bool enabled = i->second->enabled;

+      continue;
+    if (install_time < winners_install_time)
+      continue;
+
+    Value* value = NULL;
+    const PrefValueMap* prefs = GetExtensionPrefValueMap(ext_id, false);
+    if (prefs->GetValue(key, &value)) {
+      winner = value;
+      winners_install_time = install_time;
+    }
+
+    if (!incognito)
+      continue;
+
+    prefs = GetExtensionPrefValueMap(ext_id, true);
+    if (prefs->GetValue(key, &value)) {
+      winner = value;
+      winners_install_time = install_time;
+    }
+  }
+  return winner;
+}
+
+void ExtensionPrefValueMap::AddObserver(PrefStore::Observer* observer) {
+  observers_.AddObserver(observer);
+}
+
+void ExtensionPrefValueMap::RemoveObserver(PrefStore::Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+void ExtensionPrefValueMap::NotifyInitializationCompleted() {
+  FOR_EACH_OBSERVER(PrefStore::Observer, observers_,
+                    OnInitializationCompleted());
+}
+
+void ExtensionPrefValueMap::NotifyPrefValueChanged(
+    const std::set<std::string>& keys) {
+  std::set<std::string>::const_iterator i;
+  for (i = keys.begin(); i != keys.end(); ++i)
+    NotifyPrefValueChanged(*i);
+}
+
+void ExtensionPrefValueMap::NotifyPrefValueChanged(const std::string& key) {
+  FOR_EACH_OBSERVER(PrefStore::Observer, observers_, OnPrefValueChanged(key));
+}