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

Issue 2947093003: [MarkersIntersectingRange #3] Optimize SpellChecker::GetSpellCheckMarkerUnderSelection() (Closed)

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

Description

Optimize SpellChecker::GetSpellCheckMarkerUnderSelection() Currently, this method does a linear scan over all spell check markers contained in the text node(s) containing the selection. This takes about 0.553 ms on the source for the "List of Australian treaties" Wikipedia page. This CL refactors this method to instead use the newly-added DocumentMarkerController::FirstMarkerIntersectingRange() method, which is more efficient as it uses binary search. With this change, we now spend about ~0.02 ms in this method doing the same test. BUG=736181 Review-Url: https://codereview.chromium.org/2947093003 Cr-Commit-Position: refs/heads/master@{#488577} Committed: https://chromium.googlesource.com/chromium/src/+/eef04de483168c40a8eadfc840fc8ddf4432f55a

Patch Set 1 #

Total comments: 1

Patch Set 2 : Use const reference for node_marker_pairs #

Total comments: 1

Patch Set 3 : Remove GetSpellCheckMarkerTouchingSelection() #

Total comments: 1

Patch Set 4 : Rebase #

Patch Set 5 : Keep GetSpellCheckMarkerUnderSelection() #

Total comments: 1

Patch Set 6 : Rebase #

Total comments: 1

Patch Set 7 : Rebase #

Patch Set 8 : Rebase, cite bug #

Patch Set 9 : Rebase #

Patch Set 10 : Rebase #

Total comments: 4

Patch Set 11 : FirstEphemeralRangeOf(), return {} #

Unified diffs Side-by-side diffs Delta from patch set Stats (+28 lines, -26 lines) Patch
M third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp View 1 2 3 4 5 6 7 8 9 10 1 chunk +28 lines, -26 lines 0 comments Download

Depends on Patchset:

Dependent Patchsets:

Messages

Total messages: 61 (40 generated)
rlanday
https://codereview.chromium.org/2947093003/diff/1/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2947093003/diff/1/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp#newcode878 third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:878: ExpandSelectionRangeIfNecessary(selection); I still need to optimize ExpandSelectionRangeIfNecessary; I will ...
3 years, 6 months ago (2017-06-22 02:25:05 UTC) #2
rlanday
https://codereview.chromium.org/2947093003/diff/20001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2947093003/diff/20001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp#newcode878 third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:878: ExpandSelectionRangeIfNecessary(selection); I think I'm probably going to get rid ...
3 years, 6 months ago (2017-06-22 04:05:13 UTC) #3
rlanday
Ok, I've removed GetSpellCheckMarkerTouchingSelection() and ExpandSelectionRangeIfNecessary() to simplify the spell check code
3 years, 6 months ago (2017-06-22 20:38:48 UTC) #8
rlanday
https://codereview.chromium.org/2947093003/diff/40001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2947093003/diff/40001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp#newcode852 third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:852: SelectionInDOMTree::Builder() I just tried copy-pasting something from here and ...
3 years, 6 months ago (2017-06-22 21:02:01 UTC) #9
yosin_UTC9
I'm in doubt about current implementation[1]. It seems complete fix takes lots of changes, so ...
3 years, 6 months ago (2017-06-23 03:50:08 UTC) #16
rlanday
On 2017/06/23 at 03:50:08, yosin wrote: > I'm in doubt about current implementation[1]. > > ...
3 years, 6 months ago (2017-06-23 04:43:46 UTC) #17
yosin_UTC9
On 2017/06/23 at 04:43:46, rlanday wrote: > On 2017/06/23 at 03:50:08, yosin wrote: > > ...
3 years, 6 months ago (2017-06-23 05:53:08 UTC) #18
rlanday
On 2017/06/23 at 05:53:08, yosin wrote: > I would like to use one function to ...
3 years, 6 months ago (2017-06-23 06:21:34 UTC) #19
yosin_UTC9
On 2017/06/23 at 06:21:34, rlanday wrote: > On 2017/06/23 at 05:53:08, yosin wrote: > > ...
3 years, 6 months ago (2017-06-23 06:35:40 UTC) #20
rlanday
On 2017/06/23 at 06:35:40, yosin wrote: > On 2017/06/23 at 06:21:34, rlanday wrote: > > ...
3 years, 6 months ago (2017-06-23 21:38:07 UTC) #21
rlanday
Ok, now I'm just optimizing GetSpellCheckMarkerUnderSelection(). I'm going to update ContextMenuClient::SelectMisspellingAsync() to use this method ...
3 years, 6 months ago (2017-06-23 22:51:03 UTC) #24
rlanday
https://codereview.chromium.org/2947093003/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/2947093003/diff/80001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp#newcode842 third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:842: Node* const selection_start_container = This seems to involve a ...
3 years, 6 months ago (2017-06-23 23:12:15 UTC) #25
rlanday
On 2017/06/23 at 23:12:15, rlanday wrote: > https://codereview.chromium.org/2947093003/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/2947093003/diff/80001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp#newcode842 ...
3 years, 6 months ago (2017-06-23 23:28:36 UTC) #26
Xiaocheng
https://codereview.chromium.org/2947093003/diff/100001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2947093003/diff/100001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp#newcode847 third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:847: // We don't currently support the case where a ...
3 years, 5 months ago (2017-06-26 22:48:22 UTC) #29
rlanday
On 2017/06/26 at 22:48:22, xiaochengh wrote: > https://codereview.chromium.org/2947093003/diff/100001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp > File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): > > https://codereview.chromium.org/2947093003/diff/100001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp#newcode847 ...
3 years, 5 months ago (2017-07-05 18:41:17 UTC) #37
yosin_UTC9
https://codereview.chromium.org/2947093003/diff/180001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2947093003/diff/180001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp#newcode840 third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:840: selection.ToNormalizedEphemeralRange(); Could you use |FirstEphemeralRangeOf(selection)| as original? Since VS::ToNormalizedRange() ...
3 years, 5 months ago (2017-07-20 01:12:11 UTC) #49
Xiaocheng
https://codereview.chromium.org/2947093003/diff/180001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2947093003/diff/180001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp#newcode836 third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:836: return Optional<std::pair<Node*, SpellCheckMarker*>>(); Use |return WTF::nullopt| https://codereview.chromium.org/2947093003/diff/180001/third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp#newcode850 third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:850: return ...
3 years, 5 months ago (2017-07-20 21:41:42 UTC) #50
rlanday
Updated (note: I'm currently using {} and not WTF::nullopt)
3 years, 5 months ago (2017-07-21 00:12:45 UTC) #53
yosin_UTC9
lgtm Please update description: >DocumentMarkerController::MarkerIntersectingRange() method, which is more >efficient as it uses binary search. ...
3 years, 5 months ago (2017-07-21 01:01:37 UTC) #54
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/2947093003/200001
3 years, 5 months ago (2017-07-21 02:04:29 UTC) #58
commit-bot: I haz the power
3 years, 5 months ago (2017-07-21 04:17:28 UTC) #61
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/eef04de483168c40a8eadfc840fc...

Powered by Google App Engine
This is Rietveld 408576698