Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
385963002:
[Android] Support SPen events (Closed)
|
Created:
6 years, 5 months ago by Changwan Ryu Modified:
6 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Description[Android] Support SPen events
TEST=checked that this works on a test device
BUG=384803
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283294
Patch Set 1 #
Total comments: 9
Patch Set 2 : added OEM check #Patch Set 3 : #Patch Set 4 : changed spen events definitions to static #
Total comments: 4
Patch Set 5 : removed action_cancel for now #
Total comments: 6
Patch Set 6 : addressed review comments, updated constant values #
Total comments: 6
Patch Set 7 : moved to ContentViewCore and fixed nits #Patch Set 8 : added comment #
Total comments: 2
Patch Set 9 : changed constants to empirically obtained ones #Patch Set 10 : renamed eventAction as eventActionMasked #Messages
Total messages: 27 (0 generated)
This is a transparent version of the change.
https://codereview.chromium.org/385963002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://codereview.chromium.org/385963002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentView.java:133: final int spenActionDown = 11; I have a few worries about this. Suppose Vendor A has also defined some custom action types for their flavor of Android, just as Samsung has done. Suppose these types happen to coincide with these spen action types, but in a conflicting way. There's really no way for us to know if this will happen short of shipping Chrome and waiting to hear about any disasters, but the bugs could be more subtle and fly under the radar for a while. Or suppose Samsung decides to change these constants down the road via an OTA update. Are the constants documented anywhere? Perhaps in a public SDK? Does the spen have a corresponding ACTION_CANCEL type?
https://codereview.chromium.org/385963002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://codereview.chromium.org/385963002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentView.java:132: // S-Pen support: convert to normal stylus event handling Should this code be in CVC instead? This codepath isn't reached by webview
https://codereview.chromium.org/385963002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://codereview.chromium.org/385963002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentView.java:132: // S-Pen support: convert to normal stylus event handling On 2014/07/11 21:40:27, Yaron wrote: > Should this code be in CVC instead? This codepath isn't reached by webview OEMs can add the conversion logic in WebView.java separately. Do we still want to move it there? https://codereview.chromium.org/385963002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentView.java:133: final int spenActionDown = 11; On 2014/07/11 14:47:38, jdduke wrote: > > I have a few worries about this. > > Suppose Vendor A has also defined some custom action types for their flavor of > Android, just as Samsung has done. Suppose these types happen to coincide with > these spen action types, but in a conflicting way. There's really no way for us > to know if this will happen short of shipping Chrome and waiting to hear about > any disasters, but the bugs could be more subtle and fly under the radar for a > while. I think this is a valid concern. Let me add OEM check. > > Or suppose Samsung decides to change these constants down the road via an OTA > update. Are the constants documented anywhere? Perhaps in a public SDK? > > Does the spen have a corresponding ACTION_CANCEL type? Let me check these two with the OEM who requested this.
https://codereview.chromium.org/385963002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://codereview.chromium.org/385963002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentView.java:133: final int spenActionDown = 11; On 2014/07/11 14:47:38, jdduke wrote: > > I have a few worries about this. > > Suppose Vendor A has also defined some custom action types for their flavor of > Android, just as Samsung has done. Suppose these types happen to coincide with > these spen action types, but in a conflicting way. There's really no way for us > to know if this will happen short of shipping Chrome and waiting to hear about > any disasters, but the bugs could be more subtle and fly under the radar for a > while. > > Or suppose Samsung decides to change these constants down the road via an OTA > update. Are the constants documented anywhere? Perhaps in a public SDK? > > Does the spen have a corresponding ACTION_CANCEL type? OK, to summarize some of the downstream and offline discussion: if we have a sufficiently conservative check for both the SPen and Samsung device (using a combination of Build.* properties and perhaps the "com.sec.feature.spen_usp" system feature), the risk is greatly reduced. Would such a check be worth putting somewhere like ApiCompatibilityUtils.java? ApiCompatibilityUtils.isSPenSupported()? Probably use a static instance variable to avoid doing the check work repeatedly.
PTAL. Also note that this hasn't been fully tested as I'm working from home. If I happen to get approval by early next week, I'll fully test this before merging it. https://codereview.chromium.org/385963002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://codereview.chromium.org/385963002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentView.java:133: final int spenActionDown = 11; On 2014/07/11 22:17:40, jdduke wrote: > On 2014/07/11 14:47:38, jdduke wrote: > > > > I have a few worries about this. > > > > Suppose Vendor A has also defined some custom action types for their flavor of > > Android, just as Samsung has done. Suppose these types happen to coincide with > > these spen action types, but in a conflicting way. There's really no way for > us > > to know if this will happen short of shipping Chrome and waiting to hear about > > any disasters, but the bugs could be more subtle and fly under the radar for a > > while. > > > > Or suppose Samsung decides to change these constants down the road via an OTA > > update. Are the constants documented anywhere? Perhaps in a public SDK? > > > > Does the spen have a corresponding ACTION_CANCEL type? > > OK, to summarize some of the downstream and offline discussion: if we have a > sufficiently conservative check for both the SPen and Samsung device (using a > combination of Build.* properties and perhaps the "com.sec.feature.spen_usp" > system feature), the risk is greatly reduced. > > Would such a check be worth putting somewhere like ApiCompatibilityUtils.java? > ApiCompatibilityUtils.isSPenSupported()? Probably use a static instance variable > to avoid doing the check work repeatedly. ApiCompatibilityUtils is mostly concerned about Android API compatibility, so I wasn't sure if we'd want to change the semantics of the class. So I've ended up adding isSpenSupported() to ContentView.java. Also, it's not a static as it calls getContext(), but it refers to static variables.
https://codereview.chromium.org/385963002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://codereview.chromium.org/385963002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentView.java:132: // S-Pen support: convert to normal stylus event handling On 2014/07/11 22:14:20, Changwan Ryu wrote: > On 2014/07/11 21:40:27, Yaron wrote: > > Should this code be in CVC instead? This codepath isn't reached by webview > > OEMs can add the conversion logic in WebView.java separately. Do we still want > to move it there? I guess I'm worried about other programmatic invocations as well and don't really know whether they potentially need this special handling. +dtrainor who would have a better idea https://codereview.chromium.org/385963002/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://codereview.chromium.org/385963002/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentView.java:39: private static boolean mHasCheckedSPenSupport; sHasCheckedSPenSupport https://codereview.chromium.org/385963002/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentView.java:40: private static boolean mIsSPenSupported; sIsSPenSupported
https://codereview.chromium.org/385963002/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://codereview.chromium.org/385963002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentView.java:40: private static boolean mIsSPenSupported; Instead of having two variables you can change this to: private static Boolean sIsSPenSupported; ('s' instead of 'm'). Then, the nullity of that variable indicates whether it has been assigned. https://codereview.chromium.org/385963002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentView.java:138: private boolean isSPenSupported() { private static https://codereview.chromium.org/385963002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentView.java:140: return mIsSPenSupported; So this method becomes: private static boolean isSPenSupported() { if (sIsSPenSupported == null) sIsSpenSupported = detectSPenSupport(); return sIsSPenSupported.booleanValue(); } and place the detection code in |private static boolean detectSPenSupport()|. (See SysUtils.java:sLowEndDevice for an example).
PTAL https://codereview.chromium.org/385963002/diff/60001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://codereview.chromium.org/385963002/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentView.java:39: private static boolean mHasCheckedSPenSupport; On 2014/07/14 17:15:48, Yaron wrote: > sHasCheckedSPenSupport Not applicable as merged into sIsSPenSupported. https://codereview.chromium.org/385963002/diff/60001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentView.java:40: private static boolean mIsSPenSupported; On 2014/07/14 17:15:48, Yaron wrote: > sIsSPenSupported Done. https://codereview.chromium.org/385963002/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://codereview.chromium.org/385963002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentView.java:40: private static boolean mIsSPenSupported; On 2014/07/14 17:28:57, jdduke wrote: > Instead of having two variables you can change this to: > > private static Boolean sIsSPenSupported; ('s' instead of 'm'). Then, the > nullity of that variable indicates whether it has been assigned. Done. https://codereview.chromium.org/385963002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentView.java:138: private boolean isSPenSupported() { On 2014/07/14 17:28:57, jdduke wrote: > private static Done. https://codereview.chromium.org/385963002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentView.java:140: return mIsSPenSupported; Good idea! Done.
https://chromiumcodereview.appspot.com/385963002/diff/1/content/public/androi... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://chromiumcodereview.appspot.com/385963002/diff/1/content/public/androi... content/public/android/java/src/org/chromium/content/browser/ContentView.java:132: // S-Pen support: convert to normal stylus event handling On 2014/07/14 17:15:48, Yaron wrote: > On 2014/07/11 22:14:20, Changwan Ryu wrote: > > On 2014/07/11 21:40:27, Yaron wrote: > > > Should this code be in CVC instead? This codepath isn't reached by webview > > > > OEMs can add the conversion logic in WebView.java separately. Do we still want > > to move it there? > > I guess I'm worried about other programmatic invocations as well and don't > really know whether they potentially need this special handling. +dtrainor who > would have a better idea We recently moved the logic from onTouchEvent here to ContentViewCore. I'd rather we keep this class as dumb as possible and move this logic to ContentViewCore. If we decide we don't want this working on WebView we can special case this and move it back IMO.
Agreed about moving to ContentViewCore, if the OEM is customizing WebView internals, they should be able to customize the Chromium dependency internals as well. https://codereview.chromium.org/385963002/diff/100001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://codereview.chromium.org/385963002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentView.java:41: private static final int SPEN_ACTION_DOWN = 0xFF11; We probably want to use |MotionEvent.getActionMasked()|, and then these values can be 11, 12, 13, 14. Also, maybe add a short comment about how these constant values were obtained (experimentally). https://codereview.chromium.org/385963002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentView.java:148: FeatureInfo[] infos = context.getPackageManager().getSystemAvailableFeatures(); Nit: "final FeatureInfo[] ...".
https://chromiumcodereview.appspot.com/385963002/diff/1/content/public/androi... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://chromiumcodereview.appspot.com/385963002/diff/1/content/public/androi... content/public/android/java/src/org/chromium/content/browser/ContentView.java:132: // S-Pen support: convert to normal stylus event handling On 2014/07/15 00:38:08, David Trainor wrote: > On 2014/07/14 17:15:48, Yaron wrote: > > On 2014/07/11 22:14:20, Changwan Ryu wrote: > > > On 2014/07/11 21:40:27, Yaron wrote: > > > > Should this code be in CVC instead? This codepath isn't reached by webview > > > > > > OEMs can add the conversion logic in WebView.java separately. Do we still > want > > > to move it there? > > > > I guess I'm worried about other programmatic invocations as well and don't > > really know whether they potentially need this special handling. +dtrainor who > > would have a better idea > > We recently moved the logic from onTouchEvent here to ContentViewCore. I'd > rather we keep this class as dumb as possible and move this logic to > ContentViewCore. If we decide we don't want this working on WebView we can > special case this and move it back IMO. Sounds good. Done. https://chromiumcodereview.appspot.com/385963002/diff/100001/content/public/a... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://chromiumcodereview.appspot.com/385963002/diff/100001/content/public/a... content/public/android/java/src/org/chromium/content/browser/ContentView.java:41: private static final int SPEN_ACTION_DOWN = 0xFF11; On 2014/07/15 00:50:41, jdduke wrote: > We probably want to use |MotionEvent.getActionMasked()|, and then these values > can be 11, 12, 13, 14. Also, maybe add a short comment about how these constant > values were obtained (experimentally). These are through an official channel, and now we're using getActionMasked(). Not sure how I should best comment these now. https://chromiumcodereview.appspot.com/385963002/diff/100001/content/public/a... content/public/android/java/src/org/chromium/content/browser/ContentView.java:148: FeatureInfo[] infos = context.getPackageManager().getSystemAvailableFeatures(); On 2014/07/15 00:50:42, jdduke wrote: > Nit: "final FeatureInfo[] ...". Done.
lgtm please wait for jdduke's approval
https://chromiumcodereview.appspot.com/385963002/diff/100001/content/public/a... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://chromiumcodereview.appspot.com/385963002/diff/100001/content/public/a... content/public/android/java/src/org/chromium/content/browser/ContentView.java:41: private static final int SPEN_ACTION_DOWN = 0xFF11; On 2014/07/15 01:05:07, Changwan Ryu wrote: > On 2014/07/15 00:50:41, jdduke wrote: > > We probably want to use |MotionEvent.getActionMasked()|, and then these values > > can be 11, 12, 13, 14. Also, maybe add a short comment about how these > constant > > values were obtained (experimentally). > > These are through an official channel, and now we're using getActionMasked(). > Not sure how I should best comment these now. Ah, OK, then let's stick with getAction, and just mention in the comment that the values were provided from Samsung.
https://chromiumcodereview.appspot.com/385963002/diff/100001/content/public/a... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://chromiumcodereview.appspot.com/385963002/diff/100001/content/public/a... content/public/android/java/src/org/chromium/content/browser/ContentView.java:41: private static final int SPEN_ACTION_DOWN = 0xFF11; On 2014/07/15 01:31:08, jdduke wrote: > On 2014/07/15 01:05:07, Changwan Ryu wrote: > > On 2014/07/15 00:50:41, jdduke wrote: > > > We probably want to use |MotionEvent.getActionMasked()|, and then these > values > > > can be 11, 12, 13, 14. Also, maybe add a short comment about how these > > constant > > > values were obtained (experimentally). > > > > These are through an official channel, and now we're using getActionMasked(). > > Not sure how I should best comment these now. > > Ah, OK, then let's stick with getAction, and just mention in the comment that > the values were provided from Samsung. > Could you check the code again? Now it's inside CVC and I'm not sure using getAction() would make sense any more. Added comment as suggested.
lgtm per our offline discussion to remove the "0xFF" prefix on the constants, as it appears Samsung provided us some incorrect hybrid decimal/hex values =/
https://codereview.chromium.org/385963002/diff/140001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/385963002/diff/140001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1152: private static int convertSPenEventAction(int eventAction) { And let's make this |eventActionMasked|, thanks!
https://codereview.chromium.org/385963002/diff/140001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/385963002/diff/140001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1152: private static int convertSPenEventAction(int eventAction) { On 2014/07/15 01:47:09, jdduke wrote: > And let's make this |eventActionMasked|, thanks! Done.
The CQ bit was checked by changwan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/changwan@chromium.org/385963002/180001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) 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_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was checked by changwan@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/changwan@chromium.org/385963002/180001
Message was sent while issue was closed.
Change committed as 283294 |
