Remote assistance on Chrome OS Part V - It2MeHost Continue Window

Remote assistance on Chrome OS Part V - It2MeHost Continue Window

This CL implements the ContinueWindow on ChromeOS, which will
be shown to the user for confirmation after 10 minutes into a Remote
Assistance session.

To implement the ContinueWindow, this CL introduces a new class
MessageBox, which is a re-usable component built on top of the
widget framework.

BUG=424908
Committed: https://crrev.com/6410d7ad4937a4a86e9ad28b9ecf99f961305b2d
Cr-Commit-Position: refs/heads/master@{#302657}

Committed: https://crrev.com/f469112851afa11cb3894451a2b4f9724682ffbd
Cr-Commit-Position: refs/heads/master@{#302706}

[kelvinp, 2014-10-31 00:48:47]

kelvinp@chromium.org changed reviewers:
+ dcaiafa@chromium.org, wez@chromium.org

[kelvinp, 2014-10-31 00:53:34]

Patchset #1 (id:1) has been deleted

[kelvinp, 2014-10-31 00:55:51]

PTAL

[dcaiafa, 2014-10-31 18:05:49]

https://codereview.chromium.org/690183002/diff/20001/remoting/host/chromeos/m...
File remoting/host/chromeos/message_box.cc (right):

https://codereview.chromium.org/690183002/diff/20001/remoting/host/chromeos/m...
remoting/host/chromeos/message_box.cc:67: if (button == ui::DIALOG_BUTTON_OK) {
Please use a switch statement here instead.

https://codereview.chromium.org/690183002/diff/20001/remoting/host/chromeos/m...
File remoting/host/chromeos/message_box.h (right):

https://codereview.chromium.org/690183002/diff/20001/remoting/host/chromeos/m...
remoting/host/chromeos/message_box.h:15: // is not thread-safe.
Does it have to run in a specific thread?

https://codereview.chromium.org/690183002/diff/20001/remoting/host/chromeos/m...
remoting/host/chromeos/message_box.h:58: ~MessageBox();
~MessageBox() override;

https://codereview.chromium.org/690183002/diff/20001/remoting/host/continue_w...
File remoting/host/continue_window_chromeos.cc (right):

https://codereview.chromium.org/690183002/diff/20001/remoting/host/continue_w...
remoting/host/continue_window_chromeos.cc:59: void ContinueWindowAura::HideUi()
{
Why not call message_box_->Hide() here?

[kelvinp, 2014-10-31 21:12:53]

@dan.  PTAL

https://codereview.chromium.org/690183002/diff/20001/remoting/host/chromeos/m...
File remoting/host/chromeos/message_box.cc (right):

https://codereview.chromium.org/690183002/diff/20001/remoting/host/chromeos/m...
remoting/host/chromeos/message_box.cc:67: if (button == ui::DIALOG_BUTTON_OK) {
On 2014/10/31 18:05:49, dcaiafa wrote:
> Please use a switch statement here instead.

Done.

https://codereview.chromium.org/690183002/diff/20001/remoting/host/chromeos/m...
File remoting/host/chromeos/message_box.h (right):

https://codereview.chromium.org/690183002/diff/20001/remoting/host/chromeos/m...
remoting/host/chromeos/message_box.h:15: // is not thread-safe.
On 2014/10/31 18:05:49, dcaiafa wrote:
> Does it have to run in a specific thread?

Comments added.

https://codereview.chromium.org/690183002/diff/20001/remoting/host/chromeos/m...
remoting/host/chromeos/message_box.h:58: ~MessageBox();
On 2014/10/31 18:05:49, dcaiafa wrote:
> ~MessageBox() override;

Done.

https://codereview.chromium.org/690183002/diff/20001/remoting/host/continue_w...
File remoting/host/continue_window_chromeos.cc (right):

https://codereview.chromium.org/690183002/diff/20001/remoting/host/continue_w...
remoting/host/continue_window_chromeos.cc:59: void ContinueWindowAura::HideUi()
{
On 2014/10/31 18:05:49, dcaiafa wrote:
> Why not call message_box_->Hide() here?

HideUi() is called by ContinueSession which is called when the user clicks the
Ok button. The MessageBox will close the dialog when a button is clicked so it
is not necessary.  But for consistency, doesn't hurt to call it here.

Done.

[Wez, 2014-10-31 21:22:24]

https://codereview.chromium.org/690183002/diff/40001/remoting/host/chromeos/m...
File remoting/host/chromeos/message_box.cc (right):

https://codereview.chromium.org/690183002/diff/40001/remoting/host/chromeos/m...
remoting/host/chromeos/message_box.cc:27: // The widget is owned by the
NativeWidget.  See class comments for ownership.
Which class comments?

https://codereview.chromium.org/690183002/diff/40001/remoting/host/chromeos/m...
remoting/host/chromeos/message_box.cc:30: nullptr /* parent window*/, nullptr /*
parent view */);
put these one-per-line so that the comments are more readable.

https://codereview.chromium.org/690183002/diff/40001/remoting/host/chromeos/m...
File remoting/host/chromeos/message_box.h (right):

https://codereview.chromium.org/690183002/diff/40001/remoting/host/chromeos/m...
remoting/host/chromeos/message_box.h:14: // Shows a system modal message box
with ok and cancel buttons. This class
s/ok/OK

https://codereview.chromium.org/690183002/diff/40001/remoting/host/chromeos/m...
remoting/host/chromeos/message_box.h:16: // process.
If it's not thread-safe, why is it not base::NonThreadSafe?

https://codereview.chromium.org/690183002/diff/40001/remoting/host/chromeos/m...
remoting/host/chromeos/message_box.h:28: public base::RefCounted<MessageBox> {
Ugh... you really don't need to ref-count here. Just break the implementation up
into the MessageBox (the public interface, owned by the caller) and the internal
part that implements DialogDelegateView and remains valid until DeleteDelegate
is called - internally that simply holds a reference to the parent MessageBox
that the MessageBox clears if it's deleted.

https://codereview.chromium.org/690183002/diff/40001/remoting/host/chromeos/m...
remoting/host/chromeos/message_box.h:47: void Hide();
These methods comments add nothing.

https://codereview.chromium.org/690183002/diff/40001/remoting/host/chromeos/m...
remoting/host/chromeos/message_box.h:59: ~MessageBox() override;
Why are you overriding the destructor, only to do nothing?

https://codereview.chromium.org/690183002/diff/40001/remoting/host/continue_w...
File remoting/host/continue_window_chromeos.cc (right):

https://codereview.chromium.org/690183002/diff/40001/remoting/host/continue_w...
remoting/host/continue_window_chromeos.cc:29:
base::WeakPtrFactory<ContinueWindowAura> weak_factory_;
When you fix the ownership model of |message_box_| you will be able to get rid
of this...

[kelvinp, 2014-11-01 00:38:28]

Patchset #3 (id:60001) has been deleted

[kelvinp, 2014-11-01 00:42:01]

PTAL

https://codereview.chromium.org/690183002/diff/40001/remoting/host/chromeos/m...
File remoting/host/chromeos/message_box.cc (right):

https://codereview.chromium.org/690183002/diff/40001/remoting/host/chromeos/m...
remoting/host/chromeos/message_box.cc:27: // The widget is owned by the
NativeWidget.  See class comments for ownership.
On 2014/10/31 21:22:24, Wez wrote:
> Which class comments?

The comments in Widget.h

https://codereview.chromium.org/690183002/diff/40001/remoting/host/chromeos/m...
remoting/host/chromeos/message_box.cc:30: nullptr /* parent window*/, nullptr /*
parent view */);
On 2014/10/31 21:22:24, Wez wrote:
> put these one-per-line so that the comments are more readable.

Done.

https://codereview.chromium.org/690183002/diff/40001/remoting/host/chromeos/m...
File remoting/host/chromeos/message_box.h (right):

https://codereview.chromium.org/690183002/diff/40001/remoting/host/chromeos/m...
remoting/host/chromeos/message_box.h:14: // Shows a system modal message box
with ok and cancel buttons. This class
On 2014/10/31 21:22:24, Wez wrote:
> s/ok/OK

Done.

https://codereview.chromium.org/690183002/diff/40001/remoting/host/chromeos/m...
remoting/host/chromeos/message_box.h:16: // process.
On 2014/10/31 21:22:24, Wez wrote:
> If it's not thread-safe, why is it not base::NonThreadSafe?

ThreadChecker is added.  According to the comments, it is preferred over
base::NonThreadSafe.

https://codereview.chromium.org/690183002/diff/40001/remoting/host/chromeos/m...
remoting/host/chromeos/message_box.h:28: public base::RefCounted<MessageBox> {
On 2014/10/31 21:22:24, Wez wrote:
> Ugh... you really don't need to ref-count here. Just break the implementation
up
> into the MessageBox (the public interface, owned by the caller) and the
internal
> part that implements DialogDelegateView and remains valid until DeleteDelegate
> is called - internally that simply holds a reference to the parent MessageBox
> that the MessageBox clears if it's deleted.

Done.

https://codereview.chromium.org/690183002/diff/40001/remoting/host/chromeos/m...
remoting/host/chromeos/message_box.h:47: void Hide();
On 2014/10/31 21:22:24, Wez wrote:
> These methods comments add nothing.

Done.

https://codereview.chromium.org/690183002/diff/40001/remoting/host/chromeos/m...
remoting/host/chromeos/message_box.h:59: ~MessageBox() override;
On 2014/10/31 21:22:24, Wez wrote:
> Why are you overriding the destructor, only to do nothing?

Done.

https://codereview.chromium.org/690183002/diff/40001/remoting/host/continue_w...
File remoting/host/continue_window_chromeos.cc (right):

https://codereview.chromium.org/690183002/diff/40001/remoting/host/continue_w...
remoting/host/continue_window_chromeos.cc:29:
base::WeakPtrFactory<ContinueWindowAura> weak_factory_;
On 2014/10/31 21:22:24, Wez wrote:
> When you fix the ownership model of |message_box_| you will be able to get rid
> of this...

Done.

[Wez, 2014-11-04 19:10:20]

lgtm

https://codereview.chromium.org/690183002/diff/100001/remoting/host/chromeos/...
File remoting/host/chromeos/message_box.cc (right):

https://codereview.chromium.org/690183002/diff/100001/remoting/host/chromeos/...
remoting/host/chromeos/message_box.cc:21: // Core and the MessageBox hold a raw
references to each other, which is
nit: s/is/are

https://codereview.chromium.org/690183002/diff/100001/remoting/host/chromeos/...
remoting/host/chromeos/message_box.cc:32: // Mirrors the MessageBox::Core
interface
You mean mirrors the public MessageBox interface?

[kelvinp, 2014-11-04 19:28:27]

Thank you for the review

https://codereview.chromium.org/690183002/diff/100001/remoting/host/chromeos/...
File remoting/host/chromeos/message_box.cc (right):

https://codereview.chromium.org/690183002/diff/100001/remoting/host/chromeos/...
remoting/host/chromeos/message_box.cc:21: // Core and the MessageBox hold a raw
references to each other, which is
On 2014/11/04 19:10:20, Wez wrote:
> nit: s/is/are

Done.

https://codereview.chromium.org/690183002/diff/100001/remoting/host/chromeos/...
remoting/host/chromeos/message_box.cc:32: // Mirrors the MessageBox::Core
interface
On 2014/11/04 19:10:20, Wez wrote:
> You mean mirrors the public MessageBox interface?

Done.

[kelvinp, 2014-11-04 19:28:30]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-11-04 19:30:09]

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

[commit-bot: I haz the power, 2014-11-04 20:36:42]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2014-11-04 20:38:46]

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

[kelvinp, 2014-11-04 20:54:31]

A revert of this CL (patchset #5 id:120001) has been created in
https://codereview.chromium.org/689093004/ by kelvinp@chromium.org.

The reason for reverting is: Build failure on
http://build.chromium.org/p/chromium.chromiumos/buildstatus?builder=Linux%20C....

[kelvinp, 2014-11-04 22:07:13]

The CQ bit was checked by kelvinp@chromium.org

[commit-bot: I haz the power, 2014-11-04 22:09:32]

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

[commit-bot: I haz the power, 2014-11-04 23:27:12]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2014-11-04 23:27:49]

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