Moving It2Me confirmation prompt into the Validation callback flow

Moving It2Me confirmation prompt into the Validation callback flow

This change moves the confirmation dialog prompt used in It2Me from
the beginning of the connection flow to the point where we know
the client JID.  This change allows the user to have more control
over who connects to their session (they can effectively 'see who
is calling' and reject it if it isn't an address they recognize).

BUG=645540
Committed: https://crrev.com/b1b8dcaf0e18df7c78788e6ae80ed3fe9e060d33
Cr-Commit-Position: refs/heads/master@{#417960}

[joedow, 2016-09-06 23:32:52]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-06 23:33:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-06 23:51:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-06 23:51:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[joedow, 2016-09-08 20:56:35]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 20:57:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joedow, 2016-09-08 21:15:45]

Patchset #2 (id:20001) has been deleted

[joedow, 2016-09-08 21:24:08]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 21:25:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 21:46:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 21:46:47]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[joedow, 2016-09-08 23:30:50]

Patchset #3 (id:60001) has been deleted

[joedow, 2016-09-08 23:32:33]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-08 23:33:15]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-08 23:44:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-08 23:44:23]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[joedow, 2016-09-09 01:47:23]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 01:47:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joedow, 2016-09-09 02:05:09]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 02:05:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joedow, 2016-09-09 02:24:06]

Patchset #3 (id:80001) has been deleted

[joedow, 2016-09-09 02:24:23]

Patchset #3 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-09-09 03:27:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 03:27:30]

Dry run: This issue passed the CQ dry run.

[joedow, 2016-09-09 15:33:33]

joedow@chromium.org changed reviewers:
+ sergeyu@chromium.org

[joedow, 2016-09-09 15:33:33]

PTAL!

[joedow, 2016-09-09 17:42:50]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-09 17:43:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 18:39:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-09 18:39:09]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2016-09-09 18:46:57]

lgtm when my comments are addressed

https://codereview.chromium.org/2310303002/diff/140001/remoting/host/it2me/it...
File remoting/host/it2me/it2me_confirmation_dialog_chromeos.cc (right):

https://codereview.chromium.org/2310303002/diff/140001/remoting/host/it2me/it...
remoting/host/it2me/it2me_confirmation_dialog_chromeos.cc:49: if
(!remote_user_email.empty()) {
maybe add a TODO to remove the case when email is not specified?

https://codereview.chromium.org/2310303002/diff/140001/remoting/host/it2me/it...
File remoting/host/it2me/it2me_confirmation_dialog_proxy_unittest.cc (right):

https://codereview.chromium.org/2310303002/diff/140001/remoting/host/it2me/it...
remoting/host/it2me/it2me_confirmation_dialog_proxy_unittest.cc:44:
EXPECT_TRUE(task_runner_->BelongsToCurrentThread());
verify that we get correct user email here.

https://codereview.chromium.org/2310303002/diff/140001/remoting/host/it2me/it...
File remoting/host/it2me/it2me_host.cc (right):

https://codereview.chromium.org/2310303002/diff/140001/remoting/host/it2me/it...
remoting/host/it2me/it2me_host.cc:515: if (!confirmation_dialog) {
Add a comment that this is only temporary until we have the dialog implemented
on all platforms and add a TODO to fix this?

[Sergey Ulanov, 2016-09-09 18:50:40]

one more comment

https://codereview.chromium.org/2310303002/diff/140001/remoting/host/it2me/it...
File remoting/host/it2me/it2me_host.cc (right):

https://codereview.chromium.org/2310303002/diff/140001/remoting/host/it2me/it...
remoting/host/it2me/it2me_host.cc:102: FROM_HERE,
base::Bind(&It2MeHost::SetState, this, kStarting, ""));
Call SetState() in ReadPolicyAndConnect()?

[joedow, 2016-09-10 02:32:27]

Description was changed from

==========
Moving It2Me confirmation prompt into the Validation callback flow

This change moves the confirmation dialog prompt used in It2Me from
the beginning of the connection flow to the point where we know
the client JID.  This change allows the user to have more control
over who connects to their session (they can effectively 'see who
is calling' and reject it if it isn't an address they recognize).

BUG=617185
==========

to

==========
Moving It2Me confirmation prompt into the Validation callback flow

This change moves the confirmation dialog prompt used in It2Me from
the beginning of the connection flow to the point where we know
the client JID.  This change allows the user to have more control
over who connects to their session (they can effectively 'see who
is calling' and reject it if it isn't an address they recognize).

BUG=645540
==========

[joedow, 2016-09-10 02:33:27]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-10 02:33:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joedow, 2016-09-10 02:50:37]

