Enable sync for the settings from the Extension Settings API.

Enable sync for the settings from the Extension Settings API.

BUG=94575
TEST=unit tests included

[not at google - send to devlin, 2011-08-29 09:07:44]

(This is the non-WIP version of http://codereview.chromium.org/7747043/)

http://codereview.chromium.org/7775008/diff/2001/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

http://codereview.chromium.org/7775008/diff/2001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:489: LOG(ERROR) <<
"Unacknowledged extension settings.  TODO(kalman): what?";
TODO(kalman) indeed.  I presume something more than this will need to be done.

[akalin, 2011-08-31 07:49:34]

Before I look too deeply, one big question.

http://codereview.chromium.org/7775008/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_setting_sync_data.h (right):

http://codereview.chromium.org/7775008/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_setting_sync_data.h:26: explicit
ExtensionSettingSyncData(
no need for explicit here

http://codereview.chromium.org/7775008/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7775008/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings.cc:175: // Not possible to do this
synchronously, or while satisfying const, but only
Explain to me again why the sync methods can't live on the native thread (i.e.
the FILE thread)?  You're breaking a lot of assumptions sync makes by subverting
it this way, and in general it's hard to reason clearly about classes that
thread-jump like this.

Btw, one of the use cases for GetAllSyncData() is (eventually) DCHECKing that
local_service->GetAllSyncData() == remote_service->GetAllSyncData() after every
call to ProcessSyncChanges.  So having this returning nothing is already a
non-starter.

http://codereview.chromium.org/7775008/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings.cc:295: new
ExtensionSettingSyncDataList(it->second)));
this may leak.  Just pass by value.

[Ben Olmstead, 2011-08-31 17:36:43]

Haven't had a chance to go over this in detail, but given Fred's concerns, I'm
going to hold off on doing so.

http://codereview.chromium.org/7775008/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_sync_helper.h (right):

