Dedupe code in SetIdleInstallInfo(), FinishIdleInstallInfo() and OnExtensionInstalled().

Dedupe code in SetIdleInstallInfo(), FinishIdleInstallInfo() and OnExtensionInstalled().

BUG=none


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171956

[awong, 2012-12-07 01:13:23]

Hi Marijn,

Here's my attempt to merge all these bits.  I've left some comments with words
like RESOLVE BEFORE COMMIT with specific questions that I'm not quite sure about
the answer to.  If you could let me know what you think on those points, that'd
be great.

FYI, in an upcoming up CL, I will be expanding SetIdleInstallInfo to handle
another type of delayed install so you might see that function name change soon.

-Albert

[Marijn Kruisselbrink, 2012-12-07 17:29:48]

https://codereview.chromium.org/11465016/diff/4004/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

https://codereview.chromium.org/11465016/diff/4004/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1694: // RESOLVE BEFORE COMMIT: Why
did this used to use UpdateExtensionPref
Why not? Most code seems to use UpdateExtensionPref instead of using
ScopedExtensionPrefUpdate directly.

https://codereview.chromium.org/11465016/diff/4004/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:2300: // RESOLVE ME BEFORE COMMIT:
We did not preivously avoid this for LOAD extensions in
Yes, I agree. No reason to not avoid this even in the delayed install case.

https://codereview.chromium.org/11465016/diff/4004/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:2307: // RESOLVE ME BEFORE COMMIT:
This could be created post commit, but it feels
I'm not sure if this actually works though. In FinishIdleInfo we use
DictionaryValue::MergeDictionary to get the delayed info in the correct
location, but MergeDictionary recursively merges dictionaries, so setting some
of these values to empty dictionaries won't actually result in the dictionaries
being erased.

https://codereview.chromium.org/11465016/diff/4004/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:2342: /* RESOLVE ME BEFORE COMMIT:
This conditional seems impossible to fail since
Isn't initial_state not set when its value would be
Extension::ENABLED_COMPONENT? So it seems like this condition can fail
(apparently that is for supporting new prefs with old chromes... see the comment
in PopulateExtensionInfoPrefs)

[awong, 2012-12-07 22:02:25]

https://codereview.chromium.org/11465016/diff/4004/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

https://codereview.chromium.org/11465016/diff/4004/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1694: // RESOLVE BEFORE COMMIT: Why
did this used to use UpdateExtensionPref
On 2012/12/07 17:29:49, Marijn Kruisselbrink wrote:
> Why not? Most code seems to use UpdateExtensionPref instead of using
> ScopedExtensionPrefUpdate directly.

Okay...I'll move back to UpdateExtensionPref().  It's just that all the other
XXIdleInstallInfo() functions use the ScopedExtensinPrefUpdate method.  Glad to
know there's nothing special.

https://codereview.chromium.org/11465016/diff/4004/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:2300: // RESOLVE ME BEFORE COMMIT:
We did not preivously avoid this for LOAD extensions in
On 2012/12/07 17:29:49, Marijn Kruisselbrink wrote:
> Yes, I agree. No reason to not avoid this even in the delayed install case.

Done.

https://codereview.chromium.org/11465016/diff/4004/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:2307: // RESOLVE ME BEFORE COMMIT:
This could be created post commit, but it feels
On 2012/12/07 17:29:49, Marijn Kruisselbrink wrote:
> I'm not sure if this actually works though. In FinishIdleInfo we use
> DictionaryValue::MergeDictionary to get the delayed info in the correct
> location, but MergeDictionary recursively merges dictionaries, so setting some
> of these values to empty dictionaries won't actually result in the
dictionaries
> being erased.

Ah! Good catch. Moved into FinishExtensionInfoPrefs.

https://codereview.chromium.org/11465016/diff/4004/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:2342: /* RESOLVE ME BEFORE COMMIT:
This conditional seems impossible to fail since
On 2012/12/07 17:29:49, Marijn Kruisselbrink wrote:
> Isn't initial_state not set when its value would be
> Extension::ENABLED_COMPONENT? So it seems like this condition can fail
> (apparently that is for supporting new prefs with old chromes... see the
comment
> in PopulateExtensionInfoPrefs)

Oh interesting. Yeah, that came in via a last minute merge.  So is skipping the
RegisterExtension() calls in extension_pref_value_map_ and
content_settings_store_ the right behavior?

[Marijn Kruisselbrink, 2012-12-07 22:15:09]

https://codereview.chromium.org/11465016/diff/4004/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

https://codereview.chromium.org/11465016/diff/4004/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:1694: // RESOLVE BEFORE COMMIT: Why
did this used to use UpdateExtensionPref
On 2012/12/07 22:02:25, awong wrote:
> On 2012/12/07 17:29:49, Marijn Kruisselbrink wrote:
> > Why not? Most code seems to use UpdateExtensionPref instead of using
> > ScopedExtensionPrefUpdate directly.
> 
> Okay...I'll move back to UpdateExtensionPref().  It's just that all the other
> XXIdleInstallInfo() functions use the ScopedExtensinPrefUpdate method.  Glad
to
> know there's nothing special.
Ah, yeah, RemoveIdleInstallInfo I didn't touch, and apparently the old unused
version used ScopedExtensionPrefUpdate, and FinishIdleInstallInfo was mostly
just copied from OnExtensionInstalled (and there it makes sense to use
ScopedExtensionPrefUpdate directly to make sure there's only one notification
rather than one for each property that is modified).

