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

Issue 1561963003: Fullscreen: Mouse events now count as input for activating the bubble. (Closed)

Created:
4 years, 11 months ago by Matt Giuca
Modified:
4 years, 10 months ago
Reviewers:
msw, sadrul
CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, James Su, pkotwicz, chrome-apps-syd-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@exclusiveaccess-mouse-bubble-position
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fullscreen: Mouse events now count as input for activating the bubble. Only matters if the simplified-fullscreen-ui flag is enabled. After 15 minutes of no user input, the fullscreen / pointer lock bubble is re-shown on the next user input. Previously, this only considered mouse movement and key presses. Now all mouse events are also included for both extending the 15-minute timer, and re-showing the bubble after the timer has elapsed. Importantly, this works while mouselock is active (otherwise, you would not be reminded that you are in fullscreen if it also locks the mouse). The way mouse events are propagated from the RenderWidgetHostViewAura to the browser was changed to support this. Re-land (CL was committed in r369300, reverted in r369549). BUG=352425, 577983 Committed: https://crrev.com/ef9a28662b37d2397a76c00ac9aa9c7568c71e66 Cr-Commit-Position: refs/heads/master@{#376357}

Patch Set 1 #

Patch Set 2 : Added comment. #

Patch Set 3 : Fix Mac compile (use bools instead of ui::EventType). #

Patch Set 4 : Factored out ContentsMouseEvent API updates to CL 1566863002. #

Total comments: 4

Patch Set 5 : Rebase. #

Patch Set 6 : Refactor to avoid using last_mouse_pos_ in simplified mode. #

Total comments: 2

Patch Set 7 : Simplify (msw nit). #

Patch Set 8 : Rebase. #

Patch Set 9 : Avoid potentially changing the event coordinates too early. #

Patch Set 10 : Added test. #

Total comments: 6

Patch Set 11 : Respond to sadrul review. #

Total comments: 2

Patch Set 12 : Rename ForwardMouseEventToDelegate to ForwardMouseEventToParent. #

Patch Set 13 : Don't return early if the event was handled. Fix http://crbug.com/577983. #

Total comments: 4

Patch Set 14 : Still call SetHandled when forwarding mouse events. Added TODO. #

Patch Set 15 : Rebase. #

Patch Set 16 : Rebase. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+98 lines, -23 lines) Patch
M chrome/browser/ui/browser.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/ui/exclusive_access/exclusive_access_bubble.h View 1 2 3 4 5 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +8 lines, -9 lines 0 comments Download
M chrome/browser/ui/exclusive_access/exclusive_access_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/ui/exclusive_access/exclusive_access_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +5 lines, -1 line 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +3 lines, -0 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +32 lines, -12 lines 0 comments Download
M content/browser/renderer_host/render_widget_host_view_aura_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +43 lines, -0 lines 0 comments Download

Messages

