Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2963573003:
[TTS] Fix Bar text w/ selection modification (Closed)
|
Created:
3 years, 5 months ago by Donn Denman Modified:
3 years, 5 months ago Reviewers:
Theresa CC:
chromium-reviews, twellington+watch_chromium.org, donnd+watch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[TTS] Fix Bar text w/ selection modification
Fix issues with the Bar text not updating correctly when the selection
is being modified by the user.
There was a problem with ContentViewCore.getSelectedText not returning
the most recent version of the selection, so now we rely on our own
private member in CSSC that tracks the selection instead, except in
the case when the selection is first established.
Also do some minor cleanup of getBaseWebContents.
BUG=735365
Review-Url: https://codereview.chromium.org/2963573003
Cr-Commit-Position: refs/heads/master@{#483241}
Committed: https://chromium.googlesource.com/chromium/src/+/1fbe6dc80c6d1ebd7b20199a9b3f5dd270c8fb5f
Patch Set 1 #
Total comments: 3
Patch Set 2 : Removed usage of ContentViewCore.getSelectedText. #Patch Set 3 : Added usage of ContentViewCore.getSelectedText back again. #
Messages
Total messages: 21 (12 generated)
The CQ bit was checked by donnd@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...
donnd@chromium.org changed reviewers: + twellington@chromium.org
Theresa, PTAL when convenient.
https://codereview.chromium.org/2963573003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (right): https://codereview.chromium.org/2963573003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:254: if (baseContentView != null) mSelectedText = baseContentView.getSelectedText(); Do we need to refresh mSelectedText when we see the SELECTION_HANDLE_DRAG_STOPPED event?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Theresa, PTAL. Thanks! https://codereview.chromium.org/2963573003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java (right): https://codereview.chromium.org/2963573003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:253: ContentViewCore baseContentView = getBaseContentView(); Removed this usage of CVC.getSelectedText. https://codereview.chromium.org/2963573003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchSelectionController.java:254: if (baseContentView != null) mSelectedText = baseContentView.getSelectedText(); On 2017/06/27 23:52:40, Theresa wrote: > Do we need to refresh mSelectedText when we see the > SELECTION_HANDLE_DRAG_STOPPED event? Actually it's this case that was causing the problem -- when the drag stopped, we'd get the selection from the ContentViewCore, but it's currently stale for some reason. I looked at that API and it's marked @VisibleForTesting which makes me think it's not really a supported API outside of testing (and nothing else calls it except tests). So I now think it's best, and also simplest, to never try to get the selection from the CVC. Instead we'll just rely on the selectionChanged() notification to give us the selection.
lgtm
The CQ bit was checked by donnd@chromium.org
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
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...)
Description was changed from ========== [TTS] Fix Bar text w/ selection modification Fix issues with the Bar text not updating correctly when the selection is being modified by the user. There was a problem with ContentViewCore.getSelectedText not returning the most recent version of the selection, so now we rely on our own private member in CSSC that tracks the selection instead. Also do some minor cleanup of getBaseWebContents. BUG=735365 ========== to ========== [TTS] Fix Bar text w/ selection modification Fix issues with the Bar text not updating correctly when the selection is being modified by the user. There was a problem with ContentViewCore.getSelectedText not returning the most recent version of the selection, so now we rely on our own private member in CSSC that tracks the selection instead, except in the case when the selection is first established. Also do some minor cleanup of getBaseWebContents. BUG=735365 ==========
Theresa, there were two test failures that appeared to be due to the mSelectedText member not always being set up correctly. So I rolled back to patch set 1, and that fixes the tests. You've already reviewed patch set #1 and I addressed your only comment there, so I don't think you need to review this again. Feel free to look if you'd like! Committing... now.
The CQ bit was checked by donnd@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org Link to the patchset: https://codereview.chromium.org/2963573003/#ps40001 (title: "Added usage of ContentViewCore.getSelectedText back again.")
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": 1498693327520630,
"parent_rev": "3a083529baf8043e94aa394b810acdc320d953c8", "commit_rev":
"1fbe6dc80c6d1ebd7b20199a9b3f5dd270c8fb5f"}
Message was sent while issue was closed.
Description was changed from ========== [TTS] Fix Bar text w/ selection modification Fix issues with the Bar text not updating correctly when the selection is being modified by the user. There was a problem with ContentViewCore.getSelectedText not returning the most recent version of the selection, so now we rely on our own private member in CSSC that tracks the selection instead, except in the case when the selection is first established. Also do some minor cleanup of getBaseWebContents. BUG=735365 ========== to ========== [TTS] Fix Bar text w/ selection modification Fix issues with the Bar text not updating correctly when the selection is being modified by the user. There was a problem with ContentViewCore.getSelectedText not returning the most recent version of the selection, so now we rely on our own private member in CSSC that tracks the selection instead, except in the case when the selection is first established. Also do some minor cleanup of getBaseWebContents. BUG=735365 Review-Url: https://codereview.chromium.org/2963573003 Cr-Commit-Position: refs/heads/master@{#483241} Committed: https://chromium.googlesource.com/chromium/src/+/1fbe6dc80c6d1ebd7b20199a9b3f... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/1fbe6dc80c6d1ebd7b20199a9b3f... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
