Bug 72939

Created attachment 117160 [details] Patch

Comment on attachment 117160 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117160&action=review Thanks for doing this. Let us iterate once. > Source/WebCore/ChangeLog:8 > + Now Spellchecker saves a request when it is processing the previous spellcheck request. nit: Spellchecker -> SpellChecker. > Source/WebCore/editing/SpellChecker.cpp:48 > +class SpellChecker::SpellCheckRequest { Making a class like this looks a good idea. Thanks for doing this. > Source/WebCore/editing/SpellChecker.cpp:59 > + PassRefPtr<Range> range() const { return m_range; } Use Range* because this API doesn't release the ownership of the range (Making a PassRefPtr instance causes m_range to become null.) > Source/WebCore/editing/SpellChecker.cpp:89 > +PassOwnPtr<SpellChecker::SpellCheckRequest> SpellChecker::initRequest(TextCheckingTypeMask mask, PassRefPtr<Range> range) Now this becomes a factory method so calling "createRequest" or something sounds more conventional. > Source/WebCore/editing/SpellChecker.cpp:130 > + return m_processingRequest.get() && m_processingRequest->text().length() && m_processingRequest->sequence() == sequence; Is it possible to |!m_processingRequest->text().length()| ? > Source/WebCore/editing/SpellChecker.cpp:151 > + if (!request.get()) OwnPtr has operator! so you don't need get(). > Source/WebCore/editing/SpellChecker.cpp:154 > + if (m_processingRequest.get()) You don't need get() here. It uses UnspecifiedBool pattern. > Source/WebCore/editing/SpellChecker.cpp:155 > + m_queuedRequest = request.release(); I guess you don't need release(). Also, let's return early here to eliminate following else clause. > Source/WebCore/editing/SpellChecker.cpp:158 > + m_processingRequest = request.release(); I guess you don't need release(). > Source/WebCore/editing/SpellChecker.cpp:198 > + if (!isCheckable(m_processingRequest->range().get())) { Is this can be false? I guess original check had a intention to guard an accidental call of didCheck(). Now we checked it in isValid() so we no longer need this change. > Source/WebCore/editing/SpellChecker.h:67 > + int m_maxRequestedSequence; Sounds like this is the upper bound of the sequence. How about "last" or something? > Source/WebCore/editing/SpellChecker.h:69 > + Timer<SpellChecker> m_timer; Please give a name which explains the purpose of the timer. > LayoutTests/editing/spelling/spellcheck-queue.html:69 > + ok = ok && layoutTestController.hasSpellingMarker(expectedMarked[i][0], expectedMarked[i][1]); Let's use |window.internals|. I'd love to kill hasSpellingMarker someday. > LayoutTests/editing/spelling/spellcheck-queue.html:87 > +function verify(id, source, dest, expectedMarked) Why do we need |id| ? > LayoutTests/editing/spelling/spellcheck-queue.html:112 > +function pasteAndVerify(source, dests, expectedMarked) It doesn't looks to do any verify. Is the name intentional? > LayoutTests/editing/spelling/spellcheck-queue.html:122 > +done(); done() here looks storage because the test is just started, right?

Created attachment 117330 [details] Patch

Comment on attachment 117330 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117330&action=review Let's have another round. I realized that the problem is more tricky than what I thought.... > Source/WebCore/editing/SpellChecker.cpp:102 > + if (!m_queuedRequest.get()) Is this possible to true? If not, please add ASSERT(). It's ok to return here for safety, though. > Source/WebCore/editing/SpellChecker.cpp:105 > + m_processingRequest = m_queuedRequest.release(); I think it is possible to overwrite existing m_processingRequest here and it's node desirable. > Source/WebCore/editing/SpellChecker.cpp:112 > + if (!m_queuedRequest.get()) NIt: you don't need get() here.

Created attachment 117377 [details] Patch

Comment on attachment 117377 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117377&action=review > Source/WebCore/editing/SpellChecker.cpp:134 > { It looks we now can merge doRequestCheckingFor and requestCheckingFor(). > Source/WebCore/editing/SpellChecker.cpp:162 > + if (request->rootEditableElement() != (*it)->rootEditableElement()) Could you store |request->rootEditableElement()| to a local var to save extra computation? > Source/WebCore/editing/SpellChecker.cpp:165 > + fprintf(stderr, "REMOVED!\n"); Please remove this.

Created attachment 117385 [details] Patch

Comment on attachment 117385 [details] Patch Clearing flags on attachment: 117385 Committed r101731: <http://trac.webkit.org/changeset/101731>

All reviewed patches have been landed. Closing bug.

Reopening to attach new patch.

Created attachment 117819 [details] Patch

Note that original patch was rolled out. See Bug 73706.

Sorry for Bug 73706. The implementation of invokeRequest() was broken. In chromium, requestCheckingOfString was synchronous. So didCheck was called before returning requestCheckingOfString. I've changed the code so that m_processingRequest is set before invoking requestCheckingOfString. (In reply to comment #11) > Created an attachment (id=117819) [details] > Patch

Comment on attachment 117819 [details] Patch Clearing flags on attachment: 117819 Committed r101978: <http://trac.webkit.org/changeset/101978>

All reviewed patches have been landed. Closing bug.