Extensions - Check for too big or too small product icons.

Extensions - Check for too big or too small manifest icons.

both product and browser action icons are affected by this check

BUG=none

Committed: https://crrev.com/63a6923752c66e597b8a10eddeada4f2ef1e82c9
Cr-Commit-Position: refs/heads/master@{#371311}

[Evan Stade, 2016-01-22 01:03:48]

estade@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Evan Stade, 2016-01-22 01:03:53]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-22 01:05:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1618073002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1618073002/1

[commit-bot: I haz the power, 2016-01-22 01:45:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-22 01:45:02]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-01-22 23:41:56]

https://codereview.chromium.org/1618073002/diff/1/extensions/common/manifest_...
File extensions/common/manifest_handler_helpers.cc (right):

https://codereview.chromium.org/1618073002/diff/1/extensions/common/manifest_...
extensions/common/manifest_handler_helpers.cc:49: *error =
ErrorUtils::FormatErrorMessageUTF16(errors::kInvalidIconPath,
We should add a separate error for these (invalid icon path doesn't really work
for icon is too big).

https://codereview.chromium.org/1618073002/diff/1/extensions/common/manifest_...
File extensions/common/manifest_handlers/icons_handler_unittest.cc (right):

https://codereview.chromium.org/1618073002/diff/1/extensions/common/manifest_...
extensions/common/manifest_handlers/icons_handler_unittest.cc:30:
scoped_ptr<base::DictionaryValue> manifest = base::DictionaryValue::From(
IMO, extensions::DictionaryBuilder makes this cleaner, but I don't feel super
strongly.

https://codereview.chromium.org/1618073002/diff/1/extensions/common/manifest_...
extensions/common/manifest_handlers/icons_handler_unittest.cc:41:
manifest->SetString(manifest_keys::kKey, kExtensionKey);
Is this needed for anything?

[Evan Stade, 2016-01-23 02:28:37]

https://codereview.chromium.org/1618073002/diff/1/extensions/common/manifest_...
File extensions/common/manifest_handler_helpers.cc (right):

https://codereview.chromium.org/1618073002/diff/1/extensions/common/manifest_...
extensions/common/manifest_handler_helpers.cc:49: *error =
ErrorUtils::FormatErrorMessageUTF16(errors::kInvalidIconPath,
On 2016/01/22 23:41:56, Devlin (Slow until 1-27) wrote:
> We should add a separate error for these (invalid icon path doesn't really
work
> for icon is too big).

Done.

https://codereview.chromium.org/1618073002/diff/1/extensions/common/manifest_...
File extensions/common/manifest_handlers/icons_handler_unittest.cc (right):

https://codereview.chromium.org/1618073002/diff/1/extensions/common/manifest_...
extensions/common/manifest_handlers/icons_handler_unittest.cc:30:
scoped_ptr<base::DictionaryValue> manifest = base::DictionaryValue::From(
On 2016/01/22 23:41:56, Devlin (Slow until 1-27) wrote:
> IMO, extensions::DictionaryBuilder makes this cleaner, but I don't feel super
> strongly.

Acknowledged.

https://codereview.chromium.org/1618073002/diff/1/extensions/common/manifest_...
extensions/common/manifest_handlers/icons_handler_unittest.cc:41:
manifest->SetString(manifest_keys::kKey, kExtensionKey);
On 2016/01/22 23:41:56, Devlin (Slow until 1-27) wrote:
> Is this needed for anything?

Done.

[Evan Stade, 2016-01-23 02:28:41]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-01-23 02:29:02]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1618073002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1618073002/20001

[commit-bot: I haz the power, 2016-01-23 03:30:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-01-23 03:30:23]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-01-25 17:58:16]

lgtm

[Devlin, 2016-01-25 17:59:00]

Side note: doesn't this also check for manifest-specified action icons?  (So we
should probably update the CL description slightly.)

[Evan Stade, 2016-01-25 20:02:28]

On 2016/01/25 17:59:00, Devlin (Slow until 1-27) wrote:
> Side note: doesn't this also check for manifest-specified action icons?  (So
we
> should probably update the CL description slightly.)

I don't think so. Manifest action icons are loaded here:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

[Evan Stade, 2016-01-25 20:02:34]

The CQ bit was checked by estade@chromium.org

[Devlin, 2016-01-25 20:08:12]

On 2016/01/25 20:02:28, Evan Stade wrote:
> On 2016/01/25 17:59:00, Devlin (Slow until 1-27) wrote:
> > Side note: doesn't this also check for manifest-specified action icons?  (So
> we
> > should probably update the CL description slightly.)
> 
> I don't think so. Manifest action icons are loaded here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/common/exte...

(Note that "kPageActionDefaultIcon" refers to the default icon for both page and
browser actions.)

[commit-bot: I haz the power, 2016-01-25 20:11:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1618073002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1618073002/20001

[Evan Stade, 2016-01-25 20:28:39]

Description was changed from

==========
Extensions - Check for too big or too small product icons.

BUG=none
==========

to

==========
Extensions - Check for too big or too small manifest icons.

both product and browser action icons are affected by this check

BUG=none
==========

[Evan Stade, 2016-01-25 20:28:56]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2016-01-25 20:29:07]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-01-25 20:33:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1618073002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1618073002/20001

[commit-bot: I haz the power, 2016-01-25 21:07:37]

Description was changed from

==========
Extensions - Check for too big or too small manifest icons.

both product and browser action icons are affected by this check

BUG=none
==========

to

==========
Extensions - Check for too big or too small manifest icons.

both product and browser action icons are affected by this check

BUG=none
==========

[commit-bot: I haz the power, 2016-01-25 21:07:38]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-01-25 21:09:12]

Description was changed from

==========
Extensions - Check for too big or too small manifest icons.

both product and browser action icons are affected by this check

BUG=none
==========

to

==========
Extensions - Check for too big or too small manifest icons.

both product and browser action icons are affected by this check

BUG=none

Committed: https://crrev.com/63a6923752c66e597b8a10eddeada4f2ef1e82c9
Cr-Commit-Position: refs/heads/master@{#371311}
==========

[commit-bot: I haz the power, 2016-01-25 21:09:12]

Patchset 2 (id:??) landed as
https://crrev.com/63a6923752c66e597b8a10eddeada4f2ef1e82c9
Cr-Commit-Position: refs/heads/master@{#371311}