Added notification when user presses backspace to use new shortcut.

chrome/browser/ui/views/new_back_shortcut_bubble.cc

+// Copyright 2016 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/views/new_back_shortcut_bubble.h"
+
+#include <utility>
+
+#include "base/message_loop/message_loop.h"
+#include "base/strings/utf_string_conversions.h"
+#include "chrome/browser/ui/views/exclusive_access_bubble_views_context.h"
+#include "chrome/browser/ui/views/subtle_notification_view.h"
+#include "chrome/grit/generated_resources.h"
+#include "ui/base/l10n/l10n_util.h"
+#include "ui/gfx/animation/animation.h"
+#include "ui/gfx/animation/slide_animation.h"
+#include "ui/gfx/geometry/rect.h"
+#include "ui/gfx/geometry/size.h"
+#include "ui/strings/grit/ui_strings.h"
+#include "ui/views/border.h"
+#include "ui/views/view.h"
+#include "ui/views/widget/widget.h"
+
+namespace {
+
+const int kPopupTopPx = 45;
+const int kSlideInDurationMs = 350;
+const int kSlideOutDurationMs = 700;
+const int kShowDurationMs = 3800;
+
+}
+
+NewBackShortcutBubble::NewBackShortcutBubble(
+    ExclusiveAccessBubbleViewsContext* context,
+    bool forward)
+    : bubble_view_context_(context),
+      popup_(nullptr),
+      animation_(new gfx::SlideAnimation(this)) {
+  // Initially hide the bubble.
+  double initial_value = 0;
+  animation_->Reset(initial_value);

[Peter Kasting, 2016/05/24 23:35:08]

Nit: AFAICT this block does nothing, as animations are always created in the 0
state.  I would remove this entirely to avoid making callers think this is
functional.

[Matt Giuca, 2016/05/26 04:37:35]

Done.

+
+  // Create the contents view.
+  view_ = new SubtleNotificationView(nullptr);

[Peter Kasting, 2016/05/24 23:35:08]

Nit: Can this be in the initializer list?

[Matt Giuca, 2016/05/26 04:37:35]

Done.

+
+  // Initialize the popup.
+  popup_ = SubtleNotificationView::CreatePopupWidget(
+      bubble_view_context_->GetBubbleParentView(), view_, false);

[Peter Kasting, 2016/05/24 23:35:08]

Nit: If declared after |view_|, could also be in the initializer list, allowing
making this pointer const as well.

[Matt Giuca, 2016/05/26 04:37:35]

Done.

+
+  UpdateContent(forward);
+}
+
+NewBackShortcutBubble::~NewBackShortcutBubble() {
+  // Must delete the widget asynchronously. See rationale in
+  // ~ExclusiveAccessBubbleViews.

[Peter Kasting, 2016/05/24 23:35:09]

Nit: It sounded like you weren't sure whether or not this rationale actually
applied, and wanted to be safe -- if so I might indicate that uncertainty here
(in place of "must").

[Matt Giuca, 2016/05/26 04:37:35]

Done.

+  popup_->Close();
+  base::MessageLoop::current()->DeleteSoon(FROM_HERE, popup_);
+}
+
+void NewBackShortcutBubble::UpdateContent(bool forward) {
+  // Note: The key names are parameters so that we can vary by operating system
+  // or change the direction of the arrow as necessary (see
+  // https://crbug.com/612685).
+
+#if defined(OS_MACOSX)
+  // U+2318 = PLACE OF INTEREST SIGN (Mac Command symbol).
+  base::string16 accelerator = base::WideToUTF16(L"\x2318");
+#else
+  base::string16 accelerator = l10n_util::GetStringUTF16(IDS_APP_ALT_KEY);
+#endif
+
+  int message_id = forward ? IDS_PRESS_ALT_RIGHT_TO_GO_FORWARD
+                           : IDS_PRESS_ALT_LEFT_TO_GO_BACK;
+  // U+2192 = RIGHTWARDS ARROW; U+2190 = LEFTWARDS ARROW.
+  base::string16 arrow_key = base::WideToUTF16(forward ? L"\x2192" : L"\x2190");
+  view_->UpdateContent(
+      l10n_util::GetStringFUTF16(message_id, accelerator, arrow_key),
+      base::string16());
+
+  view_->SetSize(GetPopupRect(true).size());
+  // Bounds are in screen coordinates.

[Peter Kasting, 2016/05/24 23:35:09]

Nit: It's a little unclear to me why this comment is here, as it doesn't seem to
be explaining anything local (e.g. there's no local -> screen coordinate
transform occurring).

[Matt Giuca, 2016/05/26 04:37:35]

Done.

+  popup_->SetBounds(GetPopupRect(false));
+  Show();
+}
+
+void NewBackShortcutBubble::AnimationProgressed(
+    const gfx::Animation* animation) {
+  int opacity = animation_->CurrentValueBetween(0, 255);
+  if (opacity == 0) {
+    popup_->Hide();
+  } else {
+    popup_->Show();

[Peter Kasting, 2016/05/24 23:35:09]

Nit: It looks like maybe Widget::Show() is complicated?  Would it be beneficial
to only call Show() when we are currently hidden?

[Matt Giuca, 2016/05/26 04:37:35]

Done.

+    popup_->SetOpacity(opacity);
+  }
+}
+
+void NewBackShortcutBubble::AnimationEnded(const gfx::Animation* animation) {
+  AnimationProgressed(animation);
+}
+
+gfx::Rect NewBackShortcutBubble::GetPopupRect(
+    bool ignore_animation_state) const {
+  gfx::Size size = view_->GetPreferredSize();
+  gfx::Rect widget_bounds = bubble_view_context_->GetClientAreaBoundsInScreen();
+  int x = widget_bounds.x() + (widget_bounds.width() - size.width()) / 2;
+  int top_container_bottom = widget_bounds.y();

[Peter Kasting, 2016/05/24 23:35:08]

Nit: I might inline this particular variable into the expression below.

[Matt Giuca, 2016/05/26 04:37:35]

Done.

+  // |desired_top| is the top of the bubble area including the shadow.
+  int desired_top = kPopupTopPx - view_->border()->GetInsets().top();
+  int y = top_container_bottom + desired_top;
+  return gfx::Rect(gfx::Point(x, y), size);
+}
+
+void NewBackShortcutBubble::Hide() {

[Peter Kasting, 2016/05/24 23:35:08]

Nit: AFAICT both Hide() and Show() are called in only one place, and both are
relatively simple and straightforward, so I might just inline them into their
callers.

[Matt Giuca, 2016/05/26 04:37:35]

Done.

+  animation_->SetSlideDuration(kSlideOutDurationMs);
+  animation_->Hide();
+}
+
+void NewBackShortcutBubble::Show() {
+  animation_->SetSlideDuration(kSlideInDurationMs);
+  animation_->Show();
+
+  // Wait a few seconds before hiding.
+  hide_timeout_.Start(FROM_HERE,
+                      base::TimeDelta::FromMilliseconds(kShowDurationMs), this,
+                      &NewBackShortcutBubble::OnTimerElapsed);
+}
+
+void NewBackShortcutBubble::OnTimerElapsed() {
+  Hide();
+}