Integrating Mac OS Grammar checker into Chromium.

Integrating Mac OS Grammar checker into Chromium.

BUG=none
TEST=none

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

[gmorrita, 2011-02-01 09:51:29]

Work in progress....

[gmorrita, 2011-02-09 02:25:26]

Hi Bono-san, 
Now WebKit side landed. Could you take a look on chromium side?

Thanks in advance.

[Hironori Bono, 2011-02-09 04:10:03]

(+pinkerton)

Morita-san,

Thank you for your change. I have quickly read though your change and added some
trivial comments. Also, I have added Mike to the reviewers for this change, who
is an expert of Mac Chrome.

Greetings Mike,

This change integrates the unified text checker of Mac OS to Chromium so we can
check grammar as Safari does. Is it possible to give me your comments?

Regards,

Hironori Bono

[gmorrita, 2011-02-09 05:11:24]

Thank you for taking a look, Bono-san.
But I can't see your comment. Can I ask you to confirm it published if it's
ready?

[Hironori Bono, 2011-02-09 05:43:51]

Morita-san,

Sorry, I forgot publishing them.

Regards,

Hironori Bono

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_message_filter.cc (right):

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_message_filter.cc:318:
IPC_MESSAGE_HANDLER(ViewHostMsg_SpellChecker_RequestTextCheck,
nit: I prefer 'ViewHostMsg_SpellCheck_PlatformRequestTextCheck' since
OnRequestTextCheck calls a function in the SpellCheckerPlatform class.

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_message_filter.cc:1154: : filter_(filter),
route_id_(route_id), identifier_(identifier),
nit: All parameters should be aligned if possible.
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...>

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_message_filter.cc:1155:
document_tag_(document_tag) {
nit: need a 2-space indent.

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_message_filter.cc:1163: (route_id_,
identifier_, document_tag_, results)));
nit: ditto. (All parameters should be aligned if possible.)

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_message_filter.cc:1183:
&TextCheckingResponder::RespondAndRelease);
It is better to move this code that creates a callback to
'src/chrome/browser/spellchecker_mac.mm'. Since this code does not make so much
sense on Windows or on Linux, which does not use platform spellcheckers.

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_message_filter.h (right):

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_message_filter.h:181: string16 text);
nit: 'string16' -> 'const string16&'?

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/spellchecker_...
File chrome/browser/spellchecker_mac.mm (right):

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/spellchecker_...
chrome/browser/spellchecker_mac.mm:214: 
nit: need one line-break between functions.

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/spellchecker_...
chrome/browser/spellchecker_mac.mm:217: Callback1<const
TextCheckingResultList&>::Type* callback) {
nit: align parameters if possible.

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/spellchecker_...
chrome/browser/spellchecker_mac.mm:222: size_t startingAt = 0;
nit: variable names are all lowercase.
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...>

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/spellchecker_...
chrome/browser/spellchecker_mac.mm:227: wordCount:NULL];
If I understand this change correctly, this code is run on the IPC thread, i.e.
Chrome cannot process any IPC messages while this function is checking the given
text. It may be better to create a Task object and post it to the I/O thread?
('src/chrome/browser/renderer_host/render_message_filter_mac.mm' is an example
of how to create a task and post it.)

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/spellchecker_...
File chrome/browser/spellchecker_platform_engine.h (right):

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/spellchecker_...
chrome/browser/spellchecker_platform_engine.h:87: Callback1<const
TextCheckingResultList&>::Type* callback);
nit: align all parameters if possible.

http://codereview.chromium.org/6392045/diff/3001/chrome/common/render_message...
File chrome/common/render_messages.cc (right):

