Change pnacl to read all resources from the extension.

Change pnacl to read all resources from the extension.
This required enabling StreamAsFile for cross-origin requests for pnacl
resources in the chrome extension.
BUG= http://code.google.com/p/nativeclient/issues/detail?id=2365
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=115362

[sehr (please use chromium), 2011-12-19 20:56:58]

Please review this with an eye to the impact of allowing file downloads.

[jvoung - send to chromium..., 2011-12-19 21:37:08]

some nits

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/manifest.h (right):

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/manifest.h:70: // resources in their
extension origin.  Ths method returns true if such
Ths -> This

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/plugin.h (right):

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.h:301: // This is used by
SRPC-based StreamAsFile().
Perhaps add a comment about permits_extension_urls.

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right):

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:304: //
TODO(sehr,jvoung): change base url to pnacl extension/testing url.
TODO can be removed now =)

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/pnacl_resources.cc (right):

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/pnacl_resources.cc:51:
nacl::string("failed to download ") + resource_urls_[i] + "\n");
maybe incorporate the error_info string as well.

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/pnacl_resources.h (right):

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/pnacl_resources.h:66: // The manifest url
for looking up resources.
Comment about "url" may be stale?

[elijahtaylor (use chromium), 2011-12-19 21:42:52]

First pass... looks like I was scooped on a few from jvoung :/

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/manifest.h (right):

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/manifest.h:70: // resources in their
extension origin.  Ths method returns true if such
s/Ths/This/

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/manifest.h:70: // resources in their
extension origin.  Ths method returns true if such
"their" is unclear here... maybe something like (but not necessarily :) this

"need to access resources from an extension origin different than the plugin's
origin (e.g., conventional-name-of-llc-and-ld extension)."

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (left):

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:318:
CHECK(file_system_ != NULL);
Why did you delete this check?

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right):

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:38:
"chrome-extension://gcodniebolpnpaiggndmcmmfpldlknih/";
Hard-coded extension string seems a bit fragile... are we putting the private
key that the extension is signed with somewhere safe and accessible?  Is this
the only/best way to reference this path?

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:304: //
TODO(sehr,jvoung): change base url to pnacl extension/testing url.
Time to remove TODO?

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/pnacl_resources.cc (right):

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/pnacl_resources.cc:51:
nacl::string("failed to download ") + resource_urls_[i] + "\n");
s/download/resolve/  ?

Also, does this need a break like below?

[sehr (please use chromium), 2011-12-20 01:48:38]

Thanks for the reviews.  PTAL.

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/manifest.h (right):

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/manifest.h:70: // resources in their
extension origin.  Ths method returns true if such
On 2011/12/19 21:37:08, jvoung wrote:
> Ths -> This

Done.

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/manifest.h:70: // resources in their
extension origin.  Ths method returns true if such
On 2011/12/19 21:42:52, elijahtaylor wrote:
> s/Ths/This/

Done.

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/manifest.h:70: // resources in their
extension origin.  Ths method returns true if such
On 2011/12/19 21:42:52, elijahtaylor wrote:
> "their" is unclear here... maybe something like (but not necessarily :) this
> 
> "need to access resources from an extension origin different than the plugin's
> origin (e.g., conventional-name-of-llc-and-ld extension)."

Done.

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/plugin.h (right):

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/plugin.h:301: // This is used by
SRPC-based StreamAsFile().
On 2011/12/19 21:37:08, jvoung wrote:
> Perhaps add a comment about permits_extension_urls.

Done.

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (left):

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:318:
CHECK(file_system_ != NULL);
On 2011/12/19 21:42:52, elijahtaylor wrote:
> Why did you delete this check?

In chrome, new throws rather than returning null, so there is no way to reach
this point with file_system_ == NULL.  This is a vestige of a previous version,
I think.

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right):

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:38:
"chrome-extension://gcodniebolpnpaiggndmcmmfpldlknih/";
On 2011/12/19 21:42:52, elijahtaylor wrote:
> Hard-coded extension string seems a bit fragile... are we putting the private
> key that the extension is signed with somewhere safe and accessible?  Is this
> the only/best way to reference this path?

I agree that we need to have a better way to reference the pnacl chrome
extension, but can't seem to come up with one.  I have added a TODO to express
this.

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:304: //
TODO(sehr,jvoung): change base url to pnacl extension/testing url.
On 2011/12/19 21:37:08, jvoung wrote:
> TODO can be removed now =)

Done.

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:304: //
TODO(sehr,jvoung): change base url to pnacl extension/testing url.
On 2011/12/19 21:42:52, elijahtaylor wrote:
> Time to remove TODO?

Done.

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/pnacl_resources.cc (right):

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/pnacl_resources.cc:51:
nacl::string("failed to download ") + resource_urls_[i] + "\n");
On 2011/12/19 21:37:08, jvoung wrote:
> maybe incorporate the error_info string as well.

Done.

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/pnacl_resources.cc:51:
nacl::string("failed to download ") + resource_urls_[i] + "\n");
On 2011/12/19 21:42:52, elijahtaylor wrote:
> s/download/resolve/  ?
> 
> Also, does this need a break like below?

Agreed on the "resolve".  I tried to harmonize the line break style also.

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/pnacl_resources.h (right):

