Implements the dialog view for logout button tray in public sessions.

ash/system/logout_button/logout_confirmation_dialog_view.cc

+// Copyright (c) 2013 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.
+
+#include "ash/system/logout_button/logout_confirmation_dialog_view.h"
+
+#include "ash/shell.h"
+#include "ash/system/logout_button/logout_button_tray.h"
+#include "ash/system/tray/system_tray_delegate.h"
+#include "base/strings/string_number_conversions.h"

[bartfab (slow), 2013/10/24 13:11:02]

This is not used anywhere.

[binjin, 2013/10/25 13:03:10]

Done.

+#include "grit/ash_strings.h"
+#include "grit/ui_strings.h"
+#include "ui/aura/root_window.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/base/l10n/time_format.h"
+#include "ui/base/resource/resource_bundle.h"
+#include "ui/views/controls/label.h"
+#include "ui/views/layout/grid_layout.h"
+#include "ui/views/layout/layout_constants.h"
+#include "ui/views/widget/widget.h"
+
+namespace {
+
+

[bartfab (slow), 2013/10/24 13:11:02]

Remove duplicated blank line.

[binjin, 2013/10/25 13:03:10]

Done.

+const int kLogoutConfirmationDialogMaxWidth = 300;

[bartfab (slow), 2013/10/24 13:11:02]

Could you use one of the existing ash/aura/views constants here? Hard-coding
pixel values is frowned upon.

+const int kCountdownUpdateIntervalMs = 1000;

[bartfab (slow), 2013/10/24 13:11:02]

Add a comment (prefixed by two spaces):

  // 1 second.

[binjin, 2013/10/25 13:03:10]

Done.

+const int kAutoLogoutDurationMs = 5 * 1000;

[bartfab (slow), 2013/10/24 13:11:02]

- This will need to be adjustable by pref.
- Add a comment explaining what this timeout is for.

[binjin, 2013/10/25 13:03:10]

Done.

+
+}

[bartfab (slow), 2013/10/24 13:11:02]

Replace with:

}  // namespace

[binjin, 2013/10/25 13:03:10]

Done.

