Add a component installer for Portable NaCl. Registration of installer is

Add a component installer for Portable NaCl. Registration of installer is hidden behind a commandline flag '--enable-pnacl'.

Still some of TODOs: finalize the metadata associated with the CRX, figure out what to do when a NaCl plugin needs Pnacl, but it's not there yet.


BUG= http://code.google.com/p/nativeclient/issues/detail?id=2365
TEST= none yet, only manually test:

(1) put a pnacl omaha config in the omaha sandbox, pointing at a crx.

(2) run out/Debug/chrome --user-data-dir=/tmp/temp_profile/ --enable-pnacl --component-updater-debug=fast-update --apps-gallery-update-url=http://omaha.sandbox.google.com/service/update2/crx

check /tmp/temp_profile/PNaCl/0.1.0.X is installed after a few seconds.

restart chrome, check that PathService registers /tmp/temp_profile/PNaCl/0.1.0.X soonish.


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

[jvoung - send to chromium..., 2011-11-11 22:57:34]

http://codereview.chromium.org/8348026/diff/2001/chrome/nacl/pnacl_component_...
File chrome/nacl/pnacl_component_installer.cc (right):

http://codereview.chromium.org/8348026/diff/2001/chrome/nacl/pnacl_component_...
chrome/nacl/pnacl_component_installer.cc:69: // earlier if there are components
that are not yet installed (version 0.0.0.0),
Would such a change make sense?

http://codereview.chromium.org/8348026/diff/2001/chrome/nacl/pnacl_component_...
chrome/nacl/pnacl_component_installer.cc:225: // if we pre-populate the user
profile directory with the components?
Is this a race we need to worry about for testing NaCl + PNacl on the bots, or
does this registration happen early enough (assuming we pre-populate the
directory, or have a flag to point at some pre-populated directory)?

http://codereview.chromium.org/8348026/diff/8001/chrome/browser/component_upd...
File chrome/browser/component_updater/component_updater_configurator.cc (right):

http://codereview.chromium.org/8348026/diff/8001/chrome/browser/component_upd...
chrome/browser/component_updater/component_updater_configurator.cc:127: // Allow
switch to override update URL (piggyback on AppsGalleryUpdateURL).
Is it okay to override the update URL using this commandline switch?

[cpu_(ooo_6.6-7.5), 2011-11-13 00:24:39]

sorry for the delay, I am OOO. I'll take a look this weekend but don't expect a
full review until tuesday.

[cpu_(ooo_6.6-7.5), 2011-11-23 01:39:12]

I promise to look at your change in the next 24 hours!

http://codereview.chromium.org/8348026/diff/2001/chrome/nacl/pnacl_component_...
File chrome/nacl/pnacl_component_installer.cc (right):

http://codereview.chromium.org/8348026/diff/2001/chrome/nacl/pnacl_component_...
chrome/nacl/pnacl_component_installer.cc:69: // earlier if there are components
that are not yet installed (version 0.0.0.0),
On 2011/11/11 22:57:34, jvoung wrote:
> Would such a change make sense?

Sounds like an interesting possibility.

http://codereview.chromium.org/8348026/diff/2001/chrome/nacl/pnacl_component_...
chrome/nacl/pnacl_component_installer.cc:225: // if we pre-populate the user
profile directory with the components?
On 2011/11/11 22:57:34, jvoung wrote:
> Is this a race we need to worry about for testing NaCl + PNacl on the bots, or
> does this registration happen early enough (assuming we pre-populate the

Not sure, who is pre-populating? other thread? other process?

Maybe it is best to disable pnacl registration for the browser tests.
> directory, or have a flag to point at some pre-populated directory)?

[cpu_(ooo_6.6-7.5), 2011-11-23 01:51:33]

actually the code looks very good. this is the rest of the review.

http://codereview.chromium.org/8348026/diff/9001/chrome/browser/component_upd...
File chrome/browser/component_updater/component_updater_configurator.cc (right):

http://codereview.chromium.org/8348026/diff/9001/chrome/browser/component_upd...
chrome/browser/component_updater/component_updater_configurator.cc:133: }
do we need this code anymore?

We have ways to test without having this deal. See the design document.

http://codereview.chromium.org/8348026/diff/9001/chrome/browser/component_upd...
chrome/browser/component_updater/component_updater_configurator.cc:162: return
GURL(app_update_url_);
I rather have this method just return the member string and have the logic in
the constructor.

