Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
21130005:
Trigger spell check/remove markers if spell checker gets enabled/disabled. (Closed)
|
Created:
7 years, 4 months ago by pstanek Modified:
7 years, 4 months ago CC:
blink-reviews, dglazkov+blink, eae+blinkwatch Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionTrigger spell check/remove markers if spell checker gets enabled/disabled.
With this change when spell checker is turned off the misspelling markers
disappear from the document (which is what a user expects and this currently
works in chromium because UI code takes care of that - with this change it may
stop). OTOH if spell checker is turned on again one would expect misspellings
will appear right away - in the currently focused editable at least. This is how
Gecko and Presto behave and this is what this patch does. Note it's not only
about restoring old markers because content might have changed or language might
have been switched. This is why new check is requested. This change may also
come in handy if spell checking language is changed. It would require restarting
spell checker on UI side on language change, though.
Since there might be an awful lot of text in the focused editable when spell
checker gets enabled, spell checking is chunked up. If asynchronous spell
checker is enabled whole text is checked. Otherwise only one chunk is checked
(the default chunk's size is set to 16KB but it might also be slightly bigger
since all chunks are aligned to the sentence boundary).
BUG=264737
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156183
Patch Set 1 #
Total comments: 5
Patch Set 2 : Addressing review comments. #Patch Set 3 : Applying here what I've learned from other review. #Patch Set 4 : Remove markers from all frames & avoid freeze when spell checking a lot of text. #Patch Set 5 : copy&paste bug fixed #
Total comments: 3
Patch Set 6 : addressing review comments #Patch Set 7 : Improvement & test for chunked spell checking. #
Total comments: 8
Patch Set 8 : Style & improvements in tests #
Total comments: 3
Patch Set 9 : Added the comment clarifying things a bit more. #
Total comments: 4
Patch Set 10 : style & expected result #Patch Set 11 : rebase & minor WS fix in tests #Patch Set 12 : Revert paste related changes #Messages
Total messages: 37 (0 generated)
Can you add some information to the description that explains what problem this CL solves? Your description says what you're doing but not why. Without understanding why you're making this change, it's hard to review. https://codereview.chromium.org/21130005/diff/1/LayoutTests/editing/spelling/... File LayoutTests/editing/spelling/spellcheck-disable-enable.html (right): https://codereview.chromium.org/21130005/diff/1/LayoutTests/editing/spelling/... LayoutTests/editing/spelling/spellcheck-disable-enable.html:16: internals.settings.setAsynchronousSpellCheckingEnabled(false); Usually we check whether window.internals is available and let the user know that this test requires internals. That way you can run the tests in a browser and understand why it's not working. https://codereview.chromium.org/21130005/diff/1/Source/web/EditorClientImpl.cpp File Source/web/EditorClientImpl.cpp (right): https://codereview.chromium.org/21130005/diff/1/Source/web/EditorClientImpl.c... Source/web/EditorClientImpl.cpp:146: if (frame) No need for "const". There's no meaningful sense in which Frame can be const. Also, you can combine this declaration with the if statement. https://codereview.chromium.org/21130005/diff/1/Source/web/EditorClientImpl.c... Source/web/EditorClientImpl.cpp:151: if (frame) { ditto https://codereview.chromium.org/21130005/diff/1/Source/web/EditorClientImpl.c... Source/web/EditorClientImpl.cpp:155: if (rootEditableElement) { You can also combine this if statement with the previous declaration.
On 2013/07/31 00:42:14, abarth wrote: > Can you add some information to the description that explains what problem this > CL solves? Your description says what you're doing but not why. Without > understanding why you're making this change, it's hard to review. With this change when spell checker is turned off the misspelling markers disappear from the document (which is what a user expects and this currently works in chromium because UI code takes care of that - with this change it may stop). OTOH if spell checker is turned on again one would expect misspellings will appear right away - in the currently focused editable at least. This is how Gecko and Presto behave and this is what this patch does. Note it's not only about restoring old markers because content might have changed or language might have been switched. This is why new check is requested. This change may also come in handy if spell checking language is changed. It would require restarting spell checker on UI side on language change, though. > > https://codereview.chromium.org/21130005/diff/1/LayoutTests/editing/spelling/... > File LayoutTests/editing/spelling/spellcheck-disable-enable.html (right): > > https://codereview.chromium.org/21130005/diff/1/LayoutTests/editing/spelling/... > LayoutTests/editing/spelling/spellcheck-disable-enable.html:16: > internals.settings.setAsynchronousSpellCheckingEnabled(false); > Usually we check whether window.internals is available and let the user know > that this test requires internals. That way you can run the tests in a browser > and understand why it's not working. > > https://codereview.chromium.org/21130005/diff/1/Source/web/EditorClientImpl.cpp > File Source/web/EditorClientImpl.cpp (right): > > https://codereview.chromium.org/21130005/diff/1/Source/web/EditorClientImpl.c... > Source/web/EditorClientImpl.cpp:146: if (frame) > No need for "const". There's no meaningful sense in which Frame can be const. > > Also, you can combine this declaration with the if statement. > > https://codereview.chromium.org/21130005/diff/1/Source/web/EditorClientImpl.c... > Source/web/EditorClientImpl.cpp:151: if (frame) { > ditto > > https://codereview.chromium.org/21130005/diff/1/Source/web/EditorClientImpl.c... > Source/web/EditorClientImpl.cpp:155: if (rootEditableElement) { > You can also combine this if statement with the previous declaration.
Thanks, that's a great description. Would you be willing to include that in the description field so that it gets included in the commit message?
New patch set added.
https://codereview.chromium.org/21130005/diff/1/Source/web/EditorClientImpl.cpp File Source/web/EditorClientImpl.cpp (right): https://codereview.chromium.org/21130005/diff/1/Source/web/EditorClientImpl.c... Source/web/EditorClientImpl.cpp:146: if (frame) Won't this remove markers in one frame only (it should do it for the whole page)? It seems that the main frame from the page should be get here and the frame tree should be traversed.
On 2013/08/12 12:21:23, pstanek wrote: > https://codereview.chromium.org/21130005/diff/1/Source/web/EditorClientImpl.cpp > File Source/web/EditorClientImpl.cpp (right): > > https://codereview.chromium.org/21130005/diff/1/Source/web/EditorClientImpl.c... > Source/web/EditorClientImpl.cpp:146: if (frame) > Won't this remove markers in one frame only (it should do it for the whole > page)? It seems that the main frame from the page should be get here and the > frame tree should be traversed. Done. The freeze when spell check faces a lot of text (either when turning it on when some editable is focused or when pasting a lot of text to an editable) is fixed now too.
morrita or rouslan should review this change as well. Where is the test case for checking more than a single chunk? https://codereview.chromium.org/21130005/diff/30001/Source/core/editing/Edito... File Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/21130005/diff/30001/Source/core/editing/Edito... Source/core/editing/Editor.cpp:1514: static const int chunkSize = 8 * 1024; Nit: kChunkSize. Also, I'm not sure static buys you anything here. How did you pick 8k? That seems really small to me. When checking 1MB of text, does that mean we'll make 128 HTTP requests? https://codereview.chromium.org/21130005/diff/30001/Source/core/editing/Edito... Source/core/editing/Editor.cpp:1531: RefPtr<SpellCheckRequest> request = SpellCheckRequest::create(resolveTextCheckingTypeMask(textCheckingOptions), TextCheckingProcessBatch, checkRange, checkRange, 0/*iter*/); I don't think you need to pass in the last parameter here. https://codereview.chromium.org/21130005/diff/30001/Source/core/editing/Spell... File Source/core/editing/SpellChecker.h (right): https://codereview.chromium.org/21130005/diff/30001/Source/core/editing/Spell... Source/core/editing/SpellChecker.h:49: static PassRefPtr<SpellCheckRequest> create(TextCheckingTypeMask, TextCheckingProcessType, PassRefPtr<Range> checkingRange, PassRefPtr<Range> paragraphRange, int requestNo); resquestNo -> requestNumber. We don't normally use abbreviations in blink code. Also, I would merge this with the above create() method and give requestNumber a default value of 0.
On 2013/08/12 19:45:14, tony wrote: > morrita or rouslan should review this change as well. > > Where is the test case for checking more than a single chunk? > > https://codereview.chromium.org/21130005/diff/30001/Source/core/editing/Edito... > File Source/core/editing/Editor.cpp (right): > > https://codereview.chromium.org/21130005/diff/30001/Source/core/editing/Edito... > Source/core/editing/Editor.cpp:1514: static const int chunkSize = 8 * 1024; > Nit: kChunkSize. Also, I'm not sure static buys you anything here. > > How did you pick 8k? That seems really small to me. When checking 1MB of text, > does that mean we'll make 128 HTTP requests? I considered it quite big but not too big. > > https://codereview.chromium.org/21130005/diff/30001/Source/core/editing/Edito... > Source/core/editing/Editor.cpp:1531: RefPtr<SpellCheckRequest> request = > SpellCheckRequest::create(resolveTextCheckingTypeMask(textCheckingOptions), > TextCheckingProcessBatch, checkRange, checkRange, 0/*iter*/); > I don't think you need to pass in the last parameter here. Ah, this was just a temporary last minute change to check something and it seems I forgot to revert it. > > https://codereview.chromium.org/21130005/diff/30001/Source/core/editing/Spell... > File Source/core/editing/SpellChecker.h (right): > > https://codereview.chromium.org/21130005/diff/30001/Source/core/editing/Spell... > Source/core/editing/SpellChecker.h:49: static PassRefPtr<SpellCheckRequest> > create(TextCheckingTypeMask, TextCheckingProcessType, PassRefPtr<Range> > checkingRange, PassRefPtr<Range> paragraphRange, int requestNo); > resquestNo -> requestNumber. We don't normally use abbreviations in blink code. > > Also, I would merge this with the above create() method and give requestNumber a > default value of 0.
On 2013/08/12 22:10:25, pstanek wrote: > On 2013/08/12 19:45:14, tony wrote: > > morrita or rouslan should review this change as well. > > > > Where is the test case for checking more than a single chunk? > > > > > https://codereview.chromium.org/21130005/diff/30001/Source/core/editing/Edito... > > File Source/core/editing/Editor.cpp (right): > > > > > https://codereview.chromium.org/21130005/diff/30001/Source/core/editing/Edito... > > Source/core/editing/Editor.cpp:1514: static const int chunkSize = 8 * 1024; > > Nit: kChunkSize. Also, I'm not sure static buys you anything here. > > > > How did you pick 8k? That seems really small to me. When checking 1MB of > text, > > does that mean we'll make 128 HTTP requests? > > I considered it quite big but not too big. > > > > > > https://codereview.chromium.org/21130005/diff/30001/Source/core/editing/Edito... > > Source/core/editing/Editor.cpp:1531: RefPtr<SpellCheckRequest> request = > > SpellCheckRequest::create(resolveTextCheckingTypeMask(textCheckingOptions), > > TextCheckingProcessBatch, checkRange, checkRange, 0/*iter*/); > > I don't think you need to pass in the last parameter here. > > Ah, this was just a temporary last minute change to check something and it seems > I forgot to revert it. > > > > > > https://codereview.chromium.org/21130005/diff/30001/Source/core/editing/Spell... > > File Source/core/editing/SpellChecker.h (right): > > > > > https://codereview.chromium.org/21130005/diff/30001/Source/core/editing/Spell... > > Source/core/editing/SpellChecker.h:49: static PassRefPtr<SpellCheckRequest> > > create(TextCheckingTypeMask, TextCheckingProcessType, PassRefPtr<Range> > > checkingRange, PassRefPtr<Range> paragraphRange, int requestNo); > > resquestNo -> requestNumber. We don't normally use abbreviations in blink > code. > > > > Also, I would merge this with the above create() method and give requestNumber > a > > default value of 0. Added the test and addressed previous comments.
Great job. Please see inline comments. (Also please indent the patch description uniformly.) https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Edito... File Source/core/editing/Editor.cpp (right): https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Edito... Source/core/editing/Editor.cpp:1519: const int loopNum = (end - start + kChunkSize - 1) / (kChunkSize); Nit: rename loopNum to "numChunksToCheck". (Number of chunks to check). https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Edito... Source/core/editing/Editor.cpp:1529: bool asynchronous = m_frame && m_frame->settings() && m_frame->settings()->asynchronousSpellCheckingEnabled(); The "asynchronous" variable can live outside of the for loop. https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Edito... Source/core/editing/Editor.cpp:1539: checkTextOfParagraph(textChecker(), sentenceToCheck.text(), resolveTextCheckingTypeMask(textCheckingOptions), results); If you're splitting up the spellcheck requests for the synchronous case, too, then please add a layout test with asynchronous spellchecking disabled. For example, you could call it spelling-huge-text-sync.html and base it on the existing spelling-huge-text.html that you wrote. https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Edito... Source/core/editing/Editor.cpp:1541: return; It's not clear from this code that you spellcheck only one chunk with synchronous spellcheck. Perhaps you could do this instead: int loopNum = asynchronous ? (end - start + kChunkSize - 1) / (kChunkSize) : 1; Then you don't need the "return" statement here. https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Spell... File Source/core/editing/SpellChecker.cpp (right): https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Spell... Source/core/editing/SpellChecker.cpp:212: bool continuation = false; This code appears to have a bug: it enqueues all odd-numbered requests. For example, if I have four requests with request numbers 0, 1, 2, and 3, then: - Request 0 will be enqueued, because m_requestQueue.size() == 0. - Request 1 will not be enqueued, because request number 1 == 0 + 1. - Request 2 will be enqueued, because m_requestQueue.size() > 0 and 2 != 0 + 1. - Request 3 will not be enqueued, because 3 == 2 + 1. And so on... I don't think that your code was intended to skip all even requests. Please add a comment that describes what the code is trying to do and double-check that your code accomplishes that task correctly. https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Spell... File Source/core/editing/SpellChecker.h (right): https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Spell... Source/core/editing/SpellChecker.h:63: int requestNumber() { return m_requestNumber; } Nit: Please add a "const" keyword after the method declaration: int requestNumber() const { return m_requestNumber; } https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Spell... Source/core/editing/SpellChecker.h:66: SpellCheckRequest(PassRefPtr<Range> checkingRange, PassRefPtr<Range> paragraphRange, const String&, TextCheckingTypeMask, TextCheckingProcessType, const Vector<uint32_t>& documentMarkersInRange, const Vector<unsigned>& documentMarkerOffsets, int requestNo); Nit: Please be consistent in naming: s/requestNo/requestNumber/
I just looked at the test. It sounds like 8k was picked arbitrarily and seems small. How long does it take to check 8k of text on the main thread? It would be awesome if we could check until we used a certain amount of time, but it would also be OK to measure on your desktop to see how much you could check in a small amount of time (e.g., 10ms). https://codereview.chromium.org/21130005/diff/38001/LayoutTests/editing/spell... File LayoutTests/editing/spelling/spelling-huge-text.html (right): https://codereview.chromium.org/21130005/diff/38001/LayoutTests/editing/spell... LayoutTests/editing/spelling/spelling-huge-text.html:42: window.setTimeout(function() { waitForMarkersToAppear(node, nretry - 1); }, 1000); Tests should finish quickly. We shouldn't be waiting 1s for a response. It would be better to use a fast timeout (5ms?) and check frequently. How long does this test take to run on your machine? Even in debug, we shouldn't check in a test that takes more than a second.
On 2013/08/13 22:06:14, tony wrote: > I just looked at the test. > > It sounds like 8k was picked arbitrarily and seems small. How long does it take > to check 8k of text on the main thread? It would be awesome if we could check > until we used a certain amount of time, but it would also be OK to measure on > your desktop to see how much you could check in a small amount of time (e.g., > 10ms). > > https://codereview.chromium.org/21130005/diff/38001/LayoutTests/editing/spell... > File LayoutTests/editing/spelling/spelling-huge-text.html (right): > > https://codereview.chromium.org/21130005/diff/38001/LayoutTests/editing/spell... > LayoutTests/editing/spelling/spelling-huge-text.html:42: > window.setTimeout(function() { waitForMarkersToAppear(node, nretry - 1); }, > 1000); > Tests should finish quickly. We shouldn't be waiting 1s for a response. It > would be better to use a fast timeout (5ms?) and check frequently. How long > does this test take to run on your machine? Even in debug, we shouldn't check > in a test that takes more than a second. I increased the chunk size to 16k because this looked ok in debug and it's snappy in release. 32k works ok in release too on my machine but in debug I got some glitches when scrolling the editable (so might happen with slow hardware). Same with 64k in debug - didn't check in release. The test finishes in about 5-7 secs currently in debug on my machine (I saw that timeout is ~35sec).
On 2013/08/13 21:54:21, Rouslan Solomakhin wrote: > Great job. Please see inline comments. (Also please indent the patch description > uniformly.) > > https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Edito... > File Source/core/editing/Editor.cpp (right): > > https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Edito... > Source/core/editing/Editor.cpp:1519: const int loopNum = (end - start + > kChunkSize - 1) / (kChunkSize); > Nit: rename loopNum to "numChunksToCheck". (Number of chunks to check). > > https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Edito... > Source/core/editing/Editor.cpp:1529: bool asynchronous = m_frame && > m_frame->settings() && m_frame->settings()->asynchronousSpellCheckingEnabled(); > The "asynchronous" variable can live outside of the for loop. > > https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Edito... > Source/core/editing/Editor.cpp:1539: checkTextOfParagraph(textChecker(), > sentenceToCheck.text(), resolveTextCheckingTypeMask(textCheckingOptions), > results); > If you're splitting up the spellcheck requests for the synchronous case, too, > then please add a layout test with asynchronous spellchecking disabled. For > example, you could call it spelling-huge-text-sync.html and base it on the > existing spelling-huge-text.html that you wrote. > > https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Edito... > Source/core/editing/Editor.cpp:1541: return; > It's not clear from this code that you spellcheck only one chunk with > synchronous spellcheck. Perhaps you could do this instead: > > int loopNum = asynchronous ? > (end - start + kChunkSize - 1) / (kChunkSize) : 1; > > Then you don't need the "return" statement here. > > https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Spell... > File Source/core/editing/SpellChecker.cpp (right): > > https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Spell... > Source/core/editing/SpellChecker.cpp:212: bool continuation = false; > This code appears to have a bug: it enqueues all odd-numbered requests. For > example, if I have four requests with request numbers 0, 1, 2, and 3, then: > > - Request 0 will be enqueued, because m_requestQueue.size() == 0. > - Request 1 will not be enqueued, because request number 1 == 0 + 1. > - Request 2 will be enqueued, because m_requestQueue.size() > 0 and 2 != 0 + 1. > - Request 3 will not be enqueued, because 3 == 2 + 1. > > And so on... > > I don't think that your code was intended to skip all even requests. Please add > a comment that describes what the code is trying to do and double-check that > your code accomplishes that task correctly. I look at the number of the last queued request and the number of the one to queue. So: - Request 0 will be enqueued, because m_requestQueue.size() == 0. - Request 1 will be enqueued, because m_requestQueue.size() > 0 and request number 1 == 0 (last) + 1. - Request 2 will be enqueued, because m_requestQueue.size() > 0 and 2 == 1 (last) + 1. etc. This is how it was intended at least. > > https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Spell... > File Source/core/editing/SpellChecker.h (right): > > https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Spell... > Source/core/editing/SpellChecker.h:63: int requestNumber() { return > m_requestNumber; } > Nit: Please add a "const" keyword after the method declaration: > > int requestNumber() const { return m_requestNumber; } > > https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Spell... > Source/core/editing/SpellChecker.h:66: SpellCheckRequest(PassRefPtr<Range> > checkingRange, PassRefPtr<Range> paragraphRange, const String&, > TextCheckingTypeMask, TextCheckingProcessType, const Vector<uint32_t>& > documentMarkersInRange, const Vector<unsigned>& documentMarkerOffsets, int > requestNo); > Nit: Please be consistent in naming: s/requestNo/requestNumber/
On 2013/08/13 22:17:33, pstanek wrote: > On 2013/08/13 22:06:14, tony wrote: > > I just looked at the test. > > > > It sounds like 8k was picked arbitrarily and seems small. How long does it > take > > to check 8k of text on the main thread? It would be awesome if we could check > > until we used a certain amount of time, but it would also be OK to measure on > > your desktop to see how much you could check in a small amount of time (e.g., > > 10ms). > > > > > https://codereview.chromium.org/21130005/diff/38001/LayoutTests/editing/spell... > > File LayoutTests/editing/spelling/spelling-huge-text.html (right): > > > > > https://codereview.chromium.org/21130005/diff/38001/LayoutTests/editing/spell... > > LayoutTests/editing/spelling/spelling-huge-text.html:42: > > window.setTimeout(function() { waitForMarkersToAppear(node, nretry - 1); }, > > 1000); > > Tests should finish quickly. We shouldn't be waiting 1s for a response. It > > would be better to use a fast timeout (5ms?) and check frequently. How long > > does this test take to run on your machine? Even in debug, we shouldn't check > > in a test that takes more than a second. > > I increased the chunk size to 16k because this looked ok in debug and it's > snappy in release. 32k works ok in release too on my machine but in debug I got > some glitches when scrolling the editable (so might happen with slow hardware). > Same with 64k in debug - didn't check in release. > The test finishes in about 5-7 secs currently in debug on my machine (I saw that > timeout is ~35sec). I fixed the timeout and you're right: even 1ms and 5 retries is enough. All time this test takes in debug is needed to prepare the text to check (the for loop doing elm.innerText += ...) !?!? In release it's a blink ;).
On 2013/08/13 22:55:36, pstanek wrote: > I fixed the timeout and you're right: even 1ms and 5 retries is enough. All time > this test takes in debug is needed to prepare the text to check (the for loop > doing elm.innerText += ...) !?!? In release it's a blink ;). Please fix this. Instead of appending to innerText (which might be forcing a layout), we should keep a temporary string and assign it to innerText once. You could probably also double the string rather than appending a fixed amount to have fewer mallocs.
On 2013/08/13 23:06:19, tony wrote: > On 2013/08/13 22:55:36, pstanek wrote: > > I fixed the timeout and you're right: even 1ms and 5 retries is enough. All > time > > this test takes in debug is needed to prepare the text to check (the for loop > > doing elm.innerText += ...) !?!? In release it's a blink ;). > > Please fix this. Instead of appending to innerText (which might be forcing a > layout), we should keep a temporary string and assign it to innerText once. You > could probably also double the string rather than appending a fixed amount to > have fewer mallocs. Fixed.
On 2013/08/13 21:54:21, Rouslan Solomakhin wrote: > Great job. Please see inline comments. (Also please indent the patch description > uniformly.) > > https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Edito... > File Source/core/editing/Editor.cpp (right): > > https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Edito... > Source/core/editing/Editor.cpp:1519: const int loopNum = (end - start + > kChunkSize - 1) / (kChunkSize); > Nit: rename loopNum to "numChunksToCheck". (Number of chunks to check). > > https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Edito... > Source/core/editing/Editor.cpp:1529: bool asynchronous = m_frame && > m_frame->settings() && m_frame->settings()->asynchronousSpellCheckingEnabled(); > The "asynchronous" variable can live outside of the for loop. > > https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Edito... > Source/core/editing/Editor.cpp:1539: checkTextOfParagraph(textChecker(), > sentenceToCheck.text(), resolveTextCheckingTypeMask(textCheckingOptions), > results); > If you're splitting up the spellcheck requests for the synchronous case, too, > then please add a layout test with asynchronous spellchecking disabled. For > example, you could call it spelling-huge-text-sync.html and base it on the > existing spelling-huge-text.html that you wrote. > > https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Edito... > Source/core/editing/Editor.cpp:1541: return; > It's not clear from this code that you spellcheck only one chunk with > synchronous spellcheck. Perhaps you could do this instead: > > int loopNum = asynchronous ? > (end - start + kChunkSize - 1) / (kChunkSize) : 1; > > Then you don't need the "return" statement here. > > https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Spell... > File Source/core/editing/SpellChecker.cpp (right): > > https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Spell... > Source/core/editing/SpellChecker.cpp:212: bool continuation = false; > This code appears to have a bug: it enqueues all odd-numbered requests. For > example, if I have four requests with request numbers 0, 1, 2, and 3, then: > > - Request 0 will be enqueued, because m_requestQueue.size() == 0. > - Request 1 will not be enqueued, because request number 1 == 0 + 1. > - Request 2 will be enqueued, because m_requestQueue.size() > 0 and 2 != 0 + 1. > - Request 3 will not be enqueued, because 3 == 2 + 1. > > And so on... > > I don't think that your code was intended to skip all even requests. Please add > a comment that describes what the code is trying to do and double-check that > your code accomplishes that task correctly. > > https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Spell... > File Source/core/editing/SpellChecker.h (right): > > https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Spell... > Source/core/editing/SpellChecker.h:63: int requestNumber() { return > m_requestNumber; } > Nit: Please add a "const" keyword after the method declaration: > > int requestNumber() const { return m_requestNumber; } > > https://codereview.chromium.org/21130005/diff/38001/Source/core/editing/Spell... > Source/core/editing/SpellChecker.h:66: SpellCheckRequest(PassRefPtr<Range> > checkingRange, PassRefPtr<Range> paragraphRange, const String&, > TextCheckingTypeMask, TextCheckingProcessType, const Vector<uint32_t>& > documentMarkersInRange, const Vector<unsigned>& documentMarkerOffsets, int > requestNo); > Nit: Please be consistent in naming: s/requestNo/requestNumber/ All done.
PTAL
LGTM with a nit. https://codereview.chromium.org/21130005/diff/52001/Source/core/editing/Spell... File Source/core/editing/SpellChecker.cpp (right): https://codereview.chromium.org/21130005/diff/52001/Source/core/editing/Spell... Source/core/editing/SpellChecker.cpp:216: // both apply to the same editable. Ah, I see what this is doing now. Enqueueing a request used to overwrite the request for the same element. You've made sure that spellcheck requests for chunks of text for the same element do not overwrite each other. Good job! (No action necessary.) https://codereview.chromium.org/21130005/diff/52001/Source/core/editing/Spell... Source/core/editing/SpellChecker.cpp:221: if (!continuation) { Nit: Maybe add a comment here: // Spellcheck requests for chunks of text in the same element should not overwrite each other.
https://codereview.chromium.org/21130005/diff/52001/Source/core/editing/Spell... File Source/core/editing/SpellChecker.cpp (right): https://codereview.chromium.org/21130005/diff/52001/Source/core/editing/Spell... Source/core/editing/SpellChecker.cpp:221: if (!continuation) { On 2013/08/14 17:30:28, Rouslan Solomakhin wrote: > Nit: Maybe add a comment here: > > // Spellcheck requests for chunks of text in the same element should not > overwrite each other. Done.
When using the "ask google for suggestions" option, do we make a separate http request for each request chunk? It seems like this might hammer the spelling server since we can check 16k so quickly.
On 2013/08/14 22:55:31, tony wrote: > When using the "ask google for suggestions" option, do we make a separate http > request for each request chunk? Yes. > It seems like this might hammer the spelling server since we can check 16k so quickly. This shouldn't be a problem. The spelling server can handle many spelling requests. In fact, the server engineers have asked us to split up our long requests into smaller chunks, because that can be handled more efficiently.
LGTM with some style fixes. I'm surprised the spelling team doesn't want some sort of exponential back off behavior (most server teams do), but if you say it's OK, I trust you. https://codereview.chromium.org/21130005/diff/57001/LayoutTests/editing/spell... File LayoutTests/editing/spelling/spelling-huge-text-expected.txt (right): https://codereview.chromium.org/21130005/diff/57001/LayoutTests/editing/spell... LayoutTests/editing/spelling/spelling-huge-text-expected.txt:9: PASS internals.markerCountForNode(testEditable.childNodes[0], "spelling") is 6 Please remove the text from the dom after we've checked the right number of markers. No need to check in lots of repetitive text to svn/git. https://codereview.chromium.org/21130005/diff/57001/LayoutTests/editing/spell... File LayoutTests/editing/spelling/spelling-huge-text-sync-expected.txt (right): https://codereview.chromium.org/21130005/diff/57001/LayoutTests/editing/spell... LayoutTests/editing/spelling/spelling-huge-text-sync-expected.txt:9: PASS internals.markerCountForNode(testEditable.childNodes[0], "spelling") is 3 Same with the text in this result. https://codereview.chromium.org/21130005/diff/57001/Source/core/editing/Spell... File Source/core/editing/SpellChecker.cpp (right): https://codereview.chromium.org/21130005/diff/57001/Source/core/editing/Spell... Source/core/editing/SpellChecker.cpp:213: if (m_requestQueue.size() > 0) { !m_requestQueue.isEmpty()
https://codereview.chromium.org/21130005/diff/57001/LayoutTests/editing/spell... File LayoutTests/editing/spelling/spelling-huge-text-expected.txt (right): https://codereview.chromium.org/21130005/diff/57001/LayoutTests/editing/spell... LayoutTests/editing/spelling/spelling-huge-text-expected.txt:9: PASS internals.markerCountForNode(testEditable.childNodes[0], "spelling") is 6 On 2013/08/15 00:01:40, tony wrote: > Please remove the text from the dom after we've checked the right number of > markers. No need to check in lots of repetitive text to svn/git. To be clear, I mean the "zz zz zz. Good good ..." text, not the PASS text.
On 2013/08/15 00:02:29, tony wrote: > https://codereview.chromium.org/21130005/diff/57001/LayoutTests/editing/spell... > File LayoutTests/editing/spelling/spelling-huge-text-expected.txt (right): > > https://codereview.chromium.org/21130005/diff/57001/LayoutTests/editing/spell... > LayoutTests/editing/spelling/spelling-huge-text-expected.txt:9: PASS > internals.markerCountForNode(testEditable.childNodes[0], "spelling") is 6 > On 2013/08/15 00:01:40, tony wrote: > > Please remove the text from the dom after we've checked the right number of > > markers. No need to check in lots of repetitive text to svn/git. > > To be clear, I mean the "zz zz zz. Good good ..." text, not the PASS text. Done. PTAL Tony and queue for integration if still lgty, please (as you always do anyway :)). Thanks in advance.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pstanek@opera.com/21130005/66001
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 421.
Hunk #3 FAILED at 1506.
2 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
5931025ea9a51c20da854ecf48fcb311eb240469..1354d49a2408bb1601f932aaa47c037e42b2e59a
100644
--- a/Source/core/editing/Editor.cpp
+++ b/Source/core/editing/Editor.cpp
@@ -421,7 +421,7 @@ void
Editor::replaceSelectionWithFragment(PassRefPtr<DocumentFragment> fragment,
return;
RefPtr<Range> rangeToCheck = Range::create(m_frame->document(),
firstPositionInNode(nodeToCheck), lastPositionInNode(nodeToCheck));
-
m_spellChecker->requestCheckingFor(SpellCheckRequest::create(resolveTextCheckingTypeMask(TextCheckingTypeSpelling
| TextCheckingTypeGrammar), TextCheckingProcessBatch, rangeToCheck,
rangeToCheck));
+
markAllMisspellingsAndBadGrammarInRanges(resolveTextCheckingTypeMask(TextCheckingTypeSpelling
| TextCheckingTypeGrammar), rangeToCheck.get(), rangeToCheck.get());
}
void Editor::replaceSelectionWithText(const String& text, bool
selectReplacement, bool smartReplace)
@@ -1486,9 +1486,12 @@ void Editor::markBadGrammar(const VisibleSelection&
selection)
void Editor::markAllMisspellingsAndBadGrammarInRanges(TextCheckingTypeMask
textCheckingOptions, Range* spellingRange, Range* grammarRange)
{
ASSERT(m_frame);
- ASSERT(unifiedTextCheckerEnabled());
- bool shouldMarkGrammar = textCheckingOptions & TextCheckingTypeGrammar;
+ bool shouldMarkGrammar = (textCheckingOptions & TextCheckingTypeGrammar) &&
isGrammarCheckingEnabled();
+ bool shouldMarkSpellling = (textCheckingOptions & TextCheckingTypeSpelling)
&& isContinuousSpellCheckingEnabled();
+
+ if (!shouldMarkSpellling && !shouldMarkGrammar)
+ return;
// This function is called with selections already expanded to word
boundaries.
if (!client() || !spellingRange || (shouldMarkGrammar && !grammarRange))
@@ -1503,24 +1506,38 @@ void
Editor::markAllMisspellingsAndBadGrammarInRanges(TextCheckingTypeMask textC
return;
Range* rangeToCheck = shouldMarkGrammar ? grammarRange : spellingRange;
- TextCheckingParagraph paragraphToCheck(rangeToCheck);
- if (paragraphToCheck.isRangeEmpty() || paragraphToCheck.isEmpty())
+ TextCheckingParagraph fullParagraphToCheck(rangeToCheck);
+ if (fullParagraphToCheck.isRangeEmpty() || fullParagraphToCheck.isEmpty())
return;
- RefPtr<Range> paragraphRange = paragraphToCheck.paragraphRange();
+ // Since the text may be quite big chunk it up and adjust to the sentence
boundary.
+ const int kChunkSize = 16 * 1024;
+ int start = fullParagraphToCheck.checkingStart();
+ 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;
+ for (int iter = 0; iter < kNumChunksToCheck; ++iter) {
+ RefPtr<Range> checkRange =
fullParagraphToCheck.subrange(currentChunkStart, kChunkSize);
+ setStart(checkRange.get(),
startOfSentence(checkRange->startPosition()));
+ setEnd(checkRange.get(), endOfSentence(checkRange->endPosition()));
+ TextCheckingParagraph sentenceToCheck(checkRange, checkRange);
- // In asynchronous mode, we intentionally check paragraph-wide sentence.
- RefPtr<SpellCheckRequest> request =
SpellCheckRequest::create(resolveTextCheckingTypeMask(textCheckingOptions),
TextCheckingProcessIncremental, asynchronous ? paragraphRange : rangeToCheck,
paragraphRange);
+ currentChunkStart += sentenceToCheck.checkingLength();
- if (asynchronous) {
- m_spellChecker->requestCheckingFor(request);
- return;
- }
+ RefPtr<SpellCheckRequest> request =
SpellCheckRequest::create(resolveTextCheckingTypeMask(textCheckingOptions),
TextCheckingProcessBatch, checkRange, checkRange, iter);
+
+ if (asynchronous) {
+ m_spellChecker->requestCheckingFor(request);
+ continue;
+ }
- Vector<TextCheckingResult> results;
- checkTextOfParagraph(textChecker(), paragraphToCheck.text(),
resolveTextCheckingTypeMask(textCheckingOptions), results);
- markAndReplaceFor(request, results);
+ Vector<TextCheckingResult> results;
+ checkTextOfParagraph(textChecker(), sentenceToCheck.text(),
resolveTextCheckingTypeMask(textCheckingOptions), results);
+ markAndReplaceFor(request, results);
+ }
}
void Editor::markAndReplaceFor(PassRefPtr<SpellCheckRequest> request, const
Vector<TextCheckingResult>& results)
@@ -1556,7 +1573,7 @@ void
Editor::markAndReplaceFor(PassRefPtr<SpellCheckRequest> request, const Vect
for (unsigned i = 0; i < results.size(); i++) {
int spellingRangeEndOffset = paragraph.checkingEnd();
const TextCheckingResult* result = &results[i];
- int resultLocation = result->location;
+ int resultLocation = result->location + paragraph.checkingStart();
int resultLength = result->length;
bool resultEndsAtAmbiguousBoundary = ambiguousBoundaryOffset >= 0 &&
resultLocation + resultLength == ambiguousBoundaryOffset;
Pawel: Please rebase your patch.
On 2013/08/15 15:32:45, Rouslan Solomakhin wrote: > Pawel: Please rebase your patch. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pstanek@opera.com/21130005/35001
Retried try job too often on linux_blink_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...
On 2013/08/15 19:33:45, I haz the power (commit-bot) wrote: > Retried try job too often on linux_blink_rel for step(s) webkit_tests > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin... These failures look real. Looks like this patch regresses some paste tests.
On 2013/08/15 20:07:36, tony wrote: > On 2013/08/15 19:33:45, I haz the power (commit-bot) wrote: > > Retried try job too often on linux_blink_rel for step(s) webkit_tests > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin... > > These failures look real. Looks like this patch regresses some paste tests. Paste related changes were done so freeze when pasting is fixed. It seems I was too eager and since those tests pass here on windows I'll just revert them and see how it goes without them.
On 2013/08/15 21:47:48, pstanek wrote: > On 2013/08/15 20:07:36, tony wrote: > > On 2013/08/15 19:33:45, I haz the power (commit-bot) wrote: > > > Retried try job too often on linux_blink_rel for step(s) webkit_tests > > > > > > http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin... > > > > These failures look real. Looks like this patch regresses some paste tests. > > Paste related changes were done so freeze when pasting is fixed. It seems I was > too eager and since those tests pass here on windows I'll just revert them and > see how it goes without them. Reverted.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pstanek@opera.com/21130005/99001
Message was sent while issue was closed.
Change committed as 156183 |
