Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2942903002:
Remove AreCrossProcessFramesPossible on Android (Closed)
DescriptionMake AreCrossProcessFramesPossible return true on Android
The function AreCrossProcessFramePossible returns true on all platforms
but Android, because of some memory regressions that were previously
observed. The regressions do not currently manifest on perf trybots, so
this is another attempt to make the change on Android, in the hope that
the underlying problem has since been resolved.
This also includes some changes to gesture event routing on touchscreen
devices that are necessary to fix test breakage when input event
routing for Android is always turned on. We now route gesture events
even if there have been no corresponding touch events preceding them.
If no regressions are detected, the next step will be to remove the function from the code base.
BUG=690229
Review-Url: https://codereview.chromium.org/2942903002
Cr-Commit-Position: refs/heads/master@{#482333}
Committed: https://chromium.googlesource.com/chromium/src/+/00d432064a6fcd6252211a938840efb8e36b463f
Patch Set 1 #Patch Set 2 : Comment removed. #
Total comments: 2
Patch Set 3 : Speculative fix for trybot problem #Patch Set 4 : Maybe fix red bots for realz #Patch Set 5 : Another take on gesture improvements #Patch Set 6 : rebase only #Patch Set 7 : Removed an extra header #
Total comments: 2
Patch Set 8 : Removed unnecessary call #
Messages
Total messages: 42 (32 generated)
The CQ bit was checked by kenrb@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...
Description was changed from ========== [WIP] Remove AreCrossProcessFramesPossible on Android BUG=690229 ========== to ========== Make AreCrossProcessFramesPossible return true on Android The function AreCrossProcessFramePossible returns true on all platforms but Android, because of some memory regressions that were previously observed. The regressions do not currently manifest on perf trybots, so this is another attempt to make the change on Android, in the hope that the underlying problem has since been resolved. If no regressions are detected, the next step will be to remove the function from the code base. BUG=690229 ==========
kenrb@chromium.org changed reviewers: + alexmos@chromium.org
alexmos@: PTAL? I ran the regressed benchmark on most of the Android perf trybots for the first patchset, and they all have clean results. I also haven't been able to reproduce any regressions running telemetry locally, but I don't have any of the hardware that the original regressions were reported on, so that's less meaningful. It might be worth landing this and then watching the perf bots tomorrow. WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
That's good news on the perf bot not finding any regressions. But it looks like there are some Android test failures around scrolling and selection? If those turn out to be unrelated, then LGTM with a nit and let's try this out! https://codereview.chromium.org/2942903002/diff/20001/content/common/site_iso... File content/common/site_isolation_policy.cc (left): https://codereview.chromium.org/2942903002/diff/20001/content/common/site_iso... content/common/site_isolation_policy.cc:8: #include "base/feature_list.h" Might as well remove this and the next two headers. I don't think we need them anymore.
The CQ bit was checked by kenrb@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by kenrb@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by kenrb@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...
kenrb@chromium.org changed reviewers: + wjmaclean@chromium.org
wjmaclean@: I've uploaded an attempt to resolve the gesture handling problem that we were talking about yesterday, also after discussing it with dtapuska@. The existence of ContentViewCoreImpl::SendGestureEvent, whose callers simply create and dispatch new gesture events, is at odds with the assumption in RWHVIER that all touchscreen gesture events correspond to previous touch events. The current patch I have uploaded (which fixes tests locally, though trybots are still running), makes such gesture events be treated as standalone events. This breaks your earlier attempts to diagnose similar issues that had occasionally been seen on Windows, and in particular removes the UMA metric, which would no longer ever fire. If you are still interested in looking at that I could possibly use a compiler guard to separate Android vs desktop behavior on this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/23 18:53:47, kenrb wrote: > wjmaclean@: I've uploaded an attempt to resolve the gesture handling problem > that we were talking about yesterday, also after discussing it with dtapuska@. > The existence of ContentViewCoreImpl::SendGestureEvent, whose callers simply > create and dispatch new gesture events, is at odds with the assumption in > RWHVIER that all touchscreen gesture events correspond to previous touch events. > > The current patch I have uploaded (which fixes tests locally, though trybots are > still running), makes such gesture events be treated as standalone events. This > breaks your earlier attempts to diagnose similar issues that had occasionally > been seen on Windows, and in particular removes the UMA metric, which would no > longer ever fire. If you are still interested in looking at that I could > possibly use a compiler guard to separate Android vs desktop behavior on this. We'll want to keep an eye on how this behaves (in case there are corner-cases we haven't thought of), but generally this LGTM.
The CQ bit was checked by kenrb@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...
Thanks James. alexmos@: PTAL again? I have attached a couple of fixes to this, which are necessary for tests to pass -- are you okay with landing them together in this CL? https://codereview.chromium.org/2942903002/diff/20001/content/common/site_iso... File content/common/site_isolation_policy.cc (left): https://codereview.chromium.org/2942903002/diff/20001/content/common/site_iso... content/common/site_isolation_policy.cc:8: #include "base/feature_list.h" On 2017/06/19 16:45:48, alexmos wrote: > Might as well remove this and the next two headers. I don't think we need them > anymore. We still need feature_list.h and content_features.h for IsTopDocumentIsolationEnabled, but content_client.h is now removed.
Description was changed from ========== Make AreCrossProcessFramesPossible return true on Android The function AreCrossProcessFramePossible returns true on all platforms but Android, because of some memory regressions that were previously observed. The regressions do not currently manifest on perf trybots, so this is another attempt to make the change on Android, in the hope that the underlying problem has since been resolved. If no regressions are detected, the next step will be to remove the function from the code base. BUG=690229 ========== to ========== Make AreCrossProcessFramesPossible return true on Android The function AreCrossProcessFramePossible returns true on all platforms but Android, because of some memory regressions that were previously observed. The regressions do not currently manifest on perf trybots, so this is another attempt to make the change on Android, in the hope that the underlying problem has since been resolved. This also includes some changes to gesture event routing on touchscreen devices that are necessary to fix test breakage when input event routing for Android is always turned on. We now route gesture events even if there have been no corresponding touch events preceding them. If no regressions are detected, the next step will be to remove the function from the code base. BUG=690229 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> alexmos@: PTAL again? I have attached a couple of fixes to this, which are > necessary for tests to pass -- are you okay with landing them together in this > CL? I'm ok either way. There's risk that if this CL is reverted for an unrelated reason, you'd need to reland these fixes again - but if you're ok with that, I am too. :) Fixes themselves look ok (I'm not an expert on that logic, so deferring to James), with one quick question below. https://codereview.chromium.org/2942903002/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2942903002/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1020: if (SiteIsolationPolicy::AreCrossProcessFramesPossible() && Do we still want to check this, given that it'll always be true now? (I guess it'd make sense if this fix were in a separate prerequisite CL, where this being true wasn't yet guaranteed.)
The CQ bit was checked by kenrb@chromium.org to run a CQ dry run
https://codereview.chromium.org/2942903002/diff/120001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2942903002/diff/120001/content/browser/render... content/browser/renderer_host/render_widget_host_view_android.cc:1020: if (SiteIsolationPolicy::AreCrossProcessFramesPossible() && On 2017/06/26 17:03:33, alexmos wrote: > Do we still want to check this, given that it'll always be true now? (I guess > it'd make sense if this fix were in a separate prerequisite CL, where this being > true wasn't yet guaranteed.) Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 kenrb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wjmaclean@chromium.org Link to the patchset: https://codereview.chromium.org/2942903002/#ps140001 (title: "Removed unnecessary call")
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": 140001, "attempt_start_ts": 1498501986094900,
"parent_rev": "28d381082211e0ba9199c8c8de35ff77cda44066", "commit_rev":
"00d432064a6fcd6252211a938840efb8e36b463f"}
Message was sent while issue was closed.
Description was changed from ========== Make AreCrossProcessFramesPossible return true on Android The function AreCrossProcessFramePossible returns true on all platforms but Android, because of some memory regressions that were previously observed. The regressions do not currently manifest on perf trybots, so this is another attempt to make the change on Android, in the hope that the underlying problem has since been resolved. This also includes some changes to gesture event routing on touchscreen devices that are necessary to fix test breakage when input event routing for Android is always turned on. We now route gesture events even if there have been no corresponding touch events preceding them. If no regressions are detected, the next step will be to remove the function from the code base. BUG=690229 ========== to ========== Make AreCrossProcessFramesPossible return true on Android The function AreCrossProcessFramePossible returns true on all platforms but Android, because of some memory regressions that were previously observed. The regressions do not currently manifest on perf trybots, so this is another attempt to make the change on Android, in the hope that the underlying problem has since been resolved. This also includes some changes to gesture event routing on touchscreen devices that are necessary to fix test breakage when input event routing for Android is always turned on. We now route gesture events even if there have been no corresponding touch events preceding them. If no regressions are detected, the next step will be to remove the function from the code base. BUG=690229 Review-Url: https://codereview.chromium.org/2942903002 Cr-Commit-Position: refs/heads/master@{#482333} Committed: https://chromium.googlesource.com/chromium/src/+/00d432064a6fcd6252211a938840... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/00d432064a6fcd6252211a938840... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
