Allow extensions which can run without NPAPI to run under Windows 8 Metro.

Allow extensions which can run without NPAPI to run under Windows 8 Metro.

Extensions which include NPAPI plugins are assumed to require them, causing
them to be blocked on Windows 8 Metro mode, where NPAPI is not supported.

This CL allows extensions which include NPAPI plugins, but which can still run
without them, to load in Windows 8 Metro mode.

It also fixes the handling of extensions with no explicit NPAPI requirement.

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

[Wez, 2012-09-19 06:34:13]

PTAL

http://codereview.chromium.org/10949017/diff/1/chrome/browser/extensions/requ...
File chrome/browser/extensions/requirements_checker.cc (right):

http://codereview.chromium.org/10949017/diff/1/chrome/browser/extensions/requ...
chrome/browser/extensions/requirements_checker.cc:44: if
(base::win::IsMetroProcess()) {
If we assume that it doesn't make sense for an extension with no NPAPI plugins
to specify that it requires NPAPI then we can simplify this clause to avoid
duplicating platform-specific blocks.

[Aaron Boodman, 2012-09-19 21:34:52]

Done. It would be good to have some unit tests though. Is it possible to add
some? (can be different CL)

http://codereview.chromium.org/10949017/diff/1/chrome/browser/extensions/requ...
File chrome/browser/extensions/requirements_checker.cc (right):

http://codereview.chromium.org/10949017/diff/1/chrome/browser/extensions/requ...
chrome/browser/extensions/requirements_checker.cc:44: if
(base::win::IsMetroProcess()) {
On 2012/09/19 06:34:14, Wez wrote:
> If we assume that it doesn't make sense for an extension with no NPAPI plugins
> to specify that it requires NPAPI then we can simplify this clause to avoid
> duplicating platform-specific blocks.

I don't think that is a good assumption.

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

http://codereview.chromium.org/10949017/diff/1/chrome/common/extensions/exten...
chrome/common/extensions/extension.cc:1503: requirements_.npapi = true;
Why did you move this from LoadRequirements()? I think it's a little surprising
to modify requirements_ here.

[Wez, 2012-09-21 00:13:56]

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

http://codereview.chromium.org/10949017/diff/1/chrome/common/extensions/exten...
chrome/common/extensions/extension.cc:1503: requirements_.npapi = true;
On 2012/09/19 21:34:52, Aaron Boodman wrote:
> Why did you move this from LoadRequirements()? I think it's a little
surprising
> to modify requirements_ here.

The existing code was setting this value based on the size of |plugins_|, which
was always zero on platforms that don't support NPAPI, where LoadPlugins() won't
populate |plugins_|.

We're setting an implicit default requirement here, rather than reading from
explicitly-specified requirements, so I don't think it's such a surprise that it
be set here.

[Aaron Boodman, 2012-09-21 02:33:08]

I'd prefer the code moved, but don't want to hold up submission.

LGTM

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

http://codereview.chromium.org/10949017/diff/1/chrome/common/extensions/exten...
chrome/common/extensions/extension.cc:1503: requirements_.npapi = true;
On 2012/09/21 00:13:56, Wez wrote:
> On 2012/09/19 21:34:52, Aaron Boodman wrote:
> > Why did you move this from LoadRequirements()? I think it's a little
> surprising
> > to modify requirements_ here.
> 
> The existing code was setting this value based on the size of |plugins_|,
which
> was always zero on platforms that don't support NPAPI, where LoadPlugins()
won't
> populate |plugins_|.
> 
> We're setting an implicit default requirement here, rather than reading from
> explicitly-specified requirements, so I don't think it's such a surprise that
it
> be set here.

I see. I mean, you could just check manifest->HasKey("plugins") in
LoadRequirements() too. Since that function is the one that "loads requirements"
it seems more obvious to put code that sets the requirements_ member there.

Also, nit, but I think checking HasKey("plugins") is slightly weaker than what
the code used to do. It's possible (however unwise) that there are extensions
out there with "plugins": [ ]. Right?

[Wez, 2012-09-22 07:21:00]

FYI. I've also added a couple of new test cases.

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

http://codereview.chromium.org/10949017/diff/1/chrome/common/extensions/exten...
chrome/common/extensions/extension.cc:1503: requirements_.npapi = true;
On 2012/09/21 02:33:08, Aaron Boodman wrote:
> On 2012/09/21 00:13:56, Wez wrote:
> > On 2012/09/19 21:34:52, Aaron Boodman wrote:
> > > Why did you move this from LoadRequirements()? I think it's a little
> > surprising
> > > to modify requirements_ here.
> > 
> > The existing code was setting this value based on the size of |plugins_|,
> which
> > was always zero on platforms that don't support NPAPI, where LoadPlugins()
> won't
> > populate |plugins_|.
> > 
> > We're setting an implicit default requirement here, rather than reading from
> > explicitly-specified requirements, so I don't think it's such a surprise
that
> it
> > be set here.
> 
> I see. I mean, you could just check manifest->HasKey("plugins") in
> LoadRequirements() too. Since that function is the one that "loads
requirements"
> it seems more obvious to put code that sets the requirements_ member there.

OK, you've convinced me.  Moved.

> Also, nit, but I think checking HasKey("plugins") is slightly weaker than what
> the code used to do. It's possible (however unwise) that there are extensions
> out there with "plugins": [ ]. Right?

Possibly; the new code in LoadRequirements now copes with that case.

[commit-bot: I haz the power, 2012-09-22 07:21:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/10949017/7001

[commit-bot: I haz the power, 2012-09-22 09:58:26]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-09-22 17:21:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/10949017/7001

[commit-bot: I haz the power, 2012-09-22 19:45:48]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-09-23 07:18:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/10949017/15002

[commit-bot: I haz the power, 2012-09-23 09:29:01]

Retried try job too often for step(s) browser_tests

[commit-bot: I haz the power, 2012-09-23 19:12:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/wez@chromium.org/10949017/19001

[Wez, 2012-09-23 19:12:23]

jstritar: The CrossOriginXHR.AllURLs test doesn't test anything on ChromeOS,
because NPAPI plugins are blocked; would adding a "devconsole URL" to the
extension instead of plugins be equivalent for this test?

[commit-bot: I haz the power, 2012-09-23 21:12:04]

Change committed as 158217