Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2820343004:
[DMC #4] Add CompositionMarkerListImpl (Closed)
DescriptionAdd CompositionMarkerListImpl
This is the first of the specialized DocumentMarkerList implementations. It is
currently quite similar to GenericDocumentMarkerListImpl, except it doesn't have
the logic for merging Spelling/Grammar markers, and it doesn't implement the
RemoveMarkersUnderWords() method on DocumentMarkerList that will eventually be
moved to SpellCheckMarkerList. Eventually we will stop creating
RenderedDocumentMarkers in non-TextMatchMarkerLists.
BUG=707867
Review-Url: https://codereview.chromium.org/2820343004
Cr-Commit-Position: refs/heads/master@{#470183}
Committed: https://chromium.googlesource.com/chromium/src/+/45abde5e4bc317fc2e91152b1d66feec93673563
Patch Set 1 #Patch Set 2 : Remove comment in DMC::CreateListForType() that's no longer accurate #Patch Set 3 : Rebase #Patch Set 4 : Rebase again #Patch Set 5 : Add comments #
Total comments: 10
Patch Set 6 : Respond to comments #Patch Set 7 : Use correct base commit #
Total comments: 2
Patch Set 8 : Add NOTREACHED(), re-remove UpdateRenderedRectsForComposition test case #Patch Set 9 : Change kComposition case to NOTREACHED() #
Total comments: 3
Patch Set 10 : DECLARE_VIRTUAL_TRACE() #
Total comments: 1
Patch Set 11 : Fix bad comment #
Total comments: 3
Patch Set 12 : Fix nits #Dependent Patchsets: Messages
Total messages: 59 (39 generated)
The CQ bit was checked by rlanday@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...
rlanday@chromium.org changed reviewers: + xiaochengh@chromium.org, yosin@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Message was sent while issue was closed.
Description was changed from ========== Add CompositionMarkerListImpl This is the first of the specialized DocumentMarkerList implementations. I thought that maybe there's only ever one Composition marker at a time, but this isn't quite true, apparently due to the way TextIterator can split up the composition underline into multiple DocumentMarkers (some test cases failed when I tried this, see https://codereview.chromium.org/2820203003 ). However, I do believe that the Composition markers for a node are always inserted in order, so we can store them as a sorted list and DCHECK() on insert that the markers are being inserted at the end of the list. BUG=707867 ========== to ========== Add CompositionMarkerListImpl This is the first of the specialized DocumentMarkerList implementations. It is currently quite similar to GenericDocumentMarkerListImpl, except it doesn't have the logic for merging Spelling/Grammar markers, and it doesn't implement the RemoveMarkersUnderWords() method on DocumentMarkerList that will eventually be moved to SpellCheckMarkerList. Eventually we will stop creating RenderedDocumentMarkers in non-TextMatchMarkerLists. BUG=707867 ==========
The CQ bit was checked by rlanday@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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rlanday@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 rlanday@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.
https://codereview.chromium.org/2820343004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.cpp (right): https://codereview.chromium.org/2820343004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.cpp:18: RenderedDocumentMarker* rendered_marker = It seems this impl is identical to TextMatchmarkerListImpl::Add(). Should we move this to DMListEditor to share between them?
https://codereview.chromium.org/2820343004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.cpp (right): https://codereview.chromium.org/2820343004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.cpp:18: RenderedDocumentMarker* rendered_marker = On 2017/04/26 at 06:45:47, yosin_UTC9 wrote: > It seems this impl is identical to TextMatchmarkerListImpl::Add(). > Should we move this to DMListEditor to share between them? Seems that we need a patch #3.5 here so that: - DMLEditor::Add() is split into DMLEditor::AddAndMergeOverlapping() and DMLEditor::AddWithoutMergingOverlapping(). The two new functions should not check the type of the new marker - GenDMLListImpl::Add() does the following: switch (type) { case Spelling: case Grammar: DMLEditor::AddAndMergeOverlapping(...); return; case TextMatch: cast Composition: DMLEditor::AddWithoutMergingOverlapping(...); return; default: NOTREACHED(); } Then when introducing other list impls, we can split the impl of GenDMLImpl::Add, so that we don't introduce duplicate code when introducing new list impls. https://codereview.chromium.org/2820343004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.h (right): https://codereview.chromium.org/2820343004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.h:14: // Composition markers are typically inserted in an order. Is MoveMarkers() the only site making out-of-order insertions? It seems to be a little bit weird by stating an assumption without any utilization/heuristic. https://codereview.chromium.org/2820343004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.h:15: class CORE_EXPORT CompositionMarkerListImpl : public DocumentMarkerList { nit: final https://codereview.chromium.org/2820343004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImplTest.cpp (right): https://codereview.chromium.org/2820343004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImplTest.cpp:25: TEST_F(CompositionMarkerListImplTest, Add) { nit: please add a comment that this tests CompositionMarkerListImpl doesn't merge touching markers. https://codereview.chromium.org/2820343004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp (left): https://codereview.chromium.org/2820343004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp:220: TEST_F(DocumentMarkerControllerTest, UpdateRenderedRectsForComposition) { Why removing this test?
https://codereview.chromium.org/2820343004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.h (right): https://codereview.chromium.org/2820343004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.h:14: // Composition markers are typically inserted in an order. On 2017/04/26 at 14:17:53, Xiaocheng wrote: > Is MoveMarkers() the only site making out-of-order insertions? > > It seems to be a little bit weird by stating an assumption without any utilization/heuristic. The implication is supposed to be that we can keep the list sorted (to perform certain operations more efficiently), while incurring hardly any extra cost on inserts, since we're just going to be doing O(1) inserts at the end of the list (the MoveMarkers() case is the only exception I know of). I can try to make the comment more clear. https://codereview.chromium.org/2820343004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp (left): https://codereview.chromium.org/2820343004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp:220: TEST_F(DocumentMarkerControllerTest, UpdateRenderedRectsForComposition) { On 2017/04/26 at 14:17:53, Xiaocheng wrote: > Why removing this test? There was an earlier version of this CL where I made CompositionMarkerList not create the RenderedDocumentMarkers, at which point this test was testing something that was no longer supported. We're still planning to make this change, but in a later CL. So I can leave the test in for now, but it's testing for something we're actually planning on removing.
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
The CQ bit was checked by rlanday@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...
Updated https://codereview.chromium.org/2820343004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.cpp (right): https://codereview.chromium.org/2820343004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.cpp:44: return DocumentMarkerListEditor::RemoveMarkersUnderWords(&markers_, node_text, I added a call to DocumentMarkerListEditor::RemoveMarkersUnderWords() here since we have it rather than adding an empty implementation to the interface; I think it doesn't matter too much one way or the other since we're going to move this method to SpellCheckMarkerListImpl in the next CL.
https://codereview.chromium.org/2820343004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp (left): https://codereview.chromium.org/2820343004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp:220: TEST_F(DocumentMarkerControllerTest, UpdateRenderedRectsForComposition) { On 2017/04/26 at 20:33:24, rlanday wrote: > On 2017/04/26 at 14:17:53, Xiaocheng wrote: > > Why removing this test? > > There was an earlier version of this CL where I made CompositionMarkerList > not create the RenderedDocumentMarkers, at which point this test was > testing something that was no longer supported. We're still planning to > make this change, but in a later CL. So I can leave the test in for now, > but it's testing for something we're actually planning on removing. Oh, it's calling UpdateRenderedRects on composition markers. Fine with removing it. https://codereview.chromium.org/2820343004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.cpp (right): https://codereview.chromium.org/2820343004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.cpp:44: return DocumentMarkerListEditor::RemoveMarkersUnderWords(&markers_, node_text, On 2017/04/26 at 20:54:07, rlanday wrote: > I added a call to DocumentMarkerListEditor::RemoveMarkersUnderWords() here since we > have it rather than adding an empty implementation to the interface; I think it > doesn't matter too much one way or the other since we're going to move this method > to SpellCheckMarkerListImpl in the next CL. Now that DMC::RemoveMarkersUnderWords() only removes spelling and grammar markers, I think NOTREACHED() is the best function body here.
https://codereview.chromium.org/2820343004/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp (left): https://codereview.chromium.org/2820343004/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp:220: TEST_F(DocumentMarkerControllerTest, UpdateRenderedRectsForComposition) { On 2017/04/26 at 22:20:36, Xiaocheng wrote: > Oh, it's calling UpdateRenderedRects on composition markers. > > Fine with removing it. Just to clarify for anyone else reading this: there's already a second copy of the test case called UpdateRenderedRects(); right now it uses a Spelling marker, but I'm changing it to create a TextMatch marker (the only type we really care about the rendered rects for) in https://codereview.chromium.org/2829543002
The CQ bit was checked by rlanday@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Please remove the kComposition case from GenDMLImpl::AddMarker, which otherwise becomes dead code after this patch. Other impl classes should follow this pattern.
On 2017/04/27 at 04:14:52, xiaochengh wrote: > Please remove the kComposition case from GenDMLImpl::AddMarker, which otherwise becomes dead code after this patch. > > Other impl classes should follow this pattern. Ok, I changed it to NOTREACHED()
All right, no more comments. Let's wait for yosin@'s review to #3.5 https://codereview.chromium.org/2820343004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/GenericDocumentMarkerListImpl.cpp (right): https://codereview.chromium.org/2820343004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/GenericDocumentMarkerListImpl.cpp:26: case DocumentMarker::kComposition: nit: There's no need to add another NOTREACHED(), as it will be captured by the NOTREACHED() at the end.
https://codereview.chromium.org/2820343004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/GenericDocumentMarkerListImpl.cpp (right): https://codereview.chromium.org/2820343004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/GenericDocumentMarkerListImpl.cpp:26: case DocumentMarker::kComposition: On 2017/04/27 at 17:49:41, Xiaocheng wrote: > nit: There's no need to add another NOTREACHED(), as it will be captured by the NOTREACHED() at the end. I'd have to write "break" or something anyway (I can't leave the case out entirely without adding a default case, which I generally like to avoid because then someone could add a new case and forget to update some of the switch statements)
https://codereview.chromium.org/2820343004/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/GenericDocumentMarkerListImpl.cpp (right): https://codereview.chromium.org/2820343004/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/GenericDocumentMarkerListImpl.cpp:26: case DocumentMarker::kComposition: On 2017/04/27 at 18:01:02, rlanday wrote: > On 2017/04/27 at 17:49:41, Xiaocheng wrote: > > nit: There's no need to add another NOTREACHED(), as it will be captured by the NOTREACHED() at the end. > > I'd have to write "break" or something anyway (I can't leave the case out entirely > without adding a default case, which I generally like to avoid because then someone > could add a new case and forget to update some of the switch statements) All right, fine.
The CQ bit was checked by rlanday@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.
lgtm https://codereview.chromium.org/2820343004/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.cpp (right): https://codereview.chromium.org/2820343004/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.cpp:45: // TextMatchMarkerListImpl is added. DocumentMarkerController shouldn't try to nit: s/TextMatch/SpellCheck/
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
On 2017/04/28 at 18:13:12, xiaochengh wrote: > lgtm > > https://codereview.chromium.org/2820343004/diff/180001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.cpp (right): > > https://codereview.chromium.org/2820343004/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.cpp:45: // TextMatchMarkerListImpl is added. DocumentMarkerController shouldn't try to > nit: s/TextMatch/SpellCheck/ Fixed
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm w/ nits https://codereview.chromium.org/2820343004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.h (right): https://codereview.chromium.org/2820343004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/CompositionMarkerListImpl.h:37: DECLARE_VIRTUAL_TRACE(); nit: Could you add a blank line before |DECLARE_VIRTUAL_TRACE();|? https://codereview.chromium.org/2820343004/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp (right): https://codereview.chromium.org/2820343004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp:21: MarkerList::iterator pos = std::lower_bound( nit: s/MarkerList::iterator/const auto/ https://codereview.chromium.org/2820343004/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp:134: RenderedDocumentMarker* rendered_marker = nit: s/RenderedDocumentMarker*/RenderedDocumentMarker* const/
Ah, I think the DMLEditor changes were done already in https://codereview.chromium.org/2842263002 and were mistakenly included in this CL
The CQ bit was checked by rlanday@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiaochengh@chromium.org, yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2820343004/#ps220001 (title: "Fix nits")
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 rlanday@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": 220001, "attempt_start_ts": 1494291014951630,
"parent_rev": "9f96db6af2442820fed423f5b1b745bc049ae0ad", "commit_rev":
"45abde5e4bc317fc2e91152b1d66feec93673563"}
Message was sent while issue was closed.
Description was changed from ========== Add CompositionMarkerListImpl This is the first of the specialized DocumentMarkerList implementations. It is currently quite similar to GenericDocumentMarkerListImpl, except it doesn't have the logic for merging Spelling/Grammar markers, and it doesn't implement the RemoveMarkersUnderWords() method on DocumentMarkerList that will eventually be moved to SpellCheckMarkerList. Eventually we will stop creating RenderedDocumentMarkers in non-TextMatchMarkerLists. BUG=707867 ========== to ========== Add CompositionMarkerListImpl This is the first of the specialized DocumentMarkerList implementations. It is currently quite similar to GenericDocumentMarkerListImpl, except it doesn't have the logic for merging Spelling/Grammar markers, and it doesn't implement the RemoveMarkersUnderWords() method on DocumentMarkerList that will eventually be moved to SpellCheckMarkerList. Eventually we will stop creating RenderedDocumentMarkers in non-TextMatchMarkerLists. BUG=707867 Review-Url: https://codereview.chromium.org/2820343004 Cr-Commit-Position: refs/heads/master@{#470183} Committed: https://chromium.googlesource.com/chromium/src/+/45abde5e4bc317fc2e91152b1d66... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/45abde5e4bc317fc2e91152b1d66... |
