Bug 72939 - Asynchronous SpellChecker should consider multiple requests
Summary: Asynchronous SpellChecker should consider multiple requests
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
Depends on: 73706
Blocks: 71991
  Show dependency treegraph
Reported: 2011-11-22 04:46 PST by Shinya Kawanaka
Modified: 2011-12-05 00:47 PST (History)
3 users (show)

See Also:

Patch (15.40 KB, patch)
2011-11-30 03:07 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (15.61 KB, patch)
2011-11-30 21:35 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (17.87 KB, patch)
2011-12-01 01:30 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (18.44 KB, patch)
2011-12-01 02:13 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff
Patch (18.46 KB, patch)
2011-12-04 18:21 PST, Shinya Kawanaka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Shinya Kawanaka 2011-11-22 04:46:17 PST
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.
Comment 1 Shinya Kawanaka 2011-11-30 03:07:31 PST
Created attachment 117160 [details]
Comment 2 Hajime Morrita 2011-11-30 19:14:00 PST
Comment on attachment 117160 [details]

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?
Comment 3 Shinya Kawanaka 2011-11-30 21:35:39 PST
Created attachment 117330 [details]
Comment 4 Hajime Morrita 2011-11-30 22:41:43 PST
Comment on attachment 117330 [details]

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.
Comment 5 Shinya Kawanaka 2011-12-01 01:30:39 PST
Created attachment 117377 [details]
Comment 6 Hajime Morrita 2011-12-01 01:52:17 PST
Comment on attachment 117377 [details]

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.
Comment 7 Shinya Kawanaka 2011-12-01 02:13:41 PST
Created attachment 117385 [details]
Comment 8 WebKit Review Bot 2011-12-01 18:16:06 PST
Comment on attachment 117385 [details]

Clearing flags on attachment: 117385

Committed r101731: <http://trac.webkit.org/changeset/101731>
Comment 9 WebKit Review Bot 2011-12-01 18:16:11 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Shinya Kawanaka 2011-12-04 18:21:30 PST
Reopening to attach new patch.
Comment 11 Shinya Kawanaka 2011-12-04 18:21:34 PST
Created attachment 117819 [details]
Comment 12 Hajime Morrita 2011-12-04 18:25:09 PST
Note that original patch was rolled out. See Bug 73706.
Comment 13 Shinya Kawanaka 2011-12-04 18:28:29 PST
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 14 WebKit Review Bot 2011-12-05 00:47:48 PST
Comment on attachment 117819 [details]

Clearing flags on attachment: 117819

Committed r101978: <http://trac.webkit.org/changeset/101978>
Comment 15 WebKit Review Bot 2011-12-05 00:47:53 PST
All reviewed patches have been landed.  Closing bug.