Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22859062:
Chunk up the text to spell check also when the text is pasted. (Closed)
|
Created:
7 years, 4 months ago by pstanek Modified:
7 years, 3 months ago CC:
blink-reviews, dglazkov+blink, eae+blinkwatch Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionChunk up the text to spell check also when the text is pasted.
BUG=278119
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157747
Patch Set 1 #Patch Set 2 : Fix flaky test. #
Total comments: 1
Patch Set 3 : Rebase, Added TC & even more improvements. #Patch Set 4 : Unrelated: fix for typo. #Patch Set 5 : Check if merging checks are needed at all #
Total comments: 13
Patch Set 6 : Review comments & addMarker() changes revert #Patch Set 7 : Rebase #
Messages
Total messages: 18 (0 generated)
Test is not included yet as I trying to figure out how to test it. Note that all editing tests pass with this, though.
Thank you for the patch! Could you please split out the layout test fix into a separate code review? You should also file a separate bug (test flakiness) for that code review. This separation should make the code reviews faster.
On 2013/08/23 16:35:36, Rouslan Solomakhin wrote: > Thank you for the patch! Could you please split out the layout test fix into a > separate code review? You should also file a separate bug (test flakiness) for > that code review. This separation should make the code reviews faster. +1 to splitting off the test fix to a separate patch that lands first. No need to file a separate bug though.
On 2013/08/23 16:56:59, ojan wrote: > On 2013/08/23 16:35:36, Rouslan Solomakhin wrote: > > Thank you for the patch! Could you please split out the layout test fix into a > > separate code review? You should also file a separate bug (test flakiness) for > > that code review. This separation should make the code reviews faster. > > +1 to splitting off the test fix to a separate patch that lands first. No need > to file a separate bug though. https://codereview.chromium.org/23389005/
On 2013/08/23 17:22:33, pstanek wrote: > On 2013/08/23 16:56:59, ojan wrote: > > On 2013/08/23 16:35:36, Rouslan Solomakhin wrote: > > > Thank you for the patch! Could you please split out the layout test fix into > a > > > separate code review? You should also file a separate bug (test flakiness) > for > > > that code review. This separation should make the code reviews faster. > > > > +1 to splitting off the test fix to a separate patch that lands first. No need > > to file a separate bug though. > > https://codereview.chromium.org/23389005/ This can't be tested automatically in content_shell since it's addMarker() which takes so long (http://code.google.com/p/chromium/issues/detail?id=279293) and AFAICS it's not called in case of content_shell. I still believe chunking is a good idea as it makes this better even without addMarker improvements http://code.google.com/p/chromium/issues/detail?id=279293 is about.
I still see the test changes. Can you rebase and upload again? What can we do to make this change testable? Do we need to add additional methods to Source/core/testing/Internals.idl to test this? It seems like it should be possible to test this. https://codereview.chromium.org/22859062/diff/4001/Source/core/editing/Editor.h File Source/core/editing/Editor.h (right): https://codereview.chromium.org/22859062/diff/4001/Source/core/editing/Editor... Source/core/editing/Editor.h:345: void chunkAndMarkAllMisspellingsAndBadGrammar(TextCheckingTypeMask textCheckingOptions, const TextCheckingParagraph& fullParagraphToCheck, bool asynchronous); Nit: fullParagraphToCheck doesn't need to be named.
On 2013/08/26 23:40:51, tony wrote: > I still see the test changes. Can you rebase and upload again? > > What can we do to make this change testable? Do we need to add additional > methods to Source/core/testing/Internals.idl to test this? It seems like it > should be possible to test this. > > https://codereview.chromium.org/22859062/diff/4001/Source/core/editing/Editor.h > File Source/core/editing/Editor.h (right): > > https://codereview.chromium.org/22859062/diff/4001/Source/core/editing/Editor... > Source/core/editing/Editor.h:345: void > chunkAndMarkAllMisspellingsAndBadGrammar(TextCheckingTypeMask > textCheckingOptions, const TextCheckingParagraph& fullParagraphToCheck, bool > asynchronous); > Nit: fullParagraphToCheck doesn't need to be named. Will rebase. I might have been wrong about addMarker(). It's not called when launching content_shell explicitly (does not have spell checker impl.) but the test runner has spell checker mock. Will play a bit more with it and will try to come up with some test.
Rebased, added TC and even more performance improvements (which I spotted when working on the TC). Some numbers given by the test: master: Running 1 tests Running Interactive/spellcheck-paste-huge-text.html (1 of 1) RESULT Interactive: spellcheck-paste-huge-text: Time= 729.65000004 ms median= 581.500000088 ms, stdev= 712.536999897 ms, min= 47.9999999516 ms, max= 2070.0000003 ms RESULT Interactive: spellcheck-paste-huge-text: JSHeap= 1695353.4 bytes median= 1704354.0 bytes, stdev= 31519.2544736 bytes, min= 1562616.0 bytes, max= 1705792.0 bytes RESULT Interactive: spellcheck-paste-huge-text: Malloc= 0.0 bytes median= 0.0 bytes, stdev= 0.0 bytes, min= 0.0 bytes, max= 0.0 bytes Finished: 134.414000 s My branch: Running 1 tests Running Interactive/spellcheck-paste-huge-text.html (1 of 1) RESULT Interactive: spellcheck-paste-huge-text: Time= 127.950000041 ms median= 132.500000065 ms, stdev= 55.5067326766 ms, min= 49.9999998137 ms, max= 195.999999996 ms RESULT Interactive: spellcheck-paste-huge-text: JSHeap= 1682156.0 bytes median= 1704352.0 bytes, stdev= 51728.0080421 bytes, min= 1561736.0 bytes, max= 1705788.0 bytes RESULT Interactive: spellcheck-paste-huge-text: Malloc= 0.0 bytes median= 0.0 bytes, stdev= 0.0 bytes, min= 0.0 bytes, max= 0.0 bytes Finished: 11.168000 s The difference is huge. PTAL
On 2013/08/29 16:17:51, pstanek wrote: > Rebased, added TC and even more performance improvements (which I spotted when > working on the TC). > > Some numbers given by the test: > > master: > > Running 1 tests > Running Interactive/spellcheck-paste-huge-text.html (1 of 1) > RESULT Interactive: spellcheck-paste-huge-text: Time= 729.65000004 ms > median= 581.500000088 ms, stdev= 712.536999897 ms, min= 47.9999999516 ms, max= > 2070.0000003 ms > RESULT Interactive: spellcheck-paste-huge-text: JSHeap= 1695353.4 bytes > median= 1704354.0 bytes, stdev= 31519.2544736 bytes, min= 1562616.0 bytes, max= > 1705792.0 bytes > RESULT Interactive: spellcheck-paste-huge-text: Malloc= 0.0 bytes > median= 0.0 bytes, stdev= 0.0 bytes, min= 0.0 bytes, max= 0.0 bytes > Finished: 134.414000 s > > My branch: > > Running 1 tests > Running Interactive/spellcheck-paste-huge-text.html (1 of 1) > RESULT Interactive: spellcheck-paste-huge-text: Time= 127.950000041 ms > median= 132.500000065 ms, stdev= 55.5067326766 ms, min= 49.9999998137 ms, max= > 195.999999996 ms > RESULT Interactive: spellcheck-paste-huge-text: JSHeap= 1682156.0 bytes > median= 1704352.0 bytes, stdev= 51728.0080421 bytes, min= 1561736.0 bytes, max= > 1705788.0 bytes > RESULT Interactive: spellcheck-paste-huge-text: Malloc= 0.0 bytes > median= 0.0 bytes, stdev= 0.0 bytes, min= 0.0 bytes, max= 0.0 bytes > Finished: 11.168000 s > > The difference is huge. > > PTAL FYI: Tomorrow is my last day before 2 weeks vacations (and tomorrow starts here in couple of minutes ;)) and I'd love to integrate this if possible otherwise it may get a bit rusty during that period. Chunking is quite straight forward and checking the whole paragraphs is not as good as it may seem apparently (checking range may be couple hundreds of chars where the paragraph might be thousands of chars and checking it on every selection change is an overkill). If changes in addMarker() should be moved to another CL in order to speed this up - fair enough - I'll move it first things in the morning tomorrow. Sorry for nagging & thanks in advance :).
Looking at the perf dashboard (chromeperf.appspot.com), 11s seems like a long time to run a test. I would try to make the test run about 5s if possible. I'm also out of town starting in a few hours, but maybe someone else will be able to finish the review before your vacation. That said, I don't think it's a big deal to wait 2 weeks. https://codereview.chromium.org/22859062/diff/25001/PerformanceTests/Interact... File PerformanceTests/Interactive/spellcheck-paste-huge-text.html (right): https://codereview.chromium.org/22859062/diff/25001/PerformanceTests/Interact... PerformanceTests/Interactive/spellcheck-paste-huge-text.html:11: zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz. If we could generate this with script, that would be nicer to the SVN server. https://codereview.chromium.org/22859062/diff/25001/PerformanceTests/Interact... PerformanceTests/Interactive/spellcheck-paste-huge-text.html:286: PerfTestRunner.prepareToMeasureValuesAsync({unit: "ms", done:done}); Nit: space after done: https://codereview.chromium.org/22859062/diff/25001/PerformanceTests/Interact... PerformanceTests/Interactive/spellcheck-paste-huge-text.html:295: test(); Nit: Maybe put this in the onload handler? https://codereview.chromium.org/22859062/diff/25001/PerformanceTests/Interact... PerformanceTests/Interactive/spellcheck-paste-huge-text.html:367: setTimeout(test, 0); Do you need to call PerfTestRunner.gc() before the next test? Do you need to call done() or does PerfTestRunner.prepareToMeasureValuesAsync do that for you? https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... File Source/core/dom/DocumentMarkerController.cpp (right): https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... Source/core/dom/DocumentMarkerController.cpp:141: static bool notOverlaps(const DocumentMarker& lhv, const DocumentMarker& rhv) Nit: doesNotOverlap sounds better to me than notOverlaps. https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... Source/core/dom/DocumentMarkerController.cpp:165: if (toInsert.type() != DocumentMarker::TextMatch) { Can we move this block into a helper function? The name of the function should help describe what it's doing so you don't have to write. https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... Source/core/dom/DocumentMarkerController.cpp:176: size_t i; Please use a more descriptive variable than 'i'. Maybe overlappingIndex? https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... Source/core/dom/DocumentMarkerController.cpp:181: numMarkers--; Nit: --numMarkers https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... Source/core/dom/DocumentMarkerController.cpp:183: i = numMarkers; Set i = numMarkers when initializing and you can get rid of the else block. https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... Source/core/dom/DocumentMarkerController.cpp:189: while (j < numMarkers) { Looks like you could use a for loop here so that j (which you could now rename to i) is scoped within the loop. https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... Source/core/dom/DocumentMarkerController.cpp:199: numMarkers--; Nit: --numMarkers https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... Source/core/dom/DocumentMarkerController.cpp:201: j++; Nit: ++j https://codereview.chromium.org/22859062/diff/25001/Source/core/editing/Edito... File Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/22859062/diff/25001/Source/core/editing/Edito... Source/core/editing/Editor.cpp:424: chunkAndMarkAllMisspellingsAndBadGrammar(resolveTextCheckingTypeMask(TextCheckingTypeSpelling | TextCheckingTypeGrammar), textToCheck, true); Please use "bool asynchronous = true" and pass asynchronous to the function to improve readability.
On 2013/08/29 22:44:45, tony wrote: > Looking at the perf dashboard (http://chromeperf.appspot.com), 11s seems like a long > time to run a test. I would try to make the test run about 5s if possible. 5 runs take 10 sec. On the same machine (i.e. mine) 5 reps of Dom/textarea-edit.html take 26s, and Interactive\SelectAll.html takes almost 45s so this shouldn't be a problem. Making it faster would need using less text and I would like to have more than one chunk i.e. > 16k. Test currently uses ~21k of text. > > I'm also out of town starting in a few hours, but maybe someone else will be > able to finish the review before your vacation. That said, I don't think it's a > big deal to wait 2 weeks. > > https://codereview.chromium.org/22859062/diff/25001/PerformanceTests/Interact... > File PerformanceTests/Interactive/spellcheck-paste-huge-text.html (right): > > https://codereview.chromium.org/22859062/diff/25001/PerformanceTests/Interact... > PerformanceTests/Interactive/spellcheck-paste-huge-text.html:11: zz zz zz zz zz > zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz. > If we could generate this with script, that would be nicer to the SVN server. > > https://codereview.chromium.org/22859062/diff/25001/PerformanceTests/Interact... > PerformanceTests/Interactive/spellcheck-paste-huge-text.html:286: > PerfTestRunner.prepareToMeasureValuesAsync({unit: "ms", done:done}); > Nit: space after done: > > https://codereview.chromium.org/22859062/diff/25001/PerformanceTests/Interact... > PerformanceTests/Interactive/spellcheck-paste-huge-text.html:295: test(); > Nit: Maybe put this in the onload handler? > > https://codereview.chromium.org/22859062/diff/25001/PerformanceTests/Interact... > PerformanceTests/Interactive/spellcheck-paste-huge-text.html:367: > setTimeout(test, 0); > Do you need to call PerfTestRunner.gc() before the next test? > Do you need to call done() or does PerfTestRunner.prepareToMeasureValuesAsync do > that for you? Will call gc(). done() is called on test finish it after 5 runs (5 calls to PerfTestRunner.prepareToMeasureValuesAsync). > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > File Source/core/dom/DocumentMarkerController.cpp (right): > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > Source/core/dom/DocumentMarkerController.cpp:141: static bool notOverlaps(const > DocumentMarker& lhv, const DocumentMarker& rhv) > Nit: doesNotOverlap sounds better to me than notOverlaps. > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > Source/core/dom/DocumentMarkerController.cpp:165: if (toInsert.type() != > DocumentMarker::TextMatch) { > Can we move this block into a helper function? The name of the function should > help describe what it's doing so you don't have to write. > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > Source/core/dom/DocumentMarkerController.cpp:176: size_t i; > Please use a more descriptive variable than 'i'. Maybe overlappingIndex? > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > Source/core/dom/DocumentMarkerController.cpp:181: numMarkers--; > Nit: --numMarkers > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > Source/core/dom/DocumentMarkerController.cpp:183: i = numMarkers; > Set i = numMarkers when initializing and you can get rid of the else block. > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > Source/core/dom/DocumentMarkerController.cpp:189: while (j < numMarkers) { > Looks like you could use a for loop here so that j (which you could now rename > to i) is scoped within the loop. > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > Source/core/dom/DocumentMarkerController.cpp:199: numMarkers--; > Nit: --numMarkers > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > Source/core/dom/DocumentMarkerController.cpp:201: j++; > Nit: ++j > > https://codereview.chromium.org/22859062/diff/25001/Source/core/editing/Edito... > File Source/core/editing/Editor.cpp (right): > > https://codereview.chromium.org/22859062/diff/25001/Source/core/editing/Edito... > Source/core/editing/Editor.cpp:424: > chunkAndMarkAllMisspellingsAndBadGrammar(resolveTextCheckingTypeMask(TextCheckingTypeSpelling > | TextCheckingTypeGrammar), textToCheck, true); > Please use "bool asynchronous = true" and pass asynchronous to the function to > improve readability.
On 2013/08/30 11:38:42, pstanek wrote: > On 2013/08/29 22:44:45, tony wrote: > > Looking at the perf dashboard (http://chromeperf.appspot.com), 11s seems like > a long > > time to run a test. I would try to make the test run about 5s if possible. > > 5 runs take 10 sec. On the same machine (i.e. mine) 5 reps of > Dom/textarea-edit.html take 26s, and Interactive\SelectAll.html takes almost 45s > so this shouldn't be a problem. Making it faster would need using less text and > I would like to have more than one chunk i.e. > 16k. Test currently uses ~21k of > text. > > > > > > I'm also out of town starting in a few hours, but maybe someone else will be > > able to finish the review before your vacation. That said, I don't think it's > a > > big deal to wait 2 weeks. > > > > > https://codereview.chromium.org/22859062/diff/25001/PerformanceTests/Interact... > > File PerformanceTests/Interactive/spellcheck-paste-huge-text.html (right): > > > > > https://codereview.chromium.org/22859062/diff/25001/PerformanceTests/Interact... > > PerformanceTests/Interactive/spellcheck-paste-huge-text.html:11: zz zz zz zz > zz > > zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz. > > If we could generate this with script, that would be nicer to the SVN server. > > > > > https://codereview.chromium.org/22859062/diff/25001/PerformanceTests/Interact... > > PerformanceTests/Interactive/spellcheck-paste-huge-text.html:286: > > PerfTestRunner.prepareToMeasureValuesAsync({unit: "ms", done:done}); > > Nit: space after done: > > > > > https://codereview.chromium.org/22859062/diff/25001/PerformanceTests/Interact... > > PerformanceTests/Interactive/spellcheck-paste-huge-text.html:295: test(); > > Nit: Maybe put this in the onload handler? > > > > > https://codereview.chromium.org/22859062/diff/25001/PerformanceTests/Interact... > > PerformanceTests/Interactive/spellcheck-paste-huge-text.html:367: > > setTimeout(test, 0); > > Do you need to call PerfTestRunner.gc() before the next test? > > Do you need to call done() or does PerfTestRunner.prepareToMeasureValuesAsync > do > > that for you? > > Will call gc(). > done() is called on test finish it after 5 runs (5 calls to > PerfTestRunner.prepareToMeasureValuesAsync). > > > > > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > > File Source/core/dom/DocumentMarkerController.cpp (right): > > > > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > > Source/core/dom/DocumentMarkerController.cpp:141: static bool > notOverlaps(const > > DocumentMarker& lhv, const DocumentMarker& rhv) > > Nit: doesNotOverlap sounds better to me than notOverlaps. > > > > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > > Source/core/dom/DocumentMarkerController.cpp:165: if (toInsert.type() != > > DocumentMarker::TextMatch) { > > Can we move this block into a helper function? The name of the function > should > > help describe what it's doing so you don't have to write. > > > > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > > Source/core/dom/DocumentMarkerController.cpp:176: size_t i; > > Please use a more descriptive variable than 'i'. Maybe overlappingIndex? > > > > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > > Source/core/dom/DocumentMarkerController.cpp:181: numMarkers--; > > Nit: --numMarkers > > > > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > > Source/core/dom/DocumentMarkerController.cpp:183: i = numMarkers; > > Set i = numMarkers when initializing and you can get rid of the else block. > > > > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > > Source/core/dom/DocumentMarkerController.cpp:189: while (j < numMarkers) { > > Looks like you could use a for loop here so that j (which you could now rename > > to i) is scoped within the loop. > > > > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > > Source/core/dom/DocumentMarkerController.cpp:199: numMarkers--; > > Nit: --numMarkers > > > > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > > Source/core/dom/DocumentMarkerController.cpp:201: j++; > > Nit: ++j > > > > > https://codereview.chromium.org/22859062/diff/25001/Source/core/editing/Edito... > > File Source/core/editing/Editor.cpp (right): > > > > > https://codereview.chromium.org/22859062/diff/25001/Source/core/editing/Edito... > > Source/core/editing/Editor.cpp:424: > > > chunkAndMarkAllMisspellingsAndBadGrammar(resolveTextCheckingTypeMask(TextCheckingTypeSpelling > > | TextCheckingTypeGrammar), textToCheck, true); > > Please use "bool asynchronous = true" and pass asynchronous to the function to > > improve readability. Addressed the comments. I also reverted changes from addMarker() for now because I found a regression caused by them. Will put them to a separate CL after fixing. This is still a massive improvement so if it's good enough feel free to send to integration. See you in 2 weeks :).
On 2013/08/30 18:53:17, pstanek wrote: > On 2013/08/30 11:38:42, pstanek wrote: > > On 2013/08/29 22:44:45, tony wrote: > > > Looking at the perf dashboard (http://chromeperf.appspot.com), 11s seems > like > > a long > > > time to run a test. I would try to make the test run about 5s if possible. > > > > 5 runs take 10 sec. On the same machine (i.e. mine) 5 reps of > > Dom/textarea-edit.html take 26s, and Interactive\SelectAll.html takes almost > 45s > > so this shouldn't be a problem. Making it faster would need using less text > and > > I would like to have more than one chunk i.e. > 16k. Test currently uses ~21k > of > > text. > > > > > > > > > > I'm also out of town starting in a few hours, but maybe someone else will be > > > able to finish the review before your vacation. That said, I don't think > it's > > a > > > big deal to wait 2 weeks. > > > > > > > > > https://codereview.chromium.org/22859062/diff/25001/PerformanceTests/Interact... > > > File PerformanceTests/Interactive/spellcheck-paste-huge-text.html (right): > > > > > > > > > https://codereview.chromium.org/22859062/diff/25001/PerformanceTests/Interact... > > > PerformanceTests/Interactive/spellcheck-paste-huge-text.html:11: zz zz zz zz > > zz > > > zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz zz. > > > If we could generate this with script, that would be nicer to the SVN > server. > > > > > > > > > https://codereview.chromium.org/22859062/diff/25001/PerformanceTests/Interact... > > > PerformanceTests/Interactive/spellcheck-paste-huge-text.html:286: > > > PerfTestRunner.prepareToMeasureValuesAsync({unit: "ms", done:done}); > > > Nit: space after done: > > > > > > > > > https://codereview.chromium.org/22859062/diff/25001/PerformanceTests/Interact... > > > PerformanceTests/Interactive/spellcheck-paste-huge-text.html:295: test(); > > > Nit: Maybe put this in the onload handler? > > > > > > > > > https://codereview.chromium.org/22859062/diff/25001/PerformanceTests/Interact... > > > PerformanceTests/Interactive/spellcheck-paste-huge-text.html:367: > > > setTimeout(test, 0); > > > Do you need to call PerfTestRunner.gc() before the next test? > > > Do you need to call done() or does > PerfTestRunner.prepareToMeasureValuesAsync > > do > > > that for you? > > > > Will call gc(). > > done() is called on test finish it after 5 runs (5 calls to > > PerfTestRunner.prepareToMeasureValuesAsync). > > > > > > > > > > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > > > File Source/core/dom/DocumentMarkerController.cpp (right): > > > > > > > > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > > > Source/core/dom/DocumentMarkerController.cpp:141: static bool > > notOverlaps(const > > > DocumentMarker& lhv, const DocumentMarker& rhv) > > > Nit: doesNotOverlap sounds better to me than notOverlaps. > > > > > > > > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > > > Source/core/dom/DocumentMarkerController.cpp:165: if (toInsert.type() != > > > DocumentMarker::TextMatch) { > > > Can we move this block into a helper function? The name of the function > > should > > > help describe what it's doing so you don't have to write. > > > > > > > > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > > > Source/core/dom/DocumentMarkerController.cpp:176: size_t i; > > > Please use a more descriptive variable than 'i'. Maybe overlappingIndex? > > > > > > > > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > > > Source/core/dom/DocumentMarkerController.cpp:181: numMarkers--; > > > Nit: --numMarkers > > > > > > > > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > > > Source/core/dom/DocumentMarkerController.cpp:183: i = numMarkers; > > > Set i = numMarkers when initializing and you can get rid of the else block. > > > > > > > > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > > > Source/core/dom/DocumentMarkerController.cpp:189: while (j < numMarkers) { > > > Looks like you could use a for loop here so that j (which you could now > rename > > > to i) is scoped within the loop. > > > > > > > > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > > > Source/core/dom/DocumentMarkerController.cpp:199: numMarkers--; > > > Nit: --numMarkers > > > > > > > > > https://codereview.chromium.org/22859062/diff/25001/Source/core/dom/DocumentM... > > > Source/core/dom/DocumentMarkerController.cpp:201: j++; > > > Nit: ++j > > > > > > > > > https://codereview.chromium.org/22859062/diff/25001/Source/core/editing/Edito... > > > File Source/core/editing/Editor.cpp (right): > > > > > > > > > https://codereview.chromium.org/22859062/diff/25001/Source/core/editing/Edito... > > > Source/core/editing/Editor.cpp:424: > > > > > > chunkAndMarkAllMisspellingsAndBadGrammar(resolveTextCheckingTypeMask(TextCheckingTypeSpelling > > > | TextCheckingTypeGrammar), textToCheck, true); > > > Please use "bool asynchronous = true" and pass asynchronous to the function > to > > > improve readability. > > Addressed the comments. I also reverted changes from addMarker() for now because > I found a regression caused by them. Will put them to a separate CL after > fixing. This is still a massive improvement so if it's good enough feel free to > send to integration. > See you in 2 weeks :). Could we get back to this?
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pstanek@opera.com/22859062/33001
Failed to apply patch for Source/core/editing/Editor.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
patching file Source/core/editing/Editor.cpp
Hunk #1 FAILED at 419.
Hunk #2 succeeded at 1535 (offset 26 lines).
Hunk #3 succeeded at 1551 (offset 26 lines).
Hunk #4 succeeded at 1563 (offset 26 lines).
1 out of 4 hunks FAILED -- saving rejects to file
Source/core/editing/Editor.cpp.rej
Patch: Source/core/editing/Editor.cpp
Index: Source/core/editing/Editor.cpp
diff --git a/Source/core/editing/Editor.cpp b/Source/core/editing/Editor.cpp
index
b487c8d243b83d1b95f90431143bcee82be08428..c2c7238e148081a70675f2920c60175e54522ea9
100644
--- a/Source/core/editing/Editor.cpp
+++ b/Source/core/editing/Editor.cpp
@@ -419,9 +419,10 @@ void
Editor::replaceSelectionWithFragment(PassRefPtr<DocumentFragment> fragment,
Node* nodeToCheck = m_frame->selection()->rootEditableElement();
if (!nodeToCheck)
return;
-
RefPtr<Range> rangeToCheck = Range::create(m_frame->document(),
firstPositionInNode(nodeToCheck), lastPositionInNode(nodeToCheck));
-
m_spellCheckRequester->requestCheckingFor(SpellCheckRequest::create(resolveTextCheckingTypeMask(TextCheckingTypeSpelling
| TextCheckingTypeGrammar), TextCheckingProcessBatch, rangeToCheck,
rangeToCheck));
+ TextCheckingParagraph textToCheck(rangeToCheck, rangeToCheck);
+ bool asynchronous = true;
+
chunkAndMarkAllMisspellingsAndBadGrammar(resolveTextCheckingTypeMask(TextCheckingTypeSpelling
| TextCheckingTypeGrammar), textToCheck, asynchronous);
}
void Editor::replaceSelectionWithText(const String& text, bool
selectReplacement, bool smartReplace)
@@ -1509,6 +1510,13 @@ void
Editor::markAllMisspellingsAndBadGrammarInRanges(TextCheckingTypeMask textC
Range* rangeToCheck = shouldMarkGrammar ? grammarRange : spellingRange;
TextCheckingParagraph fullParagraphToCheck(rangeToCheck);
+
+ bool asynchronous = m_frame && m_frame->settings() &&
m_frame->settings()->asynchronousSpellCheckingEnabled();
+ chunkAndMarkAllMisspellingsAndBadGrammar(textCheckingOptions,
fullParagraphToCheck, asynchronous);
+}
+
+void Editor::chunkAndMarkAllMisspellingsAndBadGrammar(TextCheckingTypeMask
textCheckingOptions, const TextCheckingParagraph& fullParagraphToCheck, bool
asynchronous)
+{
if (fullParagraphToCheck.isRangeEmpty() || fullParagraphToCheck.isEmpty())
return;
@@ -1518,13 +1526,11 @@ void
Editor::markAllMisspellingsAndBadGrammarInRanges(TextCheckingTypeMask textC
int end = fullParagraphToCheck.checkingEnd();
start = std::min(start, end);
end = std::max(start, end);
- bool asynchronous = m_frame && m_frame->settings() &&
m_frame->settings()->asynchronousSpellCheckingEnabled();
const int kNumChunksToCheck = asynchronous ? (end - start + kChunkSize - 1)
/ (kChunkSize) : 1;
int currentChunkStart = start;
- RefPtr<Range> checkRange = asynchronous ?
fullParagraphToCheck.paragraphRange() : rangeToCheck;
- RefPtr<Range> paragraphRange = fullParagraphToCheck.paragraphRange();
+ RefPtr<Range> checkRange = fullParagraphToCheck.checkingRange();
if (kNumChunksToCheck == 1 && asynchronous) {
- markAllMisspellingsAndBadGrammarInRanges(textCheckingOptions,
checkRange.get(), paragraphRange.get(), asynchronous, 0);
+ markAllMisspellingsAndBadGrammarInRanges(textCheckingOptions,
checkRange.get(), checkRange.get(), asynchronous, 0);
return;
}
@@ -1532,10 +1538,9 @@ void
Editor::markAllMisspellingsAndBadGrammarInRanges(TextCheckingTypeMask textC
checkRange = fullParagraphToCheck.subrange(currentChunkStart,
kChunkSize);
setStart(checkRange.get(),
startOfSentence(checkRange->startPosition()));
setEnd(checkRange.get(), endOfSentence(checkRange->endPosition()));
- paragraphRange = checkRange;
int checkingLength = 0;
- markAllMisspellingsAndBadGrammarInRanges(textCheckingOptions,
checkRange.get(), paragraphRange.get(), asynchronous, iter, &checkingLength);
+ markAllMisspellingsAndBadGrammarInRanges(textCheckingOptions,
checkRange.get(), checkRange.get(), asynchronous, iter, &checkingLength);
currentChunkStart += checkingLength;
}
}
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pstanek@opera.com/22859062/39001
Message was sent while issue was closed.
Change committed as 157747 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
