Implement supporting UI HRD-on-ChromeOS

Implement supporting UI HRD-on-ChromeOS

Implement confirmation and continue-confirmation dialogs, and
session-in-progress UI for Hangouts Remote Desktop on ChromeOS.

BUG=237847
TESTED=HRD end-2-end and screencast on ChromeOS

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

[dcaiafa, 2013-06-17 19:11:28]

Note: will request IPC audit after LGTM

[Josh Horwich, 2013-06-17 19:36:05]

Is it possible to add some testing here, particularly to the Start and Stop of
remoting? It appears there's a few differing sequences of events that the code
is covering and it would be good to know we're not leaking/UAF'ing in any of the
cases.

I suppose the permission messages are hard to test without mocking out
chrome::ShowMessageBox and we don't tend to use mocks AFAIK.

https://codereview.chromium.org/17043006/diff/1001/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/17043006/diff/1001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:15035: +        A participant in this hangout
has offered to help you by controlling your computer. If you accept:
(Here and elsewhere) do we refer to a hangout with a capital H all the time, or
is it OK to use the lowercase h here? (I personally have no idea)

https://codereview.chromium.org/17043006/diff/1001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/pepper/pepper_talk_host.cc (right):

https://codereview.chromium.org/17043006/diff/1001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_talk_host.cc:147:
base::Bind(&StopRemotingOnUIThread));
Doesn't the fact that we're posting a task to a different thread as this is
being destroyed indicate that the method (StopRemotingOnUIThread) be static?
As-is, the method isn't virtual and we aren't dereferencing |this| so I suppose
we're ok as-is but that feels brittle.

https://codereview.chromium.org/17043006/diff/1001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_talk_host.cc:168: if (permission < 0
|| permission >= PP_TALKPERMISSION_NUM_PERMISSIONS)
Should this be 'if (permission < PP_TALKPERMISSION_SCREENCAST || ...' ?
(functionally identical, just more readable IMO)

[dcaiafa, 2013-06-17 20:34:24]

In regards to testing: I did look around but couldn't find a reasonable way to
unittest this. The original host didn't have unittests, neither does any of the
other hosts in the same directory - so I have nothing to use as a template.

I'm going to try to add some tests in browser_test, next - also not trivial, but
at least I have something to base on.

https://codereview.chromium.org/17043006/diff/1001/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/17043006/diff/1001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:15035: +        A participant in this hangout
has offered to help you by controlling your computer. If you accept:
I'm not sure either, but this one specifically is a copy & paste of the text
used in the other platforms (Win, Mac & Linux). But there will still be a
ChromeOS specific review of all strings this week.

On 2013/06/17 19:36:05, Josh Horwich wrote:
> (Here and elsewhere) do we refer to a hangout with a capital H all the time,
or
> is it OK to use the lowercase h here? (I personally have no idea)

https://codereview.chromium.org/17043006/diff/1001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/pepper/pepper_talk_host.cc (right):

https://codereview.chromium.org/17043006/diff/1001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_talk_host.cc:147:
base::Bind(&StopRemotingOnUIThread));
StopRemotingOnUIThread is a function, not a method. 

On 2013/06/17 19:36:05, Josh Horwich wrote:
> Doesn't the fact that we're posting a task to a different thread as this is
> being destroyed indicate that the method (StopRemotingOnUIThread) be static?
> As-is, the method isn't virtual and we aren't dereferencing |this| so I
suppose
> we're ok as-is but that feels brittle.

https://codereview.chromium.org/17043006/diff/1001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_talk_host.cc:168: if (permission < 0
|| permission >= PP_TALKPERMISSION_NUM_PERMISSIONS)
On 2013/06/17 19:36:05, Josh Horwich wrote:
> Should this be 'if (permission < PP_TALKPERMISSION_SCREENCAST || ...' ?
> (functionally identical, just more readable IMO)

Done.

[Josh Horwich, 2013-06-17 20:56:43]

https://codereview.chromium.org/17043006/diff/1001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/pepper/pepper_talk_host.cc (right):

https://codereview.chromium.org/17043006/diff/1001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/pepper/pepper_talk_host.cc:147:
base::Bind(&StopRemotingOnUIThread));
Ah, sorry, my mistake. That certainly addresses my (misplaced) concern. :)

