Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2972753002:
Make FrameSelection::MoveRangeSelection() not to set live granularity selection (Closed)
|
Created:
3 years, 5 months ago by yosin_UTC9 Modified:
3 years, 5 months ago Reviewers:
yoichio CC:
blink-reviews, chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake FrameSelection::MoveRangeSelection() not to set live granularity selection
This patch chagnes |FrameSelection::MoveRangeSelection()| not to set live
granularity selection as similar as the patch[1], which fixes selection created
by double-click.
[1] http://crrev.com/2951353003: Get rid of *live granularity* behavior from
selection
BUG=692923
TEST=run_webkit_unit_tests --gtest_filter=FrameSelectionTest.MoveRangeSelectionNoLiveness
Review-Url: https://codereview.chromium.org/2972753002
Cr-Commit-Position: refs/heads/master@{#484443}
Committed: https://chromium.googlesource.com/chromium/src/+/28f8baaa24a926b54e677c334a867b3dc63bef06
Patch Set 1 : 2017-07-05T14:09:35 #
Total comments: 2
Patch Set 2 : 2017-07-05T16:09:52 #
Total comments: 4
Patch Set 3 : 2017-07-05T18:14:38 #
Total comments: 2
Messages
Total messages: 29 (18 generated)
The CQ bit was checked by yosin@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 checked by yosin@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 ========== 2017-07-05T13:09:00 BUG= 2017-07-05T13:08:05 ========== to ========== Make FrameSelection::MoveRangeSelection() not to set live granularity selection This patch chagnes |FrameSelection::MoveRangeSelection()| not to set live granularity selection as similar as the patch[1], which fixes selection created by double-click. [1] http://crrev.com/2951353003: Get rid of *live granularity* behavior from selection BUG=692923 TEST=run_webkit_unit_tests --gtest_filter=FrameSelectionTest.MoveRangeSelectionNoLiveness ==========
Patchset #1 (id:1) has been deleted
yosin@chromium.org changed reviewers: + yoichio@chromium.org
PTAL
https://codereview.chromium.org/2972753002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/2972753002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/FrameSelection.cpp:1142: SetSelection(new_selection, kCloseTyping | kClearTypingStyle, Why don't you set selection even VS is none?
The CQ bit was checked by yosin@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...
PTAL Updated to follow review comment https://codereview.chromium.org/2972753002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/2972753002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/FrameSelection.cpp:1142: SetSelection(new_selection, kCloseTyping | kClearTypingStyle, On 2017/07/05 at 06:56:40, yoichio wrote: > Why don't you set selection even VS is none? Good catch! We don't need to set selection if expanded selection is none.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
https://codereview.chromium.org/2972753002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp (right): https://codereview.chromium.org/2972753002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:248: kWordGranularity); Could you expect_eq(SelectText(), 'xyz')? https://codereview.chromium.org/2972753002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:256: EXPECT_EQ(Position(sample->lastChild(), 3), selection.End()); Could you expect_eq(SelectText(), 'xyz') and expect_eq(sample->innerText(), 'abcxyz') ?
The CQ bit was checked by yosin@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...
PTAL Add EXPECT_EQ(expected, Selection().SelectedText()) https://codereview.chromium.org/2972753002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp (right): https://codereview.chromium.org/2972753002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:248: kWordGranularity); On 2017/07/05 at 08:57:24, yoichio wrote: > Could you expect_eq(SelectText(), 'xyz')? Done. https://codereview.chromium.org/2972753002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/FrameSelectionTest.cpp:256: EXPECT_EQ(Position(sample->lastChild(), 3), selection.End()); On 2017/07/05 at 08:57:24, yoichio wrote: > Could you expect_eq(SelectText(), 'xyz') and expect_eq(sample->innerText(), 'abcxyz') ? Done.
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by yosin@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": 1499306175865540,
"parent_rev": "404c9e4c524fb7aeeb6cc6b0580e3a9e53af51ab", "commit_rev":
"28f8baaa24a926b54e677c334a867b3dc63bef06"}
Message was sent while issue was closed.
Description was changed from ========== Make FrameSelection::MoveRangeSelection() not to set live granularity selection This patch chagnes |FrameSelection::MoveRangeSelection()| not to set live granularity selection as similar as the patch[1], which fixes selection created by double-click. [1] http://crrev.com/2951353003: Get rid of *live granularity* behavior from selection BUG=692923 TEST=run_webkit_unit_tests --gtest_filter=FrameSelectionTest.MoveRangeSelectionNoLiveness ========== to ========== Make FrameSelection::MoveRangeSelection() not to set live granularity selection This patch chagnes |FrameSelection::MoveRangeSelection()| not to set live granularity selection as similar as the patch[1], which fixes selection created by double-click. [1] http://crrev.com/2951353003: Get rid of *live granularity* behavior from selection BUG=692923 TEST=run_webkit_unit_tests --gtest_filter=FrameSelectionTest.MoveRangeSelectionNoLiveness Review-Url: https://codereview.chromium.org/2972753002 Cr-Commit-Position: refs/heads/master@{#484443} Committed: https://chromium.googlesource.com/chromium/src/+/28f8baaa24a926b54e677c334a86... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://chromium.googlesource.com/chromium/src/+/28f8baaa24a926b54e677c334a86...
Message was sent while issue was closed.
https://codereview.chromium.org/2972753002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/2972753002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/FrameSelection.cpp:1140: CreateVisibleSelection(new_selection); nlgtm Is this not VSinflattree?
Message was sent while issue was closed.
https://codereview.chromium.org/2972753002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/FrameSelection.cpp (right): https://codereview.chromium.org/2972753002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/FrameSelection.cpp:1140: CreateVisibleSelection(new_selection); On 2017/07/06 at 02:26:32, yoichio wrote: > nlgtm > Is this not VSinflattree? Talked offline. We should use flat tree version of position/range/selection in core/editing, except for core/editing/commands; for backward compatibility reasons[1]. In the meantime, we keep them as is, purely priority reasons. [1] http://crbug.com/739591 ⚐ Change DOM tree traversal to flat tree traversal in editing |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
