Remove a bunch of DeepCopy() calls in the chrome.storage API.

Remove a bunch of DeepCopy() calls in the chrome.storage API.

DeepCopy() is expensive and we're doing a lot of it, transferring ownership is
better. I also tidied up some code here and there, like C++11 niceties,
removing unnecessary linked_ptrs, and better commenting.

R=rdevlin.cronin@chromium.org

Committed: https://crrev.com/6387496ebfee135bca59d8043a30d12eed57e420
Cr-Commit-Position: refs/heads/master@{#330597}

[Devlin, 2015-05-15 21:59:08]

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/setting_sync_data.cc (right):

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/setting_sync_data.cc:85:
DCHECK(internal_.get()) << "value has been released";
Should these be DCHECKing internal_->value_?

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/setting_sync_data.h (right):

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/setting_sync_data.h:56: // Be careful when
calling this, as SettingSyncData is effectively a
Hmm...

Generally, it's best to avoid saying "Be careful, because it's really easy to
shoot yourself in the foot with this."

Without looking into this too much, it seems possible to make SettingSyncData
DISALLOW_COPY_AND_ASSIGN, since we could use a ScopedVector for
SettingSyncDataList.  But I haven't delved deep enough to know if we could... is
there a hard blocker preventing that?

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/sync_storage_backend.cc (right):

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/sync_storage_backend.cc:179:
std::map<std::string, base::DictionaryValue*> grouped_sync_data;
This is scary - easy leaking if we ever early-return, which, in a function of
this size, is not entirely unlikely.  At the same time, linked_ptrs are evil, so
I like getting rid of them.

I ran an idea past Jeffrey, and he says it's not totally crazy.  So, what if we
have something like a scoped assert, which yells if the function early-returns
(and the assert isn't reset)?

It would be something like:
std::map<string, dictionary> grouped_sync_data;
ScopedAssert grouped_sync_data_guard;  // Make sure we don't leak anything.
<...all code....>
for (const auto& group : grouped_sync_data)
  GetOrCreateStorageWithSyncData(group.first, make_scoped_ptr(group.second));
grouped_sync_data_guard.Reset();  // All data safely passed.

That would ensure that we never leak anything (or, if we do, we CHECK).

For a ScopedAssert, you could make your own object (which would be really
trivial - about 10 - 20 lines, including boilerplate) or use the
base::ScopedClosureRunner.

WDYT?

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/sync_storage_backend.cc:182:
base::DictionaryValue*& settings = grouped_sync_data[data.extension_id()];
This is kinda cool, but honestly, it might be worth a comment to say "No, this
isn't as crazy as it looks."

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/sync_storage_backend.cc:200:
make_scoped_ptr(grouped_sync_data[extension_id]),
this does result in a trip lookup (one in the if on line 198, one here, and one
on line 208 (that was always there)), which is probably why we had the find
before.  I think it's probably best to keep it like that, and just clean it up a
touch.

auto iter = grouped_sync_data.find(extension_id);
if (iter != grouped_sync_data.end()) {
  error = ...
  grouped_sync_data.erase(iter);
}

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/sync_storage_backend.cc:230:
std::map<std::string, SettingSyncDataList*> grouped_sync_data;
ditto here, though this one's a little less scary since it's a much smaller
function.

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/syncable_settings_storage.cc (right):

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/syncable_settings_storage.cc:229: // Sync
and local values are the same, no changes to send.
nit: Empty if statement?  Weird.  Wanna take it out?

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/syncable_settings_storage.h (right):

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/syncable_settings_storage.h:83: //
|local_state| the local storage state.
nitty nit: missing an is, here.  That said, I don't think this line really adds
much, and this isn't closure compiler (so you don't need to comment on each
parameter).

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/syncable_settings_storage.h:89: //
|local_state| the local storage state. Will be overwritten by |sync_state|.
nit: Also missing "is"s here (|local_state| _is_...).  These are also in the
opposite order that they appear in the parameter list (I only mention because
they have the same type, so getting them mixed up in a call is easier).

[not at google - send to devlin, 2015-05-15 23:28:30]

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/setting_sync_data.cc (right):

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/setting_sync_data.cc:85:
DCHECK(internal_.get()) << "value has been released";
On 2015/05/15 21:59:07, Devlin wrote:
> Should these be DCHECKing internal_->value_?

yes

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/setting_sync_data.h (right):

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/setting_sync_data.h:56: // Be careful when
calling this, as SettingSyncData is effectively a
On 2015/05/15 21:59:07, Devlin wrote:
> Hmm...
> 
> Generally, it's best to avoid saying "Be careful, because it's really easy to
> shoot yourself in the foot with this."
> 
> Without looking into this too much, it seems possible to make SettingSyncData
> DISALLOW_COPY_AND_ASSIGN, since we could use a ScopedVector for
> SettingSyncDataList.  But I haven't delved deep enough to know if we could...
is
> there a hard blocker preventing that?

The blocker is this diff blowing up even more, but done. Simplified some code,
complicated some tests. Win?

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/sync_storage_backend.cc (right):

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/sync_storage_backend.cc:179:
std::map<std::string, base::DictionaryValue*> grouped_sync_data;
On 2015/05/15 21:59:07, Devlin wrote:
> This is scary - easy leaking if we ever early-return, which, in a function of
> this size, is not entirely unlikely.  At the same time, linked_ptrs are evil,
so
> I like getting rid of them.
> 
> I ran an idea past Jeffrey, and he says it's not totally crazy.  So, what if
we
> have something like a scoped assert, which yells if the function early-returns
> (and the assert isn't reset)?
> 
> It would be something like:
> std::map<string, dictionary> grouped_sync_data;
> ScopedAssert grouped_sync_data_guard;  // Make sure we don't leak anything.
> <...all code....>
> for (const auto& group : grouped_sync_data)
>   GetOrCreateStorageWithSyncData(group.first, make_scoped_ptr(group.second));
> grouped_sync_data_guard.Reset();  // All data safely passed.
> 
> That would ensure that we never leak anything (or, if we do, we CHECK).
> 
> For a ScopedAssert, you could make your own object (which would be really
> trivial - about 10 - 20 lines, including boilerplate) or use the
> base::ScopedClosureRunner.
> 
> WDYT?

I don't really see the point. The reason to have RAII is to get compile time
safety, not to get some runtime error. Changing this back to linked pointer
seems far easier, it's just a PITA to work with, especially the conversions
between linked and scoped which this CL would introduce.

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/sync_storage_backend.cc:182:
base::DictionaryValue*& settings = grouped_sync_data[data.extension_id()];
On 2015/05/15 21:59:07, Devlin wrote:
> This is kinda cool, but honestly, it might be worth a comment to say "No, this
> isn't as crazy as it looks."

haha ok.

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/sync_storage_backend.cc:200:
make_scoped_ptr(grouped_sync_data[extension_id]),
On 2015/05/15 21:59:07, Devlin wrote:
> this does result in a trip lookup (one in the if on line 198, one here, and
one
> on line 208 (that was always there)), which is probably why we had the find
> before.  I think it's probably best to keep it like that, and just clean it up
a
> touch.
> 
> auto iter = grouped_sync_data.find(extension_id);
> if (iter != grouped_sync_data.end()) {
>   error = ...
>   grouped_sync_data.erase(iter);
> }

Done.

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/syncable_settings_storage.cc (right):

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/syncable_settings_storage.cc:229: // Sync
and local values are the same, no changes to send.
On 2015/05/15 21:59:07, Devlin wrote:
> nit: Empty if statement?  Weird.  Wanna take it out?

I like it in there for documentation.

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/syncable_settings_storage.h (right):

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/syncable_settings_storage.h:83: //
|local_state| the local storage state.
On 2015/05/15 21:59:08, Devlin wrote:
> nitty nit: missing an is, here.  That said, I don't think this line really
adds
> much, and this isn't closure compiler (so you don't need to comment on each
> parameter).

well, sometimes comments on parameters are good, regardless if they happen to go
through a compiler. but I agree in this case it's not that useful (they were
actually from before I decided to rename the parameters).

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/syncable_settings_storage.h:89: //
|local_state| the local storage state. Will be overwritten by |sync_state|.
On 2015/05/15 21:59:08, Devlin wrote:
> nit: Also missing "is"s here (|local_state| _is_...).  These are also in the
> opposite order that they appear in the parameter list (I only mention because
> they have the same type, so getting them mixed up in a call is easier).

