Implement pepper interface and plumbing for HRD's UI on ChromeOS

Implement pepper interface and plumbing for HRD's UI on ChromeOS

This change extends the PPB_Talk_Private interface (introduces version 2.0)
and adds methods to show various security related UI to support Hangouts
Remote Desktop on ChromeOS.

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

[Josh Horwich, 2013-05-31 22:22:16]

Some thoughts on an alternate approach to avoid having so many APIs / IPCs
inline.

Also, consider adding to ppapi/tests/test_talk_private.cc - in particular,
testing of both the existing 1_0 and new 2_0 GetPermission API since existing
talk uses 1_0.

https://codereview.chromium.org/16271005/diff/1/ppapi/api/private/ppb_talk_pr...
File ppapi/api/private/ppb_talk_private.idl (right):

https://codereview.chromium.org/16271005/diff/1/ppapi/api/private/ppb_talk_pr...
ppapi/api/private/ppb_talk_private.idl:62: * Same parameters and behaviors as
<code>GetPermission()</code>.
Just for clarification, is the ability to have only one "Permission" call at a
time true across the different flavors of "GetPermission"? e.g. if I call
GetPermission() for a screencast, then before it completes I call
GetRemotingPermission() would I get PP_ERROR_INPROGRESS ?

From the implementation I believe the answer is "yes" - if so we should clarify
this.

https://codereview.chromium.org/16271005/diff/1/ppapi/api/private/ppb_talk_pr...
ppapi/api/private/ppb_talk_private.idl:76: [in] PP_CompletionCallback callback);
Another idea would be to combine GetRemotingContinuePermission with
GetRemotingPermission and adding an enum to which permission is being requested.

Similarly, it might even make sense to combine all three in API 2_0 to a single
GetPermission API w/enum - and in theory we could vend the API 1_0 struct to a
chrome-provided plugin-side wrapper that uses the same IPC for
PPB_Talk_Private_1_0's GetPermission as PPB_Talk_Private_2_0's
"GetPermission(resource, SCREENCASTING, callback). If my memory's correct you
could handle this all in ppb_talk_private_thunk.cc.

We might want input from Pepper team members on whether this is a good, or bad,
idea though.

https://codereview.chromium.org/16271005/diff/1/ppapi/api/private/ppb_talk_pr...
ppapi/api/private/ppb_talk_private.idl:81: * errors.
Any restrictions on what thread can call this function? Similarly, will the
event_callback always be invoked on the Pepper main thread?

https://codereview.chromium.org/16271005/diff/1/ppapi/api/private/ppb_talk_pr...
ppapi/api/private/ppb_talk_private.idl:90: * Hides the remoting-in-progress UI
and unregisters the event callback.
Is this synchronous like StartRemoting? Is there any guarantee around when the
registered event_callback would no longer be called, e.g. only after
StopRemoting is called?

https://codereview.chromium.org/16271005/diff/1/ppapi/api/private/ppb_talk_pr...
ppapi/api/private/ppb_talk_private.idl:93: void StopRemoting(
Should this return int32_t so we can error in bizarre cases like talk_resource
being invalid, remoting wasn't started, etc?

[dcaiafa, 2013-06-04 00:11:44]

PTAL. Thanks!

https://codereview.chromium.org/16271005/diff/1/ppapi/api/private/ppb_talk_pr...
File ppapi/api/private/ppb_talk_private.idl (right):

https://codereview.chromium.org/16271005/diff/1/ppapi/api/private/ppb_talk_pr...
ppapi/api/private/ppb_talk_private.idl:62: * Same parameters and behaviors as
<code>GetPermission()</code>.
Yes, only one pending permission request is allowed at a time. The new interface
reflects that now.

On 2013/05/31 22:22:16, Josh Horwich wrote:
> Just for clarification, is the ability to have only one "Permission" call at a
> time true across the different flavors of "GetPermission"? e.g. if I call
> GetPermission() for a screencast, then before it completes I call
> GetRemotingPermission() would I get PP_ERROR_INPROGRESS ?
> 
> From the implementation I believe the answer is "yes" - if so we should
clarify
> this.

https://codereview.chromium.org/16271005/diff/1/ppapi/api/private/ppb_talk_pr...
ppapi/api/private/ppb_talk_private.idl:76: [in] PP_CompletionCallback callback);
Excellent suggestion. Done.

