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

not at google - send to devlin

9 years, 2 months ago (2011-10-07 06:41:46 UTC) #1

akalin

Initial comments http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/extension_settings_backend.h File chrome/browser/extensions/extension_settings_backend.h (right): http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/extension_settings_backend.h#newcode30 chrome/browser/extensions/extension_settings_backend.h:30: ObserverListThreadSafe<ExtensionSettingsObserver>* observers); pass a const ref to ...

9 years, 2 months ago (2011-10-07 21:06:23 UTC) #2

Matt Perry

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/extension_settings_api.cc File chrome/browser/extensions/extension_settings_api.cc (right): http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/extension_settings_api.cc#newcode62 chrome/browser/extensions/extension_settings_api.cc:62: backend->TriggerOnSettingsChanged( It seems weird to leave it up to ...

9 years, 2 months ago (2011-10-07 22:39:52 UTC) #3

not at google - send to devlin

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/extension_settings_api.cc File chrome/browser/extensions/extension_settings_api.cc (right): http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/extension_settings_api.cc#newcode62 chrome/browser/extensions/extension_settings_api.cc:62: backend->TriggerOnSettingsChanged( On 2011/10/07 22:39:52, Matt Perry wrote: > It ...

9 years, 2 months ago (2011-10-10 01:00:16 UTC) #4

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_api.cc (right):

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_api.cc:62:
backend->TriggerOnSettingsChanged(
On 2011/10/07 22:39:52, Matt Perry wrote:
> It seems weird to leave it up to the consumer of ExtensionSettingsStorage to
> trigger the onChanged event. Why not have ExtensionSettingStorage itself
trigger
> it? Maybe Set() and Remove() could have implementations, and call through to
> SetImpl/RemoveImpl virtual methods, and fire the onChanged event afterwards.

I figured it wasn't so bad since there's only 2 sources of changes, sync (needs
to notify all background pages) and set/remove from one background page to the
other.  There will actually be a 3rd at some point (enterprise policy) and it
might get more complicated when content scripts get settings, whenever that is.

I thought about how to do this within the storage objects, since conceptually
it's nicer, as you say.  I don't really want to have to propagate a SetImpl and
RemoveImpl all the way through the interface of ExtensionSettingsStorage... what
would that mean for the storage implementations that don't notify?  That Set and
SetImpl (etc) do the same thing?

The reason I decided to do it this way is because the notifications logic needs
access to the Profile* for controlling which profile the notification goes to,
so this would need to be passed through the Set/Remove methods.  Probably with a
wrapping struct, but still.

My preferred way of doing this would be to add
SyncableExtensionSettingsStorage-only methods called like
SetAndNotify/RemoveAndNotify.  SyncableExtensionSettingStorage then becomes an
inaccurate name, it's more like the outer storage object which handles
side-effectful operations, but, whatever.

Which is why the simpler option I figured was this one.

Thoughts?

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_backend.h (right):

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_backend.h:30:
ObserverListThreadSafe<ExtensionSettingsObserver>* observers);
On 2011/10/07 21:06:23, akalin wrote:
> pass a const ref to scoped_refptr

Done.

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_frontend.cc (right):

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_frontend.cc:56:
observers_->AddObserver(observer);
On 2011/10/07 21:06:23, akalin wrote:
> DCHECK UI

Done.

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_frontend.cc:61:
observers_->RemoveObserver(observer);
On 2011/10/07 21:06:23, akalin wrote:
> DCHECK UI