Thanks!

https://codereview.chromium.org/2310303002/diff/140001/remoting/host/it2me/it...
File remoting/host/it2me/it2me_confirmation_dialog_chromeos.cc (right):

https://codereview.chromium.org/2310303002/diff/140001/remoting/host/it2me/it...
remoting/host/it2me/it2me_confirmation_dialog_chromeos.cc:49: if
(!remote_user_email.empty()) {
On 2016/09/09 18:46:57, Sergey Ulanov wrote:
> maybe add a TODO to remove the case when email is not specified?

I added this code in case we did not receive an email address (somehow) but
instead I will make it a requirement to have a non-empty string.  That will
simplify things a bit.

https://codereview.chromium.org/2310303002/diff/140001/remoting/host/it2me/it...
File remoting/host/it2me/it2me_confirmation_dialog_proxy_unittest.cc (right):

https://codereview.chromium.org/2310303002/diff/140001/remoting/host/it2me/it...
remoting/host/it2me/it2me_confirmation_dialog_proxy_unittest.cc:44:
EXPECT_TRUE(task_runner_->BelongsToCurrentThread());
On 2016/09/09 18:46:57, Sergey Ulanov wrote:
> verify that we get correct user email here.

Done.

https://codereview.chromium.org/2310303002/diff/140001/remoting/host/it2me/it...
File remoting/host/it2me/it2me_host.cc (right):

https://codereview.chromium.org/2310303002/diff/140001/remoting/host/it2me/it...
remoting/host/it2me/it2me_host.cc:102: FROM_HERE,
base::Bind(&It2MeHost::SetState, this, kStarting, ""));
On 2016/09/09 18:50:40, Sergey Ulanov wrote:
> Call SetState() in ReadPolicyAndConnect()?

Done.

https://codereview.chromium.org/2310303002/diff/140001/remoting/host/it2me/it...
remoting/host/it2me/it2me_host.cc:515: if (!confirmation_dialog) {
On 2016/09/09 18:46:57, Sergey Ulanov wrote:
> Add a comment that this is only temporary until we have the dialog implemented
> on all platforms and add a TODO to fix this?

Done.

[commit-bot: I haz the power, 2016-09-10 03:10:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-10 03:10:42]

Dry run: This issue passed the CQ dry run.

[joedow, 2016-09-12 16:10:11]

The CQ bit was checked by joedow@chromium.org

[joedow, 2016-09-12 16:10:11]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2310303002/#ps160001
(title: "Addressing CR Feedback")

[commit-bot: I haz the power, 2016-09-12 16:10:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-12 17:03:43]

Committed patchset #5 (id:160001)

[commit-bot: I haz the power, 2016-09-12 17:05:32]

Description was changed from

==========
Moving It2Me confirmation prompt into the Validation callback flow

This change moves the confirmation dialog prompt used in It2Me from
the beginning of the connection flow to the point where we know
the client JID.  This change allows the user to have more control
over who connects to their session (they can effectively 'see who
is calling' and reject it if it isn't an address they recognize).

BUG=645540
==========

to

==========
Moving It2Me confirmation prompt into the Validation callback flow

This change moves the confirmation dialog prompt used in It2Me from
the beginning of the connection flow to the point where we know
the client JID.  This change allows the user to have more control
over who connects to their session (they can effectively 'see who
is calling' and reject it if it isn't an address they recognize).

BUG=645540

Committed: https://crrev.com/b1b8dcaf0e18df7c78788e6ae80ed3fe9e060d33
Cr-Commit-Position: refs/heads/master@{#417960}
==========

[commit-bot: I haz the power, 2016-09-12 17:05:33]

Patchset 5 (id:??) landed as
https://crrev.com/b1b8dcaf0e18df7c78788e6ae80ed3fe9e060d33
Cr-Commit-Position: refs/heads/master@{#417960}