Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(180)

Issue 1983803002: Added notification when user presses backspace to use new shortcut. (Closed)

Created:
4 years, 7 months ago by Matt Giuca
Modified:
4 years, 7 months ago
CC:
chrome-apps-syd-reviews_chromium.org, chromium-reviews, tfarina, Alexei Svitkine (slow)
Base URL:
https://chromium.googlesource.com/chromium/src.git@subtle-notification-view-refactor
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Added notification when user presses backspace to use new shortcut. This adds a new type of notification bubble (based on the "Press Esc to exit fullscreen" notification) that teaches users the new shortcuts for Back and Forward when they press Back or Shift+Back. Only implemented on Views systems (Windows, Linux, Chrome OS). BUG=610039 Committed: https://crrev.com/7f3ddc6b70a09c7ae3bcd0d45007e415857fd09c Cr-Commit-Position: refs/heads/master@{#396138}

Patch Set 1 #

Patch Set 2 : Rebase. #

Patch Set 3 : Hook up to the backspace button. Reset fullscreen back to normal. #

Patch Set 4 : Backspace and exclusive access bubbles cancel each other out, to avoid overlap. #

Patch Set 5 : Internationalize strings. #

Patch Set 6 : SubtleNotificationView: Rewrite view generation code to allow any number of key name segments. #

Patch Set 7 : Avoid buffer overrun if dcheck fails. #

Patch Set 8 : Choose the exact key shortcut name in code, not baked into the string. #

Patch Set 9 : Massive cleanup; stop depending on Exclusive Access system. #

Patch Set 10 : Implement ShowBackspaceNewShortcutBubble in all subclasses of BrowserWindow. #

Patch Set 11 : Rebase. #

Patch Set 12 : Change strings so there are 2 separate placeholders. #

Patch Set 13 : Rebase on 1995893002 (strings to a separate CL). #

Patch Set 14 : Rebase on 1992003002 (backspace-goes-back behind a field trial). #

Patch Set 15 : Fix merge error. #

Patch Set 16 : Avoid deleting and replacing same contents (work around invisible text bug). #

Patch Set 17 : Use UTF-16 instead of many UTF-8 conversions. #

Total comments: 53

Patch Set 18 : Fix bubble sometimes not hiding after timeout. #

Patch Set 19 : Rebase. #

Patch Set 20 : Rename BackspaceNewShortcutBubble to NewBackShortcutBubble. #

Patch Set 21 : Respond to pkasting review. #

Total comments: 26

Patch Set 22 : Respond to pkasting review post-LGTM. #

Patch Set 23 : Fix alphabetical order in chrome_browser_ui.gypi. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+297 lines, -56 lines) Patch
M chrome/app/chrome_command_ids.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/ui/browser_command_controller.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +8 lines, -2 lines 0 comments Download
M chrome/browser/ui/browser_window.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/ui/cocoa/browser_window_cocoa.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/cocoa/browser_window_cocoa.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +5 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/frame/browser_view.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 3 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/frame/browser_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 3 chunks +18 lines, -1 line 0 comments Download
A chrome/browser/ui/views/new_back_shortcut_bubble.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +66 lines, -0 lines 0 comments Download
A chrome/browser/ui/views/new_back_shortcut_bubble.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 1 chunk +118 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/subtle_notification_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 3 chunks +64 lines, -53 lines 0 comments Download
M chrome/chrome_browser_ui.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/test/base/test_browser_window.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -0 lines 0 comments Download
M chrome/test/base/test_browser_window.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +2 lines, -0 lines 0 comments Download

Dependent Patchsets:

Messages