http://codereview.chromium.org/8348026/diff/9001/chrome/common/chrome_switche...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/8348026/diff/9001/chrome/common/chrome_switche...
chrome/common/chrome_switches.cc:521: const char kEnablePnacl[]                 
  = "enable-pnacl";
I am not opposed to the switches idea, but you have other choices, for example
you can deploy pnacl only to canary.

Or if you are surfacing pnacl in the about:plugins page then there are even
better options. Let me know if you do.

http://codereview.chromium.org/8348026/diff/9001/chrome/nacl/pnacl_component_...
File chrome/nacl/pnacl_component_installer.cc (right):

http://codereview.chromium.org/8348026/diff/9001/chrome/nacl/pnacl_component_...
chrome/nacl/pnacl_component_installer.cc:27: const uint8 sha256_hash[] = {
is this the final hash? if so it is important to put this file under a
controlled OWNERS file. So no random person can change this hash.

http://codereview.chromium.org/8348026/diff/9001/chrome/nacl/pnacl_component_...
chrome/nacl/pnacl_component_installer.cc:179: // then be able to reload the page
and retry (or something).
this caveat is worthy of a bug number.

[jvoung - send to chromium..., 2011-12-14 19:34:31]

Thanks for the review, and sorry for the delay!

http://codereview.chromium.org/8348026/diff/2001/chrome/nacl/pnacl_component_...
File chrome/nacl/pnacl_component_installer.cc (right):

http://codereview.chromium.org/8348026/diff/2001/chrome/nacl/pnacl_component_...
chrome/nacl/pnacl_component_installer.cc:225: // if we pre-populate the user
profile directory with the components?
On 2011/11/23 01:39:12, cpu wrote:
> On 2011/11/11 22:57:34, jvoung wrote:
> > Is this a race we need to worry about for testing NaCl + PNacl on the bots,
or
> > does this registration happen early enough (assuming we pre-populate the
> 
> Not sure, who is pre-populating? other thread? other process?
> 
> Maybe it is best to disable pnacl registration for the browser tests.

At the very least we don't want to be pinging servers for updates during the
test.

As for pre-populating and registering, if we really want to match a "real
install" during a test, we could have the test framework do the pre-populating
in a way that matches the "install" and then have registration at least pick up
this pre-populated "install" and publish the path. The concern was that this
published path might not show up in time.

How are other components that are installed, tested?

Perhaps we don't want to match the install path completely in our tests, and we
could just load the files from somewhere with --load-extension and tell the NaCl
plugin to look for an extension (with some other fixed AppID) instead of a
component with the published path?

http://codereview.chromium.org/8348026/diff/9001/chrome/browser/component_upd...
File chrome/browser/component_updater/component_updater_configurator.cc (right):

http://codereview.chromium.org/8348026/diff/9001/chrome/browser/component_upd...
chrome/browser/component_updater/component_updater_configurator.cc:133: }
On 2011/11/23 01:51:33, cpu wrote:
> do we need this code anymore?
> 
> We have ways to test without having this deal. See the design document.

By design doc, are you referring to "Chrome Component Updater Design"?

I didn't see Omaha staging/sandbox server mentioned in that doc. This might not
be very useful, but it seemed easier to do some manual testing on the staging
server at first.

http://codereview.chromium.org/8348026/diff/9001/chrome/browser/component_upd...
chrome/browser/component_updater/component_updater_configurator.cc:162: return
GURL(app_update_url_);
On 2011/11/23 01:51:33, cpu wrote:
> I rather have this method just return the member string and have the logic in
> the constructor.

Done.

http://codereview.chromium.org/8348026/diff/9001/chrome/common/chrome_switche...
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/8348026/diff/9001/chrome/common/chrome_switche...
chrome/common/chrome_switches.cc:521: const char kEnablePnacl[]                 
  = "enable-pnacl";
On 2011/11/23 01:51:33, cpu wrote:
> I am not opposed to the switches idea, but you have other choices, for example
> you can deploy pnacl only to canary.
> 

Yes a command line doesn't seem very friendly, but I'm not sure we are ready to
enable this for all of canary yet. I'm thinking more of an internal dogfood for
now. 

> Or if you are surfacing pnacl in the about:plugins page then there are even
> better options. Let me know if you do.

I'm not really familiar with what should and should not go in about:plugins, but
PNaCl doesn't provide an additional mime handler. It's the NaCl plugin that
handles "application/x-nacl", and then may invoke PNaCl. Though it would be nice
to show the version of PNaCl somewhere for debugging.

http://codereview.chromium.org/8348026/diff/9001/chrome/nacl/pnacl_component_...
File chrome/nacl/pnacl_component_installer.cc (right):

http://codereview.chromium.org/8348026/diff/9001/chrome/nacl/pnacl_component_...
chrome/nacl/pnacl_component_installer.cc:27: const uint8 sha256_hash[] = {
On 2011/11/23 01:51:33, cpu wrote:
> is this the final hash? if so it is important to put this file under a
> controlled OWNERS file. So no random person can change this hash.

Good point I'll move this into a pnacl subdir and add an OWNERS file with some
of our team members.
It might not be the final hash, but we still don't want it to be changing
arbitrarily.

http://codereview.chromium.org/8348026/diff/9001/chrome/nacl/pnacl_component_...
chrome/nacl/pnacl_component_installer.cc:179: // then be able to reload the page
and retry (or something).
On 2011/11/23 01:51:33, cpu wrote:
> this caveat is worthy of a bug number.

Done, filed a bug.

[cpu_(ooo_6.6-7.5), 2011-12-14 23:24:07]

On 2011/12/14 19:34:31, jvoung wrote:
> Thanks for the review, and sorry for the delay!
> 
>
http://codereview.chromium.org/8348026/diff/2001/chrome/nacl/pnacl_component_...
> File chrome/nacl/pnacl_component_installer.cc (right):
> 
>
http://codereview.chromium.org/8348026/diff/2001/chrome/nacl/pnacl_component_...
> chrome/nacl/pnacl_component_installer.cc:225: // if we pre-populate the user
> profile directory with the components?
> On 2011/11/23 01:39:12, cpu wrote:
> > On 2011/11/11 22:57:34, jvoung wrote:
> > > Is this a race we need to worry about for testing NaCl + PNacl on the
bots,
> or
> > > does this registration happen early enough (assuming we pre-populate the
> > 
> > Not sure, who is pre-populating? other thread? other process?
> > 
> > Maybe it is best to disable pnacl registration for the browser tests.
> 
> At the very least we don't want to be pinging servers for updates during the
> test.
> 
> As for pre-populating and registering, if we really want to match a "real
> install" during a test, we could have the test framework do the pre-populating
> in a way that matches the "install" and then have registration at least pick
up
> this pre-populated "install" and publish the path. The concern was that this
> published path might not show up in time.
> 
> How are other components that are installed, tested?

I am not a fan of ui tests or browser tests. They are by nature flaky and so
there is a high chance they get disabled by a random sheriff.

The CU code is unit tested and the components should be 'live tested'. In other
words, staged and installed as they normally would.

One simple way is to use
--component-updater-debug=test-request,fast-update


> 
> Perhaps we don't want to match the install path completely in our tests, and
we
> could just load the files from somewhere with --load-extension and tell the
NaCl
> plugin to look for an extension (with some other fixed AppID) instead of a
> component with the published path?
> 
>
http://codereview.chromium.org/8348026/diff/9001/chrome/browser/component_upd...
> File chrome/browser/component_updater/component_updater_configurator.cc
(right):
> 
>
http://codereview.chromium.org/8348026/diff/9001/chrome/browser/component_upd...
> chrome/browser/component_updater/component_updater_configurator.cc:133: }
> On 2011/11/23 01:51:33, cpu wrote:
> > do we need this code anymore?
> > 
> > We have ways to test without having this deal. See the design document.
> 
> By design doc, are you referring to "Chrome Component Updater Design"?
> 
> I didn't see Omaha staging/sandbox server mentioned in that doc. This might
not
> be very useful, but it seemed easier to do some manual testing on the staging
> server at first.

If you care to add that to the document that will be nice. I never used it.

> 
>
http://codereview.chromium.org/8348026/diff/9001/chrome/browser/component_upd...
> chrome/browser/component_updater/component_updater_configurator.cc:162: return
> GURL(app_update_url_);
> On 2011/11/23 01:51:33, cpu wrote:
> > I rather have this method just return the member string and have the logic
in
> > the constructor.
> 
> Done.
> 
>
http://codereview.chromium.org/8348026/diff/9001/chrome/common/chrome_switche...
> File chrome/common/chrome_switches.cc (right):
> 
>
http://codereview.chromium.org/8348026/diff/9001/chrome/common/chrome_switche...
> chrome/common/chrome_switches.cc:521: const char kEnablePnacl[]               
 
>   = "enable-pnacl";
> On 2011/11/23 01:51:33, cpu wrote:
> > I am not opposed to the switches idea, but you have other choices, for
example
> > you can deploy pnacl only to canary.
> > 
> 
> Yes a command line doesn't seem very friendly, but I'm not sure we are ready
to
> enable this for all of canary yet. I'm thinking more of an internal dogfood
for
> now. 

Breaking canary is fine. To quote mark larson: "if we are not breaking canary,
we are not moving fast enough"


> 
> > Or if you are surfacing pnacl in the about:plugins page then there are even
> > better options. Let me know if you do.
> 
> I'm not really familiar with what should and should not go in about:plugins,
but
> PNaCl doesn't provide an additional mime handler. It's the NaCl plugin that
> handles "application/x-nacl", and then may invoke PNaCl. Though it would be
nice
> to show the version of PNaCl somewhere for debugging.
> 
>
http://codereview.chromium.org/8348026/diff/9001/chrome/nacl/pnacl_component_...
> File chrome/nacl/pnacl_component_installer.cc (right):
> 
>
http://codereview.chromium.org/8348026/diff/9001/chrome/nacl/pnacl_component_...
> chrome/nacl/pnacl_component_installer.cc:27: const uint8 sha256_hash[] = {
> On 2011/11/23 01:51:33, cpu wrote:
> > is this the final hash? if so it is important to put this file under a
> > controlled OWNERS file. So no random person can change this hash.
> 
> Good point I'll move this into a pnacl subdir and add an OWNERS file with some
> of our team members.
> It might not be the final hash, but we still don't want it to be changing
> arbitrarily.
> 
>
http://codereview.chromium.org/8348026/diff/9001/chrome/nacl/pnacl_component_...
> chrome/nacl/pnacl_component_installer.cc:179: // then be able to reload the
page
> and retry (or something).
> On 2011/11/23 01:51:33, cpu wrote:
> > this caveat is worthy of a bug number.
> 
> Done, filed a bug.

[cpu_(ooo_6.6-7.5), 2011-12-14 23:26:22]

http://codereview.chromium.org/8348026/diff/9001/chrome/browser/component_upd...
File chrome/browser/component_updater/component_updater_configurator.cc (right):

http://codereview.chromium.org/8348026/diff/9001/chrome/browser/component_upd...
chrome/browser/component_updater/component_updater_configurator.cc:133: }
Ok , we can leave this here for the time being.

On 2011/11/23 01:51:33, cpu wrote:
> do we need this code anymore?
> 
> We have ways to test without having this deal. See the design document.

[cpu_(ooo_6.6-7.5), 2011-12-14 23:35:25]

lgtm after talking with david.

MY only last request is that your code should respect the fact that you can
disable nacl in about:plugins. If it is disabled there and there is an update it
should remain disabled.

You can do this in a follow up CL or maybe it already does this. Let me know how
you want to handle it.

[jvoung - send to chromium..., 2011-12-14 23:57:08]

On 2011/12/14 23:35:25, cpu wrote:
> lgtm after talking with david.
> 
> MY only last request is that your code should respect the fact that you can
> disable nacl in about:plugins. If it is disabled there and there is an update
it
> should remain disabled.
> 
> You can do this in a follow up CL or maybe it already does this. Let me know
how
> you want to handle it.

The component currently does not include "libppGoogleNaClPluginChrome.so" (just
llvm and some other files) so that part of nacl shouldn't be affected by /
updated with this mechanism.

[commit-bot: I haz the power, 2011-12-15 01:14:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jvoung@google.com/8348026/27001

[commit-bot: I haz the power, 2011-12-15 01:15:01]

Can't apply patch for file chrome/common/chrome_paths.h.
While running patch -p1 --forward --force;
patching file chrome/common/chrome_paths.h
Hunk #1 FAILED at 84.
1 out of 1 hunk FAILED -- saving rejects to file
chrome/common/chrome_paths.h.rej

[commit-bot: I haz the power, 2011-12-15 01:20:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jvoung@google.com/8348026/32003

[commit-bot: I haz the power, 2011-12-15 02:53:21]

Change committed as 114584