Altered the logic that determines when NaCl is enabled. Previously, we requi...

Altered the logic that determines when NaCl is enabled.  Previously, we required an explicit command-line flag to use NaCl.  This enables NaCl without a flag for apps and extensions that have the special "native_client" permission.

Note that this causes the NaCl plugin to be registered unconditionally.  We need this, since at the time plugin registration is happening it's not clear whether we're in an extension/app.

BUG=45881
TEST=none yet

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

[gregoryd, 2010-07-30 19:09:03]

http://codereview.chromium.org/3057025/diff/1/3
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/3057025/diff/1/3#newcode3262
chrome/renderer/render_view.cc:3262: Extension::kNativeClientPermission)) {
I think the indentation here should be 4 characters

http://codereview.chromium.org/3057025/diff/1/3#newcode3265
chrome/renderer/render_view.cc:3265: }
what about 'else' here? If the extension doesn't have a NaCl permission but
attempts to use it it should probably fail, so we should return NULL.

[Cliff L. Biffle, 2010-07-30 20:10:07]

On Fri, Jul 30, 2010 at 12:09 PM,  <gregoryd@google.com> wrote:
> http://codereview.chromium.org/3057025/diff/1/3#newcode3262
> chrome/renderer/render_view.cc:3262:
> Extension::kNativeClientPermission)) {
> I think the indentation here should be 4 characters

Done-ish?  I've indented it 4 past the start of the call that takes it
as an argument.

> http://codereview.chromium.org/3057025/diff/1/3#newcode3265
> chrome/renderer/render_view.cc:3265: }
> what about 'else' here? If the extension doesn't have a NaCl permission
> but attempts to use it it should probably fail, so we should return
> NULL.

Good catch.  Fixed.

Do you think this block should be controlled by DISABLE_NACL?

-- 
Cliff L. Biffle
NaCl Team, Google, Inc.

[Erik does not do reviews, 2010-07-30 22:19:47]

This should really have a browser_test to go with it to verify that the
permission is being enforced properly.

Take a look at permissions_apitest.cc (and the associated
test/data/extensions/api_test/permissions directory).

http://codereview.chromium.org/3057025/diff/8001/9002
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/3057025/diff/8001/9002#newcode3253
chrome/renderer/render_view.cc:3253: GURL
mainFrameUrl(webview()->mainFrame()->url());
nit: main_frame_url

http://codereview.chromium.org/3057025/diff/8001/9002#newcode3254
chrome/renderer/render_view.cc:3254: const std::string &extensionId =
nit: extension_id

[Cliff L. Biffle, 2010-07-30 22:44:51]

On Fri, Jul 30, 2010 at 3:19 PM,  <erikkay@chromium.org> wrote:
> This should really have a browser_test to go with it to verify that the
> permission is being enforced properly.
>
> Take a look at permissions_apitest.cc (and the associated
> test/data/extensions/api_test/permissions directory).

I may have to hand the test off to gregoryd, I'm about to head out of town.

Fixed both style issues you noted.

-- 
Cliff L. Biffle
NaCl Team, Google, Inc.

[Erik does not do reviews, 2010-07-30 22:46:54]

On 2010/07/30 22:44:51, Cliff L. Biffle wrote:
> On Fri, Jul 30, 2010 at 3:19 PM,  <mailto:erikkay@chromium.org> wrote:
> > This should really have a browser_test to go with it to verify that the
> > permission is being enforced properly.
> >
> > Take a look at permissions_apitest.cc (and the associated
> > test/data/extensions/api_test/permissions directory).
> 
> I may have to hand the test off to gregoryd, I'm about to head out of town.

Fair enough.  I'm OK with a bug and a TODO if gregoryd can tackle it soon.

Erik

[Cliff L. Biffle, 2010-07-30 22:55:18]

On Fri, Jul 30, 2010 at 3:46 PM,  <erikkay@chromium.org> wrote:
> Fair enough.  I'm OK with a bug and a TODO if gregoryd can tackle it soon.

TODO added -- reusing the existing bug.

-- 
Cliff L. Biffle
NaCl Team, Google, Inc.

[Erik does not do reviews, 2010-07-30 23:02:13]

LGTM