[webkit-reviews] review denied: [Bug 72939] Asynchronous SpellChecker should consider multiple requests : [Attachment 117160] Patch

[bugzilla-daemon at webkit.org]

MORITA Hajime <morrita at google.com> has denied Shinya Kawanaka
<shinyak at google.com>'s request for review:
Bug 72939: Asynchronous SpellChecker should consider multiple requests
https://bugs.webkit.org/show_bug.cgi?id=72939

Attachment 117160: Patch
https://bugs.webkit.org/attachment.cgi?id=117160&action=review

------- Additional Comments from MORITA Hajime <morrita at google.com>
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?