Bug 72939 – Asynchronous SpellChecker should consider multiple requests

• Component: UI Events
• Status: RESOLVED FIXED

[Shinya Kawanaka]

The current implementation of asynchronous spellchecker ignores spellcheck requests if a request is being processed.

It might suffice if the last request of queue requests is processed.

[Shinya Kawanaka]

Created attachment 117160 [details]
Patch

[Hajime Morrita]

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?

[Shinya Kawanaka]

Created attachment 117330 [details]
Patch

[Hajime Morrita]

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.

[Shinya Kawanaka]

Created attachment 117377 [details]
Patch

[Hajime Morrita]

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.

[Shinya Kawanaka]

Created attachment 117385 [details]
Patch

[WebKit Review Bot]

Comment on attachment 117385 [details]
Patch

Clearing flags on attachment: 117385

Committed r101731: <http://trac.webkit.org/changeset/101731>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Shinya Kawanaka]

Reopening to attach new patch.

[Shinya Kawanaka]

Created attachment 117819 [details]
Patch

[Hajime Morrita]

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

[Shinya Kawanaka]

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

[WebKit Review Bot]

Comment on attachment 117819 [details]
Patch

Clearing flags on attachment: 117819

Committed r101978: <http://trac.webkit.org/changeset/101978>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.