Move Mac to using renderer spellchecker.

Move Mac to using renderer spellchecker.

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

[Evan Stade, 2009-11-13 23:24:03]

I haven't done much testing of this yet but the basic idea is there, so it's
ready for review.

[Evan Stade, 2009-11-13 23:25:38]

oh wait, hold off, I need to move the spellcheck test

[Evan Stade, 2009-11-17 01:25:03]

re-merged, ready for review.

[pink (ping after 24hrs), 2009-11-17 16:00:50]

On 2009/11/17 01:25:03, Evan Stade wrote:
> re-merged, ready for review.

Can you expand a little on what this does? I read over the CL and it looks like
the spelling is still handled via a sync message to the browser. Won't that be
really slow and incur a lot of IPC overhead, or am I missing something obvious?

[pink (ping after 24hrs), 2009-11-17 16:10:52]

Hrm, I guess this wouldn't add any more IPC than is already there, right? Didn't
think about that. Would there be issues initializing the platform spellchecker
on the file thread? We're not allowed to make any NSSpellCheck calls that aren't
on the main thread.

[Evan Stade, 2009-11-18 19:28:40]

On 2009/11/17 16:10:52, pink wrote:
> Hrm, I guess this wouldn't add any more IPC than is already there, right?
Didn't
> think about that.

correct. Thus this whole renderer-spellchecker thing isn't really a win for mac.

> Would there be issues initializing the platform spellchecker
> on the file thread? We're not allowed to make any NSSpellCheck calls that
aren't
> on the main thread.

ah, I did not realize that. Will fix.

[Evan Stade, 2009-11-18 23:58:53]

manually tested + working on mac

[stuartmorgan, 2009-11-19 00:25:22]

LGTM

[pink (ping after 24hrs), 2009-11-19 00:49:05]

LGTM to me too

On Wed, Nov 18, 2009 at 4:25 PM,  <stuartmorgan@chromium.org> wrote:
> LGTM
>
> http://codereview.chromium.org/395007
>



-- 
Mike Pinkerton
Mac Weenie
pinkerton@google.com

[jam, 2009-11-19 01:06:30]

http://codereview.chromium.org/395007/diff/10095/10121
File chrome/renderer/render_thread.h (right):

http://codereview.chromium.org/395007/diff/10095/10121#newcode153
Line 153: bool PlatformCheckSpelling(const string16& word_to_check, int tag);
I don't see much value in this function and hte one below being part of
RenderThread (RenderThread shouldn't be a dumping ground for functions that send
IPCs).  Instead, just make the callers send the IPC directly through
RenderThread::current()->Send()

http://codereview.chromium.org/395007/diff/10095/10128
File chrome/renderer/spellchecker/spellcheck_unittest.cc (right):

http://codereview.chromium.org/395007/diff/10095/10128#newcode12
Line 12: #include "chrome/browser/spellchecker_platform_engine.h"
this include is giving a DEPS error on the try bot.  so this file should be
moved to common, probably along with all the files that depend on it.  (renderer
code shouldn't include browser code).

[Evan Stade, 2009-11-19 01:09:16]

http://codereview.chromium.org/395007/diff/10095/10121
File chrome/renderer/render_thread.h (right):

http://codereview.chromium.org/395007/diff/10095/10121#newcode153
Line 153: bool PlatformCheckSpelling(const string16& word_to_check, int tag);
On 2009/11/19 01:06:30, John Abd-El-Malek wrote:
> I don't see much value in this function and hte one below being part of
> RenderThread (RenderThread shouldn't be a dumping ground for functions that
send
> IPCs).  Instead, just make the callers send the IPC directly through
> RenderThread::current()->Send()

ok

http://codereview.chromium.org/395007/diff/10095/10128
File chrome/renderer/spellchecker/spellcheck_unittest.cc (right):

http://codereview.chromium.org/395007/diff/10095/10128#newcode12
Line 12: #include "chrome/browser/spellchecker_platform_engine.h"
On 2009/11/19 01:06:30, John Abd-El-Malek wrote:
> this include is giving a DEPS error on the try bot.  so this file should be
> moved to common, probably along with all the files that depend on it. 
(renderer
> code shouldn't include browser code).

I can just remove it I think.

[Evan Stade, 2009-11-19 22:35:14]

updated; all trybots are green

[jam, 2009-11-19 22:37:49]

lgtm