Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2779813002:
[InputEvent] Don't modify selection when transpose was canceled (Closed)
Description[InputEvent] Don't modify selection when transpose was canceled
BUG=704791
Review-Url: https://codereview.chromium.org/2779813002
Cr-Commit-Position: refs/heads/master@{#462852}
Committed: https://chromium.googlesource.com/chromium/src/+/376d3697e8ce4898adee03e02ec1200f6ad05aa7
Patch Set 1 #
Total comments: 2
Patch Set 2 : Re-calculate range when selection changed #
Total comments: 20
Patch Set 3 : yosin's review: Use const& #
Messages
Total messages: 36 (24 generated)
The CQ bit was checked by chongz@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...
Description was changed from ========== Don't modify selection when transpose was canceled BUG= ========== to ========== Don't modify selection when transpose was canceled BUG=704791 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Don't modify selection when transpose was canceled BUG=704791 ========== to ========== [InputEvent] Don't modify selection when transpose was canceled BUG=704791 ==========
The CQ bit was checked by chongz@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
chongz@chromium.org changed reviewers: + yosin@chromium.org
yosin@ PTAL, thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm w/ nit https://codereview.chromium.org/2779813002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2779813002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:1427: frame().selection().computeVisibleSelectionInDOMTreeDeprecated()) nit: Since L1423 update layout, we don't need to use "deprecated" version here.
The CQ bit was checked by chongz@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.
Thinking more of this, I think we should re-calculate range&text after 'beforeinput' in case DOM or selection was changed. yosin@ PTAL again, thanks! https://codereview.chromium.org/2779813002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2779813002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:1427: frame().selection().computeVisibleSelectionInDOMTreeDeprecated()) On 2017/03/28 07:08:35, yosin_UTC9 wrote: > nit: Since L1423 update layout, we don't need to use "deprecated" version here. Done.
yosin@ Can you take a look at this CL again? Thanks!
On 2017/03/30 03:33:37, chongz wrote: > yosin@ Can you take a look at this CL again? Thanks! yosin@ Can you take a look again please? Thanks!
chongz@chromium.org changed reviewers: + tkent@chromium.org
tkent@ Can you take a look please? Thanks!
On 2017/04/05 at 15:04:41, chongz wrote: > tkent@ Can you take a look please? Thanks! yosin's review is enough.
https://codereview.chromium.org/2779813002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2779813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:148: EphemeralRange getTransposeRange(LocalFrame& frame) { s/getTransposeRange/computeRangeForTranspose/. Let's avoid using |get| for less confusion of getter of Editor class. https://codereview.chromium.org/2779813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:149: VisibleSelection selection = nit: s/VisibleSeleciton/const VisibleSelection&/ https://codereview.chromium.org/2779813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:155: VisiblePosition caret = selection.visibleStart(); nit: s/VisiblePosition/const VisiblePositon&/ https://codereview.chromium.org/2779813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:156: VisiblePosition next = nit: s/VisiblePosition/const VisiblePositon&/ https://codereview.chromium.org/2779813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:158: VisiblePosition previous = previousPositionOf(next); nit: s/VisiblePosition/const VisiblePositon&/ https://codereview.chromium.org/2779813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:161: previous = previousPositionOf(previous); Could you introduce new variable rather than re-using |previous|? https://codereview.chromium.org/2779813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:1406: EphemeralRange range = getTransposeRange(frame()); nit: s/EphemeralRange/const EphemeralRange&/ https://codereview.chromium.org/2779813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:1437: text = plainText(range); Could you introduce new variable to avoid re-using |text|? https://codereview.chromium.org/2779813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:1440: transposed = text.right(1) + text.left(1); Could you introduce new variable? https://codereview.chromium.org/2779813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:1442: const SelectionInDOMTree newSelection = nit: s/const SelectionInDOMTree/const SelectionInDOMTree&/
The CQ bit was checked by chongz@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.
yosin@ Please take a look again, thanks! https://codereview.chromium.org/2779813002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/2779813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:148: EphemeralRange getTransposeRange(LocalFrame& frame) { On 2017/04/06 04:32:23, yosin_UTC9 wrote: > s/getTransposeRange/computeRangeForTranspose/. > Let's avoid using |get| for less confusion of getter of Editor class. Done. https://codereview.chromium.org/2779813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:149: VisibleSelection selection = On 2017/04/06 04:32:23, yosin_UTC9 wrote: > nit: s/VisibleSeleciton/const VisibleSelection&/ Done. https://codereview.chromium.org/2779813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:155: VisiblePosition caret = selection.visibleStart(); On 2017/04/06 04:32:23, yosin_UTC9 wrote: > nit: s/VisiblePosition/const VisiblePositon&/ Done. https://codereview.chromium.org/2779813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:156: VisiblePosition next = On 2017/04/06 04:32:23, yosin_UTC9 wrote: > nit: s/VisiblePosition/const VisiblePositon&/ Done. https://codereview.chromium.org/2779813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:158: VisiblePosition previous = previousPositionOf(next); On 2017/04/06 04:32:23, yosin_UTC9 wrote: > nit: s/VisiblePosition/const VisiblePositon&/ Done. https://codereview.chromium.org/2779813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:161: previous = previousPositionOf(previous); On 2017/04/06 04:32:23, yosin_UTC9 wrote: > Could you introduce new variable rather than re-using |previous|? Done. https://codereview.chromium.org/2779813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:1406: EphemeralRange range = getTransposeRange(frame()); On 2017/04/06 04:32:24, yosin_UTC9 wrote: > nit: s/EphemeralRange/const EphemeralRange&/ Done. https://codereview.chromium.org/2779813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:1437: text = plainText(range); On 2017/04/06 04:32:23, yosin_UTC9 wrote: > Could you introduce new variable to avoid re-using |text|? Done. https://codereview.chromium.org/2779813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:1440: transposed = text.right(1) + text.left(1); On 2017/04/06 04:32:24, yosin_UTC9 wrote: > Could you introduce new variable? Done. https://codereview.chromium.org/2779813002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/Editor.cpp:1442: const SelectionInDOMTree newSelection = On 2017/04/06 04:32:24, yosin_UTC9 wrote: > nit: s/const SelectionInDOMTree/const SelectionInDOMTree&/ Done.
lgtm Thanks for make code cleaner! (^_^)b
The CQ bit was checked by chongz@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": 60001, "attempt_start_ts": 1491575299466020,
"parent_rev": "54ae18235d4fc39d7dad8a35f464f76ce75e2aca", "commit_rev":
"376d3697e8ce4898adee03e02ec1200f6ad05aa7"}
Message was sent while issue was closed.
Description was changed from ========== [InputEvent] Don't modify selection when transpose was canceled BUG=704791 ========== to ========== [InputEvent] Don't modify selection when transpose was canceled BUG=704791 Review-Url: https://codereview.chromium.org/2779813002 Cr-Commit-Position: refs/heads/master@{#462852} Committed: https://chromium.googlesource.com/chromium/src/+/376d3697e8ce4898adee03e02ec1... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/376d3697e8ce4898adee03e02ec1... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
