Added notification when user presses backspace to use new shortcut.

chrome/browser/ui/views/subtle_notification_view.cc

Index: chrome/browser/ui/views/subtle_notification_view.cc
diff --git a/chrome/browser/ui/views/subtle_notification_view.cc b/chrome/browser/ui/views/subtle_notification_view.cc
index 3957d701f8d0cf9ee285c0fd2760b0d98aab0b6e..17fe89274faecd22d745c39973622fef0a2bccf9 100644
--- a/chrome/browser/ui/views/subtle_notification_view.cc
+++ b/chrome/browser/ui/views/subtle_notification_view.cc
@@ -28,16 +28,22 @@ 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,
@@ -46,10 +52,14 @@ class SubtleNotificationView::InstructionView : public views::View {
   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);
 };
@@ -58,70 +68,73 @@ 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.

+
+  // 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.size() ? segments[0] : base::string16());
+  text_ = text;
+}
 
-  if (segments.size() < 3) {
-    key_name_->SetVisible(false);
-    after_key_->SetVisible(false);
-    return;
-  }
+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);
+}
 
-  before_key_->SetText(segments[0]);
-  key_name_label_->SetText(segments[1]);
-  key_name_->SetVisible(true);
-  after_key_->SetVisible(true);
-  after_key_->SetText(segments[2]);
+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(