Add success status to ReportTranslationFinished in ppb_nacl_private and IPC

Add success status to ReportTranslationFinished in ppb_nacl_private and IPC

The existing mechanism for the plugin to report a finished translation
was only used for successful translations. This CL also reports a failed
translation (but only if GetNexeFd returned successfully) to allow the
browser to clean up its tracking state.

TBR=binji@chromium.org, dmichael@chromium.org, jschuh@chromium.org, jvoung@chromium.org

BUG= https://code.google.com/p/nativeclient/issues/detail?id=3372
TEST=browser_tests NaClBrowserTestPnacl*

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

[Derek Schuff, 2013-08-06 18:12:00]

reviews:
jvoung: everything
dmichael: chrome/renderer/pepper OWNERS
jschuh: IPC security
binji: ppapi/api OWNERS

[jvoung (off chromium), 2013-08-06 18:35:28]

https://codereview.chromium.org/22309007/diff/3001/chrome/browser/nacl_host/p...
File chrome/browser/nacl_host/pnacl_host.cc (right):

https://codereview.chromium.org/22309007/diff/3001/chrome/browser/nacl_host/p...
chrome/browser/nacl_host/pnacl_host.cc:195: pending_translations_.erase(it);
leave TODO about checking |success| ?

https://codereview.chromium.org/22309007/diff/5017/chrome/renderer/pepper/pna...
File chrome/renderer/pepper/pnacl_translation_resource_host.h (right):

https://codereview.chromium.org/22309007/diff/5017/chrome/renderer/pepper/pna...
chrome/renderer/pepper/pnacl_translation_resource_host.h:36: void
ReportTranslationFinished(PP_Instance instance, PP_Bool error);
replace "error" with "success" ?

https://codereview.chromium.org/22309007/diff/5017/ppapi/api/private/ppb_nacl...
File ppapi/api/private/ppb_nacl_private.idl (right):

https://codereview.chromium.org/22309007/diff/5017/ppapi/api/private/ppb_nacl...
ppapi/api/private/ppb_nacl_private.idl:128: * guaranteed to, however; if there
is an error on the browser side
I forget, what was this last part of the comment about?  It sounds like there is
an exception when the renderer does *not* call GetNexeFd.  Doesn't call
GetNexeFd, but the callback is called w/ an error code if the file is too big
for the cache, etc. ?

https://codereview.chromium.org/22309007/diff/5017/ppapi/native_client/src/tr...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right):

https://codereview.chromium.org/22309007/diff/5017/ppapi/native_client/src/tr...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:984:
PP_FromBool(false));
just plain PP_FALSE?

[dmichael (off chromium), 2013-08-06 18:53:58]

just nits...  OWNERS lgtm

https://codereview.chromium.org/22309007/diff/5017/chrome/browser/nacl_host/p...
File chrome/browser/nacl_host/pnacl_host.cc (right):

https://codereview.chromium.org/22309007/diff/5017/chrome/browser/nacl_host/p...
chrome/browser/nacl_host/pnacl_host.cc:185: bool success) {
You're not actually doing anything with |success|. Is that intentional? It seems
like there ought to at least be a TODO, and IMHO it's helpful to comment out
success:
bool /* success */
to make it obvious it's unused.

https://codereview.chromium.org/22309007/diff/5017/ppapi/api/private/ppb_nacl...
File ppapi/api/private/ppb_nacl_private.idl (right):

https://codereview.chromium.org/22309007/diff/5017/ppapi/api/private/ppb_nacl...
ppapi/api/private/ppb_nacl_private.idl:128: * guaranteed to, however; if there
is an error on the browser side
nit: probably should have a comma after "on the browser side"

https://codereview.chromium.org/22309007/diff/5017/ppapi/native_client/src/tr...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right):

https://codereview.chromium.org/22309007/diff/5017/ppapi/native_client/src/tr...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:345:
PP_FromBool(false));
PP_FALSE?

[Derek Schuff, 2013-08-06 21:21:13]

thanks for the reviews

https://codereview.chromium.org/22309007/diff/5017/chrome/browser/nacl_host/p...
File chrome/browser/nacl_host/pnacl_host.cc (right):

https://codereview.chromium.org/22309007/diff/5017/chrome/browser/nacl_host/p...
chrome/browser/nacl_host/pnacl_host.cc:185: bool success) {
On 2013/08/06 18:53:58, dmichael wrote:
> You're not actually doing anything with |success|. Is that intentional? It
seems
> like there ought to at least be a TODO, and IMHO it's helpful to comment out
> success:
> bool /* success */
> to make it obvious it's unused.
I have another CL currently in flight which uses it.

https://codereview.chromium.org/22309007/diff/5017/chrome/renderer/pepper/pna...
File chrome/renderer/pepper/pnacl_translation_resource_host.h (right):

https://codereview.chromium.org/22309007/diff/5017/chrome/renderer/pepper/pna...
chrome/renderer/pepper/pnacl_translation_resource_host.h:36: void
ReportTranslationFinished(PP_Instance instance, PP_Bool error);
On 2013/08/06 18:35:28, jvoung (cr) wrote:
> replace "error" with "success" ?
Ah yeah... it was originally "error" but I replaced it with "success" to be
consistent with EnsurePnaclInstalledReply

https://codereview.chromium.org/22309007/diff/5017/ppapi/api/private/ppb_nacl...
File ppapi/api/private/ppb_nacl_private.idl (right):

https://codereview.chromium.org/22309007/diff/5017/ppapi/api/private/ppb_nacl...
ppapi/api/private/ppb_nacl_private.idl:128: * guaranteed to, however; if there
is an error on the browser side
On 2013/08/06 18:35:28, jvoung (cr) wrote:
> I forget, what was this last part of the comment about?  It sounds like there
is
> an exception when the renderer does *not* call GetNexeFd.  Doesn't call
> GetNexeFd, but the callback is called w/ an error code if the file is too big
> for the cache, etc. ?
Clarified.

https://codereview.chromium.org/22309007/diff/5017/ppapi/api/private/ppb_nacl...
ppapi/api/private/ppb_nacl_private.idl:128: * guaranteed to, however; if there
is an error on the browser side
On 2013/08/06 18:53:58, dmichael wrote:
> nit: probably should have a comma after "on the browser side"

Done.

https://codereview.chromium.org/22309007/diff/5017/ppapi/native_client/src/tr...
File ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc (right):

https://codereview.chromium.org/22309007/diff/5017/ppapi/native_client/src/tr...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:345:
PP_FromBool(false));
On 2013/08/06 18:53:58, dmichael wrote:
> PP_FALSE?
what an amazing world we live in, that such things exist!

https://codereview.chromium.org/22309007/diff/5017/ppapi/native_client/src/tr...
ppapi/native_client/src/trusted/plugin/pnacl_coordinator.cc:984:
PP_FromBool(false));
On 2013/08/06 18:35:28, jvoung (cr) wrote:
> just plain PP_FALSE?

Done.

[jvoung (off chromium), 2013-08-06 21:24:23]

lgtm

[jschuh, 2013-08-06 22:56:47]

this is not the one i was afraid of. ipc security lgtm. :)

[Derek Schuff, 2013-08-07 16:13:10]

I'm going to go ahead and TBR binji since this is a trivial change and not a
real pepper API

[commit-bot: I haz the power, 2013-08-07 16:13:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dschuff@chromium.org/22309007/19001

[binji, 2013-08-07 17:28:40]

ppapi/api lgtm

[Derek Schuff, 2013-08-07 19:23:47]

Committed patchset #4 manually as r216247 (presubmit successful).