Add onChange event to preference extension APIs.

Add onChange event to preference extension APIs.

BUG=73994
TEST=TBD

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=78354

[Bernhard Bauer, 2011-03-09 18:56:58]

Please review.

[battre, 2011-03-10 13:07:28]

Do we need to update the API documentation?

http://codereview.chromium.org/6596044/diff/14003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/6596044/diff/14003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:92: // Mapping from
extension pref keys to browser pref keys.
Thanks you :-)

http://codereview.chromium.org/6596044/diff/14003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:108: }
DCHECK(mapping_.size()==arraysize(kPrefMapping) &&
event_mapping_.size()==arraysize(kPrefMapping)) to be sure that all entries are
unique?

http://codereview.chromium.org/6596044/diff/14003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:159:
OnPrefChanged(Source<PrefService>(source).ptr(), *pref_key);
PREF_CHANGED is only fired when the effective value changes. Should we be
notified if the level of control changes?

http://codereview.chromium.org/6596044/diff/14003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:183: dict->Set(kValue,
pref->GetValue()->DeepCopy());
The proxy settings API needs a hook to modify this value before it is delivered
to the extension.

http://codereview.chromium.org/6596044/diff/14003/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/content_settings/onchange/test.html
(right):

http://codereview.chromium.org/6596044/diff/14003/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/content_settings/onchange/test.html:6:
function listenUntil(event, expected) {
add a comment that listenForever subscribes to both regular and incognito
preference changes?

http://codereview.chromium.org/6596044/diff/14003/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/content_settings/onchange/test.html:22:
function changeDefault() {
add comment
// Setting the value for the first time affects both regular and incognito
preferences.
?

[Bernhard Bauer, 2011-03-10 14:51:15]

On 2011/03/10 13:07:28, battre wrote:
> Do we need to update the API documentation?

Added event documentation.

http://codereview.chromium.org/6596044/diff/14003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/6596044/diff/14003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:108: }
On 2011/03/10 13:07:28, battre wrote:
> DCHECK(mapping_.size()==arraysize(kPrefMapping) &&
> event_mapping_.size()==arraysize(kPrefMapping)) to be sure that all entries
are
> unique?

Done.

http://codereview.chromium.org/6596044/diff/14003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:159:
OnPrefChanged(Source<PrefService>(source).ptr(), *pref_key);
On 2011/03/10 13:07:28, battre wrote:
> PREF_CHANGED is only fired when the effective value changes. Should we be
> notified if the level of control changes?

Oh, I was assuming it is also fired when the level of control changes. Is there
another notification for that?

http://codereview.chromium.org/6596044/diff/14003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:183: dict->Set(kValue,
pref->GetValue()->DeepCopy());
On 2011/03/10 13:07:28, battre wrote:
> The proxy settings API needs a hook to modify this value before it is
delivered
> to the extension.

Hm, I see. I guess we'll need a separate event router for proxy events then, and
we should try to pull out the value transformation for events and the get
function.

http://codereview.chromium.org/6596044/diff/14003/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/content_settings/onchange/test.html
(right):

http://codereview.chromium.org/6596044/diff/14003/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/content_settings/onchange/test.html:6:
function listenUntil(event, expected) {
On 2011/03/10 13:07:28, battre wrote:
> add a comment that listenForever subscribes to both regular and incognito
> preference changes?

I think we already explain that sufficiently below. This function is a little
bit more general.

http://codereview.chromium.org/6596044/diff/14003/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/content_settings/onchange/test.html:22:
function changeDefault() {
On 2011/03/10 13:07:28, battre wrote:
> add comment
> // Setting the value for the first time affects both regular and incognito
> preferences.
> ?

Done.

[battre, 2011-03-11 09:28:36]

http://codereview.chromium.org/6596044/diff/14003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/6596044/diff/14003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:159:
OnPrefChanged(Source<PrefService>(source).ptr(), *pref_key);
On 2011/03/10 14:51:15, Bernhard Bauer wrote:
> On 2011/03/10 13:07:28, battre wrote:
> > PREF_CHANGED is only fired when the effective value changes. Should we be
> > notified if the level of control changes?
> 
> Oh, I was assuming it is also fired when the level of control changes. Is
there
> another notification for that?

This is not implemented, yet.

http://codereview.chromium.org/6596044/diff/14003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:183: dict->Set(kValue,
pref->GetValue()->DeepCopy());
On 2011/03/10 14:51:15, Bernhard Bauer wrote:
> On 2011/03/10 13:07:28, battre wrote:
> > The proxy settings API needs a hook to modify this value before it is
> delivered
> > to the extension.
> 
> Hm, I see. I guess we'll need a separate event router for proxy events then,
and
> we should try to pull out the value transformation for events and the get
> function.

Yes, we could even extract the value transformation the other way around.

[Bernhard Bauer, 2011-03-15 17:35:15]

On 2011/03/11 09:28:36, battre wrote:
>
http://codereview.chromium.org/6596044/diff/14003/chrome/browser/extensions/e...
> File chrome/browser/extensions/extension_preference_api.cc (right):
> 
>
http://codereview.chromium.org/6596044/diff/14003/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_preference_api.cc:159:
> OnPrefChanged(Source<PrefService>(source).ptr(), *pref_key);
> On 2011/03/10 14:51:15, Bernhard Bauer wrote:
> > On 2011/03/10 13:07:28, battre wrote:
> > > PREF_CHANGED is only fired when the effective value changes. Should we be
> > > notified if the level of control changes?
> > 
> > Oh, I was assuming it is also fired when the level of control changes. Is
> there
> > another notification for that?
> 
> This is not implemented, yet.

I guess we should file a bug for it then (for the case when the controlling
PrefStore changes as well as when the controlling extension changes).

>
http://codereview.chromium.org/6596044/diff/14003/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_preference_api.cc:183: dict->Set(kValue,
> pref->GetValue()->DeepCopy());
> On 2011/03/10 14:51:15, Bernhard Bauer wrote:
> > On 2011/03/10 13:07:28, battre wrote:
> > > The proxy settings API needs a hook to modify this value before it is
> > delivered
> > > to the extension.
> > 
> > Hm, I see. I guess we'll need a separate event router for proxy events then,
> and
> > we should try to pull out the value transformation for events and the get
> > function.
> 
> Yes, we could even extract the value transformation the other way around.

Alright, the transformer is extracted and landed. PTAL?

[battre, 2011-03-16 13:07:35]

LGTM

http://codereview.chromium.org/6596044/diff/28001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_preference_api.cc (right):

http://codereview.chromium.org/6596044/diff/28001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_preference_api.cc:179: // Mapping from
browser pref keys to extension event names.
"to extension event names" is not quite correct, is it? event names and
permissions