[Webkit-unassigned] [Bug 72939] Asynchronous SpellChecker should consider multiple requests

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=72939


MORITA Hajime <morrita at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #117160|review?                     |review-
               Flag|                            |




--- Comment #2 from MORITA Hajime <morrita at google.com>  2011-11-30 19:14:00 PST ---
(From update of attachment 117160)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.