Update the code to have the SpellCheckProvider implement WebSpellCheckClient, in preparation for ...

Update the code to have the SpellCheckProvider implement WebSpellCheckClient, in preparation for moving that code out of RenderView.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=79735

[Lei Zhang, 2011-03-29 17:40:16]

moritta is probably more qualified to review this, but I will take a look.

[gmorrita, 2011-03-29 18:28:37]

Hi John, thank you for doing this!
I CCed hbono@ who is an expert on spellchecking code.

http://codereview.chromium.org/6764001/diff/4001/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/6764001/diff/4001/content/renderer/render_view...
content/renderer/render_view.cc:635:
webview()->setSpellCheckClient(spellcheck_provider_);
How about to allow SpellCheckProvider to set itself to the webview()
instead of set it by RenderView?

I also think that it might be better to setSpellCheckClient(NULL) when
the client interface is gone before webview() get deleted.
Such kind of symmetry would make its lifetime clear.

http://codereview.chromium.org/6764001/diff/4005/chrome/renderer/spellchecker...
File chrome/renderer/spellchecker/spellcheck_provider.cc (right):

http://codereview.chromium.org/6764001/diff/4005/chrome/renderer/spellchecker...
chrome/renderer/spellchecker/spellcheck_provider.cc:27: #if defined(OS_MACOSX)
Ditto about ifdef

http://codereview.chromium.org/6764001/diff/4005/chrome/renderer/spellchecker...
chrome/renderer/spellchecker/spellcheck_provider.cc:36: #if defined(OS_MACOSX)
Ditto.

http://codereview.chromium.org/6764001/diff/4005/chrome/renderer/spellchecker...
chrome/renderer/spellchecker/spellcheck_provider.cc:86: // Will be NULL during
unit tests.
Could you move this null check to where the instance is created
and skip the instance creation if it is null?
Then we can assume that RenderThread is always available
in methods in this class.

http://codereview.chromium.org/6764001/diff/4005/chrome/renderer/spellchecker...
chrome/renderer/spellchecker/spellcheck_provider.cc:106: if (thread)
Ditto about null check.

http://codereview.chromium.org/6764001/diff/4005/chrome/renderer/spellchecker...
File chrome/renderer/spellchecker/spellcheck_provider.h (right):

http://codereview.chromium.org/6764001/diff/4005/chrome/renderer/spellchecker...
chrome/renderer/spellchecker/spellcheck_provider.h:84: #if defined(OS_MACOSX)
I don't think this ifdef is necessary.
How about to just left it as false?

[jam, 2011-03-29 18:37:54]

http://codereview.chromium.org/6764001/diff/4001/content/renderer/render_view.cc
File content/renderer/render_view.cc (right):

http://codereview.chromium.org/6764001/diff/4001/content/renderer/render_view...
content/renderer/render_view.cc:635:
webview()->setSpellCheckClient(spellcheck_provider_);
On 2011/03/29 18:28:37, morrita wrote:
> How about to allow SpellCheckProvider to set itself to the webview()
> instead of set it by RenderView?

yep I was planning on doing this when I move the spellcheck dependencies
completely from RV, soon

> 
> I also think that it might be better to setSpellCheckClient(NULL) when
> the client interface is gone before webview() get deleted.
> Such kind of symmetry would make its lifetime clear.

right now I was keeping the old behavior/lifetime, then update the webkit code,
and then update RV to completely remove spelling references.  then i would null
it out in spellcheckprovider's destructor

http://codereview.chromium.org/6764001/diff/4005/chrome/renderer/spellchecker...
File chrome/renderer/spellchecker/spellcheck_provider.cc (right):

http://codereview.chromium.org/6764001/diff/4005/chrome/renderer/spellchecker...
chrome/renderer/spellchecker/spellcheck_provider.cc:86: // Will be NULL during
unit tests.
On 2011/03/29 18:28:37, morrita wrote:
> Could you move this null check to where the instance is created
> and skip the instance creation if it is null?
> Then we can assume that RenderThread is always available
> in methods in this class.

i would prefer to just copy the logic from before, and then any cleanup for the
old code can be done in followup changes.  this reduces the likelyhood of me
breaking any spellchecking tests, which I'm not familiar with.

http://codereview.chromium.org/6764001/diff/4005/chrome/renderer/spellchecker...
File chrome/renderer/spellchecker/spellcheck_provider.h (right):

http://codereview.chromium.org/6764001/diff/4005/chrome/renderer/spellchecker...
chrome/renderer/spellchecker/spellcheck_provider.h:84: #if defined(OS_MACOSX)
On 2011/03/29 18:28:37, morrita wrote:
> I don't think this ifdef is necessary.
> How about to just left it as false?