+
+namespace ash {
+
+namespace internal {
+
+

[bartfab (slow), 2013/10/24 13:11:02]

Remove duplicated blank line.

[binjin, 2013/10/25 13:03:10]

Done.

+bool LogoutConfirmationDialogView::Accept() {
+  Shell::GetInstance()->system_tray_delegate()->SignOut();
+  return true;
+}
+
+int LogoutConfirmationDialogView::GetDialogButtons() const {
+  return ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL;

[bartfab (slow), 2013/10/24 13:11:02]

This is the default. No need to override the method.

[binjin, 2013/10/25 13:03:10]

Done.

+}
+
+ui::ModalType LogoutConfirmationDialogView::GetModalType() const {
+  return ui::MODAL_TYPE_WINDOW;

[bartfab (slow), 2013/10/24 13:11:02]

- It would seem that ui::MODAL_TYPE_SYSTEM is the correct choice here. 
- #include "ui/base/ui_base_types.h"

[binjin, 2013/10/25 13:03:10]

Done.

+}
+
+string16 LogoutConfirmationDialogView::GetWindowTitle() const {
+  return l10n_util::GetStringUTF16(IDS_ASH_LOGOUT_CONFIRMATION_TITLE);
+}
+
+string16 LogoutConfirmationDialogView::GetDialogButtonLabel(
+    ui::DialogButton button) const {
+  if (button == ui::DIALOG_BUTTON_OK) {

[bartfab (slow), 2013/10/24 13:11:02]

No curly braces for single-line conditionals.

[binjin, 2013/10/25 13:03:10]

Done.

+    return l10n_util::GetStringUTF16(IDS_ASH_LOGOUT_CONFIRMATION_BUTTON);
+  }
+  if (button == ui::DIALOG_BUTTON_CANCEL) {

[bartfab (slow), 2013/10/24 13:11:02]

No curly braces for single-line conditionals.

[binjin, 2013/10/25 13:03:10]

Done.

+    return l10n_util::GetStringUTF16(IDS_APP_CANCEL);

[bartfab (slow), 2013/10/24 13:11:02]

You can defer to the inherited method instead here.

[binjin, 2013/10/25 13:03:10]

Done.

+  }
+  return base::string16();

[bartfab (slow), 2013/10/24 13:11:02]

In this implementation, there should be a NOTREACHED(). But you can defer to the
inherited method instead here.

[binjin, 2013/10/25 13:03:10]

Done.

+}
+
+LogoutConfirmationDialogView::LogoutConfirmationDialogView(
+    LogoutButtonTray *owner) : display_label_(NULL), owner_(owner) {

[bartfab (slow), 2013/10/24 13:11:02]

Break at the : and the ,

[binjin, 2013/10/25 13:03:10]

Done.

+}
+
+LogoutConfirmationDialogView::~LogoutConfirmationDialogView() {
+  if (owner_) {

[bartfab (slow), 2013/10/24 13:11:02]

No curly braces for single-line conditionals.

[binjin, 2013/10/25 13:03:10]

Done.

+    owner_->RemoveConfirmDialogInstance(this);

[bartfab (slow), 2013/10/24 13:11:02]

- The "Instance" in this method name is unnecessary.
- Instead of keeping track of the owner and manually unregistering from it, why
not have the owner keep a WeakPtr to this?

[binjin, 2013/10/25 13:03:10]

Done.

+  }
+}
+
+void LogoutConfirmationDialogView::InitAndShow() {

[bartfab (slow), 2013/10/24 13:11:02]

Why is this initialization not done in the constructor?

[binjin, 2013/10/25 13:03:10]

Done.

+  ui::ResourceBundle &rb = ui::ResourceBundle::GetSharedInstance();

[bartfab (slow), 2013/10/24 13:11:02]

Abbreviations are strongly frowned upon. Please expand the name "rb".

[binjin, 2013/10/25 13:03:10]

Done.

+
+  display_label_ = new views::Label();

[bartfab (slow), 2013/10/24 13:11:02]

No () needed for new.

[binjin, 2013/10/25 13:03:10]

Done.

+  display_label_->SetHorizontalAlignment(gfx::ALIGN_LEFT);

[bartfab (slow), 2013/10/24 13:11:02]

#include "ui/gfx/text_constants.h"

[binjin, 2013/10/25 13:03:10]

Done.

+  display_label_->SetMultiLine(true);
+  display_label_->SetFont(rb.GetFont(ui::ResourceBundle::BaseFont));

[bartfab (slow), 2013/10/24 13:11:02]

- In this implementation, #include "ui/gfx/font.h".
- Is this not the default font anyway, making this unnecessary?

[binjin, 2013/10/25 13:03:10]

Done.

+
+  views::GridLayout *layout = views::GridLayout::CreatePanel(this);

[bartfab (slow), 2013/10/24 13:11:02]

- Put the * on the data type, not the variable name.
- Is a GridLayout really the best choice here? You are using one column only.
Could you not use a simpler layout class?

[binjin, 2013/10/25 13:03:10]

Done.

+  SetLayoutManager(layout);
+
+  views::ColumnSet *column_set = layout->AddColumnSet(0);

[bartfab (slow), 2013/10/24 13:11:02]

Put the * on the data type, not the variable name.

[binjin, 2013/10/25 13:03:10]

Done.

+  column_set->AddColumn(views::GridLayout::FILL, views::GridLayout::CENTER, 1,
+                        views::GridLayout::FIXED,
+                        kLogoutConfirmationDialogMaxWidth, 0);
+  layout->StartRow(0, 0);
+  layout->AddPaddingRow(0, views::kRelatedControlHorizontalSpacing);

[bartfab (slow), 2013/10/24 13:11:02]

You are not adding spacing between related controls. Is
views::kRelatedControlHorizontalSpacing really the correct constant to use here?

+  layout->StartRow(0, 0);
+  layout->AddView(display_label_);
+  layout->AddPaddingRow(0, views::kRelatedControlHorizontalSpacing);

[bartfab (slow), 2013/10/24 13:11:02]

You are not adding spacing between related controls. Is
views::kRelatedControlHorizontalSpacing really the correct constant to use here?

+
+  Show();
+}
+
+void LogoutConfirmationDialogView::Show() {
+  countdown_end_time_ = base::Time::Now() +
+    base::TimeDelta::FromMilliseconds(kAutoLogoutDurationMs);

[bartfab (slow), 2013/10/24 13:11:02]

Indent 4 spaces relative to the line above, not 2.

[binjin, 2013/10/25 13:03:10]

Done.

+
+  UpdateCountdown();
+
+  views::DialogDelegate::CreateDialogWidget(
+      this, ash::Shell::GetPrimaryRootWindow(), NULL);
+  GetWidget()->SetAlwaysOnTop(true);

[bartfab (slow), 2013/10/24 13:11:02]

Is this really necessary when the dialog is marked as modal?

[binjin, 2013/10/25 13:03:10]

Done.

+  GetWidget()->Show();
+
+  timer_.Start(FROM_HERE,

[bartfab (slow), 2013/10/24 13:11:02]

#include "base/location.h"

[binjin, 2013/10/25 13:03:10]

Done.

+               base::TimeDelta::FromMilliseconds(kCountdownUpdateIntervalMs),
+               this,
+               &LogoutConfirmationDialogView::UpdateCountdown);
+}
+
+void LogoutConfirmationDialogView::UpdateCountdown() {
+  // 500ms is a workaround to handle inaccurate time measuring. The time

[bartfab (slow), 2013/10/24 13:11:02]

What are you trying to work around here?

+  // duration is now accurate as long as duration is multiple of
+  // kCountdownUpdateIntervalMs

[bartfab (slow), 2013/10/24 13:11:02]

Add full stop at the end of the sentence.

[binjin, 2013/10/25 13:03:10]

Done.

+  base::TimeDelta logout_warning_time = countdown_end_time_ -

[bartfab (slow), 2013/10/24 13:11:02]

Mark as const.

[binjin, 2013/10/25 13:03:10]

Done.

+                                        base::Time::Now() +
+                                        base::TimeDelta::FromMilliseconds(500);
+  if (logout_warning_time.InSeconds() > 0) {
+    display_label_->SetText(l10n_util::GetStringFUTF16(
+          IDS_ASH_LOGOUT_CONFIRMATION_WARNING,

[bartfab (slow), 2013/10/24 13:11:02]

Indent 4 spaces, not 6.

[binjin, 2013/10/25 13:03:10]

Done.

+          ui::TimeFormat::TimeDurationLong(logout_warning_time)));
+  } else {
+    display_label_->SetText(l10n_util::GetStringUTF16(
+          IDS_ASH_LOGOUT_CONFIRMATION_WARNING_NOW));

[bartfab (slow), 2013/10/24 13:11:02]

Indent 4 spaces, not 6.

[binjin, 2013/10/25 13:03:10]

Done.

+    Shell::GetInstance()->system_tray_delegate()->SignOut();
+  }
+}
+
+}  // namespace internal
+
+}  // namespace ash

[bartfab (slow), 2013/10/24 13:11:02]

Add blank line at end of file.

[binjin, 2013/10/25 13:03:10]

Done.