|
|
Created:
4 years, 11 months ago by Matt Giuca Modified:
4 years, 10 months ago 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. |
DescriptionFullscreen: 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. #Messages
Total messages: 69 (29 generated)
The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== 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. Refactored WebContentsDelegate::ContentsMouseEvent so it is now called for all mouse events, not just MOVED and EXITED. The event type is now passed in, rather than just 'motion' (it now does what it says). BUG=352425 ========== to ========== 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. BUG=352425 ==========
mgiuca@chromium.org changed reviewers: + msw@chromium.org, sadrul@chromium.org
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 on it. Pending on CL: https://codereview.chromium.org/1566863002/
Couple of questions. https://codereview.chromium.org/1561963003/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/1561963003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1596: exclusive_access_manager_->OnUserInput(); q: Is it important to capture all mouse events, even outside the content area? https://codereview.chromium.org/1561963003/diff/60001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc (right): https://codereview.chromium.org/1561963003/diff/60001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:79: mouse_position_checker_.Start( q: Can we avoid using the mouse position checker in the simplified version (since now the manager is notified on all mouse events in the content area), or do we still need it for events outside the window area?
https://codereview.chromium.org/1561963003/diff/60001/chrome/browser/ui/brows... File chrome/browser/ui/browser.cc (right): https://codereview.chromium.org/1561963003/diff/60001/chrome/browser/ui/brows... chrome/browser/ui/browser.cc:1596: exclusive_access_manager_->OnUserInput(); On 2016/01/07 01:13:00, msw wrote: > q: Is it important to capture all mouse events, even outside the content area? No. In fact it is irrelevant since this only matters if we're in fullscreen and/or mouselock mode; in either case we cannot move the mouse outside of the content area. But hypothetically if there was something that locked the bubble in a state where you could move the mouse outside the content area, I'd say it's fine to only trigger it when the user is directly sending mouse events to the content area. https://codereview.chromium.org/1561963003/diff/60001/chrome/browser/ui/exclu... File chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc (right): https://codereview.chromium.org/1561963003/diff/60001/chrome/browser/ui/exclu... chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:79: mouse_position_checker_.Start( On 2016/01/07 01:13:00, msw wrote: > q: Can we avoid using the mouse position checker in the simplified version > (since now the manager is notified on all mouse events in the content area), or > do we still need it for events outside the window area? Unfortunately, no, this function is still used for checking the hide timeout and calling Hide() --- and so it is mis-named. But I did manage to refactor it a bit so all of the last_mouse_pos_ and mouse position checking code only runs when the flag is disabled, so this code will simplify right down once we remove the flag (and then we can rename the function to just CheckHideTimeout or something).
lgtm with a minor nit. Hopefully, once the old codepath is removed, we can just reset a 15 min timer on any mouse event, rather than checking the mouse position at regular intervals; that should be more efficient. https://codereview.chromium.org/1561963003/diff/100001/chrome/browser/ui/excl... File chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc (right): https://codereview.chromium.org/1561963003/diff/100001/chrome/browser/ui/excl... chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:78: gfx::Point cursor_pos = GetCursorScreenPoint(); nit: remove this local, just assign last_mouse_pos_ = GetCursorScreenPoint();
Acknowledged. https://codereview.chromium.org/1561963003/diff/100001/chrome/browser/ui/excl... File chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc (right): https://codereview.chromium.org/1561963003/diff/100001/chrome/browser/ui/excl... chrome/browser/ui/exclusive_access/exclusive_access_bubble.cc:78: gfx::Point cursor_pos = GetCursorScreenPoint(); On 2016/01/07 06:09:23, msw wrote: > nit: remove this local, just assign last_mouse_pos_ = GetCursorScreenPoint(); Done.
Should this have tests?
The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run
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
Added test RenderWidgetHostViewAuraTest.ForwardMouseEvent. This would have failed before this CL (due to not forwarding when mouse locked). Also, I discovered a problem which is that before (Patch Set 8), calling event->ConvertLocationToTarget too early might result in changing the event coordinates (though I didn't see a problem when I tested it manually). Instead, I wrote a function and call it at the last possible moment, in all relevant paths.
The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/01/11 04:57:23, Matt Giuca wrote: > Added test RenderWidgetHostViewAuraTest.ForwardMouseEvent. This would have > failed before this CL (due to not forwarding when mouse locked). Cool. Thanks! > Also, I discovered a problem which is that before (Patch Set 8), calling > event->ConvertLocationToTarget too early might result in changing the event > coordinates (though I didn't see a problem when I tested it manually). Instead, > I wrote a function and call it at the last possible moment, in all relevant > paths. I suggested a fix for this: https://codereview.chromium.org/1561963003/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1561963003/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:2955: event->ConvertLocationToTarget(window_, window_->parent()); It's unfortunate that we have to call ForwardMouseEvenToParent() from a few places. Can we call it just once towards the beginning of RWHVA::OnMouseEvent() instead, and instead of mutating the event's location, we clone it and mutate that. i.e. this function would look like: { ... // Early returns. scoped_ptr<ui::Event> event_copy = Event::Clone(*event); ui::MouseEvent* mouse_event = static_cast<...>(event_copy.get()); mouse_event->ConvertLocationToTarget(window_, window_->parent()); window_->parent()->delegate()->OnMouseEvent(mouse_event); if (mouse_event->handled()) event->SetHandled(); } Would that work? https://codereview.chromium.org/1561963003/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/1561963003/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:3637: view_->InitAsChild(nullptr); You can send send parent.get() to InitAsChild(). That should take care of parenting, and you wouldn't need to explicitly do that in the next line. https://codereview.chromium.org/1561963003/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:3670: view_ = NULL; nullptr
Patchset #11 (id:200001) has been deleted
https://codereview.chromium.org/1561963003/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1561963003/diff/180001/content/browser/render... 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 unfortunate that we have to call ForwardMouseEvenToParent() from a few > places. Can we call it just once towards the beginning of RWHVA::OnMouseEvent() > instead, and instead of mutating the event's location, we clone it and mutate > that. i.e. this function would look like: > > { > ... // Early returns. > scoped_ptr<ui::Event> event_copy = Event::Clone(*event); > ui::MouseEvent* mouse_event = static_cast<...>(event_copy.get()); > mouse_event->ConvertLocationToTarget(window_, window_->parent()); > window_->parent()->delegate()->OnMouseEvent(mouse_event); > if (mouse_event->handled()) > event->SetHandled(); > } > > Would that work? Good idea. I didn't know about Event::Clone. Yeah that works. Thanks for reminding me to call SetHandled. Since we're doing that, I thought I may as well early return in OnMouseEvent if event->handled() is set (which I don't think it will ever be, but if it was set by the delegate it would be correct to stop here). I'm still going to leave this function abstracted even though it's only called once now. It's logically separate and we get early returns, etc. Do you think we should keep pkotwicz TODO or is this OK now? https://codereview.chromium.org/1561963003/diff/180001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/1561963003/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:3637: view_->InitAsChild(nullptr); On 2016/01/12 19:25:30, sadrul wrote: > You can send send parent.get() to InitAsChild(). That should take care of > parenting, and you wouldn't need to explicitly do that in the next line. Done. https://codereview.chromium.org/1561963003/diff/180001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:3670: view_ = NULL; On 2016/01/12 19:25:30, sadrul wrote: > nullptr Done.
The TODO still applies (the parent should install a pre-target handler or something like that to receive the event, instead the child window having to explicitly forward it). A small nit, with that, lgtm. https://codereview.chromium.org/1561963003/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.h (right): https://codereview.chromium.org/1561963003/diff/220001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.h:519: void ForwardMouseEventToDelegate(ui::MouseEvent* event); Sorry, missed this in earlier review, but let's call this ForwardMouseToParent[Delegate] instead
https://codereview.chromium.org/1561963003/diff/220001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.h (right): https://codereview.chromium.org/1561963003/diff/220001/content/browser/render... 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: > Sorry, missed this in earlier review, but let's call this > ForwardMouseToParent[Delegate] instead Done.
The CQ bit was checked by mgiuca@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/1561963003/#ps240001 (title: "Rename ForwardMouseEventToDelegate to ForwardMouseEventToParent.")
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
The CQ bit was unchecked by commit-bot@chromium.org
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 tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_clang_dbg_recipe on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) android_compile_dbg on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) cast_shell_android on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) linux_android_rel_ng on tryserver.chromium.android (JOB_TIMED_OUT, no build URL) cast_shell_linux on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by mgiuca@chromium.org
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
Message was sent while issue was closed.
Committed patchset #12 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== 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. BUG=352425 ========== to ========== 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. BUG=352425 Committed: https://crrev.com/c288c6ee67e48cfcff2be237288da0e1fc2a8662 Cr-Commit-Position: refs/heads/master@{#369300} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/c288c6ee67e48cfcff2be237288da0e1fc2a8662 Cr-Commit-Position: refs/heads/master@{#369300}
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:240001) has been created in https://codereview.chromium.org/1587313002/ by pfeldman@chromium.org. The reason for reverting is: It is no longer possible to pick items in <select> using mouse (reproed in DevTools UI)..
Message was sent while issue was closed.
On 2016/01/14 21:34:09, pfeldman wrote: > A revert of this CL (patchset #12 id:240001) has been created in > https://codereview.chromium.org/1587313002/ by mailto:pfeldman@chromium.org. > > The reason for reverting is: It is no longer possible to pick items in <select> > using mouse (reproed in DevTools UI).. Thanks for picking this up. I'll look into it. Do you have a bug ID for this regression?
Message was sent while issue was closed.
Description was changed from ========== 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. BUG=352425 Committed: https://crrev.com/c288c6ee67e48cfcff2be237288da0e1fc2a8662 Cr-Commit-Position: refs/heads/master@{#369300} ========== to ========== 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. BUG=352425 Committed: https://crrev.com/c288c6ee67e48cfcff2be237288da0e1fc2a8662 Cr-Commit-Position: refs/heads/master@{#369300} ==========
Description was changed from ========== 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. BUG=352425 Committed: https://crrev.com/c288c6ee67e48cfcff2be237288da0e1fc2a8662 Cr-Commit-Position: refs/heads/master@{#369300} ========== to ========== 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 ==========
sadrul: PTAL at PS13. The early return if the event was handled was causing drop-downs to not be clickable (see http://crbug.com/577983). Now I could have just removed that, but I figured that I'd go further and removed the call to SetHandled since I'm not exactly sure what's going on but in light of this, it doesn't seem right to have the events marked as "handled" when they still need further action. WDYT? NOTE: If possible I'd like this to land before branch point (since without it, there is a deficiency in the full screen interaction). So if you approve this, please check the commit box for me (I will be asleep). If you have minor nits, I'd like to deal with those in a follow up. If you think it warrants further discussion, that's OK. It isn't strictly necessary for fullscreen. Thanks, Matt.
Description was changed from ========== 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 ========== to ========== 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. Committed: https://crrev.com/c288c6ee67e48cfcff2be237288da0e1fc2a8662 Cr-Commit-Position: refs/heads/master@{#369300} Re-land (CL was committed in r369300, reverted in r369549). BUG=352425, 577983 ==========
Description was changed from ========== 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. Committed: https://crrev.com/c288c6ee67e48cfcff2be237288da0e1fc2a8662 Cr-Commit-Position: refs/heads/master@{#369300} Re-land (CL was committed in r369300, reverted in r369549). BUG=352425, 577983 ========== to ========== 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 ==========
The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1561963003/diff/260001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1561963003/diff/260001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:2039: ForwardMouseEventToParent(*event); As noted in the comment below: we should still send the event pointer to ForwardMouseEventToParent(), and make sure SetHandled() is called correctly. Ideally, we would also return from here if the event is handled. But that seems to break things. So please add a TODO here (e.g. "We should return if event is handled. See crbug.com/xxxxx") to try to figure out if something is incorrectly marking the event as handled. https://codereview.chromium.org/1561963003/diff/260001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:2955: window_->parent()->delegate()->OnMouseEvent(mouse_event); I think we still need to call SetHandled() on the event. But not early return from ::OnMouseEvent() above. It'd also be a good to figure out what's setting the SetHandled() in the first place, since it sounds like it's marking the event as handled when it really shouldn't.
https://codereview.chromium.org/1561963003/diff/260001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/1561963003/diff/260001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:2039: ForwardMouseEventToParent(*event); On 2016/01/15 17:11:04, sadrul wrote: > As noted in the comment below: we should still send the event pointer to > ForwardMouseEventToParent(), and make sure SetHandled() is called correctly. > Ideally, we would also return from here if the event is handled. But that seems > to break things. So please add a TODO here (e.g. "We should return if event is > handled. See crbug.com/xxxxx") to try to figure out if something is incorrectly > marking the event as handled. Done. https://codereview.chromium.org/1561963003/diff/260001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:2955: window_->parent()->delegate()->OnMouseEvent(mouse_event); On 2016/01/15 17:11:04, sadrul wrote: > I think we still need to call SetHandled() on the event. But not early return > from ::OnMouseEvent() above. It'd also be a good to figure out what's setting > the SetHandled() in the first place, since it sounds like it's marking the event > as handled when it really shouldn't. Done.
The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
sadrul: Ping? Ideally we could merge this so it should land ASAP.
Oh yeah, sorry, still lgtm
Wow, I totally missed your LGTM. Sorry. Rebased, will land tomorrow.
The CQ bit was checked by mgiuca@chromium.org to run a CQ dry run
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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mgiuca@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/1561963003/#ps320001 (title: "Rebase.")
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
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #16 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/ef9a28662b37d2397a76c00ac9aa9c7568c71e66 Cr-Commit-Position: refs/heads/master@{#376357} |