|
|
DescriptionRemove DocumentMarkerController::MarkersInRange()
Currently, MarkersInRange() is kind of broken in that it can potentially return
DocumentMarkers from multiple Nodes, and it doesn't return any information about
which Node each DocumentMarker is associated with. This means that callers are
currently resorting to dubious methods to attempt to reconstruct the
EphemeralRange associated with the returned marker(s).
After discussing with xiaochengh@ and yosin@, instead of changing the method to
also return the marker's Node or EphemeralRange, we decided to remove the method
and move the logic directly into the callers.
BUG=707867
Review-Url: https://codereview.chromium.org/2857173003
Cr-Commit-Position: refs/heads/master@{#471642}
Committed: https://chromium.googlesource.com/chromium/src/+/3639139a6216af6f66840f8348622d45ba06d5b9
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove MarkersInRange() #
Total comments: 3
Patch Set 3 : Fix logic #Patch Set 4 : Fix WebFrameTest.cpp #
Total comments: 7
Patch Set 5 : Simplify unnecessarily complicated logic #
Total comments: 4
Patch Set 6 : Use std::find_if() #
Total comments: 10
Patch Set 7 : Respond to comments #
Total comments: 2
Patch Set 8 : Remove Optional accidentally left in #Patch Set 9 : s/const DocumentMarker*/const DocumentMarker* const/ #
Total comments: 3
Patch Set 10 : Use std::find_if() #Messages
Total messages: 77 (35 generated)
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
rlanday@chromium.org changed reviewers: + xiaochengh@chromium.org, yosin@chromium.org
Looks good. Let's wait for yosin@'s comment. Life is hard when your reviewer is on vacation but you still have to work. https://codereview.chromium.org/2857173003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2857173003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:366: Vector<std::pair<Node*, DocumentMarkerVector>> Does changing the return type into Vector& ensure a move-assignment?
https://codereview.chromium.org/2857173003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2857173003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:366: Vector<std::pair<Node*, DocumentMarkerVector>> On 2017/05/04 at 00:31:59, Xiaocheng wrote: > Does changing the return type into Vector& ensure a move-assignment? Seems sketchy/not correct to return a reference to a temporary? Callers can store a const reference to the return value, or the compiler should either do a move assignment or do return value optimization anyway I think...
Do we really need to have DM::MarkersInRange()? It seems MarkesForNode() is enough. # SelectionController::SelectClosestMisspellingFromHitTestResult() DocumentMarkerVector markers = inner_node->GetDocument().Markers().MarkersInRange( EphemeralRange(ToPositionInDOMTree(marker_position)), DocumentMarker::MisspellingMarkers()); if (markes.size() == 1) { ... } Just for |marker_position| instead of range. # HitTestResultIsMisspelled() return inner_node->GetDocument() .Markers() .MarkersInRange( EphemeralRange( pos.DeepEquivalent().ParentAnchoredEquivalent()), DocumentMarker::MisspellingMarkers()) .size() > 0; Just for |pos|. # SpellChecker::ReplaceMisspelledRange() It seems it look only first marker of MarkersInRange(). We should make change this function handle "<b>d</b>ox" case. # SelectMisspellingAsync() // Caret and range selections always return valid normalized ranges. Range* selection_range = CreateRange(selection.ToNormalizedEphemeralRange()); DocumentMarkerVector markers = selected_frame->GetDocument()->Markers().MarkersInRange( EphemeralRange(selection_range), DocumentMarker::MisspellingMarkers()); if (markers.size() != 1) return String(); description = markers[0]->Description(); Iterator over node in range to get marker and use first found marker?
Can you please explain the "<b>d</b>ox" case? I have another use case that complicates things somewhat: on Android, when you tap on a misspelled word or a SuggestionSpan, we want to display the text suggestion menu even if you tap at a position only touching an endpoint of a spelling/suggestion marker (as opposed to on desktop, where when you right-click a misspelled word, you actually have to click in the *interior* of the word to get the spellcheck options in the context menu). xiaochengh and I discovered a complicating factor, which is that if you have a Position e.g. right at the beginning of a text box, the Position is apparently stored relative to the text box element (e.g. <div /> or <textarea />), and not the text node (which is what the DocumentMarker is associated with). The workaround we came up with is to create a range that includes one character on either side of the caret position, and then use MarkersInRange() on this range. Does this sound reasonable, or can you think of a better approach? Should this logic be a method in the TextSuggestionController class I'm adding, or a method on DocumentMarkerController (it would be called something like MarkersTouchingPosition() and it would need to return either the associated Node* or the EphemeralRange associated with the marker).
On 2017/05/08 at 18:35:45, rlanday wrote: > Can you please explain the "<b>d</b>ox" case? "dox" is misspelled word and in multiple nodes. SpellChecker::ReplaceMisspelledRange() should replace "dox" instead of "d". > I have another use case that complicates things somewhat: on Android, when you > tap on a misspelled word or a SuggestionSpan, we want to display the text > suggestion menu even if you tap at a position only touching an endpoint of a > spelling/suggestion marker (as opposed to on desktop, where when you > right-click a misspelled word, you actually have to click in the *interior* of > the word to get the spellcheck options in the context menu). xiaochengh and I > discovered a complicating factor, which is that if you have a Position e.g. > right at the beginning of a text box, the Position is apparently stored > relative to the text box element (e.g. <div /> or <textarea />), and not the > text node (which is what the DocumentMarker is associated with). The > workaround we came up with is to create a range that includes one character on > either side of the caret position, and then use MarkersInRange() on this range. > > Does this sound reasonable, or can you think of a better approach? Should this > logic be a method in the TextSuggestionController class I'm adding, or a method > on DocumentMarkerController (it would be called something like > MarkersTouchingPosition() and it would need to return either the associated > Node* or the EphemeralRange associated with the marker). I would like to postpone the decision to avoid over abstraction for simplifying implementation. At this time, only SpellChecker::ReplaceMisspelledRange() requires markers for range. When we have another use case, we can think it at that time. I'm not sure usage of markers are as same as SpellChecker::ReplaceMisspelledRange() and SuggestionSpan() since I don't see implementations of both of them. My guess is for SpellChecker::ReplaceMisspelledRange() for (char in Range) { collect char into word and misspell info if (misspell) { replace with suggestion from marker return; } Anyway, rule of thumb is "Don't do abstraction until we have real use cases".
The "<b>d</b>ox" case is problematic regardless of how ReplaceMisspelledRange() is implemented because we run into the same problem when trying to select the misspelled word in the first place. You can test it (and related cases) by typing this into the URL bar: data:text/html, <div contenteditable><b>d</b>ox</div> I will note two complications with this particular test case: - "dox" is actually included in the macOS dictionary, so it doesn't get flagged as misspelled on my computer (it means "search for and publish private or identifying information about (a particular individual) on the Internet, typically with malicious intent"). - If "dox" is considered misspelled, it would get one marker on the "d" and one on the "ox". If you right-click before or after the "d", we don't show spellcheck suggestions for "d" because we're not actually in the interior of the range (you can't be in the interior of a single-character range). Instead, we select "dox" in this case since right-clicking inside a word selects it by default. We also don't show suggestions for "ox" even if you're between the "d" and the "ox", I guess because the Position is anchored relative to the parent node right after the <b> element, not in the text node. However, if you right-click between the "o" and "x", then we highlight "ox" (which is also actually a real word). If there are two characters inside the <b> element and you right-click between them, the first two characters of the word get highlighted. I do not know how to properly support this case except by rewriting DocumentMarker to be more like how DOM Range works (the start and end positions can be anchored relative to different nodes); this would probably involve a fair amount of work.
On 2017/05/09 at 04:25:49, rlanday wrote: > The "<b>d</b>ox" case is problematic regardless of how ReplaceMisspelledRange() is implemented > because we run into the same problem when trying to select the misspelled word in the first > place. > > You can test it (and related cases) by typing this into the URL bar: > data:text/html, <div contenteditable><b>d</b>ox</div> > > I will note two complications with this particular test case: > > - "dox" is actually included in the macOS dictionary, so it doesn't get flagged as misspelled > on my computer (it means "search for and publish private or identifying information about (a > particular individual) on the Internet, typically with malicious intent"). > > - If "dox" is considered misspelled, it would get one marker on the "d" and one on the "ox". > If you right-click before or after the "d", we don't show spellcheck suggestions for "d" > because we're not actually in the interior of the range (you can't be in the interior of a > single-character range). Instead, we select "dox" in this case since right-clicking inside > a word selects it by default. > > We also don't show suggestions for "ox" even if you're between the "d" and the "ox", I guess > because the Position is anchored relative to the parent node right after the <b> element, not > in the text node. However, if you right-click between the "o" and "x", then we highlight "ox" > (which is also actually a real word). > > If there are two characters inside the <b> element and you right-click between them, the first > two characters of the word get highlighted. > > > I do not know how to properly support this case except by rewriting DocumentMarker to be more > like how DOM Range works (the start and end positions can be anchored relative to different > nodes); this would probably involve a fair amount of work. Sorry for confusion. "dox" is bad example. I could present a word spanning into multiple Text nodes. I don't say support "d<b>ox</b>" to replace "dog" now, since it is rare case and no people say should so. However, rewriting SpellChecker::ReplaceMisspelledRange() and others to use DMC::MarkersForNode() allows us getting rid of DMC::MarkersInRange() at this time. It is nice to do so now.
Hmm, so we actually have multiple problems here 1. Stop using the ill-formed method MarkersInRange() (or make it well-formed) 2. Fix the implementation of SpellChecker::ReplaceMisspelledRange when a misspelling spans multiple nodes 3. Support the use case that, given a position p, return any marker covering (with endpoints included) a position that is visually equivalent to p For 1, using MarkersFor(Node*) seems a clean solution. Note that all current callers of MarkersInRange() work only when the range's start and end positions have the same container node. 2 seems rather low priority. Let's leave a P3/Available bug at crbug.com and add a comment in the code. 3 has some cases (assuming that p is parent-anchored): - 3.0 (Easy) p is anchored at a text node - 3.1 p is anchored at a non-text node - 3.1.1 (Seems easy) p is visually covered by at most one marker - 3.1.2 p is in the middle of two markers that visually touching each other The question is that, do we want to support case 3.1.2? I guess we can't handle 3.1.2 correctly if we decide not to fix problem 2. Since 2 is low priority, we can leave case 3.1.2 unfixed for now. Then the remaining problem seems easy as we already have solution to 3.1.1.
On 2017/05/09 at 18:57:12, xiaochengh wrote: > 2 seems rather low priority. Let's leave a P3/Available bug at crbug.com and add a comment in the code. I've filed a bug here: https://bugs.chromium.org/p/chromium/issues/detail?id=720065 > 3 has some cases (assuming that p is parent-anchored): > - 3.0 (Easy) p is anchored at a text node > - 3.1 p is anchored at a non-text node > - 3.1.1 (Seems easy) p is visually covered by at most one marker > - 3.1.2 p is in the middle of two markers that visually touching each other > > The question is that, do we want to support case 3.1.2? I guess we can't handle 3.1.2 correctly if we decide not to fix problem 2. Since 2 is low priority, we can leave case 3.1.2 unfixed for now. Then the remaining problem seems easy as we already have solution to 3.1.1. I think if the ranges are "visually touching each other", really the markers on two halves of a single word, which I agree is related to problem 2 (ideally it would be one marker on the whole word). So let's punt on this case.
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...
Description was changed from ========== Fix DocumentMarkerController::MarkersInRange() API Currently, MarkersInRange() is kind of broken in that it can potentially return DocumentMarkers from multiple Nodes, and it doesn't return any information about which Node each DocumentMarker is associated with. This means that callers are currently resorting to dubious methods to attempt to reconstruct the EphemeralRange associated with the returned marker(s). My first idea for solving this issue was storing the Node* each DocumentMarker is associated with on the DocumentMarker itself (right now it's only stored as a map key in DocumentMarkerController). This would probably provide the cleanest API. However, we'd waste some memory storing the pointers in more than one place. xiaochengh@ came up with this idea of making MarkersInRange() return a Vector of std::pair<Node*, DocumentMarkerVector>. I think this is the most efficient solution, but the API feels rather clunky. We could probably clean it up slightly by using a struct rather than a pair, but that wouldn't change much. Alternatively, as a compromise, we could perhaps create a wrapper class or subclass, called something like DocumentMarkerWithNode, and have MarkersInRange() return that instead. Then we could have a nice, clean helper method on it that computes the EphemeralRange. BUG=707867 ========== to ========== Remove DocumentMarkerController::MarkersInRange() Currently, MarkersInRange() is kind of broken in that it can potentially return DocumentMarkers from multiple Nodes, and it doesn't return any information about which Node each DocumentMarker is associated with. This means that callers are currently resorting to dubious methods to attempt to reconstruct the EphemeralRange associated with the returned marker(s). After discussing with xiaochengh@ and yosin@, instead of changing the method to also return the marker's Node or EphemeralRange, we decided to remove the method and move the logic directly into the callers. BUG=707867 ==========
Ok, I have updated this CL to remove MarkersInRange().
https://codereview.chromium.org/2857173003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/2857173003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SelectionController.cpp:526: if (markers.size() == 1) { This can break something if there are multiple markers on |container_node| but only one covers |marker_position|. Same applies to all other call sites. Maybe we need a helper function that filters out markers in an offset range from a DocumentMarkerVector?
https://codereview.chromium.org/2857173003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/2857173003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SelectionController.cpp:526: if (markers.size() == 1) { On 2017/05/09 at 21:55:15, Xiaocheng wrote: > This can break something if there are multiple markers on |container_node| but only one covers |marker_position|. Same applies to all other call sites. > > Maybe we need a helper function that filters out markers in an offset range from a DocumentMarkerVector? This seems like something we should push back to DocumentMarkerList so we can do it more efficiently (e.g. since the list is sorted, we don't need to copy out every single marker, only the ones in the applicable range). But once we do that, it starts to seem like we should leave MarkersInRange() on DMC after all and just fix the API somehow. yosin: what do you think? Note that the current implementation of MarkersInRange() is kind of ridiculously inefficient; it calls MarkersFor() to get the full list of markers for each node without passing the specified marker types, then filters out the ones for the other types *afterward*...
On 2017/05/09 at 22:04:17, rlanday wrote: > https://codereview.chromium.org/2857173003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/SelectionController.cpp (right): > > https://codereview.chromium.org/2857173003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/SelectionController.cpp:526: if (markers.size() == 1) { > On 2017/05/09 at 21:55:15, Xiaocheng wrote: > > This can break something if there are multiple markers on |container_node| but only one covers |marker_position|. Same applies to all other call sites. > > > > Maybe we need a helper function that filters out markers in an offset range from a DocumentMarkerVector? > > This seems like something we should push back to DocumentMarkerList so we can do it more efficiently (e.g. since the list is sorted, we don't need to copy out every single marker, only the ones in the applicable range). But once we do that, it starts to seem like we should leave MarkersInRange() on DMC after all and just fix the API somehow. > > yosin: what do you think? > > Note that the current implementation of MarkersInRange() is kind of ridiculously inefficient; it calls MarkersFor() to get the full list of markers for each node without passing the specified marker types, then filters out the ones for the other types *afterward*... How about making MarkersInRange take (node, start_offset, end_offset, types) as its arguments?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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-...)
On 2017/05/09 at 22:16:41, xiaochengh wrote: > On 2017/05/09 at 22:04:17, rlanday wrote: > > https://codereview.chromium.org/2857173003/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/editing/SelectionController.cpp (right): > > > > https://codereview.chromium.org/2857173003/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/editing/SelectionController.cpp:526: if (markers.size() == 1) { > > On 2017/05/09 at 21:55:15, Xiaocheng wrote: > > > This can break something if there are multiple markers on |container_node| but only one covers |marker_position|. Same applies to all other call sites. > > > > > > Maybe we need a helper function that filters out markers in an offset range from a DocumentMarkerVector? > > > > This seems like something we should push back to DocumentMarkerList so we can do it more efficiently (e.g. since the list is sorted, we don't need to copy out every single marker, only the ones in the applicable range). But once we do that, it starts to seem like we should leave MarkersInRange() on DMC after all and just fix the API somehow. > > > > yosin: what do you think? > > > > Note that the current implementation of MarkersInRange() is kind of ridiculously inefficient; it calls MarkersFor() to get the full list of markers for each node without passing the specified marker types, then filters out the ones for the other types *afterward*... > > How about making MarkersInRange take (node, start_offset, end_offset, types) as its arguments? I don't particularly like that. It seems kind of error-prone, and I think multiple callers are interested in this pattern, so I think it makes sense to encapsulate more of the logic in DocumentMarkerController. The logic in MarkersInRange() for checking the offsets for the start/end Nodes is too complicated for us to want to have it in multiple places.
https://codereview.chromium.org/2857173003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/2857173003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SelectionController.cpp:526: if (markers.size() == 1) { On 2017/05/09 at 22:04:17, rlanday wrote: > On 2017/05/09 at 21:55:15, Xiaocheng wrote: > > This can break something if there are multiple markers on |container_node| but only one covers |marker_position|. Same applies to all other call sites. > > > > Maybe we need a helper function that filters out markers in an offset range from a DocumentMarkerVector? > > This seems like something we should push back to DocumentMarkerList so we can do it more efficiently (e.g. since the list is sorted, we don't need to copy out every single marker, only the ones in the applicable range). But once we do that, it starts to seem like we should leave MarkersInRange() on DMC after all and just fix the API somehow. > > yosin: what do you think? > > Note that the current implementation of MarkersInRange() is kind of ridiculously inefficient; it calls MarkersFor() to get the full list of markers for each node without passing the specified marker types, then filters out the ones for the other types *afterward*... Good catch! MarkesForNode() isn't equivalent to MakersAtPosition(). DocumentMarkerVector MakersAtPosition(const Position& position, DocumentMakrerTypes mrker_type) { ... for (const auto& marker marker : MakresFor(position.ComputeContainerNode())) { if (marker contains position.ComputeOffsetInContainer()) add to result } ... } We may want to introduce DMList::MarkersAtPosition() to utilized sorted list impl in another patch. It seems introducing DMC::MakersAtPosition() makes this patch smaller since we don't need to call Position::ComputeContainerNode() in each call site.
On 2017/05/10 at 01:46:59, yosin wrote: > https://codereview.chromium.org/2857173003/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/SelectionController.cpp (right): > > https://codereview.chromium.org/2857173003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/SelectionController.cpp:526: if (markers.size() == 1) { > On 2017/05/09 at 22:04:17, rlanday wrote: > > On 2017/05/09 at 21:55:15, Xiaocheng wrote: > > > This can break something if there are multiple markers on |container_node| but only one covers |marker_position|. Same applies to all other call sites. > > > > > > Maybe we need a helper function that filters out markers in an offset range from a DocumentMarkerVector? > > > > This seems like something we should push back to DocumentMarkerList so we can do it more efficiently (e.g. since the list is sorted, we don't need to copy out every single marker, only the ones in the applicable range). But once we do that, it starts to seem like we should leave MarkersInRange() on DMC after all and just fix the API somehow. > > > > yosin: what do you think? > > > > Note that the current implementation of MarkersInRange() is kind of ridiculously inefficient; it calls MarkersFor() to get the full list of markers for each node without passing the specified marker types, then filters out the ones for the other types *afterward*... > > Good catch! MarkesForNode() isn't equivalent to MakersAtPosition(). > > DocumentMarkerVector MakersAtPosition(const Position& position, DocumentMakrerTypes mrker_type) { > ... > for (const auto& marker marker : MakresFor(position.ComputeContainerNode())) { > if (marker contains position.ComputeOffsetInContainer()) > add to result > } > ... > } > > We may want to introduce DMList::MarkersAtPosition() to utilized sorted list impl in another patch. > > It seems introducing DMC::MakersAtPosition() makes this patch smaller since we don't need to > call Position::ComputeContainerNode() in each call site. In fact this can be simplified to MarkerAtPosition() (note the singular form). All current call sites can only handle at most one marker being returned. Guess we use Optional<DocumentMarker> as the return type?
> On 2017/05/10 at 01:46:59, yosin wrote: > In fact this can be simplified to MarkerAtPosition() (note the singular form). All current call sites can only handle at most one marker being returned. Guess we use Optional<DocumentMarker> as the return type? This is a method that only returns markers that include the Position in the interior right, and not markers that have the Position as an endpoint? If so, I agree it would work, but I think we'll also need another method later for my Android work that also returns markers just touching the Position (and this method will probably need to return a Node* with it since the Position isn't necessarily anchored to the text node the marker is on).
On 2017/05/10 at 02:15:23, rlanday wrote: > > On 2017/05/10 at 01:46:59, yosin wrote: > > In fact this can be simplified to MarkerAtPosition() (note the singular form). All current call sites can only handle at most one marker being returned. Guess we use Optional<DocumentMarker> as the return type? > > This is a method that only returns markers that include the Position in the interior right, and not markers that have the Position as an endpoint? Yes, to keep it consistent with what MarkersInRange() currently does. > If so, I agree it would work, but I think we'll also need another method later for my Android work that also returns markers just touching the Position (and this method will probably need to return a Node* with it since the Position isn't necessarily anchored to the text node the marker is on). Given a position anchored at a non-text node, MostFor/BackwardCaretPosition can adjust the position to the next/previous visible text node. Android spellcheck suggestion menu can use them.
On 2017/05/10 at 02:56:21, xiaochengh wrote: > On 2017/05/10 at 02:15:23, rlanday wrote: > > > On 2017/05/10 at 01:46:59, yosin wrote: > > > In fact this can be simplified to MarkerAtPosition() (note the singular form). All current call sites can only handle at most one marker being returned. Guess we use Optional<DocumentMarker> as the return type? > > > > This is a method that only returns markers that include the Position in the interior right, and not markers that have the Position as an endpoint? > > Yes, to keep it consistent with what MarkersInRange() currently does. > > > If so, I agree it would work, but I think we'll also need another method later for my Android work that also returns markers just touching the Position (and this method will probably need to return a Node* with it since the Position isn't necessarily anchored to the text node the marker is on). > > Given a position anchored at a non-text node, MostFor/BackwardCaretPosition can adjust the position to the next/previous visible text node. Android spellcheck suggestion menu can use them. Please focus on current impl only. We'll change or introduce new one when we need to have it to simplify code and discussion. It is hard to image future.
I don't think MarkerAtPosition() is enough. ContextMenuClientImpl::SelectMisspellingAsync() and SpellChecker::ReplaceMisspelledRange() both need to be able to find the range of a spellcheck marker that a selection might include. This needs to work even if the selection starts exactly on the marker (i.e. neither the start nor end position of the selection is in the interior of the marker). So we need to either duplicate this complex operation in both places or add a helper method. I'll update the CL to show what the code looks like if we duplicate it.
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/05/10 at 20:22:45, rlanday wrote: > I don't think MarkerAtPosition() is enough. ContextMenuClientImpl::SelectMisspellingAsync() and SpellChecker::ReplaceMisspelledRange() both need to be able to find the range of a spellcheck marker that a selection might include. This needs to work even if the selection starts exactly on the marker (i.e. neither the start nor end position of the selection is in the interior of the marker). So we need to either duplicate this complex operation in both places or add a helper method. I'll update the CL to show what the code looks like if we duplicate it. Updated; I think there's too much code duplication now
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_...)
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...
https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (left): https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:812: Position(caret_range.StartPosition().ComputeContainerNode(), Note that the existing code doesn't work if the start and end container nodes are different. https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:816: for (Node& node : caret_range.Nodes()) { Since we do not plan to support the case where a misspelling spans multiple nodes, there is no need to iterate over the nodes. Retrieving markers from only |caret_start_container| suffices. https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/ContextMenuClientImpl.cpp (left): https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:125: Range* marker_range = selection_range->cloneRange(); Same here. The existing code doesn't work when the start and end containers of |selection_range| are different. https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/ContextMenuClientImpl.cpp (right): https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:129: for (Node& node : selection_range.Nodes()) { Same here. No need to iterate over all nodes.
https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:816: for (Node& node : caret_range.Nodes()) { On 2017/05/10 at 22:54:22, Xiaocheng wrote: > Since we do not plan to support the case where a misspelling spans multiple nodes, there is no need to iterate over the nodes. Retrieving markers from only |caret_start_container| suffices. I don't think that's correct; the misspelling could occur right at the beginning of the node, in which case caret_start_container might not be anchored to the text node that the DocumentMarker is anchored to. (We might have a similar problem for misspellings occurring at the end of the node if we were to use caret_end_container).
https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:816: for (Node& node : caret_range.Nodes()) { On 2017/05/10 at 23:17:57, rlanday wrote: > On 2017/05/10 at 22:54:22, Xiaocheng wrote: > > Since we do not plan to support the case where a misspelling spans multiple nodes, there is no need to iterate over the nodes. Retrieving markers from only |caret_start_container| suffices. > > I don't think that's correct; the misspelling could occur right at the beginning of the node, in which case caret_start_container might not be anchored to the text node that the DocumentMarker is anchored to. (We might have a similar problem for misspellings occurring at the end of the node if we were to use caret_end_container). 1. caret_range has been adjusted by ToNormalizedEphemeralRange, which should have its start and end anchored at text nodes. 2. The old code doesn't work either if caret_range is not anchored directly at the text node with the misspelling.
https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:816: for (Node& node : caret_range.Nodes()) { On 2017/05/10 at 23:23:42, Xiaocheng wrote: > On 2017/05/10 at 23:17:57, rlanday wrote: > > On 2017/05/10 at 22:54:22, Xiaocheng wrote: > > > Since we do not plan to support the case where a misspelling spans multiple nodes, there is no need to iterate over the nodes. Retrieving markers from only |caret_start_container| suffices. > > > > I don't think that's correct; the misspelling could occur right at the beginning of the node, in which case caret_start_container might not be anchored to the text node that the DocumentMarker is anchored to. (We might have a similar problem for misspellings occurring at the end of the node if we were to use caret_end_container). > > 1. caret_range has been adjusted by ToNormalizedEphemeralRange, which should have its start and end anchored at text nodes. > > 2. The old code doesn't work either if caret_range is not anchored directly at the text node with the misspelling. Ah, OK, I'll change it
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
On 2017/05/10 at 23:25:43, rlanday wrote: > https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): > > https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:816: for (Node& node : caret_range.Nodes()) { > On 2017/05/10 at 23:23:42, Xiaocheng wrote: > > On 2017/05/10 at 23:17:57, rlanday wrote: > > > On 2017/05/10 at 22:54:22, Xiaocheng wrote: > > > > Since we do not plan to support the case where a misspelling spans multiple nodes, there is no need to iterate over the nodes. Retrieving markers from only |caret_start_container| suffices. > > > > > > I don't think that's correct; the misspelling could occur right at the beginning of the node, in which case caret_start_container might not be anchored to the text node that the DocumentMarker is anchored to. (We might have a similar problem for misspellings occurring at the end of the node if we were to use caret_end_container). > > > > 1. caret_range has been adjusted by ToNormalizedEphemeralRange, which should have its start and end anchored at text nodes. > > > > 2. The old code doesn't work either if caret_range is not anchored directly at the text node with the misspelling. > > Ah, OK, I'll change it Updated
https://codereview.chromium.org/2857173003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2857173003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:808: Node* caret_start_container = We should return if |caret_end_container != caret_start_container| https://codereview.chromium.org/2857173003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:816: const DocumentMarkerVector& markers_in_node = This chunk of code is still duplicated with that in ContextMenuClientImpl. That's why I suggested MarkersInRange(node, start_offset, end_offset). Or maybe std::find_if + lambda can make the code duplication at an acceptable level. Anyway, this is not that important now since the ill-formed API is gone. https://codereview.chromium.org/2857173003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/ContextMenuClientImpl.cpp (right): https://codereview.chromium.org/2857173003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:120: Node* selection_start_container = Same here, we should return if |selection_start_container != selection_end_container|
https://codereview.chromium.org/2857173003/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2857173003/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:816: const DocumentMarkerVector& markers_in_node = On 2017/05/10 at 23:55:15, Xiaocheng wrote: > This chunk of code is still duplicated with that in ContextMenuClientImpl. > > That's why I suggested MarkersInRange(node, start_offset, end_offset). > > Or maybe std::find_if + lambda can make the code duplication at an acceptable level. > > Anyway, this is not that important now since the ill-formed API is gone. I prefer std::find_if() to reduce # of API in DMC. Because of the predicate is simple and DocumentMarkerVector is at most have number of DocumentMakerTypes, which is less than 10 at this time.
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
On 2017/05/11 at 01:03:41, yosin wrote: > I prefer std::find_if() to reduce # of API in DMC. > Because of the predicate is simple and DocumentMarkerVector is at most have number of > DocumentMakerTypes, which is less than 10 at this time. Updated
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2857173003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2857173003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:331: Node* node = position.ComputeContainerNode(); nit: s/Node*/Node* const/ https://codereview.chromium.org/2857173003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:332: unsigned offset = nit: s/unsigned/const unsigned/ https://codereview.chromium.org/2857173003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2857173003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:91: Optional<DocumentMarker*> MarkerAtPosition(const Position&, I think we can use raw pointer and use |nullptr| to represent no marker at position. https://codereview.chromium.org/2857173003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2857173003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:806: Node* caret_start_container = nit: s/Node*/Node* const/ https://codereview.chromium.org/2857173003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:808: Node* caret_end_container = caret_range.EndPosition().ComputeContainerNode(); nit: s/Node*/Node* const/ https://codereview.chromium.org/2857173003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/ContextMenuClientImpl.cpp (right): https://codereview.chromium.org/2857173003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:118: Node* selection_start_container = nit: s/Node*/Node* const/ https://codereview.chromium.org/2857173003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:120: Node* selection_end_container = nit: s/Node*/Node* const/ https://codereview.chromium.org/2857173003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:146: const DocumentMarker* found_marker = *marker_it; nit: s/const DocumnerMarker*/const DocumnerMarker* const/ https://codereview.chromium.org/2857173003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/ContextMenuClientImpl.cpp:149: Range* marker_range = nit: s/Range*/Range* const/ https://codereview.chromium.org/2857173003/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/2857173003/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:337: for (DocumentMarker* marker : Can we use std::count_if()[1]? [1] http://en.cppreference.com/w/cpp/algorithm/count
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
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: This issue passed the CQ dry run.
https://codereview.chromium.org/2857173003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/2857173003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/SelectionController.cpp:522: Optional<DocumentMarker*> marker = Please change type of |marker| to |const DocumentMarker* const| https://codereview.chromium.org/2857173003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h (right): https://codereview.chromium.org/2857173003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h:91: DocumentMarker* MarkerAtPosition(const Position&, Yeah the return type should be DocumentMarker*... Why did I think of Optional<>...
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/05/11 at 18:25:20, xiaochengh wrote: > https://codereview.chromium.org/2857173003/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/SelectionController.cpp (right): > > https://codereview.chromium.org/2857173003/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/SelectionController.cpp:522: Optional<DocumentMarker*> marker = > Please change type of |marker| to |const DocumentMarker* const| Fixed
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: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
yosin@chromium.org changed reviewers: + tkent@chromium.org
lgtm +tkent@ for Source/web/ review https://codereview.chromium.org/2857173003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2857173003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:335: for (DocumentMarker* marker : MarkersFor(node, marker_types)) { Since other parts of this patch uses |std::find_if()|, it is better to use std::find_if()? std::find_if() and for use same number of lines. const auto& markers = MarkersFor(node, marker_types); const auto& it = std::find_if(markers.begin(), markers.end(), [=](DocumentMarker* marker) { return marker->StartOffset() <= start && start < marker->EndOffset(); }); return it == markers.end() ? nullptr : *it; https://codereview.chromium.org/2857173003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:336: if (marker->StartOffset() < offset && marker->EndOffset() > offset) Should we use |marker->StartOffset() <= offset|?
lgtm
https://codereview.chromium.org/2857173003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2857173003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:336: if (marker->StartOffset() < offset && marker->EndOffset() > offset) On 2017/05/12 at 04:44:54, yosin_UTC9 wrote: > Should we use |marker->StartOffset() <= offset|? I don't think so, we said this method is supposed to match MarkersInRange() and only return markers where the Position falls in the interior, not at an endpoint. I'm matching the current logic in MarkersInRange(). Does that answer your question or am I not understanding what you're asking?
The CQ bit was checked by rlanday@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org, yosin@chromium.org, xiaochengh@chromium.org Link to the patchset: https://codereview.chromium.org/2857173003/#ps180001 (title: "Use std::find_if()")
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_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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": 180001, "attempt_start_ts": 1494791163237210, "parent_rev": "1e7cf3cffe06a5d7b91775ebaaa450fe299118bc", "commit_rev": "3639139a6216af6f66840f8348622d45ba06d5b9"}
Message was sent while issue was closed.
Description was changed from ========== Remove DocumentMarkerController::MarkersInRange() Currently, MarkersInRange() is kind of broken in that it can potentially return DocumentMarkers from multiple Nodes, and it doesn't return any information about which Node each DocumentMarker is associated with. This means that callers are currently resorting to dubious methods to attempt to reconstruct the EphemeralRange associated with the returned marker(s). After discussing with xiaochengh@ and yosin@, instead of changing the method to also return the marker's Node or EphemeralRange, we decided to remove the method and move the logic directly into the callers. BUG=707867 ========== to ========== Remove DocumentMarkerController::MarkersInRange() Currently, MarkersInRange() is kind of broken in that it can potentially return DocumentMarkers from multiple Nodes, and it doesn't return any information about which Node each DocumentMarker is associated with. This means that callers are currently resorting to dubious methods to attempt to reconstruct the EphemeralRange associated with the returned marker(s). After discussing with xiaochengh@ and yosin@, instead of changing the method to also return the marker's Node or EphemeralRange, we decided to remove the method and move the logic directly into the callers. BUG=707867 Review-Url: https://codereview.chromium.org/2857173003 Cr-Commit-Position: refs/heads/master@{#471642} Committed: https://chromium.googlesource.com/chromium/src/+/3639139a6216af6f66840f834862... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/3639139a6216af6f66840f834862... |