Change the idle logout dialog over to use views.

chrome/browser/chromeos/ui/idle_logout_dialog.cc

+// Copyright (c) 2012 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 "chrome/browser/chromeos/ui/idle_logout_dialog.h"
+
+#include <string>

[sky, 2012/03/11 21:57:47]

Do you really need these includes in both the header and .cc?

[rkc, 2012/03/12 22:43:38]

They weren't needed in either, removed.
Done.

+#include <vector>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/time.h"
+#include "base/timer.h"
+#include "base/string_number_conversions.h"
+#include "chrome/browser/chromeos/kiosk_mode/kiosk_mode_helper.h"
+#include "chrome/browser/ui/browser_list.h"
+#include "chrome/common/logging_chrome.h"
+#include "grit/browser_resources.h"
+#include "grit/generated_resources.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/base/resource/resource_bundle.h"
+#include "ui/views/layout/grid_layout.h"
+#include "ui/views/layout/layout_constants.h"
+#include "ui/views/widget/widget.h"
+
+#if defined(USE_AURA)
+#include "ash/shell.h"
+#include "ui/aura/root_window.h"
+#endif
+
+using views::GridLayout;

[tfarina, 2012/03/11 23:42:58]

nit: I'd remove this and just have views::GridLayout below, it isn't too much to
type anyway.

[rkc, 2012/03/12 22:43:38]

Done.

