Adding a confirmation dialog for It2Me on Linux

Adding a confirmation dialog for It2Me on Linux

This change adds a confirmation dialog for the It2Me Linux host.  This
dialog will be displayed when an incoming It2Me connection and will wait
up to 60 seconds for the user to accept/reject it.  If the user accepts
the connection then we will complete the conneciton process, otherwise
the incoming request will be rejected.

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

[joedow, 2016-09-13 23:14:34]

Patchset #2 (id:20001) has been deleted

[joedow, 2016-09-13 23:16:16]

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

[commit-bot: I haz the power, 2016-09-13 23:16:53]

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-14 00:03:13]

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

[joedow, 2016-09-14 00:03:14]

PTAL!

[commit-bot: I haz the power, 2016-09-14 00:08:04]

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

[commit-bot: I haz the power, 2016-09-14 00:08:05]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Sergey Ulanov, 2016-09-14 00:25:11]

lgtm when my comments are addressed.

https://codereview.chromium.org/2343443002/diff/1/remoting/host/it2me/it2me_c...
File remoting/host/it2me/it2me_confirmation_dialog_linux.cc (right):

https://codereview.chromium.org/2343443002/diff/1/remoting/host/it2me/it2me_c...
remoting/host/it2me/it2me_confirmation_dialog_linux.cc:72:
DCHECK(result_callback_.is_null());
nit: don't need .is_null(). Callbacks can be implicitly cast to bool.

https://codereview.chromium.org/2343443002/diff/1/remoting/host/it2me/it2me_c...
remoting/host/it2me/it2me_confirmation_dialog_linux.cc:92: if
(!result_callback_.is_null()) {
nit: don't need is_null()

https://codereview.chromium.org/2343443002/diff/1/remoting/host/it2me/it2me_c...
remoting/host/it2me/it2me_confirmation_dialog_linux.cc:128:
base::UTF8ToUTF16(remote_user_email));
I don't think you need base::UTF8ToUTF16(). There is an implicit MessageArg
constructor that takes UTF8 strings:
https://codesearch.chromium.org/chromium/src/base/i18n/message_formatter.cc?r...

https://codereview.chromium.org/2343443002/diff/1/remoting/host/it2me/it2me_c...
remoting/host/it2me/it2me_confirmation_dialog_linux.cc:144: if (response_id ==
GTK_RESPONSE_OK) {
Can write as a single line: 

base::ResetAndReturn(&result_callback_).Run(
  (response_id == GTK_RESPONSE_OK) ? Result::OK : Result::Cancel);

https://codereview.chromium.org/2343443002/diff/1/remoting/host/it2me/it2me_c...
remoting/host/it2me/it2me_confirmation_dialog_linux.cc:150: Hide();
It's best to call Hide() before calling the result callback in case the callback
destroys the dialog.

[joedow, 2016-09-14 02:49:24]

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

[commit-bot: I haz the power, 2016-09-14 02:49:45]

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-14 03:44:30]

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

[commit-bot: I haz the power, 2016-09-14 03:44:31]

Dry run: This issue passed the CQ dry run.

[joedow, 2016-09-14 13:28:50]

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

[joedow, 2016-09-14 13:28:51]

dpranke@, could you take a look at the gtk build file change?

Thanks!
Joe

https://codereview.chromium.org/2343443002/diff/1/remoting/host/it2me/it2me_c...
File remoting/host/it2me/it2me_confirmation_dialog_linux.cc (right):

https://codereview.chromium.org/2343443002/diff/1/remoting/host/it2me/it2me_c...
remoting/host/it2me/it2me_confirmation_dialog_linux.cc:72:
DCHECK(result_callback_.is_null());
On 2016/09/14 00:25:11, Sergey Ulanov wrote:
> nit: don't need .is_null(). Callbacks can be implicitly cast to bool.

Done.

https://codereview.chromium.org/2343443002/diff/1/remoting/host/it2me/it2me_c...
remoting/host/it2me/it2me_confirmation_dialog_linux.cc:92: if
(!result_callback_.is_null()) {
On 2016/09/14 00:25:10, Sergey Ulanov wrote:
> nit: don't need is_null()

Done.

https://codereview.chromium.org/2343443002/diff/1/remoting/host/it2me/it2me_c...
remoting/host/it2me/it2me_confirmation_dialog_linux.cc:128:
base::UTF8ToUTF16(remote_user_email));
On 2016/09/14 00:25:11, Sergey Ulanov wrote:
> I don't think you need base::UTF8ToUTF16(). There is an implicit MessageArg
> constructor that takes UTF8 strings:
>
https://codesearch.chromium.org/chromium/src/base/i18n/message_formatter.cc?r...

Done.

https://codereview.chromium.org/2343443002/diff/1/remoting/host/it2me/it2me_c...
remoting/host/it2me/it2me_confirmation_dialog_linux.cc:144: if (response_id ==
GTK_RESPONSE_OK) {
On 2016/09/14 00:25:11, Sergey Ulanov wrote:
> Can write as a single line: 
> 
> base::ResetAndReturn(&result_callback_).Run(
>   (response_id == GTK_RESPONSE_OK) ? Result::OK : Result::Cancel);

Done.

https://codereview.chromium.org/2343443002/diff/1/remoting/host/it2me/it2me_c...
remoting/host/it2me/it2me_confirmation_dialog_linux.cc:150: Hide();
On 2016/09/14 00:25:11, Sergey Ulanov wrote:
> It's best to call Hide() before calling the result callback in case the
callback
> destroys the dialog.

Done.

[Dirk Pranke, 2016-09-14 17:25:17]

lgtm

[joedow, 2016-09-14 18:08:10]

The CQ bit was checked by joedow@chromium.org

[joedow, 2016-09-14 18:08:10]

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/2343443002/#ps40001
(title: "Addressing CR Feedback")

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

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-14 18:14:14]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-09-14 18:17:28]

Description was changed from

==========
Adding a confirmation dialog for It2Me on Linux

This change adds a confirmation dialog for the It2Me Linux host.  This
dialog will be displayed when an incoming It2Me connection and will wait
up to 60 seconds for the user to accept/reject it.  If the user accepts
the connection then we will complete the conneciton process, otherwise
the incoming request will be rejected.

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

to

==========
Adding a confirmation dialog for It2Me on Linux

This change adds a confirmation dialog for the It2Me Linux host.  This
dialog will be displayed when an incoming It2Me connection and will wait
up to 60 seconds for the user to accept/reject it.  If the user accepts
the connection then we will complete the conneciton process, otherwise
the incoming request will be rejected.

BUG=645540

Committed: https://crrev.com/6243fba501529b041abd75c9554157d70bd14d74
Cr-Commit-Position: refs/heads/master@{#418611}
==========

[commit-bot: I haz the power, 2016-09-14 18:17:29]

Patchset 2 (id:??) landed as
https://crrev.com/6243fba501529b041abd75c9554157d70bd14d74
Cr-Commit-Position: refs/heads/master@{#418611}