Implement experimental.contentSettings.misc.blockThirdPartyCookies.

chrome/browser/extensions/extension_preference_api.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_preference_api.h"
+
+#include "base/stringprintf.h"
+#include "base/values.h"
+#include "chrome/browser/extensions/extension_prefs.h"
+#include "chrome/browser/extensions/extension_service.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chrome/common/pref_names.h"
+
+namespace {
+
+struct PrefMappingEntry {
+  const char* extension_pref;
+  const char* browser_pref;
+  const char* permission;
+};
+
+PrefMappingEntry pref_mapping[] = {
+  { "blockThirdPartyCookies",
+    prefs::kBlockThirdPartyCookies,
+    Extension::kContentSettingsPermission

[Aaron Boodman, 2011/02/11 18:33:19]

Can you give an example of a pref that would have a different permission value?
This might be early generalization.

[Bernhard Bauer, 2011/02/13 18:19:12]

Proxy settings, for example. We have to do the permission check for content
settings here, as any extension has access to the experimental.contentSettings
object, and I want to keep this part as general as possible, to make it easier
for additional preferences.

[Aaron Boodman, 2011/02/14 02:54:02]

I'm sorry I don't understand. Permission checking is not usually done in the
modules -- it is generalized in extension.cc. Why is this check necessary here?

+  },
+};
+
+bool FindBrowserPrefForExtensionPref(const std::string& extension_pref,
+                                     std::string* browser_pref,
+                                     std::string* permission) {
+  for (size_t i = 0; i < arraysize(pref_mapping); ++i) {

[Aaron Boodman, 2011/02/11 18:33:19]

Suggest starting out with this in a map. Even though it is (microscopically)
slower for small numbers of items, it better represents what's actually going
on.

You can find examples of how to have static maps in various places in the code
base. Off the top of my head I believe there is one in extension.cc. Basically
it involves having a struct like StaticData that you use base::Singleton with.

[Bernhard Bauer, 2011/02/13 18:19:12]

Okay, done.

+    if (extension_pref == pref_mapping[i].extension_pref) {
+      *browser_pref = pref_mapping[i].browser_pref;
+      *permission = pref_mapping[i].permission;
+      return true;
+    }
+  }
+  return false;
+}
+
+const char kInvalidPreferenceErrorMessage[] = "Invalid preference '%s'.";

[Aaron Boodman, 2011/02/11 18:33:19]

Not used?

[Bernhard Bauer, 2011/02/13 18:19:12]

Right, leftover from a previous version.

+const char kPermissionErrorMessage[] =
+    "You do not have permission to access the preference '%s'. "
+    "Be sure to declare in your manifest what permissions you need.";
+
+}  // namespace
+
+GetPreferenceFunction::~GetPreferenceFunction() { }
+
+bool GetPreferenceFunction::RunImpl() {
+  std::string pref_key;
+  EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &pref_key));
+  DictionaryValue* details;

[Aaron Boodman, 2011/02/11 18:33:19]

Nit: initialize primitives. Multiple places in this file.

[Bernhard Bauer, 2011/02/13 18:19:12]

Done.

+  EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(1, &details));
+  Value* incognito_value;
+  bool incognito = false;
+  if (details->Get("incognito", &incognito_value))
+    EXTENSION_FUNCTION_VALIDATE(incognito_value->GetAsBoolean(&incognito));

[Aaron Boodman, 2011/02/11 18:33:19]

Nit: I think this is more clear with:

if (details->HasKey("incognito"))
  EXTENSION_FUNCTION_VALIDATE(details->GetBoolean(...))

[Bernhard Bauer, 2011/02/13 18:19:12]

Done.

+
+  PrefService* prefs = incognito ? profile_->GetOffTheRecordPrefs()
+                                 : profile_->GetPrefs();

[Aaron Boodman, 2011/02/11 18:33:19]

This seems a bit weird. How do these prefs work in incognito? Don't they fall
through to the default profile normally?

[Bernhard Bauer, 2011/02/13 18:19:12]

We already have a separate thread going with mpcomplete about that :-)

