Asynchronous spellchecking on Win and Linux

This patch implements asynchronous spellchecker on Windows and Linux.

Though Mac already has its own spellchecker, Linux and Windows don't have one. So we have implemented asynchronous spellchecker posting a task.

BUG=6424, 111593
TEST=SpellCheckTest.*


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133029

[shinyak, 2012-01-26 12:45:50]

Bono-san, Morita-san,

I submit a patch to perform asynchronous spellchecker on Mac and Linux.

Could you take a look at this? Thanks.

[Hironori Bono, 2012-01-27 06:17:41]

Kawanaka-san,

Thanks for your change. Unfortunately, this change still uses an old (and
dangerous) way to run a task. I would recommend to re-write your change with new
methods (base::Bind, WeakPtr, MessageLoopProxy, etc.)
Also, Issue 6424 is a part of Issue 111593. Is it possible to update the BUG=
line?

Regards,

Hironori Bono

http://codereview.chromium.org/9169082/diff/1/chrome/renderer/spellchecker/sp...
File chrome/renderer/spellchecker/spellcheck.cc (right):

http://codereview.chromium.org/9169082/diff/1/chrome/renderer/spellchecker/sp...
chrome/renderer/spellchecker/spellcheck.cc:27: : public
base::RefCountedThreadSafe<SpellCheck::SpellCheckRequest> {
I do not think we need a new class for this use-case now. (We can now use
base::Bind() to call a member method safely and it makes this change much
simpler.)

http://codereview.chromium.org/9169082/diff/1/chrome/renderer/spellchecker/sp...
chrome/renderer/spellchecker/spellcheck.cc:45:
spellcheck_->SpellCheckParagraph(text_, tag_, &results);
This call surely creates confusing crashes when a renderer calls function after
this SpellCheck object has been deleted. We need to use WeakPtr to access an
object from a task. (As written above, I do not recommend to use a task,
though.)

http://codereview.chromium.org/9169082/diff/1/chrome/renderer/spellchecker/sp...
chrome/renderer/spellchecker/spellcheck.cc:50: // spellchecker, this function
won't be used.
Use NOTREACHED() and let a renderer crash when someone call this function by
mistake.

http://codereview.chromium.org/9169082/diff/1/chrome/renderer/spellchecker/sp...
chrome/renderer/spellchecker/spellcheck.cc:422:
MessageLoop::current()->PostTask(FROM_HERE,
This is an old way to post a task. (This code crashes when
MessageLoop::current() does not exist.) We can now use MessageLoopProxy instead
of using MessageLoop directly.

http://codereview.chromium.org/9169082/diff/1/chrome/renderer/spellchecker/sp...
File chrome/renderer/spellchecker/spellcheck_provider.cc (right):

http://codereview.chromium.org/9169082/diff/1/chrome/renderer/spellchecker/sp...
chrome/renderer/spellchecker/spellcheck_provider.cc:67: routing_id(),
nit: I do not think we need these space characters.

http://codereview.chromium.org/9169082/diff/1/chrome/renderer/spellchecker/sp...
File chrome/renderer/spellchecker/spellcheck_unittest.cc (right):

http://codereview.chromium.org/9169082/diff/1/chrome/renderer/spellchecker/sp...
chrome/renderer/spellchecker/spellcheck_unittest.cc:90: class
FakeTextCheckingCompletion : public WebKit::WebTextCheckingCompletion {
nit: we use "Mock..." instead of "Fake...".

[shinyak, 2012-01-30 06:53:05]

Bono-san, Morita-san,

Thank you for reviewing.
I've updated the patch. So could you take a look at this?

http://codereview.chromium.org/9169082/diff/1/chrome/renderer/spellchecker/sp...
File chrome/renderer/spellchecker/spellcheck.cc (right):

http://codereview.chromium.org/9169082/diff/1/chrome/renderer/spellchecker/sp...
chrome/renderer/spellchecker/spellcheck.cc:27: : public
base::RefCountedThreadSafe<SpellCheck::SpellCheckRequest> {
On 2012/01/27 06:17:41, Hironori Bono wrote:
> I do not think we need a new class for this use-case now. (We can now use
> base::Bind() to call a member method safely and it makes this change much
> simpler.)

Actually |text| is a temporary object, so the reference may be deleted after
escaping SpellCheck::RequestTextChecking. So I want to have some references to
it. I made a queue in SpellCheck class to achieve it.

http://codereview.chromium.org/9169082/diff/1/chrome/renderer/spellchecker/sp...
chrome/renderer/spellchecker/spellcheck.cc:45:
spellcheck_->SpellCheckParagraph(text_, tag_, &results);
On 2012/01/27 06:17:41, Hironori Bono wrote:
> This call surely creates confusing crashes when a renderer calls function
after
> this SpellCheck object has been deleted. We need to use WeakPtr to access an
> object from a task. (As written above, I do not recommend to use a task,
> though.)

Done.

http://codereview.chromium.org/9169082/diff/1/chrome/renderer/spellchecker/sp...
chrome/renderer/spellchecker/spellcheck.cc:50: // spellchecker, this function
won't be used.
On 2012/01/27 06:17:41, Hironori Bono wrote:
> Use NOTREACHED() and let a renderer crash when someone call this function by
> mistake.

Done.

http://codereview.chromium.org/9169082/diff/1/chrome/renderer/spellchecker/sp...
chrome/renderer/spellchecker/spellcheck.cc:422:
MessageLoop::current()->PostTask(FROM_HERE,
On 2012/01/27 06:17:41, Hironori Bono wrote:
> This is an old way to post a task. (This code crashes when
> MessageLoop::current() does not exist.) We can now use MessageLoopProxy
instead
> of using MessageLoop directly.

Done.

http://codereview.chromium.org/9169082/diff/1/chrome/renderer/spellchecker/sp...
File chrome/renderer/spellchecker/spellcheck_provider.cc (right):

http://codereview.chromium.org/9169082/diff/1/chrome/renderer/spellchecker/sp...
chrome/renderer/spellchecker/spellcheck_provider.cc:67: routing_id(),
On 2012/01/27 06:17:41, Hironori Bono wrote:
> nit: I do not think we need these space characters.

Done. Sorry.

http://codereview.chromium.org/9169082/diff/1/chrome/renderer/spellchecker/sp...
File chrome/renderer/spellchecker/spellcheck_unittest.cc (right):

http://codereview.chromium.org/9169082/diff/1/chrome/renderer/spellchecker/sp...
chrome/renderer/spellchecker/spellcheck_unittest.cc:90: class
FakeTextCheckingCompletion : public WebKit::WebTextCheckingCompletion {
On 2012/01/27 06:17:41, Hironori Bono wrote:
> nit: we use "Mock..." instead of "Fake...".

Done.

[shinyak, 2012-04-13 10:56:39]

Rebased ToT.

1) Added params in SpellCheckMsg_RespondSpellingService to check the
RespondSpellingService is really used or not

2) Moved ToWebResultList to spellcheck.h, since we want to use it in
spellcheck.cc, too.

[Hironori Bono, 2012-04-17 07:30:11]

Greetings Kawanaka-san,

Thanks for updating this change.
Unfortunately, it seems this change causes conflicts on our trybots. Would it be
possible to re-synchronize your repository with the top of trunk and upload the
updated one? (Your latest one looks good to me, though.)

Regards,

Hironori Bono

[shinyak, 2012-04-18 01:17:11]

Hi,

I've rebased the patch again.

[shinyak, 2012-04-18 16:43:20]

Ah, the patch doesn't build.... Let me fix it soon.

[Hironori Bono, 2012-04-19 01:09:51]

LGTM.
Many thanks for your hard work. By the way, if you do not have C++ readability,
I would recommend to use this change for it.

Regards,

Hironori Bono

[commit-bot: I haz the power, 2012-04-19 16:34:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shinyak@chromium.org/9169082/31001

[commit-bot: I haz the power, 2012-04-19 18:58:25]

Change committed as 133029