On 2013/05/31 22:22:16, Josh Horwich wrote:
> Another idea would be to combine GetRemotingContinuePermission with
> GetRemotingPermission and adding an enum to which permission is being
requested.
> 
> Similarly, it might even make sense to combine all three in API 2_0 to a
single
> GetPermission API w/enum - and in theory we could vend the API 1_0 struct to a
> chrome-provided plugin-side wrapper that uses the same IPC for
> PPB_Talk_Private_1_0's GetPermission as PPB_Talk_Private_2_0's
> "GetPermission(resource, SCREENCASTING, callback). If my memory's correct you
> could handle this all in ppb_talk_private_thunk.cc.
> 
> We might want input from Pepper team members on whether this is a good, or
bad,
> idea though.

https://codereview.chromium.org/16271005/diff/1/ppapi/api/private/ppb_talk_pr...
ppapi/api/private/ppb_talk_private.idl:81: * errors.
On 2013/05/31 22:22:16, Josh Horwich wrote:
> Any restrictions on what thread can call this function? Similarly, will the
> event_callback always be invoked on the Pepper main thread?

Done.

https://codereview.chromium.org/16271005/diff/1/ppapi/api/private/ppb_talk_pr...
ppapi/api/private/ppb_talk_private.idl:90: * Hides the remoting-in-progress UI
and unregisters the event callback.
On 2013/05/31 22:22:16, Josh Horwich wrote:
> Is this synchronous like StartRemoting? Is there any guarantee around when the
> registered event_callback would no longer be called, e.g. only after
> StopRemoting is called?

Done.

https://codereview.chromium.org/16271005/diff/1/ppapi/api/private/ppb_talk_pr...
ppapi/api/private/ppb_talk_private.idl:93: void StopRemoting(
I'd rather not. There is not much to do if StopRemoting fails. Resource related
errors will be logged. (But I don't feel strongly about it, I'll make the change
if you insist).

On 2013/05/31 22:22:16, Josh Horwich wrote:
> Should this return int32_t so we can error in bizarre cases like talk_resource
> being invalid, remoting wasn't started, etc?

[Josh Horwich, 2013-06-04 19:48:37]

https://codereview.chromium.org/16271005/diff/11002/chrome/browser/renderer_h...
File chrome/browser/renderer_host/pepper/pepper_talk_host.cc (right):

https://codereview.chromium.org/16271005/diff/11002/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_talk_host.cc:83: return
PP_ERROR_FAILED;
Curious - we used to return 0 here for unknown messages, now it's
PP_ERROR_FAILED. Was the old code just wrong, or should we continue returning 0?

https://codereview.chromium.org/16271005/diff/11002/ppapi/api/private/ppb_tal...
File ppapi/api/private/ppb_talk_private.idl (right):

https://codereview.chromium.org/16271005/diff/11002/ppapi/api/private/ppb_tal...
ppapi/api/private/ppb_talk_private.idl:82: * Requests permission to the user
using a system modal dialog.
I believe it's clearer to say "Requests permission from the user" because this
is asking for something from them, not to them.

Same comment for line 84.

https://codereview.chromium.org/16271005/diff/11002/ppapi/proxy/talk_resource...
File ppapi/proxy/talk_resource_unittest.cc (right):

https://codereview.chromium.org/16271005/diff/11002/ppapi/proxy/talk_resource...
ppapi/proxy/talk_resource_unittest.cc:108: ASSERT_EQ(1, callback.result());
Please add a test to verify RequestPermission(.., bogus_value, ..) gets an
immediate error and no callback.

https://codereview.chromium.org/16271005/diff/11002/ppapi/proxy/talk_resource...
ppapi/proxy/talk_resource_unittest.cc:109: }
Should add a test to RequestPermission with a bogus permission - ensure we get
no callback and an error.

[dcaiafa, 2013-06-04 22:38:15]

Ptal

https://codereview.chromium.org/16271005/diff/11002/chrome/browser/renderer_h...
File chrome/browser/renderer_host/pepper/pepper_talk_host.cc (right):

https://codereview.chromium.org/16271005/diff/11002/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_talk_host.cc:83: return
PP_ERROR_FAILED;
I'm not an expert, but every other host in this directory returns
PP_ERROR_FAILED (PepperBrokerMessageFilter, PeppperFlashBrowserHost,
PepperFlashDRMHost, etc).