I was just copying over the existing logic from RenderView.  Was it done like
that for any reason?  It does seem less confusing to have it only there for mac,
so that people looking at this who don't know anything about document tags
realize that it's only for mac.

[gmorrita, 2011-03-29 18:45:58]

That makes sense. Let's have some cleanup after this change landed.


On 2011/03/29 18:37:54, John Abd-El-Malek wrote:
>
http://codereview.chromium.org/6764001/diff/4001/content/renderer/render_view.cc
> File content/renderer/render_view.cc (right):
> 
>
http://codereview.chromium.org/6764001/diff/4001/content/renderer/render_view...
> content/renderer/render_view.cc:635:
> webview()->setSpellCheckClient(spellcheck_provider_);
> On 2011/03/29 18:28:37, morrita wrote:
> > How about to allow SpellCheckProvider to set itself to the webview()
> > instead of set it by RenderView?
> 
> yep I was planning on doing this when I move the spellcheck dependencies
> completely from RV, soon
> 
> > 
> > I also think that it might be better to setSpellCheckClient(NULL) when
> > the client interface is gone before webview() get deleted.
> > Such kind of symmetry would make its lifetime clear.
> 
> right now I was keeping the old behavior/lifetime, then update the webkit
code,
> and then update RV to completely remove spelling references.  then i would
null
> it out in spellcheckprovider's destructor
> 
>
http://codereview.chromium.org/6764001/diff/4005/chrome/renderer/spellchecker...
> File chrome/renderer/spellchecker/spellcheck_provider.cc (right):
> 
>
http://codereview.chromium.org/6764001/diff/4005/chrome/renderer/spellchecker...
> chrome/renderer/spellchecker/spellcheck_provider.cc:86: // Will be NULL during
> unit tests.
> On 2011/03/29 18:28:37, morrita wrote:
> > Could you move this null check to where the instance is created
> > and skip the instance creation if it is null?
> > Then we can assume that RenderThread is always available
> > in methods in this class.
> 
> i would prefer to just copy the logic from before, and then any cleanup for
the
> old code can be done in followup changes.  this reduces the likelyhood of me
> breaking any spellchecking tests, which I'm not familiar with.
> 
>
http://codereview.chromium.org/6764001/diff/4005/chrome/renderer/spellchecker...
> File chrome/renderer/spellchecker/spellcheck_provider.h (right):
> 
>
http://codereview.chromium.org/6764001/diff/4005/chrome/renderer/spellchecker...
> chrome/renderer/spellchecker/spellcheck_provider.h:84: #if defined(OS_MACOSX)
> On 2011/03/29 18:28:37, morrita wrote:
> > I don't think this ifdef is necessary.
> > How about to just left it as false?
> 
> I was just copying over the existing logic from RenderView.  Was it done like
> that for any reason?  It does seem less confusing to have it only there for
mac,
> so that people looking at this who don't know anything about document tags
> realize that it's only for mac.

[jam, 2011-03-29 19:03:35]

so i'm clear, is this a lgtm?

On 2011/03/29 18:45:58, morrita wrote:
> That makes sense. Let's have some cleanup after this change landed.
> 
> 
> On 2011/03/29 18:37:54, John Abd-El-Malek wrote:
> >
>
http://codereview.chromium.org/6764001/diff/4001/content/renderer/render_view.cc
> > File content/renderer/render_view.cc (right):
> > 
> >
>
http://codereview.chromium.org/6764001/diff/4001/content/renderer/render_view...
> > content/renderer/render_view.cc:635:
> > webview()->setSpellCheckClient(spellcheck_provider_);
> > On 2011/03/29 18:28:37, morrita wrote:
> > > How about to allow SpellCheckProvider to set itself to the webview()
> > > instead of set it by RenderView?
> > 
> > yep I was planning on doing this when I move the spellcheck dependencies
> > completely from RV, soon
> > 
> > > 
> > > I also think that it might be better to setSpellCheckClient(NULL) when
> > > the client interface is gone before webview() get deleted.
> > > Such kind of symmetry would make its lifetime clear.
> > 
> > right now I was keeping the old behavior/lifetime, then update the webkit
> code,
> > and then update RV to completely remove spelling references.  then i would
> null
> > it out in spellcheckprovider's destructor
> > 
> >
>
http://codereview.chromium.org/6764001/diff/4005/chrome/renderer/spellchecker...
> > File chrome/renderer/spellchecker/spellcheck_provider.cc (right):
> > 
> >
>
http://codereview.chromium.org/6764001/diff/4005/chrome/renderer/spellchecker...
> > chrome/renderer/spellchecker/spellcheck_provider.cc:86: // Will be NULL
during
> > unit tests.
> > On 2011/03/29 18:28:37, morrita wrote:
> > > Could you move this null check to where the instance is created
> > > and skip the instance creation if it is null?
> > > Then we can assume that RenderThread is always available
> > > in methods in this class.
> > 
> > i would prefer to just copy the logic from before, and then any cleanup for
> the
> > old code can be done in followup changes.  this reduces the likelyhood of me
> > breaking any spellchecking tests, which I'm not familiar with.
> > 
> >
>
http://codereview.chromium.org/6764001/diff/4005/chrome/renderer/spellchecker...
> > File chrome/renderer/spellchecker/spellcheck_provider.h (right):
> > 
> >
>
http://codereview.chromium.org/6764001/diff/4005/chrome/renderer/spellchecker...
> > chrome/renderer/spellchecker/spellcheck_provider.h:84: #if
defined(OS_MACOSX)
> > On 2011/03/29 18:28:37, morrita wrote:
> > > I don't think this ifdef is necessary.
> > > How about to just left it as false?
> > 
> > I was just copying over the existing logic from RenderView.  Was it done
like
> > that for any reason?  It does seem less confusing to have it only there for
> mac,
> > so that people looking at this who don't know anything about document tags
> > realize that it's only for mac.

