Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(170)

Issue 2857173003: Remove DocumentMarkerController::MarkersInRange() (Closed)

Created:
3 years, 7 months ago by rlanday
Modified:
3 years, 7 months ago
Reviewers:
tkent, yosin_UTC9, Xiaocheng
CC:
blink-reviews, chromium-reviews, groby+blinkspell_chromium.org, kinuko+watch, timvolodine
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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() #

Unified diffs Side-by-side diffs Delta from patch set Stats (+141 lines, -84 lines) Patch
M third_party/WebKit/Source/core/editing/InputMethodControllerTest.cpp View 1 2 3 4 5 6 7 8 9 1 chunk +6 lines, -10 lines 0 comments Download
M third_party/WebKit/Source/core/editing/SelectionController.cpp View 1 2 3 4 5 6 7 8 9 2 chunks +10 lines, -13 lines 0 comments Download
M third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.h View 1 2 3 4 5 6 1 chunk +4 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp View 1 2 3 4 5 6 7 8 9 2 chunks +17 lines, -31 lines 0 comments Download
M third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp View 1 2 3 4 5 6 1 chunk +31 lines, -9 lines 0 comments Download
M third_party/WebKit/Source/web/ContextMenuClientImpl.cpp View 1 2 3 4 5 6 1 chunk +39 lines, -13 lines 0 comments Download
M third_party/WebKit/Source/web/tests/WebFrameTest.cpp View 1 2 3 4 5 6 7 8 9 3 chunks +34 lines, -6 lines 0 comments Download

Messages

