Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2926643002:
Only set mLastSelectedText when selection handles are present (Closed)
|
Created:
3 years, 6 months ago by amaralp Modified:
3 years, 6 months ago Reviewers:
aelias_OOO_until_Jul13 CC:
agrieve+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, jam Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionOnly set mLastSelectedText when selection comes from context menu
mLastSelectedText should only reflect text selections that trigger
|SelectionPopupController.showSelectionMenu()|. This ensures that mLastSelectedText
is only non-null when there is a selection with handles. This means |hasSelection()|
is only true when there is a touch handle selection.
BUG=729943
Review-Url: https://codereview.chromium.org/2926643002
Cr-Commit-Position: refs/heads/master@{#477491}
Committed: https://chromium.googlesource.com/chromium/src/+/a6ac8624bc411ce242955512d95d98216ce693fa
Patch Set 1 #
Total comments: 1
Messages
Total messages: 17 (11 generated)
The CQ bit was checked by amaralp@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.
Description was changed from ========== Only set mLastSelectedText when selection handles are present BUG=729943 ========== to ========== Only set mLastSelectedText when selection comes from context menu mLastSelectedText should only reflect text selections that trigger |SelectionPopupController.showSelectionMenu()|. This ensures that mLastSelecetedText is only not null when we have a selection with touch handles. BUG=729943 ==========
Description was changed from ========== Only set mLastSelectedText when selection comes from context menu mLastSelectedText should only reflect text selections that trigger |SelectionPopupController.showSelectionMenu()|. This ensures that mLastSelecetedText is only not null when we have a selection with touch handles. BUG=729943 ========== to ========== Only set mLastSelectedText when selection comes from context menu mLastSelectedText should only reflect text selections that trigger |SelectionPopupController.showSelectionMenu()|. This ensures that mLastSelecetedText is only not null and hence |hasSelection()| is only true when we have a selection with touch handles. BUG=729943 ==========
Description was changed from ========== Only set mLastSelectedText when selection comes from context menu mLastSelectedText should only reflect text selections that trigger |SelectionPopupController.showSelectionMenu()|. This ensures that mLastSelecetedText is only not null and hence |hasSelection()| is only true when we have a selection with touch handles. BUG=729943 ========== to ========== Only set mLastSelectedText when selection comes from context menu mLastSelectedText should only reflect text selections that trigger |SelectionPopupController.showSelectionMenu()|. This ensures that mLastSelectedText is only non-null when there is a selection with handles. This means |hasSelection()| is only true when there is a touch handle selection. BUG=729943 ==========
amaralp@chromium.org changed reviewers: + aelias@chromium.org
PTAL
https://codereview.chromium.org/2926643002/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (left): https://codereview.chromium.org/2926643002/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:1058: mLastSelectedText = text; This doesn't seem correct because contextual search uses mLastSelectedText via ContentViewCore.getSelectedText(). Also, are you sure it will always be updated properly if it changes after the initial action mode creation for the purposes of action mode share/search/etc? Maybe we need to go back to holding a separate boolean after all?
On 2017/06/06 at 22:14:43, aelias wrote: > https://codereview.chromium.org/2926643002/diff/1/content/public/android/java... > File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (left): > > https://codereview.chromium.org/2926643002/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:1058: mLastSelectedText = text; > This doesn't seem correct because contextual search uses mLastSelectedText via ContentViewCore.getSelectedText(). Before crrev.com/2785853002 getSelectedText() would return the empty string when mHasSelection was false. This means that if the selection didn't have handles getSelectedText() would return the empty string which is what would happen with this change. As far as I can tell Contextual Search is still working. > Also, are you sure it will always be updated properly if it changes after the initial action mode creation for the purposes of action mode share/search/etc? > I can't think of a situation. If there was one it would be a bigger problem since we are assuming that any change in the selection should trigger a new selection menu. > Maybe we need to go back to holding a separate boolean after all?
Got it, lgtm.
The CQ bit was checked by amaralp@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": 1, "attempt_start_ts": 1496796130683120, "parent_rev":
"439859f34663a7b1018449d251bbb1da1010ada0", "commit_rev":
"a6ac8624bc411ce242955512d95d98216ce693fa"}
Message was sent while issue was closed.
Description was changed from ========== Only set mLastSelectedText when selection comes from context menu mLastSelectedText should only reflect text selections that trigger |SelectionPopupController.showSelectionMenu()|. This ensures that mLastSelectedText is only non-null when there is a selection with handles. This means |hasSelection()| is only true when there is a touch handle selection. BUG=729943 ========== to ========== Only set mLastSelectedText when selection comes from context menu mLastSelectedText should only reflect text selections that trigger |SelectionPopupController.showSelectionMenu()|. This ensures that mLastSelectedText is only non-null when there is a selection with handles. This means |hasSelection()| is only true when there is a touch handle selection. BUG=729943 Review-Url: https://codereview.chromium.org/2926643002 Cr-Commit-Position: refs/heads/master@{#477491} Committed: https://chromium.googlesource.com/chromium/src/+/a6ac8624bc411ce242955512d95d... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/a6ac8624bc411ce242955512d95d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
