Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2830043002:
[DMC #1.92] Move DMC::RemoveMarkersUnderWords() to DMListEditor (Closed)
DescriptionMove DMC::RemoveMarkersUnderWords() to DMListEditor
Move the implementation of
DocumentMarkerController:RemoveMarkersUnderWords() into a new method on
DocumentMarkerListEditor.
This is because we are going to refactor DocumentMarkerController on top of
DocumentMarkerListEditor, and once we do that, without adding this method,
the only way to implement this will be to have DMC copy all the markers out,
remove some from its local copy of the list, clear the DocumentMarkerList,
and re-add all the markers that were not removed, which we don't think is
acceptable.
Note: this method will eventually be moved from the general
DocumentMarkerList to only being on SpellCheckMethodList, but we need this
DocumentMarkerList until we add that.
BUG=707867
Review-Url: https://codereview.chromium.org/2830043002
Cr-Commit-Position: refs/heads/master@{#466602}
Committed: https://chromium.googlesource.com/chromium/src/+/ce8108b5ae75934aad08fee396d8a3e79a9682bb
Patch Set 1 #Patch Set 2 : Moving directly to DocumentMarkerListEditor.cpp #Patch Set 3 : REbase #Patch Set 4 : Re-uploading because my repo is messed up and won't run trybots #Patch Set 5 : Make the MarkerList* the first param for consistency #
Total comments: 2
Patch Set 6 : Don't pass Text node into DocumentMarkerListEditor #Patch Set 7 : Use correct parent #Patch Set 8 : Rebase #
Total comments: 3
Patch Set 9 : Update #
Messages
Total messages: 47 (34 generated)
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: This issue passed the CQ dry run.
Description was changed from ========== Add DocumentMarkerListEditor::RemoveMarkers(..., const MarkerRemoverPredicate&) xiaochengh requested that I move the implementation of this method to DocumentMarkerListEditor rather than implement it by having DMC copy all the markers out, remove some, and re-add them: https://codereview.chromium.org/2820633002#msg26 Note: this method will eventually be moved to SpellCheckMethodList, and MarkerRemoverPredicate will be removed (the method will just take a list of words) BUG=707867 ========== to ========== Add DocumentMarkerListEditor::RemoveMarkersUnderWords() xiaochengh requested that I move the implementation of this method to DocumentMarkerListEditor rather than implement it by having DMC copy all the markers out, remove some, and re-add them: https://codereview.chromium.org/2820633002#msg26 Note: this method will probably eventually be moved from the general DocumentMarkerList to only being on SpellCheckMethodList 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: 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.
Please write a description what this patch changes as main topic >xiaochengh requested that I move the implementation of this method to >DocumentMarkerListEditor rather than ... Doesn't provide useful information to git log reader. We don't need to put it. Better place of this kind of statement is review request instead of patch description.
Description was changed from ========== Add DocumentMarkerListEditor::RemoveMarkersUnderWords() xiaochengh requested that I move the implementation of this method to DocumentMarkerListEditor rather than implement it by having DMC copy all the markers out, remove some, and re-add them: https://codereview.chromium.org/2820633002#msg26 Note: this method will probably eventually be moved from the general DocumentMarkerList to only being on SpellCheckMethodList BUG=707867 ========== to ========== Add DocumentMarkerListEditor::RemoveMarkersUnderWords() xiaochengh requested that I move the implementation of this method to DocumentMarkerListEditor. If we leave it in DocumentMarkerController, once we refactor DMC on top of the DocumentMarkerList interface, the only way to implement the method will be to have DMC copy all the markers out, remove some from its local copy of the list, clear the DocumentMarkerList, and re-add all the markers that were not removed. Discussion where this was requested: https://codereview.chromium.org/2820633002#msg26 Note: this method will probably eventually be moved from the general DocumentMarkerList to only being on SpellCheckMethodList BUG=707867 ==========
Description was changed from ========== Add DocumentMarkerListEditor::RemoveMarkersUnderWords() xiaochengh requested that I move the implementation of this method to DocumentMarkerListEditor. If we leave it in DocumentMarkerController, once we refactor DMC on top of the DocumentMarkerList interface, the only way to implement the method will be to have DMC copy all the markers out, remove some from its local copy of the list, clear the DocumentMarkerList, and re-add all the markers that were not removed. Discussion where this was requested: https://codereview.chromium.org/2820633002#msg26 Note: this method will probably eventually be moved from the general DocumentMarkerList to only being on SpellCheckMethodList BUG=707867 ========== to ========== Add DocumentMarkerListEditor::RemoveMarkersUnderWords() xiaochengh requested that I move the implementation of this method to DocumentMarkerListEditor, since if we leave it in DocumentMarkerController, once we refactor DMC on top of the DocumentMarkerList interface, the only way to implement the method will be to have DMC copy all the markers out, remove some from its local copy of the list, clear the DocumentMarkerList, and re-add all the markers that were not removed. Discussion where this was requested: https://codereview.chromium.org/2820633002#msg26 Note: this method will probably eventually be moved from the general DocumentMarkerList to only being on SpellCheckMethodList BUG=707867 ==========
Description was changed from ========== Add DocumentMarkerListEditor::RemoveMarkersUnderWords() xiaochengh requested that I move the implementation of this method to DocumentMarkerListEditor, since if we leave it in DocumentMarkerController, once we refactor DMC on top of the DocumentMarkerList interface, the only way to implement the method will be to have DMC copy all the markers out, remove some from its local copy of the list, clear the DocumentMarkerList, and re-add all the markers that were not removed. Discussion where this was requested: https://codereview.chromium.org/2820633002#msg26 Note: this method will probably eventually be moved from the general DocumentMarkerList to only being on SpellCheckMethodList BUG=707867 ========== to ========== Add DocumentMarkerListEditor::RemoveMarkersUnderWords() If we leave the implementation of this method in DocumentMarkerController, once we refactor DMC on top of the DocumentMarkerList interface, the only way to implement the method will be to have DMC copy all the markers out, remove some from its local copy of the list, clear the DocumentMarkerList, and re-add all the markers that were not removed. I am therefore moving the implementation to DocumentMarkerListEditor in this CL (and later to DocumentMarkerList) so we don't have to do that. Note: this method will probably eventually be moved from the general DocumentMarkerList to only being on SpellCheckMethodList BUG=707867 ==========
On 2017/04/21 at 08:32:21, yosin wrote: > Please write a description what this patch changes as main topic > > >xiaochengh requested that I move the implementation of this method to > >DocumentMarkerListEditor rather than ... > > Doesn't provide useful information to git log reader. We don't need to > put it. > > Better place of this kind of statement is review request instead of > patch description. Ok, I have updated the description
https://codereview.chromium.org/2830043002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.h (right): https://codereview.chromium.org/2830043002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.h:32: const Text& node, Can we change this to a String? I don't want DMLEditor to depend on DOM.
https://codereview.chromium.org/2830043002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.h (right): https://codereview.chromium.org/2830043002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.h:32: const Text& node, On 2017/04/21 at 08:41:45, Xiaocheng wrote: > Can we change this to a String? I don't want DMLEditor to depend on DOM. Ok
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
On 2017/04/21 at 08:42:22, rlanday wrote: > https://codereview.chromium.org/2830043002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.h (right): > > https://codereview.chromium.org/2830043002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.h:32: const Text& node, > On 2017/04/21 at 08:41:45, Xiaocheng wrote: > > Can we change this to a String? I don't want DMLEditor to depend on DOM. > > Ok 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: linux_chromium_chromeos_ozone_rel_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...)
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.
Code changes seems to be good. Please update description. I think patch description generally should be: 0. Summary = changes by this patch 1. Changes made by this patch 2. Goal of the patch 3. Benefit of the patch for Chrome/Blink 4. Background/Details For this patch, summary should be "Move DMC::RemoveMarkersUnderWords() to DMListEditor". (no abbrev in real summary). Because of this patch *just* does moving code around w/ some cosmetic changes. https://codereview.chromium.org/2830043002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp (right): https://codereview.chromium.org/2830043002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp:97: const DocumentMarker& marker = *list->at(j - 1); nit: s/*list->at(j - 1)/*list[j - 1]/ We don't need to use |std::vector::at()|. https://codereview.chromium.org/2830043002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp:100: nit: We don't need to have an extra blank line.
Description was changed from ========== Add DocumentMarkerListEditor::RemoveMarkersUnderWords() If we leave the implementation of this method in DocumentMarkerController, once we refactor DMC on top of the DocumentMarkerList interface, the only way to implement the method will be to have DMC copy all the markers out, remove some from its local copy of the list, clear the DocumentMarkerList, and re-add all the markers that were not removed. I am therefore moving the implementation to DocumentMarkerListEditor in this CL (and later to DocumentMarkerList) so we don't have to do that. Note: this method will probably eventually be moved from the general DocumentMarkerList to only being on SpellCheckMethodList BUG=707867 ========== to ========== Move DMC::RemoveMarkersUnderWords() to DMListEditor Move the implementation of DocumentMarkerController:RemoveMarkersUnderWords() into a new method on DocumentMarkerListEditor. This is because we are going to refactor DocumentMarkerController on top of DocumentMarkerListEditor, and once we do that, without adding this method, the only way to implement this will be to have DMC copy all the markers out, remove some from its local copy of the list, clear the DocumentMarkerList, and re-add all the markers that were not removed, which we don't think is acceptable. Note: this method will eventually be moved from the general DocumentMarkerList to only being on SpellCheckMethodList, but we need this DocumentMarkerList until we add that. BUG=707867 ==========
https://codereview.chromium.org/2830043002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp (right): https://codereview.chromium.org/2830043002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp:97: const DocumentMarker& marker = *list->at(j - 1); On 2017/04/24 at 04:00:44, yosin_UTC9 wrote: > nit: s/*list->at(j - 1)/*list[j - 1]/ > > We don't need to use |std::vector::at()|. If we don't use at(), we actually have to do: *(*list)[j - 1] Is that OK?
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
On 2017/04/24 at 05:40:45, rlanday wrote: > https://codereview.chromium.org/2830043002/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp (right): > > https://codereview.chromium.org/2830043002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerListEditor.cpp:97: const DocumentMarker& marker = *list->at(j - 1); > On 2017/04/24 at 04:00:44, yosin_UTC9 wrote: > > nit: s/*list->at(j - 1)/*list[j - 1]/ > > > > We don't need to use |std::vector::at()|. > > If we don't use at(), we actually have to do: > *(*list)[j - 1] > > Is that OK? OK. We don't want to do bounds checking in std::vector<T>::at().
lgtm
The CQ bit was unchecked by rlanday@chromium.org
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": 160001, "attempt_start_ts": 1493013725814800,
"parent_rev": "fd61327a91c26467fae63304cc1ee343405344e6", "commit_rev":
"ce8108b5ae75934aad08fee396d8a3e79a9682bb"}
Message was sent while issue was closed.
Description was changed from ========== Move DMC::RemoveMarkersUnderWords() to DMListEditor Move the implementation of DocumentMarkerController:RemoveMarkersUnderWords() into a new method on DocumentMarkerListEditor. This is because we are going to refactor DocumentMarkerController on top of DocumentMarkerListEditor, and once we do that, without adding this method, the only way to implement this will be to have DMC copy all the markers out, remove some from its local copy of the list, clear the DocumentMarkerList, and re-add all the markers that were not removed, which we don't think is acceptable. Note: this method will eventually be moved from the general DocumentMarkerList to only being on SpellCheckMethodList, but we need this DocumentMarkerList until we add that. BUG=707867 ========== to ========== Move DMC::RemoveMarkersUnderWords() to DMListEditor Move the implementation of DocumentMarkerController:RemoveMarkersUnderWords() into a new method on DocumentMarkerListEditor. This is because we are going to refactor DocumentMarkerController on top of DocumentMarkerListEditor, and once we do that, without adding this method, the only way to implement this will be to have DMC copy all the markers out, remove some from its local copy of the list, clear the DocumentMarkerList, and re-add all the markers that were not removed, which we don't think is acceptable. Note: this method will eventually be moved from the general DocumentMarkerList to only being on SpellCheckMethodList, but we need this DocumentMarkerList until we add that. BUG=707867 Review-Url: https://codereview.chromium.org/2830043002 Cr-Commit-Position: refs/heads/master@{#466602} Committed: https://chromium.googlesource.com/chromium/src/+/ce8108b5ae75934aad08fee396d8... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/ce8108b5ae75934aad08fee396d8... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
