Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2548363007:
Emulate Android joystick scroll with synthetic mouse wheel event (Closed)
|
Created:
4 years ago by Tima Vaisburd Modified:
4 years ago CC:
chromium-reviews, jam, darin-cc_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionEmulate Android joystick scroll with synthetic mouse wheel event
The JoystickScrollProvider Java class used to emulate a View
scroll events and call ContentViewCore.scrollBy(), now it
emulates the mouse wheel events directly, dropping the calculation
of the current event position (it is set to (0,0)).
This CL also removes JoystickScrollProvider dependency on
ContentViewCore, it talks to its own native counterpart.
BUG=620929, 650351
Committed: https://crrev.com/4537fa801ee646b95ef496083e824e468978a374
Cr-Commit-Position: refs/heads/master@{#437826}
Patch Set 1 #
Total comments: 30
Patch Set 2 : Native side and ContentViewCore.java cleanup #
Total comments: 3
Patch Set 3 : Rebase only #Patch Set 4 : Turned JoystickScrollProvider into independent DisplayObserver #
Total comments: 5
Patch Set 5 : Addressed comments #Patch Set 6 : Fixed tests #Messages
Total messages: 35 (15 generated)
Description was changed from ========== Emulate Android joystick scroll with synthetic mouse wheel event The JoystickScrollProvider Java class used to emulate a View scroll events and call ContentViewCore.scrollBy(), now it emulates the mouse wheel events directly, dropping the calculation of the current event position (it is set to (0,0)). This CL also removes JoystickScrollProvider dependency on ContentViewCore, it talks to its own native counterpart. BUG=620929 ========== to ========== Emulate Android joystick scroll with synthetic mouse wheel event The JoystickScrollProvider Java class used to emulate a View scroll events and call ContentViewCore.scrollBy(), now it emulates the mouse wheel events directly, dropping the calculation of the current event position (it is set to (0,0)). This CL also removes JoystickScrollProvider dependency on ContentViewCore, it talks to its own native counterpart. BUG=620929, 650351 ==========
timav@chromium.org changed reviewers: + aelias@chromium.org, boliu@chromium.org
As discussed offline with Alex I made the JoystickScrollProvider emulate synthetic mouse events through it's own native pair class. It was also possible to do this in the ContentViewCore. It would be much smaller change, but in light of the planned ContentViewCore removal I decided to make the bigger job and let you comment on the approach. The initialization and destruction of the native sister class was copied from ContentViewCore. If this approach is correct I will delete the now-unused flag in ContentViewCore.scrollBy() and variables. PTAL.
https://codereview.chromium.org/2548363007/diff/1/content/browser/android/joy... File content/browser/android/joystick_scroll_provider.cc (right): https://codereview.chromium.org/2548363007/diff/1/content/browser/android/joy... content/browser/android/joystick_scroll_provider.cc:50: DCHECK(!web_contents_->GetUserData(kJoystickScrollUserDataKey)); move this DCHECK to Init conceptually, this is too late to handle this failure, the new object is already created https://codereview.chromium.org/2548363007/diff/1/content/browser/android/joy... File content/browser/android/joystick_scroll_provider.h (right): https://codereview.chromium.org/2548363007/diff/1/content/browser/android/joy... content/browser/android/joystick_scroll_provider.h:10: #include "content/public/browser/web_contents_observer.h" not needed? https://codereview.chromium.org/2548363007/diff/1/content/browser/android/joy... content/browser/android/joystick_scroll_provider.h:21: WebContents* web_contents); use the impl type, then remove the extra forward declare https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:55: public void initialize(WebContents webContents, View containerView) { just pass in everything in the constructor? having a separate init method doesn't feel necessary here yes the ref won't be final anymore in CVC, but doesn't matter much I think https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:56: Log.v(TAG, "initialize"); remove https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:77: public void setDeviceScaleFactor(float dipScale, WeakReference<Context> displayContext) { package you could remove further dependency on CVC by giving WindowAndroid here, and then directly observe the display that way https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:129: mLastAnimateTimeMillis = SystemClock.uptimeMillis(); why switch this? should never tick animations using the system clock https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:138: final long timeMillis = SystemClock.uptimeMillis(); ditto https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:144: nativeScrollBy( new code isn't exactly the same as calling scrollBy on the view, where in webview the app could actually override the method and do something else. why is that ok?
https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:129: mLastAnimateTimeMillis = SystemClock.uptimeMillis(); On 2016/12/07 06:06:44, boliu wrote: > why switch this? should never tick animations using the system clock I made it do be consistent with getEventTime() which returns uptimeMillis(): https://developer.android.com/reference/android/view/MotionEvent.html#getEven... As far as I got we are not doing animations, we just post an event to UI thread and take the time it arrives? https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:144: nativeScrollBy( On 2016/12/07 06:06:44, boliu wrote: > new code isn't exactly the same as calling scrollBy on the view, where in > webview the app could actually override the method and do something else. why is > that ok? Right away I can give you a shallow answer that in the new code the scrolling with joystick seems no different than the scrolling with mouse wheel. In CVC.onGenericMotionEvent() we send the same mouse event in react to ACTION_SCROLL.
https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:129: mLastAnimateTimeMillis = SystemClock.uptimeMillis(); On 2016/12/07 18:14:40, Tima Vaisburd wrote: > On 2016/12/07 06:06:44, boliu wrote: > > why switch this? should never tick animations using the system clock > > I made it do be consistent with getEventTime() which returns uptimeMillis(): > https://developer.android.com/reference/android/view/MotionEvent.html#getEven... > > As far as I got we are not doing animations, we just post an event to UI thread > and take the time it arrives? You are implementing a smooth scroll animation here, no? Using system clock for animation is generally not ok, since the time may drift within the frame, causing "non-smooth" appearance for animations. If getEventTime is using the system clock, then that should not be used for animations. Fwiw, android makes the same mistake all the time. The scroller thing most views use to implement the fling animation was using the system clock, and was only fixed in N. https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:144: nativeScrollBy( On 2016/12/07 18:14:40, Tima Vaisburd wrote: > On 2016/12/07 06:06:44, boliu wrote: > > new code isn't exactly the same as calling scrollBy on the view, where in > > webview the app could actually override the method and do something else. why > is > > that ok? > > Right away I can give you a shallow answer that in the new code the scrolling > with joystick seems no different than the scrolling with mouse wheel. In > CVC.onGenericMotionEvent() we send the same mouse event in react to > ACTION_SCROLL. That did not answer my question at all. You just restated what I said. I mean if the belief is no app is using this, and since we have no way to measure usage in webview, then sure, let's break and see if anyone complains. But is that the thought here?
https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:144: nativeScrollBy( On 2016/12/07 18:23:21, boliu wrote: > That did not answer my question at all. You just restated what I said. I said it was shallow. > I mean if the belief is no app is using this, and since we have no way to > measure usage in webview, then sure, let's break and see if anyone complains. > But is that the thought here? No, honestly I did not see the problem. Still, I do not see how the WebView subclassing matters in the original code. When a third party app writer subclasses WebView.scrollBy() (e.g. blocks it), does the original code still calls CVC.scrollBy(x, y, true) upon Joystick operation? I think yes. I also think we do not call the subclassed scrollBy() method from CVC anyhow?
https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:144: nativeScrollBy( On 2016/12/07 18:54:44, Tima Vaisburd wrote: > On 2016/12/07 18:23:21, boliu wrote: > > That did not answer my question at all. You just restated what I said. > > I said it was shallow. > > > I mean if the belief is no app is using this, and since we have no way to > > measure usage in webview, then sure, let's break and see if anyone complains. > > But is that the thought here? > > No, honestly I did not see the problem. Still, I do not see how the WebView > subclassing matters in the original code. > When a third party app writer subclasses WebView.scrollBy() (e.g. blocks it), > does the original code still calls CVC.scrollBy(x, y, true) upon Joystick > operation? I think yes. Not necessarily. Depends on what the subclass implementation of scrollBy does. If app made it no-op, then no, CVC.scrollBy won't be called. I'm not saying there are any apps that actually do this, or that apps should be allowed to do this. But it is a change in behavior, and I'm asking what is your reasoning why that's ok. > I also think we do not call the subclassed scrollBy() > method from CVC anyhow?
https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:144: nativeScrollBy( On 2016/12/07 19:05:01, boliu wrote: > Not necessarily. Depends on what the subclass implementation of scrollBy does. > If app made it no-op, then no, CVC.scrollBy won't be called. Exactly my point, how the subclassing (blocking) of WebView.scrollBy() will prevent the original call to mView.scrollBy(dx, dy, true) on l.103? It seems onMotion() will still be called, it comes from CVC.onGenericMotionEvent()? Will they need to also subclass and block WebView.onGenenericMotionEvent()? > I'm not saying there are any apps that actually do this, or that apps should be > allowed to do this. But it is a change in behavior, and I'm asking what is your > reasoning why that's ok. After I understand how it is possible in the old code I would be able to say that I missed that important case.
https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:144: nativeScrollBy( On 2016/12/07 19:29:57, Tima Vaisburd wrote: > On 2016/12/07 19:05:01, boliu wrote: > > Not necessarily. Depends on what the subclass implementation of scrollBy does. > > If app made it no-op, then no, CVC.scrollBy won't be called. > > Exactly my point, how the subclassing (blocking) of WebView.scrollBy() will > prevent the original call to mView.scrollBy(dx, dy, true) on l.103? It won't. But since scrollBy is no-op, it will not propagate the scroll any further, ie app intentionally do *not* want to scroll. After this change, those apps may start seeing unwanted scrolls. > It seems > onMotion() will still be called, it comes from CVC.onGenericMotionEvent()? Will > they need to also subclass and block WebView.onGenenericMotionEvent()? > > > I'm not saying there are any apps that actually do this, or that apps should > be > > allowed to do this. But it is a change in behavior, and I'm asking what is > your > > reasoning why that's ok. > > After I understand how it is possible in the old code I would be able to say > that I missed that important case.
https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:144: nativeScrollBy( On 2016/12/07 19:58:30, boliu wrote: > On 2016/12/07 19:29:57, Tima Vaisburd wrote: > > Exactly my point, how the subclassing (blocking) of WebView.scrollBy() will > > prevent the original call to mView.scrollBy(dx, dy, true) on l.103? > > It won't. But since scrollBy is no-op, it will not propagate the scroll any > further, ie app intentionally do *not* want to scroll. > > After this change, those apps may start seeing unwanted scrolls. But I thought *that* scrollBy() (i.e. CVC.scrollBy()) is past the user's block? In other words, the WebView.scrollBy subclassing does not stand in the chain onMotion() -> CVC.scrollBy() -> CVC.nativeScrollBegin()? Am I right that CVC.scrollBy(float, float, boolean) is never subclassed, only a glue method which calls CVC.scrollBy can be subclassed?
https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:144: nativeScrollBy( On 2016/12/07 20:56:03, Tima Vaisburd wrote: > On 2016/12/07 19:58:30, boliu wrote: > > On 2016/12/07 19:29:57, Tima Vaisburd wrote: > > > Exactly my point, how the subclassing (blocking) of WebView.scrollBy() will > > > prevent the original call to mView.scrollBy(dx, dy, true) on l.103? > > > > It won't. But since scrollBy is no-op, it will not propagate the scroll any > > further, ie app intentionally do *not* want to scroll. > > > > After this change, those apps may start seeing unwanted scrolls. > > But I thought *that* scrollBy() (i.e. CVC.scrollBy()) is past the user's block? > In other words, the WebView.scrollBy subclassing does not stand in the chain > onMotion() -> CVC.scrollBy() -> CVC.nativeScrollBegin()? > > Am I right that CVC.scrollBy(float, float, boolean) is never subclassed, only a > glue method which calls CVC.scrollBy can be subclassed? My bad. mView on the left is ContentViewCore, not container view. Burrrrr bad variable names.
https://codereview.chromium.org/2548363007/diff/1/content/browser/android/joy... File content/browser/android/joystick_scroll_provider.cc (right): https://codereview.chromium.org/2548363007/diff/1/content/browser/android/joy... content/browser/android/joystick_scroll_provider.cc:50: DCHECK(!web_contents_->GetUserData(kJoystickScrollUserDataKey)); On 2016/12/07 06:06:43, boliu wrote: > move this DCHECK to Init > > conceptually, this is too late to handle this failure, the new object is already > created Done. https://codereview.chromium.org/2548363007/diff/1/content/browser/android/joy... File content/browser/android/joystick_scroll_provider.h (right): https://codereview.chromium.org/2548363007/diff/1/content/browser/android/joy... content/browser/android/joystick_scroll_provider.h:10: #include "content/public/browser/web_contents_observer.h" On 2016/12/07 06:06:43, boliu wrote: > not needed? Removed https://codereview.chromium.org/2548363007/diff/1/content/browser/android/joy... content/browser/android/joystick_scroll_provider.h:21: WebContents* web_contents); On 2016/12/07 06:06:43, boliu wrote: > use the impl type, then remove the extra forward declare Done. https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:55: public void initialize(WebContents webContents, View containerView) { On 2016/12/07 06:06:44, boliu wrote: > just pass in everything in the constructor? having a separate init method > doesn't feel necessary here > > yes the ref won't be final anymore in CVC, but doesn't matter much I think Done, I had to add the check in CVC that |mJoystickScrollProvider| is not null everywhere though. https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:56: Log.v(TAG, "initialize"); On 2016/12/07 06:06:44, boliu wrote: > remove Done. https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:129: mLastAnimateTimeMillis = SystemClock.uptimeMillis(); On 2016/12/07 18:23:21, boliu wrote: > On 2016/12/07 18:14:40, Tima Vaisburd wrote: > > On 2016/12/07 06:06:44, boliu wrote: > > > why switch this? should never tick animations using the system clock > > > > I made it do be consistent with getEventTime() which returns uptimeMillis(): > > > https://developer.android.com/reference/android/view/MotionEvent.html#getEven... > > > > As far as I got we are not doing animations, we just post an event to UI > thread > > and take the time it arrives? > > You are implementing a smooth scroll animation here, no? Using system clock for > animation is generally not ok, since the time may drift within the frame, > causing "non-smooth" appearance for animations. > > If getEventTime is using the system clock, then that should not be used for > animations. > > Fwiw, android makes the same mistake all the time. The scroller thing most views > use to implement the fling animation was using the system clock, and was only > fixed in N. I put the animation clock back. https://codereview.chromium.org/2548363007/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (left): https://codereview.chromium.org/2548363007/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1716: public void scrollBy(float dxPix, float dyPix, boolean useLastFocalEventLocation) { Removed now unused third parameter and variables that supported it.
https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:55: public void initialize(WebContents webContents, View containerView) { On 2016/12/08 00:35:48, Tima Vaisburd wrote: > On 2016/12/07 06:06:44, boliu wrote: > > just pass in everything in the constructor? having a separate init method > > doesn't feel necessary here > > > > yes the ref won't be final anymore in CVC, but doesn't matter much I think > > Done, I had to add the check in CVC that |mJoystickScrollProvider| is not null > everywhere though. you don't have to, CVC isn't considered initialized until initialize is called, CVC.initialize should be the first thing after constructor https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:77: public void setDeviceScaleFactor(float dipScale, WeakReference<Context> displayContext) { On 2016/12/07 06:06:44, boliu wrote: > package > > you could remove further dependency on CVC by giving WindowAndroid here, and > then directly observe the display that way not done? at least make it package visible https://codereview.chromium.org/2548363007/diff/20001/content/browser/android... File content/browser/android/joystick_scroll_provider.cc (right): https://codereview.chromium.org/2548363007/diff/20001/content/browser/android... content/browser/android/joystick_scroll_provider.cc:83: dx_dip, dy_dip, 1.0, time_ms / 1000.0, 0, 0); too much implicit casting here, long/float/double, I don't even know what the compiler is doing maybe declare 1000.0 as a double constant, or do a static cast
https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:77: public void setDeviceScaleFactor(float dipScale, WeakReference<Context> displayContext) { On 2016/12/08 06:03:09, boliu wrote: > On 2016/12/07 06:06:44, boliu wrote: > > package > > > > you could remove further dependency on CVC by giving WindowAndroid here, and > > then directly observe the display that way > > not done? at least make it package visible Not yet. I want to try before giving up though. Package scope: this file is in browser.input package and CVC is just in browser, so it has to be public, otherwise it did not compile (I checked).
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:55: public void initialize(WebContents webContents, View containerView) { On 2016/12/08 06:03:09, boliu wrote: > On 2016/12/08 00:35:48, Tima Vaisburd wrote: > > On 2016/12/07 06:06:44, boliu wrote: > > > just pass in everything in the constructor? having a separate init method > > > doesn't feel necessary here > > > > > > yes the ref won't be final anymore in CVC, but doesn't matter much I think > > > > Done, I had to add the check in CVC that |mJoystickScrollProvider| is not null > > everywhere though. > > you don't have to, CVC isn't considered initialized until initialize is called, > CVC.initialize should be the first thing after constructor Ok, removed these checks! https://codereview.chromium.org/2548363007/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:77: public void setDeviceScaleFactor(float dipScale, WeakReference<Context> displayContext) { On 2016/12/08 18:23:20, Tima Vaisburd wrote: > On 2016/12/08 06:03:09, boliu wrote: > > On 2016/12/07 06:06:44, boliu wrote: > > > package > > > > > > you could remove further dependency on CVC by giving WindowAndroid here, and > > > then directly observe the display that way > > > > not done? at least make it package visible > > Not yet. I want to try before giving up though. Package scope: this file is in > browser.input package and CVC is just in browser, so it has to be public, > otherwise it did not compile (I checked). JoystickScrollProvider subclasses DisplayObserver in PS4. The essential code is copied from ContentViewCore. Please take another look. https://codereview.chromium.org/2548363007/diff/20001/content/browser/android... File content/browser/android/joystick_scroll_provider.cc (right): https://codereview.chromium.org/2548363007/diff/20001/content/browser/android... content/browser/android/joystick_scroll_provider.cc:83: dx_dip, dy_dip, 1.0, time_ms / 1000.0, 0, 0); On 2016/12/08 06:03:09, boliu wrote: > too much implicit casting here, long/float/double, I don't even know what the > compiler is doing > > maybe declare 1000.0 as a double constant, or do a static cast Declared MILLISECONDS_IN_SECOND for 1000.0 https://codereview.chromium.org/2548363007/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/2548363007/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:33: updateScrollFactor(); This rewrite make it easier, I think, to see the same problem that we had with PopupTouchHandleDrawable: updateScrollFactor() uses theme that might get updated as dip scale change, but it might happen later, durig onConfigurationChanged() phase.
https://codereview.chromium.org/2548363007/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/2548363007/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:28: public void onRotationChanged(int rotation) {} hmm, use a private inner class? don't want to expose these methods to the world also style-wise, definitely move these down below the member variable declarations https://codereview.chromium.org/2548363007/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:86: public void onViewAttachedToWindow() { this pattern for watching displays is probably going to be wide-spread, so worthwhile to factor this out into a common base class in the future, not in this CL though
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2548363007/diff/60001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java (right): https://codereview.chromium.org/2548363007/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:28: public void onRotationChanged(int rotation) {} On 2016/12/08 21:42:24, boliu wrote: > hmm, use a private inner class? don't want to expose these methods to the world Done > also style-wise, definitely move these down below the member variable > declarations I put the inner class between constants and variables. https://codereview.chromium.org/2548363007/diff/60001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/input/JoystickScrollProvider.java:86: public void onViewAttachedToWindow() { On 2016/12/08 21:42:24, boliu wrote: > this pattern for watching displays is probably going to be wide-spread, so > worthwhile to factor this out into a common base class in the future, not in > this CL though Acknowledged.
lgtm
The CQ bit was checked by timav@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by timav@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org Link to the patchset: https://codereview.chromium.org/2548363007/#ps100001 (title: "Fixed tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481522045856380,
"parent_rev": "8ec69b8877948ec8dc4e8ad667090d91ae97ef01", "commit_rev":
"37e42bb282e43be1f1dd7b4966b6b234dcce7c4e"}
Message was sent while issue was closed.
Description was changed from ========== Emulate Android joystick scroll with synthetic mouse wheel event The JoystickScrollProvider Java class used to emulate a View scroll events and call ContentViewCore.scrollBy(), now it emulates the mouse wheel events directly, dropping the calculation of the current event position (it is set to (0,0)). This CL also removes JoystickScrollProvider dependency on ContentViewCore, it talks to its own native counterpart. BUG=620929, 650351 ========== to ========== Emulate Android joystick scroll with synthetic mouse wheel event The JoystickScrollProvider Java class used to emulate a View scroll events and call ContentViewCore.scrollBy(), now it emulates the mouse wheel events directly, dropping the calculation of the current event position (it is set to (0,0)). This CL also removes JoystickScrollProvider dependency on ContentViewCore, it talks to its own native counterpart. BUG=620929, 650351 Review-Url: https://codereview.chromium.org/2548363007 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Emulate Android joystick scroll with synthetic mouse wheel event The JoystickScrollProvider Java class used to emulate a View scroll events and call ContentViewCore.scrollBy(), now it emulates the mouse wheel events directly, dropping the calculation of the current event position (it is set to (0,0)). This CL also removes JoystickScrollProvider dependency on ContentViewCore, it talks to its own native counterpart. BUG=620929, 650351 Review-Url: https://codereview.chromium.org/2548363007 ========== to ========== Emulate Android joystick scroll with synthetic mouse wheel event The JoystickScrollProvider Java class used to emulate a View scroll events and call ContentViewCore.scrollBy(), now it emulates the mouse wheel events directly, dropping the calculation of the current event position (it is set to (0,0)). This CL also removes JoystickScrollProvider dependency on ContentViewCore, it talks to its own native counterpart. BUG=620929, 650351 Committed: https://crrev.com/4537fa801ee646b95ef496083e824e468978a374 Cr-Commit-Position: refs/heads/master@{#437826} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/4537fa801ee646b95ef496083e824e468978a374 Cr-Commit-Position: refs/heads/master@{#437826} |