Total messages: 69 (29 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1561963003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1561963003/20001
4 years, 11 months ago (2016-01-06 04:52:19 UTC) #2
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/161114)
4 years, 11 months ago (2016-01-06 05:10:50 UTC) #4
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1561963003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1561963003/40001
4 years, 11 months ago (2016-01-06 06:59:55 UTC) #6
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 11 months ago (2016-01-06 08:23:42 UTC) #8
Matt Giuca
msw: chrome/browser/ui/* sadrul: content/browser/renderer_host/render_widget_host_view_aura.cc pkotwicz: FYI since I am moving some code with your TODO ...
4 years, 11 months ago (2016-01-07 00:24:14 UTC) #11
msw
Couple of questions. https://codereview.chromium.org/1561963003/diff/60001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/1561963003/diff/60001/chrome/browser/ui/browser.cc#newcode1596 chrome/browser/ui/browser.cc:1596: exclusive_access_manager_->OnUserInput(); q: Is it important to ...
4 years, 11 months ago (2016-01-07 01:13:00 UTC) #12
Matt Giuca
https://codereview.chromium.org/1561963003/diff/60001/chrome/browser/ui/browser.cc File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/1561963003/diff/60001/chrome/browser/ui/browser.cc#newcode1596 chrome/browser/ui/browser.cc:1596: exclusive_access_manager_->OnUserInput(); On 2016/01/07 01:13:00, msw wrote: > q: Is ...
4 years, 11 months ago (2016-01-07 04:53:59 UTC) #13
msw
lgtm with a minor nit. Hopefully, once the old codepath is removed, we can just ...
4 years, 11 months ago (2016-01-07 06:09:23 UTC) #14
Matt Giuca
Acknowledged. https://codereview.chromium.org/1561963003/diff/100001/chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc File chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc (right): https://codereview.chromium.org/1561963003/diff/100001/chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc#newcode78 chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:78: gfx::Point cursor_pos = GetCursorScreenPoint(); On 2016/01/07 06:09:23, msw ...
4 years, 11 months ago (2016-01-07 08:22:24 UTC) #15
sadrul
Should this have tests?
4 years, 11 months ago (2016-01-08 17:08:40 UTC) #16
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1561963003/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1561963003/120001
4 years, 11 months ago (2016-01-11 00:30:12 UTC) #18
Matt Giuca
Added test RenderWidgetHostViewAuraTest.ForwardMouseEvent. This would have failed before this CL (due to not forwarding when ...
4 years, 11 months ago (2016-01-11 04:57:23 UTC) #19
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1561963003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1561963003/180001
4 years, 11 months ago (2016-01-11 05:43:12 UTC) #21
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 11 months ago (2016-01-11 09:06:17 UTC) #23
sadrul
On 2016/01/11 04:57:23, Matt Giuca wrote: > Added test RenderWidgetHostViewAuraTest.ForwardMouseEvent. This would have > failed ...
4 years, 11 months ago (2016-01-12 19:25:30 UTC) #24
Matt Giuca
https://codereview.chromium.org/1561963003/diff/180001/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1561963003/diff/180001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode2955 content/browser/renderer_host/render_widget_host_view_aura.cc:2955: event->ConvertLocationToTarget(window_, window_->parent()); On 2016/01/12 19:25:30, sadrul wrote: > It's ...
4 years, 11 months ago (2016-01-13 02:40:46 UTC) #26
sadrul
The TODO still applies (the parent should install a pre-target handler or something like that ...
4 years, 11 months ago (2016-01-13 15:14:03 UTC) #27
Matt Giuca
https://codereview.chromium.org/1561963003/diff/220001/content/browser/renderer_host/render_widget_host_view_aura.h File content/browser/renderer_host/render_widget_host_view_aura.h (right): https://codereview.chromium.org/1561963003/diff/220001/content/browser/renderer_host/render_widget_host_view_aura.h#newcode519 content/browser/renderer_host/render_widget_host_view_aura.h:519: void ForwardMouseEventToDelegate(ui::MouseEvent* event); On 2016/01/13 15:14:03, sadrul wrote: > ...
4 years, 11 months ago (2016-01-13 23:08:49 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1561963003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1561963003/240001
4 years, 11 months ago (2016-01-13 23:10:12 UTC) #31
commit-bot: I haz the power
Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_dbg on ...
4 years, 11 months ago (2016-01-14 01:20:54 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1561963003/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1561963003/240001
4 years, 11 months ago (2016-01-14 02:05:33 UTC) #35
commit-bot: I haz the power
Committed patchset #12 (id:240001)
4 years, 11 months ago (2016-01-14 03:23:55 UTC) #36
commit-bot: I haz the power
Patchset 12 (id:??) landed as https://crrev.com/c288c6ee67e48cfcff2be237288da0e1fc2a8662 Cr-Commit-Position: refs/heads/master@{#369300}
4 years, 11 months ago (2016-01-14 03:24:31 UTC) #38
pfeldman
A revert of this CL (patchset #12 id:240001) has been created in https://codereview.chromium.org/1587313002/ by pfeldman@chromium.org. ...
4 years, 11 months ago (2016-01-14 21:34:09 UTC) #39
Matt Giuca
On 2016/01/14 21:34:09, pfeldman wrote: > A revert of this CL (patchset #12 id:240001) has ...
4 years, 11 months ago (2016-01-15 03:37:54 UTC) #40
Matt Giuca
sadrul: PTAL at PS13. The early return if the event was handled was causing drop-downs ...
4 years, 11 months ago (2016-01-15 05:01:15 UTC) #43
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1561963003/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1561963003/260001
4 years, 11 months ago (2016-01-15 05:40:40 UTC) #47
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 11 months ago (2016-01-15 06:20:29 UTC) #49
sadrul
https://codereview.chromium.org/1561963003/diff/260001/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1561963003/diff/260001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode2039 content/browser/renderer_host/render_widget_host_view_aura.cc:2039: ForwardMouseEventToParent(*event); As noted in the comment below: we should ...
4 years, 11 months ago (2016-01-15 17:11:04 UTC) #50
Matt Giuca
https://codereview.chromium.org/1561963003/diff/260001/content/browser/renderer_host/render_widget_host_view_aura.cc File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1561963003/diff/260001/content/browser/renderer_host/render_widget_host_view_aura.cc#newcode2039 content/browser/renderer_host/render_widget_host_view_aura.cc:2039: ForwardMouseEventToParent(*event); On 2016/01/15 17:11:04, sadrul wrote: > As noted ...
4 years, 11 months ago (2016-01-19 00:29:26 UTC) #51
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1561963003/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1561963003/280001
4 years, 11 months ago (2016-01-19 00:30:42 UTC) #53
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 11 months ago (2016-01-19 01:35:58 UTC) #55
Matt Giuca
sadrul: Ping? Ideally we could merge this so it should land ASAP.
4 years, 11 months ago (2016-01-20 23:20:43 UTC) #56
sadrul
Oh yeah, sorry, still lgtm
4 years, 11 months ago (2016-01-20 23:24:06 UTC) #57
Matt Giuca
Wow, I totally missed your LGTM. Sorry. Rebased, will land tomorrow.
4 years, 10 months ago (2016-02-18 08:04:57 UTC) #58
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1561963003/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1561963003/320001
4 years, 10 months ago (2016-02-18 08:05:52 UTC) #60
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 10 months ago (2016-02-18 08:56:20 UTC) #62
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1561963003/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1561963003/320001
4 years, 10 months ago (2016-02-19 02:56:09 UTC) #65
commit-bot: I haz the power
Committed patchset #16 (id:320001)
4 years, 10 months ago (2016-02-19 03:11:38 UTC) #67
commit-bot: I haz the power
4 years, 10 months ago (2016-02-19 03:12:53 UTC) #69
Message was sent while issue was closed.
Patchset 16 (id:??) landed as
https://crrev.com/ef9a28662b37d2397a76c00ac9aa9c7568c71e66
Cr-Commit-Position: refs/heads/master@{#376357}

Powered by Google App Engine
This is Rietveld 408576698