Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
687323002:
Experiment with emulating mouse hover with touch hover capable devices (Closed)
DescriptionExperiment with emulating mouse hover with touch hover capable devices
We enable the hover feature on touchscreens on Samsung Galaxy S4, S5 by
turning on a hover feature switch, which simulated a mouse over event.
BUG=418188
Committed: https://crrev.com/d74bc3e1c493f6d253c688adb4ebac2192655ea3
Cr-Commit-Position: refs/heads/master@{#306704}
Patch Set 1 : #
Total comments: 3
Patch Set 2 : #
Total comments: 3
Patch Set 3 : #
Total comments: 9
Patch Set 4 : Change the comments #
Total comments: 6
Patch Set 5 : #
Total comments: 2
Patch Set 6 : #
Total comments: 4
Patch Set 7 : Format change #Patch Set 8 : rebase #
Messages
Total messages: 34 (10 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
lanwei@chromium.org changed reviewers: + jdduke@chromium.org, rbyers@chromium.org, tdresser@chromium.org
https://codereview.chromium.org/687323002/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/687323002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:653: public boolean onHover(View view, MotionEvent motionEvent) { It's not clear to me why we need this? Won't we already get the events through |onHoverEvent()|?
chrome/app/generated_resources.grd: I don't see other comments like: "<!-- Enable hover feature -->" So we can probably remove it. "Enable support for hover capable touchscreens" Add a period. https://codereview.chromium.org/687323002/diff/40001/chrome/browser/about_fla... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/687323002/diff/40001/chrome/browser/about_fla... chrome/browser/about_flags.cc:2003: "enable-support-for-hover-capable-touchscreens", This is more verbose than necessary. What about --enable-touch-hover? https://codereview.chromium.org/687323002/diff/40001/chrome/browser/about_fla... chrome/browser/about_flags.cc:2004: IDS_FLAGS_ENABLE_SUPPORT_HOVER_CAPABLE_TOUCHSCREENS_NAME, This could probably also be made more concise. ENABLE_TOUCH_HOVER or ENABLE_TOUCHSCREEN_HOVER
Patchset #2 (id:60001) has been deleted
https://codereview.chromium.org/687323002/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/687323002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1470: && event.getToolType(0) == MotionEvent.TOOL_TYPE_FINGER Are these both necessary? Can you get a TOOL_TYPE_FINGER without a SOURCE_TOUCHSCREEN?
https://codereview.chromium.org/687323002/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/687323002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1469: if (!CommandLine.getInstance().hasSwitch(ContentSwitches.ENABLE_TOUCH_HOVER) I still don't understand why we need a command-line flag. What data are we trying to gather? Is it possible that we'd receive hover events for touchscreens that are not from S4/S5 devices? Are we worried about performance problems by spamming hover events? It would be good to know how frequently these fire when hovering and how easy it is to trigger.
On 2014/10/31 21:03:21, jdduke wrote: > https://codereview.chromium.org/687323002/diff/80001/content/public/android/j... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/687323002/diff/80001/content/public/android/j... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1469: > if (!CommandLine.getInstance().hasSwitch(ContentSwitches.ENABLE_TOUCH_HOVER) > I still don't understand why we need a command-line flag. What data are we > trying to gather? Is it possible that we'd receive hover events for touchscreens > that are not from S4/S5 devices? > Are we worried about performance problems by spamming hover events? It would be > good to know how frequently these fire when hovering and how easy it is to > trigger. Sorry for the delay in chiming in here. Mainly we're trying to decide if this feature is sufficiently useful to justify doing the work to understand and mitigate performance problems. Eg. to ship this we almost certainly want to require the user to dwell briefly before starting to send mousemove events (otherwise we'd add blink/js overhead on every tap/scroll). I asked Lan to land the simple support behind a flag so we could more easily experiment with the behavior and evaluate usefulness. And no, I'm not aware of any other device that will send such events other than the Samsung ones (which require us to opt-in to them via a manifest switch). Of course there could always be one somewhere...
https://codereview.chromium.org/687323002/diff/100001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/687323002/diff/100001/chrome/app/generated_re... chrome/app/generated_resources.grd:14919: + Enables Finger Air View by holding your finger just over the screen to experience webpage magnifier or a mouseover event. nit: let's not use the Samsung specific feature term here - it's conceivable that other Android devices could send touch hover events as well. Also what's the reference to 'webpage magnifier'? I know that's what Samsung's chromium fork does with the hover events, but we don't have any code for that upstream do we? https://codereview.chromium.org/687323002/diff/100001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/687323002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1554: // a finger hover. Please add a comment like: TODO(lanwei) Remove this switch once experimentation is complete - crbug.com/418188 We can leave the bug open until we either decide to ship this enabled by default, or abort the experiment (too often we forget to do the final cleanup of removing code for flags which are no longer experimental)... https://codereview.chromium.org/687323002/diff/100001/content/public/android/... File content/public/android/java/src/org/chromium/content/common/ContentSwitches.java (right): https://codereview.chromium.org/687323002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/common/ContentSwitches.java:77: // Enable Finger Air View by holding your finger just over the screen Please don't use the Samsung-specific feature name - this isn't necessarily for "Finger Air View" devices only. Maybe "Enable mouse hover emulation by ...".
https://codereview.chromium.org/687323002/diff/100001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/687323002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1558: return true; So the SPen hover events always indicate that they're from a finger? Also, we should cache the command line check (let's have a static method with a static Boolean that's initialized just once (when null)). static boolean touchHoverEventsEnabled() { if (sTouchHoverEventsEnabled == null) { ... } return sTouchHoverEventsEnabled; } Hmm, now I'm wondering if the query should go in SPenSupport.java? That might also let us restrict the case when we filter the hover events (isSPenSupported() will be true).
https://codereview.chromium.org/687323002/diff/100001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/687323002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1558: return true; On 2014/11/13 19:38:44, jdduke wrote: > Hmm, now I'm wondering if the query should go in SPenSupport.java? That might > also let us restrict the case when we filter the hover events (isSPenSupported() > will be true). Nevermind, I thought the airview feature was SPen-specific but it's not.
On 2014/11/13 19:47:41, jdduke wrote: > https://codereview.chromium.org/687323002/diff/100001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/687323002/diff/100001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1558: > return true; > On 2014/11/13 19:38:44, jdduke wrote: > > Hmm, now I'm wondering if the query should go in SPenSupport.java? That might > > also let us restrict the case when we filter the hover events > (isSPenSupported() > > will be true). > > Nevermind, I thought the airview feature was SPen-specific but it's not. Right. For anyone else following along, what we're trying to do here is to support touch FINGER hover (eg. Samsung AirView: http://www.samsung.com/us/support/howtoguide/N0000004/13142/159745) without changing any of the existing support for mouse/stylus hovering.
Patchset #4 (id:120001) has been deleted
https://codereview.chromium.org/687323002/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/687323002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1470: && event.getToolType(0) == MotionEvent.TOOL_TYPE_FINGER On 2014/10/31 19:19:25, tdresser wrote: > Are these both necessary? Can you get a TOOL_TYPE_FINGER without a > SOURCE_TOUCHSCREEN? Done. https://codereview.chromium.org/687323002/diff/100001/chrome/app/generated_re... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/687323002/diff/100001/chrome/app/generated_re... chrome/app/generated_resources.grd:14919: + Enables Finger Air View by holding your finger just over the screen to experience webpage magnifier or a mouseover event. On 2014/11/13 19:32:15, Rick Byers wrote: > nit: let's not use the Samsung specific feature term here - it's conceivable > that other Android devices could send touch hover events as well. Also what's > the reference to 'webpage magnifier'? I know that's what Samsung's chromium > fork does with the hover events, but we don't have any code for that upstream do > we? Done. https://codereview.chromium.org/687323002/diff/100001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/687323002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1554: // a finger hover. On 2014/11/13 19:32:15, Rick Byers wrote: > Please add a comment like: > TODO(lanwei) Remove this switch once experimentation is complete - > crbug.com/418188 > > We can leave the bug open until we either decide to ship this enabled by > default, or abort the experiment (too often we forget to do the final cleanup of > removing code for flags which are no longer experimental)... Done. https://codereview.chromium.org/687323002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1558: return true; On 2014/11/13 19:47:40, jdduke wrote: > On 2014/11/13 19:38:44, jdduke wrote: > > Hmm, now I'm wondering if the query should go in SPenSupport.java? That might > > also let us restrict the case when we filter the hover events > (isSPenSupported() > > will be true). > > Nevermind, I thought the airview feature was SPen-specific but it's not. Done. https://codereview.chromium.org/687323002/diff/100001/content/public/android/... File content/public/android/java/src/org/chromium/content/common/ContentSwitches.java (right): https://codereview.chromium.org/687323002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/common/ContentSwitches.java:77: // Enable Finger Air View by holding your finger just over the screen On 2014/11/13 19:32:15, Rick Byers wrote: > Please don't use the Samsung-specific feature name - this isn't necessarily for > "Finger Air View" devices only. > Maybe "Enable mouse hover emulation by ...". Done.
Missing period in generated_resources.grd. https://codereview.chromium.org/687323002/diff/130006/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/687323002/diff/130006/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1615: if (!CommandLine.getInstance().hasSwitch(ContentSwitches.ENABLE_TOUCH_HOVER) Can we cache |CommandLine.getInstance().hasSwitch(ContentSwitches.ENABLE_TOUCH_HOVER|? https://codereview.chromium.org/687323002/diff/130006/content/public/android/... File content/public/android/java/src/org/chromium/content/common/ContentSwitches.java (right): https://codereview.chromium.org/687323002/diff/130006/content/public/android/... content/public/android/java/src/org/chromium/content/common/ContentSwitches.java:77: // Enable mouse hover emulation by holding your finger just over the screen Missing period. https://codereview.chromium.org/687323002/diff/130006/content/public/android/... content/public/android/java/src/org/chromium/content/common/ContentSwitches.java:79: "enable-touch-hover"; Missing newline.
lanwei@chromium.org changed reviewers: - jdduke@chromium.org, rbyers@chromium.org
lanwei@chromium.org changed reviewers: + jdduke@chromium.org, rbyers@chromium.org
https://codereview.chromium.org/687323002/diff/150001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/687323002/diff/150001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1615: String disableTouchHover = !CommandLine.getInstance().hasSwitch( What if we use a |Boolean| member variable, then you'll have something like: if (event.getToolType() == MotionEvent.TOOL_TYPE_FINGER) { if (mEnableTouchHover == null) { mEnableTouchHover = CommandLine.getInstance().hasSwitch(ContentSwitches.ENABLE_TOUCH_HOVER); } if (!mEnableTouchHover.booleanValue()) return false; }
https://codereview.chromium.org/687323002/diff/130006/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/687323002/diff/130006/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1615: if (!CommandLine.getInstance().hasSwitch(ContentSwitches.ENABLE_TOUCH_HOVER) On 2014/12/01 15:32:00, tdresser wrote: > Can we cache > |CommandLine.getInstance().hasSwitch(ContentSwitches.ENABLE_TOUCH_HOVER|? Done. https://codereview.chromium.org/687323002/diff/130006/content/public/android/... File content/public/android/java/src/org/chromium/content/common/ContentSwitches.java (right): https://codereview.chromium.org/687323002/diff/130006/content/public/android/... content/public/android/java/src/org/chromium/content/common/ContentSwitches.java:77: // Enable mouse hover emulation by holding your finger just over the screen On 2014/12/01 15:32:00, tdresser wrote: > Missing period. Done. https://codereview.chromium.org/687323002/diff/130006/content/public/android/... content/public/android/java/src/org/chromium/content/common/ContentSwitches.java:79: "enable-touch-hover"; On 2014/12/01 15:32:00, tdresser wrote: > Missing newline. Done. https://codereview.chromium.org/687323002/diff/150001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/687323002/diff/150001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1615: String disableTouchHover = !CommandLine.getInstance().hasSwitch( On 2014/12/03 00:49:55, jdduke wrote: > What if we use a |Boolean| member variable, then you'll have something like: > > if (event.getToolType() == MotionEvent.TOOL_TYPE_FINGER) { > if (mEnableTouchHover == null) { > mEnableTouchHover = > CommandLine.getInstance().hasSwitch(ContentSwitches.ENABLE_TOUCH_HOVER); > } > if (!mEnableTouchHover.booleanValue()) return false; > } Done.
On 2014/12/03 18:33:03, lanwei wrote: > https://codereview.chromium.org/687323002/diff/130006/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/687323002/diff/130006/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1615: > if (!CommandLine.getInstance().hasSwitch(ContentSwitches.ENABLE_TOUCH_HOVER) > On 2014/12/01 15:32:00, tdresser wrote: > > Can we cache > > |CommandLine.getInstance().hasSwitch(ContentSwitches.ENABLE_TOUCH_HOVER|? > > Done. > > https://codereview.chromium.org/687323002/diff/130006/content/public/android/... > File > content/public/android/java/src/org/chromium/content/common/ContentSwitches.java > (right): > > https://codereview.chromium.org/687323002/diff/130006/content/public/android/... > content/public/android/java/src/org/chromium/content/common/ContentSwitches.java:77: > // Enable mouse hover emulation by holding your finger just over the screen > On 2014/12/01 15:32:00, tdresser wrote: > > Missing period. > > Done. > > https://codereview.chromium.org/687323002/diff/130006/content/public/android/... > content/public/android/java/src/org/chromium/content/common/ContentSwitches.java:79: > "enable-touch-hover"; > On 2014/12/01 15:32:00, tdresser wrote: > > Missing newline. > > Done. > > https://codereview.chromium.org/687323002/diff/150001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/687323002/diff/150001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1615: > String disableTouchHover = !CommandLine.getInstance().hasSwitch( > On 2014/12/03 00:49:55, jdduke wrote: > > What if we use a |Boolean| member variable, then you'll have something like: > > > > if (event.getToolType() == MotionEvent.TOOL_TYPE_FINGER) { > > if (mEnableTouchHover == null) { > > mEnableTouchHover = > > CommandLine.getInstance().hasSwitch(ContentSwitches.ENABLE_TOUCH_HOVER); > > } > > if (!mEnableTouchHover.booleanValue()) return false; > > } > > Done. Still missing a period in generated_resources.grd (not sure why the side-by-side diff isn't showing). Otherwise, LGTM.
jdduke@chromium.org changed reviewers: + tedchoc@chromium.org
Adding tedchoc@ for content/common owner. https://codereview.chromium.org/687323002/diff/170001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/687323002/diff/170001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1614: // Dispatch a mouseover event when the touch hover switch is on and the You can probably get rid of this comment, and just leave the // TODO comment. https://codereview.chromium.org/687323002/diff/170001/content/public/android/... File content/public/android/java/src/org/chromium/content/common/ContentSwitches.java (right): https://codereview.chromium.org/687323002/diff/170001/content/public/android/... content/public/android/java/src/org/chromium/content/common/ContentSwitches.java:74: public static final String ENABLE_TOUCH_HOVER = Nit: I think this will fit on one line (100 characters is the Java width).
owners -lgtm
lanwei@chromium.org changed reviewers: + asvitkine@chromium.org
asvitkine@chromium.org: Please review changes in Can you please look at the tools/metrics/histograms/histograms.xml, I added a flag? Thank you.
lgtm
The CQ bit was checked by lanwei@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/687323002/210001
Message was sent while issue was closed.
Committed patchset #8 (id:210001)
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/d74bc3e1c493f6d253c688adb4ebac2192655ea3 Cr-Commit-Position: refs/heads/master@{#306704}
Message was sent while issue was closed.
https://codereview.chromium.org/687323002/diff/170001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/687323002/diff/170001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1614: // Dispatch a mouseover event when the touch hover switch is on and the On 2014/12/03 19:09:32, jdduke wrote: > You can probably get rid of this comment, and just leave the // TODO comment. Done. https://codereview.chromium.org/687323002/diff/170001/content/public/android/... File content/public/android/java/src/org/chromium/content/common/ContentSwitches.java (right): https://codereview.chromium.org/687323002/diff/170001/content/public/android/... content/public/android/java/src/org/chromium/content/common/ContentSwitches.java:74: public static final String ENABLE_TOUCH_HOVER = On 2014/12/03 19:09:32, jdduke wrote: > Nit: I think this will fit on one line (100 characters is the Java width). Done. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