http://codereview.chromium.org/6392045/diff/3001/chrome/common/render_message...
chrome/common/render_messages.cc:1178: param_type* p) {
nit: align parameters if possible.

http://codereview.chromium.org/6392045/diff/3001/chrome/common/text_checking_...
File chrome/common/text_checking_result.h (right):

http://codereview.chromium.org/6392045/diff/3001/chrome/common/text_checking_...
chrome/common/text_checking_result.h:1: // Copyright (c) 2006-2008 The Chromium
Authors. All rights reserved.
nit: '2006-2008' -> '2011'.

http://codereview.chromium.org/6392045/diff/3001/chrome/common/text_checking_...
chrome/common/text_checking_result.h:13: class TextCheckingResult {
Would it be better to add '#include
"third_party/WebKit/.../WebTextCheckingResult.h"' to
'chrome/common/webkit_param_traits.h' so we can send
WebKit::WebTextCheckingResult without conversions?

http://codereview.chromium.org/6392045/diff/3001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6392045/diff/3001/chrome/renderer/render_view....
chrome/renderer/render_view.cc:1633:
completion->didFinishCheckingText(toWebResults(results));
Out of curiosity, who deletes this 'completion' object?

http://codereview.chromium.org/6392045/diff/3001/chrome/renderer/render_view....
chrome/renderer/render_view.cc:2379: // Text check (unified request for grammar
and spell check) is only
it may be a good idea to enclose this block with '#if defined(OS_MACOSX)' and
'#endif' since the platform-spellchecker is available only on Mac OS for now.
Also, it would be better to move this code to
'src/chrome/renderer/spellcheck/spellcheck.cc', which includes all magics about
our spellchecker.

http://codereview.chromium.org/6392045/diff/3001/chrome/renderer/render_view.h
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/6392045/diff/3001/chrome/renderer/render_view....
chrome/renderer/render_view.h:484: WebKit::WebWorkerClient* client);
nit: is this change needed by your change?

http://codereview.chromium.org/6392045/diff/3001/chrome/renderer/render_view....
chrome/renderer/render_view.h:946: void OnTranslatePage(int page_id,
nit: is this function needed by your change?

http://codereview.chromium.org/6392045/diff/3001/chrome/renderer/render_view....
chrome/renderer/render_view.h:1471: // Holding ongoing spellchecking operations,
assigning IDs
nit: "holding" -> "holds", "assigning" -> "assigns". a variable comment should
be descriptive, i.e. the subject of a variable comment should be the variable
itself.
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...>

[gmorrita, 2011-02-10 02:15:21]

Bono-san, thank you for reviewing quickly!
I updated the patch to address your feedback.
Could you take another look?

Regards.
--
morrita

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_message_filter.cc (right):

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_message_filter.cc:318:
IPC_MESSAGE_HANDLER(ViewHostMsg_SpellChecker_RequestTextCheck,
On 2011/02/09 05:43:51, hbono wrote:
> nit: I prefer 'ViewHostMsg_SpellCheck_PlatformRequestTextCheck' since
> OnRequestTextCheck calls a function in the SpellCheckerPlatform class.

Done.

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_message_filter.cc:1154: : filter_(filter),
route_id_(route_id), identifier_(identifier),
On 2011/02/09 05:43:51, hbono wrote:
> nit: All parameters should be aligned if possible.
>
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...>

Done.

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_message_filter.cc:1155:
document_tag_(document_tag) {
On 2011/02/09 05:43:51, hbono wrote:
> nit: need a 2-space indent.
This is a initializer so I guess there is a different alignment rule.

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_message_filter.cc:1163: (route_id_,
identifier_, document_tag_, results)));
On 2011/02/09 05:43:51, hbono wrote:
> nit: ditto. (All parameters should be aligned if possible.)

Done.

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_message_filter.cc:1183:
&TextCheckingResponder::RespondAndRelease);
On 2011/02/09 05:43:51, hbono wrote:
> It is better to move this code that creates a callback to
> 'src/chrome/browser/spellchecker_mac.mm'. Since this code does not make so
much
> sense on Windows or on Linux, which does not use platform spellcheckers.

Done.

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/spellchecker_...
File chrome/browser/spellchecker_mac.mm (right):

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/spellchecker_...
chrome/browser/spellchecker_mac.mm:214: 
On 2011/02/09 05:43:51, hbono wrote:
> nit: need one line-break between functions.

Done.

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/spellchecker_...
chrome/browser/spellchecker_mac.mm:217: Callback1<const
TextCheckingResultList&>::Type* callback) {
On 2011/02/09 05:43:51, hbono wrote:
> nit: align parameters if possible.

Done.

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/spellchecker_...
chrome/browser/spellchecker_mac.mm:222: size_t startingAt = 0;
On 2011/02/09 05:43:51, hbono wrote:
> nit: variable names are all lowercase.
>
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...>

Done.

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/spellchecker_...
chrome/browser/spellchecker_mac.mm:227: wordCount:NULL];
On 2011/02/09 05:43:51, hbono wrote:
> If I understand this change correctly, this code is run on the IPC thread,
i.e.
> Chrome cannot process any IPC messages while this function is checking the
given
> text. It may be better to create a Task object and post it to the I/O thread?
> ('src/chrome/browser/renderer_host/render_message_filter_mac.mm' is an example
> of how to create a task and post it.)
Certainly.
So I extracted the check into a task class.

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/spellchecker_...
File chrome/browser/spellchecker_platform_engine.h (right):

