Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2710683003:
[Android] Add switch to disable native event batching (Closed)
Description[Android] Add switch to disable native event batching
This CL is the preparation for enabling compositor event queue.
We should disable Android native event batching once the compositor event queue
was enabled.
BUG=625689
Review-Url: https://codereview.chromium.org/2710683003
Cr-Commit-Position: refs/heads/master@{#452298}
Committed: https://chromium.googlesource.com/chromium/src/+/846acfd108a39beb4f9087d1daf0c00eb5aae6ea
Patch Set 1 #
Total comments: 4
Patch Set 2 : Stash the switch and move inside ACTION_DOWN block #
Total comments: 5
Patch Set 3 : aelias's review: Use @TargetApi and change switch name #
Total comments: 1
Messages
Total messages: 30 (20 generated)
The CQ bit was checked by chongz@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: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Description was changed from ========== [Android] Add switch to disable native event batching BUG=625689 ========== to ========== [Android] Add switch to disable native event batching This CL is the preparation for enabling compositor event queue. We should disable Android native event batching once the compositor event queue was enabled. BUG=625689 ==========
chongz@chromium.org changed reviewers: + yfriedman@chromium.org
yfriedman@ PTAL, thanks!
dtapuska@chromium.org changed reviewers: + dtapuska@chromium.org
https://codereview.chromium.org/2710683003/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2710683003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:944: && CommandLine.getInstance().hasSwitch(ContentSwitches.DISABLE_EVENT_BATCHING)) { I'm not really sure we want to fetch the switch every motion event. Can we not stash this somewhere?
agrieve@chromium.org changed reviewers: + agrieve@chromium.org
random fly-by https://codereview.chromium.org/2710683003/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2710683003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:945: mContainerView.requestUnbufferedDispatch(event); nit: We might also save the call to it entirely by putting it within sendTouchEvent()'s (eventAction == MotionEvent.ACTION_DOWN) block. Looking at the implementation, it doesn't apply to mouse events anyways: https://cs.chromium.org/chromium/src/third_party/android_tools/sdk/sources/an...
yfriedman@chromium.org changed reviewers: + aelias@chromium.org - yfriedman@chromium.org
I don't think I'm the best reviewer for this. yfriedman->aelias
The CQ bit was checked by chongz@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.
aelias@ PTAL, thanks! https://codereview.chromium.org/2710683003/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2710683003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:944: && CommandLine.getInstance().hasSwitch(ContentSwitches.DISABLE_EVENT_BATCHING)) { On 2017/02/22 14:57:21, dtapuska wrote: > I'm not really sure we want to fetch the switch every motion event. Can we not > stash this somewhere? Added variable |mShouldRequestUnbufferedDispatch|. https://codereview.chromium.org/2710683003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:945: mContainerView.requestUnbufferedDispatch(event); On 2017/02/22 15:15:30, agrieve wrote: > nit: We might also save the call to it entirely by putting it within > sendTouchEvent()'s (eventAction == MotionEvent.ACTION_DOWN) block. Looking at > the implementation, it doesn't apply to mouse events anyways: > https://cs.chromium.org/chromium/src/third_party/android_tools/sdk/sources/an... Done. https://codereview.chromium.org/2710683003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2710683003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1001: @SuppressLint("NewApi") I'm not sure what's the proper way of accessing new APIs, do I simply suppress the lint or should I add some config?
https://codereview.chromium.org/2710683003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2710683003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1001: @SuppressLint("NewApi") On 2017/02/22 at 21:36:15, chongz wrote: > I'm not sure what's the proper way of accessing new APIs, do I simply suppress the lint or should I add some config? It's @TargetApi. I suggest placing it in a small private method to clarify what is in the API scope: @TargetApi(Build.VERSION_CODES.L) private void requestUnbufferedDispatch(MotionEvent touchDownEvent) { mContainerView.requestUnbufferedDispatch(touchDownEvent); } https://codereview.chromium.org/2710683003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/common/ContentSwitches.java (right): https://codereview.chromium.org/2710683003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/common/ContentSwitches.java:82: public static final String DISABLE_EVENT_BATCHING = "disable-event-batching"; Please call this "request-unbuffered-dispatch".
The CQ bit was checked by chongz@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...
aelias@ I've updated as per your comments, PTAL again, thanks! https://codereview.chromium.org/2710683003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2710683003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1001: @SuppressLint("NewApi") On 2017/02/22 22:40:51, aelias wrote: > On 2017/02/22 at 21:36:15, chongz wrote: > > I'm not sure what's the proper way of accessing new APIs, do I simply suppress > the lint or should I add some config? > > It's @TargetApi. I suggest placing it in a small private method to clarify what > is in the API scope: > > @TargetApi(Build.VERSION_CODES.L) > private void requestUnbufferedDispatch(MotionEvent touchDownEvent) { > mContainerView.requestUnbufferedDispatch(touchDownEvent); > } Done. https://codereview.chromium.org/2710683003/diff/20001/content/public/android/... File content/public/android/java/src/org/chromium/content/common/ContentSwitches.java (right): https://codereview.chromium.org/2710683003/diff/20001/content/public/android/... content/public/android/java/src/org/chromium/content/common/ContentSwitches.java:82: public static final String DISABLE_EVENT_BATCHING = "disable-event-batching"; On 2017/02/22 22:40:51, aelias wrote: > Please call this "request-unbuffered-dispatch". Done. https://codereview.chromium.org/2710683003/diff/40001/content/public/android/... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2710683003/diff/40001/content/public/android/... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:2471: @TargetApi(Build.VERSION_CODES.LOLLIPOP) No idea why but "Build.VERSION_CODES.L" won't work...
lgtm
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 chongz@chromium.org
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": 40001, "attempt_start_ts": 1487808561740590,
"parent_rev": "841b84cb8f7eeb783b22af63a2c9f317d5b24691", "commit_rev":
"846acfd108a39beb4f9087d1daf0c00eb5aae6ea"}
Message was sent while issue was closed.
Description was changed from ========== [Android] Add switch to disable native event batching This CL is the preparation for enabling compositor event queue. We should disable Android native event batching once the compositor event queue was enabled. BUG=625689 ========== to ========== [Android] Add switch to disable native event batching This CL is the preparation for enabling compositor event queue. We should disable Android native event batching once the compositor event queue was enabled. BUG=625689 Review-Url: https://codereview.chromium.org/2710683003 Cr-Commit-Position: refs/heads/master@{#452298} Committed: https://chromium.googlesource.com/chromium/src/+/846acfd108a39beb4f9087d1daf0... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/846acfd108a39beb4f9087d1daf0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
