|
|
DescriptionAdd SpellCheckMarkerListImpl
This marker list implementation can be used for Spelling and Grammar markers.
It maintains the markers in a sorted list and merges markers whose endpoints
touch.
I'm moving RemoveMarkersUnderWords() from DocumentMarkerList to only be on
SpellCheckMarkerListImpl in this CL and adding a cast in
DocumentMarkerController to enable this.
BUG=707867
Review-Url: https://codereview.chromium.org/2829543002
Cr-Commit-Position: refs/heads/master@{#470847}
Committed: https://chromium.googlesource.com/chromium/src/+/5107b8dc6ef983872e59f544f14fb68434025cf0
Patch Set 1 #Patch Set 2 : Remove type casts that aren't currently needed #Patch Set 3 : Rebase, add test for RemoveMarkersUnderWords() #Patch Set 4 : Convert DoesNotOverlap to lambda for consistency #Patch Set 5 : Rebase, add comments #Patch Set 6 : Use DocumentMarkerListEditor::AddMarkerAndMergeOverlapping() #Patch Set 7 : Clean up DocumentMarkerList/GenericDMLImpl #
Total comments: 6
Patch Set 8 : Respond to comments #Patch Set 9 : Actually use DMLEditor implementation in SpellCheckMarkerListImpl::Add() #Patch Set 10 : Leave method implementations in DocumentMarkerListEditor but change signatures #
Total comments: 5
Patch Set 11 : Add TODOs #Patch Set 12 : Don't update DocumentMarkerControllerTest in this CL #
Total comments: 3
Patch Set 13 : Add newline before DECLARE_VIRTUAL_TRACE(), split off RemoveMarkesrUnderWords() changes #Patch Set 14 : Remove comment about RemoveMarkersUnderWords() being SpellCheckMarkerListImpl-specific (it's not ye… #Depends on Patchset: Dependent Patchsets: Messages
Total messages: 84 (58 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 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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Message was sent while issue was closed.
Description was changed from ========== Add SpellCheckMarkerListImpl This marker list implementation can be used for Spelling and Grammar markers. It maintains the markers in a sorted list and merges markers whose endpoints touch. In a later CL, I think we should add a RemoveMarkersForWords() method to this clas so we can clean up DocumentMarkerController::RemoveMarkers(const MarkerRemoverPredicate&). BUG=707867 ========== to ========== Add SpellCheckMarkerListImpl This marker list implementation can be used for Spelling and Grammar markers. It maintains the markers in a sorted list and merges markers whose endpoints touch. I'm moving RemoveMarkersUnderWords() from DocumentMarkerList to only be on SpellCheckMarkerListImpl in this CL and adding a cast in DocumentMarkerController to enable this. 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 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: 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 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 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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: 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 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: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
https://codereview.chromium.org/2829543002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp (left): https://codereview.chromium.org/2829543002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp:85: bool DocumentMarkerListEditor::RemoveMarkersUnderWords( Could you modify this function in-place, and move it to SpellCheckMarkerListImpl.cpp in patch #5.5? https://codereview.chromium.org/2829543002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp:131: void DocumentMarkerListEditor::AddMarkerAndMergeOverlapping( Could you modify this function in-place, and move it to SpellCheckMarkerListImpl.cpp in patch #5.5? https://codereview.chromium.org/2829543002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/SpellCheckMarkerListImpl.h (right): https://codereview.chromium.org/2829543002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/SpellCheckMarkerListImpl.h:32: DECLARE_TRACE(); Should it be DECLARE_VIRTUAL_TRACE? Should we fix it in GenDMLImpl? https://codereview.chromium.org/2829543002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/SpellCheckMarkerListImpl.h:45: DEFINE_TYPE_CASTS(SpellCheckMarkerListImpl, I prefer using static_cast at the (only) call site than defining a dangerous cast function.
https://codereview.chromium.org/2829543002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp (left): https://codereview.chromium.org/2829543002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp:131: void DocumentMarkerListEditor::AddMarkerAndMergeOverlapping( On 2017/04/27 at 19:04:01, Xiaocheng wrote: > Could you modify this function in-place, and move it to SpellCheckMarkerListImpl.cpp in patch #5.5? I don't think I'm modifying either of these methods, just moving them; I can move them in a different CL if you want though https://codereview.chromium.org/2829543002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/SpellCheckMarkerListImpl.h (right): https://codereview.chromium.org/2829543002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/SpellCheckMarkerListImpl.h:32: DECLARE_TRACE(); On 2017/04/27 at 19:04:01, Xiaocheng wrote: > Should it be DECLARE_VIRTUAL_TRACE? > > Should we fix it in GenDMLImpl? I think it only really matters in the base class; in the derived classes, it's just a style thing (I'm not sure what the convention was). I was thinking it's like how we don't write "virtual" when we declare subclass implementations (we do write "override" though which I guess fulfills the same purpose). The example Oilpan documentation uses DECLARE_VIRTUAL_TRACE in the derived class as well: https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So... I'll change it here, I don't care too much about GenDMLImpl since we're going to remove it soon...
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
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: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Updated again after talking with xiaochengh
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks. No more comments other than those inlined below. https://codereview.chromium.org/2829543002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp (left): https://codereview.chromium.org/2829543002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp:75: MarkerController().AddMarker(range.StartPosition(), range.EndPosition(), Are these the only sites creating markers of wrong types? It seems better to clean up all of them in a separate patch. https://codereview.chromium.org/2829543002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp (right): https://codereview.chromium.org/2829543002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp:86: bool SpellCheckMarkerListImpl::RemoveMarkersUnderWords( nit: Please add a TODO for moving it to SCMLImpl.cpp. https://codereview.chromium.org/2829543002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp:131: void SpellCheckMarkerListImpl::Add(DocumentMarker* marker) { nit: Please add a TODO for moving it to SCMLImpl.cpp. https://codereview.chromium.org/2829543002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/SpellCheckMarkerListImpl.h (right): https://codereview.chromium.org/2829543002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/SpellCheckMarkerListImpl.h:32: DECLARE_VIRTUAL_TRACE(); Please make sure all other impls use DECLARE_VIRTUAL_TRACE
https://codereview.chromium.org/2829543002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp (left): https://codereview.chromium.org/2829543002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp:75: MarkerController().AddMarker(range.StartPosition(), range.EndPosition(), On 2017/04/27 at 22:04:38, Xiaocheng wrote: > Are these the only sites creating markers of wrong types? > > It seems better to clean up all of them in a separate patch. No, these are actually correctly creating Spelling markers, but we actually want TextMatch markers because those are eventually going to be the only ones with RenderedRects. We could do this later (when we get rid of the RenderedRects in the non-TextMatch marker list impls) if we want.
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
On 2017/04/27 at 22:22:23, rlanday wrote: > No, these are actually correctly creating Spelling markers, but we actually want TextMatch > markers because those are eventually going to be the only ones with RenderedRects. We > could do this later (when we get rid of the RenderedRects in the non-TextMatch marker list > impls) if we want. I've added the TODOs, I'm leaving the test updates here for consistency since we're also updating the tests in the CompositionMarkerListImpl CL
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.
On 2017/04/27 at 22:22:23, rlanday wrote: > https://codereview.chromium.org/2829543002/diff/180001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp (left): > > https://codereview.chromium.org/2829543002/diff/180001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerControllerTest.cpp:75: MarkerController().AddMarker(range.StartPosition(), range.EndPosition(), > On 2017/04/27 at 22:04:38, Xiaocheng wrote: > > Are these the only sites creating markers of wrong types? > > > > It seems better to clean up all of them in a separate patch. > > No, these are actually correctly creating Spelling markers, but we actually want TextMatch > markers because those are eventually going to be the only ones with RenderedRects. We > could do this later (when we get rid of the RenderedRects in the non-TextMatch marker list > impls) if we want. Please do this later when cleaning up RenderedRect/RenderedDocumentMarker-related stuff. No other comments.
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
On 2017/04/28 at 18:21:40, xiaochengh wrote: > Please do this later when cleaning up RenderedRect/RenderedDocumentMarker-related stuff. > > No other comments. Updated
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_...)
lgtm
https://codereview.chromium.org/2829543002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2829543002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:519: static_cast<SpellCheckMarkerListImpl* const>(list) nit: s/ const// We should use DEFINE_TYPE_CAST(), in "wtf/Assertion.h" for downcasting. See Document.h for small example: DEFINE_TYPE_CASTS(Document, ExecutionContext, context, context->IsDocument(), https://codereview.chromium.org/2829543002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h (left): https://codereview.chromium.org/2829543002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerList.h:43: virtual bool RemoveMarkersUnderWords(const String& node_text, Could you make this patch into two patches? 1. Introduce SpellinMarkerListImpl 2. Move AddMarkerAndMergeOverlapping() 3. Introduce type case (DEFINE_TYPE_CASTS) for DocumentMarkerList 4. Move RemoveMarkersUnderWords() https://codereview.chromium.org/2829543002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/SpellCheckMarkerListImpl.h (right): https://codereview.chromium.org/2829543002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/SpellCheckMarkerListImpl.h:32: DECLARE_VIRTUAL_TRACE(); nit: Could you add a blank line before |DECLARE_VIRTUAL_TRACE()|?
On 2017/05/08 at 04:50:55, yosin wrote: > https://codereview.chromium.org/2829543002/diff/220001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): > > https://codereview.chromium.org/2829543002/diff/220001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:519: static_cast<SpellCheckMarkerListImpl* const>(list) > nit: s/ const// > > We should use DEFINE_TYPE_CAST(), in "wtf/Assertion.h" for downcasting. That's what I had originally, but xiaochengh didn't like it because I didn't actually have a method on the list for checking if the cast was safe or not: https://codereview.chromium.org/2829543002#msg30 I had: DEFINE_TYPE_CASTS(SpellCheckMarkerListImpl, DocumentMarkerList, list, true, true) Is this OK or should I add an IsSpellCheckMarkerList() method on DocumentMarkerList or something?
Ok, I've removed the RemoveMarkersUnderWords() changes from this CL
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...
still looks good.
On 2017/05/08 at 22:13:46, Xiaocheng wrote: > still looks good. What's your current plan for DMC#5.x patches?
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...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/05/08 at 22:15:18, xiaochengh wrote: > On 2017/05/08 at 22:13:46, Xiaocheng wrote: > > still looks good. > > What's your current plan for DMC#5.x patches? I think I'm going to do what yosin suggested: 5. Introduce SpellinMarkerListImpl 5.5. Move AddMarkerAndMergeOverlapping() 5.6. Introduce type case (DEFINE_TYPE_CASTS) for DocumentMarkerList 5.7 (+5.8?). Move RemoveMarkersUnderWords()
lgtm Thanks!
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 Link to the patchset: https://codereview.chromium.org/2829543002/#ps260001 (title: "Remove comment about RemoveMarkersUnderWords() being SpellCheckMarkerListImpl-specific (it's not ye…")
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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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": 260001, "attempt_start_ts": 1494475049916750, "parent_rev": "3a6336392c7c5c16279bcb773012fd40d6970e82", "commit_rev": "5107b8dc6ef983872e59f544f14fb68434025cf0"}
Message was sent while issue was closed.
Description was changed from ========== Add SpellCheckMarkerListImpl This marker list implementation can be used for Spelling and Grammar markers. It maintains the markers in a sorted list and merges markers whose endpoints touch. I'm moving RemoveMarkersUnderWords() from DocumentMarkerList to only be on SpellCheckMarkerListImpl in this CL and adding a cast in DocumentMarkerController to enable this. BUG=707867 ========== to ========== Add SpellCheckMarkerListImpl This marker list implementation can be used for Spelling and Grammar markers. It maintains the markers in a sorted list and merges markers whose endpoints touch. I'm moving RemoveMarkersUnderWords() from DocumentMarkerList to only be on SpellCheckMarkerListImpl in this CL and adding a cast in DocumentMarkerController to enable this. BUG=707867 Review-Url: https://codereview.chromium.org/2829543002 Cr-Commit-Position: refs/heads/master@{#470847} Committed: https://chromium.googlesource.com/chromium/src/+/5107b8dc6ef983872e59f544f14f... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:260001) as https://chromium.googlesource.com/chromium/src/+/5107b8dc6ef983872e59f544f14f... |