On 2013/06/17 20:34:25, dcaiafa wrote:
> StopRemotingOnUIThread is a function, not a method. 
> 
> On 2013/06/17 19:36:05, Josh Horwich wrote:
> > Doesn't the fact that we're posting a task to a different thread as this is
> > being destroyed indicate that the method (StopRemotingOnUIThread) be static?
> > As-is, the method isn't virtual and we aren't dereferencing |this| so I
> suppose
> > we're ok as-is but that feels brittle.
>

[Josh Horwich, 2013-06-17 21:59:41]

lgtm

[dcaiafa, 2013-06-17 22:16:09]

Adding Cris for IPC audit.

[raymes, 2013-06-18 00:30:51]

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

https://codereview.chromium.org/17043006/diff/16001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_talk_host.cc:45: 
nit: blank line

https://codereview.chromium.org/17043006/diff/16001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_talk_host.cc:121:
ash::Shell::GetInstance()->system_tray_notifier()->NotifyScreenShareStop();
Should you be checking any of these things are still alive or are their
lifetimes guaranteed to extend beyond the message loops lifetime?

https://codereview.chromium.org/17043006/diff/16001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_talk_host.cc:245: remoting_started_ =
false;
Is it possible for this to be called before OnStartRemotingCompleted is called?
I'm imagining a case where remoting is started and stopped very quickly and just
want to make sure you've considered the chance of a race here.

https://codereview.chromium.org/17043006/diff/16001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/pepper/pepper_talk_host.h (right):

https://codereview.chromium.org/17043006/diff/16001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_talk_host.h:38: int32_t
OnRequestPermission(ppapi::host::HostMessageContext* context,
nit: usually we have the message handlers grouped together.

[dcaiafa, 2013-06-18 16:36:40]

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

https://codereview.chromium.org/17043006/diff/16001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_talk_host.cc:45: 
On 2013/06/18 00:30:51, raymes wrote:
> nit: blank line

Done.

https://codereview.chromium.org/17043006/diff/16001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_talk_host.cc:121:
ash::Shell::GetInstance()->system_tray_notifier()->NotifyScreenShareStop();
Good point. RequestPermission and StartRemoting are probably safe because they
check the RVP first. In StopRemoting, a quick code-search indicates that Shell
might be deleted before the UI thread is shutdown, so a check is probably good.
system_tray_notifier() is guaranteed to have a value if Shell is valid.

On 2013/06/18 00:30:51, raymes wrote:
> Should you be checking any of these things are still alive or are their
> lifetimes guaranteed to extend beyond the message loops lifetime?

https://codereview.chromium.org/17043006/diff/16001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_talk_host.cc:245: remoting_started_ =
false;
Done.

On 2013/06/18 00:30:51, raymes wrote:
> Is it possible for this to be called before OnStartRemotingCompleted is
called?
> I'm imagining a case where remoting is started and stopped very quickly and
just
> want to make sure you've considered the chance of a race here.

https://codereview.chromium.org/17043006/diff/16001/chrome/browser/renderer_h...
File chrome/browser/renderer_host/pepper/pepper_talk_host.h (right):

https://codereview.chromium.org/17043006/diff/16001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_talk_host.h:38: int32_t
OnRequestPermission(ppapi::host::HostMessageContext* context,
On 2013/06/18 00:30:51, raymes wrote:
> nit: usually we have the message handlers grouped together.

Done.

[raymes, 2013-06-18 19:44:50]

lgtm

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

https://codereview.chromium.org/17043006/diff/22001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_talk_host.cc:189: void
PepperTalkHost::OnRequestPermissionCompleted(
nit: The order of functions in the implementation should match the order in the
header.

https://codereview.chromium.org/17043006/diff/22001/chrome/browser/renderer_h...
chrome/browser/renderer_host/pepper/pepper_talk_host.cc:225: if
(reply.params.result() != PP_OK) {
nit: no need for { for single-line if

[Cris Neckar, 2013-06-18 19:57:26]

ipc message handlers lgtm

[commit-bot: I haz the power, 2013-06-18 21:43:26]

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

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

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
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-19 01:04:44]

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

[commit-bot: I haz the power, 2013-06-19 04:08:37]

Retried try job too often on win_rel for step(s) chrome_frame_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-06-19 04:59:02]

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

[commit-bot: I haz the power, 2013-06-20 03:11:15]

Change committed as 207296