On 2013/06/04 19:48:37, Josh Horwich wrote:
> Curious - we used to return 0 here for unknown messages, now it's
> PP_ERROR_FAILED. Was the old code just wrong, or should we continue returning
0?

https://codereview.chromium.org/16271005/diff/11002/ppapi/api/private/ppb_tal...
File ppapi/api/private/ppb_talk_private.idl (right):

https://codereview.chromium.org/16271005/diff/11002/ppapi/api/private/ppb_tal...
ppapi/api/private/ppb_talk_private.idl:82: * Requests permission to the user
using a system modal dialog.
On 2013/06/04 19:48:37, Josh Horwich wrote:
> I believe it's clearer to say "Requests permission from the user" because this
> is asking for something from them, not to them.
> 
> Same comment for line 84.

Done.

https://codereview.chromium.org/16271005/diff/11002/ppapi/proxy/talk_resource...
File ppapi/proxy/talk_resource_unittest.cc (right):

https://codereview.chromium.org/16271005/diff/11002/ppapi/proxy/talk_resource...
ppapi/proxy/talk_resource_unittest.cc:108: ASSERT_EQ(1, callback.result());
Parameter check is currently done in the host (tested in browser_tests). I could
add additional checks to the proxy, but the thunk will still report the result
asynchronously (unless the callback has the "OPTIONAL_CALLBACK" flag, not
default). I could also add the checks to the thunk, but it would be awkward -
EnterResource<> asserts that SetResult is used on exit, and it's responsible for
reporting results asynchronously.

On 2013/06/04 19:48:37, Josh Horwich wrote:
> Please add a test to verify RequestPermission(.., bogus_value, ..) gets an
> immediate error and no callback.

https://codereview.chromium.org/16271005/diff/11002/ppapi/proxy/talk_resource...
ppapi/proxy/talk_resource_unittest.cc:109: }
See previous comment.

On 2013/06/04 19:48:37, Josh Horwich wrote:
> Should add a test to RequestPermission with a bogus permission - ensure we get
> no callback and an error.

[Josh Horwich, 2013-06-04 22:50:09]

lgtm

https://codereview.chromium.org/16271005/diff/11002/ppapi/proxy/talk_resource...
File ppapi/proxy/talk_resource_unittest.cc (right):

https://codereview.chromium.org/16271005/diff/11002/ppapi/proxy/talk_resource...
ppapi/proxy/talk_resource_unittest.cc:108: ASSERT_EQ(1, callback.result());
Ah, got it, thank you for clarifying. Things are never as easy as they'd seem in
this area.

On 2013/06/04 22:38:15, dcaiafa wrote:
> Parameter check is currently done in the host (tested in browser_tests). I
could
> add additional checks to the proxy, but the thunk will still report the result
> asynchronously (unless the callback has the "OPTIONAL_CALLBACK" flag, not
> default). I could also add the checks to the thunk, but it would be awkward -
> EnterResource<> asserts that SetResult is used on exit, and it's responsible
for
> reporting results asynchronously.
> 
> On 2013/06/04 19:48:37, Josh Horwich wrote:
> > Please add a test to verify RequestPermission(.., bogus_value, ..) gets an
> > immediate error and no callback.
>

https://codereview.chromium.org/16271005/diff/11002/ppapi/proxy/talk_resource...
ppapi/proxy/talk_resource_unittest.cc:109: }
Oops, I had originally entered this into codereview but it didn't appear to take
so I rewrote it in the above comment. Obviously I was mistaken, so please
disregard this one :)

On 2013/06/04 22:38:15, dcaiafa wrote:
> See previous comment.
> 
> On 2013/06/04 19:48:37, Josh Horwich wrote:
> > Should add a test to RequestPermission with a bogus permission - ensure we
get
> > no callback and an error.
>

[dcaiafa, 2013-06-04 23:01:33]

David, can you provide OWNER's approval (and feedback too, of course)

[dcaiafa, 2013-06-04 23:06:17]

Adding Cris for IPC changes.

[dmichael (off chromium), 2013-06-05 02:11:11]

https://codereview.chromium.org/16271005/diff/22001/ppapi/api/private/ppb_tal...
File ppapi/api/private/ppb_talk_private.idl (right):

https://codereview.chromium.org/16271005/diff/22001/ppapi/api/private/ppb_tal...
ppapi/api/private/ppb_talk_private.idl:88: * permission, or 0 if the user
denied.
0 is PP_OK, which usually means success. Since you're changing the interface, it
might be a good time to make it more like everything else. What about PP_OK and
PP_ERROR_NOACCESS? This would match how PPB_MouseLock responds based on user
permission.

