Add onChanged events to the extension settings API, both from sync and between

chrome/browser/extensions/extension_settings_changes.h

+// 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.
+
+#ifndef CHROME_BROWSER_EXTENSIONS_EXTENSION_SETTINGS_CHANGES_H_
+#define CHROME_BROWSER_EXTENSIONS_EXTENSION_SETTINGS_CHANGES_H_
+#pragma once
+
+#include "base/memory/ref_counted.h"
+#include "base/values.h"
+
+// A list of changes to extension settings.  Thread-safe and efficient to copy.
+class ExtensionSettingsChanges {

[akalin, 2011/10/14 10:31:49]

I think ExtensionSettingChanges is less awkward (you'll have to rename the
files, too, sorry)

[not at google - send to devlin, 2011/10/17 00:04:33]

Done.

+ public:
+  ExtensionSettingsChanges();
+  ~ExtensionSettingsChanges();
+
+  // Appends a change for a setting.
+  void AppendChange(

[akalin, 2011/10/14 10:31:49]

Unfortunately, the presence of this function makes this class potentially
non-thread-safe (even though we use it in a thread-safe manner).

I think the logic in AppendChange should be put back in the
SyncableExtensionStorage::OnSync* functions.  It should basically build the
ListValue, then hand it off to this class when it's done.  So basically,
ExtensionSettingChanges will be a thin wrapper around a const ref-counted
ListValue (with a ToJson convenience fn).

It's also better because you can avoid unnecessary null-checking and
deep-copying.

[not at google - send to devlin, 2011/10/17 00:04:33]

Hm ok.

I don't really like building up a ListValue separately since it doesn't imply
correctness in the event objects that get built.  How about that Builder Java
idiom?

Though upon writing the code... it seems like an equivalent of the Builder idiom
in C++ is the const idiom where the const version of this class is thread-safe. 
I.e. what's already here.  Eh.  WDYT?

The other option is a static method on this like

static void AppendChange(ListValue* changes, ...);

but that's rather yuck.

[akalin, 2011/10/17 08:08:57]

That works, too.
I don't know what you mean by the 'const version of this class'.  If you're
referring to the fact that the callers receive a const reference to an
ExtensionSettingsChanges object, then that's still not thread safe as they can
make a copy of it and mutate the underlying object.

[not at google - send to devlin, 2011/10/17 10:35:33]

Cool, well, I've written the code and it's uploaded and all that.

Since the only operation on ExtensionSettingChanges is now ToJson(), might as
well not store the ListValue at all and just create the JSON on Build.  The
Changes class then becomes effectively a typed and thread-safe string.

[Aaron Boodman, 2011/10/18 06:18:30]

Guys, I'm really sorry to add more noise to this CL, but I'm getting a bit
concerned about complexity.

Do we really need a Builder and a ref-counted inner object thing to build up a
ListValue? This seems over-engineered and to someone reading the code it is hard
to get a handle on what is going on.

I liked Ben's original take on this object. I don't understand why it needs to
be threadsafe. It is only used on one thread at a time right? That is the
typical pattern in Chromium and I don't see why we'd not follow it here.

+      const std::string& key,
+      // Ownership taken.  May be NULL to imply there is no old value.
+      Value* old_value,
+      // Ownership taken.  May be NULL to imply there is no new value.
+      Value* new_value);
+
+  // Gets the JSON serialized form of the changes, for example:
+  // [ { "key": "foo", "oldValue": "bar", "newValue": "baz" } ]
+  std::string ToJson() const;
+
+ private:
+  struct Inner : base::RefCountedThreadSafe<Inner> {
+    ListValue changes;

[akalin, 2011/10/14 10:31:49]

make the destructor private

[not at google - send to devlin, 2011/10/17 00:04:33]

Done.

+  };
+  scoped_refptr<Inner> inner_;
+};
+
+#endif  // CHROME_BROWSER_EXTENSIONS_EXTENSION_SETTINGS_CHANGES_H_