New issue
Advanced search Search tips
Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment
link

Issue 912309: Android OOP-D: CursorAnchorInfoController doesn't receive all updates

Reported by ericrk@chromium.org, Dec 5 Project Member

Issue description

The 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.
 

Comment 1 by changwan@google.com, Dec 5

Cc: ctzsm@chromium.org
Eric, have you also filed a bug on fieldtrial_config.json change? If you haven't, could you file one?

It's unfortunate that we couldn't find an actual IME that uses this feature.
We should just try to fix the test using the text update as you mentioned in the email thread.

Comment 2 by changwan@google.com, Dec 5

In doing so, could you also manually test that scroll update triggers updateCursorAnchorInfo with an updated position value?

Comment 3 by ericrk@chromium.org, Dec 5

Filed issue 912376 to track the filedtrial_testing_config.json issue.

Comment 4 by bugdroid1@chromium.org, Dec 7

Project Member
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

Comment 5 by sheriffbot@chromium.org, Dec 10

Project Member
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

Comment 6 by ericrk@chromium.org, Dec 10

Labels: M-72 Merge-Request-72
I'd like to merge to m72 to address any potential IME issues due to oop-d.

Comment 7 by sheriffbot@chromium.org, Dec 11

Project Member
Labels: -Merge-Request-72 Hotlist-Merge-Approved Merge-Approved-72
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

Comment 8 by gov...@chromium.org, Dec 13

Pls merge your change to M72 branch 3626 ASAP. Thank you.

Comment 9 by gov...@chromium.org, Dec 13

Pls merge your change to M72 branch 3626 ASAP so we can pick it up for next M72 Beta release. Thank you.

Comment 10 by bugdroid1@chromium.org, Dec 14

Project Member
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

Comment 11 by sheriffbot@chromium.org, Dec 17

Project Member
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

Comment 12 by gov...@chromium.org, 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.

Comment 13 by ericrk@chromium.org, Dec 17

Needed to revert he change while investigating a VR issue. Will have to land this for next week's Beta.

Comment 14 by gov...@chromium.org, Dec 17

Labels: -Merge-Approved-72 Merge-Review-72
Moving back "Merge-Review-72" per comment #13. Please update the bug once change is ready and safe to merge. Thank you.

Comment 15 by gov...@chromium.org, 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.

Comment 16 by jonr...@chromium.org, Dec 21

Cc: jonr...@chromium.org

Comment 17 by bugdroid1@chromium.org, Dec 21

Project Member
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

Comment 18 by gov...@chromium.org, 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.

Comment 19 by gov...@chromium.org, 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.

Comment 20 by gov...@chromium.org, Dec 27

How is the change looking in canary so far?

Comment 21 by gov...@chromium.org, 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.

Comment 22 by ericrk@chromium.org, Jan 2

Labels: Merge-Request-72
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.

Comment 23 by sheriffbot@chromium.org, Jan 2

Project Member
Labels: -Merge-Request-72 Hotlist-Merge-Review
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

Comment 24 by gov...@chromium.org, Jan 2

Labels: -Merge-Review-72 Merge-Approved-72
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.

Comment 25 by gov...@chromium.org, Jan 2

Pls merge your change to M72 branch 3626 ASAP so we can pick it up for next beta release. Thank you.

Comment 26 by bugdroid1@chromium.org, Jan 2

Project Member
Labels: -merge-approved-72 merge-merged-3626
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

Comment 27 by cr-audit...@appspot.gserviceaccount.com, Jan 2

Project Member
Labels: Merge-Merged-72-3626
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}

Comment 28 by ericrk@chromium.org, Jan 2

Status: Fixed (was: Started)

Sign in to add a comment