Total messages: 77 (35 generated)
rlanday
3 years, 7 months ago (2017-05-03 22:50:10 UTC) #3
Xiaocheng
Looks good. Let's wait for yosin@'s comment. Life is hard when your reviewer is on ...
3 years, 7 months ago (2017-05-04 00:31:59 UTC) #4
rlanday
https://codereview.chromium.org/2857173003/diff/1/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2857173003/diff/1/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode366 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: > ...
3 years, 7 months ago (2017-05-04 00:35:51 UTC) #5
yosin_UTC9
Do we really need to have DM::MarkersInRange()? It seems MarkesForNode() is enough. # SelectionController::SelectClosestMisspellingFromHitTestResult() DocumentMarkerVector ...
3 years, 7 months ago (2017-05-08 05:09:30 UTC) #6
rlanday
Can you please explain the "<b>d</b>ox" case? I have another use case that complicates things ...
3 years, 7 months ago (2017-05-08 18:35:45 UTC) #7
yosin_UTC9
On 2017/05/08 at 18:35:45, rlanday wrote: > Can you please explain the "<b>d</b>ox" case? "dox" ...
3 years, 7 months ago (2017-05-09 02:01:55 UTC) #8
rlanday
The "<b>d</b>ox" case is problematic regardless of how ReplaceMisspelledRange() is implemented because we run into ...
3 years, 7 months ago (2017-05-09 04:25:49 UTC) #9
yosin_UTC9
On 2017/05/09 at 04:25:49, rlanday wrote: > The "<b>d</b>ox" case is problematic regardless of how ...
3 years, 7 months ago (2017-05-09 06:31:51 UTC) #10
Xiaocheng
Hmm, so we actually have multiple problems here 1. Stop using the ill-formed method MarkersInRange() ...
3 years, 7 months ago (2017-05-09 18:57:12 UTC) #11
rlanday
On 2017/05/09 at 18:57:12, xiaochengh wrote: > 2 seems rather low priority. Let's leave a ...
3 years, 7 months ago (2017-05-09 19:24:51 UTC) #12
rlanday
Ok, I have updated this CL to remove MarkersInRange().
3 years, 7 months ago (2017-05-09 21:38:10 UTC) #16
Xiaocheng
https://codereview.chromium.org/2857173003/diff/20001/third_party/WebKit/Source/core/editing/SelectionController.cpp File third_party/WebKit/Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/2857173003/diff/20001/third_party/WebKit/Source/core/editing/SelectionController.cpp#newcode526 third_party/WebKit/Source/core/editing/SelectionController.cpp:526: if (markers.size() == 1) { This can break something ...
3 years, 7 months ago (2017-05-09 21:55:16 UTC) #17
rlanday
https://codereview.chromium.org/2857173003/diff/20001/third_party/WebKit/Source/core/editing/SelectionController.cpp File third_party/WebKit/Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/2857173003/diff/20001/third_party/WebKit/Source/core/editing/SelectionController.cpp#newcode526 third_party/WebKit/Source/core/editing/SelectionController.cpp:526: if (markers.size() == 1) { On 2017/05/09 at 21:55:15, ...
3 years, 7 months ago (2017-05-09 22:04:17 UTC) #18
Xiaocheng
On 2017/05/09 at 22:04:17, rlanday wrote: > https://codereview.chromium.org/2857173003/diff/20001/third_party/WebKit/Source/core/editing/SelectionController.cpp > File third_party/WebKit/Source/core/editing/SelectionController.cpp (right): > > https://codereview.chromium.org/2857173003/diff/20001/third_party/WebKit/Source/core/editing/SelectionController.cpp#newcode526 ...
3 years, 7 months ago (2017-05-09 22:16:41 UTC) #19
rlanday
On 2017/05/09 at 22:16:41, xiaochengh wrote: > On 2017/05/09 at 22:04:17, rlanday wrote: > > ...
3 years, 7 months ago (2017-05-10 00:11:11 UTC) #22
yosin_UTC9
https://codereview.chromium.org/2857173003/diff/20001/third_party/WebKit/Source/core/editing/SelectionController.cpp File third_party/WebKit/Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/2857173003/diff/20001/third_party/WebKit/Source/core/editing/SelectionController.cpp#newcode526 third_party/WebKit/Source/core/editing/SelectionController.cpp:526: if (markers.size() == 1) { On 2017/05/09 at 22:04:17, ...
3 years, 7 months ago (2017-05-10 01:46:59 UTC) #23
Xiaocheng
On 2017/05/10 at 01:46:59, yosin wrote: > https://codereview.chromium.org/2857173003/diff/20001/third_party/WebKit/Source/core/editing/SelectionController.cpp > File third_party/WebKit/Source/core/editing/SelectionController.cpp (right): > > https://codereview.chromium.org/2857173003/diff/20001/third_party/WebKit/Source/core/editing/SelectionController.cpp#newcode526 ...
3 years, 7 months ago (2017-05-10 02:01:34 UTC) #24
rlanday
> On 2017/05/10 at 01:46:59, yosin wrote: > In fact this can be simplified to ...
3 years, 7 months ago (2017-05-10 02:15:23 UTC) #25
Xiaocheng
On 2017/05/10 at 02:15:23, rlanday wrote: > > On 2017/05/10 at 01:46:59, yosin wrote: > ...
3 years, 7 months ago (2017-05-10 02:56:21 UTC) #26
yosin_UTC9
On 2017/05/10 at 02:56:21, xiaochengh wrote: > On 2017/05/10 at 02:15:23, rlanday wrote: > > ...
3 years, 7 months ago (2017-05-10 04:16:45 UTC) #27
rlanday
I don't think MarkerAtPosition() is enough. ContextMenuClientImpl::SelectMisspellingAsync() and SpellChecker::ReplaceMisspelledRange() both need to be able to ...
3 years, 7 months ago (2017-05-10 20:22:45 UTC) #28
rlanday
On 2017/05/10 at 20:22:45, rlanday wrote: > I don't think MarkerAtPosition() is enough. ContextMenuClientImpl::SelectMisspellingAsync() and ...
3 years, 7 months ago (2017-05-10 20:55:12 UTC) #31
Xiaocheng
https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (left): https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp#oldcode812 third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:812: Position(caret_range.StartPosition().ComputeContainerNode(), Note that the existing code doesn't work if ...
3 years, 7 months ago (2017-05-10 22:54:22 UTC) #36
rlanday
https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp#newcode816 third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:816: for (Node& node : caret_range.Nodes()) { On 2017/05/10 at ...
3 years, 7 months ago (2017-05-10 23:17:57 UTC) #37
Xiaocheng
https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp#newcode816 third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:816: for (Node& node : caret_range.Nodes()) { On 2017/05/10 at ...
3 years, 7 months ago (2017-05-10 23:23:43 UTC) #38
rlanday
https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp#newcode816 third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:816: for (Node& node : caret_range.Nodes()) { On 2017/05/10 at ...
3 years, 7 months ago (2017-05-10 23:25:43 UTC) #39
rlanday
On 2017/05/10 at 23:25:43, rlanday wrote: > https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp > File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): > > https://codereview.chromium.org/2857173003/diff/60001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp#newcode816 ...
3 years, 7 months ago (2017-05-10 23:38:55 UTC) #41
Xiaocheng
https://codereview.chromium.org/2857173003/diff/80001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2857173003/diff/80001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp#newcode808 third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:808: Node* caret_start_container = We should return if |caret_end_container != ...
3 years, 7 months ago (2017-05-10 23:55:15 UTC) #42
yosin_UTC9
https://codereview.chromium.org/2857173003/diff/80001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2857173003/diff/80001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp#newcode816 third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:816: const DocumentMarkerVector& markers_in_node = On 2017/05/10 at 23:55:15, Xiaocheng ...
3 years, 7 months ago (2017-05-11 01:03:41 UTC) #43
rlanday
On 2017/05/11 at 01:03:41, yosin wrote: > I prefer std::find_if() to reduce # of API ...
3 years, 7 months ago (2017-05-11 03:11:20 UTC) #45
yosin_UTC9
https://codereview.chromium.org/2857173003/diff/100001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2857173003/diff/100001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode331 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/Source/core/editing/markers/DocumentMarkerController.cpp#newcode332 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:332: ...
3 years, 7 months ago (2017-05-11 04:36:03 UTC) #47
rlanday
Updated
3 years, 7 months ago (2017-05-11 05:12:57 UTC) #51
Xiaocheng
https://codereview.chromium.org/2857173003/diff/120001/third_party/WebKit/Source/core/editing/SelectionController.cpp File third_party/WebKit/Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/2857173003/diff/120001/third_party/WebKit/Source/core/editing/SelectionController.cpp#newcode522 third_party/WebKit/Source/core/editing/SelectionController.cpp:522: Optional<DocumentMarker*> marker = Please change type of |marker| to ...
3 years, 7 months ago (2017-05-11 18:25:20 UTC) #55
rlanday
On 2017/05/11 at 18:25:20, xiaochengh wrote: > https://codereview.chromium.org/2857173003/diff/120001/third_party/WebKit/Source/core/editing/SelectionController.cpp > File third_party/WebKit/Source/core/editing/SelectionController.cpp (right): > > https://codereview.chromium.org/2857173003/diff/120001/third_party/WebKit/Source/core/editing/SelectionController.cpp#newcode522 ...
3 years, 7 months ago (2017-05-11 18:34:57 UTC) #59
Xiaocheng
lgtm
3 years, 7 months ago (2017-05-11 18:38:28 UTC) #61
yosin_UTC9
lgtm +tkent@ for Source/web/ review https://codereview.chromium.org/2857173003/diff/160001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2857173003/diff/160001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode335 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:335: for (DocumentMarker* marker : ...
3 years, 7 months ago (2017-05-12 04:44:55 UTC) #65
tkent
lgtm
3 years, 7 months ago (2017-05-12 04:53:55 UTC) #66
rlanday
https://codereview.chromium.org/2857173003/diff/160001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp File third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp (right): https://codereview.chromium.org/2857173003/diff/160001/third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp#newcode336 third_party/WebKit/Source/core/editing/markers/DocumentMarkerController.cpp:336: if (marker->StartOffset() < offset && marker->EndOffset() > offset) On ...
3 years, 7 months ago (2017-05-12 21:13:09 UTC) #67
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2857173003/180001
3 years, 7 months ago (2017-05-12 21:25:06 UTC) #70
commit-bot: I haz the power
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_rel_ng/builds/425657)
3 years, 7 months ago (2017-05-12 23:11:21 UTC) #72
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2857173003/180001
3 years, 7 months ago (2017-05-14 19:46:12 UTC) #74
commit-bot: I haz the power
3 years, 7 months ago (2017-05-14 23:05:30 UTC) #77
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/3639139a6216af6f66840f834862...

Powered by Google App Engine
This is Rietveld 408576698