Added notification when user presses backspace to use new shortcut.

chrome/browser/ui/views/subtle_notification_view.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/subtle_notification_view.h"
 
 #include <memory>
 
 #include "base/strings/string_split.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 10 matching lines @@
 
 // Space between the site info label and the link.
 const int kMiddlePaddingPx = 30;
 
 const int kOuterPaddingHorizPx = 40;
 const int kOuterPaddingVertPx = 8;
 
 // Partially-transparent background color.
 const SkColor kBackgroundColor = SkColorSetARGB(0xcc, 0x28, 0x2c, 0x32);
 
+// Spacing around the key name.
+const int kKeyNameMarginHorizPx = 7;
+const int kKeyNameBorderPx = 1;
+const int kKeyNameCornerRadius = 2;
+const int kKeyNamePaddingPx = 5;
+
 }  // namespace
 
 // Class containing the instruction text. Contains fancy styling on the keyboard
 // key (not just a simple label).
 class SubtleNotificationView::InstructionView : public views::View {
  public:
-  // Creates an InstructionView with specific text. |text| may contain a single
-  // segment delimited by a pair of pipes ('|'); this segment will be displayed
-  // as a keyboard key. e.g., "Press |Esc| to exit" will have "Esc" rendered as
-  // a key.
+  // Creates an InstructionView with specific text. |text| may contain one or
+  // more segments delimited by a pair of pipes ('|'); each of these segments
+  // will be displayed as a keyboard key. e.g., "Press |Alt|+|Q| to exit" will
+  // have "Alt" and "Q" rendered as keys.
   InstructionView(const base::string16& text,
                   const gfx::FontList& font_list,
                   SkColor foreground_color,
                   SkColor background_color);
 
   void SetText(const base::string16& text);
 
  private:
-  views::Label* before_key_;
-  views::Label* key_name_label_;
-  views::View* key_name_;
-  views::Label* after_key_;
+  void AddLabelSegment(const base::string16& text);
+  void AddKeyNameSegment(const base::string16& text);
+
+  const gfx::FontList& font_list_;
+  SkColor foreground_color_;
+  SkColor background_color_;
+
+  base::string16 text_;
 
   DISALLOW_COPY_AND_ASSIGN(InstructionView);
 };
 
 SubtleNotificationView::InstructionView::InstructionView(
     const base::string16& text,
     const gfx::FontList& font_list,
     SkColor foreground_color,
-    SkColor background_color) {
-  // Spacing around the key name.
-  const int kKeyNameMarginHorizPx = 7;
-  const int kKeyNameBorderPx = 1;
-  const int kKeyNameCornerRadius = 2;
-  const int kKeyNamePaddingPx = 5;
-
+    SkColor background_color)
+    : font_list_(font_list),
+      foreground_color_(foreground_color),
+      background_color_(background_color) {
   // The |between_child_spacing| is the horizontal margin of the key name.
   views::BoxLayout* layout = new views::BoxLayout(views::BoxLayout::kHorizontal,
                                                   0, 0, kKeyNameMarginHorizPx);
   SetLayoutManager(layout);
 
-  before_key_ = new views::Label(base::string16(), font_list);
-  before_key_->SetEnabledColor(foreground_color);
-  before_key_->SetBackgroundColor(background_color);
-  AddChildView(before_key_);
-
-  key_name_label_ = new views::Label(base::string16(), font_list);
-  key_name_label_->SetEnabledColor(foreground_color);
-  key_name_label_->SetBackgroundColor(background_color);
-
-  key_name_ = new views::View;
-  views::BoxLayout* key_name_layout = new views::BoxLayout(
-      views::BoxLayout::kHorizontal, kKeyNamePaddingPx, 0, 0);
-  key_name_layout->set_minimum_cross_axis_size(
-      key_name_label_->GetPreferredSize().height() + kKeyNamePaddingPx * 2);
-  key_name_->SetLayoutManager(key_name_layout);
-  key_name_->AddChildView(key_name_label_);
-  // The key name has a border around it.
-  std::unique_ptr<views::Border> border(views::Border::CreateRoundedRectBorder(
-      kKeyNameBorderPx, kKeyNameCornerRadius, foreground_color));
-  key_name_->SetBorder(std::move(border));
-  AddChildView(key_name_);
-
-  after_key_ = new views::Label(base::string16(), font_list);
-  after_key_->SetEnabledColor(foreground_color);
-  after_key_->SetBackgroundColor(background_color);
-  AddChildView(after_key_);
-
   SetText(text);
 }
 
 void SubtleNotificationView::InstructionView::SetText(
     const base::string16& text) {
+  // Avoid replacing the contents with the same text.
+  if (text == text_)
+    return;
+
+  RemoveAllChildViews(true);
+
   // Parse |text|, looking for pipe-delimited segment.

[Peter Kasting, 2016/05/19 10:08:37]

Don't do things this way -- see my comments in your separate strings CL.  Just
remove the pipes, ditch the parsing code, and ask GetStringFUTF16() to tell you
the offsets of the replacement points.

[Matt Giuca, 2016/05/19 10:33:59]

See my response there. Pipes may not have been the right decision, but it's the
way we've always done it and changing it would mean changing all of the
exclusive access strings as well. Possibly worth doing, but not right now.

[Peter Kasting, 2016/05/19 19:45:08]

I forgot this is in the common code, not in the backspace-specific code, and so
it would affect other strings.  Not changing this now is fine.  See my reply
there.

Can you file this and get back to it post-branch?

[Matt Giuca, 2016/05/19 22:11:47]

https://bugs.chromium.org/p/chromium/issues/detail?id=613338

   std::vector<base::string16> segments =
       base::SplitString(text, base::ASCIIToUTF16("|"), base::TRIM_WHITESPACE,
                         base::SPLIT_WANT_ALL);
-  // Expect 1 or 3 pieces (either no pipe-delimited segments, or one).
-  DCHECK(segments.size() <= 1 || segments.size() == 3);
+  // Expect an odd number of pieces (from an even number of pipes), or none.
+  DCHECK(segments.empty() || segments.size() % 2 == 1);

[Peter Kasting, 2016/05/19 10:08:37]

This check isn't right.

If the localizers localize as e.g. "To go back press |$1| + |$2|", you'll get an
even number of segments, but it's not a bug.

[Matt Giuca, 2016/05/19 10:33:59]

SplitString does the right thing here: empty pipe-delimited segments become
empty strings, so your example becomes {"To go back press ", "$1", " + ", "$2",
""}.

(However, SplitString as a special dumb case does return {} when the input is
the empty string, not {""}, which is why I have to check segments.empty().)

[Peter Kasting, 2016/05/19 19:45:08]

Hmm... this maybe deserves more comments to avoid confusing people like me?

[Matt Giuca, 2016/05/23 04:22:57]

Done.

 
-  before_key_->SetText(segments.size() ? segments[0] : base::string16());
-
-  if (segments.size() < 3) {
-    key_name_->SetVisible(false);
-    after_key_->SetVisible(false);
-    return;
+  // Add text segment, alternating between non-key (no border) and key (border)
+  // formatting.
+  bool format_as_key = false;
+  for (const auto& segment : segments) {
+    if (format_as_key)
+      AddKeyNameSegment(segment);
+    else
+      AddLabelSegment(segment);

[Peter Kasting, 2016/05/19 10:08:37]

Nit: AddLabelSegment() is almost a pure subset of AddKeyNameSegment().  I would
collapse these two functions into one function that takes a bool about what to
do, which will also allow you to remove this conditional.

[Matt Giuca, 2016/05/23 04:22:57]

Done.

+    format_as_key = !format_as_key;
   }
 
-  before_key_->SetText(segments[0]);
-  key_name_label_->SetText(segments[1]);
-  key_name_->SetVisible(true);
-  after_key_->SetVisible(true);
-  after_key_->SetText(segments[2]);
+  text_ = text;
+}
+
+void SubtleNotificationView::InstructionView::AddLabelSegment(
+    const base::string16& text) {
+  views::Label* label = new views::Label(text, font_list_);
+  label->SetEnabledColor(foreground_color_);
+  label->SetBackgroundColor(background_color_);
+  AddChildView(label);
+}
+
+void SubtleNotificationView::InstructionView::AddKeyNameSegment(
+    const base::string16& text) {
+  views::Label* label = new views::Label(text, font_list_);
+  label->SetEnabledColor(foreground_color_);
+  label->SetBackgroundColor(background_color_);
+
+  views::View* key = new views::View;
+  views::BoxLayout* key_name_layout = new views::BoxLayout(
+      views::BoxLayout::kHorizontal, kKeyNamePaddingPx, 0, 0);
+  key_name_layout->set_minimum_cross_axis_size(
+      label->GetPreferredSize().height() + kKeyNamePaddingPx * 2);
+  key->SetLayoutManager(key_name_layout);
+  key->AddChildView(label);
+  // The key name has a border around it.
+  std::unique_ptr<views::Border> border(views::Border::CreateRoundedRectBorder(
+      kKeyNameBorderPx, kKeyNameCornerRadius, foreground_color_));
+  key->SetBorder(std::move(border));
+  AddChildView(key);
 }
 
 SubtleNotificationView::SubtleNotificationView(
     views::LinkListener* link_listener)
     : instruction_view_(nullptr), link_(nullptr) {
   const SkColor kForegroundColor = SK_ColorWHITE;
 
   std::unique_ptr<views::BubbleBorder> bubble_border(new views::BubbleBorder(
       views::BubbleBorder::NONE, views::BubbleBorder::NO_ASSETS,
       kBackgroundColor));
@@ skipping 55 matching lines @@
   // We set layout manager to nullptr to prevent the widget from sizing its
   // contents to the same size as itself. This prevents the widget contents from
   // shrinking while we animate the height of the popup to give the impression
   // that it is sliding off the top of the screen.
   // TODO(mgiuca): This probably isn't necessary now that there is no slide
   // animation. Remove it.
   popup->GetRootView()->SetLayoutManager(nullptr);
 
   return popup;
 }