+
+namespace {
+
+// Global singleton instance of our dialog class.
+IdleLogoutDialog* g_instance = NULL;
+// Height/Width of the logout dialog.
+const int kIdleLogoutDialogWidth = 400;
+const int kIdleLogoutDialogHeight = 100;
+
+const base::TimeDelta kFudgeTime = base::TimeDelta::FromMilliseconds(100);

[tfarina, 2012/03/11 23:42:58]

I think this adds a static initializer? See
http://codereview.chromium.org/9635018/ for an example.

[rkc, 2012/03/12 22:43:38]

Removed this completely.

+
+}  // namespace
+
+////////////////////////////////////////////////////////////////////////////////
+// IdleLogoutDialog public static methods
+
+void IdleLogoutDialog::ShowIdleLogoutDialog() {
+  if (!g_instance) {
+    g_instance = new IdleLogoutDialog();
+  }
+  g_instance->Show();
+}
+
+void IdleLogoutDialog::CloseIdleLogoutDialog() {
+  if (g_instance) {
+    g_instance->Close();
+    delete g_instance;

[sky, 2012/03/11 21:57:47]

It's a lot safter to put this in DeleteDelegate. That way you don't have to
worry if a future code path allows closing via some other mechanism.

[rkc, 2012/03/12 22:43:38]

Done.

+    g_instance = NULL;
+  }
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// IdleLogoutDialog private methods
+IdleLogoutDialog::IdleLogoutDialog() {

[sky, 2012/03/11 21:57:47]

Order of methods should match header.

[sky, 2012/03/11 21:57:47]

Have the constructor initialize warning_label_ and restart_label_ to NULL.

[rkc, 2012/03/12 22:43:38]

Done.

[rkc, 2012/03/12 22:43:38]

Done.

+  Init();
+}
+
+void IdleLogoutDialog::Init() {
+  if (!chromeos::KioskModeHelper::Get()->is_initialized()) {
+    chromeos::KioskModeHelper::Get()->Initialize(

[sky, 2012/03/11 21:57:47]

What happens if Close is called this returns?

[rkc, 2012/03/12 22:43:38]

Bad things. Fixed it so it's handled correctly.
Done.

+            base::Bind(&IdleLogoutDialog::Init,
+                       base::Unretained(this)));
+    return;
+  }
+
+  warning_label_ = new views::Label(
+      l10n_util::GetStringUTF16(IDS_IDLE_LOGOUT_WARNING));
+  warning_label_->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
+  warning_label_->SetMultiLine(true);
+  warning_label_->SetFont(
+      ResourceBundle::GetSharedInstance().GetFont(ResourceBundle::BaseFont));

[tfarina, 2012/03/11 23:42:58]

nit: Make a temp var, like:

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

and use it here and elsewhere.

[rkc, 2012/03/12 22:43:38]

Done.

+
+  restart_label_ = new views::Label();
+  restart_label_->SetHorizontalAlignment(views::Label::ALIGN_LEFT);
+  restart_label_-> SetFont(

[tfarina, 2012/03/11 23:42:58]

nit: looks like there is an extra space between -> and SetFont

[rkc, 2012/03/12 22:43:38]

Done.

+      ResourceBundle::GetSharedInstance().GetFont(
+          ResourceBundle::BoldFont));

[tfarina, 2012/03/11 23:42:58]

nit: ui::ResourceBundle::

[rkc, 2012/03/12 22:43:38]

Done.

+
+  GridLayout* layout = GridLayout::CreatePanel(this);
+  SetLayoutManager(layout);
+
+  views::ColumnSet* column_set = layout->AddColumnSet(0);
+  column_set->AddColumn(GridLayout::FILL, GridLayout::CENTER, 1,
+                        GridLayout::USE_PREF, 0, 0);
+  layout->StartRow(0, 0);
+  layout->AddView(warning_label_);
+  layout->AddPaddingRow(0, views::kUnrelatedControlVerticalSpacing);
+  layout->StartRow(0, 0);
+  layout->AddView(restart_label_);
+}
+
+void IdleLogoutDialog::Show() {
+  // Setup the countdown label before showing.
+  countdown_end_time_ = base::Time::Now() + base::TimeDelta::FromSeconds(
+      chromeos::KioskModeHelper::Get()->GetIdleLogoutWarningTimeout());
+  UpdateCountdownTimer();
+
+#if defined(USE_AURA)
+  aura::RootWindow* root_window = ash::Shell::GetRootWindow();
+  views::Widget::CreateWindowWithParent(this, root_window);
+  GetWidget()->Show();
+#else
+  NOTIMPLEMENTED();

[sky, 2012/03/11 21:57:47]

Do you even intend to implement this for non-aura? If not, nuke the ifdefs and
exclude this file on non-aura chromeos.

[rkc, 2012/03/12 22:43:38]

Code from this file is called from other places.
I could put ifdefs around all those locations and around the #include - but
since this is the 'only' place that we're doing anything aura specific, it
seemed simpler to put the #ifdefs here and get the non-aura bots to pass.

+#endif
+
+  // Update countdown every 1 second.
+  timer_.Start(FROM_HERE, base::TimeDelta::FromSeconds(1),
+               this, &IdleLogoutDialog::UpdateCountdownTimer);
+}
+
+void IdleLogoutDialog::Close() {
+  DCHECK(GetWidget());
+
+  timer_.Stop();
+  GetWidget()->Close();
+}
+
+void IdleLogoutDialog::UpdateCountdownTimer() {
+  base::TimeDelta logout_warning_time = countdown_end_time_ -
+                                        base::Time::Now() + kFudgeTime;

[sky, 2012/03/11 21:57:47]

Why the kFudgeTime?

[rkc, 2012/03/12 22:43:38]

InSeconds doesn't actually round the milli/microseconds off to the nearest
second. Since we don't get Time::Now at 'exactly' -1 second from last time, we
can display the incorrect time to the user (usually -1 from what it should
show).

Adding a fudge value is a simple way to ensure that our internal processing time
is adjusted for when updating the display.

Looking at it again, it seems better if we do the ms->s conversion here rounding
correctly in the process.

Done.

+  if (logout_warning_time.InSeconds() > 1) {
+    restart_label_->SetText(l10n_util::GetStringFUTF16(
+        IDS_IDLE_LOGOUT_WARNING_RESTART,
+        string16(base::Int64ToString16(logout_warning_time.InSeconds()))));
+  } else if (logout_warning_time.InSeconds() > 0) {
+    restart_label_->SetText(l10n_util::GetStringUTF16(
+        IDS_IDLE_LOGOUT_WARNING_RESTART_1S));
+  } else {
+    // Set the label - the logout probably won't be instant.
+    restart_label_->SetText(l10n_util::GetStringUTF16(
+        IDS_IDLE_LOGOUT_WARNING_RESTART_NOW));
+
+    // Logout the current user.
+    BrowserList::AttemptUserExit();
+  }
+}
+
+////////////////////////////////////////////////////////////////////////////////
+// Overridden from HtmlDialogUIDelegate
+int IdleLogoutDialog::GetDialogButtons() const {
+  return ui::DIALOG_BUTTON_NONE;
+}
+
+ui::ModalType IdleLogoutDialog::GetModalType() const {
+  return ui::MODAL_TYPE_WINDOW;
+}
+
+string16 IdleLogoutDialog::GetWindowTitle() const {
+  return l10n_util::GetStringUTF16(IDS_IDLE_LOGOUT_TITLE);
+}
+
+views::View* IdleLogoutDialog::GetContentsView() {
+  return this;
+}
+
+gfx::Size IdleLogoutDialog::GetPreferredSize() {
+  return gfx::Size(kIdleLogoutDialogWidth, kIdleLogoutDialogHeight);

[sky, 2012/03/11 21:57:47]

Don't you want to make sure it can fit the content?

[rkc, 2012/03/12 22:43:38]

Thought that is what 'preferred size' did? Set it to that size 'if' it fits the
content?

[sky, 2012/03/12 23:33:38]

The only way it knows is to ask for the preferred size, which you're overriding
here.

[rkc, 2012/03/12 23:57:51]

I think the user should keep the size adjusted to the text; we have a certain
amount of white space also around the text  also which makes the dialog look
nicer.

[sky, 2012/03/13 03:43:54]

I agree, but this isn't adjusting to the size of the text. You're returning a
hard coded size which may not be enough in some locales. 

[rkc, 2012/03/14 22:35:32]

I realized the main thing we really want is that our text doesn't go too wide
and end up giving us a thin horizontal dialog. So setting just the max width on
the label elements and letting GetPreferredSize handle the rest gives us the
desirable result.

Done.

+}
+
+void IdleLogoutDialog::Layout() {
+  GetLayoutManager()->Layout(this);
+}