https://codereview.chromium.org/16271005/diff/22001/ppapi/api/private/ppb_tal...
ppapi/api/private/ppb_talk_private.idl:102: * This function executes
synchronously. Returns PP_OK if there were no
Do you mean asynchronously?

https://codereview.chromium.org/16271005/diff/22001/ppapi/api/private/ppb_tal...
ppapi/api/private/ppb_talk_private.idl:111: [in] PP_TalkEventCallback
event_callback,
Is there some reason you can't use PP_CompletionCallback with a PP_TalkEvent
out-param? Or better still, just user the error code and pick some reasonable
errors from pp_errors.h.

Or is this going to get called more than once?

[dcaiafa, 2013-06-05 18:49:31]

https://codereview.chromium.org/16271005/diff/22001/ppapi/api/private/ppb_tal...
File ppapi/api/private/ppb_talk_private.idl (right):

https://codereview.chromium.org/16271005/diff/22001/ppapi/api/private/ppb_tal...
ppapi/api/private/ppb_talk_private.idl:88: * permission, or 0 if the user
denied.
It would make things more complicated on both sides.  |GetPermission()| is
implemented as a wrapper around
|RequestPermission(PP_TALKPERMISSION_SCREENCAST)|. Different result codes will
require translation on the proxy. On the plugin side, both interfaces will need
to be supported (at least initially), with a fallback to the 1.0 interface. The
different result codes will complicate the code there too.

These are not a huge deal, but they do increase the code complexity a little
(and the potential for bugs). But I don't feel strongly against it. Let me know
if you still prefer the new codes and I'll do it.

Also, please note that these APIs are temporary and will be deleted (not just
deprecated) when WebRTC comes up. This is as certain as anything can be. The
only reason we're even doing this work (instead of just waiting for WebRTC) is
because we have important business reasons to launch this feature on CrOS right
now.

On 2013/06/05 02:11:11, dmichael wrote:
> 0 is PP_OK, which usually means success. Since you're changing the interface,
it
> might be a good time to make it more like everything else. What about PP_OK
and
> PP_ERROR_NOACCESS? This would match how PPB_MouseLock responds based on user
> permission.

https://codereview.chromium.org/16271005/diff/22001/ppapi/api/private/ppb_tal...
ppapi/api/private/ppb_talk_private.idl:102: * This function executes
synchronously. Returns PP_OK if there were no
I do mean synchronously. This API shows the in-session remoting UI (actionable
notification and item on the system tray menu). It is modeless, and it will be
available until the plugin calls |StopRemoting()| or the resource is deleted.

On 2013/06/05 02:11:11, dmichael wrote:
> Do you mean asynchronously?

https://codereview.chromium.org/16271005/diff/22001/ppapi/api/private/ppb_tal...
ppapi/api/private/ppb_talk_private.idl:111: [in] PP_TalkEventCallback
event_callback,
It can be called more than once, although in practice it probably won't since
the plugin will call |StopRemoting()| when it receives a TERMINATE or ERROR
event. It can also not be called at all (if the session is terminated through
other mechanisms). We have been discussing adding more features to the remoting
session UI like a button to toggle keyboard or mouse-clicks off, in which case
multiple events per session would be a more common scenario (although these will
not be seriously considered before the transition to WebRTC). 
 
On 2013/06/05 02:11:11, dmichael wrote:
> Is there some reason you can't use PP_CompletionCallback with a PP_TalkEvent
> out-param? Or better still, just user the error code and pick some reasonable
> errors from pp_errors.h.
> 
> Or is this going to get called more than once?

[dmichael (off chromium), 2013-06-05 21:02:13]

https://codereview.chromium.org/16271005/diff/22001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/pepper/pepper_talk_host.cc (right):

https://codereview.chromium.org/16271005/diff/22001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_talk_host.cc:81: OnRequestPermission)
Is it intentional that you're not handling StartRemoting or StopRemoting yet? If
so, a TODO and/or bug number would be helpful.

https://codereview.chromium.org/16271005/diff/22001/ppapi/api/private/ppb_tal...
File ppapi/api/private/ppb_talk_private.idl (right):

