Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
296943003:
Allow mouse-input to be enabled even if the plugin does not have keyboard focus. (Closed)
DescriptionAllow mouse-input to be enabled even if the plugin does not have keyboard focus.
BUG=376070
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=272330
Patch Set 1 #
Total comments: 11
Patch Set 2 : Reviewer feedback. #Patch Set 3 : Rebase #
Messages
Total messages: 18 (0 generated)
ptal
https://codereview.chromium.org/296943003/diff/1/remoting/client/plugin/chrom... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/296943003/diff/1/remoting/client/plugin/chrom... remoting/client/plugin/chromoting_instance.cc:178: "sendMouseInputWhenUnfocused"; nit: Unless there is some fallback behaviour the app can reasonably do in the absence of this feature, is there any point advertising it in kApiFeatures? https://codereview.chromium.org/296943003/diff/1/remoting/client/plugin/chrom... File remoting/client/plugin/chromoting_instance.h (right): https://codereview.chromium.org/296943003/diff/1/remoting/client/plugin/chrom... remoting/client/plugin/chromoting_instance.h:211: void HandleSendMouseInputWhenUnfocused(); nit: Unfocused -> Blurred for consistency w/ DOM terminology? https://codereview.chromium.org/296943003/diff/1/remoting/client/plugin/peppe... File remoting/client/plugin/pepper_input_handler.cc (right): https://codereview.chromium.org/296943003/diff/1/remoting/client/plugin/peppe... remoting/client/plugin/pepper_input_handler.cc:27: send_mouse_input_when_unfocused_(false), nit: You could call this (and the related method, etc) move_mouse_when_blurred; setter could then be set_mmwb(bool) (even if the API we export to JS never really needs the bool=false capability). https://codereview.chromium.org/296943003/diff/1/remoting/client/plugin/peppe... remoting/client/plugin/pepper_input_handler.cc:79: if (!has_focus_ && !send_mouse_input_when_unfocused_) Should we be filtering these at all? Is there any way we can even receive a mouse-down without having focus[1]? [1] Focus is generally either follows-mouse or click-to-focus, so in either case by the time we see mouse-down, we must have focus, I think. https://codereview.chromium.org/296943003/diff/1/remoting/client/plugin/peppe... File remoting/client/plugin/pepper_input_handler.h (right): https://codereview.chromium.org/296943003/diff/1/remoting/client/plugin/peppe... remoting/client/plugin/pepper_input_handler.h:47: // Enables sending input when the plugin does not have input focus. nit: mouse input
ptal https://codereview.chromium.org/296943003/diff/1/remoting/client/plugin/chrom... File remoting/client/plugin/chromoting_instance.cc (right): https://codereview.chromium.org/296943003/diff/1/remoting/client/plugin/chrom... remoting/client/plugin/chromoting_instance.cc:178: "sendMouseInputWhenUnfocused"; On 2014/05/22 01:14:23, Wez wrote: > nit: Unless there is some fallback behaviour the app can reasonably do in the > absence of this feature, is there any point advertising it in kApiFeatures? Done. https://codereview.chromium.org/296943003/diff/1/remoting/client/plugin/chrom... File remoting/client/plugin/chromoting_instance.h (right): https://codereview.chromium.org/296943003/diff/1/remoting/client/plugin/chrom... remoting/client/plugin/chromoting_instance.h:211: void HandleSendMouseInputWhenUnfocused(); On 2014/05/22 01:14:23, Wez wrote: > nit: Unfocused -> Blurred for consistency w/ DOM terminology? "blur" is a horrible terminology for the unfocused state IMO, especially since we visually blur the plugin when it has lost connectivity. https://codereview.chromium.org/296943003/diff/1/remoting/client/plugin/peppe... File remoting/client/plugin/pepper_input_handler.cc (right): https://codereview.chromium.org/296943003/diff/1/remoting/client/plugin/peppe... remoting/client/plugin/pepper_input_handler.cc:27: send_mouse_input_when_unfocused_(false), On 2014/05/22 01:14:23, Wez wrote: > nit: You could call this (and the related method, etc) move_mouse_when_blurred; > setter could then be set_mmwb(bool) (even if the API we export to JS never > really needs the bool=false capability). Since it controls more than just mouse movements, I prefer to keep this terminology. I've changed the interface on PIH as you suggest. I was in two minds as to whether to do the same for the JS API; I decided against it for consistency with enableMediaSourceRendering, but LMK if you disagree. https://codereview.chromium.org/296943003/diff/1/remoting/client/plugin/peppe... remoting/client/plugin/pepper_input_handler.cc:79: if (!has_focus_ && !send_mouse_input_when_unfocused_) On 2014/05/22 01:14:23, Wez wrote: > Should we be filtering these at all? Is there any way we can even receive a > mouse-down without having focus[1]? > > [1] Focus is generally either follows-mouse or click-to-focus, so in either case > by the time we see mouse-down, we must have focus, I think. I wrote up a summary when I added this code: https://codereview.chromium.org/136093013/diff/60001/remoting/client/plugin/p... In short, suppressing these clicks is the right thing to do on Mac, and arguably makes sense on other platforms as well. https://codereview.chromium.org/296943003/diff/1/remoting/client/plugin/peppe... File remoting/client/plugin/pepper_input_handler.h (right): https://codereview.chromium.org/296943003/diff/1/remoting/client/plugin/peppe... remoting/client/plugin/pepper_input_handler.h:47: // Enables sending input when the plugin does not have input focus. On 2014/05/22 01:14:23, Wez wrote: > nit: mouse input Done.
lgtm https://codereview.chromium.org/296943003/diff/1/remoting/client/plugin/chrom... File remoting/client/plugin/chromoting_instance.h (right): https://codereview.chromium.org/296943003/diff/1/remoting/client/plugin/chrom... remoting/client/plugin/chromoting_instance.h:211: void HandleSendMouseInputWhenUnfocused(); On 2014/05/22 01:32:09, Jamie wrote: > On 2014/05/22 01:14:23, Wez wrote: > > nit: Unfocused -> Blurred for consistency w/ DOM terminology? > > "blur" is a horrible terminology for the unfocused state IMO, especially since > we visually blur the plugin when it has lost connectivity. True!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/296943003/20001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
The CQ bit was checked by jamiewalch@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/296943003/20001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...) android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...) linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...) win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was checked by jamiewalch@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/296943003/20001
The CQ bit was checked by jamiewalch@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/296943003/40001
Message was sent while issue was closed.
Change committed as 272330 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