http://codereview.chromium.org/6392045/diff/3001/chrome/browser/spellchecker_...
chrome/browser/spellchecker_platform_engine.h:87: Callback1<const
TextCheckingResultList&>::Type* callback);
On 2011/02/09 05:43:51, hbono wrote:
> nit: align all parameters if possible.

Done.

http://codereview.chromium.org/6392045/diff/3001/chrome/common/render_message...
File chrome/common/render_messages.cc (right):

http://codereview.chromium.org/6392045/diff/3001/chrome/common/render_message...
chrome/common/render_messages.cc:1178: param_type* p) {
On 2011/02/09 05:43:51, hbono wrote:
> nit: align parameters if possible.
Done. 
Note that the traits class is now for WebKit::WebTextCheckingResult 
and moved to webkit_param_traits.cc

http://codereview.chromium.org/6392045/diff/3001/chrome/common/text_checking_...
File chrome/common/text_checking_result.h (right):

http://codereview.chromium.org/6392045/diff/3001/chrome/common/text_checking_...
chrome/common/text_checking_result.h:1: // Copyright (c) 2006-2008 The Chromium
Authors. All rights reserved.
On 2011/02/09 05:43:51, hbono wrote:
> nit: '2006-2008' -> '2011'.
This file is removed.

http://codereview.chromium.org/6392045/diff/3001/chrome/common/text_checking_...
chrome/common/text_checking_result.h:13: class TextCheckingResult {
On 2011/02/09 05:43:51, hbono wrote:
> Would it be better to add '#include
> "third_party/WebKit/.../WebTextCheckingResult.h"' to
> 'chrome/common/webkit_param_traits.h' so we can send
> WebKit::WebTextCheckingResult without conversions?
Ah, I didn't know that file!
So I removed this class and changed to use WebTextCheckingResult  instead.
Thank you for pointing this out.

http://codereview.chromium.org/6392045/diff/3001/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6392045/diff/3001/chrome/renderer/render_view....
chrome/renderer/render_view.cc:1633:
completion->didFinishCheckingText(toWebResults(results));
On 2011/02/09 05:43:51, hbono wrote:
> Out of curiosity, who deletes this 'completion' object?
didFinishCheckingText() removes |this| at the end of the call.

http://codereview.chromium.org/6392045/diff/3001/chrome/renderer/render_view....
chrome/renderer/render_view.cc:2379: // Text check (unified request for grammar
and spell check) is only
On 2011/02/09 05:43:51, hbono wrote:
> it may be a good idea to enclose this block with '#if defined(OS_MACOSX)' and
> '#endif' since the platform-spellchecker is available only on Mac OS for now.
> Also, it would be better to move this code to
> 'src/chrome/renderer/spellcheck/spellcheck.cc', which includes all magics
about
> our spellchecker.
Absolutely. Moved to spellchecker.cc

http://codereview.chromium.org/6392045/diff/3001/chrome/renderer/render_view.h
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/6392045/diff/3001/chrome/renderer/render_view....
chrome/renderer/render_view.h:484: WebKit::WebWorkerClient* client);
On 2011/02/09 05:43:51, hbono wrote:
> nit: is this change needed by your change?
Hmm. Something wrong happened. Recovered original.