Done.

[Devlin, 2015-05-15 23:58:19]

I think this lgtm, but my brain is a bit burnt.  If you're in a hurry, I'll
trust that this works; otherwise I'll take a second look on Monday.

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/setting_sync_data.h (right):

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/setting_sync_data.h:56: // Be careful when
calling this, as SettingSyncData is effectively a
On 2015/05/15 23:28:30, kalman wrote:
> On 2015/05/15 21:59:07, Devlin wrote:
> > Hmm...
> > 
> > Generally, it's best to avoid saying "Be careful, because it's really easy
to
> > shoot yourself in the foot with this."
> > 
> > Without looking into this too much, it seems possible to make
SettingSyncData
> > DISALLOW_COPY_AND_ASSIGN, since we could use a ScopedVector for
> > SettingSyncDataList.  But I haven't delved deep enough to know if we
could...
> is
> > there a hard blocker preventing that?
> 
> The blocker is this diff blowing up even more, but done. Simplified some code,
> complicated some tests. Win?

Win.

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/sync_storage_backend.cc (right):

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/sync_storage_backend.cc:179:
std::map<std::string, base::DictionaryValue*> grouped_sync_data;
On 2015/05/15 23:28:30, kalman wrote:
> On 2015/05/15 21:59:07, Devlin wrote:
> > This is scary - easy leaking if we ever early-return, which, in a function
of
> > this size, is not entirely unlikely.  At the same time, linked_ptrs are
evil,
> so
> > I like getting rid of them.
> > 
> > I ran an idea past Jeffrey, and he says it's not totally crazy.  So, what if
> we
> > have something like a scoped assert, which yells if the function
early-returns
> > (and the assert isn't reset)?
> > 
> > It would be something like:
> > std::map<string, dictionary> grouped_sync_data;
> > ScopedAssert grouped_sync_data_guard;  // Make sure we don't leak anything.
> > <...all code....>
> > for (const auto& group : grouped_sync_data)
> >   GetOrCreateStorageWithSyncData(group.first,
make_scoped_ptr(group.second));
> > grouped_sync_data_guard.Reset();  // All data safely passed.
> > 
> > That would ensure that we never leak anything (or, if we do, we CHECK).
> > 
> > For a ScopedAssert, you could make your own object (which would be really
> > trivial - about 10 - 20 lines, including boilerplate) or use the
> > base::ScopedClosureRunner.
> > 
> > WDYT?
> 
> I don't really see the point. The reason to have RAII is to get compile time
> safety, not to get some runtime error. Changing this back to linked pointer
> seems far easier, it's just a PITA to work with, especially the conversions
> between linked and scoped which this CL would introduce.

Meh, fine then. :P  At the very least, can you document why it's safe?

https://codereview.chromium.org/1141963002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/setting_sync_data.cc (right):

https://codereview.chromium.org/1141963002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/setting_sync_data.cc:18:
Init(sync_change.sync_data());
nit: The fact that this is called Init() but isn't called from all constructors
almost seems like a bug at first glance.  Can we rename it to something like
InitFromSyncData or something?  Or, if you wanted, you could use delegated
constructors between these two.

https://codereview.chromium.org/1141963002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/setting_sync_data.h (right):

https://codereview.chromium.org/1141963002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/setting_sync_data.h:73:
DISALLOW_COPY_AND_ASSIGN(SettingSyncData);
Woo!

https://codereview.chromium.org/1141963002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/sync_storage_backend.cc (right):

https://codereview.chromium.org/1141963002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/sync_storage_backend.cc:182: // (yes this
really is a reference to a pointer)
nitty nitty nit: Capitalization and punctuation.  Since I'm the one complaining,
here's the copy-paste.
// (Yes, this really is a reference to a pointer.)

