Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2847133004:
Smart Select shouldn't shorten selection (Closed)
DescriptionSmart Select shouldn't shorten selection
If the Smart Select classifier suggests shortening the selection, we discard the
suggestion. This makes it so Smart Select can only extend a selection and thus
fixes the issue where sometimes it would shorten a SelectAll selection.
BUG=714106
Review-Url: https://codereview.chromium.org/2847133004
Cr-Commit-Position: refs/heads/master@{#468473}
Committed: https://chromium.googlesource.com/chromium/src/+/79b60e688e852d5613913610c349eaf075601853
Patch Set 1 #
Total comments: 2
Patch Set 2 : adding tod #Messages
Total messages: 24 (15 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 ========== Smart Select shouldn't shorten selection BUG= ========== to ========== Smart Select shouldn't shorten selection If the Smart Select classifier suggests shortening the selection we discard the suggestion. This makes it so Smart Select can only extend a selection and thus fixes the issue where sometimes it would shorten a SelectAll selection. BUG=714106 ==========
Description was changed from ========== Smart Select shouldn't shorten selection If the Smart Select classifier suggests shortening the selection we discard the suggestion. This makes it so Smart Select can only extend a selection and thus fixes the issue where sometimes it would shorten a SelectAll selection. BUG=714106 ========== to ========== Smart Select shouldn't shorten selection If the Smart Select classifier suggests shortening the selection, we discard the suggestion. This makes it so Smart Select can only extend a selection and thus fixes the issue where sometimes it would shorten a SelectAll selection. BUG=714106 ==========
amaralp@chromium.org changed reviewers: + aelias@chromium.org, timav@chromium.org
PTAL
lgtm
On 2017/05/01 18:05:34, aelias wrote: > lgtm If we have to do this, I'd prefer reverting https://codereview.chromium.org/2722273002 instead till M60 where we should have a reliable way to tell which action initiated the selection. Alex, what is your opinion?
On 2017/05/01 at 18:14:29, timav wrote: > On 2017/05/01 18:05:34, aelias wrote: > > lgtm > > If we have to do this, I'd prefer reverting > https://codereview.chromium.org/2722273002 > instead till M60 where we should have a reliable way to tell > which action initiated the selection. > > Alex, what is your opinion? The Android TPM has been pushing hard for select-all-on-paste for 58 on http://crbug.com/615435, has argued it was already delayed way too long, and now that we've finally declared it fixed, I'm reluctant to reopen the debate with him. Furthermore smart-select is an O-specific problem affecting a tiny amount of users for now, whereas select-all-on-paste affects all Android users. As this workaround seems safe enough, I'd prefer to land it for now and to delete it as part of the patch that introduces "reliable way to tell which action initiated the selection".
Thank you for the thorough explanation. Please explain that this is a temporary fix though, otherwise lgtm https://codereview.chromium.org/2847133004/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): https://codereview.chromium.org/2847133004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:1038: // smaller than the original we throw away classification result and show the menu. Now I'm thinking that SelectAll just needs to classify without doing adjustment instead of throwing away. This has to be consistent with SelectAll for the selection menu, so not in this CL. https://codereview.chromium.org/2847133004/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:1039: if (result.startAdjust > 0 || result.endAdjust < 0) { If this is intended as a work around to figure out whether user pressed SelectAll, please indicate do in comments.
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.
The CQ bit was checked by amaralp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, timav@chromium.org Link to the patchset: https://codereview.chromium.org/2847133004/#ps20001 (title: "adding tod")
On 2017/05/01 at 19:31:22, timav wrote: > Thank you for the thorough explanation. > > Please explain that this is a temporary fix though, otherwise > lgtm > > https://codereview.chromium.org/2847133004/diff/1/content/public/android/java... > File content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java (right): > > https://codereview.chromium.org/2847133004/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:1038: // smaller than the original we throw away classification result and show the menu. > Now I'm thinking that SelectAll just needs to classify without doing adjustment instead of throwing away. This has to be consistent with SelectAll for the selection menu, so not in this CL. > Once we know it comes from a SelectAll we will just classify as opposed to suggest and classify. > https://codereview.chromium.org/2847133004/diff/1/content/public/android/java... > content/public/android/java/src/org/chromium/content/browser/SelectionPopupController.java:1039: if (result.startAdjust > 0 || result.endAdjust < 0) { > If this is intended as a work around to figure out whether user pressed SelectAll, please indicate do in comments. Done.
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": 20001, "attempt_start_ts": 1493679800907420,
"parent_rev": "c8feff62823e784dccb19ce9819ccda96bcb1763", "commit_rev":
"79b60e688e852d5613913610c349eaf075601853"}
Message was sent while issue was closed.
Description was changed from ========== Smart Select shouldn't shorten selection If the Smart Select classifier suggests shortening the selection, we discard the suggestion. This makes it so Smart Select can only extend a selection and thus fixes the issue where sometimes it would shorten a SelectAll selection. BUG=714106 ========== to ========== Smart Select shouldn't shorten selection If the Smart Select classifier suggests shortening the selection, we discard the suggestion. This makes it so Smart Select can only extend a selection and thus fixes the issue where sometimes it would shorten a SelectAll selection. BUG=714106 Review-Url: https://codereview.chromium.org/2847133004 Cr-Commit-Position: refs/heads/master@{#468473} Committed: https://chromium.googlesource.com/chromium/src/+/79b60e688e852d5613913610c349... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/79b60e688e852d5613913610c349...
Message was sent while issue was closed.
aelias@, could you please cherry-pick this to M59? |