http://codereview.chromium.org/6392045/diff/3001/chrome/renderer/render_view....
chrome/renderer/render_view.h:946: void OnTranslatePage(int page_id,
On 2011/02/09 05:43:51, hbono wrote:
> nit: is this function needed by your change?
No... thank you for pointing this out. Removed.
Something wrong happened on updating the patch.

http://codereview.chromium.org/6392045/diff/3001/chrome/renderer/render_view....
chrome/renderer/render_view.h:1471: // Holding ongoing spellchecking operations,
assigning IDs
On 2011/02/09 05:43:51, hbono wrote:
> nit: "holding" -> "holds", "assigning" -> "assigns". a variable comment should
> be descriptive, i.e. the subject of a variable comment should be the variable
> itself.
>
<http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab...>
Thanks for the clarification. Fixed.

[Hironori Bono, 2011-02-10 11:02:56]

Morita-san,

Thank you for your updated change. Unfortunately, these comments are only nits
since I focused on another issue and could not read this change deeply. I will
do it next week. Sorry for your inconvenience.

Regards,

Hironori Bono

http://codereview.chromium.org/6392045/diff/11016/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_message_filter.h (right):

http://codereview.chromium.org/6392045/diff/11016/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_message_filter.h:40: class
TextCheckingResult;
nit: is this description still required?

http://codereview.chromium.org/6392045/diff/11016/chrome/browser/spellchecker...
File chrome/browser/spellchecker_mac.mm (right):

http://codereview.chromium.org/6392045/diff/11016/chrome/browser/spellchecker...
chrome/browser/spellchecker_mac.mm:28: class TextCheckingTask : public Task {
nit: I would be great if this class has a class comment.

http://codereview.chromium.org/6392045/diff/11016/chrome/common/render_messag...
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/6392045/diff/11016/chrome/common/render_messag...
chrome/common/render_messages_internal.h:2348: int /* route_id for response */,
nit: is this parameter aligned to the above one?

http://codereview.chromium.org/6392045/diff/11016/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6392045/diff/11016/chrome/renderer/render_view...
chrome/renderer/render_view.cc:999: OnRespondTextCheck);
nit: OnRespondTextCheck -> OnSpellCheckerRespondTextCheck. (The name of a
message handler should match with its message.)

http://codereview.chromium.org/6392045/diff/11016/chrome/renderer/spellchecke...
File chrome/renderer/spellchecker/spellcheck.h (right):

http://codereview.chromium.org/6392045/diff/11016/chrome/renderer/spellchecke...
chrome/renderer/spellchecker/spellcheck.h:72: const string16& word,
nit: 'word' -> 'text'? (I assume this string consists of two or more words.)

[pink (ping after 24hrs), 2011-02-11 17:33:40]

I know jam was working on a new way to add render messages for features, can
this use his new mechanism? Maybe talk to him.

http://codereview.chromium.org/6392045/diff/11016/chrome/browser/spellchecker...
File chrome/browser/spellchecker_mac.mm (right):

http://codereview.chromium.org/6392045/diff/11016/chrome/browser/spellchecker...
chrome/browser/spellchecker_mac.mm:28: class TextCheckingTask : public Task {
On 2011/02/10 11:02:56, hbono wrote:
> nit: I would be great if this class has a class comment.

100% agreed. not a nit at all.

http://codereview.chromium.org/6392045/diff/11016/chrome/browser/spellchecker...
chrome/browser/spellchecker_mac.mm:44: // when the build target goes upto 10.6
s/upto/up to.

Can you do a respondsToSelector: check and use it for OS's that currently
support it? It's ok if it's not available in the sdk if you do a check before
use.

http://codereview.chromium.org/6392045/diff/11016/chrome/browser/spellchecker...
chrome/browser/spellchecker_mac.mm:51: startingAt:starting_at
line up colons

http://codereview.chromium.org/6392045/diff/11016/chrome/browser/spellchecker...
chrome/browser/spellchecker_mac.mm:55: if (0 == range.length)
if (range.length == 0). The constant before is much less readable, and we have
warnings enabled to catch inadvertent assignment in expressions.

http://codereview.chromium.org/6392045/diff/11016/chrome/browser/spellchecker...
chrome/browser/spellchecker_mac.mm:74: BrowserMessageFilter* destination_;
comment this is a weak pointer.

http://codereview.chromium.org/6392045/diff/11016/chrome/browser/spellchecker...
File chrome/browser/spellchecker_platform_engine.h (right):

http://codereview.chromium.org/6392045/diff/11016/chrome/browser/spellchecker...
chrome/browser/spellchecker_platform_engine.h:87: void RequestTextCheck(int
route_id,
Comment all public api. Can |destination| be null?

http://codereview.chromium.org/6392045/diff/11016/chrome/common/webkit_param_...
File chrome/common/webkit_param_traits.cc (right):

http://codereview.chromium.org/6392045/diff/11016/chrome/common/webkit_param_...
chrome/common/webkit_param_traits.cc:188: int error;
might as well be safe and initialize this to 0

http://codereview.chromium.org/6392045/diff/11016/chrome/common/webkit_param_...
chrome/common/webkit_param_traits.cc:194: int position;
init to 0

http://codereview.chromium.org/6392045/diff/11016/chrome/common/webkit_param_...
chrome/common/webkit_param_traits.cc:197: int length;
init to 0

http://codereview.chromium.org/6392045/diff/11016/chrome/renderer/render_view.h
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/6392045/diff/11016/chrome/renderer/render_view...
chrome/renderer/render_view.h:423: virtual void requestCheckingOfText(
I'd call this |-requestTextChecking|. It's more in line with the naming
elsewhere.

http://codereview.chromium.org/6392045/diff/11016/chrome/renderer/render_view...
chrome/renderer/render_view.h:1462: // Holds ongoing spellchecking operations,
assigns IDs
dont wrap early. Put as much of the comment on this line as possible before
wrapping.

[gmorrita, 2011-02-17 11:39:44]

Bono-san, Pink, thank you for your review and I'm sorry for my slow response...
But could you take another look?
I rewrote the code to adopt new Carnitas approach which Pink mentioned,
and added tests both for renderer side and browser side.

http://codereview.chromium.org/6392045/diff/11016/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_message_filter.h (right):

http://codereview.chromium.org/6392045/diff/11016/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_message_filter.h:40: class
TextCheckingResult;
On 2011/02/10 11:02:56, hbono wrote:
> nit: is this description still required?

http://codereview.chromium.org/6392045/diff/11016/chrome/browser/spellchecker...
File chrome/browser/spellchecker_mac.mm (right):

http://codereview.chromium.org/6392045/diff/11016/chrome/browser/spellchecker...
chrome/browser/spellchecker_mac.mm:28: class TextCheckingTask : public Task {
On 2011/02/11 17:33:40, pink wrote:
> On 2011/02/10 11:02:56, hbono wrote:
> > nit: I would be great if this class has a class comment.
> 
> 100% agreed. not a nit at all.
Added an description.

http://codereview.chromium.org/6392045/diff/11016/chrome/browser/spellchecker...
chrome/browser/spellchecker_mac.mm:44: // when the build target goes upto 10.6
On 2011/02/11 17:33:40, pink wrote:
> s/upto/up to.
Fixed.
> 
> Can you do a respondsToSelector: check and use it for OS's that currently
> support it? It's ok if it's not available in the sdk if you do a check before
> use.
New API has much different style:
uses worker thread and returns the result asynchronously. 
I'd like to keep current API to avoid maintaining different versions of code.

http://codereview.chromium.org/6392045/diff/11016/chrome/browser/spellchecker...
chrome/browser/spellchecker_mac.mm:51: startingAt:starting_at
On 2011/02/11 17:33:40, pink wrote:
> line up colons

Done.

http://codereview.chromium.org/6392045/diff/11016/chrome/browser/spellchecker...
chrome/browser/spellchecker_mac.mm:55: if (0 == range.length)
On 2011/02/11 17:33:40, pink wrote:
> if (range.length == 0). The constant before is much less readable, and we have
> warnings enabled to catch inadvertent assignment in expressions.

Done.

http://codereview.chromium.org/6392045/diff/11016/chrome/browser/spellchecker...
chrome/browser/spellchecker_mac.mm:74: BrowserMessageFilter* destination_;
On 2011/02/11 17:33:40, pink wrote:
> comment this is a weak pointer.
Noticed we have to own it to avoiding the dangling pointer.
So changed to use scoped_refptr.

http://codereview.chromium.org/6392045/diff/11016/chrome/browser/spellchecker...
File chrome/browser/spellchecker_platform_engine.h (right):

http://codereview.chromium.org/6392045/diff/11016/chrome/browser/spellchecker...
chrome/browser/spellchecker_platform_engine.h:87: void RequestTextCheck(int
route_id,
On 2011/02/11 17:33:40, pink wrote:
> Comment all public api. Can |destination| be null?

Done.

http://codereview.chromium.org/6392045/diff/11016/chrome/common/render_messag...
File chrome/common/render_messages_internal.h (right):

http://codereview.chromium.org/6392045/diff/11016/chrome/common/render_messag...
chrome/common/render_messages_internal.h:2348: int /* route_id for response */,
On 2011/02/10 11:02:56, hbono wrote:
> nit: is this parameter aligned to the above one?
Had gaps. Fixed.

http://codereview.chromium.org/6392045/diff/11016/chrome/common/webkit_param_...
File chrome/common/webkit_param_traits.cc (right):

http://codereview.chromium.org/6392045/diff/11016/chrome/common/webkit_param_...
chrome/common/webkit_param_traits.cc:188: int error;
On 2011/02/11 17:33:40, pink wrote:
> might as well be safe and initialize this to 0

Done.

http://codereview.chromium.org/6392045/diff/11016/chrome/common/webkit_param_...
chrome/common/webkit_param_traits.cc:194: int position;
On 2011/02/11 17:33:40, pink wrote:
> init to 0

Done.

http://codereview.chromium.org/6392045/diff/11016/chrome/common/webkit_param_...
chrome/common/webkit_param_traits.cc:197: int length;
On 2011/02/11 17:33:40, pink wrote:
> init to 0

Done.

http://codereview.chromium.org/6392045/diff/11016/chrome/renderer/render_view.cc
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/6392045/diff/11016/chrome/renderer/render_view...
chrome/renderer/render_view.cc:999: OnRespondTextCheck);
On 2011/02/10 11:02:56, hbono wrote:
> nit: OnRespondTextCheck -> OnSpellCheckerRespondTextCheck. (The name of a
> message handler should match with its message.)

Done.

http://codereview.chromium.org/6392045/diff/11016/chrome/renderer/render_view.h
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/6392045/diff/11016/chrome/renderer/render_view...
chrome/renderer/render_view.h:423: virtual void requestCheckingOfText(
On 2011/02/11 17:33:40, pink wrote:
> I'd call this |-requestTextChecking|. It's more in line with the naming
> elsewhere.
I cannot change the name
because this API overrides a WebKit-provided abstract class.
(The name comes from Cocoa thing.)

http://codereview.chromium.org/6392045/diff/11016/chrome/renderer/render_view...
chrome/renderer/render_view.h:1462: // Holds ongoing spellchecking operations,
assigns IDs
On 2011/02/11 17:33:40, pink wrote:
> dont wrap early. Put as much of the comment on this line as possible before
> wrapping.

Done.

http://codereview.chromium.org/6392045/diff/11016/chrome/renderer/spellchecke...
File chrome/renderer/spellchecker/spellcheck.h (right):

http://codereview.chromium.org/6392045/diff/11016/chrome/renderer/spellchecke...
chrome/renderer/spellchecker/spellcheck.h:72: const string16& word,
On 2011/02/10 11:02:56, hbono wrote:
> nit: 'word' -> 'text'? (I assume this string consists of two or more words.)

Done.

[jam, 2011-02-17 18:36:58]

http://codereview.chromium.org/6392045/diff/26001/chrome/browser/spellcheck_m...
File chrome/browser/spellcheck_message_filter.h (right):

http://codereview.chromium.org/6392045/diff/26001/chrome/browser/spellcheck_m...
chrome/browser/spellcheck_message_filter.h:11: class Message;
nit: this forward declaration isn't necessary, browser_message_filter.h includes
ipc_message.h

http://codereview.chromium.org/6392045/diff/26001/chrome/browser/spellchecker...
File chrome/browser/spellchecker_mac.mm (right):

http://codereview.chromium.org/6392045/diff/26001/chrome/browser/spellchecker...
chrome/browser/spellchecker_mac.mm:31: // the IO thread.
nit: you mean FILE?

http://codereview.chromium.org/6392045/diff/26001/chrome/renderer/render_view.h
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/6392045/diff/26001/chrome/renderer/render_view...
chrome/renderer/render_view.h:919: 
nit: why?

http://codereview.chromium.org/6392045/diff/26001/chrome/renderer/spellchecke...
File chrome/renderer/spellchecker/spellcheck_provider_unittest.cc (right):

http://codereview.chromium.org/6392045/diff/26001/chrome/renderer/spellchecke...
chrome/renderer/spellchecker/spellcheck_provider_unittest.cc:95: const
MessageParameters& parameters)
nit: spacing is off.  also brace bracket belongs on thi line

[gmorrita, 2011-02-18 01:08:00]

Hi jam, thank you for taking a look! I updated the patch.
Could you take another look?

http://codereview.chromium.org/6392045/diff/26001/chrome/browser/spellcheck_m...
File chrome/browser/spellcheck_message_filter.h (right):

http://codereview.chromium.org/6392045/diff/26001/chrome/browser/spellcheck_m...
chrome/browser/spellcheck_message_filter.h:11: class Message;
On 2011/02/17 18:36:59, John Abd-El-Malek wrote:
> nit: this forward declaration isn't necessary, browser_message_filter.h
includes
> ipc_message.h

Done.

http://codereview.chromium.org/6392045/diff/26001/chrome/browser/spellchecker...
File chrome/browser/spellchecker_mac.mm (right):

http://codereview.chromium.org/6392045/diff/26001/chrome/browser/spellchecker...
chrome/browser/spellchecker_mac.mm:31: // the IO thread.
On 2011/02/17 18:36:59, John Abd-El-Malek wrote:
> nit: you mean FILE?
Oops, yes. Fixed.

http://codereview.chromium.org/6392045/diff/26001/chrome/renderer/render_view.h
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/6392045/diff/26001/chrome/renderer/render_view...
chrome/renderer/render_view.h:919: 
On 2011/02/17 18:36:59, John Abd-El-Malek wrote:
> nit: why?
Oops. Removed.

http://codereview.chromium.org/6392045/diff/26001/chrome/renderer/spellchecke...
File chrome/renderer/spellchecker/spellcheck_provider_unittest.cc (right):

http://codereview.chromium.org/6392045/diff/26001/chrome/renderer/spellchecke...
chrome/renderer/spellchecker/spellcheck_provider_unittest.cc:95: const
MessageParameters& parameters)
On 2011/02/17 18:36:59, John Abd-El-Malek wrote:
> nit: spacing is off.  also brace bracket belongs on thi line

Done.

[jam, 2011-02-18 01:17:27]

my nits lgtm

[gmorrita, 2011-02-18 11:09:19]

> my nits lgtm
@Jam: thanks!

Bono-san, As discussed offline, I updated the patch to fix cluttered license
lines
and add missing class comments. Trybots are in flight.
Could you take a look again when you have time? 
Thanks in advance.

[Hironori Bono, 2011-02-18 11:09:27]

Morita-san

Thank you for updates.
Even though the browser-side code of this change looks good, I'm a little
wondering about its renderer-side code from the point of arbitration. As you
know, SpellCheck::CheckSpelling() also sends IPC messages to use NSSpellChecker
owned by a browser process. So, it may send IPC messages to use NSSpellChecker
while your TextCheckingTask are checking text. Even though it is just a rough
idea, it may be good to change your renderer code so the SpellCheck class owns
your SpellCheckProvider object so SpellCheck::CheckSpelling() can notice your
provider is checking text and stop IPC messages during the period?

Regards,

Hironori Bono

http://codereview.chromium.org/6392045/diff/31001/chrome/browser/spellcheck_m...
File chrome/browser/spellcheck_message_filter.h (right):

http://codereview.chromium.org/6392045/diff/31001/chrome/browser/spellcheck_m...
chrome/browser/spellcheck_message_filter.h:1: // Copyright (c) 2010 The Chromium
Authors. All rights reserved.
nit: 2010 -> 2011 to all files to help us keep all copyright notices up-to-date.
nit: add 'svn:eol-style' to LF to all added files.

http://codereview.chromium.org/6392045/diff/31001/chrome/browser/spellcheck_m...
chrome/browser/spellcheck_message_filter.h:10: class SpellCheckMessageFilter :
public BrowserMessageFilter {
nit: class comments?

http://codereview.chromium.org/6392045/diff/31001/chrome/renderer/render_view.h
File chrome/renderer/render_view.h (right):

http://codereview.chromium.org/6392045/diff/31001/chrome/renderer/render_view...
chrome/renderer/render_view.h:1339: SpellCheckProvider* spellcheck_provider_;
nit: I would recommend to use scoped_ptr<SpellCheckProvider>.
I'm wondering why this class is not owned by the SpellCheck class since this
class name made me think this class is a provider for the SpellCheck class, a
controller that controls a platform spellchecker running on a browser and
hunspell running on a renderer.

http://codereview.chromium.org/6392045/diff/31001/chrome/renderer/spellchecke...
File chrome/renderer/spellchecker/spellcheck_provider.h (right):

http://codereview.chromium.org/6392045/diff/31001/chrome/renderer/spellchecke...
chrome/renderer/spellchecker/spellcheck_provider.h:25: class SpellCheckProvider
: public RenderViewObserver {
I'm a little wondering if this name describes the properties of this class well:
this class looks like a proxy than just a provider. I'm a little wondering if it
is good to denote a class derived from an observer as a provider.

[Hironori Bono, 2011-02-18 12:18:38]

Greetings Evan,

Currently, Morita-san has been integrating an asynchronous spellchecker into
Chrome, and this change adds a SpellCheckProvider class (which is owned by
RenderView and implements RenderViewObserver added by jam) so it can receive IPC
messages from a browser. On the other hand, the current SpellCheck class is
owned by RenderThread to receive IPC messages from a browser. Even though it may
be a good idea that the SpellCheck class also implements RenderViewObserver so
it can observe spellchecker messages, I'm wondering if we can move the
SpellCheck class from RenderThread to RenderView. Is it possible for us to move
the SpellCheck class to RenderView so it can implement RenderViewObserver?

Regards,

Hironori Bono

On 2011/02/18 11:09:27, hbono wrote:
> Morita-san
> 
> Thank you for updates.
> Even though the browser-side code of this change looks good, I'm a little
> wondering about its renderer-side code from the point of arbitration. As you
> know, SpellCheck::CheckSpelling() also sends IPC messages to use
NSSpellChecker
> owned by a browser process. So, it may send IPC messages to use NSSpellChecker
> while your TextCheckingTask are checking text. Even though it is just a rough
> idea, it may be good to change your renderer code so the SpellCheck class owns
> your SpellCheckProvider object so SpellCheck::CheckSpelling() can notice your
> provider is checking text and stop IPC messages during the period?
> 
> Regards,
> 
> Hironori Bono
> 
>
http://codereview.chromium.org/6392045/diff/31001/chrome/browser/spellcheck_m...
> File chrome/browser/spellcheck_message_filter.h (right):
> 
>
http://codereview.chromium.org/6392045/diff/31001/chrome/browser/spellcheck_m...
> chrome/browser/spellcheck_message_filter.h:1: // Copyright (c) 2010 The
Chromium
> Authors. All rights reserved.
> nit: 2010 -> 2011 to all files to help us keep all copyright notices
up-to-date.
> nit: add 'svn:eol-style' to LF to all added files.
> 
>
http://codereview.chromium.org/6392045/diff/31001/chrome/browser/spellcheck_m...
> chrome/browser/spellcheck_message_filter.h:10: class SpellCheckMessageFilter :
> public BrowserMessageFilter {
> nit: class comments?
> 
>
http://codereview.chromium.org/6392045/diff/31001/chrome/renderer/render_view.h
> File chrome/renderer/render_view.h (right):
> 
>
http://codereview.chromium.org/6392045/diff/31001/chrome/renderer/render_view...
> chrome/renderer/render_view.h:1339: SpellCheckProvider* spellcheck_provider_;
> nit: I would recommend to use scoped_ptr<SpellCheckProvider>.
> I'm wondering why this class is not owned by the SpellCheck class since this
> class name made me think this class is a provider for the SpellCheck class, a
> controller that controls a platform spellchecker running on a browser and
> hunspell running on a renderer.
> 
>
http://codereview.chromium.org/6392045/diff/31001/chrome/renderer/spellchecke...
> File chrome/renderer/spellchecker/spellcheck_provider.h (right):
> 
>
http://codereview.chromium.org/6392045/diff/31001/chrome/renderer/spellchecke...
> chrome/renderer/spellchecker/spellcheck_provider.h:25: class
SpellCheckProvider
> : public RenderViewObserver {
> I'm a little wondering if this name describes the properties of this class
well:
> this class looks like a proxy than just a provider. I'm a little wondering if
it
> is good to denote a class derived from an observer as a provider.

[Evan Stade, 2011-02-18 22:01:56]

it's probably possible. I did this over a year ago, but I think the idea was to
cut down on the IPC and memory requirements by sharing the SpellCheck class
among multiple RenderViews. I haven't been hacking on the renderer recently, so
if you think it's better to move it so we can use this new architecture, I'll
defer to your judgment.

[Hironori Bono, 2011-02-22 02:02:23]

Greetings Evan,

Thank you for your advices.
When I investigated the renderer, it is better to create another change for
refactoring the SpellCheck class in the renderer rather than adding more code
for the purpose. (This change becomes already big.) When we create a refactoring
change, we will send its review request and ask your thoughts again.

Morita-san,

Is it possible to file a bug and add a comment to your SpellCheckProvider class?
This change will looks good to me when you add this comment.

Regards,

Hironori Bono

[gmorrita, 2011-02-22 03:01:05]

Bono-san, Evan, thanks you for your review and giving the advice!
I'll land this soon. 

Bono-san, I filed the refactoring work at http://crbug.com/73699
and referred it from the comment.

[Hironori Bono, 2011-02-22 08:25:57]

LGTM.

Regards,

Hironori Bono