|
|
Created:
5 years, 5 months ago by Matt Giuca Modified:
5 years, 4 months ago CC:
chromium-reviews, tfarina, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@exclusiveaccess-remove-confirmation Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionChange exclusive access popup behaviour with simplified-fullscreen-ui.
With the simplified-fullscreen-ui flag enabled, the popup behaves as
follows:
- Now waits for a short period (500ms) of inactivity, then more activity,
before displaying the initial bubble.
- The popup remains active for 2.3s, then hides as long as the user is
not mousing over the top part of the screen.
- After a long period (1m) of inactivity, then more activity, displays
the bubble again.
Design document:
https://docs.google.com/document/d/10Hm6dstWEY9sOdbENijz70I6Tnbz91mPkNKeteMJ-Kw/edit#
BUG=352425
Committed: https://crrev.com/e145b91dd677b4cbb5b04307c61b79ada06a9f87
Cr-Commit-Position: refs/heads/master@{#342058}
Patch Set 1 #Patch Set 2 : Remove unnecessary timer. #Patch Set 3 : Put all the new functionality behind simplified-fullscreen-ui flag. No changes by default. #Patch Set 4 : Don't start hide_timeout_ on startup in simplified. #Patch Set 5 : Minor. #
Total comments: 26
Patch Set 6 : Respond to reviews. #
Total comments: 6
Patch Set 7 : Respond to nits. #
Depends on Patchset: Messages
Total messages: 19 (5 generated)
palmer@chromium.org changed reviewers: + palmer@chromium.org
I get a build error: obj/chrome/browser_ui.gen/ui_localizer_table.h:184:32: error: use of undeclared identifier 'IDS_FULLSCREEN_ALLOW' { "^IDS_FULLSCREEN_ALLOW", IDS_FULLSCREEN_ALLOW, 0 }, ^ obj/chrome/browser_ui.gen/ui_localizer_table.h:275:40: error: no matching function for call to 'ArraySizeHelper' static const size_t kUIResourcesSize = arraysize(kUIResources); ^~~~~~~~~~~~~~~~~~~~~~~ ../../base/macros.h:56:34: note: expanded from macro 'arraysize' #define arraysize(array) (sizeof(ArraySizeHelper(array))) ^~~~~~~~~~~~~~~ ../../base/macros.h:55:40: note: candidate template ignored: could not match 'T [N]' against 'const UILocalizerResourceMap []' template <typename T, size_t N> char (&ArraySizeHelper(T (&array)[N]))[N]; ^
That's a bizarre error. I didn't touch IDS_FULLSCREEN_ALLOW and certainly don't know why arraysize has issues. Perhaps there were some non-fatal patching issues. Hold tight, I'm going to change this CL that makes this change behind a flag (and therefore isn't deleting so much stuff).
mgiuca@chromium.org changed reviewers: + msw@chromium.org, scheib@chromium.org
Updated to use the flag; should not change any behaviour when the flag is disabled. scheib@chromium.org: Please review changes in exclusive_access_bubble.*. msw@chromium.org: Please review changes in exclusive_access_bubble_views.cc.
The behavioral changes seem correct, but this code is not as straightforward as it could be. I made some minor comments, and you might want to rethink the timer names in general, but mainly I hope that once we switch to the 'simplified' behavior we can actually simplify the code a bit more... Please also fix the CL description's "500s" to "500ms". https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc (right): https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:28: const int ExclusiveAccessBubble::kInitialDebounceTimeMs = 500; nit: rename this |kInitialNotifyTimeMs| to match |kRenotifyTimeMs| https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:44: // time elapses, we will notify the user upon the next mouse/keyboard input. nit: no keyboard... https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:56: // Start the initial delay timer and begin watching the mouse. nit: update comment ('initial delay')? https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:119: // Do not aggressively show the notification again until a long time has nit: remove 'aggressively', maybe rephrase to something like "Wait a long time before again showing the notification regardless of mouse position." https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/exclusive_access_bubble.h (right): https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:110: // When this timer has elapsed, we will aggressively notify the user about What's aggressive about this? Remove 'aggressive' from the comment & identifier. Maybe say below "on the next mouse input, regardless of the mouse location". https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:111: // currently active exclusive access (on the next mouse/keyboard input). While nit: no keyboard...
D'oh, couple more comments. https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/exclusive_access_bubble_views.cc:269: double initial_value = nit: maybe add a comment here https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/exclusive_access_bubble_views.cc:304: popup_->Hide(); // Keep hidden by default. nit: can you just avoid the ShowInactive instead of calling that and then calling Hide?
https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/exclusive_access_bubble.h (right): https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:37: static const int kRenotifyTimeMs; kRenotifyTimeM -> kSnoozeNotificationsTimeMs And comment: // Time to wait and suppress notifications because we have recently // shown one and expect the user remembers. Notifications will not be // shown again until this amount of time has passed and then the user // interacts in a way that causes a notification to be shown. Perhaps make move this explanation up to a larger design comment at top of file, and have references, "See Notification Display Design Note above." here and below. https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:112: // the timer is ticking, we do not notify unless the user mouses to the top of Mouse location should have no impact on notification display. Explanation added to design doc: https://docs.google.com/document/d/10Hm6dstWEY9sOdbENijz70I6Tnbz91mPkNKeteMJ-...
Patchset #6 (id:100001) has been deleted
https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc (right): https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:28: const int ExclusiveAccessBubble::kInitialDebounceTimeMs = 500; Since scheib told me to rename kRenotifyTimeMs, I'm calling this kDebounceNotificationsTimeMs (to match kSnoozeNotificationsTimeMs). https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:44: // time elapses, we will notify the user upon the next mouse/keyboard input. On 2015/08/04 18:21:51, msw wrote: > nit: no keyboard... Done. https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:56: // Start the initial delay timer and begin watching the mouse. I think this comment is OK (we are using hide_timeout_ as an initial delay timer in this context). https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:119: // Do not aggressively show the notification again until a long time has On 2015/08/04 18:21:50, msw wrote: > nit: remove 'aggressively', maybe rephrase to something like "Wait a long time > before again showing the notification regardless of mouse position." Done. https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/exclusive_access_bubble.h (right): https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:37: static const int kRenotifyTimeMs; On 2015/08/04 19:36:28, scheib wrote: > kRenotifyTimeM -> kSnoozeNotificationsTimeMs > And comment: > // Time to wait and suppress notifications because we have recently > // shown one and expect the user remembers. Notifications will not be > // shown again until this amount of time has passed and then the user > // interacts in a way that causes a notification to be shown. > > Perhaps make move this explanation up to a larger design comment at top of file, > and have references, "See Notification Display Design Note above." here and > below. Done. https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:110: // When this timer has elapsed, we will aggressively notify the user about On 2015/08/04 18:21:51, msw wrote: > What's aggressive about this? Remove 'aggressive' from the comment & identifier. > Maybe say below "on the next mouse input, regardless of the mouse location". Done. (And renamed to suppress_notify_timeout_). https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:111: // currently active exclusive access (on the next mouse/keyboard input). While On 2015/08/04 18:21:51, msw wrote: > nit: no keyboard... Done. https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:112: // the timer is ticking, we do not notify unless the user mouses to the top of On 2015/08/04 19:36:28, scheib wrote: > Mouse location should have no impact on notification display. Explanation added > to design doc: > https://docs.google.com/document/d/10Hm6dstWEY9sOdbENijz70I6Tnbz91mPkNKeteMJ-... Done. https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/exclusive_access_bubble_views.cc:269: double initial_value = On 2015/08/04 18:23:57, msw wrote: > nit: maybe add a comment here Done. https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/exclusive_access_bubble_views.cc:304: popup_->Hide(); // Keep hidden by default. On 2015/08/04 18:23:57, msw wrote: > nit: can you just avoid the ShowInactive instead of calling that and then > calling Hide? Done.
https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/exclusive_access_bubble.h (right): https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:112: // the timer is ticking, we do not notify unless the user mouses to the top of Oh sorry, by "Done" I mean "not done" (just updated comment). Current behaviour is mouse to top will re-show the bubble. I'll change this but I want to do this incrementally and not make too big of a CL here. Mind if I postpone this?
LGTM, optional changes on this patch or future: https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/exclusive_access_bubble.h (right): https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:37: static const int kRenotifyTimeMs; On 2015/08/05 03:27:20, Matt Giuca wrote: > On 2015/08/04 19:36:28, scheib wrote: > > kRenotifyTimeM -> kSnoozeNotificationsTimeMs > > And comment: > > // Time to wait and suppress notifications because we have recently > > // shown one and expect the user remembers. Notifications will not be > > // shown again until this amount of time has passed and then the user > > // interacts in a way that causes a notification to be shown. > > > > Perhaps make move this explanation up to a larger design comment at top of > file, > > and have references, "See Notification Display Design Note above." here and > > below. > > Done. Thanks. OK to leave like this, but BTW I think it's OK to move entire comment contents here (and below for the timer) to the note at the top and just have a brief // See "Design Note" above. https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:111: // currently active exclusive access (on the next mouse/keyboard input). While On 2015/08/05 03:27:20, Matt Giuca wrote: > On 2015/08/04 18:21:51, msw wrote: > > nit: no keyboard... > > Done. Long term we'd want keyboard too, though, right? https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:112: // the timer is ticking, we do not notify unless the user mouses to the top of On 2015/08/05 03:35:08, Matt Giuca wrote: > Oh sorry, by "Done" I mean "not done" (just updated comment). > > Current behaviour is mouse to top will re-show the bubble. I'll change this but > I want to do this incrementally and not make too big of a CL here. Mind if I > postpone this? Acknowledged. Follow patch is fine. https://codereview.chromium.org/1254543002/diff/120001/chrome/browser/ui/excl... File chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc (right): https://codereview.chromium.org/1254543002/diff/120001/chrome/browser/ui/excl... chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:29: const int ExclusiveAccessBubble::kSnoozeNotificationsTimeMs = 60000; // 1m. 1 minute is too small. We had a previous email thread where we discussed much larger times, in range of 10 min to 1 hour. I'm OK landing this as is, or if you modify on this patch, but for the real feature this would be too annoying when giving a presentation.
lgtm with nits. Please don't remove comments when replying inline. https://codereview.chromium.org/1254543002/diff/120001/chrome/browser/ui/excl... File chrome/browser/ui/exclusive_access/exclusive_access_bubble.h (right): https://codereview.chromium.org/1254543002/diff/120001/chrome/browser/ui/excl... chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:43: // Time before showing initial message (simplified fullscreen UI). nit: "showing the initial" https://codereview.chromium.org/1254543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/1254543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/exclusive_access_bubble_views.cc:269: // With the simplified fullscreen UI flag, initially hidden. In the normal nit: "Hide the bubble initially with the simplified fullscreen UI flag; otherwise make the bubble initially visible."
https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/exclusive_access_bubble.h (right): https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:37: static const int kRenotifyTimeMs; On 2015/08/05 05:24:19, scheib wrote: > On 2015/08/05 03:27:20, Matt Giuca wrote: > > On 2015/08/04 19:36:28, scheib wrote: > > > kRenotifyTimeM -> kSnoozeNotificationsTimeMs > > > And comment: > > > // Time to wait and suppress notifications because we have recently > > > // shown one and expect the user remembers. Notifications will not be > > > // shown again until this amount of time has passed and then the user > > > // interacts in a way that causes a notification to be shown. > > > > > > Perhaps make move this explanation up to a larger design comment at top of > > file, > > > and have references, "See Notification Display Design Note above." here and > > > below. > > > > Done. > > Thanks. OK to leave like this, but BTW I think it's OK to move entire comment > contents here (and below for the timer) to the note at the top and just have a > brief // See "Design Note" above. Done. https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:111: // currently active exclusive access (on the next mouse/keyboard input). While On 2015/08/05 05:24:19, scheib wrote: > Long term we'd want keyboard too, though, right? Yeah. I originally had that comment there because that was the plan, but I think msw is right and we want the comments to accurately reflect the current behaviour. https://codereview.chromium.org/1254543002/diff/120001/chrome/browser/ui/excl... File chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc (right): https://codereview.chromium.org/1254543002/diff/120001/chrome/browser/ui/excl... chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:29: const int ExclusiveAccessBubble::kSnoozeNotificationsTimeMs = 60000; // 1m. On 2015/08/05 05:24:19, scheib wrote: > 1 minute is too small. We had a previous email thread where we discussed much > larger times, in range of 10 min to 1 hour. I'm OK landing this as is, or if you > modify on this patch, but for the real feature this would be too annoying when > giving a presentation. OK let's go 1 hour. https://codereview.chromium.org/1254543002/diff/120001/chrome/browser/ui/excl... File chrome/browser/ui/exclusive_access/exclusive_access_bubble.h (right): https://codereview.chromium.org/1254543002/diff/120001/chrome/browser/ui/excl... chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:43: // Time before showing initial message (simplified fullscreen UI). On 2015/08/05 17:29:55, msw wrote: > nit: "showing the initial" N/A https://codereview.chromium.org/1254543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/1254543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/exclusive_access_bubble_views.cc:269: // With the simplified fullscreen UI flag, initially hidden. In the normal On 2015/08/05 17:29:55, msw wrote: > nit: "Hide the bubble initially with the simplified fullscreen UI flag; > otherwise make the bubble initially visible." Done~ish.
The CQ bit was checked by mgiuca@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from scheib@chromium.org, msw@chromium.org Link to the patchset: https://codereview.chromium.org/1254543002/#ps140001 (title: "Respond to nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1254543002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1254543002/140001
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/e145b91dd677b4cbb5b04307c61b79ada06a9f87 Cr-Commit-Position: refs/heads/master@{#342058} |