Done.

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_frontend.cc:68: observers_->Notify(
On 2011/10/07 21:06:23, akalin wrote:
> DCHECK UI

Method deleted.

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_frontend.cc:102:
profile_observers_.erase(old_profile);
On 2011/10/07 21:06:23, akalin wrote:
> this is a leak.  Suggest using link_ptrs instead.

Ah.

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_frontend.h (right):

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_frontend.h:43: void
TriggerOnSettingsChanged(
On 2011/10/07 22:39:52, Matt Perry wrote:
> Is this actually used? I only see TriggerOnSettingsChanged being called on the
> backend.

Done.

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_frontend.h:59: explicit
Core(ObserverListThreadSafe<ExtensionSettingsObserver>* observers);
On 2011/10/07 21:06:23, akalin wrote:
> const ref to scoped_refptr

Done.

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_frontend.h:86: class
DefaultObserver : public ExtensionSettingsObserver {
On 2011/10/07 22:39:52, Matt Perry wrote:
> nit: move the implementation into the .cc file. you can just forward declare:
>  class DefaultObserver;

Done.

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_frontend.h:111: // MUST ONLY BE
ACCESSED ON THE UI THREAD.
On 2011/10/07 21:06:23, akalin wrote:
> this warning goes for any member of this class besides core_/observers_, no?

I think that observers_, core_, and registrar_ (fwiw) are thread safe, profile_
might be (but probably isn't -- but it's not even necessary to have as a member
variable, so moot).  Notably observers_ needs to be.

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_leveldb_storage.cc (right):

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:248:
base::JSONReader().JsonToValue(old_value_as_json, false, false);
On 2011/10/07 22:39:52, Matt Perry wrote:
> I don't understand this conversion to/from json. What's wrong with DeepCopy?

I don't understand... what conversion?

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/sy...
File chrome/browser/extensions/syncable_extension_settings_storage.cc (right):

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/sy...
chrome/browser/extensions/syncable_extension_settings_storage.cc:263:
errors.push_back(SyncError(
On 2011/10/07 21:06:23, akalin wrote:
> i don't know if this should be an error -- it's possible that someone could
add
> a key locally and an ADD could come in from sync immediately after, right?

Probably... maybe?  If somebody adds a key then it will be sent to sync
immediately; what does sync do if it gets a remote sync add after a local sync
add forn the same key?  Does it swallow the add?  Turn it into an update?  Just
send the add anyway?

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/sy...
chrome/browser/extensions/syncable_extension_settings_storage.cc:287: if
(!current_value.get()) {
On 2011/10/07 21:06:23, akalin wrote:
> same here -- local delete, update from sync.  maybe just drop and warn?  or
may
> be apply anyway. (prefer the latter as it doesn't destroy data)

Ditto.

Btw assuming sync *does* just send the add anyway, I'll apply it anyway and use
the result of changed_keys to determine whether a change should be appended to
change_event.

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/sy...
chrome/browser/extensions/syncable_extension_settings_storage.cc:313: if
(!current_value.get()) {
On 2011/10/07 21:06:23, akalin wrote:
> yeah, this should be a warning, too

Ditto.

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/sy...
File chrome/browser/extensions/syncable_extension_settings_storage.h (left):

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/sy...
chrome/browser/extensions/syncable_extension_settings_storage.h:21: std::string
extension_id,
On 2011/10/07 21:06:23, akalin wrote:
> const ref to string

Done.

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/sy...
File chrome/browser/extensions/syncable_extension_settings_storage.h (right):

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/sy...
chrome/browser/extensions/syncable_extension_settings_storage.h:24:
ObserverListThreadSafe<ExtensionSettingsObserver>* observers,
On 2011/10/07 21:06:23, akalin wrote:
> const ref to scoped_ptr

Done.

http://codereview.chromium.org/8177022/diff/4001/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/8177022/diff/4001/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:1028: "oldValue": {
On 2011/10/07 22:39:52, Matt Perry wrote:
> can you think of a use case for needing the old value? i would expect
extensions
> to only care:
> (a) that a particular key has changed, and/or
> (b) what the new value of the key is.

I would expect that too.  However... I made it this way because the HTML event
for localStorage changes has key/newValue/oldValue and I figured it can't hurt
to just match it.

I could imagine it helping with debugging for extension authors perhaps?

Matt Perry

lgtm http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/extension_settings_api.cc File chrome/browser/extensions/extension_settings_api.cc (right): http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/extension_settings_api.cc#newcode62 chrome/browser/extensions/extension_settings_api.cc:62: backend->TriggerOnSettingsChanged( On 2011/10/10 01:00:16, kalman wrote: > On ...

9 years, 2 months ago (2011-10-10 20:37:15 UTC) #5

lgtm

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_api.cc (right):

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_api.cc:62:
backend->TriggerOnSettingsChanged(
On 2011/10/10 01:00:16, kalman wrote:
> On 2011/10/07 22:39:52, Matt Perry wrote:
> > It seems weird to leave it up to the consumer of ExtensionSettingsStorage to
> > trigger the onChanged event. Why not have ExtensionSettingStorage itself
> trigger
> > it? Maybe Set() and Remove() could have implementations, and call through to
> > SetImpl/RemoveImpl virtual methods, and fire the onChanged event afterwards.
> 
> I figured it wasn't so bad since there's only 2 sources of changes, sync
(needs
> to notify all background pages) and set/remove from one background page to the
> other.  There will actually be a 3rd at some point (enterprise policy) and it
> might get more complicated when content scripts get settings, whenever that
is.
> 
> I thought about how to do this within the storage objects, since conceptually
> it's nicer, as you say.  I don't really want to have to propagate a SetImpl
and
> RemoveImpl all the way through the interface of ExtensionSettingsStorage...
what
> would that mean for the storage implementations that don't notify?  That Set
and
> SetImpl (etc) do the same thing?
> 
> The reason I decided to do it this way is because the notifications logic
needs
> access to the Profile* for controlling which profile the notification goes to,
> so this would need to be passed through the Set/Remove methods.  Probably with
a
> wrapping struct, but still.
> 
> My preferred way of doing this would be to add
> SyncableExtensionSettingsStorage-only methods called like
> SetAndNotify/RemoveAndNotify.  SyncableExtensionSettingStorage then becomes an
> inaccurate name, it's more like the outer storage object which handles
> side-effectful operations, but, whatever.
> 
> Which is why the simpler option I figured was this one.
> 
> Thoughts?

After thinking about it more, having the notification be separate from
Set/Remove allows you to aggregate several changes into 1 event, which looks
useful for sync. I'm OK with separating them this way.

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_leveldb_storage.cc (right):

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_leveldb_storage.cc:248:
base::JSONReader().JsonToValue(old_value_as_json, false, false);
On 2011/10/10 01:00:16, kalman wrote:
> On 2011/10/07 22:39:52, Matt Perry wrote:
> > I don't understand this conversion to/from json. What's wrong with DeepCopy?
> 
> I don't understand... what conversion?

old_value is the same as it->value()->DeepCopy(), but you convert it to and from
json first.

http://codereview.chromium.org/8177022/diff/4001/chrome/common/extensions/api...
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/8177022/diff/4001/chrome/common/extensions/api...
chrome/common/extensions/api/extension_api.json:1028: "oldValue": {
On 2011/10/10 01:00:16, kalman wrote:
> On 2011/10/07 22:39:52, Matt Perry wrote:
> > can you think of a use case for needing the old value? i would expect
> extensions
> > to only care:
> > (a) that a particular key has changed, and/or
> > (b) what the new value of the key is.
> 
> I would expect that too.  However... I made it this way because the HTML event
> for localStorage changes has key/newValue/oldValue and I figured it can't hurt
> to just match it.
> 
> I could imagine it helping with debugging for extension authors perhaps?

OK, fair enough.

not at google - send to devlin

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/extension_settings_leveldb_storage.cc File chrome/browser/extensions/extension_settings_leveldb_storage.cc (right): http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/extension_settings_leveldb_storage.cc#newcode248 chrome/browser/extensions/extension_settings_leveldb_storage.cc:248: base::JSONReader().JsonToValue(old_value_as_json, false, false); On 2011/10/10 20:37:15, Matt Perry wrote: ...

9 years, 2 months ago (2011-10-10 22:57:53 UTC) #6

Matt Perry

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/extension_settings_leveldb_storage.cc File chrome/browser/extensions/extension_settings_leveldb_storage.cc (right): http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/extension_settings_leveldb_storage.cc#newcode248 chrome/browser/extensions/extension_settings_leveldb_storage.cc:248: base::JSONReader().JsonToValue(old_value_as_json, false, false); On 2011/10/10 22:57:53, kalman wrote: > ...

9 years, 2 months ago (2011-10-10 23:01:17 UTC) #7

not at google - send to devlin

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/extension_settings_leveldb_storage.cc File chrome/browser/extensions/extension_settings_leveldb_storage.cc (right): http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/extension_settings_leveldb_storage.cc#newcode248 chrome/browser/extensions/extension_settings_leveldb_storage.cc:248: base::JSONReader().JsonToValue(old_value_as_json, false, false); On 2011/10/10 23:01:17, Matt Perry wrote: ...

9 years, 2 months ago (2011-10-11 03:58:47 UTC) #8

not at google - send to devlin

http://codereview.chromium.org/8177022/diff/14001/chrome/browser/extensions/extension_settings_apitest.cc File chrome/browser/extensions/extension_settings_apitest.cc (right): http://codereview.chromium.org/8177022/diff/14001/chrome/browser/extensions/extension_settings_apitest.cc#newcode177 chrome/browser/extensions/extension_settings_apitest.cc:177: ReplyWhenSatisfied("assertAddFooNotification", "assertNoNotifications"); On 2011/10/11 03:58:48, kalman wrote: > Hmmm, ...

9 years, 2 months ago (2011-10-12 05:43:29 UTC) #9

Aaron Boodman

On 2011/10/12 05:43:29, kalman wrote: > There might be something I can do on the ...

9 years, 2 months ago (2011-10-12 05:53:12 UTC) #10

Matt Perry

http://codereview.chromium.org/8177022/diff/24001/chrome/test/data/extensions/api_test/settings/split_incognito/background.html File chrome/test/data/extensions/api_test/settings/split_incognito/background.html (right): http://codereview.chromium.org/8177022/diff/24001/chrome/test/data/extensions/api_test/settings/split_incognito/background.html#newcode59 chrome/test/data/extensions/api_test/settings/split_incognito/background.html:59: retry("assertNoNotifications", callback); Polling is almost always a bad idea ...

9 years, 2 months ago (2011-10-12 17:23:01 UTC) #11

Matt Perry

On 2011/10/12 05:53:12, Aaron Boodman wrote: > On 2011/10/12 05:43:29, kalman wrote: > > There ...

9 years, 2 months ago (2011-10-12 17:23:54 UTC) #12

Aaron Boodman

On 2011/10/12 17:23:54, Matt Perry wrote: > On 2011/10/12 05:53:12, Aaron Boodman wrote: > > ...

9 years, 2 months ago (2011-10-12 18:30:20 UTC) #13

Matt Perry

On 2011/10/12 18:30:20, Aaron Boodman wrote: > On 2011/10/12 17:23:54, Matt Perry wrote: > > ...

9 years, 2 months ago (2011-10-12 18:33:43 UTC) #14

Aaron Boodman

On Wed, Oct 12, 2011 at 11:33 AM, <mpcomplete@chromium.org> wrote: > On 2011/10/12 18:30:20, Aaron ...

9 years, 2 months ago (2011-10-12 18:53:05 UTC) #15

akalin

http://codereview.chromium.org/8177022/diff/24001/chrome/browser/extensions/extension_settings_backend.cc File chrome/browser/extensions/extension_settings_backend.cc (right): http://codereview.chromium.org/8177022/diff/24001/chrome/browser/extensions/extension_settings_backend.cc#newcode91 chrome/browser/extensions/extension_settings_backend.cc:91: observers_.get(), just observers_ http://codereview.chromium.org/8177022/diff/24001/chrome/browser/extensions/extension_settings_backend.h File chrome/browser/extensions/extension_settings_backend.h (right): http://codereview.chromium.org/8177022/diff/24001/chrome/browser/extensions/extension_settings_backend.h#newcode78 chrome/browser/extensions/extension_settings_backend.h:78: ...

9 years, 2 months ago (2011-10-12 22:26:30 UTC) #16

not at google - send to devlin

Thanks again guys. Re the test retry stuff: Using RunAllPendingTasks() makes sense, and indeed seems ...

9 years, 2 months ago (2011-10-13 06:40:43 UTC) #17

Aaron Boodman

On 2011/10/13 06:40:43, kalman wrote: > Re the test retry stuff: Using RunAllPendingTasks() makes sense, ...

9 years, 2 months ago (2011-10-13 20:33:56 UTC) #18

akalin

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/syncable_extension_settings_storage.cc File chrome/browser/extensions/syncable_extension_settings_storage.cc (right): http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/syncable_extension_settings_storage.cc#newcode246 chrome/browser/extensions/syncable_extension_settings_storage.cc:246: std::string("Error getting current sync state for ") + On ...

9 years, 2 months ago (2011-10-13 20:40:41 UTC) #19

akalin

http://codereview.chromium.org/8177022/diff/24001/chrome/browser/extensions/extension_settings_observer.h File chrome/browser/extensions/extension_settings_observer.h (right): http://codereview.chromium.org/8177022/diff/24001/chrome/browser/extensions/extension_settings_observer.h#newcode50 chrome/browser/extensions/extension_settings_observer.h:50: const std::string& event_json) = 0; On 2011/10/13 06:40:43, kalman ...

9 years, 2 months ago (2011-10-13 20:45:15 UTC) #20

akalin

http://codereview.chromium.org/8177022/diff/24001/chrome/browser/extensions/extension_settings_observer.h File chrome/browser/extensions/extension_settings_observer.h (right): http://codereview.chromium.org/8177022/diff/24001/chrome/browser/extensions/extension_settings_observer.h#newcode16 chrome/browser/extensions/extension_settings_observer.h:16: // A list of setting changes. Use this class ...

9 years, 2 months ago (2011-10-13 20:45:57 UTC) #21

not at google - send to devlin

http://codereview.chromium.org/8177022/diff/24001/chrome/browser/extensions/extension_settings_observer.h File chrome/browser/extensions/extension_settings_observer.h (right): http://codereview.chromium.org/8177022/diff/24001/chrome/browser/extensions/extension_settings_observer.h#newcode16 chrome/browser/extensions/extension_settings_observer.h:16: // A list of setting changes. Use this class ...

9 years, 2 months ago (2011-10-13 23:08:14 UTC) #22

Matt Perry

http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/syncable_extension_settings_storage.cc File chrome/browser/extensions/syncable_extension_settings_storage.cc (right): http://codereview.chromium.org/8177022/diff/4001/chrome/browser/extensions/syncable_extension_settings_storage.cc#newcode246 chrome/browser/extensions/syncable_extension_settings_storage.cc:246: std::string("Error getting current sync state for ") + On ...

9 years, 2 months ago (2011-10-13 23:20:24 UTC) #23

akalin

On 2011/10/13 23:20:24, Matt Perry wrote: > I disagree. StringPrintf is perfectly typesafe if you ...

9 years, 2 months ago (2011-10-13 23:39:53 UTC) #24

not at google - send to devlin

On 2011/10/13 23:39:53, akalin wrote: > On 2011/10/13 23:20:24, Matt Perry wrote: > > I ...

9 years, 2 months ago (2011-10-14 00:22:03 UTC) #25

akalin

Few more structural comments http://codereview.chromium.org/8177022/diff/47001/chrome/browser/extensions/extension_settings_changes.cc File chrome/browser/extensions/extension_settings_changes.cc (right): http://codereview.chromium.org/8177022/diff/47001/chrome/browser/extensions/extension_settings_changes.cc#newcode16 chrome/browser/extensions/extension_settings_changes.cc:16: change->Set("key", Value::CreateStringValue(key)); can do SetString() ...

9 years, 2 months ago (2011-10-14 10:31:49 UTC) #26

not at google - send to devlin

http://codereview.chromium.org/8177022/diff/47001/chrome/browser/extensions/extension_settings_changes.cc File chrome/browser/extensions/extension_settings_changes.cc (right): http://codereview.chromium.org/8177022/diff/47001/chrome/browser/extensions/extension_settings_changes.cc#newcode16 chrome/browser/extensions/extension_settings_changes.cc:16: change->Set("key", Value::CreateStringValue(key)); On 2011/10/14 10:31:49, akalin wrote: > can ...

9 years, 2 months ago (2011-10-17 00:04:33 UTC) #27

http://codereview.chromium.org/8177022/diff/47001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_changes.cc (right):

http://codereview.chromium.org/8177022/diff/47001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_changes.cc:16: change->Set("key",
Value::CreateStringValue(key));
On 2011/10/14 10:31:49, akalin wrote:
> can do SetString()

Done.

http://codereview.chromium.org/8177022/diff/47001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_changes.h (right):

http://codereview.chromium.org/8177022/diff/47001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_changes.h:13: class
ExtensionSettingsChanges {
On 2011/10/14 10:31:49, akalin wrote:
> I think ExtensionSettingChanges is less awkward (you'll have to rename the
> files, too, sorry)

Done.

http://codereview.chromium.org/8177022/diff/47001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_changes.h:19: void AppendChange(
On 2011/10/14 10:31:49, akalin wrote:
> 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.

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.

http://codereview.chromium.org/8177022/diff/47001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_changes.h:32: ListValue changes;
On 2011/10/14 10:31:49, akalin wrote:
> make the destructor private

Done.

http://codereview.chromium.org/8177022/diff/47001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_observer.h (right):

http://codereview.chromium.org/8177022/diff/47001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_observer.h:18: // |profile| is the
profile of the event originator, or NULL if the event
On 2011/10/14 10:31:49, akalin wrote:
> is profile actually used, or is it only for disambiguation?  If the latter,
then
> you can make it a const Profile*

Only for disambiguation.  Done.

http://codereview.chromium.org/8177022/diff/47001/chrome/browser/extensions/s...
File chrome/browser/extensions/syncable_extension_settings_storage.cc (right):

http://codereview.chromium.org/8177022/diff/47001/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:380:
synced_keys_.insert(key);
On 2011/10/14 10:31:49, akalin wrote:
> should it really be put in synced_keys_ if delegate hits an error?

Yes, even if the delegate fails to write (e.g. leveldb write error), sync has
still indicated that this setting is being synced.  Synced_keys is only used for
determining whether a change should be an ADD or UPDATE (in
SendAddsOrUpdatesToSync).  It would actually be unnecessary if there was some
queryable API to sync that returned this information.

Come to think of it, there's a bit of a bug in SendDeletesToSync... it DCHECKs
that there's and entry in synced_keys_, but really it should be an if() with a
warning.  Fixed.

I'll add a comment to sync_keys, though... is there such an API?  It would
simplify code, and I believe sync does the checks anyway (but throws a DCHECK if
the wrong action is sent).

http://codereview.chromium.org/8177022/diff/47001/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:414:
synced_keys_.erase(key);
On 2011/10/14 10:31:49, akalin wrote:
> similarly, should it be erased if delegate hits an error?

See above.

akalin

http://codereview.chromium.org/8177022/diff/47001/chrome/browser/extensions/extension_settings_changes.h File chrome/browser/extensions/extension_settings_changes.h (right): http://codereview.chromium.org/8177022/diff/47001/chrome/browser/extensions/extension_settings_changes.h#newcode19 chrome/browser/extensions/extension_settings_changes.h:19: void AppendChange( On 2011/10/17 00:04:33, kalman wrote: > On ...

9 years, 2 months ago (2011-10-17 08:08:57 UTC) #28

not at google - send to devlin

http://codereview.chromium.org/8177022/diff/47001/chrome/browser/extensions/extension_settings_changes.h File chrome/browser/extensions/extension_settings_changes.h (right): http://codereview.chromium.org/8177022/diff/47001/chrome/browser/extensions/extension_settings_changes.h#newcode19 chrome/browser/extensions/extension_settings_changes.h:19: void AppendChange( On 2011/10/17 08:08:57, akalin wrote: > On ...

9 years, 2 months ago (2011-10-17 10:35:33 UTC) #29

akalin

Few more comments, I'll take a more detailed pass later today http://codereview.chromium.org/8177022/diff/55002/chrome/browser/extensions/extension_setting_changes.cc File chrome/browser/extensions/extension_setting_changes.cc (right): ...

9 years, 2 months ago (2011-10-17 17:37:38 UTC) #30

not at google - send to devlin

http://codereview.chromium.org/8177022/diff/55002/chrome/browser/extensions/extension_setting_changes.cc File chrome/browser/extensions/extension_setting_changes.cc (right): http://codereview.chromium.org/8177022/diff/55002/chrome/browser/extensions/extension_setting_changes.cc#newcode24 chrome/browser/extensions/extension_setting_changes.cc:24: std::string changes_json; On 2011/10/17 17:37:38, akalin wrote: > should ...

9 years, 2 months ago (2011-10-17 23:05:44 UTC) #31

Aaron Boodman

Drive-by: Gee, there sure are a lot of files related to this feature. Can you ...

9 years, 2 months ago (2011-10-18 04:32:08 UTC) #32

Aaron Boodman

http://codereview.chromium.org/8177022/diff/47001/chrome/browser/extensions/extension_settings_changes.h File chrome/browser/extensions/extension_settings_changes.h (right): http://codereview.chromium.org/8177022/diff/47001/chrome/browser/extensions/extension_settings_changes.h#newcode19 chrome/browser/extensions/extension_settings_changes.h:19: void AppendChange( Guys, I'm really sorry to add more ...

9 years, 2 months ago (2011-10-18 06:18:30 UTC) #33

Aaron Boodman

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/extension_settings_api.cc File chrome/browser/extensions/extension_settings_api.cc (right): http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/extension_settings_api.cc#newcode61 chrome/browser/extensions/extension_settings_api.cc:61: backend->TriggerOnSettingsChanged( We just came from backend. Why not have ...

9 years, 2 months ago (2011-10-18 06:22:39 UTC) #34

not at google - send to devlin

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/extension_settings_api.cc File chrome/browser/extensions/extension_settings_api.cc (right): http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/extension_settings_api.cc#newcode61 chrome/browser/extensions/extension_settings_api.cc:61: backend->TriggerOnSettingsChanged( On 2011/10/18 06:22:39, Aaron Boodman wrote: > We ...

9 years, 2 months ago (2011-10-18 06:26:55 UTC) #35

Aaron Boodman

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/extension_settings_api.cc File chrome/browser/extensions/extension_settings_api.cc (right): http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/extension_settings_api.cc#newcode61 chrome/browser/extensions/extension_settings_api.cc:61: backend->TriggerOnSettingsChanged( On 2011/10/18 06:26:56, kalman wrote: > On 2011/10/18 ...

9 years, 2 months ago (2011-10-18 06:53:01 UTC) #36

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_api.cc (right):

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_api.cc:61:
backend->TriggerOnSettingsChanged(
On 2011/10/18 06:26:56, kalman wrote:
> On 2011/10/18 06:22:39, Aaron Boodman wrote:
> > We just came from backend. Why not have it fire the change notification
> itself?
> > It seems weird for it to be way up here in the API implementation.
> 
> To do this the profile of the caller would need to be threaded through to the
> implementation.

It seems like ExtensionSettingsBackend could be passed a Profile* in its
constructor. I have seen the pattern elsewhere in Chrome of storing a Profile*
as a void* on non-UI threads so that it can be later passed back to the UI
thread or used for comparisons.

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_backend.cc (right):

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_backend.cc:80: storage = new
InMemoryExtensionSettingsStorage();
On 2011/10/18 06:26:56, kalman wrote:
> On 2011/10/18 06:18:30, Aaron Boodman wrote:
> > Why would this fail? Is it really OK to just silently ignore it?
> 
> If the hard drive fails, or NFS not available, or some other IO failure, or if
> leveldb has a mysterious bug?
> 
> I guess Chrome would have other issues to worry about in most of those cases,
> but yes I think it's acceptable to silently ignore it (the failure would have
> been logged in the ExtensionSettingsLeveldbStorage class, fwiw).

I see. And sync will repopulate it from the server? Ok, clever. I'm a little
concerned that this will mask real problems since it will seem to work to users,
just slower (and less efficiently for us). It also adds a teeny bit of
complexity which I assume is untested.

Also, as you say, if this fails, nothing else is working either.

not at google - send to devlin

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/extension_settings_api.cc File chrome/browser/extensions/extension_settings_api.cc (right): http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/extension_settings_api.cc#newcode61 chrome/browser/extensions/extension_settings_api.cc:61: backend->TriggerOnSettingsChanged( On 2011/10/18 06:53:01, Aaron Boodman wrote: > On ...

9 years, 2 months ago (2011-10-18 08:04:34 UTC) #37

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_api.cc (right):

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_api.cc:61:
backend->TriggerOnSettingsChanged(
On 2011/10/18 06:53:01, Aaron Boodman wrote:
> On 2011/10/18 06:26:56, kalman wrote:
> > On 2011/10/18 06:22:39, Aaron Boodman wrote:
> > > We just came from backend. Why not have it fire the change notification
> > itself?
> > > It seems weird for it to be way up here in the API implementation.
> > 
> > To do this the profile of the caller would need to be threaded through to
the
> > implementation.
> 
> It seems like ExtensionSettingsBackend could be passed a Profile* in its
> constructor. I have seen the pattern elsewhere in Chrome of storing a Profile*
> as a void* on non-UI threads so that it can be later passed back to the UI
> thread or used for comparisons.

There's already only one Backend per profile right, since it belongs to an
ExtensionService which belongs to a Profile?

This is to disambiguate between incognito and normal profiles, since there's one
ExtensionService per pair, I hope (it certainly seems that way).  So injecting
it on construction wouldn't help.

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_backend.cc (right):

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_backend.cc:80: storage = new
InMemoryExtensionSettingsStorage();
On 2011/10/18 06:53:01, Aaron Boodman wrote:
> On 2011/10/18 06:26:56, kalman wrote:
> > On 2011/10/18 06:18:30, Aaron Boodman wrote:
> > > Why would this fail? Is it really OK to just silently ignore it?
> > 
> > If the hard drive fails, or NFS not available, or some other IO failure, or
if
> > leveldb has a mysterious bug?
> > 
> > I guess Chrome would have other issues to worry about in most of those
cases,
> > but yes I think it's acceptable to silently ignore it (the failure would
have
> > been logged in the ExtensionSettingsLeveldbStorage class, fwiw).
> 
> I see. And sync will repopulate it from the server? Ok, clever. I'm a little
> concerned that this will mask real problems since it will seem to work to
users,
> just slower (and less efficiently for us). It also adds a teeny bit of
> complexity which I assume is untested.
> 
> Also, as you say, if this fails, nothing else is working either.

It's tested in that the in-memory implementation is tested in isolation.  This
code path never gets tested, which I presume is what you meant.

Is there a precedent for what to do here?  I didn't want to crash the browser,
that would seem wrong, and this seems to be the most transparent way for a
single extension to fail.  The other sensible thing would be to fail this single
operation and return NULL or something, but then all callers of GetStorage need
to worry about a NULL storage area... bleh.

Btw by slower do you mean slower to initialise?  Yes.  It will also not
instantaneously be correct, but appear to be empty until sync kicks in, which is
a shame.

akalin

> Do we really need a Builder and a ref-counted inner object thing to > ...

9 years, 2 months ago (2011-10-18 15:57:00 UTC) #38

akalin

On Tue, Oct 18, 2011 at 8:56 AM, Fred Akalin <akalin@chromium.org> wrote: > The Builder ...

9 years, 2 months ago (2011-10-18 16:42:44 UTC) #39

akalin

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/extension_setting_changes.h File chrome/browser/extensions/extension_setting_changes.h (right): http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/extension_setting_changes.h#newcode47 chrome/browser/extensions/extension_setting_changes.h:47: // Create using the Builder class. Say that |changes| ...

9 years, 2 months ago (2011-10-18 19:00:04 UTC) #40

Aaron Boodman

On Tue, Oct 18, 2011 at 8:56 AM, Fred Akalin <akalin@chromium.org> wrote: >> Do we ...

9 years, 2 months ago (2011-10-18 19:10:08 UTC) #41

Aaron Boodman

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/extension_settings_api.cc File chrome/browser/extensions/extension_settings_api.cc (right): http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/extension_settings_api.cc#newcode61 chrome/browser/extensions/extension_settings_api.cc:61: backend->TriggerOnSettingsChanged( On 2011/10/18 08:04:34, kalman wrote: > On 2011/10/18 ...

9 years, 2 months ago (2011-10-18 19:10:21 UTC) #42

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_api.cc (right):

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_api.cc:61:
backend->TriggerOnSettingsChanged(
On 2011/10/18 08:04:34, kalman wrote:
> On 2011/10/18 06:53:01, Aaron Boodman wrote:
> > On 2011/10/18 06:26:56, kalman wrote:
> > > On 2011/10/18 06:22:39, Aaron Boodman wrote:
> > > > We just came from backend. Why not have it fire the change notification
> > > itself?
> > > > It seems weird for it to be way up here in the API implementation.
> > > 
> > > To do this the profile of the caller would need to be threaded through to
> the
> > > implementation.
> > 
> > It seems like ExtensionSettingsBackend could be passed a Profile* in its
> > constructor. I have seen the pattern elsewhere in Chrome of storing a
Profile*
> > as a void* on non-UI threads so that it can be later passed back to the UI
> > thread or used for comparisons.
> 
> There's already only one Backend per profile right, since it belongs to an
> ExtensionService which belongs to a Profile?
> 
> This is to disambiguate between incognito and normal profiles, since there's
one
> ExtensionService per pair, I hope (it certainly seems that way).  So injecting
> it on construction wouldn't help.

Ok... ExtensionFunction has the profile*. It seems you could pass it down to
ExtensionBackend and trigger the event from there.

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_backend.cc (right):

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_backend.cc:80: storage = new
InMemoryExtensionSettingsStorage();
On 2011/10/18 08:04:34, kalman wrote:
> On 2011/10/18 06:53:01, Aaron Boodman wrote:
> > On 2011/10/18 06:26:56, kalman wrote:
> > > On 2011/10/18 06:18:30, Aaron Boodman wrote:
> > > > Why would this fail? Is it really OK to just silently ignore it?
> > > 
> > > If the hard drive fails, or NFS not available, or some other IO failure,
or
> if
> > > leveldb has a mysterious bug?
> > > 
> > > I guess Chrome would have other issues to worry about in most of those
> cases,
> > > but yes I think it's acceptable to silently ignore it (the failure would
> have
> > > been logged in the ExtensionSettingsLeveldbStorage class, fwiw).
> > 
> > I see. And sync will repopulate it from the server? Ok, clever. I'm a little
> > concerned that this will mask real problems since it will seem to work to
> users,
> > just slower (and less efficiently for us). It also adds a teeny bit of
> > complexity which I assume is untested.
> > 
> > Also, as you say, if this fails, nothing else is working either.
> 
> It's tested in that the in-memory implementation is tested in isolation.  This
> code path never gets tested, which I presume is what you meant.

Yes, that's what I meant.

> Is there a precedent for what to do here?  I didn't want to crash the browser,
> that would seem wrong, and this seems to be the most transparent way for a
> single extension to fail.  The other sensible thing would be to fail this
single
> operation and return NULL or something, but then all callers of GetStorage
need
> to worry about a NULL storage area... bleh.

I'm not sure. We certainly have lots of persistent storage in Chrome (bookmarks,
preferences, history, etc), you could look at what they do.

My expectation of the behavior in the case of NSF failing, etc, would be that
the extension developer gets an error from the API call.

I agree that having many callers having to worry about a NULL pointer will lead
to problems, but it seems that there should be ways to deal with that in one
place.

> Btw by slower do you mean slower to initialise?  Yes.  It will also not
> instantaneously be correct, but appear to be empty until sync kicks in, which
is
> a shame.

Yes, that is what I was getting at with "slower".

Sorry to be a bummer. You've done some really nice work here, and I did go back
and forth about this suggestion because substituting the in-memory storage is a
neat trick made possible by your design.

However, this strikes me as one of those things that will rarely get tested or
used. In a project as big as Chromium, rarely-used codepaths are certain to
eventually break. That's why I'm encouraging you to find a way to remove it
cleanly.

Aaron Boodman

On 2011/10/18 19:00:04, akalin wrote: > http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/extension_setting_changes.h > File chrome/browser/extensions/extension_setting_changes.h (right): > > http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/extension_setting_changes.h#newcode47 > ...

9 years, 2 months ago (2011-10-18 19:33:20 UTC) #43

On 2011/10/18 19:00:04, akalin wrote:
>
http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/e...
> File chrome/browser/extensions/extension_setting_changes.h (right):
> 
>
http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_setting_changes.h:47: // Create using the
> Builder class.
> Say that |changes| is cleared after this is called.
> 
>
http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/e...
> File chrome/browser/extensions/extension_settings_frontend.cc (right):
> 
>
http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/e...
> chrome/browser/extensions/extension_settings_frontend.cc:99:
> registrar_.Add(this, chrome::NOTIFICATION_PROFILE_CREATED,
> Wait, you're listening to creation of *any* profile?  That seems wrong, as
then
> you'll get notifications across different profiles.  Are you doing this for
> normal vs. incognito?  This seems like the wrong way to approach this.
> 
> Wouldn't a better way be to store the regular profile and the incognito
profile
> in the constructor?  Then wouldn't you remove the need to pass around the
> Profile* from the api/backend, etc.?

After looking at the code more carefully I take back this comment. I think the
current impl is good.

Fred, the relationship between extensions and incognito is unfortunately subtle
and complex. Extensions are usually cross-incognito. The same extension process
is shared between background page is shared between incognito windows and normal
windows. However they can sometimes be "split", in which case a different
instance of the extension is used in incognito and normal windows.

The design of the settings API is for the storage to be shared between these
split-mode extensions. Ben is trying to broadcast change notifications across
incognito/normal here, but he wants to avoid notifying the extension instance
that is responsible for the change.

Ben: did you consider not having ExtensionService own this guy? ExtensionService
is already so large and hard to test. Is there any reason to not just let
Profile own it? Profile already owns several extension-related service thingers.

akalin

> I understand that the ref-counted inner objects are meant to avoid > copies. Do ...

9 years, 2 months ago (2011-10-18 19:54:31 UTC) #44

Aaron Boodman

On Tue, Oct 18, 2011 at 12:54 PM, Fred Akalin <akalin@chromium.org> wrote: >> I understand ...

9 years, 2 months ago (2011-10-18 20:30:51 UTC) #45

akalin

> Fred, the relationship between extensions and incognito is unfortunately > subtle > and complex. ...

9 years, 2 months ago (2011-10-18 20:52:24 UTC) #46

not at google - send to devlin

Christ. Ok. Let me try to address all of these, paraphrased by reading through what ...

9 years, 2 months ago (2011-10-19 01:03:56 UTC) #47

Christ.  Ok.  Let me try to address all of these, paraphrased by reading through
what I have on gmail.


* On the overengineering or otherwise of the Builder, refcounting, all that:

If you want a typed threadsafe wrapper around an immutable ListValue, then this
is what you get.  It's not great and feels wrong, but it's that or static method
calls building up a correct ListValue for the event, with post-validation if
you're feeling conservative/rigorous, and I don't like that.

Or, go back to what I had in the first place and pass around a const string. 
It's simpler and I prefer it.  Can I please move on?

(On the matter of efficiency -- there will be at least one potentially large
event, the first time Chrome is synced if there is a large amount of data.  It
will be at most 100k since that's what the quota is, though, as Fred mentioned. 
I think that's negligible.)


* On Profile destruction/creation:

Thanks for seeing that listening-to-all-profiles bug, it was rather bad.  Yes
this is for events across normal/incognito version of the same profile.  As for
storing the incognito profile on construction: it needs to be this way (via
events) since the incognito profile is destroyed/create all the time with
different values, so it's not a constant.  I've fixed the code.


* On triggering the events from the API class:

I'm not sure what you're suggesting here.  Just move the event triggering to
ExtensionSettingsBackend but build the event from the API?  Wrap the
Set/Remove/Clear methods in ExtensionSettingsBackend to also accept a profile
and trigger an event from there?  Pass the Profile* down through
Set/Remove/Clear?  They all seem like unnecessary complexity... am I
misunderstanding something?


* On GetStorage() failures for leveldb:

It makes sense to just propagate the failure.  The only thing that worries me is
what happens if the failure is the GetStorage that sync makes to populate the
storage?  Just... nothing gets synced?  Here are a few consistent options:
 1 - Keep the sync data around in memory and if the storage area does manage to
be created, populate it then.  This has implications for what to tell Sync, but
can figure that out later.
 2 - Just fail sync.  Since sync is now no longer consistent, disable sync
entirely for this storage area.
 3 - Have an InvalidExtensionSettingsStorage or something, which always fails
(kind of the opposite for InMemoryExtesnionSettingStorage which obviously always
succeeds).
 4 - ... there are many options.


* On split mode extensions:

Aaron... I'm confused, is there a separate ExtensionFunction per background page
if in split mode, or is it up to the ExntesionFunction to differentiate between
the two based on the value of profile()?  If the former... there are probably
bugs... but testing this, seems to not be the case?


* On Profile owning the settings:

Having ExtensionService own it was an arbitrary decision because other
extension-related stuff belongs to the ExtensionService.  No particular reason. 
I can change this.



... I will now address the inline comments in the code, as I find them.  After
lunch.

not at google - send to devlin

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/extension_setting_changes.h File chrome/browser/extensions/extension_setting_changes.h (right): http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/extension_setting_changes.h#newcode47 chrome/browser/extensions/extension_setting_changes.h:47: // Create using the Builder class. On 2011/10/18 19:00:04, ...

9 years, 2 months ago (2011-10-19 03:14:32 UTC) #48

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_setting_changes.h (right):

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_changes.h:47: // Create using the
Builder class.
On 2011/10/18 19:00:04, akalin wrote:
> Say that |changes| is cleared after this is called.

Done.

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_api.cc (right):

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_api.cc:61:
backend->TriggerOnSettingsChanged(
On 2011/10/18 19:10:22, Aaron Boodman wrote:
> On 2011/10/18 08:04:34, kalman wrote:
> > On 2011/10/18 06:53:01, Aaron Boodman wrote:
> > > On 2011/10/18 06:26:56, kalman wrote:
> > > > On 2011/10/18 06:22:39, Aaron Boodman wrote:
> > > > > We just came from backend. Why not have it fire the change
notification
> > > > itself?
> > > > > It seems weird for it to be way up here in the API implementation.
> > > > 
> > > > To do this the profile of the caller would need to be threaded through
to
> > the
> > > > implementation.
> > > 
> > > It seems like ExtensionSettingsBackend could be passed a Profile* in its
> > > constructor. I have seen the pattern elsewhere in Chrome of storing a
> Profile*
> > > as a void* on non-UI threads so that it can be later passed back to the UI
> > > thread or used for comparisons.
> > 
> > There's already only one Backend per profile right, since it belongs to an
> > ExtensionService which belongs to a Profile?
> > 
> > This is to disambiguate between incognito and normal profiles, since there's
> one
> > ExtensionService per pair, I hope (it certainly seems that way).  So
injecting
> > it on construction wouldn't help.
> 
> Ok... ExtensionFunction has the profile*. It seems you could pass it down to
> ExtensionBackend and trigger the event from there.

See my overall comment.  Not sure what exactly you're suggesting.

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_frontend.cc (right):

http://codereview.chromium.org/8177022/diff/61001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_frontend.cc:99:
registrar_.Add(this, chrome::NOTIFICATION_PROFILE_CREATED,
On 2011/10/18 19:00:04, akalin wrote:
> Wait, you're listening to creation of *any* profile?  That seems wrong, as
then
> you'll get notifications across different profiles.

Thanks, fixed.

> Are you doing this for
> normal vs. incognito?  This seems like the wrong way to approach this.
> 
> Wouldn't a better way be to store the regular profile and the incognito
profile
> in the constructor?  Then wouldn't you remove the need to pass around the
> Profile* from the api/backend, etc.?

As I mentioned in the overall comment, the incognito profile isn't constant so
can't store it in the constructor.

And I would still need to pass it through the events so that the profiles can be
distinguished from each other.

Aaron Boodman

On Tue, Oct 18, 2011 at 6:03 PM, <kalman@chromium.org> wrote: > * On triggering the ...

9 years, 2 months ago (2011-10-19 03:38:18 UTC) #49

On Tue, Oct 18, 2011 at 6:03 PM,  <kalman@chromium.org> wrote:
> * On triggering the events from the API class:
>
> I'm not sure what you're suggesting here.  Just move the event triggering to
> ExtensionSettingsBackend but build the event from the API?  Wrap the
> Set/Remove/Clear methods in ExtensionSettingsBackend to also accept a
> profile
> and trigger an event from there?  Pass the Profile* down through
> Set/Remove/Clear?  They all seem like unnecessary complexity... am I
> misunderstanding something?

I retracted this suggestion.

I was originally suggesting that a lower-level class, such as maybe
one of the SettingsStorage subclasses could generate change events
since it is the one who ultimately knows when something changes. That
way you won't have the trigger code in two places. I can't remember
what convinced me to give up on this, but oh well.

> * On GetStorage() failures for leveldb:
>
> It makes sense to just propagate the failure.  The only thing that worries
> me is
> what happens if the failure is the GetStorage that sync makes to populate
> the
> storage?  Just... nothing gets synced?  Here are a few consistent options:
>  1 - Keep the sync data around in memory and if the storage area does manage
> to
> be created, populate it then.  This has implications for what to tell Sync,
> but
> can figure that out later.
>  2 - Just fail sync.  Since sync is now no longer consistent, disable sync
> entirely for this storage area.
>  3 - Have an InvalidExtensionSettingsStorage or something, which always
> fails
> (kind of the opposite for InMemoryExtesnionSettingStorage which obviously
> always
> succeeds).
>  4 - ... there are many options.

I don't understand how sync works enough to make a suggestion here. 2)
is what I would expect feature-wise. Can you explain your consistency
concern in more detail?

> * On split mode extensions:
>
> Aaron... I'm confused, is there a separate ExtensionFunction per background
> page
> if in split mode, or is it up to the ExntesionFunction to differentiate
> between
> the two based on the value of profile()?  If the former... there are
> probably
> bugs... but testing this, seems to not be the case?

I wasn't trying to point out a bug wrt split mode, just explaining to
Fred the background (which it turns out he already knew).

To answer your question: In split mode, an extension will have two
completely separate instances. Two processes, two background pages,
etc. When an ExtensionFunction is invoked in spanning mode, its
profile() will always be the normal profile. When an ExtensionFunction
is invoked in split mode its profile() will either be incognito or
normal profile, depending on which type of process the extension
instance is running in.

> * On Profile owning the settings:
>
> Having ExtensionService own it was an arbitrary decision because other
> extension-related stuff belongs to the ExtensionService.  No particular
> reason.
> I can change this.

Thanks.

- a

not at google - send to devlin

On 2011/10/19 03:38:18, Aaron Boodman wrote: > On Tue, Oct 18, 2011 at 6:03 PM, ...

9 years, 2 months ago (2011-10-19 05:00:15 UTC) #50

On 2011/10/19 03:38:18, Aaron Boodman wrote:
> On Tue, Oct 18, 2011 at 6:03 PM,  <mailto:kalman@chromium.org> wrote:
> > * On triggering the events from the API class:
> >
> > I'm not sure what you're suggesting here. &nbsp;Just move the event
triggering to
> > ExtensionSettingsBackend but build the event from the API? &nbsp;Wrap the
> > Set/Remove/Clear methods in ExtensionSettingsBackend to also accept a
> > profile
> > and trigger an event from there? &nbsp;Pass the Profile* down through
> > Set/Remove/Clear? &nbsp;They all seem like unnecessary complexity... am I
> > misunderstanding something?
> 
> I retracted this suggestion.
> 
> I was originally suggesting that a lower-level class, such as maybe
> one of the SettingsStorage subclasses could generate change events
> since it is the one who ultimately knows when something changes. That
> way you won't have the trigger code in two places. I can't remember
> what convinced me to give up on this, but oh well.

I'm sure there is a way that is more logical to do this.  I agree that it would
be nicer to only be triggering the events from a single place.

> 
> > * On GetStorage() failures for leveldb:
> >
> > It makes sense to just propagate the failure. &nbsp;The only thing that
worries
> > me is
> > what happens if the failure is the GetStorage that sync makes to populate
> > the
> > storage? &nbsp;Just... nothing gets synced? &nbsp;Here are a few consistent
options:
> > &nbsp;1 - Keep the sync data around in memory and if the storage area does
manage
> > to
> > be created, populate it then. &nbsp;This has implications for what to tell
Sync,
> > but
> > can figure that out later.
> > &nbsp;2 - Just fail sync. &nbsp;Since sync is now no longer consistent,
disable sync
> > entirely for this storage area.
> > &nbsp;3 - Have an InvalidExtensionSettingsStorage or something, which always
> > fails
> > (kind of the opposite for InMemoryExtesnionSettingStorage which obviously
> > always
> > succeeds).
> > &nbsp;4 - ... there are many options.
> 
> I don't understand how sync works enough to make a suggestion here. 2)
> is what I would expect feature-wise. Can you explain your consistency
> concern in more detail?

Explained offline.

The error handling is something that has implications at the moment, not just in
this patch.  I've filed a bug.

> 
> > * On split mode extensions:
> >
> > Aaron... I'm confused, is there a separate ExtensionFunction per background
> > page
> > if in split mode, or is it up to the ExntesionFunction to differentiate
> > between
> > the two based on the value of profile()? &nbsp;If the former... there are
> > probably
> > bugs... but testing this, seems to not be the case?
> 
> I wasn't trying to point out a bug wrt split mode, just explaining to
> Fred the background (which it turns out he already knew).
> 
> To answer your question: In split mode, an extension will have two
> completely separate instances. Two processes, two background pages,
> etc. When an ExtensionFunction is invoked in spanning mode, its
> profile() will always be the normal profile. When an ExtensionFunction
> is invoked in split mode its profile() will either be incognito or
> normal profile, depending on which type of process the extension
> instance is running in.
> 
> 
> > * On Profile owning the settings:
> >
> > Having ExtensionService own it was an arbitrary decision because other
> > extension-related stuff belongs to the ExtensionService. &nbsp;No particular
> > reason.
> > I can change this.
> 
> Thanks.

Bug filed.


And in other news -- that windows integration test is failing on the trybots
again.  Uhhhhh.

akalin

Some more comments for ExtensionSettingsFrontend. I think I'll recuse myself from reviewing everything but ExtensionSettingsFrontend. ...

9 years, 2 months ago (2011-10-19 19:09:49 UTC) #51

not at google - send to devlin

http://codereview.chromium.org/8177022/diff/70004/chrome/browser/extensions/extension_settings_frontend.cc File chrome/browser/extensions/extension_settings_frontend.cc (right): http://codereview.chromium.org/8177022/diff/70004/chrome/browser/extensions/extension_settings_frontend.cc#newcode27 chrome/browser/extensions/extension_settings_frontend.cc:27: const ExtensionSettingChanges& changes) OVERRIDE { On 2011/10/19 19:09:49, akalin ...

9 years, 2 months ago (2011-10-19 22:59:36 UTC) #52

akalin

http://codereview.chromium.org/8177022/diff/70004/chrome/browser/extensions/extension_settings_frontend.cc File chrome/browser/extensions/extension_settings_frontend.cc (right): http://codereview.chromium.org/8177022/diff/70004/chrome/browser/extensions/extension_settings_frontend.cc#newcode27 chrome/browser/extensions/extension_settings_frontend.cc:27: const ExtensionSettingChanges& changes) OVERRIDE { On 2011/10/19 22:59:36, kalman ...

9 years, 2 months ago (2011-10-19 23:17:00 UTC) #53

not at google - send to devlin

http://codereview.chromium.org/8177022/diff/70004/chrome/browser/extensions/extension_settings_frontend.cc File chrome/browser/extensions/extension_settings_frontend.cc (right): http://codereview.chromium.org/8177022/diff/70004/chrome/browser/extensions/extension_settings_frontend.cc#newcode27 chrome/browser/extensions/extension_settings_frontend.cc:27: const ExtensionSettingChanges& changes) OVERRIDE { On 2011/10/19 23:17:00, akalin ...

9 years, 2 months ago (2011-10-19 23:47:34 UTC) #54

Matt Perry

On 2011/10/19 23:55:26, akalin wrote: > Okay LGTM. I leave the rest to Aaron/Matt. I ...

9 years, 2 months ago (2011-10-20 00:07:21 UTC) #56

not at google - send to devlin

On 2011/10/20 00:07:21, Matt Perry wrote: > On 2011/10/19 23:55:26, akalin wrote: > > Okay ...

9 years, 2 months ago (2011-10-20 00:35:56 UTC) #57

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kalman@chromium.org/8177022/87031

9 years, 2 months ago (2011-10-21 00:45:09 UTC) #58

Change committed as 106660

Issue 8177022: Add onChanged events to the extension settings API, both from sync and between (Closed)

Description

Patch Set 1 #

Patch Set 2 : Update HTML docs #

Patch Set 3 : . #

Patch Set 4 : comments #1 #

Patch Set 5 : Fix split_incognito API test for windows #

Patch Set 6 : . #

Patch Set 7 : . #

Patch Set 8 : comments #

Patch Set 9 : . #

Patch Set 10 : extension_settings_changes #

Patch Set 11 : sync (post clear-on-deletion patch) #

Patch Set 12 : sync again... #

Patch Set 13 : comments #

Patch Set 14 : win build #

Patch Set 15 : win build #2 #

Patch Set 16 : changes return string #

Patch Set 17 : comments #

Patch Set 18 : profile fix, comments #

Patch Set 19 : comments #

Patch Set 20 : . #

Patch Set 21 : message loop test fix #

Patch Set 22 : . #

Patch Set 23 : sync past more content breakages #

Patch Set 24 : . #

Patch Set 25 : just disable the test on windows :( #

Patch Set 26 : use correct bug number, mark as FLAKYL #

Patch Set 27 : sync #

Messages