Pepper: Simplify OpenResource() for Non-SFI.

Pepper: Simplify OpenResource() for Non-SFI.

The Non-SFI implementation of OpenResource is pretty complicated. This is
because the logic to support that operation was entirely in the trusted
plugin. Now, we can perform the necessary logic entirely in Chromium, so
it can be made much simpler.

CQ_EXTRA_TRYBOTS=tryserver.chromium:linux_rel_precise32
BUG=239656
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278859

[dmichael (off chromium), 2014-06-17 21:52:28]

https://codereview.chromium.org/339213003/diff/1/components/nacl/renderer/ppb...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/339213003/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:202: PP_PNaClOptions
pnacl_options = {PP_FALSE, PP_FALSE, 2};
nit: This would be more readable if you zero-initialized and then set the fields
by name...
PP_PNaClOptions pnacl_options = {};
pnacl_options.foo = PP_FALSE;
etc

https://codereview.chromium.org/339213003/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:1424:
base::MessageLoop::current()->PostTask(
Is this only a cosmetic change, or are you trying to make this callable from a
background thread?

I'm pretty sure we haven't made accessing and using NexeLoadManager thread-safe.

If it's just cosmetic, it might be good to DCHECK that you're on the main thread
at the beginning of the method.

https://codereview.chromium.org/339213003/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:1523: DCHECK(callback.func);
nit: This DCHECK seems either unnecessary or like it should be inside
ExternalDownloadFile (to make it easier to catch a bad call at the right place)

https://codereview.chromium.org/339213003/diff/1/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/339213003/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:105: NULL
Can we remove that from PPP_ManifestService in this CL?

[hidehiko, 2014-06-18 04:43:12]

https://codereview.chromium.org/339213003/diff/1/components/nacl/renderer/ppb...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/339213003/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:116: PP_NaClFileInfo
MakeInvalidNaClFileInfo() {
Just FYI:
How about using a constant here?

const PP_NaClFileInfo kInvalidNaClFileInfo = {
  PP_kInvalidFileHandle,
  0,  // token_lo
  0,  // token_hi
};

https://codereview.chromium.org/339213003/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:156: bool
ManifestResolveKey(PP_Instance instance,
I'm rarely see this kind of forward declaration. Can we avoid?

https://codereview.chromium.org/339213003/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:164:
base::Callback<void(int32_t, PP_NaClFileInfo)> callback);
const PP_NaClFileInfo& ? Ditto for below.

https://codereview.chromium.org/339213003/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:217: void
DidDownloadFile(ManifestServiceChannel::OpenResourceCallback callback,
Can be static? Or is it intentional change that the callback won't be called
after this instance is destructed?

Would be great to document the cancellation policy.

Also, if this instance is deleted during the file downloading, the opened file
handle will be leaked.

https://codereview.chromium.org/339213003/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:1065: // For "helper"
processes (llc and ld), we resolve keys manually as there is
Update this comment, too?

https://codereview.chromium.org/339213003/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:1412: file_info.handle =
file.TakePlatformFile();
Setting the platform file handle to invalid file info looks a bit strange.
Can we create:
- a valid file info with the platform file, or
- an invalid file info iff an operation fails.
?

https://codereview.chromium.org/339213003/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:1525: out_file_info->handle =
file_info.handle;
Maybe:
*out_file_info = file_info;

https://codereview.chromium.org/339213003/diff/1/ppapi/nacl_irt/manifest_serv...
File ppapi/nacl_irt/manifest_service.cc (left):

https://codereview.chromium.org/339213003/diff/1/ppapi/nacl_irt/manifest_serv...
ppapi/nacl_irt/manifest_service.cc:45: !ipc_fd.is_file()) {
Why this is removed?

[teravest, 2014-06-18 20:13:44]

https://codereview.chromium.org/339213003/diff/1/components/nacl/renderer/ppb...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/339213003/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:116: PP_NaClFileInfo
MakeInvalidNaClFileInfo() {
On 2014/06/18 04:43:11, hidehiko wrote:
> Just FYI:
> How about using a constant here?
> 
> const PP_NaClFileInfo kInvalidNaClFileInfo = {
>   PP_kInvalidFileHandle,
>   0,  // token_lo
>   0,  // token_hi
> };

Done.

https://codereview.chromium.org/339213003/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:156: bool
ManifestResolveKey(PP_Instance instance,
On 2014/06/18 04:43:11, hidehiko wrote:
> I'm rarely see this kind of forward declaration. Can we avoid?

I don't think so; I could try to reorganize the file so implementations come
before they're used, but I'd like for the function declarations to roughly match
the interface order.

I hope to get remove ManifestResolveKey from PPB_NaCl_Private soon, which should
make things better.

https://codereview.chromium.org/339213003/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:164:
base::Callback<void(int32_t, PP_NaClFileInfo)> callback);
On 2014/06/18 04:43:11, hidehiko wrote:
> const PP_NaClFileInfo& ? Ditto for below.

Done.

https://codereview.chromium.org/339213003/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:202: PP_PNaClOptions
pnacl_options = {PP_FALSE, PP_FALSE, 2};
On 2014/06/17 21:52:28, dmichael wrote:
> nit: This would be more readable if you zero-initialized and then set the
fields
> by name...
> PP_PNaClOptions pnacl_options = {};
> pnacl_options.foo = PP_FALSE;
> etc

Done.

https://codereview.chromium.org/339213003/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:217: void
DidDownloadFile(ManifestServiceChannel::OpenResourceCallback callback,
On 2014/06/18 04:43:11, hidehiko wrote:
> Can be static? Or is it intentional change that the callback won't be called
> after this instance is destructed?
> 
> Would be great to document the cancellation policy.
> 
> Also, if this instance is deleted during the file downloading, the opened file
> handle will be leaked.

Changed this to be static. It's better without the weak ref issue.

https://codereview.chromium.org/339213003/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:1065: // For "helper"
processes (llc and ld), we resolve keys manually as there is
On 2014/06/18 04:43:11, hidehiko wrote:
> Update this comment, too?

Not exactly sure what you want here, but I did update the comment some.

https://codereview.chromium.org/339213003/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:1412: file_info.handle =
file.TakePlatformFile();
On 2014/06/18 04:43:11, hidehiko wrote:
> Setting the platform file handle to invalid file info looks a bit strange.
> Can we create:
> - a valid file info with the platform file, or
> - an invalid file info iff an operation fails.
> ?

Done.

https://codereview.chromium.org/339213003/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:1424:
base::MessageLoop::current()->PostTask(
On 2014/06/17 21:52:28, dmichael wrote:
> Is this only a cosmetic change, or are you trying to make this callable from a
> background thread?
> 
> I'm pretty sure we haven't made accessing and using NexeLoadManager
thread-safe.
> 
> If it's just cosmetic, it might be good to DCHECK that you're on the main
thread
> at the beginning of the method.

Done.

https://codereview.chromium.org/339213003/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:1523: DCHECK(callback.func);
On 2014/06/17 21:52:28, dmichael wrote:
> nit: This DCHECK seems either unnecessary or like it should be inside
> ExternalDownloadFile (to make it easier to catch a bad call at the right
place)

Done.

https://codereview.chromium.org/339213003/diff/1/components/nacl/renderer/ppb...
components/nacl/renderer/ppb_nacl_private_impl.cc:1525: out_file_info->handle =
file_info.handle;
On 2014/06/18 04:43:11, hidehiko wrote:
> Maybe:
> *out_file_info = file_info; 

Done.

https://codereview.chromium.org/339213003/diff/1/ppapi/nacl_irt/manifest_serv...
File ppapi/nacl_irt/manifest_service.cc (left):

https://codereview.chromium.org/339213003/diff/1/ppapi/nacl_irt/manifest_serv...
ppapi/nacl_irt/manifest_service.cc:45: !ipc_fd.is_file()) {
On 2014/06/18 04:43:11, hidehiko wrote:
> Why this is removed?

That's some carryover from debugging. Restored.

https://codereview.chromium.org/339213003/diff/1/ppapi/native_client/src/trus...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://codereview.chromium.org/339213003/diff/1/ppapi/native_client/src/trus...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:105: NULL
On 2014/06/17 21:52:28, dmichael wrote:
> Can we remove that from PPP_ManifestService in this CL?

Done.

[dmichael (off chromium), 2014-06-18 20:40:30]

lgtm

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

https://codereview.chromium.org/339213003/diff/40001/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:117: static const
PP_NaClFileInfo kInvalidNaClFileInfo = {
Note this is not thread-safe, so you should take the static off if you call this
on >1 thread.

[teravest, 2014-06-18 21:17:07]

On 2014/06/18 20:40:30, dmichael wrote:
> lgtm
> 
>
https://codereview.chromium.org/339213003/diff/40001/components/nacl/renderer...
> File components/nacl/renderer/ppb_nacl_private_impl.cc (right):
> 
>
https://codereview.chromium.org/339213003/diff/40001/components/nacl/renderer...
> components/nacl/renderer/ppb_nacl_private_impl.cc:117: static const
> PP_NaClFileInfo kInvalidNaClFileInfo = {
> Note this is not thread-safe, so you should take the static off if you call
this
> on >1 thread.

I removed the "static" here for simplicity.

[hidehiko, 2014-06-20 04:36:11]

lgtm

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

https://codereview.chromium.org/339213003/diff/50006/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:117: const PP_NaClFileInfo
kInvalidNaClFileInfo = {
nit: Sorry for my poor explanation. I meant that put this as a global constant,
and use it directly in the callers.

https://codereview.chromium.org/339213003/diff/50006/components/nacl/renderer...
components/nacl/renderer/ppb_nacl_private_impl.cc:168: DownloadFileCallback
callback);
Oops, sorry I overlooked this in a last cycle.
const DownloadFileCallback& here?

[teravest, 2014-06-20 16:46:11]

On Thu, Jun 19, 2014 at 10:36 PM,  <hidehiko@chromium.org> wrote:
> lgtm
>
>
>
>
>
https://codereview.chromium.org/339213003/diff/50006/components/nacl/renderer...
> File components/nacl/renderer/ppb_nacl_private_impl.cc (right):
>
>
https://codereview.chromium.org/339213003/diff/50006/components/nacl/renderer...
> components/nacl/renderer/ppb_nacl_private_impl.cc:117: const
> PP_NaClFileInfo kInvalidNaClFileInfo = {
> nit: Sorry for my poor explanation. I meant that put this as a global
> constant, and use it directly in the callers.

Done.

>
>
https://codereview.chromium.org/339213003/diff/50006/components/nacl/renderer...
> components/nacl/renderer/ppb_nacl_private_impl.cc:168:
> DownloadFileCallback callback);
> Oops, sorry I overlooked this in a last cycle.
> const DownloadFileCallback& here?

Done.

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

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

[teravest, 2014-06-20 19:10:01]

The CQ bit was checked by teravest@chromium.org

[commit-bot: I haz the power, 2014-06-20 19:13:12]

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

[commit-bot: I haz the power, 2014-06-20 22:09:37]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-06-20 23:31:37]

Change committed as 278859