Change extension icon load errors to warnings

Change extension icon load errors to warnings

During the Extension parsing step, check if the icon file exists and if not, remove that entry from the dictionary.

Keep the same check during the validation phase and don't apply the workaround to unpacked extensions. This will more strongly discourage new extensions from making this mistake.

BUG=570249
Committed: https://crrev.com/6e8e7d1c49657e82d0e8f2518ad463794346321b
Cr-Commit-Position: refs/heads/master@{#366253}

[Evan Stade, 2015-12-17 04:16:23]

Description was changed from

==========
WIP - change extension icon load errors to warnings

BUG=
==========

to

==========
Change extension icon load errors to warnings

During the Extension parsing step, check if the icon file exists and if not,
remove that entry from the dictionary.

Keep the same check during the validation phase and don't apply the workaround
to unpacked extensions. This will more strongly discourage new extensions from
making this mistake.

BUG=570249
==========

[Evan Stade, 2015-12-17 04:16:23]

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

[Evan Stade, 2015-12-17 04:27:57]

https://codereview.chromium.org/1537473003/diff/20001/extensions/common/manif...
File extensions/common/manifest_handler_helpers.cc (right):

https://codereview.chromium.org/1537473003/diff/20001/extensions/common/manif...
extensions/common/manifest_handler_helpers.cc:60: file_util::ValidateFilePath(
to be honest, I'm not super happy with this because it seems like a violation of
the Parse/Validate contract (the former just looks at the manifest and the
latter checks that the manifest makes sense relative to the rest of the
extension pack). However, editing the icon set during validation would require
breaking a lot of const qualifiers (on Extension, ActionIcon, etc.).

I'd be more happy just telling extension developers that what was previously
undefined behavior but clearly broken and just happened to be ignored by Chrome
is now defined as an error, especially because in this case the fix is so dang
easy (delete a line from the manifest).

[Evan Stade, 2015-12-17 04:28:47]

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

[commit-bot: I haz the power, 2015-12-17 04:28:58]

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

[commit-bot: I haz the power, 2015-12-17 04:52:11]

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

[commit-bot: I haz the power, 2015-12-17 04:52:12]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Devlin, 2015-12-17 17:23:08]

https://codereview.chromium.org/1537473003/diff/20001/extensions/common/manif...
File extensions/common/manifest_handler_helpers.cc (right):

https://codereview.chromium.org/1537473003/diff/20001/extensions/common/manif...
extensions/common/manifest_handler_helpers.cc:60: file_util::ValidateFilePath(
On 2015/12/17 04:27:56, Evan Stade wrote:
> to be honest, I'm not super happy with this because it seems like a violation
of
> the Parse/Validate contract (the former just looks at the manifest and the
> latter checks that the manifest makes sense relative to the rest of the
> extension pack). However, editing the icon set during validation would require
> breaking a lot of const qualifiers (on Extension, ActionIcon, etc.).

I agree... :/

In my mind, this would be a little cleaner (though still far from ideal):
- If the path doesn't exist, add the size and an empty path.
- Skip empty paths in getting the icon.
- Validate() adds an install warning (or error) on any empty paths.

It results in adding a useless entry to the map, but doesn't break anything else
and keeps all the error/install warnings in one place, which I think is more
important.

> I'd be more happy just telling extension developers that what was previously
> undefined behavior but clearly broken and just happened to be ignored by
Chrome
> is now defined as an error, especially because in this case the fix is so dang
> easy (delete a line from the manifest).

My worry is just because extensions can be auto-updated, this risks a bunch of
weirdness with extensions that exist on the store already.  I'd say let's give
it a few milestones and then we can just fail.

https://codereview.chromium.org/1537473003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/1537473003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:334:
IN_PROC_BROWSER_TEST_F(BrowserActionApiTest, MultiIcons) {
Can this be a unittest?

https://codereview.chromium.org/1537473003/diff/60001/extensions/common/manif...
File extensions/common/manifest_handler_helpers.h (right):

https://codereview.chromium.org/1537473003/diff/60001/extensions/common/manif...
extensions/common/manifest_handler_helpers.h:9: #include <vector>
?

[Evan Stade, 2015-12-17 20:15:58]

https://codereview.chromium.org/1537473003/diff/20001/extensions/common/manif...
File extensions/common/manifest_handler_helpers.cc (right):

https://codereview.chromium.org/1537473003/diff/20001/extensions/common/manif...
extensions/common/manifest_handler_helpers.cc:60: file_util::ValidateFilePath(
On 2015/12/17 17:23:08, Devlin wrote:
> On 2015/12/17 04:27:56, Evan Stade wrote:
> > to be honest, I'm not super happy with this because it seems like a
violation
> of
> > the Parse/Validate contract (the former just looks at the manifest and the
> > latter checks that the manifest makes sense relative to the rest of the
> > extension pack). However, editing the icon set during validation would
require
> > breaking a lot of const qualifiers (on Extension, ActionIcon, etc.).
> 
> I agree... :/
> 
> In my mind, this would be a little cleaner (though still far from ideal):
> - If the path doesn't exist, add the size and an empty path.
> - Skip empty paths in getting the icon.
> - Validate() adds an install warning (or error) on any empty paths.
> 
> It results in adding a useless entry to the map, but doesn't break anything
else
> and keeps all the error/install warnings in one place, which I think is more
> important.

I tried this at first, but it's not correct that nothing else breaks. There were
more errors down the line and it seems as though many places assume that if it's
in the map and the extension has passed validation, it's valid. The best way to
fix this that I found was to actually remove it from the map.

https://codereview.chromium.org/1537473003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/1537473003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:334:
IN_PROC_BROWSER_TEST_F(BrowserActionApiTest, MultiIcons) {
On 2015/12/17 17:23:08, Devlin wrote:
> Can this be a unittest?

There already is a unit test that does almost exactly the same thing, but this
one exercises the new code to check the image exists. As far as I could tell
this was the best way to do that.

https://codereview.chromium.org/1537473003/diff/60001/extensions/common/manif...
File extensions/common/manifest_handler_helpers.h (right):

https://codereview.chromium.org/1537473003/diff/60001/extensions/common/manif...
extensions/common/manifest_handler_helpers.h:9: #include <vector>
On 2015/12/17 17:23:08, Devlin wrote:
> ?

oops, both the new includes date back to a version that took
std::vector<InstallWarning>*

[Devlin, 2015-12-17 22:05:58]

https://codereview.chromium.org/1537473003/diff/20001/extensions/common/manif...
File extensions/common/manifest_handler_helpers.cc (right):

https://codereview.chromium.org/1537473003/diff/20001/extensions/common/manif...
extensions/common/manifest_handler_helpers.cc:60: file_util::ValidateFilePath(
On 2015/12/17 20:15:58, Evan Stade wrote:
> On 2015/12/17 17:23:08, Devlin wrote:
> > On 2015/12/17 04:27:56, Evan Stade wrote:
> > > to be honest, I'm not super happy with this because it seems like a
> violation
> > of
> > > the Parse/Validate contract (the former just looks at the manifest and the
> > > latter checks that the manifest makes sense relative to the rest of the
> > > extension pack). However, editing the icon set during validation would
> require
> > > breaking a lot of const qualifiers (on Extension, ActionIcon, etc.).
> > 
> > I agree... :/
> > 
> > In my mind, this would be a little cleaner (though still far from ideal):
> > - If the path doesn't exist, add the size and an empty path.
> > - Skip empty paths in getting the icon.
> > - Validate() adds an install warning (or error) on any empty paths.
> > 
> > It results in adding a useless entry to the map, but doesn't break anything
> else
> > and keeps all the error/install warnings in one place, which I think is more
> > important.
> 
> I tried this at first, but it's not correct that nothing else breaks. There
were
> more errors down the line and it seems as though many places assume that if
it's
> in the map and the extension has passed validation, it's valid. The best way
to
> fix this that I found was to actually remove it from the map.

Well, shoot.  Okay.

https://codereview.chromium.org/1537473003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/1537473003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:334:
IN_PROC_BROWSER_TEST_F(BrowserActionApiTest, MultiIcons) {
On 2015/12/17 20:15:58, Evan Stade wrote:
> On 2015/12/17 17:23:08, Devlin wrote:
> > Can this be a unittest?
> 
> There already is a unit test that does almost exactly the same thing, but this
> one exercises the new code to check the image exists. As far as I could tell
> this was the best way to do that.

This should be doable with a "unittest" (here basically just meaning something
that doesn't actually open a full-blown chrome browser, rather than something
which tests only a unit of code).  See, e.g., ExtensionServiceTestWithInstall.

https://codereview.chromium.org/1537473003/diff/60001/chrome/common/extension...
File
chrome/common/extensions/api/extension_action/browser_action_manifest_unittest.cc
(right):

https://codereview.chromium.org/1537473003/diff/60001/chrome/common/extension...
chrome/common/extensions/api/extension_action/browser_action_manifest_unittest.cc:84:
// Don't check for existence of icons.
This is very unintuitive, since other code states that we check more strictly on
unpacked locations. (Is this right?)

https://codereview.chromium.org/1537473003/diff/60001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/browser_action/multi_icons/manifest.json
(right):

https://codereview.chromium.org/1537473003/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/browser_action/multi_icons/manifest.json:8:
"permissions": [
let's remove some of this extraneous stuff.

[Evan Stade, 2015-12-17 22:56:06]

https://codereview.chromium.org/1537473003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/1537473003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:334:
IN_PROC_BROWSER_TEST_F(BrowserActionApiTest, MultiIcons) {
On 2015/12/17 22:05:58, Devlin wrote:
> On 2015/12/17 20:15:58, Evan Stade wrote:
> > On 2015/12/17 17:23:08, Devlin wrote:
> > > Can this be a unittest?
> > 
> > There already is a unit test that does almost exactly the same thing, but
this
> > one exercises the new code to check the image exists. As far as I could tell
> > this was the best way to do that.
> 
> This should be doable with a "unittest" (here basically just meaning something
> that doesn't actually open a full-blown chrome browser, rather than something
> which tests only a unit of code).  See, e.g., ExtensionServiceTestWithInstall.

thanks for the pointer. Done.

https://codereview.chromium.org/1537473003/diff/60001/chrome/common/extension...
File
chrome/common/extensions/api/extension_action/browser_action_manifest_unittest.cc
(right):

https://codereview.chromium.org/1537473003/diff/60001/chrome/common/extension...
chrome/common/extensions/api/extension_action/browser_action_manifest_unittest.cc:84:
// Don't check for existence of icons.
On 2015/12/17 22:05:58, Devlin wrote:
> This is very unintuitive, since other code states that we check more strictly
on
> unpacked locations. (Is this right?)

During the validation phase, not during the parsing phase (this is testing
parsing). I can put "during parsing" into the comment.

https://codereview.chromium.org/1537473003/diff/60001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/browser_action/multi_icons/manifest.json
(right):

https://codereview.chromium.org/1537473003/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/browser_action/multi_icons/manifest.json:8:
"permissions": [
On 2015/12/17 22:05:58, Devlin wrote:
> let's remove some of this extraneous stuff.

Done.

[Evan Stade, 2015-12-17 22:56:17]

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

[commit-bot: I haz the power, 2015-12-17 22:58:21]

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

[commit-bot: I haz the power, 2015-12-18 01:19:59]

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

[commit-bot: I haz the power, 2015-12-18 01:19:59]

Dry run: This issue passed the CQ dry run.

[Devlin, 2015-12-18 22:29:02]

lgtm with easy fixes

https://codereview.chromium.org/1537473003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/api/extension_action/browser_action_unittest.cc
(right):

https://codereview.chromium.org/1537473003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_unittest.cc:27:
ASSERT_FALSE(browser_action_info->default_icon.empty());
nit: EXPECT here

https://codereview.chromium.org/1537473003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service_test_with_install.cc (right):

https://codereview.chromium.org/1537473003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_test_with_install.cc:133:
base::FilePath crx_path;
I'd say that we should instead be currying the location to another method that
takes all the arguments rather than duplicating the logic here, but apparently
we can't because InstallCRXWithLocation doesn't do that.  Grr.  I'll fix it
later, but can you add a TODO(devlin) here?

https://codereview.chromium.org/1537473003/diff/100001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/browser_action/multi_icons/background.html
(right):

https://codereview.chromium.org/1537473003/diff/100001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/browser_action/multi_icons/background.html:2:
* Copyright (c) 2011 The Chromium Authors. All rights reserved.  Use of this
This file isn't needed, right?

https://codereview.chromium.org/1537473003/diff/100001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/browser_action/multi_icons/background.js
(right):

https://codereview.chromium.org/1537473003/diff/100001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/browser_action/multi_icons/background.js:1:
// Copyright (c) 2011 The Chromium Authors. All rights reserved.
Ditto

https://codereview.chromium.org/1537473003/diff/100001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/browser_action/multi_icons/manifest.json
(right):

https://codereview.chromium.org/1537473003/diff/100001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/browser_action/multi_icons/manifest.json:7:
"19": "icon19.png",
For funzies, can we throw in a totally oddball icon size here to prove that
anything is accepted?

https://codereview.chromium.org/1537473003/diff/100001/extensions/common/mani...
File extensions/common/manifest_handler_helpers.cc (right):

https://codereview.chromium.org/1537473003/diff/100001/extensions/common/mani...
extensions/common/manifest_handler_helpers.cc:57: // during validation if the
file isn't present. See crbug.com/570249
TODO(estade|devlin): Make this an install error after a reasonable amount of
time.

[Evan Stade, 2015-12-19 00:02:41]

https://codereview.chromium.org/1537473003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/api/extension_action/browser_action_unittest.cc
(right):

https://codereview.chromium.org/1537473003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_unittest.cc:27:
ASSERT_FALSE(browser_action_info->default_icon.empty());
On 2015/12/18 22:29:01, Devlin wrote:
> nit: EXPECT here

i guess this is redundant with L32, so removed

https://codereview.chromium.org/1537473003/diff/100001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service_test_with_install.cc (right):

https://codereview.chromium.org/1537473003/diff/100001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_test_with_install.cc:133:
base::FilePath crx_path;
On 2015/12/18 22:29:02, Devlin wrote:
> I'd say that we should instead be currying the location to another method that
> takes all the arguments rather than duplicating the logic here, but apparently
> we can't because InstallCRXWithLocation doesn't do that.  Grr.  I'll fix it
> later, but can you add a TODO(devlin) here?

Done.

https://codereview.chromium.org/1537473003/diff/100001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/browser_action/multi_icons/background.html
(right):

https://codereview.chromium.org/1537473003/diff/100001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/browser_action/multi_icons/background.html:2:
* Copyright (c) 2011 The Chromium Authors. All rights reserved.  Use of this
On 2015/12/18 22:29:02, Devlin wrote:
> This file isn't needed, right?

Done.

https://codereview.chromium.org/1537473003/diff/100001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/browser_action/multi_icons/background.js
(right):

https://codereview.chromium.org/1537473003/diff/100001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/browser_action/multi_icons/background.js:1:
// Copyright (c) 2011 The Chromium Authors. All rights reserved.
On 2015/12/18 22:29:02, Devlin wrote:
> Ditto

Done.

https://codereview.chromium.org/1537473003/diff/100001/chrome/test/data/exten...
File
chrome/test/data/extensions/api_test/browser_action/multi_icons/manifest.json
(right):

https://codereview.chromium.org/1537473003/diff/100001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/browser_action/multi_icons/manifest.json:7:
"19": "icon19.png",
On 2015/12/18 22:29:02, Devlin wrote:
> For funzies, can we throw in a totally oddball icon size here to prove that
> anything is accepted?

that's what the 24 is supposed to be. Or did you mean something even crazier
like 31? Done.

https://codereview.chromium.org/1537473003/diff/100001/extensions/common/mani...
File extensions/common/manifest_handler_helpers.cc (right):

https://codereview.chromium.org/1537473003/diff/100001/extensions/common/mani...
extensions/common/manifest_handler_helpers.cc:57: // during validation if the
file isn't present. See crbug.com/570249
On 2015/12/18 22:29:02, Devlin wrote:
> TODO(estade|devlin): Make this an install error after a reasonable amount of
> time.

Done.

[Evan Stade, 2015-12-19 00:02:54]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-12-19 00:02:54]

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/1537473003/#ps120001
(title: "final review changes")

[commit-bot: I haz the power, 2015-12-19 00:04:49]

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

[commit-bot: I haz the power, 2015-12-19 00:10:41]

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

[commit-bot: I haz the power, 2015-12-19 00:10:42]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Evan Stade, 2015-12-19 01:15:38]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-12-19 01:15:39]

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/1537473003/#ps140001
(title: "ultimate weirdness")

[commit-bot: I haz the power, 2015-12-19 01:16:09]

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

[Devlin, 2015-12-19 01:43:19]

https://codereview.chromium.org/1537473003/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/extension_action/browser_action_unittest.cc
(right):

https://codereview.chromium.org/1537473003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_unittest.cc:34:
EXPECT_EQ("icon24.png", icons.Get(31, ExtensionIconSet::MATCH_EXACTLY));
This looks a little weird, even though it's correct in the manifest file.  Mind
just copying icon24.png to icon31.png so that it doesn't look like we're just
matching nearest?

(If you like, fine to do in a followup)

[Evan Stade, 2015-12-19 01:49:58]

https://codereview.chromium.org/1537473003/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/extension_action/browser_action_unittest.cc
(right):

https://codereview.chromium.org/1537473003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_unittest.cc:34:
EXPECT_EQ("icon24.png", icons.Get(31, ExtensionIconSet::MATCH_EXACTLY));
On 2015/12/19 01:43:19, Devlin wrote:
> This looks a little weird, even though it's correct in the manifest file. 
Mind
> just copying icon24.png to icon31.png so that it doesn't look like we're just
> matching nearest?
> 
> (If you like, fine to do in a followup)

I could do that but I thought we were going for ultimate oddballer status. We
don't want to imply we care about the name of the icon or that we couldn't have
one icon be pointed at two times... maybe?

[Devlin, 2015-12-19 02:06:11]

https://codereview.chromium.org/1537473003/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/api/extension_action/browser_action_unittest.cc
(right):

https://codereview.chromium.org/1537473003/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/api/extension_action/browser_action_unittest.cc:34:
EXPECT_EQ("icon24.png", icons.Get(31, ExtensionIconSet::MATCH_EXACTLY));
On 2015/12/19 01:49:58, Evan Stade wrote:
> On 2015/12/19 01:43:19, Devlin wrote:
> > This looks a little weird, even though it's correct in the manifest file. 
> Mind
> > just copying icon24.png to icon31.png so that it doesn't look like we're
just
> > matching nearest?
> > 
> > (If you like, fine to do in a followup)
> 
> I could do that but I thought we were going for ultimate oddballer status. We
> don't want to imply we care about the name of the icon or that we couldn't
have
> one icon be pointed at two times... maybe?

Unittests are cheap.  EXPECT_EQs in a unittest are practically free.  Let's
check the following oddballs. ;)
- Size that isn't one of the main sizes with a normal key (e.g., "31":
"icon31.png")
- Size that uses the same icon twice - let's even make it deliberately not the
closest icon (e.g., "38": "icon24.png")
- Size that uses a random name (e.g., "40": "an_unrelated_name.png")

+Any others you can think of.

[commit-bot: I haz the power, 2015-12-19 02:39:44]

Description was changed from

==========
Change extension icon load errors to warnings

During the Extension parsing step, check if the icon file exists and if not,
remove that entry from the dictionary.

Keep the same check during the validation phase and don't apply the workaround
to unpacked extensions. This will more strongly discourage new extensions from
making this mistake.

BUG=570249
==========

to

==========
Change extension icon load errors to warnings

During the Extension parsing step, check if the icon file exists and if not,
remove that entry from the dictionary.

Keep the same check during the validation phase and don't apply the workaround
to unpacked extensions. This will more strongly discourage new extensions from
making this mistake.

BUG=570249
==========

[commit-bot: I haz the power, 2015-12-19 02:39:45]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-12-19 02:40:43]

Description was changed from

==========
Change extension icon load errors to warnings

During the Extension parsing step, check if the icon file exists and if not,
remove that entry from the dictionary.

Keep the same check during the validation phase and don't apply the workaround
to unpacked extensions. This will more strongly discourage new extensions from
making this mistake.

BUG=570249
==========

to

==========
Change extension icon load errors to warnings

During the Extension parsing step, check if the icon file exists and if not,
remove that entry from the dictionary.

Keep the same check during the validation phase and don't apply the workaround
to unpacked extensions. This will more strongly discourage new extensions from
making this mistake.

BUG=570249

Committed: https://crrev.com/6e8e7d1c49657e82d0e8f2518ad463794346321b
Cr-Commit-Position: refs/heads/master@{#366253}
==========

[commit-bot: I haz the power, 2015-12-19 02:40:44]

Patchset 8 (id:??) landed as
https://crrev.com/6e8e7d1c49657e82d0e8f2518ad463794346321b
Cr-Commit-Position: refs/heads/master@{#366253}

[Devlin, 2015-12-22 23:12:38]

On 2015/12/19 02:40:44, commit-bot: I haz the power wrote:
> Patchset 8 (id:??) landed as
> https://crrev.com/6e8e7d1c49657e82d0e8f2518ad463794346321b
> Cr-Commit-Position: refs/heads/master@{#366253}

This caused crbug.com/571701.  I thought I could just bandaid it with
ScopedAllowIO, but apparently we outright ban that now. :(

I think it's best to just revert this in the meantime - the problem won't get
much worse and is confined to canary/dev.  Maybe we'll even see that no one
really cares and there are few enough extensions we don't have to do this.

[Devlin, 2015-12-22 23:13:34]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1541113003/ by rdevlin.cronin@chromium.org.

The reason for reverting is: caused crbug.com/571701.

[Evan Stade, 2015-12-29 17:32:04]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/1554583002/ by estade@chromium.org.

The reason for reverting is: file read from wrong thread.