Allow an empty list of page actions.

Allow an empty list of page actions.

BUG=26050
TEST=Updated existing unit tests ExtensionTest.InitFromValue*, manual testing on Linux.

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

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

[Finnur, 2010-01-07 18:17:03]

LGTM (with nits)

http://codereview.chromium.org/523132/diff/1/2
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/523132/diff/1/2#newcode1081
chrome/common/extensions/extension.cc:1081: DictionaryValue* page_action_value =
NULL;
nit: This seems a little out of place, maybe move it under the comment below?

http://codereview.chromium.org/523132/diff/1/2#newcode1096
chrome/common/extensions/extension.cc:1096: page_action_value = NULL;
nit: This is redundant. You already initialized it to NULL.

http://codereview.chromium.org/523132/diff/1/2#newcode1102
chrome/common/extensions/extension.cc:1102: } else {  // list_value_length > 1u
nit: End all comments with period.

[Sam Kerner (Chrome), 2010-01-07 20:05:03]

http://codereview.chromium.org/523132/diff/1/2
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/523132/diff/1/2#newcode1081
chrome/common/extensions/extension.cc:1081: DictionaryValue* page_action_value =
NULL;
On 2010/01/07 18:17:03, Finnur wrote:
> nit: This seems a little out of place, maybe move it under the comment below?

Done.

http://codereview.chromium.org/523132/diff/1/2#newcode1096
chrome/common/extensions/extension.cc:1096: page_action_value = NULL;
On 2010/01/07 18:17:03, Finnur wrote:
> nit: This is redundant. You already initialized it to NULL.

Done.

http://codereview.chromium.org/523132/diff/1/2#newcode1102
chrome/common/extensions/extension.cc:1102: } else {  // list_value_length > 1u
On 2010/01/07 18:17:03, Finnur wrote:
> nit: End all comments with period.

Done.

[Finnur, 2010-01-07 20:10:01]

Still LG. Two nits:

http://codereview.chromium.org/523132/diff/3001/4001
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/523132/diff/3001/4001#newcode1101
chrome/common/extensions/extension.cc:1101: } else {  // list_value_length > 1u
.
nit: Why the extra space in front of the period?

http://codereview.chromium.org/523132/diff/3001/4002
File chrome/common/extensions/extension_constants.cc (right):

http://codereview.chromium.org/523132/diff/3001/4002#newcode125
chrome/common/extensions/extension_constants.cc:125: 
nit: Why add line breaks?

[Sam Kerner (Chrome), 2010-01-07 20:21:20]

http://codereview.chromium.org/523132/diff/3001/4001
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/523132/diff/3001/4001#newcode1101
chrome/common/extensions/extension.cc:1101: } else {  // list_value_length > 1u
.
On 2010/01/07 20:10:01, Finnur wrote:
> nit: Why the extra space in front of the period?

I was following the convention to add a space between numbers and periods, to
avoid confusion with a decimal point.  The 'u' makes this a corner case.  Done.

http://codereview.chromium.org/523132/diff/3001/4002
File chrome/common/extensions/extension_constants.cc (right):

http://codereview.chromium.org/523132/diff/3001/4002#newcode125
chrome/common/extensions/extension_constants.cc:125: 
On 2010/01/07 20:10:01, Finnur wrote:
> nit: Why add line breaks?

Merge artifact.  Removed.

[Finnur, 2010-01-11 00:21:09]

Patch set 7 with mem leak fix LGTM.

On 2010/01/07 20:21:20, Sam Kerner (Chrome) wrote:
> http://codereview.chromium.org/523132/diff/3001/4001
> File chrome/common/extensions/extension.cc (right):
> 
> http://codereview.chromium.org/523132/diff/3001/4001#newcode1101
> chrome/common/extensions/extension.cc:1101: } else {  // list_value_length >
1u
> .
> On 2010/01/07 20:10:01, Finnur wrote:
> > nit: Why the extra space in front of the period?
> 
> I was following the convention to add a space between numbers and periods, to
> avoid confusion with a decimal point.  The 'u' makes this a corner case. 
Done.
> 
> http://codereview.chromium.org/523132/diff/3001/4002
> File chrome/common/extensions/extension_constants.cc (right):
> 
> http://codereview.chromium.org/523132/diff/3001/4002#newcode125
> chrome/common/extensions/extension_constants.cc:125: 
> On 2010/01/07 20:10:01, Finnur wrote:
> > nit: Why add line breaks?
> 
> Merge artifact.  Removed.