Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2851363003:
[InputEvent] Update EndingSelection when appending UndoStep (Closed)
Description[InputEvent] Update EndingSelection when appending UndoStep
BUG=710707
Review-Url: https://codereview.chromium.org/2851363003
Cr-Commit-Position: refs/heads/master@{#470567}
Committed: https://chromium.googlesource.com/chromium/src/+/6e66e1c2bd52b4790d7899535c77f13c3a5cc499
Patch Set 1 #
Total comments: 4
Patch Set 2 : Address yosin's comments #
Total comments: 2
Patch Set 3 : yosin's comment: Use |start === undefined| #
Messages
Total messages: 29 (18 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 ========== Update |EndingSelection| in |CompositeEditCommand::AppendCommandToUndoStep()| BUG= ========== to ========== [InputEvent] Update |EndingSelection| in |CompositeEditCommand::AppendCommandToUndoStep()| BUG=710707 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== [InputEvent] Update |EndingSelection| in |CompositeEditCommand::AppendCommandToUndoStep()| BUG=710707 ========== to ========== [InputEvent] Update EndingSelection when appending UndoStep BUG=710707 ==========
chongz@chromium.org changed reviewers: + yutak@chromium.org
yutak@ PTAL, thanks!
I'm not quite familiar with those code at this point; if this is not urgent, can you hold on until yosin is back?
chongz@chromium.org changed reviewers: + yosin@chromium.org
On 2017/05/02 03:58:35, Yuta Kitamura wrote: > I'm not quite familiar with those code at this point; > if this is not urgent, can you hold on until yosin is back? Hmmm should be fine since Safari also has this issue in stable. Adding yosin as reviewer.
https://codereview.chromium.org/2851363003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-drag-drop.html (right): https://codereview.chromium.org/2851363003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-drag-drop.html:316: input_drag.value = "12345678"; nit: Could you use single-quote since other parts of the script use single-quote? https://codereview.chromium.org/2851363003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp (right): https://codereview.chromium.org/2851363003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:219: EnsureUndoStep()->SetEndingSelection( Could you move |SetEndingSelection()| call to |Editor::AppliedEditing()| after |AppendCommandToUndoStep()|? To make |AppendCommandToUndoStep()| does just appending? I'm not sure whether |AppendCommandToUndoStep()| should update ending selection or not. I would like to postpone the decision until we have another call site.
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: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
yosin@ PTAL again, thanks! https://codereview.chromium.org/2851363003/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-drag-drop.html (right): https://codereview.chromium.org/2851363003/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-drag-drop.html:316: input_drag.value = "12345678"; On 2017/05/08 04:01:05, yosin_UTC9 wrote: > nit: Could you use single-quote since other parts of the script use > single-quote? Done. https://codereview.chromium.org/2851363003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp (right): https://codereview.chromium.org/2851363003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp:219: EnsureUndoStep()->SetEndingSelection( On 2017/05/08 04:01:05, yosin_UTC9 wrote: > Could you move |SetEndingSelection()| call to |Editor::AppliedEditing()| after > |AppendCommandToUndoStep()|? > To make |AppendCommandToUndoStep()| does just appending? > I'm not sure whether |AppendCommandToUndoStep()| should update ending selection > or not. > I would like to postpone the decision until we have another call site. Moved to |Editor::AppliedEditing()| but I have to call |SetEndingSelection()| before |AppendCommandToUndoStep()|. e.g. |command->undo_step_| will be cleared during |AppendCommandToUndoStep()|.
lgtm w/ nit https://codereview.chromium.org/2851363003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-drag-drop.html (right): https://codereview.chromium.org/2851363003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-drag-drop.html:24: if (typeof start === 'undefined' || typeof end === 'undefined') { nit: No need to use |typeof| operator, just write |start === undefined|
https://codereview.chromium.org/2851363003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-drag-drop.html (right): https://codereview.chromium.org/2851363003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/events/inputevents/inputevent-drag-drop.html:24: if (typeof start === 'undefined' || typeof end === 'undefined') { On 2017/05/09 00:57:58, yosin_UTC9 wrote: > nit: No need to use |typeof| operator, just write |start === undefined| Done.
The CQ bit was checked by chongz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2851363003/#ps40001 (title: "yosin's comment: Use |start === undefined|")
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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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": 40001, "attempt_start_ts": 1494426688623310,
"parent_rev": "1f8fe2383c7805bb0385c118b19a8efe008ec741", "commit_rev":
"6e66e1c2bd52b4790d7899535c77f13c3a5cc499"}
Message was sent while issue was closed.
Description was changed from ========== [InputEvent] Update EndingSelection when appending UndoStep BUG=710707 ========== to ========== [InputEvent] Update EndingSelection when appending UndoStep BUG=710707 Review-Url: https://codereview.chromium.org/2851363003 Cr-Commit-Position: refs/heads/master@{#470567} Committed: https://chromium.googlesource.com/chromium/src/+/6e66e1c2bd52b4790d7899535c77... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/6e66e1c2bd52b4790d7899535c77... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
