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

Issue 1254543002: Change exclusive access popup behaviour with simplified-fullscreen-ui. (Closed)

Created:
5 years, 5 months ago by Matt Giuca
Modified:
5 years, 4 months ago
Reviewers:
palmer, scheib, msw
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.

Description

Change 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. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+70 lines, -10 lines) Patch
M chrome/browser/ui/exclusive_access/exclusive_access_bubble.h View 1 2 3 4 5 6 3 chunks +22 lines, -3 lines 0 comments Download
M chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc View 1 2 3 4 5 6 6 chunks +41 lines, -5 lines 0 comments Download
M chrome/browser/ui/views/exclusive_access_bubble_views.cc View 1 2 3 4 5 6 2 chunks +7 lines, -2 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 19 (5 generated)
palmer
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, ...
5 years, 4 months ago (2015-07-28 03:07:27 UTC) #2
Matt Giuca
That's a bizarre error. I didn't touch IDS_FULLSCREEN_ALLOW and certainly don't know why arraysize has ...
5 years, 4 months ago (2015-07-28 03:27:16 UTC) #3
Matt Giuca
Updated to use the flag; should not change any behaviour when the flag is disabled. ...
5 years, 4 months ago (2015-08-04 04:29:35 UTC) #5
msw
The behavioral changes seem correct, but this code is not as straightforward as it could ...
5 years, 4 months ago (2015-08-04 18:21:51 UTC) #6
msw
D'oh, couple more comments. https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/views/exclusive_access_bubble_views.cc File chrome/browser/ui/views/exclusive_access_bubble_views.cc (right): https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/views/exclusive_access_bubble_views.cc#newcode269 chrome/browser/ui/views/exclusive_access_bubble_views.cc:269: double initial_value = nit: maybe ...
5 years, 4 months ago (2015-08-04 18:23:57 UTC) #7
scheib
https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclusive_access/exclusive_access_bubble.h File chrome/browser/ui/exclusive_access/exclusive_access_bubble.h (right): https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclusive_access/exclusive_access_bubble.h#newcode37 chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:37: static const int kRenotifyTimeMs; kRenotifyTimeM -> kSnoozeNotificationsTimeMs And comment: ...
5 years, 4 months ago (2015-08-04 19:36:28 UTC) #8
Matt Giuca
https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc File chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc (right): https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc#newcode28 chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:28: const int ExclusiveAccessBubble::kInitialDebounceTimeMs = 500; Since scheib told me ...
5 years, 4 months ago (2015-08-05 03:27:20 UTC) #10
Matt Giuca
https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclusive_access/exclusive_access_bubble.h File chrome/browser/ui/exclusive_access/exclusive_access_bubble.h (right): https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclusive_access/exclusive_access_bubble.h#newcode112 chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:112: // the timer is ticking, we do not notify ...
5 years, 4 months ago (2015-08-05 03:35:09 UTC) #11
scheib
LGTM, optional changes on this patch or future: https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclusive_access/exclusive_access_bubble.h File chrome/browser/ui/exclusive_access/exclusive_access_bubble.h (right): https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclusive_access/exclusive_access_bubble.h#newcode37 chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:37: static ...
5 years, 4 months ago (2015-08-05 05:24:19 UTC) #12
msw
lgtm with nits. Please don't remove comments when replying inline. https://codereview.chromium.org/1254543002/diff/120001/chrome/browser/ui/exclusive_access/exclusive_access_bubble.h File chrome/browser/ui/exclusive_access/exclusive_access_bubble.h (right): https://codereview.chromium.org/1254543002/diff/120001/chrome/browser/ui/exclusive_access/exclusive_access_bubble.h#newcode43 ...
5 years, 4 months ago (2015-08-05 17:29:55 UTC) #13
Matt Giuca
https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclusive_access/exclusive_access_bubble.h File chrome/browser/ui/exclusive_access/exclusive_access_bubble.h (right): https://codereview.chromium.org/1254543002/diff/80001/chrome/browser/ui/exclusive_access/exclusive_access_bubble.h#newcode37 chrome/browser/ui/exclusive_access/exclusive_access_bubble.h:37: static const int kRenotifyTimeMs; On 2015/08/05 05:24:19, scheib wrote: ...
5 years, 4 months ago (2015-08-06 03:33:04 UTC) #14
commit-bot: I haz the power
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
5 years, 4 months ago (2015-08-06 03:35:31 UTC) #17
commit-bot: I haz the power
Committed patchset #7 (id:140001)
5 years, 4 months ago (2015-08-06 04:17:16 UTC) #18
commit-bot: I haz the power
5 years, 4 months ago (2015-08-06 04:18:05 UTC) #19
Message was sent while issue was closed.
Patchset 7 (id:??) landed as
https://crrev.com/e145b91dd677b4cbb5b04307c61b79ada06a9f87
Cr-Commit-Position: refs/heads/master@{#342058}

Powered by Google App Engine
This is Rietveld 408576698