In general, we do allow separate preferences for incognito mode now, only
falling back to the normal preferences if there is no value for the incognito
preferences (copy-on-write semantics, if you will).

[Aaron Boodman, 2011/02/14 02:54:02]

Should we only allow this if the extension is enabled for incognito mode?

+  std::string browser_pref;
+  std::string permission;
+  EXTENSION_FUNCTION_VALIDATE(
+      FindBrowserPrefForExtensionPref(pref_key, &browser_pref, &permission));
+  if (!GetExtension()->HasApiPermission(permission)) {
+    error_ = base::StringPrintf(kPermissionErrorMessage, pref_key.c_str());
+    return false;
+  }
+  const PrefService::Preference* pref =
+      prefs->FindPreference(browser_pref.c_str());
+  DCHECK(pref);

[Aaron Boodman, 2011/02/11 18:33:19]

Don't be afraid. If this really can't happen, use CHECK.

[Bernhard Bauer, 2011/02/13 18:19:12]

I thought if it really can't happen, we should use DCHECK? :D

The only way this can happen is if there is an invalid preference defined in the
pref mapping, which would be a) a programmer error and b) something we would
find very quickly in tests, so it shouldn't come up in a release build, and we
can leave out the check.

[Aaron Boodman, 2011/02/14 02:54:02]

This is my personal preference, not Chrome policy. In my opinion code should use
CHECK except in cases where there is a significant performance penalty.

Reasons:

1. Many developers don't use debug builds for local development, so they won't
see this issue.

2. There are lots of invalid DCHECKs sitting in the codebase unfortunately, so
invalid DCHECKs are less likely to get attention.

3. Users don't run debug builds, so the assertions don't help us in the field.
Instead of getting an assertion sent to us, we get reports of weird behavior, or
crashes at some later point in the code.
I would make it a check then.

+  result_.reset(pref->GetValue()->DeepCopy());

[Aaron Boodman, 2011/02/11 18:33:19]

Is GetValue() always a fundamental Value? Maybe you should CHECK that.

[Bernhard Bauer, 2011/02/13 18:19:12]

Does it have to be? In theory, we could return an arbitrary structured value,
no?

[Aaron Boodman, 2011/02/14 02:54:02]

I suppose. Ok, n/m.

+  return true;
+}
+
+SetPreferenceFunction::~SetPreferenceFunction() { }
+
+bool SetPreferenceFunction::RunImpl() {
+  std::string pref_key;
+  EXTENSION_FUNCTION_VALIDATE(args_->GetString(0, &pref_key));
+  DictionaryValue* details;
+  EXTENSION_FUNCTION_VALIDATE(args_->GetDictionary(1, &details));
+
+  Value* value;
+  EXTENSION_FUNCTION_VALIDATE(details->Get("value", &value));
+
+  Value* incognito_value;
+  bool incognito = false;
+  if (details->Get("incognito", &incognito_value))
+    EXTENSION_FUNCTION_VALIDATE(incognito_value->GetAsBoolean(&incognito));
+
+  std::string browser_pref;
+  std::string permission;
+  EXTENSION_FUNCTION_VALIDATE(
+      FindBrowserPrefForExtensionPref(pref_key, &browser_pref, &permission));
+  if (!GetExtension()->HasApiPermission(permission)) {
+    error_ = base::StringPrintf(kPermissionErrorMessage, pref_key.c_str());
+    return false;
+  }
+  ExtensionPrefs* prefs = profile_->GetExtensionService()->extension_prefs();
+  const PrefService::Preference* pref =
+      prefs->FindPreference(browser_pref.c_str());
+  DCHECK(pref);
+  EXTENSION_FUNCTION_VALIDATE(value->GetType == pref->GetType());

[Aaron Boodman, 2011/02/11 18:33:19]

Syntax error? GetType() ?

[Bernhard Bauer, 2011/02/13 18:19:12]

Woah. Fixed.

+  prefs->SetExtensionControlledPref(extension_id(),
+                                    browser_pref,
+                                    incognito,
+                                    value->DeepCopy());
+  return true;
+}