Change status button menu implementation from Menu2 to MenuItemView.

chrome/browser/chromeos/status/clock_menu_button.cc

-// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Copyright (c) 2011 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/status/clock_menu_button.h"
 
 #include "base/i18n/time_formatting.h"
 #include "base/string_util.h"
 #include "base/time.h"
 #include "base/utf_string_conversions.h"
 #include "chrome/browser/chromeos/cros/cros_library.h"
 #include "chrome/browser/chromeos/status/status_area_host.h"
 #include "grit/generated_resources.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/gfx/canvas.h"
 #include "ui/gfx/font.h"
+#include "views/widget/widget.h"
+#include "views/window/window.h"
+
+using views::MenuItemView;
 
 namespace chromeos {
 
+// MenuItemView item ids
+enum {
+  CLOCK_DISPLAY_ITEM,
+  CLOCK_OPEN_OPTIONS_ITEM,
+};
+
 // Amount of slop to add into the timer to make sure we're into the next minute
 // when the timer goes off.
 const int kTimerSlopSeconds = 1;
 
 #if defined(CROS_FONTS_USING_BCI)
 const int kFontSizeDelta = 0;
 #else
 const int kFontSizeDelta = 1;
 #endif

[oshima, 2011/04/14 17:27:31]

can you move these (enum to kFontSizeDelta) into anonymous namespace?

[rhashimoto, 2011/04/14 18:27:04]

Done.

 
 ClockMenuButton::ClockMenuButton(StatusAreaHost* host)
     : StatusAreaButton(this),
       host_(host) {
   // Add as SystemLibrary observer. We update the clock if timezone changes.
   CrosLibrary::Get()->GetSystemLibrary()->AddObserver(this);
   CrosLibrary::Get()->GetPowerLibrary()->AddObserver(this);
 
   set_border(NULL);
   set_use_menu_button_paint(true);
@@ skipping 35 matching lines @@
 }
 
 void ClockMenuButton::UpdateText() {
   base::Time time(base::Time::Now());
   SetText(UTF16ToWide(base::TimeFormatTimeOfDay(time)));
   SetTooltipText(UTF16ToWide(base::TimeFormatShortDate(time)));
   SchedulePaint();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
-// ClockMenuButton, ui::MenuModel implementation:
-
-int ClockMenuButton::GetItemCount() const {
-  // If options dialog is unavailable, don't count a separator and configure
-  // menu item.
-  return host_->ShouldOpenButtonOptions(this) ? 3 : 1;
+// ClockMenuButton, views::MenuDelegate implementation:
+std::wstring ClockMenuButton::GetLabel(int id) const
+{
+  DCHECK(id == CLOCK_DISPLAY_ITEM);

[oshima, 2011/04/14 17:27:31]

DCHECK_EQ

note: expected value first

[rhashimoto, 2011/04/14 18:27:04]

Done.

+  const string16 label = base::TimeFormatFriendlyDate(base::Time::Now());
+  return UTF16ToWide(label);
 }
 
-ui::MenuModel::ItemType ClockMenuButton::GetTypeAt(int index) const {
-  // There's a separator between the current date and the menu item to open
-  // the options menu.
-  return index == 1 ? ui::MenuModel::TYPE_SEPARATOR:
-                      ui::MenuModel::TYPE_COMMAND;
+bool ClockMenuButton::IsCommandEnabled(int id) const
+{
+  DCHECK(id == CLOCK_DISPLAY_ITEM || id == CLOCK_OPEN_OPTIONS_ITEM);
+  if (id == CLOCK_DISPLAY_ITEM)

[oshima, 2011/04/14 17:27:31]

return id == CLOCK_OPEN_OPTIONS_ITEM

[rhashimoto, 2011/04/14 18:27:04]

Done.

+    return false;
+  else
+    return true;
 }
 
-string16 ClockMenuButton::GetLabelAt(int index) const {
-  if (index == 0)
-    return base::TimeFormatFriendlyDate(base::Time::Now());
-  return l10n_util::GetStringUTF16(IDS_STATUSBAR_CLOCK_OPEN_OPTIONS_DIALOG);
-}
-
-bool ClockMenuButton::IsEnabledAt(int index) const {
-  // The 1st item is the current date, which is disabled.
-  return index != 0;
-}
-
-void ClockMenuButton::ActivatedAt(int index) {
+void ClockMenuButton::ExecuteCommand(int id)
+{
+  DCHECK(id == CLOCK_OPEN_OPTIONS_ITEM);

[oshima, 2011/04/14 17:27:31]

DCHECK_EQ

[rhashimoto, 2011/04/14 18:27:04]

Done.

   host_->OpenButtonOptions(this);
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // ClockMenuButton, PowerLibrary::Observer implementation:
 
 void ClockMenuButton::SystemResumed() {
   UpdateText();
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // ClockMenuButton, SystemLibrary::Observer implementation:
 
 void ClockMenuButton::TimezoneChanged(const icu::TimeZone& timezone) {
   UpdateText();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // ClockMenuButton, views::ViewMenuDelegate implementation:
 
 void ClockMenuButton::RunMenu(views::View* source, const gfx::Point& pt) {
-  if (!clock_menu_.get())
-    clock_menu_.reset(new views::Menu2(this));
-  else
-    clock_menu_->Rebuild();
-  clock_menu_->UpdateStates();
-  clock_menu_->RunMenuAt(pt, views::Menu2::ALIGN_TOPRIGHT);
+  // View passed in must be a views::MenuButton, i.e. the ClockMenuButton.
+  DCHECK(source == this);

[oshima, 2011/04/14 17:27:31]

DCHECK_EQ

[rhashimoto, 2011/04/14 18:27:04]

Done.

+
+  if (!menu_.get()) {
+    menu_.reset(new MenuItemView(this));
+
+    // Text for this item will be set by GetLabel().
+    menu_->AppendDelegateMenuItem(CLOCK_DISPLAY_ITEM);
+
+    // If options dialog is unavailable, don't count a separator and configure
+    // menu item.
+    if (host_->ShouldOpenButtonOptions(this)) {
+      menu_->AppendSeparator();
+
+      const string16 clock_open_options_label =
+          l10n_util::GetStringUTF16(IDS_STATUSBAR_CLOCK_OPEN_OPTIONS_DIALOG);
+      menu_->AppendMenuItemWithLabel(
+          CLOCK_OPEN_OPTIONS_ITEM,
+          UTF16ToWide(clock_open_options_label));
+    }
+  }

[oshima, 2011/04/14 17:27:31]

It's probably better to separate above code. I'm fine with either

MaybeInitMenu() (or EnsureMenu()) and check menu_.get() inside, 

or

if (!menu_.get()) {
   InitMenu();
}

[rhashimoto, 2011/04/14 18:27:04]

Done.

+
+  // TODO(rhashimoto): Remove this workaround when WebUI provides a
+  // top-level widget on the ChromeOS login screen that is a window.
+  // The current BackgroundView class for the ChromeOS login screen
+  // creates a owning Widget that has a native GtkWindow but is not a
+  // Window.  This makes it impossible to get the NativeWindow via
+  // the views API.  This workaround casts the top-level NativeWidget
+  // to a NativeWindow that we can pass to MenuItemView::RunMenuAt().

[oshima, 2011/04/14 17:27:31]

won't this work?

source->GetWidget()->GetToplevelWidget()->GetNativeView() 

[rhashimoto, 2011/04/14 18:27:04]

That gets the NativeView, but we have to pass a NativeWindow to
MenuItemView::RunMenuAt(), and I don't see a way to convert a NativeView to a
NativeWindow in a platform-independent manner.

[oshima, 2011/04/15 17:40:12]

NativeView is gtk_windonw instance in gtk_widget type, so you can do the
same as

window = GTK_WINDOW(source->GetWidget()->GetToplevelWidget()->GetNativeView() 
)

Yes, this part is a bit mess, and I think menu API should simply takes either
view or Widget and let implementation do the details. (outside of this CL's
scope)

[rhashimoto, 2011/04/15 21:20:27]

I think this is fundamentally what I've done, i.e. casting NativeView using the
GTK_WINDOW casting macro.  I'm not quite sure if you want something different
here.

+  gfx::NativeWindow window;
+  if (source->GetWindow()) {
+    // This is the normal case with a browser.
+    window = source->GetWindow()->GetNativeWindow();
+  } else {
+#if defined(OS_WIN)
+    NOTREACHED();
+#elif defined(USE_X11)
+    window = GTK_WINDOW(source->GetWidget()->GetNativeView());
+#endif

[oshima, 2011/04/15 17:40:12]

no need for ifdefs as ths is only for chromeos

[rhashimoto, 2011/04/15 21:20:27]

Done.  I was thinking that the #ifdefs signalled the platform-specific ugliness
in this supposedly platform-independent class, but perhaps the TODO is signal
enough.

+  }
+
+  gfx::Point screen_loc;
+  views::View::ConvertPointToScreen(source, &screen_loc);
+  gfx::Rect bounds(screen_loc, source->size());
+  menu_->RunMenuAt(
+      window,
+      this,
+      bounds,
+      base::i18n::IsRTL() ? MenuItemView::TOPLEFT : MenuItemView::TOPRIGHT,
+      true);
 }
 
 }  // namespace chromeos