https://codereview.chromium.org/16271005/diff/22001/ppapi/api/private/ppb_tal...
ppapi/api/private/ppb_talk_private.idl:102: * This function executes
synchronously. Returns PP_OK if there were no
On 2013/06/05 18:49:32, dcaiafa wrote:
> I do mean synchronously. This API shows the in-session remoting UI (actionable
> notification and item on the system tray menu). It is modeless, and it will be
> available until the plugin calls |StopRemoting()| or the resource is deleted.
> 
> On 2013/06/05 02:11:11, dmichael wrote:
> > Do you mean asynchronously?
> 
Can you clarify what you mean by "synchronously", then? I was a bit confused by
it; it's not clear to me what it has to do before returning. It's not actually
blocking and spinning up a nested loop or something, right? You just mean that
when StartRemoting returns, you know that the UI has been displayed?

Also, you appear to be using an asynchronous message. So regardless of what the
host does, it seems like it will behave asynchronously from the plugin's
perspective. Am I misunderstanding something?

https://codereview.chromium.org/16271005/diff/22001/ppapi/proxy/talk_resource.h
File ppapi/proxy/talk_resource.h (right):

https://codereview.chromium.org/16271005/diff/22001/ppapi/proxy/talk_resource...
ppapi/proxy/talk_resource.h:32: virtual void StopRemoting() OVERRIDE;
nit: These could probably all be private.

https://codereview.chromium.org/16271005/diff/22001/ppapi/proxy/talk_resource...
File ppapi/proxy/talk_resource_unittest.cc (right):

https://codereview.chromium.org/16271005/diff/22001/ppapi/proxy/talk_resource...
ppapi/proxy/talk_resource_unittest.cc:47: typedef CallbackBase<int32_t>
CompletionCallback;
Could you name this something else, to avoid confusion with
pp::CompletionCallback? Maybe MockCompletionCallback or FakeCompletionCallback?

https://codereview.chromium.org/16271005/diff/22001/ppapi/proxy/talk_resource...
ppapi/proxy/talk_resource_unittest.cc:48: typedef CallbackBase<PP_TalkEvent>
TalkEventCallback;
same prefix here might also be clearer

[dcaiafa, 2013-06-06 01:24:32]

PTAL. Thanks!

https://codereview.chromium.org/16271005/diff/22001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/pepper/pepper_talk_host.cc (right):

https://codereview.chromium.org/16271005/diff/22001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_talk_host.cc:81: OnRequestPermission)
On 2013/06/05 21:02:13, dmichael wrote:
> Is it intentional that you're not handling StartRemoting or StopRemoting yet?
If
> so, a TODO and/or bug number would be helpful.

Done.

https://codereview.chromium.org/16271005/diff/22001/ppapi/api/private/ppb_tal...
File ppapi/api/private/ppb_talk_private.idl (right):

https://codereview.chromium.org/16271005/diff/22001/ppapi/api/private/ppb_tal...
ppapi/api/private/ppb_talk_private.idl:102: * This function executes
synchronously. Returns PP_OK if there were no
You are right, it is confusing. I decided to make Start/StopRemoting
asynchronous for consistency. The plugin really wants to know if the UI was
successfully shown before starting a remoting session.

On 2013/06/05 21:02:13, dmichael wrote:
> On 2013/06/05 18:49:32, dcaiafa wrote:
> > I do mean synchronously. This API shows the in-session remoting UI
(actionable
> > notification and item on the system tray menu). It is modeless, and it will
be
> > available until the plugin calls |StopRemoting()| or the resource is
deleted.
> > 
> > On 2013/06/05 02:11:11, dmichael wrote:
> > > Do you mean asynchronously?
> > 
> Can you clarify what you mean by "synchronously", then? I was a bit confused
by
> it; it's not clear to me what it has to do before returning. It's not actually
> blocking and spinning up a nested loop or something, right? You just mean that
> when StartRemoting returns, you know that the UI has been displayed?
> 
> Also, you appear to be using an asynchronous message. So regardless of what
the
> host does, it seems like it will behave asynchronously from the plugin's
> perspective. Am I misunderstanding something?

https://codereview.chromium.org/16271005/diff/22001/ppapi/proxy/talk_resource.h
File ppapi/proxy/talk_resource.h (right):

https://codereview.chromium.org/16271005/diff/22001/ppapi/proxy/talk_resource...
ppapi/proxy/talk_resource.h:32: virtual void StopRemoting() OVERRIDE;
On 2013/06/05 21:02:13, dmichael wrote:
> nit: These could probably all be private.

Done.

