Pepper: Nexe downloading out of the trusted plugin.

Pepper: Nexe downloading out of the trusted plugin.

This moves the logic for nexe downloading outside the trusted plugin. This is
part of a series of changes to remove FileDownloader and its associated logic.

I've tried to make this change small so it will be easy to review. A followup
change will clean up some of the accounting logic post-download to make this
path simpler and easier to follow.

BUG=370556
R=bbudge@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271090

[bbudge, 2014-05-12 22:32:05]

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
File components/nacl/renderer/file_downloader.cc (right):

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
components/nacl/renderer/file_downloader.cc:49: status_code_ =
response.httpStatusCode();
Move this before previous statement, and use it instead of accessor?

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
File components/nacl/renderer/file_downloader.h (right):

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
components/nacl/renderer/file_downloader.h:29: typedef
base::Callback<void(Status, int)> FileDownloaderCallback;
name suggestion: Callback or StatusCallback

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
components/nacl/renderer/file_downloader.h:33: FileDownloaderCallback cb);
nit: fits on a single line.

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:263:
plugin_instance->GetContainer()->element().document().securityOrigin();
This function and the following one could be simplified significantly if you
pass in the WebDocument as the first parameter. i.e. caller does
plugin_instance->GetContainer()->element().document().

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:1260: class
DownloadNexeProgressTracker {
I don't see where this is used. Also, it doesn't seem to do anything.

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:1307: // We shouldn't hit this
if the file URL is in an installed app.
comment seems wrong.

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:1318: blink::WebURLLoader*
url_loader = CreateWebURLLoader(instance, gurl);
Now that we're in Chrome-land, can we use scoped_ptr to protect against leaks?

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:1322: FileDownloader* client =
new FileDownloader(
'client' is a slightly confusing name.

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:1339:
DispatchProgressEvent(instance, event);
Might be a little more readable if you create the event as a temporary in the
function invocation.

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:1343: blink::WebURLLoader*
loader,
scoped_ptr<blink::WebURLLoader>?

[teravest, 2014-05-13 17:05:32]

I'm still investigating the Windows "bad ELF magic" failures.

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
File components/nacl/renderer/file_downloader.cc (right):

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
components/nacl/renderer/file_downloader.cc:49: status_code_ =
response.httpStatusCode();
On 2014/05/12 22:32:06, bbudge wrote:
> Move this before previous statement, and use it instead of accessor?

Done.

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
File components/nacl/renderer/file_downloader.h (right):

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
components/nacl/renderer/file_downloader.h:29: typedef
base::Callback<void(Status, int)> FileDownloaderCallback;
On 2014/05/12 22:32:06, bbudge wrote:
> name suggestion: Callback or StatusCallback

Done.

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
components/nacl/renderer/file_downloader.h:33: FileDownloaderCallback cb);
On 2014/05/12 22:32:06, bbudge wrote:
> nit: fits on a single line.

Done.

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:263:
plugin_instance->GetContainer()->element().document().securityOrigin();
On 2014/05/12 22:32:06, bbudge wrote:
> This function and the following one could be simplified significantly if you
> pass in the WebDocument as the first parameter. i.e. caller does
> plugin_instance->GetContainer()->element().document().

Done.

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:1260: class
DownloadNexeProgressTracker {
On 2014/05/12 22:32:06, bbudge wrote:
> I don't see where this is used. Also, it doesn't seem to do anything.

Thanks for pointing that out. I'd completely forgotten about this.
I've changed the progress event code to use this now; this object is necessary
to keep from dispatching progress events more often than once every 10ms.

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:1307: // We shouldn't hit this
if the file URL is in an installed app.
On 2014/05/12 22:32:06, bbudge wrote:
> comment seems wrong.

Removed.

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:1318: blink::WebURLLoader*
url_loader = CreateWebURLLoader(instance, gurl);
On 2014/05/12 22:32:06, bbudge wrote:
> Now that we're in Chrome-land, can we use scoped_ptr to protect against leaks?

Done.

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:1322: FileDownloader* client =
new FileDownloader(
On 2014/05/12 22:32:06, bbudge wrote:
> 'client' is a slightly confusing name.

Changed to file_downloader

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:1339:
DispatchProgressEvent(instance, event);
On 2014/05/12 22:32:06, bbudge wrote:
> Might be a little more readable if you create the event as a temporary in the
> function invocation.

Done.

https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:1343: blink::WebURLLoader*
loader,
On 2014/05/12 22:32:06, bbudge wrote:
> scoped_ptr<blink::WebURLLoader>?

Done.

[teravest, 2014-05-13 21:36:40]

It's something ugly with tests hanging after I changed the memory
management for WebURLLoader; I'll nudge the review thread again once
I've got that fixed.

On Tue, May 13, 2014 at 11:05 AM,  <teravest@chromium.org> wrote:
> I'm still investigating the Windows "bad ELF magic" failures.
>
>
>
>
https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
> File components/nacl/renderer/file_downloader.cc (right):
>
>
https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
> components/nacl/renderer/file_downloader.cc:49: status_code_ =
> response.httpStatusCode();
> On 2014/05/12 22:32:06, bbudge wrote:
>>
>> Move this before previous statement, and use it instead of accessor?
>
>
> Done.
>
>
>
https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
> File components/nacl/renderer/file_downloader.h (right):
>
>
https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
> components/nacl/renderer/file_downloader.h:29: typedef
> base::Callback<void(Status, int)> FileDownloaderCallback;
> On 2014/05/12 22:32:06, bbudge wrote:
>>
>> name suggestion: Callback or StatusCallback
>
>
> Done.
>
>
>
https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
> components/nacl/renderer/file_downloader.h:33: FileDownloaderCallback
> cb);
> On 2014/05/12 22:32:06, bbudge wrote:
>>
>> nit: fits on a single line.
>
>
> Done.
>
>
>
https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
> File components/nacl/renderer/ppb_nacl_private_impl.cc (right):
>
>
https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
> components/nacl/renderer/ppb_nacl_private_impl.cc:263:
> plugin_instance->GetContainer()->element().document().securityOrigin();
> On 2014/05/12 22:32:06, bbudge wrote:
>>
>> This function and the following one could be simplified significantly
>
> if you
>>
>> pass in the WebDocument as the first parameter. i.e. caller does
>> plugin_instance->GetContainer()->element().document().
>
>
> Done.
>
>
>
https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
> components/nacl/renderer/ppb_nacl_private_impl.cc:1260: class
> DownloadNexeProgressTracker {
> On 2014/05/12 22:32:06, bbudge wrote:
>>
>> I don't see where this is used. Also, it doesn't seem to do anything.
>
>
> Thanks for pointing that out. I'd completely forgotten about this.
> I've changed the progress event code to use this now; this object is
> necessary to keep from dispatching progress events more often than once
> every 10ms.
>
>
>
https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
> components/nacl/renderer/ppb_nacl_private_impl.cc:1307: // We shouldn't
> hit this if the file URL is in an installed app.
> On 2014/05/12 22:32:06, bbudge wrote:
>>
>> comment seems wrong.
>
>
> Removed.
>
>
>
https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
> components/nacl/renderer/ppb_nacl_private_impl.cc:1318:
> blink::WebURLLoader* url_loader = CreateWebURLLoader(instance, gurl);
> On 2014/05/12 22:32:06, bbudge wrote:
>>
>> Now that we're in Chrome-land, can we use scoped_ptr to protect
>
> against leaks?
>
> Done.
>
>
>
https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
> components/nacl/renderer/ppb_nacl_private_impl.cc:1322: FileDownloader*
> client = new FileDownloader(
> On 2014/05/12 22:32:06, bbudge wrote:
>>
>> 'client' is a slightly confusing name.
>
>
> Changed to file_downloader
>
>
>
https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
> components/nacl/renderer/ppb_nacl_private_impl.cc:1339:
> DispatchProgressEvent(instance, event);
> On 2014/05/12 22:32:06, bbudge wrote:
>>
>> Might be a little more readable if you create the event as a temporary
>
> in the
>>
>> function invocation.
>
>
> Done.
>
>
>
https://codereview.chromium.org/276423003/diff/80001/components/nacl/renderer...
> components/nacl/renderer/ppb_nacl_private_impl.cc:1343:
> blink::WebURLLoader* loader,
> On 2014/05/12 22:32:06, bbudge wrote:
>>
>> scoped_ptr<blink::WebURLLoader>?
>
>
> Done.
>
> https://codereview.chromium.org/276423003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[teravest, 2014-05-14 17:03:20]

Testing on Windows locally looks good. Mind taking another look?

[teravest, 2014-05-14 21:35:44]

I'm investigating the Windows failures.

[bbudge, 2014-05-14 21:46:18]

A few more things.

https://codereview.chromium.org/276423003/diff/160001/components/nacl/rendere...
File components/nacl/renderer/file_downloader.h (right):

https://codereview.chromium.org/276423003/diff/160001/components/nacl/rendere...
components/nacl/renderer/file_downloader.h:28: };
Comment here to say what the 'int' param (http status code) is?

https://codereview.chromium.org/276423003/diff/160001/components/nacl/rendere...
components/nacl/renderer/file_downloader.h:29: typedef
base::Callback<void(Status, int)> StatusCallback;
Comment here to say what the int64_t's are?

https://codereview.chromium.org/276423003/diff/160001/components/nacl/rendere...
components/nacl/renderer/file_downloader.h:58: StatusCallback cb_;
s/cb_/status_cb_ ?

https://codereview.chromium.org/276423003/diff/160001/components/nacl/rendere...
components/nacl/renderer/file_downloader.h:60: int status_code_;
Could we rename this to http_status_code_? Different statuses (stati?) are
confusing me.

https://codereview.chromium.org/276423003/diff/160001/components/nacl/rendere...
File components/nacl/renderer/manifest_downloader.h (right):

https://codereview.chromium.org/276423003/diff/160001/components/nacl/rendere...
components/nacl/renderer/manifest_downloader.h:25: ManifestDownloaderCallback;
Could be named 'ResultCallback' or even 'Callback'.

https://codereview.chromium.org/276423003/diff/160001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/276423003/diff/160001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:1252: const char* url;
This pointer makes me very uneasy. It seems it could easily become a dangling
pointer unless you and the callers are very careful. Can we make it a
std::string?

https://codereview.chromium.org/276423003/diff/160001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:1259: class
DownloadProgressTracker {
How about ProgressEventRateLimiter?

https://codereview.chromium.org/276423003/diff/160001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:1346: target_file,
out_handle),
In this case, request's 'url' field likely will be invalid by the time the
callback is invoked.

https://codereview.chromium.org/276423003/diff/160001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/plugin.cc (left):

https://codereview.chromium.org/276423003/diff/160001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/plugin.cc:785: file_downloader =
&nexe_downloader_;
Since we're eliminating the special handling of nexe_downloader_, I'm not sure
how well multiple downloads are tested. Perhaps you could verify that something
like Angrybots still works with the patch:

https://chrome.google.com/webstore/detail/angrybots/ibdomdppnicebcgjhcjfchbon...

https://codereview.chromium.org/276423003/diff/160001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/276423003/diff/160001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/plugin.cc:67: file_desc =
reinterpret_cast<uintptr_t>(handle);
I did a little code search, and everywhere else we cast to intptr_t.

https://codereview.chromium.org/276423003/diff/160001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/plugin.h (right):

https://codereview.chromium.org/276423003/diff/160001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/plugin.h:284: // Original URL (without
resolution) for the .nexe program to load.
How about 'Original, unresolved URL for ...' ?

[teravest, 2014-05-15 19:12:21]

https://codereview.chromium.org/276423003/diff/160001/components/nacl/rendere...
File components/nacl/renderer/file_downloader.h (right):

https://codereview.chromium.org/276423003/diff/160001/components/nacl/rendere...
components/nacl/renderer/file_downloader.h:28: };
On 2014/05/14 21:46:18, bbudge wrote:
> Comment here to say what the 'int' param (http status code) is?

Done.

https://codereview.chromium.org/276423003/diff/160001/components/nacl/rendere...
components/nacl/renderer/file_downloader.h:29: typedef
base::Callback<void(Status, int)> StatusCallback;
On 2014/05/14 21:46:18, bbudge wrote:
> Comment here to say what the int64_t's are?

Done.

https://codereview.chromium.org/276423003/diff/160001/components/nacl/rendere...
components/nacl/renderer/file_downloader.h:58: StatusCallback cb_;
On 2014/05/14 21:46:18, bbudge wrote:
> s/cb_/status_cb_ ?

Done.

https://codereview.chromium.org/276423003/diff/160001/components/nacl/rendere...
components/nacl/renderer/file_downloader.h:60: int status_code_;
On 2014/05/14 21:46:18, bbudge wrote:
> Could we rename this to http_status_code_? Different statuses (stati?) are
> confusing me.

Done.

https://codereview.chromium.org/276423003/diff/160001/components/nacl/rendere...
File components/nacl/renderer/manifest_downloader.h (right):

https://codereview.chromium.org/276423003/diff/160001/components/nacl/rendere...
components/nacl/renderer/manifest_downloader.h:25: ManifestDownloaderCallback;
On 2014/05/14 21:46:18, bbudge wrote:
> Could be named 'ResultCallback' or even 'Callback'.

Done.

https://codereview.chromium.org/276423003/diff/160001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/276423003/diff/160001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:1252: const char* url;
On 2014/05/14 21:46:18, bbudge wrote:
> This pointer makes me very uneasy. It seems it could easily become a dangling
> pointer unless you and the callers are very careful. Can we make it a
> std::string?

Done.

https://codereview.chromium.org/276423003/diff/160001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:1259: class
DownloadProgressTracker {
On 2014/05/14 21:46:18, bbudge wrote:
> How about ProgressEventRateLimiter?

Done.

https://codereview.chromium.org/276423003/diff/160001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:1346: target_file,
out_handle),
On 2014/05/14 21:46:18, bbudge wrote:
> In this case, request's 'url' field likely will be invalid by the time the
> callback is invoked.

Changed to std::string.

https://codereview.chromium.org/276423003/diff/160001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/plugin.cc (left):

https://codereview.chromium.org/276423003/diff/160001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/plugin.cc:785: file_downloader =
&nexe_downloader_;
On 2014/05/14 21:46:18, bbudge wrote:
> Since we're eliminating the special handling of nexe_downloader_, I'm not sure
> how well multiple downloads are tested. Perhaps you could verify that
something
> like Angrybots still works with the patch:
> 
>
https://chrome.google.com/webstore/detail/angrybots/ibdomdppnicebcgjhcjfchbon...

I tested with Angry bots, it loaded and ran fine.

https://codereview.chromium.org/276423003/diff/160001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://codereview.chromium.org/276423003/diff/160001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/plugin.cc:67: file_desc =
reinterpret_cast<uintptr_t>(handle);
On 2014/05/14 21:46:18, bbudge wrote:
> I did a little code search, and everywhere else we cast to intptr_t.

Done.

https://codereview.chromium.org/276423003/diff/160001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/plugin.h (right):

https://codereview.chromium.org/276423003/diff/160001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/plugin.h:284: // Original URL (without
resolution) for the .nexe program to load.
On 2014/05/14 21:46:18, bbudge wrote:
> How about 'Original, unresolved URL for ...' ?

