Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
901183002:
Enable smooth scroll/pan through gamepad (Closed)
DescriptionEnable smooth scroll/pan through gamepad
This change allows smooth scrolling and panning
using gamepad left stick.
BUG=454355
Committed: https://crrev.com/c631fde80e1e8166dc933a497626f04c0f5d1ed1
Cr-Commit-Position: refs/heads/master@{#340459}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Enable smooth scroll/pan through gamepad #
Total comments: 23
Patch Set 3 : Enable smooth scroll/pan through gamepad #
Total comments: 3
Patch Set 4 : Enable smooth scroll/pan through gamepad #Patch Set 5 : Enable smooth scroll/pan through gamepad #
Total comments: 9
Patch Set 6 : Enable smooth scroll/pan through gamepad #
Total comments: 21
Patch Set 7 : Enable smooth scroll/pan through gamepad #
Total comments: 21
Patch Set 8 : Enable smooth scroll/pan through gamepad #
Total comments: 7
Patch Set 9 : Enable smooth scroll/pan through gamepad #
Total comments: 10
Patch Set 10 : Enable smooth scroll/pan through gamepad #
Total comments: 22
Patch Set 11 : Enable smooth scroll/pan through gamepad #
Total comments: 6
Patch Set 12 : fixed comments + rebase #
Total comments: 2
Patch Set 13 : fixed previous comment #Patch Set 14 : Rebase #
Messages
Total messages: 70 (9 generated)
sshelke@nvidia.com changed reviewers: + jdduke@chromium.org
+rbyers@ for additional thoughts. Certainly this will work, but before we proceed further I think we'd like to agree on a structured approach for exposing gamepad motion/intent to web content and/or default browser actions. After all, something like this would also benefit, say, Linux with SteamOS or other media PCs hosted on a different OS. https://codereview.chromium.org/901183002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/901183002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1765: nativeScrollBegin(mNativeContentViewCore, mDownTime, I wonder if wheel events would be a more faithful emulation pattern here? In theory we can make wheel events scroll smoothly on Android (I don't believe they do currently, but that could be changed). In any case, this is quite a lot of logic to add to ContentViewCore, so we probably need to find a better place for it (I would hold off on that until we can reach a general consensus on the emulation approach).
On 2015/02/05 21:49:15, jdduke wrote: > +rbyers@ for additional thoughts. Certainly this will work, but before we > proceed further I think we'd like to agree on a structured approach for exposing > gamepad motion/intent to web content and/or default browser actions. After all, > something like this would also benefit, say, Linux with SteamOS or other media > PCs hosted on a different OS. I haven't looked at the patch in detail yet. A key question is how does one determine the point of focus with the gamepad? Is there a mouse cursor? When scrolling, what should happen as the thing moves underneath the point of interest? If there's a mouse cursor then you probably want to follow the wheel model of scrolling whatever is currently under the cursor. If there isn't then perhaps you want the gesture module of latching to an element at the start of a scroll, then gestures are probably more appropriate. Also what should happen in apps like google maps when you try to scroll with a gamepad? If you send wheel events then you'll get zoom. If you send just gesture events then you'll get nothing. Does gamepad generate mouse / touch events in other scenarios?
>> Is there a mouse cursor? In gamepad, right stick is mapped to mouse cursor, so we can determine point of focus. But in implementation we have not considered point of focus. Left stick scrolling behaves same as scrolling through swipe. >> Also what should happen in apps like google maps when you try to scroll with a gamepad? So far, I tried scrolling with wheel based model in browser. In this model, left stick motion events are translated into number of pixels(x,y) and page will be scrolled with these many pixels. Have not tried this approach on any app, so not sure about result. But, if webpage handles gamepad related events (may be through java scripts) then we don't take any action. Consider the example of gamepad api in which user can assign functionality to buttons/axes.If left stick is assigned for different functionality then scrolling through left stick will not happened. >> Does gamepad generate mouse / touch events in other scenarios? Yes, mouse events are emulated using right stick in gamepad (for NVIDIA shield). Gamepad mapping is vendor specific. On 2015/02/08 06:05:07, Rick Byers wrote: > On 2015/02/05 21:49:15, jdduke wrote: > > +rbyers@ for additional thoughts. Certainly this will work, but before we > > proceed further I think we'd like to agree on a structured approach for > exposing > > gamepad motion/intent to web content and/or default browser actions. After > all, > > something like this would also benefit, say, Linux with SteamOS or other media > > PCs hosted on a different OS. > > I haven't looked at the patch in detail yet. A key question is how does one > determine the point of focus with the gamepad? Is there a mouse cursor? When > scrolling, what should happen as the thing moves underneath the point of > interest? If there's a mouse cursor then you probably want to follow the wheel > model of scrolling whatever is currently under the cursor. If there isn't then > perhaps you want the gesture module of latching to an element at the start of a > scroll, then gestures are probably more appropriate. > > Also what should happen in apps like google maps when you try to scroll with a > gamepad? If you send wheel events then you'll get zoom. If you send just > gesture events then you'll get nothing. > > Does gamepad generate mouse / touch events in other scenarios?
Jared, I am still figuring out better place to expose gamepad motion events to web content.I was going through Gamepad API implementation (Issue 330094), where gamepad events are exposed through GamepadDevice class. But this implementation seems to be android specific and Gamepad API needs to be used at generic level (all os and platforms). So are we planning to change GamepadAPI impelementation in near future? Could you provide me pointer to look at particular module for implementation? I have been looking at content/public/browser module for implementation. On 2015/02/05 21:49:15, jdduke wrote: > +rbyers@ for additional thoughts. Certainly this will work, but before we > proceed further I think we'd like to agree on a structured approach for exposing > gamepad motion/intent to web content and/or default browser actions. After all, > something like this would also benefit, say, Linux with SteamOS or other media > PCs hosted on a different OS. > > https://codereview.chromium.org/901183002/diff/1/content/public/android/java/... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/901183002/diff/1/content/public/android/java/... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1765: > nativeScrollBegin(mNativeContentViewCore, mDownTime, > I wonder if wheel events would be a more faithful emulation pattern here? In > theory we can make wheel events scroll smoothly on Android (I don't believe they > do currently, but that could be changed). In any case, this is quite a lot of > logic to add to ContentViewCore, so we probably need to find a better place for > it (I would hold off on that until we can reach a general consensus on the > emulation approach).
On 2015/02/12 14:55:10, sshelke wrote: > Jared, > I am still figuring out better place to expose gamepad motion events to web > content.I was > going through Gamepad API implementation (Issue 330094), where gamepad events > are exposed > through GamepadDevice class. But this implementation seems to be android > specific and > Gamepad API needs to be used at generic level (all os and platforms). So are we > planning > to change GamepadAPI impelementation in near future? > Could you provide me pointer to look at particular module for implementation? > I have been looking at content/public/browser module for implementation. It's a good question, and I don't have a good answer. Outside of the HTML5 gamepad spec I don't believe gamepad events are exposed to the web in a structured way. I think there may have been a proposal to craft such a spec, but I don't know of the details. As for control of web contents, ideally the default interaction here would be identical to how the gamepad controls native Android views and widgets. Is there standardized behavior for this, i.e., if the corresponding MotionEvent/KeyEvents go unhandled by a View or Widget, what is the typical default resulting action? Will the joystick scroll the View for most Android apps?
On 2015/02/12 18:31:11, jdduke wrote: > On 2015/02/12 14:55:10, sshelke wrote: > > Jared, > > I am still figuring out better place to expose gamepad motion events to web > > content.I was > > going through Gamepad API implementation (Issue 330094), where gamepad events > > are exposed > > through GamepadDevice class. But this implementation seems to be android > > specific and > > Gamepad API needs to be used at generic level (all os and platforms). So are > we > > planning > > to change GamepadAPI impelementation in near future? > > Could you provide me pointer to look at particular module for implementation? > > > I have been looking at content/public/browser module for implementation. > > It's a good question, and I don't have a good answer. Outside of the HTML5 > gamepad spec I don't believe gamepad events are exposed to the web in a > structured way. I think there may have been a proposal to craft such a spec, but > I don't know of the details. > > As for control of web contents, ideally the default interaction here would be > identical to how the gamepad controls native Android views and widgets. Is there > standardized behavior for this, i.e., if the corresponding MotionEvent/KeyEvents > go unhandled by a View or Widget, what is the typical default resulting action? > Will the joystick scroll the View for most Android apps? Joystick event is handled in contentviewcore for android implementation. If event is not handled in viewcore then it is passed to ViewRootImpl class where event is translated to DPAD events. So,View is scrolled (rather say moved) using DPAD events. >> nativeScrollBegin(mNativeContentViewCore, mDownTime, >> I wonder if wheel events would be a more faithful emulation pattern here? nativeScrollBegin uses SendGesture method underneath for scrolling. So, scrolling is emulated using gesture based model rather than wheelbased model. nativeScrollBegin API implementation is provided in content_view_core_impl.cc in content/browser module. Will it be fine to use gesture based emulation?
https://codereview.chromium.org/901183002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/901183002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1697: getJoystickMotionEventScrollPixels(event, MotionEvent.AXIS_X), Does this distinguish between analog/digital joystick motion? I think it's OK for us to handle analog sticks explicitly here, but digital dpad motion should probably propagate as it did previously using arrow key simulation. Also, is there anything wrong with this simulating dpad motion, other than arrow key movement being jerky? What if that we're smoothed? https://codereview.chromium.org/901183002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1704: event.setLocation(0, 0); Can't you just return true here?
>Does this distinguish between analog/digital joystick motion? I think it's OK >for us to handle analog sticks explicitly here, but digital dpad motion should >probably propagate as it did previously using arrow key simulation. Yes, it does distinguish between analog/digital motion. We have send MotionEvent.AXIS_X in getJoystickMotionEventScrollPixels which is mapped to left analog stick instead of MotionEvent.AXIS_HAT_X which is mapped to Dpad. Events related to Dpad will be propagated to parent view since we have not handled them here. > Also, is there anything wrong with this simulating dpad motion, other than arrow > key movement being jerky? What if that we're smoothed? Could you please clarify more? >Can't you just return true here? In this handler, we are passing all events (RS,LS,DPAD)to parent view. If we return true here, DPAD events and RS won't be passed to parent view to perform their default/fallback operations. We are even allowing LS event to propagate,since there is no default action/fallback operations to be performed by parent view, it will just discard the event.
https://codereview.chromium.org/901183002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/901183002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1704: event.setLocation(0, 0); On 2015/03/16 21:46:17, jdduke wrote: > Can't you just return true here? So the system generates DPAD events for both AXIS_* motion and AXIS_HAT_* motion? https://codereview.chromium.org/901183002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1739: throw new IllegalStateException( We should probably log an error here and have some graceful default fallback. https://codereview.chromium.org/901183002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1748: class AutoScroller implements Runnable { I'd be a lot happier if this class and the other logic could be pulled out into a separate class. So we'd have something like: final JoystickScrollProvider mJoystickScrollProvider; ... mJoystickScrollProvider = new JoystickScrolllProvider(this); ... mJoystickScrollProvider.onMotionEvent(event); and it deals with the runnable and such (shouldn't the runnable be synchronized with animate, e.g., ApiCompatibilityUtils.postOnAnimation(view, mRunnable))?
https://codereview.chromium.org/901183002/diff/1/content/public/android/java/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/901183002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1704: event.setLocation(0, 0); On 2015/03/17 16:40:38, jdduke wrote: > On 2015/03/16 21:46:17, jdduke wrote: > > Can't you just return true here? > > So the system generates DPAD events for both AXIS_* motion and AXIS_HAT_* > motion? System generates AXIS_HAT_* for Dpad events and AXIS_* for left joystick. But if we don't handle AXIS_* in content view handler, parent view i.e ViewRootImpl simulate it as Dpad event. https://codereview.chromium.org/901183002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1739: throw new IllegalStateException( On 2015/03/17 16:40:38, jdduke wrote: > We should probably log an error here and have some graceful default fallback. I will add fallback and log in next patch. https://codereview.chromium.org/901183002/diff/1/content/public/android/java/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1748: class AutoScroller implements Runnable { On 2015/03/17 16:40:38, jdduke wrote: > I'd be a lot happier if this class and the other logic could be pulled out into > a separate class. > > So we'd have something like: > > final JoystickScrollProvider mJoystickScrollProvider; > > ... > > mJoystickScrollProvider = new JoystickScrolllProvider(this); > > ... > > mJoystickScrollProvider.onMotionEvent(event); > > > and it deals with the runnable and such (shouldn't the runnable be synchronized > with animate, e.g., ApiCompatibilityUtils.postOnAnimation(view, mRunnable))? Yeah, even I was thinking to add new class. It is better to use OnAnimation than postDelayed. I will make these changes.
For better or worse, reviewers aren't notified of code changes/uploads, you can either "Publish + Mail comments" or reply to the thread to notify the reviewers that the patch is ready for further review.
https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:425: private final JoystickScrollProvider mScrollProvider; Let's call this |mJoystickScrollProvider|. https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1565: mShowingSoftKeyboard = false; Should we move this to hideImeIfNeeded()? https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1678: event.setLocation(0, 0); This location zeroing is a bit concerning, it's at least deserving of a comment. Can you point me to the Android source location where this effectively disables the default scroll action for Android? https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:23: private Point mScrollPosPix; Let's call this |mTargetScrollOffsetPix| https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:28: private static final float JOYSTICK_SCROLL_FACTOR_MULTIPLIER = (float) 2.0; Let's move the static finals to the top. https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:40: mRenderCoordinates = mView.getRenderCoordinates(); I'm not sure we should be caching this, what if ContentViewCore decides to create a new set of render coordinates? https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:53: ApiCompatibilityUtils.postOnAnimation(mView.getContainerView(), this); What's the reason to do this in the animate callback? We're not actually animating a smooth scroll curve here (which I thought was the original intent). Is this to ensure we scroll but once/frame? https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:67: public void onMotion(MotionEvent event) { Need comments. https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:71: if (!mAutoScrollEnabled) Nit: always needs braces if the body is on a new line. https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:72: run(); Hmm, what about the timing of the events relative to the animation? It seems like we could get 2 scrolls per frame here initially, but maybe that's not such a bad thing? https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:86: Point pos = new Point(); This should just be |computeNewScrollOffset| and have it directly update mScrollPosPix. No need to create garbage after every event.
https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:425: private final JoystickScrollProvider mScrollProvider; On 2015/04/06 15:28:26, jdduke wrote: > Let's call this |mJoystickScrollProvider|. Done. https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1565: mShowingSoftKeyboard = false; On 2015/04/06 15:28:26, jdduke wrote: > Should we move this to hideImeIfNeeded()? Done. https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1678: event.setLocation(0, 0); On 2015/04/06 15:28:26, jdduke wrote: > This location zeroing is a bit concerning, it's at least deserving of a comment. > Can you point me to the Android source location where this effectively disables > the default scroll action for Android? Possible comment is as follows. Android framework generates secondary DPAD_[DIRECTION] events from unhandled (LS,RS)joystick events. These events cause webview focus to change to next focus-able item or force default scroll action. Setting the location to 0,0 effectively disable these DPAD events. https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:23: private Point mScrollPosPix; On 2015/04/06 15:28:26, jdduke wrote: > Let's call this |mTargetScrollOffsetPix| Will modify name of variable. https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:28: private static final float JOYSTICK_SCROLL_FACTOR_MULTIPLIER = (float) 2.0; On 2015/04/06 15:28:26, jdduke wrote: > Let's move the static finals to the top. Yep, sure will combine all static definitions in a group. https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:40: mRenderCoordinates = mView.getRenderCoordinates(); On 2015/04/06 15:28:26, jdduke wrote: > I'm not sure we should be caching this, what if ContentViewCore decides to > create a new set of render coordinates? Each ContentViewCore creates single render coordinate object in its constructor. With each viewCore, there is associated frame and render coordinates caches this frame coordinates. Is there any possible scenario where viewCore would have two possible frames attached to it? I mean, is there any condition or scenario where content view will need multiple set of render coordinates? If yes, then we might need to add function which will translate normalized event position to pixel coordinates using screen height and width. https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:53: ApiCompatibilityUtils.postOnAnimation(mView.getContainerView(), this); On 2015/04/06 15:28:26, jdduke wrote: > What's the reason to do this in the animate callback? We're not actually > animating a smooth scroll curve here (which I thought was the original intent). > Is this to ensure we scroll but once/frame? Yes, we want view to be scrolled with a single value in a frame interval. Could you please explain me how to animate a smooth scroll curve? I am not aware of it. https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:67: public void onMotion(MotionEvent event) { On 2015/04/06 15:28:26, jdduke wrote: > Need comments. Will add. https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:71: if (!mAutoScrollEnabled) On 2015/04/06 15:28:26, jdduke wrote: > Nit: always needs braces if the body is on a new line. Done. https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:72: run(); On 2015/04/06 15:28:26, jdduke wrote: > Hmm, what about the timing of the events relative to the animation? It seems > like we could get 2 scrolls per frame here initially, but maybe that's not such > a bad thing? Could you explain, how view will get 2 scrolls initially? for example, lets say 3 events arrived at intervals t= 2, t= 6, t= 12 At t=2 OnGenericMotion will be called which updates JoystickScrollProvider scroll offset and calls scrollBy API to scroll view and makes mAutoScrollEnabled = true. At t=6, OnGenericMotion will be called which updates JoystickScrollProvider scroll offset but wont be able to call scrollBy API since mAutoScrollEnabled is already true. At t= 12, OnGenericMotion will be called , overwrites JoystickScrollProvider scroll offset but wont be able to call scrollBy API since mAutoScrollEnabled is already true. At t= 16 (next frame interval) JoystickScrollProvider kicks off and scrolls view with offset value provided in t=12 Joystick event at t=6 is discarded but it wont be noticeable to user. https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:86: Point pos = new Point(); On 2015/04/06 15:28:26, jdduke wrote: > This should just be |computeNewScrollOffset| and have it directly update > mScrollPosPix. No need to create garbage after every event. Done.
Can you upload the new patchset? I don't see any of the changes (note that code review in general has been having some issues lately, so apologies if you were bumping into that).
On 2015/04/13 23:30:49, jdduke wrote: > Can you upload the new patchset? I don't see any of the changes (note that code > review in general has been having some issues lately, so apologies if you were > bumping into that). Uploaded new patch. Please review.
https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/20001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:40: mRenderCoordinates = mView.getRenderCoordinates(); On 2015/04/07 11:13:39, sshelke_ooo_16Apr_6May wrote: > On 2015/04/06 15:28:26, jdduke wrote: > > I'm not sure we should be caching this, what if ContentViewCore decides to > > create a new set of render coordinates? > > Each ContentViewCore creates single render coordinate object in its constructor. > With each viewCore, there is associated frame and render coordinates caches this > frame coordinates. > Is there any possible scenario where viewCore would have two possible frames > attached to it? > > I mean, is there any condition or scenario where content view will need multiple > set of render coordinates? > If yes, then we might need to add function which will translate normalized event > position to pixel coordinates using screen height and width. > > > > I mean, we should probably just call |mView.getRenderCoordinates()| instead of caching the mRenderCoordinates. That ContentViewCore is using just one instance is an implementation details I don't want to rely on. https://codereview.chromium.org/901183002/diff/40001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:26: private static final float JOYSTICK_SCROLL_DEADZONE = (float) 0.2; Nit: Just add a "f" suffix instead of the float casting. Also, a brief comment for SCROLL_FACTOR_FALLBACK might be worthwhile as it's not clear what it means. https://codereview.chromium.org/901183002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:40: mScaledTouchSlope = ViewConfiguration.get(mContext).getScaledTouchSlop(); Nit: Slope -> Slop https://codereview.chromium.org/901183002/diff/40001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:56: mView.scrollTo(xScrollPix, yScrollPix); Hmm, I'm not really comfortable using |scrollTo| here. It's actually a pretty bad method that is highly racy. Would it be possible for us to use a |scrollBy| variant instead? I think that will eliminate races and give us more consistent behavior. The current version of scrollBy probably won't work, but I can create a fixed version that should. Then I think we can get rid of all this target offset computation, and we can simply use the magnitude of the joystick offset combined with the time delta to compute a desired scroll delta, i.e., the joystick offset then becomes the "velocity", and with the animation ticks we compute discete displacements. Thoughts?
FWIW, here's a version I played with locally that uses scrollBy + velocity, https://codereview.chromium.org/1089663002. If you think that's reasonable, I can either patch the scrollBy now or you can just include it in your patch. Either way, let me know your preference. Regardless, it feels pretty good, thanks for the addition!
On 2015/04/15 20:47:39, jdduke wrote: > FWIW, here's a version I played with locally that uses scrollBy + velocity, > https://codereview.chromium.org/1089663002. If you think that's reasonable, I > can either patch the scrollBy now or you can just include it in your patch. > Either way, let me know your preference. > > Regardless, it feels pretty good, thanks for the addition! Hi Jared, I could not access internet when I was ooo, so sorry for delayed response. I am glad you have tried different approach and it seems reasonable to me. I also tried https://codereview.chromium.org/1089663002 change on our device, scrolling seems smooth. I have some small doubts which I asked on the patch. I would be glad if you clarify those doubts.
On 2015/05/12 12:11:16, sshelke wrote: > On 2015/04/15 20:47:39, jdduke wrote: > > FWIW, here's a version I played with locally that uses scrollBy + velocity, > > https://codereview.chromium.org/1089663002. If you think that's reasonable, I > > can either patch the scrollBy now or you can just include it in your patch. > > Either way, let me know your preference. > > > > Regardless, it feels pretty good, thanks for the addition! > > Hi Jared, > I could not access internet when I was ooo, so sorry for delayed response. > I am glad you have tried different approach and it seems reasonable to me. > I also tried https://codereview.chromium.org/1089663002 change on our device, > scrolling seems smooth. > I have some small doubts which I asked on the patch. I would be glad if you > clarify those doubts. No problem, I posted a few comments on that change.
Modified patch so that focused element should be scrolled instead of entire view. Please review and let me know your feedback.
On 2015/05/28 13:01:23, sshelke wrote: > Modified patch so that focused element should be > scrolled instead of entire view. Please review and > let me know your feedback. Hmm, I think that might be an improvement, but I wonder if we'll run into consistency/correctness issues. What if, for the V1 version of this change, we go with the original plan just targeting (0,0), then we can adjust it as we get feedback? I'd almost rather do that than guess based on the mouse position, which may or may not be current (unless the mouse position can be dictated by the gamepad? Can it?).
Current cursor position is determined not only by mouse event but also by touch event whichever is recent. >> mouse position can be dictated by the gamepad? Can it?) There are couple of use cases where mouse can be dictated by gamepad. 1) In shield portable where gamepad is attached to device itself, Right stick (RS) emulates as a mouse, which is used for navigation. We have added support for gamepad controller in Android browser (before webview made GMS prebuilt), where LS was used for smooth scrolling and RS was used as mouse for page navigation and selecting links or elements on webpage. 2) Recently, I have tried opera browser on ATV and I have observed that they have used D-PAD and LS as mouse. So, they are achieving page navigation using mouse mapped to D-PAD or LS. On similar lines, if chrome is used for ATV platforms (just an assumption) we need to use D-PAD or RS to emulate mouse for navigation and selecting elements. If we install chrome browser on shield portable and if select iframe using mouse and try to scroll using LS then whole view will be scrolled instead of iframe. Our logic of targeting(0,0) will look inaccurate. Anyways, If you think latest patch leads to consistency/correctness issues, we can move to our original patch. Not an issue. Let me know your view, I will modify patch accordingly.
Basic Joystick scrolling tests are added in ContentViewScrollingTest.java file. Please review.
Thanks, I think we can try to run with this and see how it works. https://codereview.chromium.org/901183002/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/901183002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:556: private float mLastTapX; It looks like mLastTapX is no longer used by anybody. I'd be OK if we changed the variable to something like "mLastFocalEventX". Then we don't need the time, and we just update it based on the mouse move location and the tap location. Thoughts? https://codereview.chromium.org/901183002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1823: * @see View#scrollBy(float, float, float, float) This signature should not change. https://codereview.chromium.org/901183002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1829: public void scrollBy(float cursorPosX, float cursorPosY, float dxPix, float dyPix) { Let's just call these arguments "float x, float y, float dxPix, float dyPix);" https://codereview.chromium.org/901183002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1846: public void scrollTo(float cursorPosX, float cursorPosY, float xPix, float yPix) { I don't think we need to change the signature of this method. https://codereview.chromium.org/901183002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1965: public long getLastTapEventTime() { See my comments above about repurposing "mLastTap" variables. https://codereview.chromium.org/901183002/diff/80001/content/public/android/j... File content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:49: mCursorPos = getCursorPos(); Let's avoid creating a new PointF each frame, we can just query the x/y values independently (and it should be simplified b/c of the new variable type in ContentViewCore). https://codereview.chromium.org/901183002/diff/80001/content/public/android/j... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:123: return ((axisValWithNoise > JOYSTICK_SCROLL_DEADZONE) Can you separate out these branches? It's a bit hard to read, i.e., if (axisValWithNoise > ...) return axisValWithNoise - JOYSTICK_SCROLL_DEADZONE; if (axisValWithNoise < ...) return ...; return 0; https://codereview.chromium.org/901183002/diff/80001/content/public/android/j... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java (right): https://codereview.chromium.org/901183002/diff/80001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java:278: assertWaitForScroll(true, true); Unfortunately, both the "No scroll" and "deadzone" calls to "assertWaitForScroll" calls are racy. That is, the conditions are already satisfied before the async action begins, so we're not necessarily testing the final side effects. I'm not sure of the best way to check this, it might require some additional synthetic delay before we actually check what happened, but that can also be racy so it's only marginally better. Probably the best thing would be a proper unit test, but then we'd have to add a bunch of mocking. Maybe for now just add a // TODO(jdduke): Make the deadzone scroll checks non-racy. https://codereview.chromium.org/901183002/diff/80001/content/public/android/j... content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java:291: // send joystick event to stop scrolling. Nit: Uppercase, maybe say "Send joystick event at origin to stop scrolling".
Thanks Jared for quick reply, Modified patch according to suggestion/review comments provided in previous patch.
jdduke@chromium.org changed reviewers: + aurimas@chromium.org
+aurimas for thoughts on the keyboard logic here. I'd love for us to not expose this from ContentViewCore, ideally we could just set a bit on the scroll provider when the keyboard state changes. https://codereview.chromium.org/901183002/diff/100001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/901183002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:555: // Store the x, y coordinates of the last focal event. maybe add some clarification, "last focal event, either from a tap event or mouse movement". https://codereview.chromium.org/901183002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:596: private boolean mShowingSoftKeyboard; What if, instead of exposing this flag, we expose a "setEnabled" call on the scroll provider, and we just set it to true/false when the keyboard state changes? https://codereview.chromium.org/901183002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:663: public boolean getSoftKeyboardShowFlag() { This needs a Javadoc, and should have a line break after the function ending brace. https://codereview.chromium.org/901183002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1793: event.setLocation(0, 0); Should we only set the location to (0, 0) if the event is "consumed" in some fashion? https://codereview.chromium.org/901183002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1839: public void scrollTo(float x, float y, float xPix, float yPix) { Hmm, I still think we should leave this public API alone, just have it take an xPix, yPix, and we'll then call scrollBy(0, 0, dxPix, dyPix) internally. https://codereview.chromium.org/901183002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1939: mLastFocalEventX = xPix; Nit: one space after "=" https://codereview.chromium.org/901183002/diff/100001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:20: private static final float JOYSTICK_SCROLL_FACTOR_MULTIPLIER = 25f; Nit: Let's insert a linke break after the TAG. https://codereview.chromium.org/901183002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:27: private final PointF mScrollVelocity = new PointF(); Nit: I would just make two mScrollVelocityX, mScrollVelocityY float components.
It does seem that we are adding quite a bit to ContentViewCore. I would prefer if we pulled out more logic to do with scrolling out of it if we can. https://codereview.chromium.org/901183002/diff/100001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:18: public class JoystickScrollProvider implements Runnable { Can this live under content/browser/input/ ? https://codereview.chromium.org/901183002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:48: mView.scrollBy(mView.getLastFocalEventX(), Passing getLastFocalEventX() values from ContentViewCore to ContentViewCore seems odd. Should we instead make scrollBy(float dx,float dy,boolean useLastFocalEvent)?
https://codereview.chromium.org/901183002/diff/100001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:48: mView.scrollBy(mView.getLastFocalEventX(), On 2015/06/03 18:40:49, aurimas wrote: > Passing getLastFocalEventX() values from ContentViewCore to ContentViewCore > seems odd. Should we instead make scrollBy(float dx,float dy,boolean > useLastFocalEvent)? I like that, maybe "targetLastFocalEventLocation" or "useLastFocalEventLocation". Eventually we might make it an enum as we'd like to be able to optionally target the (root) viewport layer.
Patchset #7 (id:120001) has been deleted
https://codereview.chromium.org/901183002/diff/100001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/901183002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:555: // Store the x, y coordinates of the last focal event. On 2015/06/03 16:37:33, jdduke wrote: > maybe add some clarification, "last focal event, either from a tap event or > mouse movement". Done. https://codereview.chromium.org/901183002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:596: private boolean mShowingSoftKeyboard; Removed keyboard flag and its related code. It seems there is no need to maintain keyboard status for joystick scrolling. We will check whether view is focused or not, if its not focused then stop joystick scrolling. Previously, we were using hasWindowFocus() to check window focus, instead of that we can use isFocused() api. https://codereview.chromium.org/901183002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1793: event.setLocation(0, 0); We will handle Left joystick (LS) event and return with true value from OnGenericMotion handler. For other events such as RS, DPAD, we will pass them to framework to process. No need to use event.setLocation() api. so logic would be, if(mJoystickScrollProvider.onMotion(event)) return true; https://codereview.chromium.org/901183002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1839: public void scrollTo(float x, float y, float xPix, float yPix) { Changed previous signature of scrollTo api. The new signature is scrollTo(float, float) and it internally calls scrollBy(float, float, false) api. But above implementation raised a question. ScrollTo will always be scrolled from origin which is (0,0). If mouse is pointing to iframe and if there is call for scrollTo api to scroll till some location. Instead of iframe scrolling to destined location, entire view will be scrolled to the location. https://codereview.chromium.org/901183002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1939: mLastFocalEventX = xPix; On 2015/06/03 16:37:33, jdduke wrote: > Nit: one space after "=" Done. https://codereview.chromium.org/901183002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1939: mLastFocalEventX = xPix; On 2015/06/03 16:37:33, jdduke wrote: > Nit: one space after "=" Done. https://codereview.chromium.org/901183002/diff/100001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:18: public class JoystickScrollProvider implements Runnable { On 2015/06/03 18:40:49, aurimas wrote: > Can this live under content/browser/input/ ? Done. https://codereview.chromium.org/901183002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:20: private static final float JOYSTICK_SCROLL_FACTOR_MULTIPLIER = 25f; On 2015/06/03 16:37:33, jdduke wrote: > Nit: Let's insert a linke break after the TAG. Done. https://codereview.chromium.org/901183002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:27: private final PointF mScrollVelocity = new PointF(); On 2015/06/03 16:37:33, jdduke wrote: > Nit: I would just make two mScrollVelocityX, mScrollVelocityY float components. Done. https://codereview.chromium.org/901183002/diff/100001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/JoystickScrollProvider.java:48: mView.scrollBy(mView.getLastFocalEventX(), On 2015/06/03 18:40:49, aurimas wrote: > Passing getLastFocalEventX() values from ContentViewCore to ContentViewCore > seems odd. Should we instead make scrollBy(float dx,float dy,boolean > useLastFocalEvent)? Done.
Awesome, I think after these last nits are resolved I can rubberstamp approve this for landing. https://codereview.chromium.org/901183002/diff/140001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://codereview.chromium.org/901183002/diff/140001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentView.java:189: mContentViewCore.scrollTo(0, 0); Hmm, I don't think this change was intentional was it? https://codereview.chromium.org/901183002/diff/140001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/901183002/diff/140001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:504: private final JoystickScrollProvider mJoystickScrollProvider; Maybe insert a line break here and add a comment: "Provides smooth gamepad joystick-driven scrolling, lazily created." https://codereview.chromium.org/901183002/diff/140001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:628: mJoystickScrollProvider = new JoystickScrollProvider(this); See my comment about lazy creation. https://codereview.chromium.org/901183002/diff/140001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:763: Nit: Unnecessary newline. https://codereview.chromium.org/901183002/diff/140001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1778: //Handle left stick event Nit: I think we can just leave out the comment. Awesome, now that this is the only place that uses mJoystickScrollProvider, we can lazily create it here before using, instead of upon creation (minor point, but this will only end up getting used rarely). https://codereview.chromium.org/901183002/diff/140001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/140001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:42: public void run() { Minor nit: Can we instead make this an internal runnable to hide this method? So then: private final Runnable mScrollRunnable = new Runnable() { @Override public void run() { animateScroll(); } }; ... private void animateScroll() { ... } That way we don't need to expose run() at all. https://codereview.chromium.org/901183002/diff/140001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:43: if (!mView.getContainerView().isFocused()) { Hmm, some WebView instances aren't focusable, so this might get tricky. Maybe let's instead do: if (!mView.getContainerView().isAttachedToWindow() || !mView.hasFocus()) { ... } https://codereview.chromium.org/901183002/diff/140001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:116: } else return 0f; Hmm, let's just stick the "return 0f" at the end on a new line, get rid of the final else. https://codereview.chromium.org/901183002/diff/140001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java (right): https://codereview.chromium.org/901183002/diff/140001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java:150: final float deltaAxisX, Hmm, I think these arguments should be packed? I think the command "git cl format" will automatically format Java now too for your patch. https://codereview.chromium.org/901183002/diff/140001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java:155: //Synthesize joystick motion event and send to ContentViewCore Nit: Space after "//" and end the comment with a period.
https://codereview.chromium.org/901183002/diff/140001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentView.java (right): https://codereview.chromium.org/901183002/diff/140001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentView.java:189: mContentViewCore.scrollTo(0, 0); Oops,huge mistake !!!!! Thanks for pointing out, otherwise it would have disturb existing logic. Done. https://codereview.chromium.org/901183002/diff/140001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/901183002/diff/140001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:504: private final JoystickScrollProvider mJoystickScrollProvider; On 2015/06/10 00:14:58, jdduke wrote: > Maybe insert a line break here and add a comment: > > "Provides smooth gamepad joystick-driven scrolling, lazily created." Done. https://codereview.chromium.org/901183002/diff/140001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:628: mJoystickScrollProvider = new JoystickScrollProvider(this); On 2015/06/10 00:14:58, jdduke wrote: > See my comment about lazy creation. Done. https://codereview.chromium.org/901183002/diff/140001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1778: //Handle left stick event On 2015/06/10 00:14:58, jdduke wrote: > Nit: I think we can just leave out the comment. > > Awesome, now that this is the only place that uses mJoystickScrollProvider, we > can lazily create it here before using, instead of upon creation (minor point, > but this will only end up getting used rarely). Done. https://codereview.chromium.org/901183002/diff/140001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/140001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:42: public void run() { On 2015/06/10 00:14:58, jdduke wrote: > Minor nit: Can we instead make this an internal runnable to hide this method? So > then: > > private final Runnable mScrollRunnable = new Runnable() { > @Override > public void run() { > animateScroll(); > } > }; > > ... > > private void animateScroll() { > ... > } > > That way we don't need to expose run() at all. Done. https://codereview.chromium.org/901183002/diff/140001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:43: if (!mView.getContainerView().isFocused()) { Checks like (!mView.getContainerView().isAttachedToWindow() || !mView.hasFocus()) stops scrolling when soft keyboard gets called when user touches URL bar, since view loses its focus (window containing view gains focus). When user types in text box, soft keyboard pops up, but this time view remains focused unlike above scenario. So above check will fail and view keeps on scrolling. To avoid this, we can keep mShowingSoftKeyboard flag which gets modified in ResultReceiver. We can allow view to be scrolled if mShowingSoftKeyboard == false https://codereview.chromium.org/901183002/diff/140001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:116: } else return 0f; On 2015/06/10 00:14:58, jdduke wrote: > Hmm, let's just stick the "return 0f" at the end on a new line, get rid of the > final else. Done. https://codereview.chromium.org/901183002/diff/140001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java (right): https://codereview.chromium.org/901183002/diff/140001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java:150: final float deltaAxisX, On 2015/06/10 00:14:58, jdduke wrote: > Hmm, I think these arguments should be packed? I think the command "git cl > format" will automatically format Java now too for your patch. Done. https://codereview.chromium.org/901183002/diff/140001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewScrollingTest.java:155: //Synthesize joystick motion event and send to ContentViewCore On 2015/06/10 00:14:58, jdduke wrote: > Nit: Space after "//" and end the comment with a period. Done.
https://codereview.chromium.org/901183002/diff/140001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/140001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:43: if (!mView.getContainerView().isFocused()) { On 2015/06/10 15:21:16, sshelke wrote: > Checks like (!mView.getContainerView().isAttachedToWindow() > || !mView.hasFocus()) stops scrolling when soft > keyboard gets called when user touches URL bar, since > view loses its focus (window containing view gains focus). > > When user types in text box, soft keyboard pops up, but this time > view remains focused unlike above scenario. So above check will fail > and view keeps on scrolling. > To avoid this, we can keep mShowingSoftKeyboard flag which gets > modified in ResultReceiver. We can allow view to be scrolled if > mShowingSoftKeyboard == false Why is it so important that we explicitly stop the scroll when a user is typing? How frequent do the two interactions happen together, e.g., I can scroll with my mousewheel and type at the same time, which might be bad but it doesn't happen very often. Or is it because the joystick has a different behavior when a textbox is focused? https://codereview.chromium.org/901183002/diff/160001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/901183002/diff/160001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1781: if (!mShowingSoftKeyboard) { Hmm, I'm not sure we can do this. What if we start scrolling, then the keyboard shows? Won't we continue scrolling indefinitely? https://codereview.chromium.org/901183002/diff/160001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/160001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:44: private void animateScroll() { Nit: Let's move this below the public method. https://codereview.chromium.org/901183002/diff/160001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:72: if (mScrollRunnable == null) { Let's just create this in the constructor, I think creation of the JoystickScrollProvider is a strong indicator that scrolling might happen, so we might as well avoid any null issues.
https://codereview.chromium.org/901183002/diff/140001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/140001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:43: if (!mView.getContainerView().isFocused()) { When textbox is selected or even URL bar is focused, default behavior of left stick is to navigate on soft keyboard. If we handle left joystick event in onGenericMotion handler then framework wont be able to provide LS support for navigation. https://codereview.chromium.org/901183002/diff/160001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/901183002/diff/160001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1781: if (!mShowingSoftKeyboard) { That would be very rare, user scrolling a view and selecting text box on same view. If user taps of URL bar when ScrollProvider is scrolling a view then in that case, the view is going to lose its focus and ScrollProvider will stop scrolling. If we still want to avoid issue, either we can expose mShowingKeyboard flag to JoyStickScrollProvider or as you suggested we can create setEnabled(boolean) API in ScrollProvider and call it from ResultReceiver to disable scrolling. Let me know what would be best way to handle the issue. "if (!mShowingSoftKeyboard)" condition is important since we want framework to handle the LS event to navigate on keyboard when text box is selected. https://codereview.chromium.org/901183002/diff/160001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/160001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:44: private void animateScroll() { Consider it done. https://codereview.chromium.org/901183002/diff/160001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:72: if (mScrollRunnable == null) { I will modify the code.
https://codereview.chromium.org/901183002/diff/160001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/901183002/diff/160001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1781: if (!mShowingSoftKeyboard) { On 2015/06/12 07:07:52, sshelke wrote: > That would be very rare, user scrolling a view and > selecting text box on same view. > If user taps of URL bar when ScrollProvider is scrolling > a view then in that case, the view is going to lose its > focus and ScrollProvider will stop scrolling. > > > If we still want to avoid issue, either we can expose > mShowingKeyboard flag to JoyStickScrollProvider or > as you suggested we can create setEnabled(boolean) API > in ScrollProvider and call it from ResultReceiver > to disable scrolling. > > Let me know what would be best way to handle the issue. > > "if (!mShowingSoftKeyboard)" condition is important since > we want framework to handle the LS event to navigate on > keyboard when text box is selected. I see, hmm, what if there's a hardware keyboard? Do we still want the LS to navigate the text box? If so, we might need to use the "mFocusedNodeEditable" flag, would that work? Either way, I'd be OK adding a "setEnabled" flag to the provider.
Uploaded patch and tried to resolve all cases related to soft keyboard. Please review.
Looks good, just a couple nits then I'll lg2m. https://codereview.chromium.org/901183002/diff/180001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/901183002/diff/180001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:762: boolean mShowingSoftKeyboard = Nit: showingSoftKeyboard. https://codereview.chromium.org/901183002/diff/180001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/180001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:37: private boolean mScrollEnabled; Nit: mEnabled is enough here, maybe we should rename "mAutoScrollEnabled" to "mAutoScrollAcive" to distinguish between active/enabled? https://codereview.chromium.org/901183002/diff/180001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:41: public JoystickScrollProvider(ContentViewCore contentView) { Nit: Let's add javadocs for all public methods. https://codereview.chromium.org/901183002/diff/180001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:76: computeNewScrollVelocity(event); We should probably assert on |event|'s input type, or just early return if it's not a joystick input. https://codereview.chromium.org/901183002/diff/180001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:77: if (mScrollVelocityX != 0 || mScrollVelocityY != 0) { Should this be: if (mEnabled && mScrollVelocityX....)?
Thanks Jared, I have modified patch fixing previous comments. https://codereview.chromium.org/901183002/diff/180001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/901183002/diff/180001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:762: boolean mShowingSoftKeyboard = On 2015/06/25 16:40:22, jdduke wrote: > Nit: showingSoftKeyboard. Done. https://codereview.chromium.org/901183002/diff/180001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/180001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:37: private boolean mScrollEnabled; On 2015/06/25 16:40:23, jdduke wrote: > Nit: mEnabled is enough here, maybe we should rename "mAutoScrollEnabled" to > "mAutoScrollAcive" to distinguish between active/enabled? Done. https://codereview.chromium.org/901183002/diff/180001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:41: public JoystickScrollProvider(ContentViewCore contentView) { On 2015/06/25 16:40:22, jdduke wrote: > Nit: Let's add javadocs for all public methods. Done. https://codereview.chromium.org/901183002/diff/180001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:76: computeNewScrollVelocity(event); On 2015/06/25 16:40:22, jdduke wrote: > We should probably assert on |event|'s input type, or just early return if it's > not a joystick input. Done. https://codereview.chromium.org/901183002/diff/180001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:77: if (mScrollVelocityX != 0 || mScrollVelocityY != 0) { On 2015/06/25 16:40:23, jdduke wrote: > Should this be: if (mEnabled && mScrollVelocityX....)? Done.
Thanks! aurimas@: Anything else you'd like to see here? Also, please run "git cl format" and it should cleanup any Java style issues. https://codereview.chromium.org/901183002/diff/200001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/901183002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1776: if (!mFocusedNodeEditable) { Question: Would it be silly to synchronize the mJoystickScrollProvider with mFocusedNodeEditable? That should handle the soft keyboard case, right? So then we'd move the |setEnabled| call to where we set mFocusedNodeEditable. https://codereview.chromium.org/901183002/diff/200001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:52: private void animateScroll() { Can you move the |animateScroll()| method below |onMotion()| so that the public methods are grouped together? https://codereview.chromium.org/901183002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:65: mAutoScrollActive = true; This should probably be |assert mAutoScrollActive;|. (see comment below). https://codereview.chromium.org/901183002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:90: computeNewScrollVelocity(event); I'd sleep better if we had: if ((event.getSource() & InputDevice.SOURCE_CLASS_JOYSTICK) == 0) return false; https://codereview.chromium.org/901183002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:104: mView.getContainerView().postOnAnimation(mScrollRunnable); Please add |mAutoScrollActive = true;| here, otherwise we might post the runnable multiple times. https://codereview.chromium.org/901183002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:117: /** Nit: Looks like * has an extra indent in this comment and the one for getFilteredAxisValue.
It looks reasonable to me LTGM after jdduke is ok with the patch. https://codereview.chromium.org/901183002/diff/200001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/901183002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1776: if (!mFocusedNodeEditable) { Can we combine these two if statements to be if (!mFocusedNodeEditable && mJoystickScrollProvider.onMotion(event))? https://codereview.chromium.org/901183002/diff/200001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:15: * This class implements auto scrolling and panning Wrapping looks odd, we wrap at 100 chars for java. https://codereview.chromium.org/901183002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:129: + "switching to fallback scroll factor "); We wrap by indenting 8 spaces on the new line. Please fix this.
https://codereview.chromium.org/901183002/diff/200001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/901183002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1776: if (!mFocusedNodeEditable) { I guess, we should stick to our old implementation which used mShowingSoftKeyboard. We can synchronize this flag with JoystickScrollProvider and can be made public. I am not sure complexity involved with flag mFocusedNodeEditable. Will it okay? https://codereview.chromium.org/901183002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1776: if (!mFocusedNodeEditable) { Yes we combine statements, why did't I think in first place. https://codereview.chromium.org/901183002/diff/200001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:65: mAutoScrollActive = true; Agreed. Its possible that two consecutive events can start two different runnables, if first runnable is not started before processing of second event. https://codereview.chromium.org/901183002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:90: computeNewScrollVelocity(event); We already have same check for Joystick event in OnGenericMotionHandler of ContentViewCore. OnMotion wont be called if event does not belong to Joystick.
On 2015/06/26 18:20:32, sshelke wrote: > https://codereview.chromium.org/901183002/diff/200001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/901183002/diff/200001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1776: > if (!mFocusedNodeEditable) { > I guess, we should stick to our old implementation which used > mShowingSoftKeyboard. We can synchronize this flag with > JoystickScrollProvider and can be made public. > I am not sure complexity involved with flag mFocusedNodeEditable. So, mFocusedNodeEditable is a superset of mShowingSoftKeyboard. It will be true whenever an editable region has been focused. What you have now is probably fine. https://codereview.chromium.org/901183002/diff/200001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:90: > computeNewScrollVelocity(event); > We already have same check for Joystick event in OnGenericMotionHandler > of ContentViewCore. OnMotion wont be called if event does not belong > to Joystick. Right, but I'd prefer we not make that assumption, e.g., I'd like for the class to be slightly more robust to changes in the owning class.
Fixed previous comments. https://codereview.chromium.org/901183002/diff/200001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/901183002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1776: if (!mFocusedNodeEditable) { On 2015/06/26 15:28:55, jdduke wrote: > Question: Would it be silly to synchronize the mJoystickScrollProvider with > mFocusedNodeEditable? That should handle the soft keyboard case, right? So then > we'd move the |setEnabled| call to where we set mFocusedNodeEditable. synchronized with mFocusedNodeEditable. Done. https://codereview.chromium.org/901183002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1776: if (!mFocusedNodeEditable) { On 2015/06/26 16:36:16, aurimas wrote: > Can we combine these two if statements to be if (!mFocusedNodeEditable && > mJoystickScrollProvider.onMotion(event))? Done. https://codereview.chromium.org/901183002/diff/200001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:15: * This class implements auto scrolling and panning On 2015/06/26 16:36:16, aurimas wrote: > Wrapping looks odd, we wrap at 100 chars for java. Done. https://codereview.chromium.org/901183002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:52: private void animateScroll() { On 2015/06/26 15:28:55, jdduke wrote: > Can you move the |animateScroll()| method below |onMotion()| so that the public > methods are grouped together? Done. https://codereview.chromium.org/901183002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:65: mAutoScrollActive = true; On 2015/06/26 15:28:55, jdduke wrote: > This should probably be |assert mAutoScrollActive;|. (see comment below). Done. https://codereview.chromium.org/901183002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:90: computeNewScrollVelocity(event); On 2015/06/26 15:28:55, jdduke wrote: > I'd sleep better if we had: > > if ((event.getSource() & InputDevice.SOURCE_CLASS_JOYSTICK) == 0) return false; Actually, computeScrollVelocity returns non-zero values for LS events only, since we are reading MotionEvent.AXIS_X value, so value return for events like RS, DPAD will be zero. We have handled that condition using check (mScrollVelocityX == 0 && mScrollVelocityY == 0). But there is no harm in keeping double check. Done. https://codereview.chromium.org/901183002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:104: mView.getContainerView().postOnAnimation(mScrollRunnable); On 2015/06/26 15:28:55, jdduke wrote: > Please add |mAutoScrollActive = true;| here, otherwise we might post the > runnable multiple times. Done. https://codereview.chromium.org/901183002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:117: /** On 2015/06/26 15:28:55, jdduke wrote: > Nit: Looks like * has an extra indent in this comment and the one for > getFilteredAxisValue. Done. https://codereview.chromium.org/901183002/diff/200001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:129: + "switching to fallback scroll factor "); On 2015/06/26 16:36:16, aurimas wrote: > We wrap by indenting 8 spaces on the new line. Please fix this. git cl format forces intend more than 8 for this line. should I change manually to 8 without running git cl format?
lgtm with a couple nits. https://codereview.chromium.org/901183002/diff/220001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/901183002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1773: && !mFocusedNodeEditable && mJoystickScrollProvider.onMotion(event)) { Hmm, I think it's a bit more clear if we split the conditionals here. So keep the "(event.getSource() & InputDevice.SOURCE_CLASS_JOYSTICK) != 0" as the toplevel condition, then have if (!mFocusedNodeEditable && mJoystickScrollProvider.onMotion(event)) return true; as the nested conditional. To be honest I'm not sure we even need the "mFocusedNodeEditable" check as we explicitly disable the scroller when that value changes. https://codereview.chromium.org/901183002/diff/220001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:58: mEnabled = enabled; Why not add a: if (!enabled) stop(); That ensures we only have an in-flight callback if we're enabled. https://codereview.chromium.org/901183002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:73: if (((event.getSource() & InputDevice.SOURCE_CLASS_JOYSTICK) == 0) || !mEnabled I don't understand why these conditions come after "computeNewScrollVelocity". I'd prefer they come first: if (!mEnabled) return false; if ((event.getSource() & InputDevice.SOURCE_CLASS_JOYSTICK) == 0) return false; computeNewScrollVelocity(event); ...
Thanks Jared, Fixed previous comments. https://codereview.chromium.org/901183002/diff/220001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/901183002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1773: && !mFocusedNodeEditable && mJoystickScrollProvider.onMotion(event)) { On 2015/06/30 15:47:56, jdduke wrote: > Hmm, I think it's a bit more clear if we split the conditionals here. So keep > the "(event.getSource() & InputDevice.SOURCE_CLASS_JOYSTICK) != 0" as the > toplevel condition, then have > > if (!mFocusedNodeEditable && mJoystickScrollProvider.onMotion(event)) return > true; > > as the nested conditional. > > To be honest I'm not sure we even need the "mFocusedNodeEditable" check as we > explicitly disable the scroller when that value changes. Done. https://codereview.chromium.org/901183002/diff/220001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:58: mEnabled = enabled; On 2015/06/30 15:47:56, jdduke wrote: > Why not add a: > > if (!enabled) stop(); > > That ensures we only have an in-flight callback if we're enabled. Done. https://codereview.chromium.org/901183002/diff/220001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:73: if (((event.getSource() & InputDevice.SOURCE_CLASS_JOYSTICK) == 0) || !mEnabled On 2015/06/30 15:47:56, jdduke wrote: > I don't understand why these conditions come after "computeNewScrollVelocity". > I'd prefer they come first: > > if (!mEnabled) return false; > if ((event.getSource() & InputDevice.SOURCE_CLASS_JOYSTICK) == 0) return false; > > computeNewScrollVelocity(event); > > ... Done.
The CQ bit was checked by sshelke@nvidia.com
The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org Link to the patchset: https://codereview.chromium.org/901183002/#ps240001 (title: "fixed comments + rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/901183002/240001
The author sshelke@nvidia.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The author sshelke@nvidia.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
On 2015/07/01 13:02:20, commit-bot: I haz the power wrote: > The author mailto:sshelke@nvidia.com has not signed Google Contributor License > Agreement. Please visit https://cla.developers.google.com to sign and manage > CLA. Hmm, I thought @nvidia.com was OK but maybe not. Can you follow the instructions there? https://codereview.chromium.org/901183002/diff/240001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:73: if (!mEnabled || (event.getSource() & InputDevice.SOURCE_CLASS_JOYSTICK) == 0) { These branch has different semantics than what I stated. If we're disabled, we should already have been stopped. If we get a *different* MotionEvent type, we shouldn't assume that that should cancel motion. It should be sufficient to just early return false.
Thanks Jared, It seems there are some issues regarding CLA with nvidia users. We are sorting it out. Once the issue is resolved I will commit the change. Also uploading new change which fixes previous comment. https://codereview.chromium.org/901183002/diff/240001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/901183002/diff/240001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:73: if (!mEnabled || (event.getSource() & InputDevice.SOURCE_CLASS_JOYSTICK) == 0) { Hmm, I will revert to semantics, you have mentioned in previous comment.
On 2015/07/02 06:29:44, sshelke wrote: > Thanks Jared, > It seems there are some issues regarding CLA with nvidia users. We are sorting > it out. Once the issue is resolved I will commit the change. > > Also uploading new change which fixes previous comment. > > https://codereview.chromium.org/901183002/diff/240001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java > (right): > > https://codereview.chromium.org/901183002/diff/240001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:73: > if (!mEnabled || (event.getSource() & InputDevice.SOURCE_CLASS_JOYSTICK) == 0) { > Hmm, I will revert to semantics, you have mentioned > in previous comment. Patch re-based.
The CQ bit was checked by sshelke@nvidia.com
The patchset sent to the CQ was uploaded after l-g-t-m from jdduke@chromium.org Link to the patchset: https://codereview.chromium.org/901183002/#ps280001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/901183002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/901183002/280001
The author sshelke@nvidia.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The author sshelke@nvidia.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The author sshelke@nvidia.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The author sshelke@nvidia.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
On 2015/07/27 14:16:40, commit-bot: I haz the power wrote: > The author mailto:sshelke@nvidia.com has not signed Google Contributor License > Agreement. Please visit https://cla.developers.google.com to sign and manage > CLA. Until the CLA issue is resolved you won't be able to commit this.
The author sshelke@nvidia.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
Message was sent while issue was closed.
Committed patchset #14 (id:280001)
Message was sent while issue was closed.
Patchset 14 (id:??) landed as https://crrev.com/c631fde80e1e8166dc933a497626f04c0f5d1ed1 Cr-Commit-Position: refs/heads/master@{#340459}
Message was sent while issue was closed.
Patchset #15 (id:300001) has been deleted |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