http://codereview.chromium.org/7775008/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_sync_helper.h:14: class
ExtensionSettingsSyncHelper {
We usually just use a namespace for a collection of functions, not a class full
of statics.

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

http://codereview.chromium.org/7775008/diff/2001/chrome/browser/extensions/sy...
chrome/browser/extensions/syncable_extension_settings_storage.h:50: virtual void
Get(const std::string& key, Callback* callback) OVERRIDE;
No need for virtual and OVERRIDE together, just OVERRIDE.

http://codereview.chromium.org/7775008/diff/2001/chrome/browser/ui/webui/ntp/...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

http://codereview.chromium.org/7775008/diff/2001/chrome/browser/ui/webui/ntp/...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:478: // scale indefinitely
(we'd need N*(N+1)/2 different strings to represent
N*(N+1)/2 should be 2^N-1

[akalin, 2011-08-31 21:12:51]

+atwilson, who may have an opinion re. the NTP string stuff

Also, we should probably talk about what to do re. the split between
extensions/apps, and how that affects extension settings.  Do we also need a
separate app settings type, or can we get away with something else?

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

http://codereview.chromium.org/7775008/diff/2001/chrome/browser/extensions/sy...
chrome/browser/extensions/syncable_extension_settings_storage.h:50: virtual void
Get(const std::string& key, Callback* callback) OVERRIDE;
On 2011/08/31 17:36:43, Ben Olmstead wrote:
> No need for virtual and OVERRIDE together, just OVERRIDE.

Don't we always want 'virtual' for clarity?  Since OVERRIDE may evaluate to
nothing, I'd prefer keeping it.

[Ben Olmstead, 2011-08-31 21:19:42]

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

http://codereview.chromium.org/7775008/diff/2001/chrome/browser/extensions/sy...
chrome/browser/extensions/syncable_extension_settings_storage.h:50: virtual void
Get(const std::string& key, Callback* callback) OVERRIDE;
On 2011/08/31 21:12:51, akalin wrote:
> On 2011/08/31 17:36:43, Ben Olmstead wrote:
> > No need for virtual and OVERRIDE together, just OVERRIDE.
> 
> Don't we always want 'virtual' for clarity?  Since OVERRIDE may evaluate to
> nothing, I'd prefer keeping it.

For some reason, I thought we didn't use the two together, but a quick grep of
the code proves me wrong.

[not at google - send to devlin, 2011-09-02 05:00:40]

http://codereview.chromium.org/7775008/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_setting_sync_data.h (right):

http://codereview.chromium.org/7775008/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_setting_sync_data.h:26: explicit
ExtensionSettingSyncData(
On 2011/08/31 07:49:34, akalin wrote:
> no need for explicit here

Done.

http://codereview.chromium.org/7775008/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7775008/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings.cc:175: // Not possible to do this
synchronously, or while satisfying const, but only
On 2011/08/31 07:49:34, akalin wrote:
> Explain to me again why the sync methods can't live on the native thread (i.e.
> the FILE thread)?  You're breaking a lot of assumptions sync makes by
subverting
> it this way, and in general it's hard to reason clearly about classes that
> thread-jump like this.

The extension settings code already thread jumps because it needs to go to the
FILE thread for the leveldb stuff, but the extension API handling code lives on
the UI thread.

As for why the sync code lives on the UI thread, it mostly comes down to not
realising that it needs to be on the FILE thread (fwiw it runs fine?).  Also the
way I wrote the code, to avoid bloating up a single file with all the settings
functionality, was to implement the cache/sync/quota(eventually) layers as
wrappers i.e. decorator pattern.  Putting the sync code on the FILE thread would
require entirely subverting this.

> 
> Btw, one of the use cases for GetAllSyncData() is (eventually) DCHECKing that
> local_service->GetAllSyncData() == remote_service->GetAllSyncData() after
every
> call to ProcessSyncChanges.  So having this returning nothing is already a
> non-starter.

Hm ok.  Well I was thinking about this and there are two ways to fix it.
1. pre-load all the extension settings in memory so that this function can
access some cache and return the correct data.  Kind of a bad solution since it
makes startup more complicated, plus there might be race condition issues.
2. assume that GetAllSyncData is run on the FILE thread (could check this now
but will write comment instead, it's mostly a moot point) and read everything
from leveldb synchronously.

The latter is more palatable, but it all depends on where the code needs to go.

http://codereview.chromium.org/7775008/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings.cc:295: new
ExtensionSettingSyncDataList(it->second)));
On 2011/08/31 07:49:34, akalin wrote:
> this may leak.  Just pass by value.

How can it leak?

But passing by value works, so, done.

http://codereview.chromium.org/7775008/diff/2001/chrome/browser/extensions/ex...
File chrome/browser/extensions/extension_settings_sync_helper.h (right):

http://codereview.chromium.org/7775008/diff/2001/chrome/browser/extensions/ex...
chrome/browser/extensions/extension_settings_sync_helper.h:14: class
ExtensionSettingsSyncHelper {
On 2011/08/31 17:36:43, Ben Olmstead wrote:
> We usually just use a namespace for a collection of functions, not a class
full
> of statics.

Done.

Also renamed helper -> util since that seems more common in chromium.

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

http://codereview.chromium.org/7775008/diff/2001/chrome/browser/extensions/sy...
chrome/browser/extensions/syncable_extension_settings_storage.h:50: virtual void
Get(const std::string& key, Callback* callback) OVERRIDE;
On 2011/08/31 21:19:43, Ben Olmstead wrote:
> On 2011/08/31 21:12:51, akalin wrote:
> > On 2011/08/31 17:36:43, Ben Olmstead wrote:
> > > No need for virtual and OVERRIDE together, just OVERRIDE.
> > 
> > Don't we always want 'virtual' for clarity?  Since OVERRIDE may evaluate to
> > nothing, I'd prefer keeping it.
> 
> For some reason, I thought we didn't use the two together, but a quick grep of
> the code proves me wrong.

OVERRIDE and virtual are separate things, yes.

[akalin, 2011-09-02 05:43:56]

> The extension settings code already thread jumps because it needs to go
> to the FILE thread for the leveldb stuff, but the extension API handling
> code lives on the UI thread.
>
> As for why the sync code lives on the UI thread, it mostly comes down to
> not realising that it needs to be on the FILE thread (fwiw it runs
> fine?).  Also the way I wrote the code, to avoid bloating up a single
> file with all the settings functionality, was to implement the
> cache/sync/quota(eventually) layers as wrappers i.e. decorator pattern.
> Putting the sync code on the FILE thread would require entirely
> subverting this.

Hmm.  Can you do something like wrap the settings code that lives in
the FILE thread with the sync code such the the interface presented to
the UI thread remains the same?  That is, you'll have some layers on
the FILE thread (leveldb, sync) and the other layers on the UI thread
(cache/quota/extension API).

That would be preferable from the sync side.

> Hm ok.  Well I was thinking about this and there are two ways to fix it.
> 1. pre-load all the extension settings in memory so that this function
> can access some cache and return the correct data.  Kind of a bad
> solution since it makes startup more complicated, plus there might be
> race condition issues.
> 2. assume that GetAllSyncData is run on the FILE thread (could check
> this now but will write comment instead, it's mostly a moot point) and
> read everything from leveldb synchronously.
>
> The latter is more palatable, but it all depends on where the code needs
> to go.

Yeah, putting the sync code in the FILE thread and doing #2 would be preferable.

> How can it leak?
>
> But passing by value works, so, done.

Posting a callback is no guarantee that it will run, so if it's not
run, the argument will never be deleted.

Sorry about blocking you at this late stage. :( But I think having the
sync code live in the FILE thread will end up being better in the long
run.

[not at google - send to devlin, 2011-09-02 06:12:13]

That's fine, it... shouldn't be too hard to change.

It's basically the weekend though so I won't have time until next week (and I
was away for the past 3 days hence the lack of activity).

[not at google - send to devlin, 2011-09-10 19:41:07]

Ok, this is ready for another look.

It has changed drastically so there is no point looking at the diffs to the
earlier version.

[akalin, 2011-09-13 23:04:44]

On 2011/09/10 19:41:07, kalman1 wrote:
> Ok, this is ready for another look.
> 
> It has changed drastically so there is no point looking at the diffs to the
> earlier version.

Sorry, I was on sick leave until today.  I'll try looking at this today.

[akalin, 2011-09-14 05:57:29]

Some initial comments.  I suspect you may have to do more refactoring to handle
those, so I'll take a closer look until after you do.

http://codereview.chromium.org/7775008/diff/20003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7775008/diff/20003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:129: if
(!BrowserThread::CurrentlyOn(BrowserThread::FILE)) {
ExtensionSettings lives on the FILE thread right?  If so, this should be a
DCHECK

http://codereview.chromium.org/7775008/diff/20003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:174: BrowserThread::PostTask(
Remove this and DCHECK that you're on FILE thread.  Merge with
MergeDataAndStartSyncing2

http://codereview.chromium.org/7775008/diff/20003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:243: if
(BrowserThread::CurrentlyOn(BrowserThread::FILE)) {
Here, too

http://codereview.chromium.org/7775008/diff/20003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:286: // Re-run ths method on the
FILE thread.
DCHECK, etc.

http://codereview.chromium.org/7775008/diff/20003/chrome/browser/sync/glue/ex...
File chrome/browser/sync/glue/extension_setting_data_type_controller.h (right):

http://codereview.chromium.org/7775008/diff/20003/chrome/browser/sync/glue/ex...
chrome/browser/sync/glue/extension_setting_data_type_controller.h:16: class
ExtensionSettingDataTypeController : public FrontendDataTypeController {
So this should be a NonFrontendDataTypeController, now that it lives on the FILE
thread

[not at google - send to devlin, 2011-09-14 20:23:57]

Most recent version of this patch, uploaded against
http://codereview.chromium.org/7891054/

http://codereview.chromium.org/7775008/diff/20003/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7775008/diff/20003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:129: if
(!BrowserThread::CurrentlyOn(BrowserThread::FILE)) {
On 2011/09/14 05:57:29, akalin wrote:
> ExtensionSettings lives on the FILE thread right?  If so, this should be a
> DCHECK

Done.

http://codereview.chromium.org/7775008/diff/20003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:174: BrowserThread::PostTask(
On 2011/09/14 05:57:29, akalin wrote:
> Remove this and DCHECK that you're on FILE thread.  Merge with
> MergeDataAndStartSyncing2

Done.

http://codereview.chromium.org/7775008/diff/20003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:243: if
(BrowserThread::CurrentlyOn(BrowserThread::FILE)) {
On 2011/09/14 05:57:29, akalin wrote:
> Here, too

Done.

http://codereview.chromium.org/7775008/diff/20003/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:286: // Re-run ths method on the
FILE thread.
On 2011/09/14 05:57:29, akalin wrote:
> DCHECK, etc.

Done.

http://codereview.chromium.org/7775008/diff/20003/chrome/browser/sync/glue/ex...
File chrome/browser/sync/glue/extension_setting_data_type_controller.h (right):

http://codereview.chromium.org/7775008/diff/20003/chrome/browser/sync/glue/ex...
chrome/browser/sync/glue/extension_setting_data_type_controller.h:16: class
ExtensionSettingDataTypeController : public FrontendDataTypeController {
On 2011/09/14 05:57:29, akalin wrote:
> So this should be a NonFrontendDataTypeController, now that it lives on the
FILE
> thread

That would explain why the DCHECKs I originally had re FILE thread were failing.

[Ben Olmstead, 2011-09-14 20:33:24]

http://codereview.chromium.org/7775008/diff/27002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7775008/diff/27002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:137: const ExtensionList*
extensions = extension_service_->extensions();
You will also need to go through the disabled_extensions and
terminated_extensions lists.

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

http://codereview.chromium.org/7775008/diff/27002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:229: Result
result = delegate_->Remove(it->key());
Are there any issues with removing a nonexistent key?

[akalin, 2011-09-14 21:08:52]

This is looking much better, thanks.

Some initial comments, will look more later this afternoon.

http://codereview.chromium.org/7775008/diff/27002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7775008/diff/27002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:111: if (storage == NULL) {
i believe chrome style is:

!storage

http://codereview.chromium.org/7775008/diff/27002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:120: if (sync_processor_ !=
NULL) {
chrome style: if (sync_processor_)

(here and other places)

http://codereview.chromium.org/7775008/diff/27002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:130: DCHECK(type ==
syncable::EXTENSION_SETTINGS);
DCHECK_EQ

http://codereview.chromium.org/7775008/diff/27002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:137: const ExtensionList*
extensions = extension_service_->extensions();
On 2011/09/14 20:33:24, Ben Olmstead wrote:
> You will also need to go through the disabled_extensions and
> terminated_extensions lists.

Maybe we should expose a method in ExtensionsService like GetAllExtensions() or
something?  (not necessarily in this CL)

http://codereview.chromium.org/7775008/diff/27002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:173: std::map<std::string,
DictionaryValue*> grouped_sync_data;
i'd rather you use linked_ptr<DictionaryValue> here, just to avoid manual memory
management

http://codereview.chromium.org/7775008/diff/27002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:193: std::map<std::string,
SyncableExtensionSettingsStorage*>::iterator storage_it;
prefer scoping the iterator in the for loop, even it means awkward indentation.

This also has the advantage of not having to pick unique names for them

http://codereview.chromium.org/7775008/diff/27002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:197: if (sync_data.get() ==
NULL) {
prefer the "positive" case first, i.e.

if (sync_data_get()) {
} else {
  ...
}

http://codereview.chromium.org/7775008/diff/27002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:207: // Under normal
circumstances this will probably be all of them.
"normal circumstances" -> "normal circumstances (i.e., not first-time sync)"

[not at google - send to devlin, 2011-09-14 22:45:04]

http://codereview.chromium.org/7775008/diff/27002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7775008/diff/27002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:111: if (storage == NULL) {
On 2011/09/14 21:08:53, akalin wrote:
> i believe chrome style is:
> 
> !storage

Done.

http://codereview.chromium.org/7775008/diff/27002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:120: if (sync_processor_ !=
NULL) {
On 2011/09/14 21:08:53, akalin wrote:
> chrome style: if (sync_processor_)
> 
> (here and other places)

Done.

http://codereview.chromium.org/7775008/diff/27002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:130: DCHECK(type ==
syncable::EXTENSION_SETTINGS);
On 2011/09/14 21:08:53, akalin wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/7775008/diff/27002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:137: const ExtensionList*
extensions = extension_service_->extensions();
On 2011/09/14 21:08:53, akalin wrote:
> On 2011/09/14 20:33:24, Ben Olmstead wrote:
> > You will also need to go through the disabled_extensions and
> > terminated_extensions lists.

Done.

> 
> Maybe we should expose a method in ExtensionsService like GetAllExtensions()
or
> something?  (not necessarily in this CL)

Yeah, there's the GetSyncDataList() method already on ExtensionService.  This
would be similar.

http://codereview.chromium.org/7775008/diff/27002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:173: std::map<std::string,
DictionaryValue*> grouped_sync_data;
On 2011/09/14 21:08:53, akalin wrote:
> i'd rather you use linked_ptr<DictionaryValue> here, just to avoid manual
memory
> management

Done.

http://codereview.chromium.org/7775008/diff/27002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:193: std::map<std::string,
SyncableExtensionSettingsStorage*>::iterator storage_it;
On 2011/09/14 21:08:53, akalin wrote:
> prefer scoping the iterator in the for loop, even it means awkward
indentation.
> 
> This also has the advantage of not having to pick unique names for them

Done.

http://codereview.chromium.org/7775008/diff/27002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:197: if (sync_data.get() ==
NULL) {
On 2011/09/14 21:08:53, akalin wrote:
> prefer the "positive" case first, i.e.
> 
> if (sync_data_get()) {
> } else {
>   ...
> }

Done.

http://codereview.chromium.org/7775008/diff/27002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:207: // Under normal
circumstances this will probably be all of them.
On 2011/09/14 21:08:53, akalin wrote:
> "normal circumstances" -> "normal circumstances (i.e., not first-time sync)"

Done.

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

http://codereview.chromium.org/7775008/diff/27002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:229: Result
result = delegate_->Remove(it->key());
On 2011/09/14 20:33:24, Ben Olmstead wrote:
> Are there any issues with removing a nonexistent key?

Not at the moment.  Extensions can remove nonexistent keys too, it's treated
much the same way.

However, there is a problem here with change notifications, we do need to check
whether the key is there or not.  This code in the patch which adds
notifications (... which I will put out for review when this patch is
submitted).

[akalin, 2011-09-15 19:56:44]

Okay, took a detailed look at pretty much everything except maybe the unit test
file.

Overall, looks good, no major issues.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_setting_sync_data.cc (right):

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_sync_data.cc:13: *this =
ExtensionSettingSyncData(sync_change.sync_data());
prefer having an Init() method and calling that instead.  It's Google style, and
messing with *this and operator= on a non-fully-constructed object gives me a
bad feeling

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_setting_sync_data.h (right):

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_sync_data.h:45: const Value* value()
const;
should return const Value&

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_sync_data.h:48: // Ref-counted
container for the data.
Add TODO to use Immutable<T> (see http://codereview.chromium.org/7904021/ )

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_sync_data.h:64: virtual ~Internal();
no need for virtual

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:7: #include "base/bind.h"
no need for bind

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:25: sync_processor_(NULL) {}
add DCHECK for FILE thread

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:28: std::map<std::string,
SyncableExtensionSettingsStorage*>::iterator it;
add DCHECK for FILE thread

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:42: DictionaryValue empty;
i prefer just inlining this, e.g.

return GetOrCreate(..., DictionaryValue())

but it's up to you

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:55: if (!storage) {
What happens in this case?  That means that the settings won't be written to
disk, but will be in sync?  This isn't very comment is it?  Add a comment
explaining how this works.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:99:
DCHECK(storage_objs_.count(extension_id) == 0);
DCHECK_EQ

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:137: SyncDataList all_sync_data;
I think the logic that gathers all the extensions should be in ExtensionService,
as adding another extension type would silently break this.  Maybe add a
GetAllExtensions() method to it and use that?  Or since you only really need the
ID, maybe GetAllExtensionIDs()

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:148: for
(ExtensionList::const_iterator it = extensions.begin();
DCHECK, file thread

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:162: Value *value;
 = NULL

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:186: LOG(WARNING) << "NULL value
in sync data";
prefer

LOG(WARNING) << "Could not parse setting: " <<
it->GetSpecifics().GetExtension(...).key() << " " << ...value();

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:207: } else {
from the logic above, I don't think sync_data should ever be NULL.  add
DCHECK(sync_data.get())

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:211:
grouped_sync_data.erase(it->first);
no need for this, and in fact it's dangerous given that you're invaliding it and
then incrementing it

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:237: LOG(WARNING) << "NULL value
in sync data";
revamp error message as above (possibly decomp into utility method)

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:243: std::map<std::string,
ExtensionSettingSyncDataList>::iterator it;
move into for loop

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:258: std::map<std::string,
SyncableExtensionSettingsStorage*>::iterator it;
fold into for loop

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.h (right):

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:17: class ExtensionSettings
Say that this lives on the FILE thread, and all methods must be called from
there.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:18: : public
base::RefCountedThreadSafe<ExtensionSettings>,
since this lives on the FILE thread only, shouldn't need to be ref-counted
anymore

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:27: void
SetExtensionService(ExtensionServiceInterface* extension_service);
Prefer to make this a constructor arg, since it'll never change

also, prefer const ExtensionServiceInterface* (or even &)

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_sync_unittest.cc (right):

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_sync_unittest.cc:23: 
Add TODO somewhere to also write integration tests

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_sync_unittest.cc:83:
DCHECK(data.value());
prefer

 if (!data.value()) {
  ADD_FAILURE();
  continue;
}

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_sync_unittest.cc:106: DCHECK_EQ(1u,
matching_changes.size()) <<
prefer:

if (matching_changes.size() != 1u) {
  ADD_FAILURE() << ...
  return ExtensionSettingSyncData()
}

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_sync_unittest.cc:156: static int
next = 0;
doesn't need to be static, just make it a member variable

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_sync_unittest.cc:232:
scoped_ptr<MessageLoopForUI> ui_message_loop_;
No need to use scoped_ptr, just set them up in constructor

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_sync_util.h (right):

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_sync_util.h:9: #include
"base/values.h"
forward-declare Value instead

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

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:7: #include
"base/bind.h"
no need for bind

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:9: #include
"base/message_loop.h"
no need for message loop or task either

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:18: :
extension_id_(extension_id), delegate_(delegate), sync_processor_(NULL) {}
add DCHECKs in all methods for FILE thread

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:112: for
(DictionaryValue::key_iterator it = sync_state.begin_keys();
i think you can do something like:

synced_keys_.insert(sync_state.begin_keys(), sync_state.end_keys());

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:121: return;
I think we should propagate the error up and let the caller handle it

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:153: for
(DictionaryValue::key_iterator it = settings.begin_keys();
use insert(begin, end) here, too

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:168: Value*
sync_value;
 = NULL

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:170: Value*
local_value;
here too

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:176:
SyncChange::ACTION_UPDATE, "", *it, sync_value));
"" for extension ID?  Is this correct?  even if it doesn't matter, rather just
use extension_id_ for correctness

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:220: if
(result.HasError()) {
prefer propagating errors up, although i guess you'd have to return a vector of
errors or something

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:276: for
(DictionaryValue::key_iterator it = settings.begin_keys();
use insert(begin, end)

probably should decomp this out into a utility method, though, since it's used
so much

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:309:
synced_keys_.erase(*it);
might be an erase(begin, end) method, too

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
File chrome/browser/extensions/syncable_extension_settings_storage.h (right):

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.h:62: std::string
extension_id_;
const?

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/sync/glue/ex...
File chrome/browser/sync/glue/extension_setting_data_type_controller.cc (right):

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/sync/glue/ex...
chrome/browser/sync/glue/extension_setting_data_type_controller.cc:39:
BrowserThread::PostTask(
if (!PostTask) {
  NOTREACHED();
}

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/sync/glue/ex...
chrome/browser/sync/glue/extension_setting_data_type_controller.cc:59:
BrowserThread::PostTask(
if (!PostTask()) {
  NOTREACHED();
}

http://codereview.chromium.org/7775008/diff/32002/chrome/common/chrome_switch...
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/7775008/diff/32002/chrome/common/chrome_switch...
chrome/common/chrome_switches.h:149: extern const char
kEnableSyncExtensionSettings[];
add a TODO to add to about:flags once we figure out the story with the UI (since
I believe we want to tie this to the extensions checkbox)

[not at google - send to devlin, 2011-09-16 05:18:59]

Ok, done most things apart from the error propagation from StartSyncing (as I've
said in the comment).  Need to catch a plane.

Note the addition of GetAllExtensionIds to ExtensionService (and
ExtensionServiceInterface).  I'll need to call this GetAllExtensionIDs (Ids is a
habit from Java), but besides that, definitely needs an extension person to sign
off on.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_setting_sync_data.cc (right):

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_sync_data.cc:13: *this =
ExtensionSettingSyncData(sync_change.sync_data());
On 2011/09/15 19:56:44, akalin wrote:
> prefer having an Init() method and calling that instead.  It's Google style,
and
> messing with *this and operator= on a non-fully-constructed object gives me a
> bad feeling

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_setting_sync_data.h (right):

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_sync_data.h:45: const Value* value()
const;
On 2011/09/15 19:56:44, akalin wrote:
> should return const Value&

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_sync_data.h:48: // Ref-counted
container for the data.
On 2011/09/15 19:56:44, akalin wrote:
> Add TODO to use Immutable<T> (see http://codereview.chromium.org/7904021/ )

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_sync_data.h:64: virtual ~Internal();
On 2011/09/15 19:56:44, akalin wrote:
> no need for virtual

IIRC clang complains if a destructor isn't virtual and it's inherited.

Or maybe it only complains if there are actually virtual methods defined on the
class.  Hmm.  I'll try removing virtual and hope for the best.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:7: #include "base/bind.h"
On 2011/09/15 19:56:44, akalin wrote:
> no need for bind

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:25: sync_processor_(NULL) {}
On 2011/09/15 19:56:44, akalin wrote:
> add DCHECK for FILE thread

... except it doesn't need to be constructed on the FILE thread, and it would be
kind of a pain to need to do that.  Well, probably not *that* much of a pain,
but it would make the code which constructs these need to be asynchronous, and
then refcount or something because they need to be injected elsewhere
immediately on construction... ugh.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:28: std::map<std::string,
SyncableExtensionSettingsStorage*>::iterator it;
On 2011/09/15 19:56:44, akalin wrote:
> add DCHECK for FILE thread

Ditto above comment, which is why they're deleted with
BrowserThread::DeleteSoon().

Although come to think of it, destruction of this still isn't thread safe, since
storage_objs_ isn't thread safe.  Bummer.  I'll push the
BrowserThread::DeleteSoon call up the stack.  Which means I can make the
SyncableExtensionSettingsStorage* a
linked_ptr<SyncableExtensionSettingsStorage>.  Yay, 80 character long iterator
name!

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:42: DictionaryValue empty;
On 2011/09/15 19:56:44, akalin wrote:
> i prefer just inlining this, e.g.
> 
> return GetOrCreate(..., DictionaryValue())
> 
> but it's up to you

Ah right.  Yes, I would do that but DictionaryValue() has no copy constructor so
C++ doesn't like it.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:55: if (!storage) {
On 2011/09/15 19:56:44, akalin wrote:
> What happens in this case?  That means that the settings won't be written to
> disk, but will be in sync?  This isn't very comment is it?  Add a comment
> explaining how this works.

I thought about this earlier, and I think it's fine.  If the leveldb storage
doesn't initialise and the in-memory version is active and syncing, then it just
means that the next time the browser starts it (presumably) won't be in sync. 
But that's fine... the sync initialisation logic is to overwrite local settings
with those from sync.

Comment added.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:99:
DCHECK(storage_objs_.count(extension_id) == 0);
On 2011/09/15 19:56:44, akalin wrote:
> DCHECK_EQ

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:137: SyncDataList all_sync_data;
On 2011/09/15 19:56:44, akalin wrote:
> I think the logic that gathers all the extensions should be in
ExtensionService,
> as adding another extension type would silently break this.  Maybe add a
> GetAllExtensions() method to it and use that?  Or since you only really need
the
> ID, maybe GetAllExtensionIDs()

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:148: for
(ExtensionList::const_iterator it = extensions.begin();
On 2011/09/15 19:56:44, akalin wrote:
> DCHECK, file thread

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:162: Value *value;
On 2011/09/15 19:56:44, akalin wrote:
>  = NULL

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:186: LOG(WARNING) << "NULL value
in sync data";
On 2011/09/15 19:56:44, akalin wrote:
> prefer
> 
> LOG(WARNING) << "Could not parse setting: " <<
> it->GetSpecifics().GetExtension(...).key() << " " << ...value();

Changing Value* to Value& made this moot.  Which is a good thing.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:207: } else {
On 2011/09/15 19:56:44, akalin wrote:
> from the logic above, I don't think sync_data should ever be NULL.  add
> DCHECK(sync_data.get())

I think it can be NULL if sync data exists for storage areas that haven't been
seen locally yet.  These would appear in grouped_sync_data but not
storage_objs_.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:211:
grouped_sync_data.erase(it->first);
On 2011/09/15 19:56:44, akalin wrote:
> no need for this, and in fact it's dangerous given that you're invaliding it
and
> then incrementing it

Should be safe, I'm incrementing an iterator to storage_objs_ not
grouped_sync_data?

Or am I confused?

Is there some idiomatic way to make this clearer?

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:237: LOG(WARNING) << "NULL value
in sync data";
On 2011/09/15 19:56:44, akalin wrote:
> revamp error message as above (possibly decomp into utility method)

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:243: std::map<std::string,
ExtensionSettingSyncDataList>::iterator it;
On 2011/09/15 19:56:44, akalin wrote:
> move into for loop

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:258: std::map<std::string,
SyncableExtensionSettingsStorage*>::iterator it;
On 2011/09/15 19:56:44, akalin wrote:
> fold into for loop

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.h (right):

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:17: class ExtensionSettings
On 2011/09/15 19:56:44, akalin wrote:
> Say that this lives on the FILE thread, and all methods must be called from
> there.

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:18: : public
base::RefCountedThreadSafe<ExtensionSettings>,
On 2011/09/15 19:56:44, akalin wrote:
> since this lives on the FILE thread only, shouldn't need to be ref-counted
> anymore

It seemed easiest to do the RefCountedThreadSafe with DeleteOnFileThread given
the way that the owner of this lives/dies on the UI thread while this has to die
on the FILE thread.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:27: void
SetExtensionService(ExtensionServiceInterface* extension_service);
On 2011/09/15 19:56:44, akalin wrote:
> Prefer to make this a constructor arg, since it'll never change
> 
> also, prefer const ExtensionServiceInterface* (or even &)

Can't make it a ctor argument unfortunately because they're mutually dependent
(an ExtensionService already takes a ExtensionSettings in its ctor).

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_sync_unittest.cc (right):

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_sync_unittest.cc:23: 
On 2011/09/15 19:56:44, akalin wrote:
> Add TODO somewhere to also write integration tests

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_sync_unittest.cc:83:
DCHECK(data.value());
On 2011/09/15 19:56:44, akalin wrote:
> prefer
> 
>  if (!data.value()) {
>   ADD_FAILURE();
>   continue;
> }

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_sync_unittest.cc:106: DCHECK_EQ(1u,
matching_changes.size()) <<
On 2011/09/15 19:56:44, akalin wrote:
> prefer:
> 
> if (matching_changes.size() != 1u) {
>   ADD_FAILURE() << ...
>   return ExtensionSettingSyncData()
> }

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_sync_unittest.cc:156: static int
next = 0;
On 2011/09/15 19:56:44, akalin wrote:
> doesn't need to be static, just make it a member variable

:(

(Though don't need this any more after adding the GetAllExtensions() method to
ExtensionService)

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_sync_unittest.cc:232:
scoped_ptr<MessageLoopForUI> ui_message_loop_;
On 2011/09/15 19:56:44, akalin wrote:
> No need to use scoped_ptr, just set them up in constructor

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_sync_util.h (right):

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_sync_util.h:9: #include
"base/values.h"
On 2011/09/15 19:56:44, akalin wrote:
> forward-declare Value instead

Done.

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

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:7: #include
"base/bind.h"
On 2011/09/15 19:56:44, akalin wrote:
> no need for bind

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:9: #include
"base/message_loop.h"
On 2011/09/15 19:56:44, akalin wrote:
> no need for message loop or task either

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:18: :
extension_id_(extension_id), delegate_(delegate), sync_processor_(NULL) {}
On 2011/09/15 19:56:44, akalin wrote:
> add DCHECKs in all methods for FILE thread

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:112: for
(DictionaryValue::key_iterator it = sync_state.begin_keys();
On 2011/09/15 19:56:44, akalin wrote:
> i think you can do something like:
> 
> synced_keys_.insert(sync_state.begin_keys(), sync_state.end_keys());

It would be nice, and GCC/clang are fine with it, but something about the
implementation of DictionaryValue::key_iterator disagrees with visual c++ and it
doesn't compile.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:121: return;
On 2011/09/15 19:56:44, akalin wrote:
> I think we should propagate the error up and let the caller handle it

Didn't have time to do this before my flight, in fact, it's board now.  What can
the caller do though?  I presume... this can return the Result and the caller
log the error rather than the error being logged here.  But... it seems wrong to
return a Result from this, those Result objects are for storage operations.

And a bool wouldn't really be all that useful.

Anyway, I'll deal with it / think about it later.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:153: for
(DictionaryValue::key_iterator it = settings.begin_keys();
On 2011/09/15 19:56:44, akalin wrote:
> use insert(begin, end) here, too

Ditto problem.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:168: Value*
sync_value;
On 2011/09/15 19:56:44, akalin wrote:
>  = NULL

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:170: Value*
local_value;
On 2011/09/15 19:56:44, akalin wrote:
> here too

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:176:
SyncChange::ACTION_UPDATE, "", *it, sync_value));
On 2011/09/15 19:56:44, akalin wrote:
> "" for extension ID?  Is this correct?  even if it doesn't matter, rather just
> use extension_id_ for correctness

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:220: if
(result.HasError()) {
On 2011/09/15 19:56:44, akalin wrote:
> prefer propagating errors up, although i guess you'd have to return a vector
of
> errors or something

Ditto.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:276: for
(DictionaryValue::key_iterator it = settings.begin_keys();
On 2011/09/15 19:56:44, akalin wrote:
> use insert(begin, end)
> 
> probably should decomp this out into a utility method, though, since it's used
> so much

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:309:
synced_keys_.erase(*it);
On 2011/09/15 19:56:44, akalin wrote:
> might be an erase(begin, end) method, too

Heh, it's funny because I tried all these things a while ago and ran into crazy
bugs.  The weird windows compiler one was just a puzzling compile error, but if
you want some fun trivia, read on:

std::set does have an erase(begin, end) method.  Unlike .insert though, it
assumes that the iterators are to *itself*.  If you try to do
std::set.erase(keys.start(), keys.end()) it puts the container in some invalid
state... I can't really remember the symptoms of the problem, but it manifested
itself later on and it took me a little while to figure out that this was the
cause.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
File chrome/browser/extensions/syncable_extension_settings_storage.h (right):

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.h:62: std::string
extension_id_;
On 2011/09/15 19:56:44, akalin wrote:
> const?

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/sync/glue/ex...
File chrome/browser/sync/glue/extension_setting_data_type_controller.cc (right):

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/sync/glue/ex...
chrome/browser/sync/glue/extension_setting_data_type_controller.cc:39:
BrowserThread::PostTask(
On 2011/09/15 19:56:44, akalin wrote:
> if (!PostTask) {
>   NOTREACHED();
> }

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/sync/glue/ex...
chrome/browser/sync/glue/extension_setting_data_type_controller.cc:59:
BrowserThread::PostTask(
On 2011/09/15 19:56:44, akalin wrote:
> if (!PostTask()) {
>   NOTREACHED();
> }

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/common/chrome_switch...
File chrome/common/chrome_switches.h (right):

http://codereview.chromium.org/7775008/diff/32002/chrome/common/chrome_switch...
chrome/common/chrome_switches.h:149: extern const char
kEnableSyncExtensionSettings[];
On 2011/09/15 19:56:44, akalin wrote:
> add a TODO to add to about:flags once we figure out the story with the UI
(since
> I believe we want to tie this to the extensions checkbox)

Done.

[akalin, 2011-09-16 15:36:39]

Replies to comments, will take another look at new version, too

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:25: sync_processor_(NULL) {}
On 2011/09/16 05:18:59, kalman1 wrote:
> On 2011/09/15 19:56:44, akalin wrote:
> > add DCHECK for FILE thread
> 
> ... except it doesn't need to be constructed on the FILE thread, and it would
be
> kind of a pain to need to do that.  Well, probably not *that* much of a pain,
> but it would make the code which constructs these need to be asynchronous, and
> then refcount or something because they need to be injected elsewhere
> immediately on construction... ugh.

An object which is constructed/destroyed on one thread but used on another
doesn't make sense by itself.  It may make sense in context of the whole
program, where you know how it's used, but it's much more preferable to have
objects that can be understood by themselves.

Yeah, it's more complex to do things correctly.  But the reason why the simple
way is simple is because it's wrong. :-) I'd have to take a look at the code
that uses this class, but I bet there are some race conditions lurking in there,
with probably some leaks (using DeleteOn*Thread with a ref-counted object is
basically a recipe for a shutdown leak).

So, I'd much prefer this class to live completely on the FILE thread.  If
necessary, you can push up the ref-counted/delete-on-file-thread grossness to an
inner struct of the owner of this class.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:28: std::map<std::string,
SyncableExtensionSettingsStorage*>::iterator it;
On 2011/09/16 05:18:59, kalman1 wrote:
> On 2011/09/15 19:56:44, akalin wrote:
> > add DCHECK for FILE thread
> 
> Ditto above comment, which is why they're deleted with
> BrowserThread::DeleteSoon().
> 
> Although come to think of it, destruction of this still isn't thread safe,
since
> storage_objs_ isn't thread safe.  Bummer.  I'll push the
> BrowserThread::DeleteSoon call up the stack.  Which means I can make the
> SyncableExtensionSettingsStorage* a
> linked_ptr<SyncableExtensionSettingsStorage>.  Yay, 80 character long iterator
> name!

See earlier comment.  If this class lives completely on the FILE thread, there's
no need for DeleteSoon.  linked_ptr<> may be a good idea for storage_objs_. 
Keep in mind that linked_ptr<> is *not* thread-safe.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:42: DictionaryValue empty;
On 2011/09/16 05:18:59, kalman1 wrote:
> On 2011/09/15 19:56:44, akalin wrote:
> > i prefer just inlining this, e.g.
> > 
> > return GetOrCreate(..., DictionaryValue())
> > 
> > but it's up to you
> 
> Ah right.  Yes, I would do that but DictionaryValue() has no copy constructor
so
> C++ doesn't like it.

Right, I forgot about that. Ugh.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:204: linked_ptr<DictionaryValue>
sync_data = grouped_sync_data[it->first];
prefer using group_sync_data.find() so that you don't auto-create a value if it
doesn't exist.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:207: } else {
On 2011/09/16 05:18:59, kalman1 wrote:
> On 2011/09/15 19:56:44, akalin wrote:
> > from the logic above, I don't think sync_data should ever be NULL.  add
> > DCHECK(sync_data.get())
> 
> I think it can be NULL if sync data exists for storage areas that haven't been
> seen locally yet.  These would appear in grouped_sync_data but not
> storage_objs_.

Ah, I see, right.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:211:
grouped_sync_data.erase(it->first);
On 2011/09/16 05:18:59, kalman1 wrote:
> On 2011/09/15 19:56:44, akalin wrote:
> > no need for this, and in fact it's dangerous given that you're invaliding it
> and
> > then incrementing it
> 
> Should be safe, I'm incrementing an iterator to storage_objs_ not
> grouped_sync_data?
> 
> Or am I confused?
> 
> Is there some idiomatic way to make this clearer?

My mistake, I didn't see you were iterating over storage_objs_.

May as well move this into the if statement (see earlier comment about using
group_sync_data.find())

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.h (right):

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:18: : public
base::RefCountedThreadSafe<ExtensionSettings>,
On 2011/09/16 05:18:59, kalman1 wrote:
> On 2011/09/15 19:56:44, akalin wrote:
> > since this lives on the FILE thread only, shouldn't need to be ref-counted
> > anymore
> 
> It seemed easiest to do the RefCountedThreadSafe with DeleteOnFileThread given
> the way that the owner of this lives/dies on the UI thread while this has to
die
> on the FILE thread.

See earlier rant about lifetime management.  This class should live completely
on the FILE thread, and any complexity with the owner living on the UI thread
should belong with the owner.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:27: void
SetExtensionService(ExtensionServiceInterface* extension_service);
On 2011/09/16 05:18:59, kalman1 wrote:
> On 2011/09/15 19:56:44, akalin wrote:
> > Prefer to make this a constructor arg, since it'll never change
> > 
> > also, prefer const ExtensionServiceInterface* (or even &)
> 
> Can't make it a ctor argument unfortunately because they're mutually dependent
> (an ExtensionService already takes a ExtensionSettings in its ctor).

Ugh.  Maybe add a comment?  Although if ExtensionSettings needs access to
ExtensionService, it seems to me that ExtensionService should create the
ExtensionSettings object.

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

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:97:
std::vector<std::string> keys;
i think you can do:

std::vector<...> keys(synced_keys_.begin(), ....end());

directly

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:112: for
(DictionaryValue::key_iterator it = sync_state.begin_keys();
On 2011/09/16 05:18:59, kalman1 wrote:
> On 2011/09/15 19:56:44, akalin wrote:
> > i think you can do something like:
> > 
> > synced_keys_.insert(sync_state.begin_keys(), sync_state.end_keys());
> 
> It would be nice, and GCC/clang are fine with it, but something about the
> implementation of DictionaryValue::key_iterator disagrees with visual c++ and
it
> doesn't compile.

I see.  Add comment and/or decomp into another function?

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:121: return;
On 2011/09/16 05:18:59, kalman1 wrote:
> On 2011/09/15 19:56:44, akalin wrote:
> > I think we should propagate the error up and let the caller handle it
> 
> Didn't have time to do this before my flight, in fact, it's board now.  What
can
> the caller do though?  I presume... this can return the Result and the caller
> log the error rather than the error being logged here.  But... it seems wrong
to
> return a Result from this, those Result objects are for storage operations.
> 
> And a bool wouldn't really be all that useful.
> 
> Anyway, I'll deal with it / think about it later.

Suggest wrapping any error message from maybe_settings here into a SyncError and
returning that.  Seems more useful than a bool, and you wouldn't expose the
Result object.  WDYT?

[Ben Olmstead, 2011-09-16 18:21:43]

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:653: static void
AddAllExtensionIds(
Wrap in anonymous namespace.

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:82: virtual const
std::vector<std::string> GetAllExtensionIds() const = 0;
I'm entirely in favor of adding this method.

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:196: // Gets the list of currently
installed extensions.
While you're in here, could you change this comment to something like "Gets the
list of currently running extensions."?

[akalin, 2011-09-17 08:44:47]

some more comments re. ExtensionSettings.  I think this + my replies to your
comments are enough work for now, so I'll hold off on a more detailed pass until
after those are addressed.

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:654: std::vector<std::string>*
dst, const ExtensionList& src) {
i think output args are usually put last

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:1904:
std::vector<ExtensionSyncData> extension_sync_list;
rewrite this fn using GetAllExtensionIds() and GetInstalledExtension() instead
of GetSyncDataListHelper.

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:82: virtual const
std::vector<std::string> GetAllExtensionIds() const = 0;
const doesn't make sense in a return value

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:82: virtual const
std::vector<std::string> GetAllExtensionIds() const = 0;
On 2011/09/16 18:21:43, Ben Olmstead wrote:
> I'm entirely in favor of adding this method.

May want to decomp this change out and check that in separately, esp. given my
comment in the .cc file to make GetAllSyncData use this fn.

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_setting_sync_data.cc (right):

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_sync_data.cc:45: : internal_(new
Internal(change_type, extension_id, key, value)) {}
if value is NULL, then value() will crash.  Either mandate a non-null |value|
(and make callers handle NULL) or create a dummy one here, or make Internal
create one on NULL.

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_setting_sync_data.h (right):

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_sync_data.h:43: // Returns the value
of the setting, possibly NULL.
fix comment

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:133: std::vector<std::string>
extension_ids(
Wait, this will include apps, too, right?  Surely we must filter those out.

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:134:
extension_service_->GetAllExtensionIds());
hmm, problem.  I'm pretty sure ExtensionService lives on the UI thread.  So we
either have to make GetAllExtensionIds explicitly thread-safe, or figure
something else out.

Actually, why do we have to grab the current list of extensions and then loop
over that, possibly creating new storage objects?  If the storage lives on the
FILE thread, shouldn't there be a way to access all currently-existing storage
objects without having to consult the extension service?  I think it's okay if
there are some extensions without settings, yet.

This'll remove the dependency on ExtensionService, too.

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:136: for
(std::vector<std::string>::iterator it = extension_ids.begin();
const_iterator

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:172: linked_ptr<DictionaryValue>
sync_data =
Didn't address my previous comment:

prefer using group_sync_data.find() so that you don't auto-create a value if it
doesn't exist.

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:187: linked_ptr<DictionaryValue>
sync_data = grouped_sync_data[it->first];
use find() here, too

[not at google - send to devlin, 2011-09-19 07:10:46]

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:25: sync_processor_(NULL) {}
> An object which is constructed/destroyed on one thread but used on another
> doesn't make sense by itself.  It may make sense in context of the whole
> program, where you know how it's used, but it's much more preferable to have
> objects that can be understood by themselves.

I don't see why it can't be understood by itself.  Methods that need to run on
the FILE thread are DCHECKed as such.  This is all methods apart from the
constructor, a very lightweight operation that just sets a pointer to NULL and
constructs an empty map.

Particularly now that there's a clear owner (see below), it's a pain to arrange
it any other way.

> 
> Yeah, it's more complex to do things correctly.  But the reason why the simple
> way is simple is because it's wrong. :-) I'd have to take a look at the code
> that uses this class, but I bet there are some race conditions lurking in
there,
> with probably some leaks (using DeleteOn*Thread with a ref-counted object is
> basically a recipe for a shutdown leak).

Not sure this comment still applies given the changes described below, but let
me know if it does.

> 
> So, I'd much prefer this class to live completely on the FILE thread.  If
> necessary, you can push up the ref-counted/delete-on-file-thread grossness to
an
> inner struct of the owner of this class.

Ok, so I'll summarise all the changes regarding this and just say "see comment"
for the other related comments.

You're right -- there's no reason why the ExtensionSettings object needs to be
constructed outside the ExtensionService that I can see.  I originally based the
structure of the code on ExtensionPreferences, but now that I'm more familiar
with the code it looks like that's unnecessarily complicated.

So now the ExtensionService just owns and constructs its own ExtensionSettings. 
This is much clearer.  The thread restriction on construction is whatever the
ExtensionService's restriction is (since there are no restrictions), this allows
simple/synchronous construction, I can't see why now there'd be any other way
that makes sense.  Then the service ensures that it's destroyed on the FILE
thread (DeleteSoon without any ref counting).

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:28: std::map<std::string,
SyncableExtensionSettingsStorage*>::iterator it;
> See earlier comment.  If this class lives completely on the FILE thread,
there's
> no need for DeleteSoon.  linked_ptr<> may be a good idea for storage_objs_. 
> Keep in mind that linked_ptr<> is *not* thread-safe.

Yep, I made this change in the version I uploaded before.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:204: linked_ptr<DictionaryValue>
sync_data = grouped_sync_data[it->first];
On 2011/09/16 15:36:40, akalin wrote:
> prefer using group_sync_data.find() so that you don't auto-create a value if
it
> doesn't exist.

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:211:
grouped_sync_data.erase(it->first);
On 2011/09/16 15:36:40, akalin wrote:
> On 2011/09/16 05:18:59, kalman1 wrote:
> > On 2011/09/15 19:56:44, akalin wrote:
> > > no need for this, and in fact it's dangerous given that you're invaliding
it
> > and
> > > then incrementing it
> > 
> > Should be safe, I'm incrementing an iterator to storage_objs_ not
> > grouped_sync_data?
> > 
> > Or am I confused?
> > 
> > Is there some idiomatic way to make this clearer?
> 
> My mistake, I didn't see you were iterating over storage_objs_.
> 
> May as well move this into the if statement (see earlier comment about using
> group_sync_data.find())

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.h (right):

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:18: : public
base::RefCountedThreadSafe<ExtensionSettings>,
> See earlier rant about lifetime management.  This class should live completely
> on the FILE thread, and any complexity with the owner living on the UI thread
> should belong with the owner.

See previous comment.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:27: void
SetExtensionService(ExtensionServiceInterface* extension_service);
> Ugh.  Maybe add a comment?  Although if ExtensionSettings needs access to
> ExtensionService, it seems to me that ExtensionService should create the
> ExtensionSettings object.

Don't need this any more.

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

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:97:
std::vector<std::string> keys;
On 2011/09/16 15:36:40, akalin wrote:
> i think you can do:
> 
> std::vector<...> keys(synced_keys_.begin(), ....end());
> 
> directly

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:112: for
(DictionaryValue::key_iterator it = sync_state.begin_keys();
On 2011/09/16 15:36:40, akalin wrote:
> On 2011/09/16 05:18:59, kalman1 wrote:
> > On 2011/09/15 19:56:44, akalin wrote:
> > > i think you can do something like:
> > > 
> > > synced_keys_.insert(sync_state.begin_keys(), sync_state.end_keys());
> > 
> > It would be nice, and GCC/clang are fine with it, but something about the
> > implementation of DictionaryValue::key_iterator disagrees with visual c++
and
> it
> > doesn't compile.
> 
> I see.  Add comment and/or decomp into another function?

Done.

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:121: return;
On 2011/09/16 15:36:40, akalin wrote:
> On 2011/09/16 05:18:59, kalman1 wrote:
> > On 2011/09/15 19:56:44, akalin wrote:
> > > I think we should propagate the error up and let the caller handle it
> > 
> > Didn't have time to do this before my flight, in fact, it's board now.  What
> can
> > the caller do though?  I presume... this can return the Result and the
caller
> > log the error rather than the error being logged here.  But... it seems
wrong
> to
> > return a Result from this, those Result objects are for storage operations.
> > 
> > And a bool wouldn't really be all that useful.
> > 
> > Anyway, I'll deal with it / think about it later.
> 
> Suggest wrapping any error message from maybe_settings here into a SyncError
and
> returning that.  Seems more useful than a bool, and you wouldn't expose the
> Result object.  WDYT?

I looked at this some more.  I think it makes the most sense to return a
SyncError() since that's basically what it seems a SyncError is for.  However,
the ExtensionSettings which calls these StartSyncing methods can't really do
much about it, since it just means that this single extension failed to start;
others may have worked.

Which implies two problems: Firstly, the docs for SyncError say that they are
"unrecoverable" but what does this mean for partial recoverability?  Secondly,
if ExtensionSettings should propagate the error further up to its return value,
what should it be for the combination of all errors?

The same issue exists with the return value from ProcessSyncChanges, along with
the additional problem of there being multiple failable operations within the
method itself (as you already said).

Anyway, for now I've made this method return a SyncError (which will do the
logging itself, on construction of an error), but not done anything about it at
the ExtensionSettings level.

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.cc (right):

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:653: static void
AddAllExtensionIds(
On 2011/09/16 18:21:43, Ben Olmstead wrote:
> Wrap in anonymous namespace.

No longer needed.

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.cc:654: std::vector<std::string>*
dst, const ExtensionList& src) {
On 2011/09/17 08:44:47, akalin wrote:
> i think output args are usually put last

No longer needed.

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:82: virtual const
std::vector<std::string> GetAllExtensionIds() const = 0;
> May want to decomp this change out and check that in separately, esp. given my
> comment in the .cc file to make GetAllSyncData use this fn.

No longer needed.

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:196: // Gets the list of currently
installed extensions.
On 2011/09/16 18:21:43, Ben Olmstead wrote:
> While you're in here, could you change this comment to something like "Gets
the
> list of currently running extensions."?

Maybe the GetAllExtensionIDs and/or the comment update should happen in an
entirely unrelated patch, but it's not needed for the sync stuff anymore.

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_setting_sync_data.cc (right):

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_sync_data.cc:45: : internal_(new
Internal(change_type, extension_id, key, value)) {}
On 2011/09/17 08:44:47, akalin wrote:
> if value is NULL, then value() will crash.  Either mandate a non-null |value|
> (and make callers handle NULL) or create a dummy one here, or make Internal
> create one on NULL.

Done.

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_setting_sync_data.h (right):

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_sync_data.h:43: // Returns the value
of the setting, possibly NULL.
On 2011/09/17 08:44:47, akalin wrote:
> fix comment

Done.

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:133: std::vector<std::string>
extension_ids(
On 2011/09/17 08:44:47, akalin wrote:
> Wait, this will include apps, too, right?  Surely we must filter those out.

Apps are fine, no reason to filter them out?

Anyway, no longer needed anyway.

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:134:
extension_service_->GetAllExtensionIds());
> hmm, problem.  I'm pretty sure ExtensionService lives on the UI thread.  So we
> either have to make GetAllExtensionIds explicitly thread-safe, or figure
> something else out.
> 
> Actually, why do we have to grab the current list of extensions and then loop
> over that, possibly creating new storage objects?  If the storage lives on the
> FILE thread, shouldn't there be a way to access all currently-existing storage
> objects without having to consult the extension service?  I think it's okay if
> there are some extensions without settings, yet.
> 
> This'll remove the dependency on ExtensionService, too.

Yeah, this cleans up a whole lot of stuff.  See comment I made about this
earlier.

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:136: for
(std::vector<std::string>::iterator it = extension_ids.begin();
On 2011/09/17 08:44:47, akalin wrote:
> const_iterator

Done.

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:172: linked_ptr<DictionaryValue>
sync_data =
> Didn't address my previous comment:
> 
> prefer using group_sync_data.find() so that you don't auto-create a value if
it
> doesn't exist.

Why here?  Unlike below where you can avoid unnecessary construction, the
linked_ptr will be created on the next line anyway.  In terms of efficiency I
don't know whether the construction of a map iterator is lighter than the
assignment operation of a linked_ptr, but either way, it's hardly an important
consideration.  On the other hand using find() would be more verbose than this
NULL-checking way.

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:187: linked_ptr<DictionaryValue>
sync_data = grouped_sync_data[it->first];
On 2011/09/17 08:44:47, akalin wrote:
> use find() here, too

Done.

[akalin, 2011-09-19 07:49:16]

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:133: std::vector<std::string>
extension_ids(
On 2011/09/19 07:10:47, kalman1 wrote:
> On 2011/09/17 08:44:47, akalin wrote:
> > Wait, this will include apps, too, right?  Surely we must filter those out.
> 
> Apps are fine, no reason to filter them out?
> 
> Anyway, no longer needed anyway.

Well, you can turn extension sync and app sync on and off independently.  So if
we tie extensions setting sync to extension sync but also sync apps settings,
then that's surprising if a user turned off app sync.

As I mentioned before, we have to figure out the story with apps.

[akalin, 2011-09-19 07:51:24]

> Well, you can turn extension sync and app sync on and off independently.  So
if
> we tie extensions setting sync to extension sync but also sync apps settings,
> then that's surprising if a user turned off app sync.
> 
> As I mentioned before, we have to figure out the story with apps.

Also raises the question of what to do with settings for extensions that aren't
syncable (with NPAPI plugins, non-gallery plugins, user scripts).  I suspect it
may be okay to just sync settings for those anyway, but we should think through
it.

[not at google - send to devlin, 2011-09-19 08:42:19]

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:133: std::vector<std::string>
extension_ids(
On 2011/09/19 07:49:16, akalin wrote:
> On 2011/09/19 07:10:47, kalman1 wrote:
> > On 2011/09/17 08:44:47, akalin wrote:
> > > Wait, this will include apps, too, right?  Surely we must filter those
out.
> > 
> > Apps are fine, no reason to filter them out?
> > 
> > Anyway, no longer needed anyway.
> 
> Well, you can turn extension sync and app sync on and off independently.  So
if
> we tie extensions setting sync to extension sync but also sync apps settings,
> then that's surprising if a user turned off app sync.
> 
> As I mentioned before, we have to figure out the story with apps.

Ah right, that's what you meant.  Interesting.  This is something that
ExtensionSettings will need to be aware of since it's responsible for
interpreting the settings data type per-extension.

Doesn't sound hard, but hopefully something for a follow-up patch, this one is
big enough.

[not at google - send to devlin, 2011-09-19 08:43:15]

On 2011/09/19 07:51:24, akalin wrote:
> > Well, you can turn extension sync and app sync on and off independently.  So
> if
> > we tie extensions setting sync to extension sync but also sync apps
settings,
> > then that's surprising if a user turned off app sync.
> > 
> > As I mentioned before, we have to figure out the story with apps.
> 
> Also raises the question of what to do with settings for extensions that
aren't
> syncable (with NPAPI plugins, non-gallery plugins, user scripts).  I suspect
it
> may be okay to just sync settings for those anyway, but we should think
through
> it.

I rely on syncing non-gallery extension settings for my own testing :)

Anyway, I agree.

[akalin, 2011-09-19 18:46:16]

On 2011/09/19 07:10:46, kalman1 wrote:
>
http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/e...
> I don't see why it can't be understood by itself.  Methods that need to run on
> the FILE thread are DCHECKed as such.  This is all methods apart from the
> constructor, a very lightweight operation that just sets a pointer to NULL and
> constructs an empty map.

It can't be understood by itself because someone reading the code doesn't know
the reason why the object would want to be constructed on a thread separate from
where it's used, without knowing about the owner, i.e. ExtensionService.

> So now the ExtensionService just owns and constructs its own
ExtensionSettings. 
> This is much clearer.  The thread restriction on construction is whatever the
> ExtensionService's restriction is (since there are no restrictions), this
allows
> simple/synchronous construction, I can't see why now there'd be any other way
> that makes sense.  Then the service ensures that it's destroyed on the FILE
> thread (DeleteSoon without any ref counting).

ExtensionService lives on the UI thread and has to be constructed there (I
believe there's a DCHECK for it).  This is much better but I think it can be
improved.

So the reason i'm so adamant about making each class live on a single thread is
because doing so otherwise inevitably leads to subtle bugs.  I dug into how
ExtensionSettings is used and I found one without much effort.  In
extension_settings_api.cc, SettingsFunction::RunOnFileThread() calls
profile()->GetExtensionService()->extension_settings() and operates on it.  This
is bad because the profile and/or the extension service may have already been
deleted (on the UI thread).  In fact, UIThreadExtensionFunction::profile()
should probably have a DCHECK to make sure it's only called from the UI thread
(but that can be done separately).

The naive fix for this would be to access the extension_settings() from
RunImpl() and make it an argument of RunOnFileThread().  This works, but you'd
have to know that ~ExtensionService calls DeleteSoon() on its
extension_settings, so the ordering works out.  So now it's impossible to
understand the correctness of SettingsFunction without knowing the gory details
of ExtensionService's destructor.

So here's what I think the correct way to fix this is:

1) Make ExtensionSettings live completely on the FILE thread, including the
constructor.
2) Write a new class, ExtensionSettingsUIWrapper, that lives completely on the
UI thread, with the following interface:

class ExtensionSettingsUIWrapper {
 public:
  // |fn| will be called on the FILE thread.
  void RunWithStorage(const std::string& extension_id, const
base::Callback<ExtensionSettingsStorage*>& fn);
};

3) Make ExtensionService have a ExtensionSettingsUIWrapper member variable. 
Since it lives on the UI thread, you don't need it to be stack-allocated.  Make
extension_settings() return an ExtensionSettingsUIWrapper* instead.  The other
nice thing is that now, like every other pointer ExtensionService returns, this
one lives on the UI thread.

4) Make SettingsFunction in extension_settings_api.cc use this new interface. 
Something like:

bool SettingsFunction::RunImpl() {
  DCHECK(on UI thread);
 
profile()->GetExtensionService()->extension_settings()->RunWithStorage(extension_id(),
base::Bind(&RunOnFileThread, this));
  return true;
}

void SettingsFunction::RunOnFileThread(ExtensionServiceStorage* storage) {
  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::FILE));
  bool success = RunWithStorage(storage);
  BrowserThread::PostTask(
      BrowserThread::UI,
      FROM_HERE,
      base::Bind(&SettingsFunction::SendResponse, this, success));
}

How to implement ExtensionSettingsUIWrapper correctly and safely will be in a
follow-up message.

[akalin, 2011-09-19 18:57:57]

Okay, so how to implement ExtensionSettingsUIWrapper (a sketch):

class ExtensionSettingsUIWrapper {
 public:
  ExtensionSettingsUIWrapper(<extension settings args>) : core_(new
Core(<extension settings args>)) {
    DCHECK(on UI thread);
  }

  // Called on UI thread.  |fn| will be called on the FILE thread.
  void RunWithStorage(const std::string& extension_id, const
base::Callback<ExtensionSettingsStorage*>& fn) {
    DCHECK(on UI thread);
    core->RunWithStorage(extension_id, fn);
  }

  ~ExtensionSettingsUIWrapper() {
    DCHECK(on UI thread);
  }

 private:
  class Core : public RefCountedThreadSafe<Core> {
   public:
    // Constructed on UI thread.
    Core(<extension settings args>) : extension_settings_(NULL) {
      DCHECK(on ui thread);
      PostOnFileThread(&InitOnFileThread, <extension settings args>);
    }

    // Called on UI thread.  |fn| will be called on the FILE thread.
    void RunWithStorage(const std::string& extension_id, const
base::Callback<ExtensionSettingsStorage*>& fn) {
      DCHECK(on ui thread);
      PostOnFileThread(&RunWithStorageOnFileThread, this, extension_id, fn);
    }

    // Used only on FILE thread.
    ExtensionSettings* extension_settings() {
      DCHECK(on file thread);
      // We know this must happen after InitOnFileThread and before
extension_settings_ is deleted on FILE thread.
      DCHECK(extension_settings_);
      return extension_settings_;
    }

   private:
    // Can be destroyed on either UI or FILE thread.
    ~Core() {
      DCHECK(on ui thread or file thread);
      DeleteSoon(on file thread, extension_settings_);
    }

    void InitOnFileThread(<extension settings args>) {
      DCHECK(on file thread);
      DCHECK(!extension_settings_);
      extension_settings_ = new ExtensionSettings(<args>);
    }

    // Called on FILE thread.
    void RunWithStorageOnFileThread(const std::string& extension_id, const
base::Callback<ExtensionSettingsStorage*>& fn) {
      DCHECK(on file thread);
      DCHECK(extension_settings);
      fn.Run(extension_settings_->GetStorage(extension_id));
    }

    // lives on FILE therad.
    ExtensionSettings* extension_settings_;
  };
};

So basically all the gory details of the UI/FILE thread jumping are encapsulated
in this class, instead of being scattered in the implementations of
ExtensionService, ExtensionSettings, and SettingsFunction.

[akalin, 2011-09-19 19:00:28]

> Doesn't sound hard, but hopefully something for a follow-up patch, this one is
> big enough.

Okay, I'm fine with adding a TODO for this.

It may actually be hard, and may involve adding another sync type (APP_SETTINGS
or something).

[akalin, 2011-09-19 19:24:10]

Next round of comments

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

http://codereview.chromium.org/7775008/diff/32002/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:121: return;
> I looked at this some more.  I think it makes the most sense to return a
> SyncError() since that's basically what it seems a SyncError is for.  However,
> the ExtensionSettings which calls these StartSyncing methods can't really do
> much about it, since it just means that this single extension failed to start;
> others may have worked.
> 
> Which implies two problems: Firstly, the docs for SyncError say that they are
> "unrecoverable" but what does this mean for partial recoverability?  Secondly,
> if ExtensionSettings should propagate the error further up to its return
value,
> what should it be for the combination of all errors?

Yeah, I'll have to think about "recoverable" errors like these.

> The same issue exists with the return value from ProcessSyncChanges, along
with
> the additional problem of there being multiple failable operations within the
> method itself (as you already said).
> 
> Anyway, for now I've made this method return a SyncError (which will do the
> logging itself, on construction of an error), but not done anything about it
at
> the ExtensionSettings level.

Yeah, I think that's fine for now.

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7775008/diff/42051/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:172: linked_ptr<DictionaryValue>
sync_data =
On 2011/09/19 07:10:47, kalman1 wrote:
> > Didn't address my previous comment:
> > 
> > prefer using group_sync_data.find() so that you don't auto-create a value if
> it
> > doesn't exist.
> 
> Why here?  Unlike below where you can avoid unnecessary construction, the
> linked_ptr will be created on the next line anyway.  In terms of efficiency I
> don't know whether the construction of a map iterator is lighter than the
> assignment operation of a linked_ptr, but either way, it's hardly an important
> consideration.  On the other hand using find() would be more verbose than this
> NULL-checking way.

You're right, I think this is fine.

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:47: // Fall back to an in-memory
storage area (cached NOOP).
add comment about how settings in that storage area will be lost, but that's
okay if sync is on.  (I thought you were going to add this?)

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:112:
synced_storage->StartSyncing(initial_sync_data, sync_processor_);
do ignore_result(...StartSyncing()); (#include base/compiler_specific.h) and
have a TODO for what to do on error

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:119: void
ExtensionSettings::GetKnownExtensionIDs(
just have this return a set directly

also, DCHECK on file thread

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:209:
it->second->StartSyncing(*maybe_sync_data->second, sync_processor);
like above, use ignore_result and TODO

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:213:
it->second->StartSyncing(empty, sync_processor);
ignore_result and TODO

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:245:
GetOrCreateStorageWithSyncData(it->first, empty)->
ignore_result for errors, and TODO

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

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:13: // Inserts
all the keys from a dictionary into a set of strings.
prefer anon namespace instead of static

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:14: static void
InsertAll(std::set<std::string>* dst, const DictionaryValue& src) {
output args last

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:27:
SyncableExtensionSettingsStorage::~SyncableExtensionSettingsStorage() {}
DCHECK file thread

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:232: if
(result.HasError()) {
maybe have a vector of SyncErrors and push on those whenever we get a SyncError?

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:242: if
(result.HasError()) {
here too

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:252: }
return vector of sync errors

[not at google - send to devlin, 2011-09-20 07:59:36]

Thanks for all that.

ExtensionSettingsUIWrapper is done; it wasn't identical to what you said but
more or less the same.  I also had to delegate SyncableService calls from that
Core class, for the ProfileSyncFactory -- I've convinced myself that it's safe.

P.S. I keep on messing up git cl upload (against wrong branch), apologies for
the large number of unnecessary interim patch uploads.

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:47: // Fall back to an in-memory
storage area (cached NOOP).
On 2011/09/19 19:24:11, akalin wrote:
> add comment about how settings in that storage area will be lost, but that's
> okay if sync is on.  (I thought you were going to add this?)

Hmm so did I.  Done.

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:112:
synced_storage->StartSyncing(initial_sync_data, sync_processor_);
On 2011/09/19 19:24:11, akalin wrote:
> do ignore_result(...StartSyncing()); (#include base/compiler_specific.h) and
> have a TODO for what to do on error

Done.

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:119: void
ExtensionSettings::GetKnownExtensionIDs(
On 2011/09/19 19:24:11, akalin wrote:
> just have this return a set directly
> 
> also, DCHECK on file thread

Done.

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:209:
it->second->StartSyncing(*maybe_sync_data->second, sync_processor);
On 2011/09/19 19:24:11, akalin wrote:
> like above, use ignore_result and TODO

Done.

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:213:
it->second->StartSyncing(empty, sync_processor);
On 2011/09/19 19:24:11, akalin wrote:
> ignore_result and TODO

Done.

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:245:
GetOrCreateStorageWithSyncData(it->first, empty)->
On 2011/09/19 19:24:11, akalin wrote:
> ignore_result for errors, and TODO

Done.

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:245:
GetOrCreateStorageWithSyncData(it->first, empty)->
On 2011/09/19 19:24:11, akalin wrote:
> ignore_result for errors, and TODO

Done.

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

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:13: // Inserts
all the keys from a dictionary into a set of strings.
On 2011/09/19 19:24:11, akalin wrote:
> prefer anon namespace instead of static

Done.

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:14: static void
InsertAll(std::set<std::string>* dst, const DictionaryValue& src) {
On 2011/09/19 19:24:11, akalin wrote:
> output args last

Done.

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:27:
SyncableExtensionSettingsStorage::~SyncableExtensionSettingsStorage() {}
On 2011/09/19 19:24:11, akalin wrote:
> DCHECK file thread

Done.

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:27:
SyncableExtensionSettingsStorage::~SyncableExtensionSettingsStorage() {}
On 2011/09/19 19:24:11, akalin wrote:
> DCHECK file thread

Done.

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:232: if
(result.HasError()) {
On 2011/09/19 19:24:11, akalin wrote:
> maybe have a vector of SyncErrors and push on those whenever we get a
SyncError?

Done.

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:242: if
(result.HasError()) {
On 2011/09/19 19:24:11, akalin wrote:
> here too

Done.

http://codereview.chromium.org/7775008/diff/50001/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:252: }
On 2011/09/19 19:24:11, akalin wrote:
> return vector of sync errors

Done.

[akalin, 2011-09-20 14:53:10]

Thanks for making the changes.  This is looking a lot better!

More comments, mostly minor at this point.

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:669:
scoped_ptr<ExtensionSettingsUIWrapper> extension_settings_;
no need to make this a scoped_ptr; just have 'ExtensionSettingsUIWrapper
extension_settings;'

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_setting_sync_data.cc (right):

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_sync_data.cc:46: CHECK(value !=
NULL);
CHECK(value);

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_sync_data.cc:64: return
*internal_->value_.get();
can just use '*internal->value_;'

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_sync_data.cc:76: DCHECK(value !=
NULL);
CHECK(value);

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:25: sync_processor_(NULL) {}
DCHECK file thread

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.h (right):

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:18: // Lives on the FILE thread,
so all methods (apart from the constructor, but
remove parenthesized clause

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:45: virtual SyncDataList
GetAllSyncData(syncable::ModelType type) const OVERRIDE;
#include base/compiler_specific.h

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:57: friend class
DeleteTask<ExtensionSettings>;
no need for this anymore.  Just make the destructor public, since you already
DCHECK you're on the FILE thread from there.

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_storage_unittest.h (right):

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage_unittest.h:58:
ExtensionSettings* settings_;
No need for this to be a pointer anymore (just have 'ExtensionSettings
settings_;'

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_sync_unittest.cc (right):

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_sync_unittest.cc:122: class
ExtensionSettingsSyncUnittest : public testing::Test {
usual naming convention is just a 'Test' prefix, so 'ExtensionSettingsSyncTest'

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_sync_unittest.cc:149:
ExtensionSettings* settings_;
can just be 'ExtensionSettings settings_;'

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_ui_wrapper.cc (right):

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_ui_wrapper.cc:30: return
core_.get();
DCHECK ui thread

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_ui_wrapper.h (right):

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_ui_wrapper.h:9: #include
"base/file_path.h"
forward-declare FilePath instead

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_ui_wrapper.h:12: #include
"chrome/browser/extensions/extension_settings.h"
forward-declare extension settings

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_ui_wrapper.h:22: typedef
base::Callback<void(ExtensionSettingsStorage*)> Callback;
StorageCallback would be a better name

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_ui_wrapper.h:31: SyncableService*
GetSyncableService();
Add a comment saying this is needed for ProfileSyncFactory.

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_ui_wrapper.h:54:
syncable::ModelType type) const OVERRIDE;
#include base/compiler_specific.h for OVERRIDE

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

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:164:
LOG(WARNING) << "Failed to send changes to sync: " << error.message();
have this return an error and propagate to StartSyncing, etc.

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:213:
ProcessSyncChanges(changes);
propagate return value from here to StartSyncing

[not at google - send to devlin, 2011-09-21 00:20:13]

I'm going to spend some time syncing this to tip (haven't done so in a while) so
that I can run the try jobs.  Wish me luck :/

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:669:
scoped_ptr<ExtensionSettingsUIWrapper> extension_settings_;
On 2011/09/20 14:53:11, akalin wrote:
> no need to make this a scoped_ptr; just have 'ExtensionSettingsUIWrapper
> extension_settings;'

Done.

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_setting_sync_data.cc (right):

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_sync_data.cc:46: CHECK(value !=
NULL);
On 2011/09/20 14:53:11, akalin wrote:
> CHECK(value);

Done.

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_sync_data.cc:64: return
*internal_->value_.get();
On 2011/09/20 14:53:11, akalin wrote:
> can just use '*internal->value_;'

Done.

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_sync_data.cc:76: DCHECK(value !=
NULL);
On 2011/09/20 14:53:11, akalin wrote:
> CHECK(value);

Done.

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.cc (right):

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.cc:25: sync_processor_(NULL) {}
On 2011/09/20 14:53:11, akalin wrote:
> DCHECK file thread

Done.

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings.h (right):

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:18: // Lives on the FILE thread,
so all methods (apart from the constructor, but
On 2011/09/20 14:53:11, akalin wrote:
> remove parenthesized clause

Done.

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:45: virtual SyncDataList
GetAllSyncData(syncable::ModelType type) const OVERRIDE;
On 2011/09/20 14:53:11, akalin wrote:
> #include base/compiler_specific.h

Done.

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings.h:57: friend class
DeleteTask<ExtensionSettings>;
On 2011/09/20 14:53:11, akalin wrote:
> no need for this anymore.  Just make the destructor public, since you already
> DCHECK you're on the FILE thread from there.

Done.

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_storage_unittest.h (right):

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage_unittest.h:58:
ExtensionSettings* settings_;
On 2011/09/20 14:53:11, akalin wrote:
> No need for this to be a pointer anymore (just have 'ExtensionSettings
> settings_;'

It's still easier for it to be a pointer, since the constructor takes a
temporary directory and it's a bit convoluted to have this fully constructed on
the stack.

(You need to (1) have a ScopedTempDir, (2) have a utility function to create it
and call CreateNewTempDir, assign it on the stack (3) call
scoped_temp_dir.Set(temp_dir)... easier to just call new at the right time).

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_sync_unittest.cc (right):

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_sync_unittest.cc:122: class
ExtensionSettingsSyncUnittest : public testing::Test {
On 2011/09/20 14:53:11, akalin wrote:
> usual naming convention is just a 'Test' prefix, so
'ExtensionSettingsSyncTest'

Done.

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_sync_unittest.cc:149:
ExtensionSettings* settings_;
On 2011/09/20 14:53:11, akalin wrote:
> can just be 'ExtensionSettings settings_;'

Ditto previous comment.

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_ui_wrapper.cc (right):

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_ui_wrapper.cc:30: return
core_.get();
On 2011/09/20 14:53:11, akalin wrote:
> DCHECK ui thread

Done.

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_ui_wrapper.h (right):

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_ui_wrapper.h:9: #include
"base/file_path.h"
On 2011/09/20 14:53:11, akalin wrote:
> forward-declare FilePath instead

Done.

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_ui_wrapper.h:12: #include
"chrome/browser/extensions/extension_settings.h"
On 2011/09/20 14:53:11, akalin wrote:
> forward-declare extension settings

Done.

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_ui_wrapper.h:22: typedef
base::Callback<void(ExtensionSettingsStorage*)> Callback;
On 2011/09/20 14:53:11, akalin wrote:
> StorageCallback would be a better name

Done.

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_ui_wrapper.h:31: SyncableService*
GetSyncableService();
On 2011/09/20 14:53:11, akalin wrote:
> Add a comment saying this is needed for ProfileSyncFactory.

Done.

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_ui_wrapper.h:54:
syncable::ModelType type) const OVERRIDE;
On 2011/09/20 14:53:11, akalin wrote:
> #include base/compiler_specific.h for OVERRIDE

Done.

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

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:164:
LOG(WARNING) << "Failed to send changes to sync: " << error.message();
On 2011/09/20 14:53:11, akalin wrote:
> have this return an error and propagate to StartSyncing, etc.

Done.

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/s...
chrome/browser/extensions/syncable_extension_settings_storage.cc:213:
ProcessSyncChanges(changes);
On 2011/09/20 14:53:11, akalin wrote:
> propagate return value from here to StartSyncing

Done.

[not at google - send to devlin, 2011-09-21 01:44:00]

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_ui_wrapper.cc (right):

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_ui_wrapper.cc:30: return
core_.get();
On 2011/09/21 00:20:14, kalman1 wrote:
> On 2011/09/20 14:53:11, akalin wrote:
> > DCHECK ui thread
> 
> Done.

Actually this can't work that way it's structured.  GetSyncableService is called
from CreateExtensionSyncComponents which is called on the FILE thread because of
the whole NonFrontendDataTypeController thing causing
ExtensionSettingsDataTypeController::CreateSyncComponents to be run on the FILE
thread.

So as it stands, this patch causes sync to crash.  I'll change this to asserting
it's on the FILE thread but that makes the whole Core delegating sync calls to
the underlying ExtensionSettings rather unnecessary.  In the meantime... I'll
think... ideas?

[akalin, 2011-09-21 01:51:56]

> So as it stands, this patch causes sync to crash.  I'll change this to
> asserting it's on the FILE thread but that makes the whole Core
> delegating sync calls to the underlying ExtensionSettings rather
> unnecessary.  In the meantime... I'll think... ideas?

Ah, yes, I see the problem.  I think I have a solution, give me a few
minutes to write it up.

[akalin, 2011-09-21 01:58:41]

Okay, I think this should work.  Sorry about this, I know that this part of
setting up a new (non-UI) syncable service  is pretty messy. :/

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_ui_wrapper.h (right):

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_ui_wrapper.h:32: // Gets the
SyncableService component of the ExtensionSettings.  The service
remove this, and instead have:

void RunWithExtensionSettings(const base::Callback<void(ExtensionSettings*)>
callback); and make it analogous to RunWithStorage.

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_ui_wrapper.h:41: class Core :
public base::RefCountedThreadSafe<Core>, public SyncableService {
no need to have this be a syncableservice anymore

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/sync/glue/ex...
File chrome/browser/sync/glue/extension_setting_data_type_controller.cc (right):

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/sync/glue/ex...
chrome/browser/sync/glue/extension_setting_data_type_controller.cc:19:
profile_sync_service_(profile_sync_service) {}
stash a pointer to ExtensionSettingsUIWrapper:

extension_settings_ui_wrapper_(profile->GetExtensionService()->GetExtensionSettingsUIWrapper())

also DCHECK on UI thread

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/sync/glue/ex...
chrome/browser/sync/glue/extension_setting_data_type_controller.cc:39: if
(!BrowserThread::PostTask(
instead of this, do:

extension_settings_ui_wrapper_->RunWithExtensionSettings(base::Bind(&StartAssociationWithExtensionSettings,
this));

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/sync/glue/ex...
chrome/browser/sync/glue/extension_setting_data_type_controller.cc:48: }
have:

void StartAssociationWithExtensionSettings(ExtensionSettings*
extension_settings) {
  DCHECK(on file thread);
  extension_settings_ = extension_settings;
  // calls CreateSyncComponents.
  StartAssociation();
}

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/sync/glue/ex...
chrome/browser/sync/glue/extension_setting_data_type_controller.cc:54:
profile_sync_factory()->CreateExtensionSettingSyncComponents(
pass in extension_settings_ (DCHECK it's non-NULL) here

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/sync/glue/ex...
File chrome/browser/sync/glue/extension_setting_data_type_controller.h (right):

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/sync/glue/ex...
chrome/browser/sync/glue/extension_setting_data_type_controller.h:41: 
define:

void StartAssociationWithExtensionSettings(ExtensionSettings* settings);

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_factory_impl.cc (right):

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_factory_impl.cc:262: SyncableService*
sync_service =
DCHECK on file thread

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_factory_impl.cc:263:
profile_->GetExtensionService()->extension_settings()->
sync_service = extension_settings (or just use it directly)

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_factory_impl.h (right):

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_factory_impl.h:54:
browser_sync::UnrecoverableErrorHandler* error_handler);
have this take ExtensionSettings* extension_settings

[akalin, 2011-09-21 02:05:56]

Some more comments.  I'll let you implement my suggested fix for the problem you
had and get it working with tip of tree, etc.,  then I'll take another detailed
look.  This is getting close!

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_storage_unittest.h (right):

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage_unittest.h:58:
ExtensionSettings* settings_;
On 2011/09/21 00:20:14, kalman1 wrote:
> On 2011/09/20 14:53:11, akalin wrote:
> > No need for this to be a pointer anymore (just have 'ExtensionSettings
> > settings_;'
> 
> It's still easier for it to be a pointer, since the constructor takes a
> temporary directory and it's a bit convoluted to have this fully constructed
on
> the stack.
> 
> (You need to (1) have a ScopedTempDir, (2) have a utility function to create
it
> and call CreateNewTempDir, assign it on the stack (3) call
> scoped_temp_dir.Set(temp_dir)... easier to just call new at the right time).

Ah, that makes sense.  But surely it can be a scoped_ptr<> now, no?  Since it's
not required to be deleted via DeleteSoon now.

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_sync_unittest.cc (right):

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_sync_unittest.cc:149:
ExtensionSettings* settings_;
On 2011/09/21 00:20:14, kalman1 wrote:
> On 2011/09/20 14:53:11, akalin wrote:
> > can just be 'ExtensionSettings settings_;'
> 
> Ditto previous comment.

As in the other place, can be scoped_ptr here, too.

(Even though you do end up manually resetting it anyway, it's still a good
habit.)

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_storage_unittest.cc (right):

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage_unittest.cc:91: delete
settings_;
can do settings_.reset(); if scoped ptr

[not at google - send to devlin, 2011-09-21 03:55:18]

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_storage_unittest.h (right):

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage_unittest.h:58:
ExtensionSettings* settings_;
On 2011/09/21 02:05:56, akalin wrote:
> On 2011/09/21 00:20:14, kalman1 wrote:
> > On 2011/09/20 14:53:11, akalin wrote:
> > > No need for this to be a pointer anymore (just have 'ExtensionSettings
> > > settings_;'
> > 
> > It's still easier for it to be a pointer, since the constructor takes a
> > temporary directory and it's a bit convoluted to have this fully constructed
> on
> > the stack.
> > 
> > (You need to (1) have a ScopedTempDir, (2) have a utility function to create
> it
> > and call CreateNewTempDir, assign it on the stack (3) call
> > scoped_temp_dir.Set(temp_dir)... easier to just call new at the right time).
> 
> Ah, that makes sense.  But surely it can be a scoped_ptr<> now, no?  Since
it's
> not required to be deleted via DeleteSoon now.

Yeah it can be a scoped ptr.  I had it this way but it looked strange to have
.reset(settings) in SetUp and .reset() in TearDown; it needs to be symmetrical,
and new/delete is clearly/idiomatically symmetrical wheras
.reset(settings)/.reset().... isn't so clearly.

Anyway, that's why I consciously didn't use scoped_ptr, but I don't feel
strongly.

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_sync_unittest.cc (right):

http://codereview.chromium.org/7775008/diff/63007/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_sync_unittest.cc:149:
ExtensionSettings* settings_;
On 2011/09/21 02:05:56, akalin wrote:
> On 2011/09/21 00:20:14, kalman1 wrote:
> > On 2011/09/20 14:53:11, akalin wrote:
> > > can just be 'ExtensionSettings settings_;'
> > 
> > Ditto previous comment.
> 
> As in the other place, can be scoped_ptr here, too.
> 
> (Even though you do end up manually resetting it anyway, it's still a good
> habit.)

Done.

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_storage_unittest.cc (right):

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage_unittest.cc:91: delete
settings_;
On 2011/09/21 02:05:57, akalin wrote:
> can do settings_.reset(); if scoped ptr

(same comment as before)

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_ui_wrapper.h (right):

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_ui_wrapper.h:32: // Gets the
SyncableService component of the ExtensionSettings.  The service
On 2011/09/21 01:58:41, akalin wrote:
> remove this, and instead have:
> 
> void RunWithExtensionSettings(const base::Callback<void(ExtensionSettings*)>
> callback); and make it analogous to RunWithStorage.

This should make RunWithStorage unnecessary, right?

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_ui_wrapper.h:41: class Core :
public base::RefCountedThreadSafe<Core>, public SyncableService {
On 2011/09/21 01:58:41, akalin wrote:
> no need to have this be a syncableservice anymore

Done.

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/sync/glue/ex...
File chrome/browser/sync/glue/extension_setting_data_type_controller.cc (right):

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/sync/glue/ex...
chrome/browser/sync/glue/extension_setting_data_type_controller.cc:19:
profile_sync_service_(profile_sync_service) {}
On 2011/09/21 01:58:41, akalin wrote:
> stash a pointer to ExtensionSettingsUIWrapper:
> 
>
extension_settings_ui_wrapper_(profile->GetExtensionService()->GetExtensionSettingsUIWrapper())
> 
> also DCHECK on UI thread

Done.

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/sync/glue/ex...
chrome/browser/sync/glue/extension_setting_data_type_controller.cc:39: if
(!BrowserThread::PostTask(
On 2011/09/21 01:58:41, akalin wrote:
> instead of this, do:
> 
>
extension_settings_ui_wrapper_->RunWithExtensionSettings(base::Bind(&StartAssociationWithExtensionSettings,
> this));

Done.

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/sync/glue/ex...
chrome/browser/sync/glue/extension_setting_data_type_controller.cc:48: }
On 2011/09/21 01:58:41, akalin wrote:
> have:
> 
> void StartAssociationWithExtensionSettings(ExtensionSettings*
> extension_settings) {
>   DCHECK(on file thread);
>   extension_settings_ = extension_settings;
>   // calls CreateSyncComponents.
>   StartAssociation();
> }

Done.

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/sync/glue/ex...
chrome/browser/sync/glue/extension_setting_data_type_controller.cc:54:
profile_sync_factory()->CreateExtensionSettingSyncComponents(
On 2011/09/21 01:58:41, akalin wrote:
> pass in extension_settings_ (DCHECK it's non-NULL) here

Done.

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/sync/glue/ex...
File chrome/browser/sync/glue/extension_setting_data_type_controller.h (right):

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/sync/glue/ex...
chrome/browser/sync/glue/extension_setting_data_type_controller.h:41: 
On 2011/09/21 01:58:41, akalin wrote:
> define:
> 
> void StartAssociationWithExtensionSettings(ExtensionSettings* settings);

Done.

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_factory_impl.cc (right):

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_factory_impl.cc:262: SyncableService*
sync_service =
On 2011/09/21 01:58:41, akalin wrote:
> DCHECK on file thread

Done.

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_factory_impl.cc:263:
profile_->GetExtensionService()->extension_settings()->
On 2011/09/21 01:58:41, akalin wrote:
> sync_service = extension_settings (or just use it directly)

Done.

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_factory_impl.h (right):

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_factory_impl.h:54:
browser_sync::UnrecoverableErrorHandler* error_handler);
On 2011/09/21 01:58:41, akalin wrote:
> have this take ExtensionSettings* extension_settings

Done.

[akalin, 2011-09-21 05:41:53]

LGTM after nits below!!

Your latest patch set has some files with missing statuses (the letter on the
left) so the commit bot will choke on those.  But you'll have to upload a new
patchset for these nits anyway.

Make sure to sync to head, and run trybots against at least
linux,win,mac,linux_sync,win_sync,mac_sync.  After that, I can land it for you
if the commit bot can't do it.

Thanks for doing this!

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_ui_wrapper.h (right):

http://codereview.chromium.org/7775008/diff/71001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_ui_wrapper.h:32: // Gets the
SyncableService component of the ExtensionSettings.  The service
On 2011/09/21 03:55:18, kalman1 wrote:
> On 2011/09/21 01:58:41, akalin wrote:
> > remove this, and instead have:
> > 
> > void RunWithExtensionSettings(const base::Callback<void(ExtensionSettings*)>
> > callback); and make it analogous to RunWithStorage.
> 
> This should make RunWithStorage unnecessary, right?

Yeah, that's true.  Cool.

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_setting_sync_data.cc (right):

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_sync_data.cc:76: DCHECK(value);
CHECK?  You're already CHECKing in ExtensionSettingSyncData anyway

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_sync_data.cc:76: DCHECK(value);
You're CHECKing in ExtensionSettingSyncData's constructor but DCHECKing here. 
Make them both be the same (I prefer both CHECKs).

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_setting_sync_data.h (right):

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_sync_data.h:12: #include
"chrome/browser/sync/api/sync_data.h"
i think you can forward declare SyncData

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_storage_unittest.h (right):

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage_unittest.h:63:
MessageLoopForUI ui_message_loop_;
I think this can be just MessageLoop message_loop_;

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_sync_unittest.cc (right):

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_sync_unittest.cc:169:
MessageLoopForUI ui_message_loop_;
i think this can just be MessageLoop message_loop_;

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/sync/glue/ex...
File chrome/browser/sync/glue/extension_setting_data_type_controller.cc (right):

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/sync/glue/ex...
chrome/browser/sync/glue/extension_setting_data_type_controller.cc:59: // Calls
CreateSyncComponents.
// Calls CreateSyncComponents, which expects extension_settings_ to be non-NULL.

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/sync/glue/ex...
File chrome/browser/sync/glue/extension_setting_data_type_controller.h (right):

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/sync/glue/ex...
chrome/browser/sync/glue/extension_setting_data_type_controller.h:13: #include
"chrome/browser/sync/profile_sync_factory.h"
forward-declare instead

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/sync/glue/ex...
chrome/browser/sync/glue/extension_setting_data_type_controller.h:51:
ExtensionSettings* extension_settings_;
add a comment that extension_settings_ is used only on the FILE thread, and the
other two are used only on the UI thread

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/sync/profile...
File chrome/browser/sync/profile_sync_factory.h (right):

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/sync/profile...
chrome/browser/sync/profile_sync_factory.h:101: // owned by the caller.
add that this is called on the FILE thread.

[not at google - send to devlin, 2011-09-21 06:22:41]

Thanks Fred.

I've synced to tip... I'm having problems running the try bots though.  They all
instantly fail in the updating stage with

remoteFailed: [Failure instance: Traceback: <class
'twisted.spread.banana.BananaError'>: string is too long to send (183196308)

(see
http://build.chromium.org/p/tryserver.chromium/builders/mac_sync/builds/5306/...
-- all the other bots are the same)

so perhaps the patch is too large for the trybots (i.e. twisted/python) to
handle?  I've replied to the failure email (as suggested), so... hopefully I'll
be someway towards a resolution when I wake up tomorrow.

Also the missing status thing hasn't resolved itself.  I don't know what that
means.

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_setting_sync_data.cc (right):

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_sync_data.cc:76: DCHECK(value);
On 2011/09/21 05:41:53, akalin wrote:
> You're CHECKing in ExtensionSettingSyncData's constructor but DCHECKing here. 
> Make them both be the same (I prefer both CHECKs).

CHECK above because it will crash later anyway if it's not NULL and it would be
better to crash on construction (obviously).  DCHECK here because it will never
fail given the CHECK above (and because other construction points clearly pass
in non-NULL), so it might as well be compiled out.

Would make most sense to just remove the DCHECK altogether I think.

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_setting_sync_data.h (right):

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_sync_data.h:12: #include
"chrome/browser/sync/api/sync_data.h"
On 2011/09/21 05:41:53, akalin wrote:
> i think you can forward declare SyncData

Done.

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_storage_unittest.h (right):

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_storage_unittest.h:63:
MessageLoopForUI ui_message_loop_;
On 2011/09/21 05:41:53, akalin wrote:
> I think this can be just MessageLoop message_loop_;

Done.

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_settings_sync_unittest.cc (right):

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_settings_sync_unittest.cc:169:
MessageLoopForUI ui_message_loop_;
On 2011/09/21 05:41:53, akalin wrote:
> i think this can just be MessageLoop message_loop_;

Done.

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/sync/glue/ex...
File chrome/browser/sync/glue/extension_setting_data_type_controller.cc (right):

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/sync/glue/ex...
chrome/browser/sync/glue/extension_setting_data_type_controller.cc:59: // Calls
CreateSyncComponents.
On 2011/09/21 05:41:53, akalin wrote:
> // Calls CreateSyncComponents, which expects extension_settings_ to be
non-NULL.

Done.

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/sync/glue/ex...
File chrome/browser/sync/glue/extension_setting_data_type_controller.h (right):

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/sync/glue/ex...
chrome/browser/sync/glue/extension_setting_data_type_controller.h:13: #include
"chrome/browser/sync/profile_sync_factory.h"
On 2011/09/21 05:41:53, akalin wrote:
> forward-declare instead

Done.

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/sync/glue/ex...
chrome/browser/sync/glue/extension_setting_data_type_controller.h:51:
ExtensionSettings* extension_settings_;
On 2011/09/21 05:41:53, akalin wrote:
> add a comment that extension_settings_ is used only on the FILE thread, and
the
> other two are used only on the UI thread

Done.

[akalin, 2011-09-21 06:28:22]

Hunh, those errors are weird.  I'll copy your patch set and I'll see if I can
get it to run tryjobs successfully.

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_setting_sync_data.cc (right):

http://codereview.chromium.org/7775008/diff/71006/chrome/browser/extensions/e...
chrome/browser/extensions/extension_setting_sync_data.cc:76: DCHECK(value);
On 2011/09/21 06:22:41, kalman1 wrote:
> On 2011/09/21 05:41:53, akalin wrote:
> > You're CHECKing in ExtensionSettingSyncData's constructor but DCHECKing
here. 
> > Make them both be the same (I prefer both CHECKs).
> 
> CHECK above because it will crash later anyway if it's not NULL and it would
be
> better to crash on construction (obviously).  DCHECK here because it will
never
> fail given the CHECK above (and because other construction points clearly pass
> in non-NULL), so it might as well be compiled out.
> 
> Would make most sense to just remove the DCHECK altogether I think.

SGTM.

[akalin, 2011-09-21 06:49:09]

http://codereview.chromium.org/7775008/diff/66058/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/7775008/diff/66058/chrome/chrome_tests.gypi#ne...
chrome/chrome_tests.gypi:2327:
'browser/extensions/extension_settings_sync_unittest.cc',
this is in browser_tests, which is meant for tests that exercise the full
browser (each test runs with in its own browser process, etc.).  That executable
is pretty heavyweight.

I think ExtensionSettingsSyncTest can just be put in unit_tests.  And probably
the other extension_settings_*_unittest files.  Basically, if you inherit from
::testing::Test, it can go in unit_tests.  Only if you inherit from
InProcessBrowserTest (or ExtensionBrowserTest, or some other subclass) does it
need to be in browser_tests.

[akalin, 2011-09-21 06:52:52]

On 2011/09/21 06:49:09, akalin wrote:
> http://codereview.chromium.org/7775008/diff/66058/chrome/chrome_tests.gypi
> File chrome/chrome_tests.gypi (right):
> 
>
http://codereview.chromium.org/7775008/diff/66058/chrome/chrome_tests.gypi#ne...
> chrome/chrome_tests.gypi:2327:
> 'browser/extensions/extension_settings_sync_unittest.cc',
> this is in browser_tests, which is meant for tests that exercise the full
> browser (each test runs with in its own browser process, etc.).  That
executable
> is pretty heavyweight.
> 
> I think ExtensionSettingsSyncTest can just be put in unit_tests.  And probably
> the other extension_settings_*_unittest files.  Basically, if you inherit from
> ::testing::Test, it can go in unit_tests.  Only if you inherit from
> InProcessBrowserTest (or ExtensionBrowserTest, or some other subclass) does it
> need to be in browser_tests.

Uploaded my copy: http://codereview.chromium.org/7977018/

I took the liberty of moving the unittests files to unit_tests, as I suggested
above.

[akalin, 2011-09-21 19:08:07]

Committed as 102140.  Yay!