Done.

[bbudge, 2014-05-16 17:20:51]

LGTM w/nits

https://codereview.chromium.org/276423003/diff/200001/components/nacl/rendere...
File components/nacl/renderer/file_downloader.cc (right):

https://codereview.chromium.org/276423003/diff/200001/components/nacl/rendere...
components/nacl/renderer/file_downloader.cc:46: // Sets -1 if the content length
is unknown. Send before issuing callback.
s/Sets -1/Set to -1
s/Send/Set ?

https://codereview.chromium.org/276423003/diff/200001/components/nacl/rendere...
File components/nacl/renderer/file_downloader.h (right):

https://codereview.chromium.org/276423003/diff/200001/components/nacl/rendere...
components/nacl/renderer/file_downloader.h:26: ACCESS_DENIED,  // Access denied
nit: comments seem redundant

https://codereview.chromium.org/276423003/diff/200001/components/nacl/rendere...
components/nacl/renderer/file_downloader.h:30: // Provides a FileDownloader
status and the HTTP status code.
nit s/a/the

https://codereview.chromium.org/276423003/diff/200001/components/nacl/rendere...
components/nacl/renderer/file_downloader.h:34: // expected.
nit: how about?

// Provides the bytes received so far, and the total bytes expected to be
// received.

https://codereview.chromium.org/276423003/diff/200001/components/nacl/rendere...
components/nacl/renderer/file_downloader.h:37: // Creates a FileDownloader that
notifies the caller about download progress.
optionally notifies?

https://codereview.chromium.org/276423003/diff/200001/components/nacl/rendere...
components/nacl/renderer/file_downloader.h:40: StatusCallback cb,
s/cb/status_cb

https://codereview.chromium.org/276423003/diff/200001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/276423003/diff/200001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:1339: // FileDownloader
deletes itself after invoking the callback.
s/callback/status callback ?
or alternately, DownloadNexeCompletion.

https://codereview.chromium.org/276423003/diff/200001/ppapi/api/private/ppb_n...
File ppapi/api/private/ppb_nacl_private.idl (right):

https://codereview.chromium.org/276423003/diff/200001/ppapi/api/private/ppb_n...
ppapi/api/private/ppb_nacl_private.idl:440: /* Downloads the .nexe file at the
given URL to a file, and populates
nit s/populates/sets
following line s/with/to

[teravest, 2014-05-16 17:46:18]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-05-16 17:47:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/teravest@chromium.org/276423003/240001

[teravest, 2014-05-16 20:18:42]

Committed patchset #9 manually as r271090 (presubmit successful).