It2Me Confirmation Dialog

It2Me Confirmation Dialog (ChromeOS)

This CL adds support for a native confirmation dialog shown after the
user presses "Share" on the webapp but before an access code is
generated. It also includes a ChromeOS (Aura) implementation.
Implementation for other platforms will soon follow.

More details
============

It2MeConfirmationDialog is an interface for the confirmation dialog.
Implementations can assume they will be called only on the UI thread.

It2MeConfirmationDialogFactory is used to create platform specific
implementations of the dialog.

It2MeConfirmationDialogChromeOS is the dialog's ChromeOS implementation.

It2MeConfirmationDialogProxy is a helper class that allows the caller
(in this case, It2MeHost) to use the dialog outside of the UI thread.

BUG=444216
Committed: https://crrev.com/3a5e72899629e0c8d254109707e747742686fbca
Cr-Commit-Position: refs/heads/master@{#310149}

[dcaiafa, 2014-12-19 01:51:42]

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

[dcaiafa, 2014-12-19 01:51:42]

Please take a look at the general approach, but PLEASE ignore the details for
now. I haven't spent any time verifying the code, adding comments, tests, etc. I
would like confirmation that I'm on the right track before I make this
production quality.

Thanks!

[Sergey Ulanov, 2014-12-19 03:16:19]

approach in general looks good.

https://codereview.chromium.org/816903002/diff/1/remoting/host/it2me/it2me_co...
File remoting/host/it2me/it2me_confirmation_dialog.cc (right):

https://codereview.chromium.org/816903002/diff/1/remoting/host/it2me/it2me_co...
remoting/host/it2me/it2me_confirmation_dialog.cc:17: return
scoped_ptr<It2MeConfirmationDialog>();
return nullptr;

https://codereview.chromium.org/816903002/diff/1/remoting/host/it2me/it2me_co...
File remoting/host/it2me/it2me_confirmation_dialog.h (right):

https://codereview.chromium.org/816903002/diff/1/remoting/host/it2me/it2me_co...
remoting/host/it2me/it2me_confirmation_dialog.h:18: enum class Result {
maybe just use bool? we are unlikely to add other values here and the code
should be readable without this enum. But feel free to keep it if you disagree.

https://codereview.chromium.org/816903002/diff/1/remoting/host/it2me/it2me_co...
remoting/host/it2me/it2me_confirmation_dialog.h:25: virtual
~It2MeConfirmationDialog();
inline distructor for virtual interfaces.

https://codereview.chromium.org/816903002/diff/1/remoting/host/it2me/it2me_co...
File remoting/host/it2me/it2me_confirmation_dialog_proxy.h (right):

https://codereview.chromium.org/816903002/diff/1/remoting/host/it2me/it2me_co...
remoting/host/it2me/it2me_confirmation_dialog_proxy.h:17: class
It2MeConfirmationDialogProxy {
should this implement It2MeConfirmationDialog?

https://codereview.chromium.org/816903002/diff/1/remoting/host/it2me/it2me_co...
remoting/host/it2me/it2me_confirmation_dialog_proxy.h:29: class Core : public
base::RefCountedThreadSafe<Core> {
don't need this to be ref-counted. Use DeleteSoon() to delete the Core on the UI
thread and the proxy is deleted and a weak_ptr to post tasks from core to the
proxy.

https://codereview.chromium.org/816903002/diff/1/remoting/host/it2me/it2me_co...
remoting/host/it2me/it2me_confirmation_dialog_proxy.h:29: class Core : public
base::RefCountedThreadSafe<Core> {
this can be moved to the .cc file.

https://codereview.chromium.org/816903002/diff/1/remoting/host/it2me/it2me_ho...
File remoting/host/it2me/it2me_host.cc (right):

https://codereview.chromium.org/816903002/diff/1/remoting/host/it2me/it2me_ho...
remoting/host/it2me/it2me_host.cc:159: case It2MeConfirmationDialog::Result::OK:
I think you don't need ::Result here

[dcaiafa, 2014-12-19 23:59:43]

Patchset #2 (id:20001) has been deleted

[dcaiafa, 2014-12-20 00:05:54]

Patchset #2 (id:40001) has been deleted

[dcaiafa, 2014-12-20 00:17:20]

Patchset #2 (id:60001) has been deleted

[dcaiafa, 2014-12-20 00:21:21]

Patchset #2 (id:80001) has been deleted

[dcaiafa, 2014-12-20 00:22:00]

PTAL

https://codereview.chromium.org/816903002/diff/1/remoting/host/it2me/it2me_co...
File remoting/host/it2me/it2me_confirmation_dialog.cc (right):

https://codereview.chromium.org/816903002/diff/1/remoting/host/it2me/it2me_co...
remoting/host/it2me/it2me_confirmation_dialog.cc:17: return
scoped_ptr<It2MeConfirmationDialog>();
On 2014/12/19 03:16:19, Sergey Ulanov wrote:
> return nullptr;

Done.

https://codereview.chromium.org/816903002/diff/1/remoting/host/it2me/it2me_co...
File remoting/host/it2me/it2me_confirmation_dialog.h (right):

https://codereview.chromium.org/816903002/diff/1/remoting/host/it2me/it2me_co...
remoting/host/it2me/it2me_confirmation_dialog.h:18: enum class Result {
On 2014/12/19 03:16:19, Sergey Ulanov wrote:
> maybe just use bool? we are unlikely to add other values here and the code
> should be readable without this enum. But feel free to keep it if you
disagree.

I generally dislike boolean parameters (i.e. ReportResult(false) <
ReportResult(Result::CANCEL)), so I'd like to keep this.

https://codereview.chromium.org/816903002/diff/1/remoting/host/it2me/it2me_co...
remoting/host/it2me/it2me_confirmation_dialog.h:25: virtual
~It2MeConfirmationDialog();
On 2014/12/19 03:16:19, Sergey Ulanov wrote:
> inline distructor for virtual interfaces.

Done.

https://codereview.chromium.org/816903002/diff/1/remoting/host/it2me/it2me_co...
File remoting/host/it2me/it2me_confirmation_dialog_proxy.h (right):

https://codereview.chromium.org/816903002/diff/1/remoting/host/it2me/it2me_co...
remoting/host/it2me/it2me_confirmation_dialog_proxy.h:17: class
It2MeConfirmationDialogProxy {
On 2014/12/19 03:16:19, Sergey Ulanov wrote:
> should this implement It2MeConfirmationDialog?

Done.

https://codereview.chromium.org/816903002/diff/1/remoting/host/it2me/it2me_co...
remoting/host/it2me/it2me_confirmation_dialog_proxy.h:29: class Core : public
base::RefCountedThreadSafe<Core> {
On 2014/12/19 03:16:19, Sergey Ulanov wrote:
> don't need this to be ref-counted. Use DeleteSoon() to delete the Core on the
UI
> thread and the proxy is deleted and a weak_ptr to post tasks from core to the
> proxy.

Done.

https://codereview.chromium.org/816903002/diff/1/remoting/host/it2me/it2me_co...
remoting/host/it2me/it2me_confirmation_dialog_proxy.h:29: class Core : public
base::RefCountedThreadSafe<Core> {
On 2014/12/19 03:16:19, Sergey Ulanov wrote:
> this can be moved to the .cc file.

Done.

https://codereview.chromium.org/816903002/diff/1/remoting/host/it2me/it2me_ho...
File remoting/host/it2me/it2me_host.cc (right):

https://codereview.chromium.org/816903002/diff/1/remoting/host/it2me/it2me_ho...
remoting/host/it2me/it2me_host.cc:159: case It2MeConfirmationDialog::Result::OK:
On 2014/12/19 03:16:19, Sergey Ulanov wrote:
> I think you don't need ::Result here

It's needed because it's an "enum class" instead of just enum.

[Sergey Ulanov, 2014-12-22 19:29:09]

https://codereview.chromium.org/816903002/diff/1/remoting/host/it2me/it2me_ho...
File remoting/host/it2me/it2me_host.cc (right):

https://codereview.chromium.org/816903002/diff/1/remoting/host/it2me/it2me_ho...
remoting/host/it2me/it2me_host.cc:159: case It2MeConfirmationDialog::Result::OK:
On 2014/12/20 00:22:00, dcaiafa wrote:
> On 2014/12/19 03:16:19, Sergey Ulanov wrote:
> > I think you don't need ::Result here
> 
> It's needed because it's an "enum class" instead of just enum.

oh, yay for C++11!

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_confirmation_dialog_chromeos.cc (right):

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_confirmation_dialog_chromeos.cc:44: base::string16(),
Use IDS_MODE_IT2ME?

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_confirmation_dialog_proxy.cc (right):

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_confirmation_dialog_proxy.cc:70:
weak_factory_->GetWeakPtr()));
It should be safe to use base::Unretained() here. Core owns |dialog_| and the
|dialog_| shouldn't call the callback after it's destroyed.

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_confirmation_dialog_proxy.h (right):

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_confirmation_dialog_proxy.h:27:
scoped_refptr<base::SingleThreadTaskRunner> caller_task_runner,
Maybe use ThreadTaskRunnerHandle::Get() instead of passing this parameter here?

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_host.cc (right):

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_host.cc:41: scoped_ptr<It2MeConfirmationDialogFactory>
confirmation_dialog_factory,
I'm not sure why you need this parameter. Why not just call
It2MeConfirmationDialog::Create() in  It2MeHost::Confirm() and then you can
remove the factory as well.

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_host.cc:134: void It2MeHost::Confirm() {
Maybe call this ShowConfirmPrompt()?

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_host.cc:175: DCHECK(state_ == kStarting);
DCHECK_EQ please

[kelvinp, 2014-12-29 22:27:41]

kelvinp@chromium.org changed reviewers:
+ kelvinp@chromium.org

[kelvinp, 2014-12-29 22:27:42]

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_confirmation_dialog.h (right):

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_confirmation_dialog.h:36: class
It2MeConfirmationDialogFactory {
Do we need a factory class, or can we simply do a static method?

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_confirmation_dialog_chromeos.cc (right):

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_confirmation_dialog_chromeos.cc:61: //
It2MeConfirmationDialogFactory =============================================
This kind of style is not used elsewhere in our codebase.  Consider removing it
for consistency?

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_confirmation_dialog_proxy.cc (right):

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_confirmation_dialog_proxy.cc:33: friend
base::RefCountedThreadSafe<Core>;
This is no longer needed as Core is no longer ref-counted.

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_host.cc (right):

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_host.cc:41: scoped_ptr<It2MeConfirmationDialogFactory>
confirmation_dialog_factory,
On 2014/12/22 19:29:09, Sergey Ulanov wrote:
> I'm not sure why you need this parameter. Why not just call
> It2MeConfirmationDialog::Create() in  It2MeHost::Confirm() and then you can
> remove the factory as well.

+1

https://codereview.chromium.org/816903002/diff/100001/remoting/resources/remo...
File remoting/resources/remoting_strings.grd (right):

https://codereview.chromium.org/816903002/diff/100001/remoting/resources/remo...
remoting/resources/remoting_strings.grd:945: Would like to share this computer
for another user to see and control?
s/Would like/Would you like

[dcaiafa, 2015-01-05 19:39:27]

pTaL

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_confirmation_dialog.h (right):

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_confirmation_dialog.h:36: class
It2MeConfirmationDialogFactory {
On 2014/12/29 22:27:42, kelvinp wrote:
> Do we need a factory class, or can we simply do a static method?

The factory allows the dependency-injection of It2MeConfirmationDialog into
It2MeHost. Alas, there is no unit-test for It2MeHost, as I found out when I
started writing tests for this. But we should write one, and if so, this would
be a required step.

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_confirmation_dialog_chromeos.cc (right):

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_confirmation_dialog_chromeos.cc:44: base::string16(),
On 2014/12/22 19:29:08, Sergey Ulanov wrote:
> Use IDS_MODE_IT2ME?

Done.

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_confirmation_dialog_chromeos.cc:61: //
It2MeConfirmationDialogFactory =============================================
On 2014/12/29 22:27:42, kelvinp wrote:
> This kind of style is not used elsewhere in our codebase.  Consider removing
it
> for consistency?

Done.

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_confirmation_dialog_proxy.cc (right):

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_confirmation_dialog_proxy.cc:33: friend
base::RefCountedThreadSafe<Core>;
On 2014/12/29 22:27:42, kelvinp wrote:
> This is no longer needed as Core is no longer ref-counted.

Done.

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_confirmation_dialog_proxy.cc:70:
weak_factory_->GetWeakPtr()));
On 2014/12/22 19:29:08, Sergey Ulanov wrote:
> It should be safe to use base::Unretained() here. Core owns |dialog_| and the
> |dialog_| shouldn't call the callback after it's destroyed.

Done.

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_confirmation_dialog_proxy.h (right):

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_confirmation_dialog_proxy.h:27:
scoped_refptr<base::SingleThreadTaskRunner> caller_task_runner,
On 2014/12/22 19:29:08, Sergey Ulanov wrote:
> Maybe use ThreadTaskRunnerHandle::Get() instead of passing this parameter
here?

Done.

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
File remoting/host/it2me/it2me_host.cc (right):

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_host.cc:41: scoped_ptr<It2MeConfirmationDialogFactory>
confirmation_dialog_factory,
On 2014/12/22 19:29:09, Sergey Ulanov wrote:
> I'm not sure why you need this parameter. Why not just call
> It2MeConfirmationDialog::Create() in  It2MeHost::Confirm() and then you can
> remove the factory as well.

Injecting the confirmation dialog as a dependency is a requirement for writing a
unit test for It2MeHost. I thought there were some tests already when I was
writing this. But I think we should keep the factory to minimize the required
refactoring when (I hope) we come around to write one.

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_host.cc:134: void It2MeHost::Confirm() {
On 2014/12/22 19:29:08, Sergey Ulanov wrote:
> Maybe call this ShowConfirmPrompt()?

Done.

https://codereview.chromium.org/816903002/diff/100001/remoting/host/it2me/it2...
remoting/host/it2me/it2me_host.cc:175: DCHECK(state_ == kStarting);
On 2014/12/22 19:29:08, Sergey Ulanov wrote:
> DCHECK_EQ please

Done.

https://codereview.chromium.org/816903002/diff/100001/remoting/resources/remo...
File remoting/resources/remoting_strings.grd (right):

https://codereview.chromium.org/816903002/diff/100001/remoting/resources/remo...
remoting/resources/remoting_strings.grd:945: Would like to share this computer
for another user to see and control?
On 2014/12/29 22:27:42, kelvinp wrote:
> s/Would like/Would you like

Done.

[kelvinp, 2015-01-05 21:02:03]

lgtm

[dcaiafa, 2015-01-06 18:45:11]

sergey: ping :)

[Sergey Ulanov, 2015-01-06 19:17:58]

lgtm

[dcaiafa, 2015-01-06 19:20:10]

The CQ bit was checked by dcaiafa@chromium.org

[commit-bot: I haz the power, 2015-01-06 19:20:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/816903002/140001

[commit-bot: I haz the power, 2015-01-06 21:21:53]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2015-01-06 21:23:43]

Patchset 4 (id:??) landed as
https://crrev.com/3a5e72899629e0c8d254109707e747742686fbca
Cr-Commit-Position: refs/heads/master@{#310149}