https://codereview.chromium.org/16271005/diff/22001/ppapi/proxy/talk_resource...
File ppapi/proxy/talk_resource_unittest.cc (right):

https://codereview.chromium.org/16271005/diff/22001/ppapi/proxy/talk_resource...
ppapi/proxy/talk_resource_unittest.cc:47: typedef CallbackBase<int32_t>
CompletionCallback;
On 2013/06/05 21:02:13, dmichael wrote:
> Could you name this something else, to avoid confusion with
> pp::CompletionCallback? Maybe MockCompletionCallback or
FakeCompletionCallback?

Done.

https://codereview.chromium.org/16271005/diff/22001/ppapi/proxy/talk_resource...
ppapi/proxy/talk_resource_unittest.cc:48: typedef CallbackBase<PP_TalkEvent>
TalkEventCallback;
On 2013/06/05 21:02:13, dmichael wrote:
> same prefix here might also be clearer

Done.

[dmichael (off chromium), 2013-06-06 15:57:33]

This is looking pretty close now, thanks! Just a couple of other things.

https://codereview.chromium.org/16271005/diff/36001/ppapi/proxy/talk_resource.cc
File ppapi/proxy/talk_resource.cc (right):

https://codereview.chromium.org/16271005/diff/36001/ppapi/proxy/talk_resource...
ppapi/proxy/talk_resource.cc:47: 
Do you also want to return some other error if remoting is already happening?
(i.e., if event_callback_ is already valid)?

https://codereview.chromium.org/16271005/diff/36001/ppapi/proxy/talk_resource...
ppapi/proxy/talk_resource.cc:56: base::Unretained(this)));
You shouldn't need to use base::Unretained; this is ref-counted. Using
base::Unretained is likely to lead to a use-after-free if the resource is
deleted while a message is pending. (Here and in the other two places you use
base::Unretained)

https://codereview.chromium.org/16271005/diff/36001/ppapi/proxy/talk_resource...
ppapi/proxy/talk_resource.cc:63: 
And here, it may be a good idea to check that either event_callback_ is valid or
start_callback_ is pending, and fail otherwise (i.e., StartRemoting has not been
called).

PP_ERROR_FAILED is kind of a catch-all you could use.

https://codereview.chromium.org/16271005/diff/36001/ppapi/proxy/talk_resource...
File ppapi/proxy/talk_resource_unittest.cc (right):

https://codereview.chromium.org/16271005/diff/36001/ppapi/proxy/talk_resource...
ppapi/proxy/talk_resource_unittest.cc:143:
PpapiPluginMsg_Talk_StartRemotingReply(),
nit: indentation

[dcaiafa, 2013-06-06 16:45:12]

PTAL. Thanks again!

https://codereview.chromium.org/16271005/diff/36001/ppapi/proxy/talk_resource.cc
File ppapi/proxy/talk_resource.cc (right):

https://codereview.chromium.org/16271005/diff/36001/ppapi/proxy/talk_resource...
ppapi/proxy/talk_resource.cc:47: 
On 2013/06/06 15:57:33, dmichael wrote:
> Do you also want to return some other error if remoting is already happening?
> (i.e., if event_callback_ is already valid)?

Done.

https://codereview.chromium.org/16271005/diff/36001/ppapi/proxy/talk_resource...
ppapi/proxy/talk_resource.cc:56: base::Unretained(this)));
According to comments in ppapi/proxy/plugin_resource.h, callbacks shouldn't take
references on |this| because it can potentially lead to a circular reference. It
also says that base::Unretained is safe because the resource is guaranteed to
outlive the callback.

On 2013/06/06 15:57:33, dmichael wrote:
> You shouldn't need to use base::Unretained; this is ref-counted. Using
> base::Unretained is likely to lead to a use-after-free if the resource is
> deleted while a message is pending. (Here and in the other two places you use
> base::Unretained)

https://codereview.chromium.org/16271005/diff/36001/ppapi/proxy/talk_resource...
ppapi/proxy/talk_resource.cc:63: 
On 2013/06/06 15:57:33, dmichael wrote:
> And here, it may be a good idea to check that either event_callback_ is valid
or
> start_callback_ is pending, and fail otherwise (i.e., StartRemoting has not
been
> called).
> 
> PP_ERROR_FAILED is kind of a catch-all you could use.

Done.

https://codereview.chromium.org/16271005/diff/36001/ppapi/proxy/talk_resource...
File ppapi/proxy/talk_resource_unittest.cc (right):

