Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1288243004:
Enable zoom through gamedpad trigger joystick (Closed)
DescriptionEnable zoom through gamedpad trigger joystick
This change allows page zoomin/out using trigger joysticks.
Page zoomin and zoomout can be achieved using left and right
trigger joysticks respectively.
BUG=454355
Committed: https://crrev.com/84583b7d70d5bf5adac05346811b5422033c4e86
Cr-Commit-Position: refs/heads/master@{#380572}
Committed: https://crrev.com/386000b5edda92212e1a0835c58acb4a59fe8378
Cr-Commit-Position: refs/heads/master@{#381188}
Patch Set 1 #
Total comments: 13
Patch Set 2 : fixed review comments #Patch Set 3 : swap axes functionalities: RTRIGGER: zoomin LTRIGGER:zoomout #
Total comments: 17
Patch Set 4 : fixed formula previous comments #
Total comments: 2
Patch Set 5 : fixed formula (removed page scale factor) add test #
Total comments: 32
Patch Set 6 : fixed previous comments #
Total comments: 7
Patch Set 7 : fixed previous comments #Patch Set 8 : #
Total comments: 4
Patch Set 9 : #
Total comments: 1
Patch Set 10 : rebase #
Total comments: 13
Patch Set 11 : #Patch Set 12 : fixed static class error #
Messages
Total messages: 92 (13 generated)
sshelke@nvidia.com changed reviewers: + jdduke@chromium.org
JoystickZoomProvider design is quite same as JoystickScrollProvider
but has different implementation for animation.
Though, I still think, to avoid method repetitions, we can use delegation
pattern and create interface class JoystickControlDelegate and
delegates its tasks to JoystickScroll/ZoomProvider classes.
For example,
interface JoystickControlDelegate {
bool onMotion(MotionEvent event);
float getFilteredAxisValue(MotionEvent event);
void computeNewValue(MotionEvent event);
void animate();
}
class JoystickZoomInProvider implements JoystickControlDelegate {
void computeNewValue(MotionEvent event) {
mZoomInVelocity =
getFilteredAxisValue(event, MotionEvent.AXIS_LTRIGGER)
* deviceScaleFactor;
}
void animate() {
animatezoomin();
}
}
class JoystickZoomOutProvider implements JoystickControlDelegate {
void computeNewValue(MotionEvent event) {
mZoomOutVelocity =
getFilteredAxisValue(event, MotionEvent.AXIS_RTRIGGER)
* deviceScaleFactor;
}
void animate() {
animatezoomOut();
}
}
class JoystickScrollProvider implements JoystickControlDelegate {
void computeNewValue(MotionEvent event) {
mScrollVelocity =
getFilteredAxisValue(event, MotionEvent.AXIS_X/Y)
* deviceScaleFactor;
}
void animate() {
animateScroll();
}
}
On 2015/08/20 12:50:29, sshelke wrote:
> JoystickZoomProvider design is quite same as JoystickScrollProvider
> but has different implementation for animation.
>
> Though, I still think, to avoid method repetitions, we can use delegation
> pattern and create interface class JoystickControlDelegate and
> delegates its tasks to JoystickScroll/ZoomProvider classes.
>
> For example,
> interface JoystickControlDelegate {
>
> bool onMotion(MotionEvent event);
> float getFilteredAxisValue(MotionEvent event);
> void computeNewValue(MotionEvent event);
> void animate();
> }
>
> class JoystickZoomInProvider implements JoystickControlDelegate {
> void computeNewValue(MotionEvent event) {
> mZoomInVelocity =
> getFilteredAxisValue(event, MotionEvent.AXIS_LTRIGGER)
> * deviceScaleFactor;
> }
> void animate() {
> animatezoomin();
> }
> }
>
> class JoystickZoomOutProvider implements JoystickControlDelegate {
> void computeNewValue(MotionEvent event) {
> mZoomOutVelocity =
> getFilteredAxisValue(event, MotionEvent.AXIS_RTRIGGER)
> * deviceScaleFactor;
> }
> void animate() {
> animatezoomOut();
> }
> }
>
> class JoystickScrollProvider implements JoystickControlDelegate {
> void computeNewValue(MotionEvent event) {
> mScrollVelocity =
> getFilteredAxisValue(event, MotionEvent.AXIS_X/Y)
> * deviceScaleFactor;
> }
> void animate() {
> animateScroll();
> }
> }
If we want this to be extensible by web content (e.g., with maps), for pan and
zoom, we'll probably need to simulate touchpad and/or wheel events.
I will look into possibilities of simulating pan and zoom through touchpad. Meanwhile, you can go through patch and let me know if any further refinement is required.
On 2015/08/21 14:04:46, sshelke wrote: > I will look into possibilities of simulating pan and zoom through touchpad. > Meanwhile, you can go through patch and let me know if any further refinement > is required. So, I think what all we need to do to support wheel events is send the pinch events but set the source device to touchpad instead of touchscreen. See https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/an..., we should be able to tweak the sourceDevice there, providing a new argument from ContentViewCore. The compositor will then check if there is a wheel handler, if not, it will zoom, if so, it will send it to Blink and there it will either be consumed or cause zooming. I'm somewhat inclined to say we should just zoom at the center of the screen, and not trying to use the last focus position, what do you think? I'll look into how we can make scrolling work, we might add an additional argument to ContentViewCore scrollBy that says what device to use. Then we'll create wheel events if the source device is set to touchpad (in ContentViewCoreImpl). Ideally we could just set the source device to touchpad, but I don't think our input pipeline will do the right thing there.
>> I'm somewhat inclined to say we should just zoom at the center of the screen, >> and not trying to use the last focus position, what do you think? Even i was skeptical about this, so i asked random developers to try zoom using trigger joysticks. Funny thing, they could notice zooming happening at the center. They asked me "what if i want to zoom specific area?", that was the reason I moved to zooming particular area, so that it wont look like a bug. But, I am fine zooming central area, if that is expected operation.
On 2015/08/24 11:00:42, sshelke wrote: > >> I'm somewhat inclined to say we should just zoom at the center of the screen, > >> and not trying to use the last focus position, what do you think? > > Even i was skeptical about this, so i asked random developers to try zoom using > trigger joysticks. Funny thing, they could notice zooming happening at the > center. > They asked me "what if i want to zoom specific area?", that was the reason I > moved > to zooming particular area, so that it wont look like a bug. > But, I am fine zooming central area, if that is expected operation. But if you only have a gamepad, it's pretty difficult to position the focus over a particular element, right? I don't mind what we have now, I just want to make sure we don't get into a state where we default to zooming at the origin (0, 0). That's wrong in most cases.
>> But if you only have a gamepad, it's pretty difficult to position the focus over >> a particular element, right? Sometimes gamepad joystick is mapped to mouse at android framework, for example in shield, RS is mapped to mouse so basically user is able to navigate and select element using mouse. Also, in opera browser for ATV, special logic is written to simulate DPAD as mouse. >> I just want to make sure we don't get into a state >> where we default to zooming at the origin (0, 0). >> That's wrong in most cases. We can put condition explicitly, not to use last focal position in case touch/mouse events are generated from origin(0,0).
On 2015/08/27 12:59:30, sshelke wrote: > >> But if you only have a gamepad, it's pretty difficult to position the focus > over > >> a particular element, right? > > Sometimes gamepad joystick is mapped to mouse at android framework, for > example in shield, RS is mapped to mouse so basically user is able to > navigate and select element using mouse. > > Also, in opera browser for ATV, special logic is written to simulate DPAD > as mouse. > > >> I just want to make sure we don't get into a state > >> where we default to zooming at the origin (0, 0). > >> That's wrong in most cases. > We can put condition explicitly, not to use last focal position > in case touch/mouse events are generated from origin(0,0). Sounds good, let me know how the touchpad sourceDevice trial goes and we can go from there.
Hi Jared, Sorry for not replying to message early, I was involved in other activities. I have hard-coded event.sourceDevice to WebGestureDeviceTouchpad in pinch begin/end/by API's. But still not able to zoom page through Trigger joystick event. Also observed that mRenderCoordinates.getMaxPageScaleFactor is always equal to mRenderCoordinates.getPageScaleFactor for google map page. So theoretically, from content view perspective page is already zoomed to maximum level. I guess, Pinch gesture events are reaching to compositor but somehow it is not able to zoom the page.
jdduke@chromium.org changed reviewers: + aelias@chromium.org, tdresser@chromium.org
+aelias, tdresser for further guidance here.
I found the reason due to which google map page was not able to zoom through Pinch gestures. Please refer the link https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre.... In this function zoom factor gets clamped to 1 since MaxPageScale == 1 for google map page. If return early from ClampPageScaleFactorToLimits without checking limit conditions, page zooms as per expectations. Even if google page zooms with TRIGGER joystick events, behavior is not similar to page zooming thorough touch or mouse events. After zooming through joystick, new contents do not load, only page scale changes.
I think zooming through joystick should be implemented in two patches. Patch 1 : PageMaxScale!=1 Implementation similar to this patch. Zooming can be easily achieved through pinch gestures using pinchByDelta api. Patch 2: PageMaxScale == 1 (Pages like google maps) Convert Trigger Joystick events to touch or mouse events and pass it to compositor. Setting event.sourceDevice to WebGestureDeviceTouchpad does not solve purpose since there is no handler for WebGestureDeviceTouchpad. We can translate joystick events to touch events or mouse events and send to blink and then blink will zoom the page.
Sorry for the delay. I still haven't actually tested this patch yet. Google maps shouldn't be zoomed by the compositor, the web page itself should end up receiving a touchpad scroll with control held down, and then it runs some JS to perform the zoom. We convert from touchpad pinch to touchpad scroll with control held down here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... Are you able to hit the code path? https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2753: public boolean pinchByDelta(float delta, boolean useLastFocalEventLocation) { Would it be clearer to pass in the focal point? https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java (right): https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:15: * Page Zoomin is propertional to LTRIGGER axis movement. This seems backwards to me. In almost all UIs I've ever seen, zoom in is to the right of zoom out. https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:21: private static final float JOYSTICK_NOISE_THRESHOLD = 0.2f; Add a comment describing why we require a noise threshold. https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:92: private void animateZoom() { I still need to try this out, but at this point it isn't obvious to me why we need an animation. Why can't we just update every time we receive an event from the gamepad? Is the event frequency from the gamepad not high enough?
https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2753: public boolean pinchByDelta(float delta, boolean useLastFocalEventLocation) { On 2015/11/24 18:12:43, tdresser wrote: > Would it be clearer to pass in the focal point? Could you please elaborate? Currently, logic is to zoom area containing last touch event location. Do you wish to pass some other focal points? https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java (right): https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:15: * Page Zoomin is propertional to LTRIGGER axis movement. On 2015/11/24 18:12:43, tdresser wrote: > This seems backwards to me. In almost all UIs I've ever seen, zoom in is to the > right of zoom out. Okay, I will change logic to zoomin through RTRIGGER and zoomout through LTRIGGER. https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:21: private static final float JOYSTICK_NOISE_THRESHOLD = 0.2f; On 2015/11/24 18:12:43, tdresser wrote: > Add a comment describing why we require a noise threshold. Will add and upload new patch. https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:92: private void animateZoom() { On 2015/11/24 18:12:43, tdresser wrote: > I still need to try this out, but at this point it isn't obvious to me why we > need an animation. Why can't we just update every time we receive an event from > the gamepad? Is the event frequency from the gamepad not high enough? There are two main reasons, they are as follows. 1) Page should be zoomed even when user has moved joystick in y direction and kept it fixed.For example, if user move along y-axis to 0.5 and stopped , page should keep zooming since there is absolute movement in y direction. Currently in android, If joystick is not moving along any direction, even though there was displacement before, event handler receives value equal to 0. 2) When joystick is touched, series of events are generated, all are queued and passed to motion handler.We don't want to request compositor to paint each time. Choreographer in android tries to maintain 60 fps frame rate. We should not request compositor to maintain frame rate more than that. For example, if there are 4 events arrived in animation interval. t=4, t=10, t=12, t=16 We should pick value which is nearer to animation interval i.e t=16 and ask compositor to zoom using same value. It would be overhead for compositor to zoom page at t=4,10,12,16
https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2753: public boolean pinchByDelta(float delta, boolean useLastFocalEventLocation) { On 2015/11/27 13:14:46, sshelke wrote: > On 2015/11/24 18:12:43, tdresser wrote: > > Would it be clearer to pass in the focal point? > Could you please elaborate? Currently, logic is to zoom area containing last > touch event location. Do you wish to pass some other focal points? Sorry, I just mean that it might be clearer if this function took the focal point to use, instead of a bool indicating which focal point to use. Then the callsite would provide the focal point, instead of this method. Another option would be to have two methods, one which only takes the delta, and one which takes the delta and the focal point. I'm just not a big fan of using a bool here, as from the callsites it's not clear what the bool is used for. https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java (right): https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:92: private void animateZoom() { On 2015/11/27 13:14:46, sshelke wrote: > On 2015/11/24 18:12:43, tdresser wrote: > > I still need to try this out, but at this point it isn't obvious to me why we > > need an animation. Why can't we just update every time we receive an event > from > > the gamepad? Is the event frequency from the gamepad not high enough? > > There are two main reasons, they are as follows. > 1) Page should be zoomed even when user has moved joystick in y > direction and kept it fixed.For example, if user move along > y-axis to 0.5 and stopped , page should keep zooming since > there is absolute movement in y direction. > Currently in android, If joystick is not moving along any direction, > even though there was displacement before, event handler > receives value equal to 0. > 2) When joystick is touched, series of events are generated, all are queued > and passed to motion handler.We don't want to request compositor > to paint each time. Choreographer in android tries to maintain 60 fps > frame rate. We should not request compositor to maintain frame rate > more than that. > For example, if there are 4 events arrived in animation interval. > t=4, t=10, t=12, t=16 > We should pick value which is nearer to animation interval i.e t=16 and > ask compositor to zoom using same value. It would be overhead for > compositor to zoom page at t=4,10,12,16 > Of course, thanks for clarifying.
>>We convert from touchpad pinch to touchpad scroll with control held down here: >>https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... >> Are you able to hit the code path? Not able to hit the code path due to failure in following condition. https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/blink/in... I guess, there is no wheel handler associated with layer for specified gesture event location.
Fixed previous comments. please review. https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2753: public boolean pinchByDelta(float delta, boolean useLastFocalEventLocation) { On 2015/11/27 14:58:21, tdresser wrote: > On 2015/11/27 13:14:46, sshelke wrote: > > On 2015/11/24 18:12:43, tdresser wrote: > > > Would it be clearer to pass in the focal point? > > Could you please elaborate? Currently, logic is to zoom area containing last > > touch event location. Do you wish to pass some other focal points? > > Sorry, I just mean that it might be clearer if this function took the focal > point to use, instead of a bool indicating which focal point to use. Then the > callsite would provide the focal point, instead of this method. > > Another option would be to have two methods, one which only takes the delta, and > one which takes the delta and the focal point. > > I'm just not a big fan of using a bool here, as from the callsites it's not > clear what the bool is used for. Done. https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java (right): https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:15: * Page Zoomin is propertional to LTRIGGER axis movement. On 2015/11/27 13:14:46, sshelke wrote: > On 2015/11/24 18:12:43, tdresser wrote: > > This seems backwards to me. In almost all UIs I've ever seen, zoom in is to > the > > right of zoom out. > > Okay, I will change logic to zoomin through RTRIGGER and zoomout through > LTRIGGER. Done. https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:21: private static final float JOYSTICK_NOISE_THRESHOLD = 0.2f; On 2015/11/24 18:12:43, tdresser wrote: > Add a comment describing why we require a noise threshold. Done.
On 2015/11/30 13:35:52, sshelke wrote: > Fixed previous comments. please review. > > https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2753: > public boolean pinchByDelta(float delta, boolean useLastFocalEventLocation) { > On 2015/11/27 14:58:21, tdresser wrote: > > On 2015/11/27 13:14:46, sshelke wrote: > > > On 2015/11/24 18:12:43, tdresser wrote: > > > > Would it be clearer to pass in the focal point? > > > Could you please elaborate? Currently, logic is to zoom area containing > last > > > touch event location. Do you wish to pass some other focal points? > > > > Sorry, I just mean that it might be clearer if this function took the focal > > point to use, instead of a bool indicating which focal point to use. Then the > > callsite would provide the focal point, instead of this method. > > > > Another option would be to have two methods, one which only takes the delta, > and > > one which takes the delta and the focal point. > > > > I'm just not a big fan of using a bool here, as from the callsites it's not > > clear what the bool is used for. > > Done. > > https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... > File > content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java > (right): > > https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:15: > * Page Zoomin is propertional to LTRIGGER axis movement. > On 2015/11/27 13:14:46, sshelke wrote: > > On 2015/11/24 18:12:43, tdresser wrote: > > > This seems backwards to me. In almost all UIs I've ever seen, zoom in is to > > the > > > right of zoom out. > > > > Okay, I will change logic to zoomin through RTRIGGER and zoomout through > > LTRIGGER. > > Done. > > https://codereview.chromium.org/1288243004/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:21: > private static final float JOYSTICK_NOISE_THRESHOLD = 0.2f; > On 2015/11/24 18:12:43, tdresser wrote: > > Add a comment describing why we require a noise threshold. > > Done. You're testing on Google maps, and finding there's no wheel event handler? That would be quite strange, as there is a wheel event handler on the map in Google maps.
> You're testing on Google maps, and finding there's no wheel event handler? > > That would be quite strange, as there is a wheel event handler on the map in > Google maps. Yes, code does not hit WebViewImpl::handleSyntheticWheelFromTouchpadPinchEvent in case of google maps because of failure in https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/blink/in.... Though I have observed that code path does hit above API for google page. I guess, compositor is able to find wheel handler in specific layer for google page.
On 2015/12/01 12:10:55, sshelke wrote: > > You're testing on Google maps, and finding there's no wheel event handler? > > > > That would be quite strange, as there is a wheel event handler on the map in > > Google maps. > Yes, code does not hit WebViewImpl::handleSyntheticWheelFromTouchpadPinchEvent > in case of google maps because of failure in > https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/blink/in.... > > Though I have observed that code path does hit above API for google page. > I guess, compositor is able to find wheel handler in specific layer for google > page. Sorry for the delay here. I just tested this on a mac with Google maps, input_handler_->HaveWheelEventHandlersAt( gfx::Point(gesture_event.x, gesture_event.y)) returns true in my testing. I'm not sure why you're seeing this fail. Ideally the joystick behavior would be identical to touchpad pinch zoom, which works on Google maps via a wheel event handler. Do you have a Mac available? Does touchpad pinch zoom work for you on Google maps?
On 2015/12/02 20:08:16, tdresser wrote: > On 2015/12/01 12:10:55, sshelke wrote: > > > You're testing on Google maps, and finding there's no wheel event handler? > > > > > > That would be quite strange, as there is a wheel event handler on the map in > > > Google maps. > > Yes, code does not hit WebViewImpl::handleSyntheticWheelFromTouchpadPinchEvent > > in case of google maps because of failure in > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/blink/in.... > > > > Though I have observed that code path does hit above API for google page. > > I guess, compositor is able to find wheel handler in specific layer for google > > page. > > Sorry for the delay here. > > I just tested this on a mac with Google maps, > input_handler_->HaveWheelEventHandlersAt( > gfx::Point(gesture_event.x, gesture_event.y)) > returns true in my testing. > Did you test after adding event.sourceDevice = WebGestureDeviceTouchpad here? https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/an... I mean, how did you translate joystick event to wheel event? > Do you have a Mac available? Does touchpad pinch zoom work for you on Google > maps? I don't have Mac setup, so cant verify/find root cause for failure. On android, google map does not zoom even without my change. I am not sure why condition is getting failed for android platforms.
On 2015/12/04 14:05:02, sshelke wrote: > On 2015/12/02 20:08:16, tdresser wrote: > > On 2015/12/01 12:10:55, sshelke wrote: > > > > You're testing on Google maps, and finding there's no wheel event handler? > > > > > > > > That would be quite strange, as there is a wheel event handler on the map > in > > > > Google maps. > > > Yes, code does not hit > WebViewImpl::handleSyntheticWheelFromTouchpadPinchEvent > > > in case of google maps because of failure in > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/blink/in.... > > > > > > Though I have observed that code path does hit above API for google page. > > > I guess, compositor is able to find wheel handler in specific layer for > google > > > page. > > > > Sorry for the delay here. > > > > I just tested this on a mac with Google maps, > > input_handler_->HaveWheelEventHandlersAt( > > gfx::Point(gesture_event.x, gesture_event.y)) > > returns true in my testing. > > > Did you test after adding event.sourceDevice = WebGestureDeviceTouchpad here? > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/an... > I mean, how did you translate joystick event to wheel event? > > > Do you have a Mac available? Does touchpad pinch zoom work for you on Google > > maps? > > I don't have Mac setup, so cant verify/find root cause for failure. > On android, google map does not zoom even without my change. > I am not sure why condition is getting failed for android platforms. I was just testing touchpad pinch zoom on mac. Sorry, I forgot that you'd be getting the mobile version of Google maps, which doesn't listen to wheel events, so Google maps is a bad test case. You could test desktop maps by setting the user agent via the --user-agent command line flag. I think that behaving the same as touchpad zoom is correct, even if Google maps doesn't currently support it. Does that sound reasonable?
On 2015/12/04 14:25:41, tdresser wrote: > On 2015/12/04 14:05:02, sshelke wrote: > > On 2015/12/02 20:08:16, tdresser wrote: > > > On 2015/12/01 12:10:55, sshelke wrote: > > > > > You're testing on Google maps, and finding there's no wheel event > handler? > > > > > > > > > > That would be quite strange, as there is a wheel event handler on the > map > > in > > > > > Google maps. > > > > Yes, code does not hit > > WebViewImpl::handleSyntheticWheelFromTouchpadPinchEvent > > > > in case of google maps because of failure in > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ui/events/blink/in.... > > > > > > > > Though I have observed that code path does hit above API for google page. > > > > I guess, compositor is able to find wheel handler in specific layer for > > google > > > > page. > > > > > > Sorry for the delay here. > > > > > > I just tested this on a mac with Google maps, > > > input_handler_->HaveWheelEventHandlersAt( > > > gfx::Point(gesture_event.x, gesture_event.y)) > > > returns true in my testing. > > > > > Did you test after adding event.sourceDevice = WebGestureDeviceTouchpad here? > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/an... > > I mean, how did you translate joystick event to wheel event? > > > > > Do you have a Mac available? Does touchpad pinch zoom work for you on Google > > > maps? > > > > I don't have Mac setup, so cant verify/find root cause for failure. > > On android, google map does not zoom even without my change. > > I am not sure why condition is getting failed for android platforms. > > I was just testing touchpad pinch zoom on mac. > > Sorry, I forgot that you'd be getting the mobile version of Google maps, which > doesn't listen to wheel events, so Google maps is a bad test case. > > You could test desktop maps by setting the user agent via the --user-agent > command line flag. > > I think that behaving the same as touchpad zoom is correct, even if Google maps > doesn't currently support it. > Does that sound reasonable? Yes, I agree on the fact that there can be issue to specific sites. I guess, you can review the remaining patch and let me know if any further refinements are needed.
aelias, can you take a look at this? I'm happy with how this looks, though I still need to test it out. https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2749: long timeMs = SystemClock.uptimeMillis(); Is this clock in the same timebase as normal WebInputEvent timestamps? https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2754: nativePinchEnd(mNativeContentViewCore, timeMs); Instead of sending Begin/Update/End every animation tick, could we just send begin and end at the begin and end of the animation? https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java (right): https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:15: * Page Zoomin is propertional to LTRIGGER axis movement. Update comment.
Please add a unit test for this along the lines of ContentViewScrollingTest.scrollWithJoystick. https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2751: mNativeContentViewCore, timeMs, (int) mLastFocalEventX, (int) mLastFocalEventY); Will there necessarily be a reasonable mLastFocalEvent given a gamepad-primary interaction scenario? Should we just use the center of the screen instead? https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java (right): https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:18: public class JoystickZoomProvider { This new class is pretty similar to JoystickScrollProvider, can you merge them together? It can be renamed to JoystickScrollAndZoomProvider if you prefer. https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:85: final float deviceScaleFactor = mContentView.getRenderCoordinates().getDeviceScaleFactor(); I don't see any relationship between deviceScaleFactor and this feature. Please remove the multiplication by deviceScaleFactor. If needed express the speed you observe from this as a constant instead. Otherwise zoom velocity will just vary weirdly on different devices. https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:104: if (mZoomInVelocity != 0) { There shouldn't be an if/else here. The two triggers may be pressed at the same time and I think they should cancel each other out if so. Let's just include both effects into the same formula. https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:105: final float zoomInFactor = 1.0f + (zoomScaleFactor * mZoomInVelocity * dt / 1000f); These formulas don't make sense to me. As a rule it's incorrect to ever add/subtract scales, and I don't any reason that either the current scale or max scale need to be used here. This code is supposed to convert a two magnitudes from zero to one (I assume that's the axis range) to a multiplicative delta around 1. Let's just find a formula to do that directly without involving in random unrelated values. I'd suggest a pow formula looking like this: const float maxZoomPerFrame = 1.01f; // tweak this constant based on feel float numFramesElapsed = dt / 16; float zoomFactor = Math.pow(maxZoomPerFrame, (mZoomInVelocity - mZoomOutVelocity) * numFramesElapsed); mContentView.pinchByDeltaUseLastLocation(zoomFactor); https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:106: if (mContentView.canZoomIn()) { I think it's unnecessary complexity to check this, in general it's not the browser process's business to think about the min/max scale limits. Let's remove this and the stop() call here and just rely on stop getting called when the axis magnitudes are near zero. https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:139: return axisValWithNoise - JOYSTICK_NOISE_THRESHOLD; Please remove the subtraction here, it denormalizes the value in a weird way.
aelias@chromium.org changed reviewers: - jdduke@chromium.org
https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2751: mNativeContentViewCore, timeMs, (int) mLastFocalEventX, (int) mLastFocalEventY); On 2015/12/08 21:10:20, aelias wrote: > Will there necessarily be a reasonable mLastFocalEvent given a gamepad-primary > interaction scenario? Should we just use the center of the screen instead? I was just playing with this, and most of my interactions end up using the top left of the screen, which is pretty annoying. We should at least use the center of the screen when there's nothing focused. https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java (right): https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:105: final float zoomInFactor = 1.0f + (zoomScaleFactor * mZoomInVelocity * dt / 1000f); On 2015/12/08 21:10:20, aelias wrote: > These formulas don't make sense to me. As a rule it's incorrect to ever > add/subtract scales, and I don't any reason that either the current scale or max > scale need to be used here. > > This code is supposed to convert a two magnitudes from zero to one (I assume > that's the axis range) to a multiplicative delta around 1. Let's just find a > formula to do that directly without involving in random unrelated values. I'd > suggest a pow formula looking like this: > > const float maxZoomPerFrame = 1.01f; // tweak this constant based on feel > float numFramesElapsed = dt / 16; > float zoomFactor = Math.pow(maxZoomPerFrame, (mZoomInVelocity - > mZoomOutVelocity) * numFramesElapsed); > mContentView.pinchByDeltaUseLastLocation(zoomFactor); The current logic also feels pretty odd - zooming is much slower at higher magnitudes (including zooming out). I'll try again once the formula has been updated.
Thanks for comments, I will accommodate changes in next patch. https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2751: mNativeContentViewCore, timeMs, (int) mLastFocalEventX, (int) mLastFocalEventY); On 2015/12/09 13:34:50, tdresser wrote: > On 2015/12/08 21:10:20, aelias wrote: > > Will there necessarily be a reasonable mLastFocalEvent given a gamepad-primary > > interaction scenario? Should we just use the center of the screen instead? > > I was just playing with this, and most of my interactions end up using the top > left of the screen, which is pretty annoying. > > We should at least use the center of the screen when there's nothing focused. I have replied to same query on comment #6. But if feels annoying then I am fine with zooming at center. I will update the logic. https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2754: nativePinchEnd(mNativeContentViewCore, timeMs); On 2015/12/07 13:06:15, tdresser wrote: > Instead of sending Begin/Update/End every animation tick, could we just send > begin and end at the begin and end of the animation? Okay, I will accommodate it in next patch. https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java (right): https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:105: final float zoomInFactor = 1.0f + (zoomScaleFactor * mZoomInVelocity * dt / 1000f); On 2015/12/09 13:34:50, tdresser wrote: > On 2015/12/08 21:10:20, aelias wrote: > > These formulas don't make sense to me. As a rule it's incorrect to ever > > add/subtract scales, and I don't any reason that either the current scale or > max > > scale need to be used here. > > > > This code is supposed to convert a two magnitudes from zero to one (I assume > > that's the axis range) to a multiplicative delta around 1. Let's just find a > > formula to do that directly without involving in random unrelated values. I'd > > suggest a pow formula looking like this: > > > > const float maxZoomPerFrame = 1.01f; // tweak this constant based on feel > > float numFramesElapsed = dt / 16; > > float zoomFactor = Math.pow(maxZoomPerFrame, (mZoomInVelocity - > > mZoomOutVelocity) * numFramesElapsed); > > mContentView.pinchByDeltaUseLastLocation(zoomFactor); > > The current logic also feels pretty odd - zooming is much slower at higher > magnitudes (including zooming out). > > I'll try again once the formula has been updated. It has done intentionally to zoom at lower rate when current page is already zoomed. If we keep zooming page in linear rate, user gets small window to zoom. In little time page gets scaled to max value. Following equation takes care of decreasing zoom rate at higher zoom current level. zoomScaleFactor = (pageMaxScale - pageScale) / pageMaxScale You can verify functionality by removing zoomScaleFactor variable from zoomFactor calculation. https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:105: final float zoomInFactor = 1.0f + (zoomScaleFactor * mZoomInVelocity * dt / 1000f); On 2015/12/08 21:10:20, aelias wrote: > These formulas don't make sense to me. As a rule it's incorrect to ever > add/subtract scales, and I don't any reason that either the current scale or max > scale need to be used here. > > This code is supposed to convert a two magnitudes from zero to one (I assume > that's the axis range) to a multiplicative delta around 1. Let's just find a > formula to do that directly without involving in random unrelated values. I'd > suggest a pow formula looking like this: > > const float maxZoomPerFrame = 1.01f; // tweak this constant based on feel > float numFramesElapsed = dt / 16; > float zoomFactor = Math.pow(maxZoomPerFrame, (mZoomInVelocity - > mZoomOutVelocity) * numFramesElapsed); > mContentView.pinchByDeltaUseLastLocation(zoomFactor); Thanks for formula. I will update the formula and check the behavior on device.
On 2015/12/10 11:00:18, sshelke wrote: > Thanks for comments, I will accommodate changes in next patch. > > https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2751: > mNativeContentViewCore, timeMs, (int) mLastFocalEventX, (int) mLastFocalEventY); > On 2015/12/09 13:34:50, tdresser wrote: > > On 2015/12/08 21:10:20, aelias wrote: > > > Will there necessarily be a reasonable mLastFocalEvent given a > gamepad-primary > > > interaction scenario? Should we just use the center of the screen instead? > > > > I was just playing with this, and most of my interactions end up using the top > > left of the screen, which is pretty annoying. > > > > We should at least use the center of the screen when there's nothing focused. > > I have replied to same query on comment #6. But if feels annoying then I am fine > with > zooming at center. I will update the logic. > > https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2754: > nativePinchEnd(mNativeContentViewCore, timeMs); > On 2015/12/07 13:06:15, tdresser wrote: > > Instead of sending Begin/Update/End every animation tick, could we just send > > begin and end at the begin and end of the animation? > > Okay, I will accommodate it in next patch. > > https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java > (right): > > https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:105: > final float zoomInFactor = 1.0f + (zoomScaleFactor * mZoomInVelocity * dt / > 1000f); > On 2015/12/09 13:34:50, tdresser wrote: > > On 2015/12/08 21:10:20, aelias wrote: > > > These formulas don't make sense to me. As a rule it's incorrect to ever > > > add/subtract scales, and I don't any reason that either the current scale or > > max > > > scale need to be used here. > > > > > > This code is supposed to convert a two magnitudes from zero to one (I assume > > > that's the axis range) to a multiplicative delta around 1. Let's just find > a > > > formula to do that directly without involving in random unrelated values. > I'd > > > suggest a pow formula looking like this: > > > > > > const float maxZoomPerFrame = 1.01f; // tweak this constant based on feel > > > float numFramesElapsed = dt / 16; > > > float zoomFactor = Math.pow(maxZoomPerFrame, (mZoomInVelocity - > > > mZoomOutVelocity) * numFramesElapsed); > > > mContentView.pinchByDeltaUseLastLocation(zoomFactor); > > > > The current logic also feels pretty odd - zooming is much slower at higher > > magnitudes (including zooming out). > > > > I'll try again once the formula has been updated. > It has done intentionally to zoom at lower rate when current page is already > zoomed. > If we keep zooming page in linear rate, user gets small window to zoom. In > little time > page gets scaled to max value. Following equation takes care of decreasing zoom > rate at > higher zoom current level. > zoomScaleFactor = (pageMaxScale - pageScale) / pageMaxScale > > You can verify functionality by removing zoomScaleFactor variable from > zoomFactor > calculation. > > https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:105: > final float zoomInFactor = 1.0f + (zoomScaleFactor * mZoomInVelocity * dt / > 1000f); > On 2015/12/08 21:10:20, aelias wrote: > > These formulas don't make sense to me. As a rule it's incorrect to ever > > add/subtract scales, and I don't any reason that either the current scale or > max > > scale need to be used here. > > > > This code is supposed to convert a two magnitudes from zero to one (I assume > > that's the axis range) to a multiplicative delta around 1. Let's just find a > > formula to do that directly without involving in random unrelated values. I'd > > suggest a pow formula looking like this: > > > > const float maxZoomPerFrame = 1.01f; // tweak this constant based on feel > > float numFramesElapsed = dt / 16; > > float zoomFactor = Math.pow(maxZoomPerFrame, (mZoomInVelocity - > > mZoomOutVelocity) * numFramesElapsed); > > mContentView.pinchByDeltaUseLastLocation(zoomFactor); > > Thanks for formula. I will update the formula and check the behavior on device. I don't think we want to decrease the zoom rate at higher zoom levels - at least not for both zooming in and out. If the user wants to zoom all the way out, that should be a quick gesture. I can picture maybe zooming in slower once you're zoomed in a lot, but you don't want zooming out to be slow at that point.
https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java (right): https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:104: if (mZoomInVelocity != 0) { On 2015/12/08 21:10:20, aelias wrote: > There shouldn't be an if/else here. The two triggers may be pressed at the same > time and I think they should cancel each other out if so. Let's just include > both effects into the same formula. I guess, only one event gets processed even if two triggers are pressed simultaneously.Android queues similar events and send to browser app only one at a time for processing.So I think, content layer would be receiving either LT or RT event and not both at same time.I put if/else condition to have different logic for zoom-in and zoom-out.If page is zoomed a lot already then zooming rate should be decreased. Same way if page is zoomed a lot already then zoom out rate should be more to zoom out fast initially and then zooming out rate would keep on decreasing.
On 2015/12/16 08:31:20, sshelke wrote: > https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java > (right): > > https://codereview.chromium.org/1288243004/diff/40001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:104: > if (mZoomInVelocity != 0) { > On 2015/12/08 21:10:20, aelias wrote: > > There shouldn't be an if/else here. The two triggers may be pressed at the > same > > time and I think they should cancel each other out if so. Let's just include > > both effects into the same formula. > > I guess, only one event gets processed even if two triggers are pressed > simultaneously.Android queues similar events and send to browser app only one at > a time for processing.So I think, content layer would be receiving either LT or > RT event and not both at same time.I put if/else condition to have different > logic for zoom-in and zoom-out.If page is zoomed a lot already then zooming rate > should be decreased. Same way if page is zoomed a lot already then zoom out rate > should be more to zoom out fast initially and then zooming out rate would keep > on decreasing. Uploaded new patch which contains new formula for zoom in/out. Also fixed previous comments. Please review and verify functionality on device. Thanks
This still needs a test, and aelias@ requested that this be merged with the JoystickScrollProvider. Both zooming in and out feel too slow on a Nexus 5 with an XBox 360 controller. What devices are you testing on? https://codereview.chromium.org/1288243004/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java (right): https://codereview.chromium.org/1288243004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:25: private static final float MAXZOOMPERFRAME = 1.01f; MAX_ZOOM_PER_FRAME
>Both zooming in and out feel too slow on a Nexus 5 with an XBox 360 controller. > What devices are you testing on? I am verifying code on shield tablet and PixelC with XBox 360 controller. I guess, we should include deviceScaleFactor in zoomFactor calculations. It will allow change in zooming percentage as per device resolution. Have seen significant increased in zoom rate after including deviceScaleFactor in calculations. Could you verify functionality by multiplying zoom factor with device scale factor on Nexus 5? > This still needs a test, and aelias@ requested that this be merged with the > JoystickScrollProvider. I am writing the test, also finding ways to merge zoom and scroll code together. It is bit complex to maintain two runnable threads inside one class since both have different start and exit conditions. Also we cant have advantage of lazy loading in case of JoystickZoomProvider. In case of JoystickScrollProvider, it needs to be initialized in ContentViewCore constructor. Could design be modified as per comment #2 instead of merging scroll and zooming logic in one class?
On 2015/12/17 10:19:51, sshelke wrote: > >Both zooming in and out feel too slow on a Nexus 5 with an XBox 360 controller. > > > What devices are you testing on? > > I am verifying code on shield tablet and PixelC with XBox 360 controller. > I guess, we should include deviceScaleFactor in zoomFactor calculations. > It will allow change in zooming percentage as per device resolution. > Have seen significant increased in zoom rate after including > deviceScaleFactor in calculations. > Could you verify functionality by multiplying zoom factor with device > scale factor on Nexus 5? This feels pretty great, to me! Should I be able to focus things using the gamepad? I don't seem to be able to - am I just missing something? > > > This still needs a test, and aelias@ requested that this be merged with the > > JoystickScrollProvider. > I am writing the test, also finding ways to merge zoom and scroll code together. > It is bit complex to maintain two runnable threads inside one class since both > have different start and exit conditions. Also we cant have advantage of lazy > loading in case of JoystickZoomProvider. In case of JoystickScrollProvider, it > needs to be initialized in ContentViewCore constructor. > Could design be modified as per comment #2 instead of merging scroll and zooming > logic in one class? aelias@, what do you think of that approach? It seems reasonable to me.
> Should I be able to focus things using the gamepad? > I don't seem to be able to - am I just missing something? Have not implemented focus using gamepad logic in chromium yet. But devices likes shield, map right joystick (RS) to mouse. This mapping is done at android framework level so content layer receive it as mouse events instead of RS events. So for shield devices, you can focus specific element using RS. You can also avail spatial navigation functionality using --enable-spatial-navigation. This feature allows to focus elements using DPAD. Please build chromium public apk and use all gamepad functionality (check mapping @ http://code.google.com/p/chromium/issues/detail?id=454355#c21) and let me know if you have any suggestions. Thanks for review.
This doesn't appear to work well on pages that start off zoomed out (likely due to the way it reads the PageScaleFactor).
OK, I don't feel that strongly about merging the classes, it's fine to leave them separate. This still needs a test though. https://codereview.chromium.org/1288243004/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java (right): https://codereview.chromium.org/1288243004/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:115: ? (pageMaxScale - pageScale) / pageMaxScale This formula here still makes no sense. As I said, adding/subtracting scales is always incorrect. Please don't make use of the current pageScale/maxPageScale anywhere in this class. They're not appropriate inputs to the formula and they can only make things worse.
Removed page scale variables from page factor calculations. Added testJoystickZoom test. Please review and let me know any further refinements need to be done.
https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java (right): https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:15: * Page Zoomin is propertional to RTRIGGER axis movement. propertional -> proportional https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:21: private static final float JOYSTICK_NOISE_THRESHOLD = 0.2f; Can you add a comment explaining each of these constants? https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:115: deviceScaleFactor * (mZoomInVelocity - mZoomOutVelocity) * dt / 1000f); I was a fan of the curve that was here previously, where the zoom slowed down as it reached its destination. aelias@, isn't that a reasonable use of pageScale / maxPageScale? https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java (right): https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:24: + "initial-scale=2.0, minimum-scale=1.0, maximum-scale=5.0\" />" Let's use a non-standard minimum scale here. https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:30: final float maxThreshold) throws InterruptedException { Couldn't you just wait until the zoom is approximately equal to some value, instead of giving such a large range of acceptable values? https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:74: // At the start of test, page scale might be greater than 2 Why would page scale be greater than 2?
https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java (right): https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:115: deviceScaleFactor * (mZoomInVelocity - mZoomOutVelocity) * dt / 1000f); On 2016/01/06 at 16:09:06, tdresser (OOO until Jan 13) wrote: > I was a fan of the curve that was here previously, where the zoom slowed down as it reached its destination. > > aelias@, isn't that a reasonable use of pageScale / maxPageScale? Well not exactly, for starters I would also expect to see minPageScale somewhere in the formula if you want to slow down near both ends of the range. But that doesn't feel right to me either, I feel like just like scrolling isn't affected by how far it is from the extents, zooming shouldn't be either. Is your concern that it stops too abruptly? I'd suggest there could be some inertial effect so that the zoom loses momentum gradually after release, still without taking maxPageScale into consideration. https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java (right): https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:21: public class ContentViewZoomingTest extends ContentShellTestBase { I remembered the joystick scrolling test you added is disabled for being flaky (http://crbug.com/538781 ). Let's not land this patch containing a new similar test until that one is deflaked and reenabled, otherwise we risk just adding another flake.
On 2016/01/07 00:08:36, aelias wrote: > https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... > File > content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java > (right): > > https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... > content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:115: > deviceScaleFactor * (mZoomInVelocity - mZoomOutVelocity) * dt / 1000f); > On 2016/01/06 at 16:09:06, tdresser (OOO until Jan 13) wrote: > > I was a fan of the curve that was here previously, where the zoom slowed down > as it reached its destination. > > > > aelias@, isn't that a reasonable use of pageScale / maxPageScale? > > Well not exactly, for starters I would also expect to see minPageScale somewhere > in the formula if you want to slow down near both ends of the range. But that > doesn't feel right to me either, I feel like just like scrolling isn't affected > by how far it is from the extents, zooming shouldn't be either. > > Is your concern that it stops too abruptly? I'd suggest there could be some > inertial effect so that the zoom loses momentum gradually after release, still > without taking maxPageScale into consideration. > > https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... > File > content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java > (right): > > https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... > content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:21: > public class ContentViewZoomingTest extends ContentShellTestBase { > I remembered the joystick scrolling test you added is disabled for being flaky > (http://crbug.com/538781 ). Let's not land this patch containing a new similar > test until that one is deflaked and reenabled, otherwise we risk just adding > another flake. I guess, testJoystickScroll and testJoystickZoom tests are completely different. I am not sure whether reasons causing testJoystickScroll flakiness can cause similar flakiness in testJoystickZoom. I also tried to repro scrolling flakiness issue on tegra devices but could not repro. Not sure how to contribute in fixing flakiness issue. If you think, zoom test might cause flakiness issue then its better to wait till joystick scrolling test gets re-enabled.
aelias@, yeah, I'm concerned that it feels too abrupt. The primary case that it feels a bit chunky is when you hit the max or min zoom, so adding an inertial effect doesn't help at all. Your argument that we should be consistent with scrolling is reasonably convincing, though intuitively I find it feels a lot better with a curve there. I'm fine without a curve though.
OK, getting there. https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java (right): https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:23: private static final float JOYSTICK_ZOOM_MULTIPLIER = 50f; This value isn't needed, please delete it. You can just set MAX_ZOOM_PER_FRAME to 1.65 (which is 1.01^50) instead. https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:25: private static final float MAX_ZOOM_PER_FRAME = 1.01f; This name isn't accurate anymore because of the other constants you set, maybe just call it ZOOM_SPEED. https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:29: private boolean mAutoZoomActive; Please delete this and rely on the time value as in https://codereview.chromium.org/1571043002/ https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:37: private int mZoomXcord; Spelling: "coord" https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:82: mAutoZoomActive = true; Please set starting time here as in https://codereview.chromium.org/1571043002/ https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:110: if (mLastAnimateTimeMillis != 0 && timeMillis > mLastAnimateTimeMillis) { Please replace this with an early-return as in https://codereview.chromium.org/1571043002/ https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:115: deviceScaleFactor * (mZoomInVelocity - mZoomOutVelocity) * dt / 1000f); On 2016/01/07 at 00:08:36, aelias wrote: > Is your concern that it stops too abruptly? I'd suggest there could be some inertial effect so that the zoom loses momentum gradually after release, still without taking maxPageScale into consideration. I tried it and I don't feel the stop at the edges is a big problem (feels similar to touch pinch zoom stoppage), so let's leave this as is. https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java (right): https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:71: @RerunWithUpdatedContainerView Please remove RerunWithUpdatedContainerView as the scenario (WebView fullscreen video) doesn't matter to test, and it might've caused flakiness last time. https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:89: assertWaitForZoom(false, 2.5f, 3.5f); This risks flaking if the test bot is running slowly enough that the animation skips right over the range 2.5-3.5. Please only write a test either 1) checking the deadzone or 2) waiting for maximum and minimum scale to be reached, no intermediate values. https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:102: @RerunWithUpdatedContainerView Likewise please remove this annotaion.
fixed comments in latest patch. Please review. https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java (right): https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:15: * Page Zoomin is propertional to RTRIGGER axis movement. On 2016/01/06 16:09:06, tdresser wrote: > propertional -> proportional Done. https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:21: private static final float JOYSTICK_NOISE_THRESHOLD = 0.2f; On 2016/01/06 16:09:06, tdresser wrote: > Can you add a comment explaining each of these constants? Done. https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:23: private static final float JOYSTICK_ZOOM_MULTIPLIER = 50f; On 2016/01/16 02:59:33, aelias wrote: > This value isn't needed, please delete it. You can just set MAX_ZOOM_PER_FRAME > to 1.65 (which is 1.01^50) instead. Done. https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:25: private static final float MAX_ZOOM_PER_FRAME = 1.01f; On 2016/01/16 02:59:33, aelias wrote: > This name isn't accurate anymore because of the other constants you set, maybe > just call it ZOOM_SPEED. Done. https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:29: private boolean mAutoZoomActive; On 2016/01/16 02:59:33, aelias wrote: > Please delete this and rely on the time value as in > https://codereview.chromium.org/1571043002/ Done. https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:37: private int mZoomXcord; On 2016/01/16 02:59:33, aelias wrote: > Spelling: "coord" Done. https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:82: mAutoZoomActive = true; On 2016/01/16 02:59:33, aelias wrote: > Please set starting time here as in https://codereview.chromium.org/1571043002/ Done. https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:110: if (mLastAnimateTimeMillis != 0 && timeMillis > mLastAnimateTimeMillis) { On 2016/01/16 02:59:32, aelias wrote: > Please replace this with an early-return as in > https://codereview.chromium.org/1571043002/ Done. https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java (right): https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:21: public class ContentViewZoomingTest extends ContentShellTestBase { On 2016/01/07 00:08:36, aelias wrote: > I remembered the joystick scrolling test you added is disabled for being flaky > (http://crbug.com/538781 ). Let's not land this patch containing a new similar > test until that one is deflaked and reenabled, otherwise we risk just adding > another flake. Done. https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:24: + "initial-scale=2.0, minimum-scale=1.0, maximum-scale=5.0\" />" On 2016/01/06 16:09:06, tdresser wrote: > Let's use a non-standard minimum scale here. Done. https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:71: @RerunWithUpdatedContainerView On 2016/01/16 02:59:33, aelias wrote: > Please remove RerunWithUpdatedContainerView as the scenario (WebView fullscreen > video) doesn't matter to test, and it might've caused flakiness last time. Done. https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:74: // At the start of test, page scale might be greater than 2 On 2016/01/06 16:09:06, tdresser wrote: > Why would page scale be greater than 2? Done. https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:89: assertWaitForZoom(false, 2.5f, 3.5f); On 2016/01/16 02:59:33, aelias wrote: > This risks flaking if the test bot is running slowly enough that the animation > skips right over the range 2.5-3.5. Please only write a test either 1) checking > the deadzone or 2) waiting for maximum and minimum scale to be reached, no > intermediate values. Done. https://codereview.chromium.org/1288243004/diff/80001/content/public/android/... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:102: @RerunWithUpdatedContainerView On 2016/01/16 02:59:33, aelias wrote: > Likewise please remove this annotaion. Done.
Almost ready, I just have a few comments on the test. https://codereview.chromium.org/1288243004/diff/100001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java (right): https://codereview.chromium.org/1288243004/diff/100001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:29: private void assertWaitForZoom(final boolean isMinZoom, final float minThreshold, Please wait for an exact value instead of having a range, it should be fine to test for exact equality at the min/max scale bounds. https://codereview.chromium.org/1288243004/diff/100001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:82: // Check whether page can be scaled beyond max page scale I don't see what this is testing, please delete it (and the equivalent clause in zoomOut).
Sorry for delayed response. Was OOO last week. https://codereview.chromium.org/1288243004/diff/100001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java (right): https://codereview.chromium.org/1288243004/diff/100001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:29: private void assertWaitForZoom(final boolean isMinZoom, final float minThreshold, On 2016/01/20 04:10:43, aelias wrote: > Please wait for an exact value instead of having a range, it should be fine to > test for exact equality at the min/max scale bounds. I have designed test case on similar lines of scrolling test. Actually, I am bit worried of this test case becoming flaky if we check exact values. A slower test bot might miss checking exact values. If we allow exact value checking we might end up in introducing race conditions. https://codereview.chromium.org/1288243004/diff/100001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:82: // Check whether page can be scaled beyond max page scale On 2016/01/20 04:10:43, aelias wrote: > I don't see what this is testing, please delete it (and the equivalent clause in > zoomOut). This test case ensures that 1) Page should not be zoomed more than max page scale even if joystick is pressed. On the previous test case (i.e 3rd test case), page is zoomed to max scale (5.0). After that joystick event is provided to check whether page can be scaled beyond max page scale, if yes mark it as failure.
https://codereview.chromium.org/1288243004/diff/100001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java (right): https://codereview.chromium.org/1288243004/diff/100001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:29: private void assertWaitForZoom(final boolean isMinZoom, final float minThreshold, On 2016/01/25 09:19:16, sshelke wrote: > On 2016/01/20 04:10:43, aelias wrote: > > Please wait for an exact value instead of having a range, it should be fine to > > test for exact equality at the min/max scale bounds. > > I have designed test case on similar lines of scrolling test. > Actually, I am bit worried of this test case becoming flaky if > we check exact values. A slower test bot might miss checking > exact values. If we allow exact value checking we might end up > in introducing race conditions. We should wait until we're done zooming, in which case the speed of the bot shouldn't matter, should it?
https://codereview.chromium.org/1288243004/diff/100001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java (right): https://codereview.chromium.org/1288243004/diff/100001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:29: private void assertWaitForZoom(final boolean isMinZoom, final float minThreshold, On 2016/01/25 at 13:12:13, tdresser wrote: > On 2016/01/25 09:19:16, sshelke wrote: > > On 2016/01/20 04:10:43, aelias wrote: > > > Please wait for an exact value instead of having a range, it should be fine to > > > test for exact equality at the min/max scale bounds. > > > > I have designed test case on similar lines of scrolling test. > > Actually, I am bit worried of this test case becoming flaky if > > we check exact values. A slower test bot might miss checking > > exact values. If we allow exact value checking we might end up > > in introducing race conditions. > > We should wait until we're done zooming, in which case the speed of the bot shouldn't matter, should it? Yeah, there's no race if you wait until the min/max is reached. Only checking for intermediate values is racy. https://codereview.chromium.org/1288243004/diff/100001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:82: // Check whether page can be scaled beyond max page scale On 2016/01/25 at 09:19:16, sshelke wrote: > On 2016/01/20 04:10:43, aelias wrote: > > I don't see what this is testing, please delete it (and the equivalent clause in > > zoomOut). > > This test case ensures that > 1) Page should not be zoomed more than max page scale even > if joystick is pressed. > > On the previous test case (i.e 3rd test case), page is zoomed to max scale (5.0). > After that joystick event is provided to check whether page can be scaled > beyond max page scale, if yes mark it as failure. I don't see why this assertion would fail even if it did zoom beyond max scale. Anyway, it's the renderer CC responsibility to enforce scale limits and there's no real risk that joystick will somehow cause a regression.
On 2016/01/25 13:12:13, tdresser wrote: > https://codereview.chromium.org/1288243004/diff/100001/content/public/android... > File > content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java > (right): > > https://codereview.chromium.org/1288243004/diff/100001/content/public/android... > content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:29: > private void assertWaitForZoom(final boolean isMinZoom, final float > minThreshold, > On 2016/01/25 09:19:16, sshelke wrote: > > On 2016/01/20 04:10:43, aelias wrote: > > > Please wait for an exact value instead of having a range, it should be fine > to > > > test for exact equality at the min/max scale bounds. > > > > I have designed test case on similar lines of scrolling test. > > Actually, I am bit worried of this test case becoming flaky if > > we check exact values. A slower test bot might miss checking > > exact values. If we allow exact value checking we might end up > > in introducing race conditions. > > We should wait until we're done zooming, in which case the speed of the bot > shouldn't matter, should it? Page keeps on zooming until maximum/minimum point is reached. We don't have control over page zooming once joystick event is dispatched. It is test bot responsibility to stop page zooming once criterion is met. If we allow exact value checking then I guess we should not check intermediate page scale values, we should have test cases which compare current page scale values to max/min page scale.
Modified patch to remove certain test cases. Also in test cases only boundary values (min/max page scale) are checked.Please review, Thanks.
On 2016/02/03 09:26:25, sshelke wrote: > Modified patch to remove certain test cases. > Also in test cases only boundary values (min/max page scale) > are checked.Please review, Thanks. Could we explicitly tick the animation for some of these tests? That would let us test the intermediate values, without risk of timing related flake. You could make a TestJoystickZoomProvider which set mZoomRunnable to do nothing, and exposed a public method for ticking the animation.
On 2016/02/03 14:41:01, tdresser wrote: > On 2016/02/03 09:26:25, sshelke wrote: > > Modified patch to remove certain test cases. > > Also in test cases only boundary values (min/max page scale) > > are checked.Please review, Thanks. > > Could we explicitly tick the animation for some of these tests? > That would let us test the intermediate values, without risk of timing related > flake. > > You could make a TestJoystickZoomProvider which set mZoomRunnable to do nothing, > and exposed a public method for ticking the animation. Sorry, I could not understand it clearly. Could you please elaborate? Is there issue with checking intermediate values with previous logic (i.e checking range rather than checking exact values)?
On 2016/02/04 12:38:57, sshelke wrote: > On 2016/02/03 14:41:01, tdresser wrote: > > On 2016/02/03 09:26:25, sshelke wrote: > > > Modified patch to remove certain test cases. > > > Also in test cases only boundary values (min/max page scale) > > > are checked.Please review, Thanks. > > > > Could we explicitly tick the animation for some of these tests? > > That would let us test the intermediate values, without risk of timing related > > flake. > > > > You could make a TestJoystickZoomProvider which set mZoomRunnable to do > nothing, > > and exposed a public method for ticking the animation. > > Sorry, I could not understand it clearly. Could you > please elaborate? Is there issue with checking intermediate > values with previous logic (i.e checking range rather than > checking exact values)? Checking intermediate values with the previous logic is likely to be flakey, because it depends on how fast the animation runs. If a bot runs too slowly or quickly, the test will fail. We could explicitly control how fast the animation progresses, which would let us test the intermediate values without requiring a range.
I am still not sure how to achieve explicit control on animation progress. Could you please elaborate little bit on implementation. In all test cases related to content view, waiting is achieved only through CriteriaHelper.pollForCriteria which we have already used in zooming test. Actually, I have written logic on the line of ContentViewScrolling test in which intermediate values are checked using range instead of exact values.
On 2016/02/05 11:15:13, sshelke wrote: > I am still not sure how to achieve explicit control on animation progress. > Could you please elaborate little bit on implementation. > > In all test cases related to content view, waiting is achieved only > through CriteriaHelper.pollForCriteria which we have already used in > zooming test. Actually, I have written logic on the line of ContentViewScrolling > test in which intermediate values are checked using range instead of exact > values. The vast majority of the uses of CriteriaHelper.pollForCriteria don't suffer from timing issues though. Even if you're testing a range, we could poll at times at which the criteria is never satisfied. Instead of having to worry about timing issues, you could make You could make a TestJoystickZoomProvider, which is a subclass of JoystickZoomProvider. In the subclass, you would assign mZoomRunnable to a Runnable that does nothing, so that the animation is never ticked automatically. You would also add a public method to TestJoystickZoomProvider, something like AnimateZoomForTest, which would just call AnimateZoom. Your test would instantiate a TestJoystickZoomProvider, and test it, instead of a normal JoystickZoomProvider. With this approach, you can send some input, then tick 20 times, and assert that the zoom level is correct, and then tick another 50 times, and assert that the zoom level is correct. There's no timing issue (and I suspect the test will also run faster since it won't depend on the rate that animations are normally run at). Does that seem reasonable to you?
Thanks for clarification, seems nice approach in terms of avoiding flakiness
caused by animation. Let me put in words what I understood,
So in ContentViewZoomingTest, there will function accessing
TestJoystickZoomProvider,
void zoomWithJoystick() {
// We need to send joystick event to JoyStickZoomProvider
// TestJoystickZoomProvider.onMotion needs to be called since
// it is entry point to zoomProvider for motion events
MotionEvent rt_joystick_event;
int num_ticks = 20;
float expected_zoom_factor = 2.5;
TestJoyStickZoomProvider.onMotion(rt_joystick_event, num_ticks);
assertEquals(getContentViewCore().getScale(), expected_zoom_factor);
num_ticks = 20;
expected_zoom_factor = 3.57;
TestJoyStickZoomProvider.onMotion(rt_joystick_event, num_ticks);
assertEquals(getContentViewCore().getScale(), expected_zoom_factor);
}
If I am thinking in right direction, we should know expected zoom factor
after specific number of ticks. But zoom factor calculation is bit of random
in JoystickZoomProvider, since it depends on dt which is calculated using
AnimationUtils.currentAnimationTimeMillis().
Value returned by above api can be varied from 3ms to 16ms,
so we cant predict exact value for zoom factor.
On 2016/02/08 09:16:23, sshelke wrote:
> Thanks for clarification, seems nice approach in terms of avoiding flakiness
> caused by animation. Let me put in words what I understood,
> So in ContentViewZoomingTest, there will function accessing
> TestJoystickZoomProvider,
>
> void zoomWithJoystick() {
> // We need to send joystick event to JoyStickZoomProvider
> // TestJoystickZoomProvider.onMotion needs to be called since
> // it is entry point to zoomProvider for motion events
> MotionEvent rt_joystick_event;
>
> int num_ticks = 20;
> float expected_zoom_factor = 2.5;
> TestJoyStickZoomProvider.onMotion(rt_joystick_event, num_ticks);
> assertEquals(getContentViewCore().getScale(), expected_zoom_factor);
>
> num_ticks = 20;
> expected_zoom_factor = 3.57;
> TestJoyStickZoomProvider.onMotion(rt_joystick_event, num_ticks);
> assertEquals(getContentViewCore().getScale(), expected_zoom_factor);
>
> }
>
> If I am thinking in right direction, we should know expected zoom factor
> after specific number of ticks. But zoom factor calculation is bit of random
> in JoystickZoomProvider, since it depends on dt which is calculated using
> AnimationUtils.currentAnimationTimeMillis().
> Value returned by above api can be varied from 3ms to 16ms,
> so we cant predict exact value for zoom factor.
That looks similar to what I was thinking.
I was thinking you'd tick explicitly in each test, so something like:
@Feature({"JoystickZoom"})
public void testJoystickZoomIn() throws Throwable {
// Verify page does not zoom-in if trigger motion falls in deadzone.
zoomWithJoystick(0.1f, true);
int num_ticks = 20;
testJoyStickZoomProvider.AnimateZoomForTest(num_ticks);
assertEquals(getContentViewCore().getScale(), ?);
...
}
You're right that the dt computation makes this a bit trickier.
You can get around this by adding a protected method, something like
getTimeSinceLastAnimationFrame
which just returns
AnimationUtils.currentAnimationTimeMillis() - mLastAnimateTimeMillis.
Then TestJoystickZoomProvider can override this method - I suspect that just
always returning 16 would be fine for tests.
Does that seem reasonable to you?
On 2016/02/08 15:23:35, tdresser wrote:
> On 2016/02/08 09:16:23, sshelke wrote:
> > Thanks for clarification, seems nice approach in terms of avoiding flakiness
> > caused by animation. Let me put in words what I understood,
> > So in ContentViewZoomingTest, there will function accessing
> > TestJoystickZoomProvider,
> >
> > void zoomWithJoystick() {
> > // We need to send joystick event to JoyStickZoomProvider
> > // TestJoystickZoomProvider.onMotion needs to be called since
> > // it is entry point to zoomProvider for motion events
> > MotionEvent rt_joystick_event;
> >
> > int num_ticks = 20;
> > float expected_zoom_factor = 2.5;
> > TestJoyStickZoomProvider.onMotion(rt_joystick_event, num_ticks);
> > assertEquals(getContentViewCore().getScale(), expected_zoom_factor);
> >
> > num_ticks = 20;
> > expected_zoom_factor = 3.57;
> > TestJoyStickZoomProvider.onMotion(rt_joystick_event, num_ticks);
> > assertEquals(getContentViewCore().getScale(), expected_zoom_factor);
> >
> > }
> >
> > If I am thinking in right direction, we should know expected zoom factor
> > after specific number of ticks. But zoom factor calculation is bit of random
> > in JoystickZoomProvider, since it depends on dt which is calculated using
> > AnimationUtils.currentAnimationTimeMillis().
> > Value returned by above api can be varied from 3ms to 16ms,
> > so we cant predict exact value for zoom factor.
>
> That looks similar to what I was thinking.
>
> I was thinking you'd tick explicitly in each test, so something like:
>
> @Feature({"JoystickZoom"})
> public void testJoystickZoomIn() throws Throwable {
> // Verify page does not zoom-in if trigger motion falls in deadzone.
> zoomWithJoystick(0.1f, true);
> int num_ticks = 20;
> testJoyStickZoomProvider.AnimateZoomForTest(num_ticks);
> assertEquals(getContentViewCore().getScale(), ?);
> ...
> }
>
> You're right that the dt computation makes this a bit trickier.
>
> You can get around this by adding a protected method, something like
> getTimeSinceLastAnimationFrame
> which just returns
> AnimationUtils.currentAnimationTimeMillis() - mLastAnimateTimeMillis.
>
> Then TestJoystickZoomProvider can override this method - I suspect that just
> always returning 16 would be fine for tests.
>
> Does that seem reasonable to you?
Okay, seem reasonable to me but I guess it would diverge from traditional test
skeleton of content view. Would that be issue?
On 2016/02/09 12:49:46, sshelke wrote:
> On 2016/02/08 15:23:35, tdresser wrote:
> > On 2016/02/08 09:16:23, sshelke wrote:
> > > Thanks for clarification, seems nice approach in terms of avoiding
flakiness
>
> > > caused by animation. Let me put in words what I understood,
> > > So in ContentViewZoomingTest, there will function accessing
> > > TestJoystickZoomProvider,
> > >
> > > void zoomWithJoystick() {
> > > // We need to send joystick event to JoyStickZoomProvider
> > > // TestJoystickZoomProvider.onMotion needs to be called since
> > > // it is entry point to zoomProvider for motion events
> > > MotionEvent rt_joystick_event;
> > >
> > > int num_ticks = 20;
> > > float expected_zoom_factor = 2.5;
> > > TestJoyStickZoomProvider.onMotion(rt_joystick_event, num_ticks);
> > > assertEquals(getContentViewCore().getScale(), expected_zoom_factor);
> > >
> > > num_ticks = 20;
> > > expected_zoom_factor = 3.57;
> > > TestJoyStickZoomProvider.onMotion(rt_joystick_event, num_ticks);
> > > assertEquals(getContentViewCore().getScale(), expected_zoom_factor);
> > >
> > > }
> > >
> > > If I am thinking in right direction, we should know expected zoom factor
> > > after specific number of ticks. But zoom factor calculation is bit of
random
> > > in JoystickZoomProvider, since it depends on dt which is calculated using
> > > AnimationUtils.currentAnimationTimeMillis().
> > > Value returned by above api can be varied from 3ms to 16ms,
> > > so we cant predict exact value for zoom factor.
> >
> > That looks similar to what I was thinking.
> >
> > I was thinking you'd tick explicitly in each test, so something like:
> >
> > @Feature({"JoystickZoom"})
> > public void testJoystickZoomIn() throws Throwable {
> > // Verify page does not zoom-in if trigger motion falls in deadzone.
> > zoomWithJoystick(0.1f, true);
> > int num_ticks = 20;
> > testJoyStickZoomProvider.AnimateZoomForTest(num_ticks);
> > assertEquals(getContentViewCore().getScale(), ?);
> > ...
> > }
> >
> > You're right that the dt computation makes this a bit trickier.
> >
> > You can get around this by adding a protected method, something like
> > getTimeSinceLastAnimationFrame
> > which just returns
> > AnimationUtils.currentAnimationTimeMillis() - mLastAnimateTimeMillis.
> >
> > Then TestJoystickZoomProvider can override this method - I suspect that just
> > always returning 16 would be fine for tests.
> >
> > Does that seem reasonable to you?
>
> Okay, seem reasonable to me but I guess it would diverge from traditional test
> skeleton of content view. Would that be issue?
I'll defer to aelias@ on whether or not this approach to testing is acceptable
in Java-land.
I think the most natural way would be dependency injection, i.e. the class would take a interface with one getCurrentAnimationTime method and the test can provide a different implementation.
On 2016/02/10 07:13:12, aelias wrote: > I think the most natural way would be dependency injection, i.e. the class would > take a interface with one getCurrentAnimationTime method and the test can > provide a different implementation. Since you still need to make a subclass in order to expose the animateZoom method, I'd have a mild preference to just using a subclass, but that may be because that's the approach we use more often in C++ in chromium. sshelke@, how about you give it a shot using the approach that Alex described.
On 2016/02/10 12:57:51, tdresser wrote: > On 2016/02/10 07:13:12, aelias wrote: > > I think the most natural way would be dependency injection, i.e. the class > would > > take a interface with one getCurrentAnimationTime method and the test can > > provide a different implementation. > > Since you still need to make a subclass in order to expose the animateZoom > method, I'd have a mild preference to just using a subclass, but that may be > because that's the approach we use more often in C++ in chromium. > > sshelke@, how about you give it a shot using the approach that Alex described. Okay sure, I will implement logic suggested by Alex and upload modified patch. Thanks
On 2016/02/10 13:04:58, sshelke wrote: > On 2016/02/10 12:57:51, tdresser wrote: > > On 2016/02/10 07:13:12, aelias wrote: > > > I think the most natural way would be dependency injection, i.e. the class > > would > > > take a interface with one getCurrentAnimationTime method and the test can > > > provide a different implementation. > > > > Since you still need to make a subclass in order to expose the animateZoom > > method, I'd have a mild preference to just using a subclass, but that may be > > because that's the approach we use more often in C++ in chromium. > > > > sshelke@, how about you give it a shot using the approach that Alex described. > > Okay sure, I will implement logic suggested by Alex and upload > modified patch. Thanks PTAL, Thanks
This is much better, the tests will be much more robust now. I've made comments on the current approach that the code takes. This isn't performing the dependency injection that aelias@ suggested though. He proposed that you define an interface with one getCurrentAnimationTime method. The JoystickZoomProvider would take an object with this interface as a parameter to its constructor. ContentViewCore would pass the JoystickZoomProvider the standard implementation of this interface, and the test would pass it a different implementation. https://codereview.chromium.org/1288243004/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java (right): https://codereview.chromium.org/1288243004/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:85: public long getLastAnimationFrameInterval() { Can this be protected? https://codereview.chromium.org/1288243004/diff/140001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ZoomProvider.java (right): https://codereview.chromium.org/1288243004/diff/140001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ZoomProvider.java:10: public interface ZoomProvider { Why is this interface required? https://codereview.chromium.org/1288243004/diff/140001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java (right): https://codereview.chromium.org/1288243004/diff/140001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:92: assertWaitForPageScaleFactorMatch(2.0f); Do we still need to wait here?
Have made modification as per previous comments. Please review. https://codereview.chromium.org/1288243004/diff/140001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java (right): https://codereview.chromium.org/1288243004/diff/140001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:92: assertWaitForPageScaleFactorMatch(2.0f); On 2016/02/24 17:02:31, tdresser wrote: > Do we still need to wait here? Yes, content view core takes time to zoom page. If we don't wait and check value at that instant, we end up in assert failure.
This LGTM. aelias@, can you take another look? https://codereview.chromium.org/1288243004/diff/160001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java (right): https://codereview.chromium.org/1288243004/diff/160001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:121: // Zoom page to max size Add period.
aelias@chromium.org changed reviewers: + yfriedman@chromium.org
lgtm, adding yfriedman@ for ContentViewCore OWNERS.
On 2016/03/08 02:19:00, aelias wrote: > lgtm, adding yfriedman@ for ContentViewCore OWNERS. Rebased + fixed previous comment. yfriedman@ PTAL.
https://codereview.chromium.org/1288243004/diff/180001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1288243004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:354: private class SystemAnimationIntervalProvider implements AnimationIntervalProvider { static. also add comment https://codereview.chromium.org/1288243004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2749: public boolean pinchBegin(int xPix, int yPix) { These should have brief comments. https://codereview.chromium.org/1288243004/diff/180001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java (right): https://codereview.chromium.org/1288243004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. 2016 https://codereview.chromium.org/1288243004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:14: * Page Zoomin is propertional to RTRIGGER axis movement. proportional ? (change throughout) https://codereview.chromium.org/1288243004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:30: protected final ContentViewCore mContentView; mContentViewCore. ContentView is something else so it's confusing https://codereview.chromium.org/1288243004/diff/180001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java (right): https://codereview.chromium.org/1288243004/diff/180001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:64: private MotionEvent simulateJoystickEvent(final float delta, final boolean isZoomInRequest) { These events are still sent from the test thread - should they be posted to ui thread too? I was wondering about making the test a @UiThreadTest to avoid all this but I think assertWaitForPageScaleFactorMatch might fail
Fixed previous comment. PTAL https://codereview.chromium.org/1288243004/diff/180001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/1288243004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:354: private class SystemAnimationIntervalProvider implements AnimationIntervalProvider { On 2016/03/08 14:50:53, Yaron wrote: > static. also add comment Done. https://codereview.chromium.org/1288243004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2749: public boolean pinchBegin(int xPix, int yPix) { On 2016/03/08 14:50:53, Yaron wrote: > These should have brief comments. Done. https://codereview.chromium.org/1288243004/diff/180001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java (right): https://codereview.chromium.org/1288243004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/03/08 14:50:53, Yaron wrote: > 2016 Done. https://codereview.chromium.org/1288243004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:14: * Page Zoomin is propertional to RTRIGGER axis movement. On 2016/03/08 14:50:53, Yaron wrote: > proportional ? (change throughout) Done. https://codereview.chromium.org/1288243004/diff/180001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/JoystickZoomProvider.java:30: protected final ContentViewCore mContentView; On 2016/03/08 14:50:53, Yaron wrote: > mContentViewCore. ContentView is something else so it's confusing Done. https://codereview.chromium.org/1288243004/diff/180001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java (right): https://codereview.chromium.org/1288243004/diff/180001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:64: private MotionEvent simulateJoystickEvent(final float delta, final boolean isZoomInRequest) { >> should they be posted to ui thread too? In test logic, joystick event is send to JoystickZoomProvider by calling animationZoomTest method directly. Event is not passed to contentview core events queue. What testing benefits will be gained by passing event to ui thread? Could you please elaborate? Basically, this test is written on similar lines of ContentViewScrollingTest.
lgtm https://codereview.chromium.org/1288243004/diff/180001/content/public/android... File content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java (right): https://codereview.chromium.org/1288243004/diff/180001/content/public/android... content/public/android/javatests/src/org/chromium/content/browser/ContentViewZoomingTest.java:64: private MotionEvent simulateJoystickEvent(final float delta, final boolean isZoomInRequest) { On 2016/03/10 10:06:45, sshelke wrote: > >> should they be posted to ui thread too? > In test logic, joystick event is send to JoystickZoomProvider > by calling animationZoomTest method directly. Event is not > passed to contentview core events queue. What testing benefits > will be gained by passing event to ui thread? Could you please > elaborate? Basically, this test is written on similar lines of > ContentViewScrollingTest. OH I mis-read this, sorry. I see that this is just constructing the event and it's later posted to the UI thread. The spirit of my request was that this represents same threading model as prod code which it does so it's fine.
The CQ bit was checked by sshelke@nvidia.com
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/1288243004/#ps200001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288243004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288243004/200001
Message was sent while issue was closed.
Description was changed from ========== Enable zoom through gamedpad trigger joystick This change allows page zoomin/out using trigger joysticks. Page zoomin and zoomout can be achieved using left and right trigger joysticks respectively. BUG=454355 ========== to ========== Enable zoom through gamedpad trigger joystick This change allows page zoomin/out using trigger joysticks. Page zoomin and zoomout can be achieved using left and right trigger joysticks respectively. BUG=454355 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Enable zoom through gamedpad trigger joystick This change allows page zoomin/out using trigger joysticks. Page zoomin and zoomout can be achieved using left and right trigger joysticks respectively. BUG=454355 ========== to ========== Enable zoom through gamedpad trigger joystick This change allows page zoomin/out using trigger joysticks. Page zoomin and zoomout can be achieved using left and right trigger joysticks respectively. BUG=454355 Committed: https://crrev.com/84583b7d70d5bf5adac05346811b5422033c4e86 Cr-Commit-Position: refs/heads/master@{#380572} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/84583b7d70d5bf5adac05346811b5422033c4e86 Cr-Commit-Position: refs/heads/master@{#380572}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/1790493002/ by maybelle@chromium.org. The reason for reverting is: FindBugs issues on ContentViewZoomingTest.java Builder: clang-clankium-tot-builder FindBugs reported the following issues: SIC_INNER_SHOULD_BE_STATIC: Should be a static inner class In class org.chromium.content.browser.ContentViewZoomingTest$TestAnimationIntervalProvider At ContentViewZoomingTest.java:[lines 29-34].
Message was sent while issue was closed.
On 2016/03/11 11:45:59, May wrote: > A revert of this CL (patchset #11 id:200001) has been created in > https://codereview.chromium.org/1790493002/ by mailto:maybelle@chromium.org. > > The reason for reverting is: FindBugs issues on ContentViewZoomingTest.java > > Builder: clang-clankium-tot-builder > > FindBugs reported the following issues: > SIC_INNER_SHOULD_BE_STATIC: Should be a static inner class > In class > org.chromium.content.browser.ContentViewZoomingTest$TestAnimationIntervalProvider > At ContentViewZoomingTest.java:[lines 29-34]. Added static keyword to TestAnimationIntervalProvider inner class. PTAL
Message was sent while issue was closed.
lgtm
Message was sent while issue was closed.
Description was changed from ========== Enable zoom through gamedpad trigger joystick This change allows page zoomin/out using trigger joysticks. Page zoomin and zoomout can be achieved using left and right trigger joysticks respectively. BUG=454355 Committed: https://crrev.com/84583b7d70d5bf5adac05346811b5422033c4e86 Cr-Commit-Position: refs/heads/master@{#380572} ========== to ========== Enable zoom through gamedpad trigger joystick This change allows page zoomin/out using trigger joysticks. Page zoomin and zoomout can be achieved using left and right trigger joysticks respectively. BUG=454355 Committed: https://crrev.com/84583b7d70d5bf5adac05346811b5422033c4e86 Cr-Commit-Position: refs/heads/master@{#380572} ==========
The CQ bit was checked by sshelke@nvidia.com
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, tdresser@chromium.org Link to the patchset: https://codereview.chromium.org/1288243004/#ps220001 (title: "fixed static class error")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1288243004/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1288243004/220001
Message was sent while issue was closed.
Description was changed from ========== Enable zoom through gamedpad trigger joystick This change allows page zoomin/out using trigger joysticks. Page zoomin and zoomout can be achieved using left and right trigger joysticks respectively. BUG=454355 Committed: https://crrev.com/84583b7d70d5bf5adac05346811b5422033c4e86 Cr-Commit-Position: refs/heads/master@{#380572} ========== to ========== Enable zoom through gamedpad trigger joystick This change allows page zoomin/out using trigger joysticks. Page zoomin and zoomout can be achieved using left and right trigger joysticks respectively. BUG=454355 Committed: https://crrev.com/84583b7d70d5bf5adac05346811b5422033c4e86 Cr-Commit-Position: refs/heads/master@{#380572} ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Enable zoom through gamedpad trigger joystick This change allows page zoomin/out using trigger joysticks. Page zoomin and zoomout can be achieved using left and right trigger joysticks respectively. BUG=454355 Committed: https://crrev.com/84583b7d70d5bf5adac05346811b5422033c4e86 Cr-Commit-Position: refs/heads/master@{#380572} ========== to ========== Enable zoom through gamedpad trigger joystick This change allows page zoomin/out using trigger joysticks. Page zoomin and zoomout can be achieved using left and right trigger joysticks respectively. BUG=454355 Committed: https://crrev.com/84583b7d70d5bf5adac05346811b5422033c4e86 Cr-Commit-Position: refs/heads/master@{#380572} Committed: https://crrev.com/386000b5edda92212e1a0835c58acb4a59fe8378 Cr-Commit-Position: refs/heads/master@{#381188} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/386000b5edda92212e1a0835c58acb4a59fe8378 Cr-Commit-Position: refs/heads/master@{#381188} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