[gmorrita, 2011-03-29 20:29:38]

lgtm.

On 2011/03/29 19:03:35, John Abd-El-Malek wrote:
> so i'm clear, is this a lgtm?
> 
> On 2011/03/29 18:45:58, morrita wrote:
> > That makes sense. Let's have some cleanup after this change landed.
> > 
> > 
> > On 2011/03/29 18:37:54, John Abd-El-Malek wrote:
> > >
> >
>
http://codereview.chromium.org/6764001/diff/4001/content/renderer/render_view.cc
> > > File content/renderer/render_view.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/6764001/diff/4001/content/renderer/render_view...
> > > content/renderer/render_view.cc:635:
> > > webview()->setSpellCheckClient(spellcheck_provider_);
> > > On 2011/03/29 18:28:37, morrita wrote:
> > > > How about to allow SpellCheckProvider to set itself to the webview()
> > > > instead of set it by RenderView?
> > > 
> > > yep I was planning on doing this when I move the spellcheck dependencies
> > > completely from RV, soon
> > > 
> > > > 
> > > > I also think that it might be better to setSpellCheckClient(NULL) when
> > > > the client interface is gone before webview() get deleted.
> > > > Such kind of symmetry would make its lifetime clear.
> > > 
> > > right now I was keeping the old behavior/lifetime, then update the webkit
> > code,
> > > and then update RV to completely remove spelling references.  then i would
> > null
> > > it out in spellcheckprovider's destructor
> > > 
> > >
> >
>
http://codereview.chromium.org/6764001/diff/4005/chrome/renderer/spellchecker...
> > > File chrome/renderer/spellchecker/spellcheck_provider.cc (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/6764001/diff/4005/chrome/renderer/spellchecker...
> > > chrome/renderer/spellchecker/spellcheck_provider.cc:86: // Will be NULL
> during
> > > unit tests.
> > > On 2011/03/29 18:28:37, morrita wrote:
> > > > Could you move this null check to where the instance is created
> > > > and skip the instance creation if it is null?
> > > > Then we can assume that RenderThread is always available
> > > > in methods in this class.
> > > 
> > > i would prefer to just copy the logic from before, and then any cleanup
for
> > the
> > > old code can be done in followup changes.  this reduces the likelyhood of
me
> > > breaking any spellchecking tests, which I'm not familiar with.
> > > 
> > >
> >
>
http://codereview.chromium.org/6764001/diff/4005/chrome/renderer/spellchecker...
> > > File chrome/renderer/spellchecker/spellcheck_provider.h (right):
> > > 
> > >
> >
>
http://codereview.chromium.org/6764001/diff/4005/chrome/renderer/spellchecker...
> > > chrome/renderer/spellchecker/spellcheck_provider.h:84: #if
> defined(OS_MACOSX)
> > > On 2011/03/29 18:28:37, morrita wrote:
> > > > I don't think this ifdef is necessary.
> > > > How about to just left it as false?
> > > 
> > > I was just copying over the existing logic from RenderView.  Was it done
> like
> > > that for any reason?  It does seem less confusing to have it only there
for
> > mac,
> > > so that people looking at this who don't know anything about document tags
> > > realize that it's only for mac.