https://codereview.chromium.org/16271005/diff/36001/ppapi/proxy/talk_resource...
ppapi/proxy/talk_resource_unittest.cc:143:
PpapiPluginMsg_Talk_StartRemotingReply(),
On 2013/06/06 15:57:33, dmichael wrote:
> nit: indentation

Done.

[dmichael (off chromium), 2013-06-06 17:14:36]

lgtm

https://codereview.chromium.org/16271005/diff/36001/ppapi/proxy/talk_resource.cc
File ppapi/proxy/talk_resource.cc (right):

https://codereview.chromium.org/16271005/diff/36001/ppapi/proxy/talk_resource...
ppapi/proxy/talk_resource.cc:63: 
On 2013/06/06 16:45:12, dcaiafa wrote:
> On 2013/06/06 15:57:33, dmichael wrote:
> > And here, it may be a good idea to check that either event_callback_ is
valid
> or
> > start_callback_ is pending, and fail otherwise (i.e., StartRemoting has not
> been
> > called).
> > 
> > PP_ERROR_FAILED is kind of a catch-all you could use.
> 
> Done.
Just for the record, you're requiring that StopRemoting can only be called after
StartRemoting has completed. I think you could make it possible to call
StopRemoting while StartRemoting is still pending. But what you have is fine,
too, if it works for the Talk plugin.

[Cris Neckar, 2013-06-06 17:44:08]

https://codereview.chromium.org/16271005/diff/53001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/pepper/pepper_talk_host.cc (right):

https://codereview.chromium.org/16271005/diff/53001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_talk_host.cc:120: NOTIMPLEMENTED();
Please request another IPC audit once these are implemented

https://codereview.chromium.org/16271005/diff/53001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_talk_host.cc:127: NOTIMPLEMENTED();
Ditto

https://codereview.chromium.org/16271005/diff/53001/ppapi/proxy/ppapi_messages.h
File ppapi/proxy/ppapi_messages.h (right):

https://codereview.chromium.org/16271005/diff/53001/ppapi/proxy/ppapi_message...
ppapi/proxy/ppapi_messages.h:97: IPC_ENUM_TRAITS(PP_TalkPermission)
We are asking that people now use the new bounds checked macros for enums passed
over IPC. Please see
https://code.google.com/p/chromium/codesearch#chromium/src/ipc/param_traits_m...

[dcaiafa, 2013-06-06 18:23:32]

PTAL. Thanks!

https://codereview.chromium.org/16271005/diff/53001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/pepper/pepper_talk_host.cc (right):

https://codereview.chromium.org/16271005/diff/53001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_talk_host.cc:120: NOTIMPLEMENTED();
On 2013/06/06 17:44:08, Cris Neckar wrote:
> Please request another IPC audit once these are implemented

Done.

https://codereview.chromium.org/16271005/diff/53001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_talk_host.cc:127: NOTIMPLEMENTED();
On 2013/06/06 17:44:08, Cris Neckar wrote:
> Ditto

Done.

[Cris Neckar, 2013-06-06 18:47:20]

IPC messages LGTM

[commit-bot: I haz the power, 2013-06-06 18:50:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcaiafa@chromium.org/16271005/67001

[commit-bot: I haz the power, 2013-06-06 19:49:19]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[dcaiafa, 2013-06-06 20:01:59]

Adding Raymes for chrome/browser/renderer_host/pepper/... approval.

[raymes1, 2013-06-06 21:38:45]

lgtm for chrome/browser/renderer_host/pepper/*

[commit-bot: I haz the power, 2013-06-06 21:44:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcaiafa@chromium.org/16271005/67001

[commit-bot: I haz the power, 2013-06-06 22:48:47]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[raymes, 2013-06-06 23:36:24]

lgtm

On Thu, Jun 6, 2013 at 1:01 PM,  <dcaiafa@chromium.org> wrote:
> Adding Raymes for chrome/browser/renderer_host/pepper/... approval.
>
> https://codereview.chromium.org/16271005/

[commit-bot: I haz the power, 2013-06-06 23:39:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcaiafa@chromium.org/16271005/67001

[commit-bot: I haz the power, 2013-06-07 00:36:38]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-06-07 16:17:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dcaiafa@chromium.org/16271005/97001

[commit-bot: I haz the power, 2013-06-07 19:02:13]

Change committed as 204889