Remote assistance on Chrome OS Part V - It2MeHost Continue Window

remoting/host/chromeos/message_box.h

Index: remoting/host/chromeos/message_box.h
diff --git a/remoting/host/chromeos/message_box.h b/remoting/host/chromeos/message_box.h
new file mode 100644
index 0000000000000000000000000000000000000000..bf928d808672079921e7ef01f5999c8dbea1d56b
--- /dev/null
+++ b/remoting/host/chromeos/message_box.h
@@ -0,0 +1,71 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef REMOTING_HOST_CHROMEOS_MESSAGE_BOX_H_
+#define REMOTING_HOST_CHROMEOS_MESSAGE_BOX_H_
+
+#include "base/memory/ref_counted.h"
+#include "ui/views/window/dialog_delegate.h"
+
+namespace remoting {
+
+// Overview:
+// Shows a system modal message box with ok and cancel buttons. This class

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

s/ok/OK

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

Done.

+// is not thread-safe, it must be called on the UI thread of the browser
+// process.

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

If it's not thread-safe, why is it not base::NonThreadSafe?

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

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

+//
+// Implementation details:
+// The MessageBox creates the dialog using the views::DialogWidget.  The
+// DialogWidget is created by the caller but its lifetime is managed by the
+// NativeWidget.  The DialogWidget communicates with the MessageBox using the
+// DialogDelegateView interface.  DialogDelegateView must remain valid until
+// DeleteDelegate() is called, at which the DialogDelegateView deletes itself.
+// To abstract this awkward ownership model, the MessageBox object is
+// RefCounted.  It calls AddRef() on itself when the widget is created and calls
+// Release() when DeleteDelegate() is called.
+class MessageBox : public views::DialogDelegateView,
+                   public base::RefCounted<MessageBox> {

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

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.

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

Done.

+ public:
+  enum Result {
+    OK,
+    CANCEL
+  };
+
+  // ResultCallback will be invoked with Result::Cancel if the user closes the
+  // MessageBox without clicking on any buttons.
+  typedef base::Callback<void(Result)> ResultCallback;
+
+  MessageBox(const base::string16& message_label,
+             const base::string16& ok_label,
+             const base::string16& cancel_label,
+             ResultCallback result_callback);
+
+  // Shows the message box.
+  void Show();
+  // Hides the message box.
+  void Hide();

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

These methods comments add nothing.

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

Done.

+
+  // views::DialogDelegateView interface.
+  bool Accept() override;
+  bool Cancel() override;
+  ui::ModalType GetModalType() const override;
+  base::string16 GetWindowTitle() const override;
+  base::string16 GetDialogButtonLabel(ui::DialogButton button) const override;
+  void DeleteDelegate() override;
+
+ private:
+  friend class base::RefCounted<MessageBox>;
+  ~MessageBox() override;

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

Why are you overriding the destructor, only to do nothing?

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

Done.

+
+  const base::string16 message_label_;
+  const base::string16 ok_label_;
+  const base::string16 cancel_label_;
+  ResultCallback result_callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(MessageBox);
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_HOST_CHROMEOS_MESSAGE_BOX_H_