NTP5: Starting implementation of a native menu for showing other device sessions.

chrome/browser/ui/search/other_device_menu.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/ui/search/other_device_menu.h"
+
+#include "base/string16.h"
+#include "base/utf_string_conversions.h"

[dhollowa, 2012/10/01 17:37:38]

This should be removed.  We'll put the string in resources instead.

[jeremycho, 2012/10/01 22:53:40]

Done.

+#include "chrome/browser/ui/browser.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/browser_window.h"
+#include "chrome/browser/ui/webui/ntp/foreign_session_handler.h"
+#include "content/public/browser/web_contents.h"
+#include "content/public/browser/web_ui.h"
+#include "chrome/browser/sync/glue/session_model_associator.h"
+#include "ui/base/text/text_elider.h"
+#include "ui/views/controls/menu/menu_model_adapter.h"
+#include "ui/views/controls/menu/menu_runner.h"
+#include "ui/views/widget/widget.h"
+#include "webkit/glue/window_open_disposition.h"
+
+using browser_sync::ForeignSessionHandler;
+using browser_sync::SessionModelAssociator;
+
+// The max width of a menu.  Menu text exceeding this will be elided.
+static const int kMaxWidth = 375;
+
+OtherDeviceMenu::OtherDeviceMenu(content::WebUI* web_ui,
+                                 const std::string& session_id,
+                                 const gfx::Point& location) :
+    web_ui_(web_ui), session_id_(session_id), location_(location),
+    ALLOW_THIS_IN_INITIALIZER_LIST(menu_model_(this)) {
+  SessionModelAssociator* associator =
+      ForeignSessionHandler::GetModelAssociator(web_ui);
+  std::vector<const SessionWindow*> windows;
+
+  // Populate the menu with the device's tabs, using separators between windows.
+  // TODO(jeremycho): Limit the number of menu items?

[dhollowa, 2012/10/01 17:37:38]

I see browser_sync::kMaxSessionsToShow.  Use that?  And maybe a create a local
|const int kMaxTabsToShow = 256;| locally to prevent DoS'ing the menu?

[jeremycho, 2012/10/01 22:53:40]

A menu should only contain a single session.  Added kMaxTabsToShow.

+  if (associator && associator->GetForeignSession(session_id, &windows)) {

[dhollowa, 2012/10/01 17:37:38]

Should |associator| ever be NULL here?  If not prefer a DCHECK(associator);

[jeremycho, 2012/10/01 22:53:40]

Yes it can be, e.g. if it hasn't finished syncing.

+    int command_id = 0;
+    bool lastWindowHasTabs = false;

[dhollowa, 2012/10/01 17:37:38]

C++ style is: |last_window_has_tabs|.

[jeremycho, 2012/10/01 22:53:40]

Done.

+    for (std::vector<const SessionWindow*>::const_iterator it =
+             windows.begin(); it != windows.end(); ++it) {
+      if (lastWindowHasTabs)
+        menu_model_.AddSeparator(ui::NORMAL_SEPARATOR);
+      lastWindowHasTabs = false;
+
+      const SessionWindow* window = *it;
+      for (size_t i = 0; i < window->tabs.size(); ++i) {
+        linked_ptr<DictionaryValue> tab_value =
+            linked_ptr<DictionaryValue>(new DictionaryValue());
+        if (ForeignSessionHandler::SessionTabToValue(
+                *window->tabs[i], tab_value.get())) {
+          lastWindowHasTabs = true;
+          tab_value->SetInteger("windowId", window->window_id.id());
+          string16 title;
+          tab_value->GetString("title", &title);
+          title = ui::ElideText(
+              title, gfx::Font(), kMaxWidth, ui::ELIDE_AT_END);
+          menu_model_.AddItem(command_id, title);
+          session_data_[++command_id] = tab_value;
+          // TODO(jeremycho): Use tab_value.GetString("url", &url) to

[dhollowa, 2012/10/01 17:37:38]

Please add a link to associated bug in this TODO.  I.e.:
// TODO(jeremycho): http://crbug.com/12345 ...

[jeremycho, 2012/10/01 22:53:40]

Done.

+          // request favicons.
+        }
+      }
+    }
+
+    // Add a "Show all" menu item if there is more than one tab.
+    if (session_data_.size() > 1) {
+      linked_ptr<DictionaryValue> show_all_tab_value =
+          linked_ptr<DictionaryValue>(new DictionaryValue());
+      // kInvalidId signifies that the entire session should be opened.
+      show_all_tab_value->SetInteger(
+          "sessionId",
+          ForeignSessionHandler::kInvalidId);
+      show_all_tab_value->SetInteger(
+          "windowId",
+          ForeignSessionHandler::kInvalidId);
+      menu_model_.AddSeparator(ui::NORMAL_SEPARATOR);
+      // TODO(jeremycho): i18n.
+      menu_model_.AddItem(command_id, UTF8ToUTF16("Show all"));

[dhollowa, 2012/10/01 17:37:38]

Let's do this now.  We shouldn't be hard coding strings into source.  See a good
example of how to do this in
|BackgroundModeManager::BackgroundModeData::BuildProfileMenu| and store the
string in src/chrome/app/generated_resources.grd

[jeremycho, 2012/10/01 22:53:40]

Reused the existing string from NTP4's other device menu.

+      session_data_[++command_id] = show_all_tab_value;
+    }
+  }
+}
+
+OtherDeviceMenu::~OtherDeviceMenu() {
+}
+
+void OtherDeviceMenu::ShowMenu() {
+  content::WebContents* web_contents = web_ui_->GetWebContents();

[dhollowa, 2012/10/01 17:37:38]

Is it possible to get a NULL result back here?  I'd be concerned that the
web_contents, browser, and/or widget could be NULL if the menu is somehow
triggered during a tear-down sequence.

[jeremycho, 2012/10/01 22:53:40]

WebContents* shouldn't be NULL, but the others could.  Done.

+  Browser* browser =  browser::FindBrowserWithWebContents(web_contents);
+  views::Widget* widget = views::Widget::GetWidgetForNativeWindow(
+      browser->window()->GetNativeWindow());
+  views::MenuModelAdapter menu_model_adapter(&menu_model_);
+  menu_runner_.reset(new views::MenuRunner(menu_model_adapter.CreateMenu()));
+
+  if (menu_runner_->RunMenuAt(widget, NULL, gfx::Rect(location_, gfx::Size()),

[dhollowa, 2012/10/01 17:37:38]

Prefer: RunResult result = menu_runner_->RunMenuAt(...);
  DCHECK_EQ(result, MENU_DELETED);
Maybe with comment as to why you expect this.

[jeremycho, 2012/10/01 22:53:40]

We don't expect that necessarily - only if the MenuRunner was deleted.  This
idiom is used throughout, e.g.:
http://code.google.com/codesearch#OAMlx_jo-ck/src/ash/shell_context_menu.cc&e...

+                              views::MenuItemView::TOPLEFT, 0) ==
+      views::MenuRunner::MENU_DELETED)
+    return;
+}
+
+bool OtherDeviceMenu::IsCommandIdChecked(int command_id) const {
+  return false;
+}
+
+bool OtherDeviceMenu::IsCommandIdEnabled(int command_id) const {
+  return true;
+}
+
+void OtherDeviceMenu::ExecuteCommand(int command_id) {
+  ExecuteCommand(command_id, 0);
+}
+
+// TODO(jeremycho): Figure out why mouse wheel clicks don't trigger this.
+void OtherDeviceMenu::ExecuteCommand(int command_id, int event_flags) {
+  if (session_data_.count(command_id) == 0) {

[dhollowa, 2012/10/01 17:37:38]

Chrome style typically does not mask logic errors with |return| like you have
here.  We prefer to DCHECK and crash so we see things turn up in the crash logs,
and we avoid silent failures.  So, this should be simply:

DCHECK_EQ(session_data_.count(command_id), 0) << "Invalid...";

[jeremycho, 2012/10/01 22:53:40]

Done.

+    NOTREACHED() << "Invalid command_id from other device menu.";
+    return;
+  }
+
+  linked_ptr<DictionaryValue> tab_data = session_data_[command_id];
+  // This is not a mistake - sessionId actually refers to the tab id.
+  // See ForeignSessionHandler::SessionTabToValue.
+  int tab_id = ForeignSessionHandler::kInvalidId;
+  tab_data->GetInteger("sessionId", &tab_id);
+
+  int window_id = ForeignSessionHandler::kInvalidId;
+  tab_data->GetInteger("windowId", &window_id);
+
+  WindowOpenDisposition disposition =
+      chrome::DispositionFromEventFlags(event_flags);
+  ForeignSessionHandler::OpenForeignSession(
+      web_ui_, session_id_, window_id, tab_id, disposition);
+  // TODO(jeremycho): Figure out what we want to log.

[dhollowa, 2012/10/01 17:37:38]

If in doubt, log nothing.

[jeremycho, 2012/10/01 22:53:40]

other_sessions.js logs session clicks, so most likely we will want to as well,
but we may want additional data too (e.g. tab index).

+}
+
+bool OtherDeviceMenu::GetAcceleratorForCommandId(
+      int command_id,
+      ui::Accelerator* accelerator) {
+  return false;
+}