http://codereview.chromium.org/8974020/diff/3003/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/pnacl_resources.h:66: // The manifest url
for looking up resources.
On 2011/12/19 21:37:08, jvoung wrote:
> Comment about "url" may be stale?

Thanks.  Done.

[sehr (please use chromium), 2011-12-20 17:18:52]

Justin, here's the issue we discussed yesterday.

[elijahtaylor (use chromium), 2011-12-20 17:59:11]

Address the 'break' in pnacl_resources.cc and LGTM

http://codereview.chromium.org/8974020/diff/8001/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right):

http://codereview.chromium.org/8974020/diff/8001/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:232: // Since the
pnacl coordinator manifest is in a base in the chrome extension
I can't grok this: "is in a base in the chrome extension scheme"

http://codereview.chromium.org/8974020/diff/8001/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/pnacl_resources.cc (right):

http://codereview.chromium.org/8974020/diff/8001/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/pnacl_resources.cc:52:
error_info.message() + "\n");
Sorry, what I meant was a C 'break', not a line break.  There's one at line 63
for the download failure case.

[jvoung - send to chromium..., 2011-12-20 18:34:44]

lgtm

http://codereview.chromium.org/8974020/diff/8001/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/file_downloader.cc (right):

http://codereview.chromium.org/8974020/diff/8001/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/file_downloader.cc:95:
url_loader_trusted_interface_->GrantUniversalAccess(
Question: can we use pp::URLRequestInfo::SetAllowCrossOriginRequests(), or does
CORS not make sense/work with extensions and maybe that is why we had this in
the first place?

[sehr (please use chromium), 2011-12-21 00:52:37]

Thanks, waiting for Chrome trybots.  PTAL if you like :-)

http://codereview.chromium.org/8974020/diff/8001/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/file_downloader.cc (right):

http://codereview.chromium.org/8974020/diff/8001/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/file_downloader.cc:95:
url_loader_trusted_interface_->GrantUniversalAccess(
On 2011/12/20 18:34:44, jvoung wrote:
> Question: can we use pp::URLRequestInfo::SetAllowCrossOriginRequests(), or
does
> CORS not make sense/work with extensions and maybe that is why we had this in
> the first place?

As we discussed, we would like to make these web accessible resources using
CORS, and use that.  I am adding a TODO until that support lands.

http://codereview.chromium.org/8974020/diff/8001/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right):

http://codereview.chromium.org/8974020/diff/8001/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:232: // Since the
pnacl coordinator manifest is in a base in the chrome extension
On 2011/12/20 17:59:11, elijahtaylor wrote:
> I can't grok this: "is in a base in the chrome extension scheme"

Agreed.  Hope what's there is better now.

http://codereview.chromium.org/8974020/diff/8001/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/pnacl_resources.cc (right):

http://codereview.chromium.org/8974020/diff/8001/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/pnacl_resources.cc:52:
error_info.message() + "\n");
On 2011/12/20 17:59:11, elijahtaylor wrote:
> Sorry, what I meant was a C 'break', not a line break.  There's one at line 63
> for the download failure case.

Doh, of course.  Fixed.

[sehr (please use chromium), 2011-12-21 20:44:22]

On 2011/12/21 00:52:37, sehr wrote:
> Thanks, waiting for Chrome trybots.  PTAL if you like :-)
> 
>
http://codereview.chromium.org/8974020/diff/8001/ppapi/native_client/src/trus...
> File ppapi/native_client/src/trusted/plugin/file_downloader.cc (right):
> 
>
http://codereview.chromium.org/8974020/diff/8001/ppapi/native_client/src/trus...
> ppapi/native_client/src/trusted/plugin/file_downloader.cc:95:
> url_loader_trusted_interface_->GrantUniversalAccess(
> On 2011/12/20 18:34:44, jvoung wrote:
> > Question: can we use pp::URLRequestInfo::SetAllowCrossOriginRequests(), or
> does
> > CORS not make sense/work with extensions and maybe that is why we had this
in
> > the first place?
> 
> As we discussed, we would like to make these web accessible resources using
> CORS, and use that.  I am adding a TODO until that support lands.
> 
>
http://codereview.chromium.org/8974020/diff/8001/ppapi/native_client/src/trus...
> File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right):
> 
>
http://codereview.chromium.org/8974020/diff/8001/ppapi/native_client/src/trus...
> ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:232: // Since the
> pnacl coordinator manifest is in a base in the chrome extension
> On 2011/12/20 17:59:11, elijahtaylor wrote:
> > I can't grok this: "is in a base in the chrome extension scheme"
> 
> Agreed.  Hope what's there is better now.
> 
>
http://codereview.chromium.org/8974020/diff/8001/ppapi/native_client/src/trus...
> File ppapi/native_client/src/trusted/plugin/pnacl_resources.cc (right):
> 
>
http://codereview.chromium.org/8974020/diff/8001/ppapi/native_client/src/trus...
> ppapi/native_client/src/trusted/plugin/pnacl_resources.cc:52:
> error_info.message() + "\n");
> On 2011/12/20 17:59:11, elijahtaylor wrote:
> > Sorry, what I meant was a C 'break', not a line break.  There's one at line
63
> > for the download failure case.
> 
> Doh, of course.  Fixed.

Thanks.  Committed as r115362.