|
|
DescriptionAdd DocumentMarkerController::FirstMarkerIntersectingOffsetRange()
This is a method that takes a text node, a pair of start/end offsets, and a list
of MarkerTypes, and tries to find a DocumentMarker in the node of one of those
types that intersects the range [start_offset, end_offset].
This is similar to DocumentMarkerController::MarkersIntersectingRange(), which
I'm introducing in https://codereview.chromium.org/2948133004, except that this
method takes a Text node and offsets instead of taking an EphemeralRange/
EphemeralRangeInFlatTree, and only returns at most one DocumentMarker, instead
of returning all of them that match.
BUG=707867
Review-Url: https://codereview.chromium.org/2960473002
Cr-Commit-Position: refs/heads/master@{#488549}
Committed: https://chromium.googlesource.com/chromium/src/+/a959e15bb4141aaf7041b2bbfd42f2d6d72cfc66
Patch Set 1 #Patch Set 2 : Update comment, Empty => Collapsed #Patch Set 3 : Rebase #
Total comments: 1
Patch Set 4 : Rebase #Patch Set 5 : Rename method #Patch Set 6 : Remove dependency #Patch Set 7 : Rebase #Patch Set 8 : Rename MarkerIntersectingCollapsedRange test to FirstMarkerIntersectingOffsetRange_collapsed #
Total comments: 2
Patch Set 9 : Use new DocumentMarkerList::FirstMarkerIntersectingRange() method #
Total comments: 1
Patch Set 10 : Try to clarify comment #Patch Set 11 : Rebase #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 57 (42 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
On 2017/06/23 at 21:36:35, rlanday wrote: > Sorry I'm lost... Too many patches... Could you write a summary of the ultimate objective of these patches related to MarkersIntersectingRange, and where each patch fits in the full plan?
On 2017/06/23 at 22:13:23, xiaochengh wrote: > On 2017/06/23 at 21:36:35, rlanday wrote: > > > > Sorry I'm lost... Too many patches... > > Could you write a summary of the ultimate objective of these patches related to MarkersIntersectingRange, and where each patch fits in the full plan? My main goal is to be able to efficiently implement TextSuggestionController::HandlePotentialTextSuggestionTap() in my Android spell check CL (https://codereview.chromium.org/2931443003). I'm also trying to clean up some spell check code that I've polluted by adding stuff I no longer need. Step 1: add some methods to DocumentMarkerController so we can get the markers intersecting/touching the selection without copying the whole list out and doing a linear scan. Add MarkersIntersectingRange() to DocumentMarkerList: https://codereview.chromium.org/2952953002 Add MarkersIntersectingRange() to DocumentMarkerController (calls the one on DocumentMarkerList for each type/node pair): https://codereview.chromium.org/2948133004 yosin said we should also have a MarkerIntersectingRange() method that takes a single node and returns a single marker, so I'm adding that too: https://codereview.chromium.org/2960473002 Step 2: clean up the spell check code. Make SpellChecker::GetSpellCheckMarkerTouchingSelection() not handle the Android-only case where you touch the endpoint of a word (we're going to handle that in TextSuggestionController): https://codereview.chromium.org/2954763002 Now I need some more CLs I haven't written yet: - Make GetSpellCheckMarkerTouchingSelection() use DMC::MarkerIntersectingRange() - Make ContextMenuClient::ReplaceMisspelledRange() use GetSpellCheckMarkerTouchingSelection() After this, I'm going to go back to working on the editing code in my Android spell check menu CL: https://codereview.chromium.org/2931443003 I put up a couple other CLs to fix minor things I noticed while I was working on the other CLs: - Optimize DMC::MarkerAtPosition() using DML::MarkersIntersectingRange(): https://codereview.chromium.org/2949333002 - Clean up DMC::MarkersFor()
On 2017/06/23 at 22:25:15, rlanday wrote: > - Make ContextMenuClient::ReplaceMisspelledRange() use GetSpellCheckMarkerTouchingSelection() sorry, I meant ContextMenuClient::SelectMisspellingAsync()
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
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...
No problem with code change. https://codereview.chromium.org/2960473002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2960473002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:101: DocumentMarker* MarkerIntersectingRange(const Text&, I prefer naming it more different from MarkersIntersectiongRange to reduce confusion, since these two functions are quite different. How about FirstMarkerIntersectingTextOffsetRange? Or any shorter name that you can come up with.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/26 at 22:44:18, xiaochengh wrote: > No problem with code change. > > https://codereview.chromium.org/2960473002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): > > https://codereview.chromium.org/2960473002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:101: DocumentMarker* MarkerIntersectingRange(const Text&, > I prefer naming it more different from MarkersIntersectiongRange to reduce confusion, since these two functions are quite different. > > How about FirstMarkerIntersectingTextOffsetRange? Or any shorter name that you can come up with. Your suggestion seems clear enough to me, although it's kind of long; yosin@, do you have an opinion?
Description was changed from ========== Add DocumentMarkerController::MarkerIntersectingRange() This is a method that takes a text node, a pair of start/end offsets, and a list of MarkerTypes, and tries to find a DocumentMarker in the node of one of those types that intersects the range [start_offset, end_offset]. This is similar to DocumentMarkerController::MarkersIntersectingRange(), which I'm introducing in https://codereview.chromium.org/2948133004, except that this method takes a Text node and offsets instead of taking an EphemeralRange/ EphemeralRangeInFlatTree, and only returns at most one DocumentMarker, instead of returning all of them that match. BUG=707867 ========== to ========== Add DocumentMarkerController::FirstMarkerIntersectingOffsetRange() This is a method that takes a text node, a pair of start/end offsets, and a list of MarkerTypes, and tries to find a DocumentMarker in the node of one of those types that intersects the range [start_offset, end_offset]. This is similar to DocumentMarkerController::MarkersIntersectingRange(), which I'm introducing in https://codereview.chromium.org/2948133004, except that this method takes a Text node and offsets instead of taking an EphemeralRange/ EphemeralRangeInFlatTree, and only returns at most one DocumentMarker, instead of returning all of them that match. BUG=707867 ==========
On 2017/06/28 at 01:59:44, rlanday wrote: > On 2017/06/26 at 22:44:18, xiaochengh wrote: > > No problem with code change. > > > > https://codereview.chromium.org/2960473002/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): > > > > https://codereview.chromium.org/2960473002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:101: DocumentMarker* MarkerIntersectingRange(const Text&, > > I prefer naming it more different from MarkersIntersectiongRange to reduce confusion, since these two functions are quite different. > > > > How about FirstMarkerIntersectingTextOffsetRange? Or any shorter name that you can come up with. > > Your suggestion seems clear enough to me, although it's kind of long; yosin@, do you have an opinion? I have renamed the method to FirstMarkerIntersectingOffsetRange()
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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
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: 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: This issue passed the CQ dry run.
https://codereview.chromium.org/2960473002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2960473002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:419: list->MarkersIntersectingRange(start_offset, end_offset); We should have DocumentMarjerList::FirstMarkerIntersectingOffsetRange() instead of taking first one from list of markers. Except for TextSuggestion marker, we don't need to have list of markes intersecting range. https://codereview.chromium.org/2960473002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2960473002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:101: DocumentMarker* FirstMarkerIntersectingOffsetRange( Can we mark |FirstMarkerIntersectingOffsetRange()| as |const| function?
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...
On 2017/07/20 at 01:07:01, yosin wrote: > https://codereview.chromium.org/2960473002/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): > > https://codereview.chromium.org/2960473002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:419: list->MarkersIntersectingRange(start_offset, end_offset); > We should have DocumentMarjerList::FirstMarkerIntersectingOffsetRange() instead of > taking first one from list of markers. Except for TextSuggestion marker, we don't > need to have list of markes intersecting range. Ok, I put up a CL to add that method (https://codereview.chromium.org/2982313002) and rebased this one on top. > https://codereview.chromium.org/2960473002/diff/140001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): > > https://codereview.chromium.org/2960473002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:101: DocumentMarker* FirstMarkerIntersectingOffsetRange( > Can we mark |FirstMarkerIntersectingOffsetRange()| as |const| function? Currently, no, because it calls PossiblyHasMarkers(), which is non-const as it affects possibly_existing_marker_types_. I think we could make possibly_existing_marker_types_ mutable, and then make PossiblyHasMarkers() const. Should I do that?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The function impl is fine, but there's an issue of the specified behavior of the function (commented inline). How about changing the spec to that the function returns an arbitrary marker when there are multiple intersecting the range? https://codereview.chromium.org/2960473002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2960473002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:421: return found_marker; This is not always the first marker. In fact, as markers of different types can overlap, the comparison between overlapping markers is not well-defined, let alone the semantics of "first marker".
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...
On 2017/07/20 at 21:13:49, xiaochengh wrote: > The function impl is fine, but there's an issue of the specified behavior of the function (commented inline). > > How about changing the spec to that the function returns an arbitrary marker when there are multiple intersecting the range? > > https://codereview.chromium.org/2960473002/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): > > https://codereview.chromium.org/2960473002/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:421: return found_marker; > This is not always the first marker. In fact, as markers of different types can overlap, the comparison between overlapping markers is not well-defined, let alone the semantics of "first marker". I've tried to update the comment to be more clear. The method returns the "first" marker as in the first one found, not necessary the first by start or end offset. I think if a caller is picky about which marker is returned, it would most likely want to use a different method that returns all the markers in the range, so I think there shouldn't be too much confusion.
All right, lgtm then
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...
lgtm
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
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/2960473002/#ps200001 (title: "Rebase")
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": 200001, "attempt_start_ts": 1500602048062510, "parent_rev": "f50732fbe5cc6fff535adbdee0f9066e1dcc61e7", "commit_rev": "a959e15bb4141aaf7041b2bbfd42f2d6d72cfc66"}
Message was sent while issue was closed.
Description was changed from ========== Add DocumentMarkerController::FirstMarkerIntersectingOffsetRange() This is a method that takes a text node, a pair of start/end offsets, and a list of MarkerTypes, and tries to find a DocumentMarker in the node of one of those types that intersects the range [start_offset, end_offset]. This is similar to DocumentMarkerController::MarkersIntersectingRange(), which I'm introducing in https://codereview.chromium.org/2948133004, except that this method takes a Text node and offsets instead of taking an EphemeralRange/ EphemeralRangeInFlatTree, and only returns at most one DocumentMarker, instead of returning all of them that match. BUG=707867 ========== to ========== Add DocumentMarkerController::FirstMarkerIntersectingOffsetRange() This is a method that takes a text node, a pair of start/end offsets, and a list of MarkerTypes, and tries to find a DocumentMarker in the node of one of those types that intersects the range [start_offset, end_offset]. This is similar to DocumentMarkerController::MarkersIntersectingRange(), which I'm introducing in https://codereview.chromium.org/2948133004, except that this method takes a Text node and offsets instead of taking an EphemeralRange/ EphemeralRangeInFlatTree, and only returns at most one DocumentMarker, instead of returning all of them that match. BUG=707867 Review-Url: https://codereview.chromium.org/2960473002 Cr-Commit-Position: refs/heads/master@{#488549} Committed: https://chromium.googlesource.com/chromium/src/+/a959e15bb4141aaf7041b2bbfd42... ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://chromium.googlesource.com/chromium/src/+/a959e15bb4141aaf7041b2bbfd42... |