Android OOP-D: CursorAnchorInfoController doesn't receive all updates |
|||||||||||
Issue descriptionThe CursorAnchorInfoController class relies on its onFrameUpdate method being called to deliver updateCursorAnchorInfo calls to the InputMethodManager. This function is called whenever RenderWidgetHostViewAndroid gets new render frame metadata. Before the OOP-D feature, we used to send render frame metadata for every frame submission. With the re-architecture around OOP-D, we only send metadata from the renderer to the browser if the metadata has changed. Unfortunately, it appears that in many text editing scenarios, there is no change to the metadata as the text field is edited. This means that after enabling OOP-D, we aren't guaranteed to updateCursorAnchorInfo on each text change. Instead, we only reliably call updateCursorAnchorInfo if there is an insertion marker change or the content Y offset changes. This was found while trying to enable OOP-D by default. See bot run here: https://ci.chromium.org/p/chromium/builders/luci.chromium.try/android-marshmallow-arm64-rel/144188 If you search for "UpdateCursorAnchorInfo" you'll see the failing test. Current discussion leads me to believe that this may be a problem on certain Japanese IMEs which want to draw overlays to show suggestions.
,
Dec 5
In doing so, could you also manually test that scroll update triggers updateCursorAnchorInfo with an updated position value?
,
Dec 5
Filed issue 912376 to track the filedtrial_testing_config.json issue.
,
Dec 7
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fe193e30a172b63cc67222c5f353916e44e6db1e commit fe193e30a172b63cc67222c5f353916e44e6db1e Author: Eric Karl <ericrk@chromium.org> Date: Fri Dec 07 17:15:44 2018 Android OOP-D: Fix CursorAnchorInfoController updates This chnage causes RenderWidget to invalidate its current LocalSurfaceId whenever it sends WidgetHostMsg_TextInputStateChanged, causing a new RenderFrameMetadata to be sent for the given frame. This ensures that CursorAnchorInfoController receives updates on any frame with a text change. Bug: 912309 Change-Id: Ia1b0faa8bbb0208187d31f0024a17ffdd75bda16 Reviewed-on: https://chromium-review.googlesource.com/c/1364164 Commit-Queue: Eric Karl <ericrk@chromium.org> Reviewed-by: Saman Sami <samans@chromium.org> Cr-Commit-Position: refs/heads/master@{#614729} [modify] https://crrev.com/fe193e30a172b63cc67222c5f353916e44e6db1e/content/renderer/render_widget.cc
,
Dec 10
This issue is marked as a release blocker with no milestone associated. Please add an appropriate milestone. All release blocking issues should have milestones associated to it, so that the issue can tracked and the fixes can be pushed promptly. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 10
I'd like to merge to m72 to address any potential IME issues due to oop-d.
,
Dec 11
Your change meets the bar and is auto-approved for M72. Please go ahead and merge the CL to branch 3626 manually. Please contact milestone owner if you have questions. Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 13
Pls merge your change to M72 branch 3626 ASAP. Thank you.
,
Dec 13
Pls merge your change to M72 branch 3626 ASAP so we can pick it up for next M72 Beta release. Thank you.
,
Dec 14
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/fe9575cf58bf8bf9b1341f2024879a1c165009ba commit fe9575cf58bf8bf9b1341f2024879a1c165009ba Author: Brian Sheedy <bsheedy@chromium.org> Date: Fri Dec 14 17:28:43 2018 Revert "Android OOP-D: Fix CursorAnchorInfoController updates" This reverts commit fe193e30a172b63cc67222c5f353916e44e6db1e. Reason for revert: Cause of https://crbug.com/913161 Original change's description: > Android OOP-D: Fix CursorAnchorInfoController updates > > This chnage causes RenderWidget to invalidate its current > LocalSurfaceId whenever it sends WidgetHostMsg_TextInputStateChanged, > causing a new RenderFrameMetadata to be sent for the given frame. > > This ensures that CursorAnchorInfoController receives updates on any > frame with a text change. > > Bug: 912309 > Change-Id: Ia1b0faa8bbb0208187d31f0024a17ffdd75bda16 > Reviewed-on: https://chromium-review.googlesource.com/c/1364164 > Commit-Queue: Eric Karl <ericrk@chromium.org> > Reviewed-by: Saman Sami <samans@chromium.org> > Cr-Commit-Position: refs/heads/master@{#614729} TBR=piman@chromium.org,samans@chromium.org,ericrk@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 912309 Change-Id: I9bcca12fc7a7040f6deffb1f098ba60486349062 Reviewed-on: https://chromium-review.googlesource.com/c/1376314 Commit-Queue: Eric Karl <ericrk@chromium.org> Reviewed-by: Brian Sheedy <bsheedy@chromium.org> Cr-Commit-Position: refs/heads/master@{#616724} [modify] https://crrev.com/fe9575cf58bf8bf9b1341f2024879a1c165009ba/content/renderer/render_widget.cc
,
Dec 17
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Dec 17
Pls merge your change to M72 branch 3626 ASAP so we can pick it up for this week beta release, RC cut tomorrow, Tuesday @ 1:00 PM PT.
,
Dec 17
Needed to revert he change while investigating a VR issue. Will have to land this for next week's Beta.
,
Dec 17
Moving back "Merge-Review-72" per comment #13. Please update the bug once change is ready and safe to merge. Thank you.
,
Dec 20
Reminder M72 Stable is coming soon. Please review this bug and assess if this is indeed a RBS. If not, please remove the RBS label. If so, please make sure to land the fix and request a merge into the release branch ASAP. Thank you.
,
Dec 21
,
Dec 21
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/706ea6713adeaa8953f05e9b2ffdc2dc17ec52dd commit 706ea6713adeaa8953f05e9b2ffdc2dc17ec52dd Author: Eric Karl <ericrk@chromium.org> Date: Fri Dec 21 22:41:38 2018 Android OOP-D: Re-land Fix CursorAnchorInfoController updates This change adds a RequestForceSendMetadata method to LayerTreeView/Host/Impl, which allows us to force sending RFM with the next frame. This ensures that CursorAnchorInfoController receives updates on any frame with a text change. Bug: 912309 Change-Id: I21185b57c2937c94d306d4ceed239d2f49bc824d Reviewed-on: https://chromium-review.googlesource.com/c/1383533 Reviewed-by: Saman Sami <samans@chromium.org> Commit-Queue: Eric Karl <ericrk@chromium.org> Cr-Commit-Position: refs/heads/master@{#618643} [modify] https://crrev.com/706ea6713adeaa8953f05e9b2ffdc2dc17ec52dd/cc/trees/layer_tree_host.cc [modify] https://crrev.com/706ea6713adeaa8953f05e9b2ffdc2dc17ec52dd/cc/trees/layer_tree_host.h [modify] https://crrev.com/706ea6713adeaa8953f05e9b2ffdc2dc17ec52dd/cc/trees/layer_tree_host_impl.cc [modify] https://crrev.com/706ea6713adeaa8953f05e9b2ffdc2dc17ec52dd/cc/trees/layer_tree_host_impl_unittest.cc [modify] https://crrev.com/706ea6713adeaa8953f05e9b2ffdc2dc17ec52dd/cc/trees/layer_tree_host_unittest.cc [modify] https://crrev.com/706ea6713adeaa8953f05e9b2ffdc2dc17ec52dd/cc/trees/layer_tree_impl.cc [modify] https://crrev.com/706ea6713adeaa8953f05e9b2ffdc2dc17ec52dd/cc/trees/layer_tree_impl.h [modify] https://crrev.com/706ea6713adeaa8953f05e9b2ffdc2dc17ec52dd/cc/trees/render_frame_metadata_observer.h [modify] https://crrev.com/706ea6713adeaa8953f05e9b2ffdc2dc17ec52dd/content/renderer/compositor/layer_tree_view.cc [modify] https://crrev.com/706ea6713adeaa8953f05e9b2ffdc2dc17ec52dd/content/renderer/compositor/layer_tree_view.h [modify] https://crrev.com/706ea6713adeaa8953f05e9b2ffdc2dc17ec52dd/content/renderer/render_frame_metadata_observer_impl.cc [modify] https://crrev.com/706ea6713adeaa8953f05e9b2ffdc2dc17ec52dd/content/renderer/render_frame_metadata_observer_impl.h [modify] https://crrev.com/706ea6713adeaa8953f05e9b2ffdc2dc17ec52dd/content/renderer/render_frame_metadata_observer_impl_unittest.cc [modify] https://crrev.com/706ea6713adeaa8953f05e9b2ffdc2dc17ec52dd/content/renderer/render_widget.cc [modify] https://crrev.com/706ea6713adeaa8953f05e9b2ffdc2dc17ec52dd/content/renderer/render_widget_unittest.cc
,
Dec 22
Pls update bug with canary result. If change looks good in canary and safe to merge, pls request a merge to M72. Thank you.
,
Dec 27
Reminder M72 Stable is coming soon. Please review this bug and assess if this is indeed a RBS. If not, please remove the RBS label. If so, please make sure to land the fix and request a merge into the release branch ASAP. Thank you.
,
Dec 27
How is the change looking in canary so far?
,
Jan 2
Reminder M72 Stable is coming VERY soon. Please review this bug and assess if this is indeed a RBS. If not, please remove the RBS label. If so, please make sure to land the fix and request a merge into the release branch ASAP. Thank you.
,
Jan 2
The latest version of the change looks good. It fixes the issue in question and has baked in Canary for the last 1.5 weeks with no reported issues, so at this point I'm confident that this fixes the issue and is safe to merge.
,
Jan 2
This bug requires manual review: Reverts referenced in bugdroid comments after merge request. Please contact the milestone owner if you have questions. Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 2
Approving merge to M72 branch 3626 based on comment #22. Please merge ASAP and mark bug as fixed after the merge if nothing else is pending. Thank you.
,
Jan 2
Pls merge your change to M72 branch 3626 ASAP so we can pick it up for next beta release. Thank you.
,
Jan 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/29a92ec89dc5bedda9e64e874caf0eb5c9bee51a commit 29a92ec89dc5bedda9e64e874caf0eb5c9bee51a Author: Eric Karl <ericrk@chromium.org> Date: Wed Jan 02 19:50:10 2019 Android OOP-D: Re-land Fix CursorAnchorInfoController updates This change adds a RequestForceSendMetadata method to LayerTreeView/Host/Impl, which allows us to force sending RFM with the next frame. This ensures that CursorAnchorInfoController receives updates on any frame with a text change. (cherry picked from commit 706ea6713adeaa8953f05e9b2ffdc2dc17ec52dd) Bug: 912309 Change-Id: I21185b57c2937c94d306d4ceed239d2f49bc824d Reviewed-on: https://chromium-review.googlesource.com/c/1383533 Reviewed-by: Saman Sami <samans@chromium.org> Commit-Queue: Eric Karl <ericrk@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#618643} Reviewed-on: https://chromium-review.googlesource.com/c/1393469 Reviewed-by: Eric Karl <ericrk@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#540} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/29a92ec89dc5bedda9e64e874caf0eb5c9bee51a/cc/trees/layer_tree_host.cc [modify] https://crrev.com/29a92ec89dc5bedda9e64e874caf0eb5c9bee51a/cc/trees/layer_tree_host.h [modify] https://crrev.com/29a92ec89dc5bedda9e64e874caf0eb5c9bee51a/cc/trees/layer_tree_host_impl.cc [modify] https://crrev.com/29a92ec89dc5bedda9e64e874caf0eb5c9bee51a/cc/trees/layer_tree_host_impl_unittest.cc [modify] https://crrev.com/29a92ec89dc5bedda9e64e874caf0eb5c9bee51a/cc/trees/layer_tree_host_unittest.cc [modify] https://crrev.com/29a92ec89dc5bedda9e64e874caf0eb5c9bee51a/cc/trees/layer_tree_impl.cc [modify] https://crrev.com/29a92ec89dc5bedda9e64e874caf0eb5c9bee51a/cc/trees/layer_tree_impl.h [modify] https://crrev.com/29a92ec89dc5bedda9e64e874caf0eb5c9bee51a/cc/trees/render_frame_metadata_observer.h [modify] https://crrev.com/29a92ec89dc5bedda9e64e874caf0eb5c9bee51a/content/renderer/gpu/layer_tree_view.cc [modify] https://crrev.com/29a92ec89dc5bedda9e64e874caf0eb5c9bee51a/content/renderer/gpu/layer_tree_view.h [modify] https://crrev.com/29a92ec89dc5bedda9e64e874caf0eb5c9bee51a/content/renderer/render_frame_metadata_observer_impl.cc [modify] https://crrev.com/29a92ec89dc5bedda9e64e874caf0eb5c9bee51a/content/renderer/render_frame_metadata_observer_impl.h [modify] https://crrev.com/29a92ec89dc5bedda9e64e874caf0eb5c9bee51a/content/renderer/render_frame_metadata_observer_impl_unittest.cc [modify] https://crrev.com/29a92ec89dc5bedda9e64e874caf0eb5c9bee51a/content/renderer/render_widget.cc [modify] https://crrev.com/29a92ec89dc5bedda9e64e874caf0eb5c9bee51a/content/renderer/render_widget_unittest.cc
,
Jan 2
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/29a92ec89dc5bedda9e64e874caf0eb5c9bee51a Commit: 29a92ec89dc5bedda9e64e874caf0eb5c9bee51a Author: ericrk@chromium.org Commiter: ericrk@chromium.org Date: 2019-01-02 19:50:10 +0000 UTC Android OOP-D: Re-land Fix CursorAnchorInfoController updates This change adds a RequestForceSendMetadata method to LayerTreeView/Host/Impl, which allows us to force sending RFM with the next frame. This ensures that CursorAnchorInfoController receives updates on any frame with a text change. (cherry picked from commit 706ea6713adeaa8953f05e9b2ffdc2dc17ec52dd) Bug: 912309 Change-Id: I21185b57c2937c94d306d4ceed239d2f49bc824d Reviewed-on: https://chromium-review.googlesource.com/c/1383533 Reviewed-by: Saman Sami <samans@chromium.org> Commit-Queue: Eric Karl <ericrk@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#618643} Reviewed-on: https://chromium-review.googlesource.com/c/1393469 Reviewed-by: Eric Karl <ericrk@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#540} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
,
Jan 2
|
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by changwan@google.com
, Dec 5