Total messages: 40 (15 generated)
Matt Giuca
Hi Peter, I think I've got this into a reasonable state now. Note that this ...
4 years, 7 months ago (2016-05-18 09:00:54 UTC) #3
Matt Giuca
Patch failure ... not much I can do about this tonight :( I'll rebase in ...
4 years, 7 months ago (2016-05-18 09:01:23 UTC) #4
Matt Giuca
grt: chrome_command_ids phajdan.jr: test_browser_window asvitkine: histograms (All pretty minor.)
4 years, 7 months ago (2016-05-18 09:03:18 UTC) #6
Paweł Hajdan Jr.
LGTM
4 years, 7 months ago (2016-05-18 15:00:27 UTC) #7
grt (UTC plus 2)
chrome/app/chrome_command_ids.h lgtm
4 years, 7 months ago (2016-05-18 15:02:23 UTC) #8
Matt Giuca
asvitkine: histograms owners. Thanks! (Not sure why I didn't manage to add you yesterday.)
4 years, 7 months ago (2016-05-19 00:38:02 UTC) #10
Matt Giuca
asvitkine: Actually, I rebased it on 1992003002 and histograms no longer needs to change. (Not ...
4 years, 7 months ago (2016-05-19 02:04:16 UTC) #13
Matt Giuca
pkasting: I've done a bunch more work on this. Somewhat disturbing: I found a bug ...
4 years, 7 months ago (2016-05-19 03:27:06 UTC) #15
Peter Kasting
On 2016/05/19 03:27:06, Matt Giuca wrote: > Somewhat disturbing: I found a bug in the ...
4 years, 7 months ago (2016-05-19 09:37:00 UTC) #16
Peter Kasting
https://codereview.chromium.org/1983803002/diff/350001/chrome/browser/ui/browser_window.h File chrome/browser/ui/browser_window.h (right): https://codereview.chromium.org/1983803002/diff/350001/chrome/browser/ui/browser_window.h#newcode150 chrome/browser/ui/browser_window.h:150: // |!forward|) or Forward (if |forward|). Nit: I'd remove ...
4 years, 7 months ago (2016-05-19 10:08:37 UTC) #17
Matt Giuca
Thanks for the detailed review. A few comments below; the rest I agree with and ...
4 years, 7 months ago (2016-05-19 10:33:59 UTC) #18
Peter Kasting
https://codereview.chromium.org/1983803002/diff/350001/chrome/browser/ui/views/subtle_notification_view.cc File chrome/browser/ui/views/subtle_notification_view.cc (right): https://codereview.chromium.org/1983803002/diff/350001/chrome/browser/ui/views/subtle_notification_view.cc#newcode91 chrome/browser/ui/views/subtle_notification_view.cc:91: // Parse |text|, looking for pipe-delimited segment. On 2016/05/19 ...
4 years, 7 months ago (2016-05-19 19:45:08 UTC) #19
Matt Giuca
> Can you file a separate bug for this with whatever detail you > were ...
4 years, 7 months ago (2016-05-19 22:11:47 UTC) #20
Matt Giuca
PTAL. https://codereview.chromium.org/1983803002/diff/350001/chrome/browser/ui/browser_window.h File chrome/browser/ui/browser_window.h (right): https://codereview.chromium.org/1983803002/diff/350001/chrome/browser/ui/browser_window.h#newcode150 chrome/browser/ui/browser_window.h:150: // |!forward|) or Forward (if |forward|). On 2016/05/19 ...
4 years, 7 months ago (2016-05-23 04:22:57 UTC) #21
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1983803002/430001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1983803002/430001
4 years, 7 months ago (2016-05-23 04:23:22 UTC) #23
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 7 months ago (2016-05-23 05:13:21 UTC) #25
Peter Kasting
LGTM https://codereview.chromium.org/1983803002/diff/350001/chrome/browser/ui/views/frame/browser_view.cc File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1983803002/diff/350001/chrome/browser/ui/views/frame/browser_view.cc#newcode1035 chrome/browser/ui/views/frame/browser_view.cc:1035: void BrowserView::ShowBackspaceNewShortcutBubble(bool forward) { On 2016/05/23 04:22:57, Matt ...
4 years, 7 months ago (2016-05-24 23:35:09 UTC) #26
Matt Giuca
https://codereview.chromium.org/1983803002/diff/430001/chrome/browser/ui/views/frame/browser_view.cc File chrome/browser/ui/views/frame/browser_view.cc (right): https://codereview.chromium.org/1983803002/diff/430001/chrome/browser/ui/views/frame/browser_view.cc#newcode1026 chrome/browser/ui/views/frame/browser_view.cc:1026: new_back_shortcut_bubble_.reset(new NewBackShortcutBubble(this, forward)); On 2016/05/24 23:35:08, Peter Kasting wrote: ...
4 years, 7 months ago (2016-05-26 04:37:35 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1983803002/450001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1983803002/450001
4 years, 7 months ago (2016-05-26 04:37:53 UTC) #30
commit-bot: I haz the power
Try jobs failed on following builders: ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/builds/11893) ios-simulator on tryserver.chromium.mac (JOB_FAILED, ...
4 years, 7 months ago (2016-05-26 04:39:54 UTC) #32
Peter Kasting
Have I said thanks for all your work here yet? You have gone way beyond ...
4 years, 7 months ago (2016-05-26 05:19:40 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1983803002/470001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1983803002/470001
4 years, 7 months ago (2016-05-26 06:03:25 UTC) #36
Matt Giuca
> Have I said thanks for all your work here yet? You have gone way ...
4 years, 7 months ago (2016-05-26 06:05:40 UTC) #37
commit-bot: I haz the power
Committed patchset #23 (id:470001)
4 years, 7 months ago (2016-05-26 07:22:34 UTC) #38
commit-bot: I haz the power
4 years, 7 months ago (2016-05-26 07:24:16 UTC) #40
Message was sent while issue was closed.
Patchset 23 (id:??) landed as
https://crrev.com/7f3ddc6b70a09c7ae3bcd0d45007e415857fd09c
Cr-Commit-Position: refs/heads/master@{#396138}

Powered by Google App Engine
This is Rietveld 408576698