[Devlin, 2015-05-19 15:41:55]

A few more nits, but still lgtm.

https://codereview.chromium.org/1141963002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/setting_sync_data.cc (right):

https://codereview.chromium.org/1141963002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/setting_sync_data.cc:48: if
(specifics.has_extension_setting())
nit: Ternary?  Up to you.

https://codereview.chromium.org/1141963002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/setting_sync_data.h (right):

https://codereview.chromium.org/1141963002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/setting_sync_data.h:45: // Returns the
type of the sync change; may be ACTION_INVALID.
nit: We almost never document simple accessors.  Most of these comments, if
they're not totally silly ("Returns the settings key" is pretty silly) should be
moved to the member variable.  If you feel up for the move, please do,
otherwise, whatevs.

https://codereview.chromium.org/1141963002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/syncable_settings_storage.cc (right):

https://codereview.chromium.org/1141963002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/syncable_settings_storage.cc:312: // race
and change's not a bug, so pretend change's an update.
nit: I think these should read "the change", unless there's a variable called
"change" (in which case it should read "|change|"

[not at google - send to devlin, 2015-05-19 19:43:58]

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/sync_storage_backend.cc (right):

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/sync_storage_backend.cc:179:
std::map<std::string, base::DictionaryValue*> grouped_sync_data;
On 2015/05/15 23:58:18, Devlin wrote:
> On 2015/05/15 23:28:30, kalman wrote:
> > On 2015/05/15 21:59:07, Devlin wrote:
> > > This is scary - easy leaking if we ever early-return, which, in a function
> of
> > > this size, is not entirely unlikely.  At the same time, linked_ptrs are
> evil,
> > so
> > > I like getting rid of them.
> > > 
> > > I ran an idea past Jeffrey, and he says it's not totally crazy.  So, what
if
> > we
> > > have something like a scoped assert, which yells if the function
> early-returns
> > > (and the assert isn't reset)?
> > > 
> > > It would be something like:
> > > std::map<string, dictionary> grouped_sync_data;
> > > ScopedAssert grouped_sync_data_guard;  // Make sure we don't leak
anything.
> > > <...all code....>
> > > for (const auto& group : grouped_sync_data)
> > >   GetOrCreateStorageWithSyncData(group.first,
> make_scoped_ptr(group.second));
> > > grouped_sync_data_guard.Reset();  // All data safely passed.
> > > 
> > > That would ensure that we never leak anything (or, if we do, we CHECK).
> > > 
> > > For a ScopedAssert, you could make your own object (which would be really
> > > trivial - about 10 - 20 lines, including boilerplate) or use the
> > > base::ScopedClosureRunner.
> > > 
> > > WDYT?
> > 
> > I don't really see the point. The reason to have RAII is to get compile time
> > safety, not to get some runtime error. Changing this back to linked pointer
> > seems far easier, it's just a PITA to work with, especially the conversions
> > between linked and scoped which this CL would introduce.
> 
> Meh, fine then. :P  At the very least, can you document why it's safe?

Documented.

https://codereview.chromium.org/1141963002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/sync_storage_backend.cc:230:
std::map<std::string, SettingSyncDataList*> grouped_sync_data;
On 2015/05/15 21:59:07, Devlin wrote:
> ditto here, though this one's a little less scary since it's a much smaller
> function.

Documented.

https://codereview.chromium.org/1141963002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/setting_sync_data.cc (right):

https://codereview.chromium.org/1141963002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/setting_sync_data.cc:18:
Init(sync_change.sync_data());
On 2015/05/15 23:58:18, Devlin wrote:
> nit: The fact that this is called Init() but isn't called from all
constructors
> almost seems like a bug at first glance.  Can we rename it to something like
> InitFromSyncData or something?  Or, if you wanted, you could use delegated
> constructors between these two.

I looked into changing this to something better factored. Unfortunately
delegated constructors won't work because the difference is unwrapping either
|sync_change| or |sync_data| into its components - and there isn't away to
express that. Even if there was it would be a little verbose needing to declare
all 4 real constructor arguments, have some function "PullApart()" which assigns
to each of them, then "delegate" construction.

I can rename from "Init" because I agree it's an overloaded term.

https://codereview.chromium.org/1141963002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/setting_sync_data.cc:48: if
(specifics.has_extension_setting())
On 2015/05/19 15:41:54, Devlin wrote:
> nit: Ternary?  Up to you.

hah I do love ternary :)

https://codereview.chromium.org/1141963002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/setting_sync_data.h (right):

https://codereview.chromium.org/1141963002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/setting_sync_data.h:45: // Returns the
type of the sync change; may be ACTION_INVALID.
On 2015/05/19 15:41:55, Devlin wrote:
> nit: We almost never document simple accessors.  Most of these comments, if
> they're not totally silly ("Returns the settings key" is pretty silly) should
be
> moved to the member variable.  If you feel up for the move, please do,
> otherwise, whatevs.

This comment serves a purpose.

https://codereview.chromium.org/1141963002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/setting_sync_data.h:50: // Returns the
extension id the setting is for.
this does not

https://codereview.chromium.org/1141963002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/setting_sync_data.h:53: // Returns the
settings key.
nor does this

https://codereview.chromium.org/1141963002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/setting_sync_data.h:57: // called.
this does.

https://codereview.chromium.org/1141963002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/setting_sync_data.h:60: // Releases
ownership of the value to the caller.
I should improve this comment because it's important to say that neither
PassValue() nor value() can be called from this point onwards.

with that in mind, it's visually consistent to document every accessor, but I
can remove them anyway.

https://codereview.chromium.org/1141963002/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/api/storage/sync_storage_backend.cc (right):

https://codereview.chromium.org/1141963002/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/api/storage/sync_storage_backend.cc:182: // (yes this
really is a reference to a pointer)
On 2015/05/15 23:58:18, Devlin wrote:
> nitty nitty nit: Capitalization and punctuation.  Since I'm the one
complaining,
> here's the copy-paste.
> // (Yes, this really is a reference to a pointer.)

(but it's not a sentence, so doesn't necessarily need capitalization)
So, fine, I made it a sentence.

[not at google - send to devlin, 2015-05-19 19:44:02]

The CQ bit was checked by kalman@chromium.org

[not at google - send to devlin, 2015-05-19 19:44:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1141963002/#ps60001
(title: "comments")

[commit-bot: I haz the power, 2015-05-19 19:44:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1141963002/60001

[commit-bot: I haz the power, 2015-05-19 20:35:47]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-05-19 20:36:45]

Patchset 4 (id:??) landed as
https://crrev.com/6387496ebfee135bca59d8043a30d12eed57e420
Cr-Commit-Position: refs/heads/master@{#330597}