Pepper: Move StreamAsFile out of trusted plugin.

Pepper: Move StreamAsFile out of trusted plugin.

This removes a significant amount of code from the trusted plugin and
simplifies the callbacks that are used in PluginReverseInterface.

I was confused at first by the existing use of url_file_info_map_; it looks
like it was intended to serve as a cache for retrieved files, but I think the
files would be reopened every time, even if the file was already open. For
simplicity, I didn't try to add any caching behavior in this change.

Note that this changes progress events to be rate limited at 10ms per-file,
not 10ms per-plugin. It's a bit simpler to write this way, but I can
change that limit to be per-plugin if that makes more sense.

BUG=370556
R=bbudge@chromium.org

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

[teravest, 2014-05-21 20:29:08]

Mind taking another look, Bill?

This looks ok on Windows now, here's a successful run:
http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...

[bbudge, 2014-05-22 17:14:15]

I'm a little confused about NaClFileInfo. I thought that was for the fast path
loading of nexes.

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

https://codereview.chromium.org/294633003/diff/130001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:293: default:
Leave out the default and it will catch if someone adds an error and doesn't
translate it here.
the fn can still return PP_ERROR_FAILED at the end.

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

https://codereview.chromium.org/294633003/diff/130001/ppapi/api/private/ppb_n...
ppapi/api/private/ppb_nacl_private.idl:437: [in] PP_CompletionCallback
callback);
I thought NaClFileInfo was for nexes on the fast path.

[teravest, 2014-05-22 17:29:54]

On Thu, May 22, 2014 at 11:14 AM,  <bbudge@chromium.org> wrote:
> I'm a little confused about NaClFileInfo. I thought that was for the fast
> path
> loading of nexes.

I think it's used for files that are fetched as part of the manifest as well.
A while back, I sent a patch that changed this logic to just send a
descriptor instead of all the NaClFileInfo, but ncbray@ said those
tokens are needed.

>
>
>
https://codereview.chromium.org/294633003/diff/130001/components/nacl/rendere...
> File components/nacl/renderer/ppb_nacl_private_impl.cc (right):
>
>
https://codereview.chromium.org/294633003/diff/130001/components/nacl/rendere...
> components/nacl/renderer/ppb_nacl_private_impl.cc:293: default:
> Leave out the default and it will catch if someone adds an error and
> doesn't translate it here.
> the fn can still return PP_ERROR_FAILED at the end.

Done.

>
>
https://codereview.chromium.org/294633003/diff/130001/ppapi/api/private/ppb_n...
> File ppapi/api/private/ppb_nacl_private.idl (right):
>
>
https://codereview.chromium.org/294633003/diff/130001/ppapi/api/private/ppb_n...
> ppapi/api/private/ppb_nacl_private.idl:437: [in] PP_CompletionCallback
> callback);
> I thought NaClFileInfo was for nexes on the fast path.
>
> https://codereview.chromium.org/294633003/

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

[bbudge, 2014-05-22 20:31:22]

Couple of comments. Otherwise LGTM

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

https://codereview.chromium.org/294633003/diff/170001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:292: return PP_ERROR_FAILED;
Comment to explain why default is omitted? e.g.
// No default case, to catch unhandled Status values.

https://codereview.chromium.org/294633003/diff/170001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:1425: int http_status);
You could just define the fn here.

https://codereview.chromium.org/294633003/diff/170001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:1511: file_info->handle =
file;
indent.

https://codereview.chromium.org/294633003/diff/170001/ppapi/native_client/src...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/294633003/diff/170001/ppapi/native_client/src...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:421: // reasonable to
provide a file descriptor with write access.
We know the plugin can't map this with executable access?

[teravest, 2014-05-23 15:16:58]

On Thu, May 22, 2014 at 2:31 PM,  <bbudge@chromium.org> wrote:
> Couple of comments. Otherwise LGTM
>
>
>
https://codereview.chromium.org/294633003/diff/170001/components/nacl/rendere...
> File components/nacl/renderer/ppb_nacl_private_impl.cc (right):
>
>
https://codereview.chromium.org/294633003/diff/170001/components/nacl/rendere...
> components/nacl/renderer/ppb_nacl_private_impl.cc:292: return
> PP_ERROR_FAILED;
> Comment to explain why default is omitted? e.g.
> // No default case, to catch unhandled Status values.

Done.

>
>
https://codereview.chromium.org/294633003/diff/170001/components/nacl/rendere...
> components/nacl/renderer/ppb_nacl_private_impl.cc:1425: int
> http_status);
> You could just define the fn here.

Done.

>
>
https://codereview.chromium.org/294633003/diff/170001/components/nacl/rendere...
> components/nacl/renderer/ppb_nacl_private_impl.cc:1511:
> file_info->handle = file;
> indent.

Done.

>
>
https://codereview.chromium.org/294633003/diff/170001/ppapi/native_client/src...
> File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):
>
>
https://codereview.chromium.org/294633003/diff/170001/ppapi/native_client/src...
> ppapi/native_client/src/trusted/plugin/service_runtime.cc:421: //
> reasonable to provide a file descriptor with write access.
> We know the plugin can't map this with executable access?

This patch doesn't change the flags that this file descriptor is
opened with-- I was just adding a comment to make that decision more
explicit.

The plugin author could have any contents in the opened file. I'm not
catching what the problem is you're alluding to. :)


>
> https://codereview.chromium.org/294633003/

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

[teravest, 2014-05-23 15:48:23]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-05-23 15:49:22]

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

[teravest, 2014-05-23 16:55:19]

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