https://codereview.chromium.org/11465016/diff/4004/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:2342: /* RESOLVE ME BEFORE COMMIT:
This conditional seems impossible to fail since
On 2012/12/07 22:02:25, awong wrote:
> On 2012/12/07 17:29:49, Marijn Kruisselbrink wrote:
> > Isn't initial_state not set when its value would be
> > Extension::ENABLED_COMPONENT? So it seems like this condition can fail
> > (apparently that is for supporting new prefs with old chromes... see the
> comment
> > in PopulateExtensionInfoPrefs)
> 
> Oh interesting. Yeah, that came in via a last minute merge.  So is skipping
the
> RegisterExtension() calls in extension_pref_value_map_ and
> content_settings_store_ the right behavior?
It probably isn't right to skip these calls. OnExtensionInstalled always called
these methods.

[awong, 2012-12-07 22:56:52]

PTAL

https://codereview.chromium.org/11465016/diff/4004/chrome/browser/extensions/...
File chrome/browser/extensions/extension_prefs.cc (right):

https://codereview.chromium.org/11465016/diff/4004/chrome/browser/extensions/...
chrome/browser/extensions/extension_prefs.cc:2342: /* RESOLVE ME BEFORE COMMIT:
This conditional seems impossible to fail since
On 2012/12/07 22:15:09, Marijn Kruisselbrink wrote:
> On 2012/12/07 22:02:25, awong wrote:
> > On 2012/12/07 17:29:49, Marijn Kruisselbrink wrote:
> > > Isn't initial_state not set when its value would be
> > > Extension::ENABLED_COMPONENT? So it seems like this condition can fail
> > > (apparently that is for supporting new prefs with old chromes... see the
> > comment
> > > in PopulateExtensionInfoPrefs)
> > 
> > Oh interesting. Yeah, that came in via a last minute merge.  So is skipping
> the
> > RegisterExtension() calls in extension_pref_value_map_ and
> > content_settings_store_ the right behavior?
> It probably isn't right to skip these calls. OnExtensionInstalled always
called
> these methods.

Added some logic that I *think* has the correct behavior.

[Marijn Kruisselbrink, 2012-12-07 22:59:53]

lgtm

https://codereview.chromium.org/11465016/diff/13004/chrome/browser/extensions...
File chrome/browser/extensions/extension_prefs.cc (right):

https://codereview.chromium.org/11465016/diff/13004/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs.cc:2336: initial_state =
Extension::ENABLED_COMPONENT;
The code only cares if initial_state == ENABLED, so it might be cleaner to just
treat not having an initial state as that comparison being false, without
hard-coding some somewhat arbitrary state as default? But this is fine with me
too.

[awong, 2012-12-07 23:06:53]

one more look?

https://codereview.chromium.org/11465016/diff/13004/chrome/browser/extensions...
File chrome/browser/extensions/extension_prefs.cc (right):

https://codereview.chromium.org/11465016/diff/13004/chrome/browser/extensions...
chrome/browser/extensions/extension_prefs.cc:2336: initial_state =
Extension::ENABLED_COMPONENT;
On 2012/12/07 22:59:53, Marijn Kruisselbrink wrote:
> The code only cares if initial_state == ENABLED, so it might be cleaner to
just
> treat not having an initial state as that comparison being false, without
> hard-coding some somewhat arbitrary state as default? But this is fine with me
> too.

Done.

[Marijn Kruisselbrink, 2012-12-07 23:07:44]

On 2012/12/07 23:06:53, awong wrote:
> one more look?
still lgtm

[commit-bot: I haz the power, 2012-12-07 23:14:17]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[awong, 2012-12-07 23:38:38]

mpcomplete: Care to do OWNERS?

On Fri, Dec 7, 2012 at 3:14 PM, <commit-bot@chromium.org> wrote:

> No LGTM from a valid reviewer yet. Only full committers are accepted.
> Even if an LGTM may have been provided, it was from a non-committer or
> a lowly provisional committer, _not_ a full super star committer.
> See
http://www.chromium.org/**getting-involved/become-a-**committer<http://www.ch...
> Note that this has nothing to do with OWNERS files.
>
>
https://chromiumcodereview.**appspot.com/11465016/<https://chromiumcodereview...
>

[Matt Perry, 2012-12-07 23:58:06]

lgtm

[awong, 2012-12-08 02:00:12]

Found a small bug (pesky ! in conditional).  Also added some function comments. 
Will commit unless there is an objection.

[commit-bot: I haz the power, 2012-12-08 02:00:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajwong@chromium.org/11465016/2012

[commit-bot: I haz the power, 2012-12-08 07:05:57]

Change committed as 171956