Move the spellchecker to the renderer....

Move the spellchecker to the renderer.

The motivation is that this removes the sync IPC on every call to the spellchecker. Also, currently we spellcheck in the IO thread, which frequently needs to go to disk (in particular, the entire spellcheck dictionary starts paged out), so this will block just the single renderer when that happens, rather than the whole IO thread.

This breaks the SpellChecker class into two new classes.
1) On the browser side, we have SpellCheckHost. This class handles browser-wide tasks, such as keeping the custom words list in sync with the on-disk custom words dictionary, downloading missing dictionaries, etc. On Posix, it also opens the bdic file since the renderer isn't allowed to open files. SpellCheckHost is created and destroyed on the UI thread. It is initialized on the file thread.
2) On the renderer side, SpellChecker2. This class will one day be renamed SpellChecker. It handles actual checking of the words, memory maps the dictionary file, loads hunspell, etc. There is one SpellChecker2 per RenderThread (hence one per render process).

My intention is for this patch to move Linux to this new approach, and follow up with ports for Windows (which will involve passing a dictionary file name rather than a file descriptor through to the renderer) and Mac (which will involve adding sync ViewHost IPC callsfor when the platform spellchecker is enabled). Note that anyone using the platform spellchecker rather than Hunspell will get no benefit out of this refactor.

There should be no loss of functionality for Linux (or any other platform) in this patch. The following should all still work:
- dictionary is loaded lazily
- hunspell is initialized lazily, per renderer
- language changes work.
- Dynamic downloading of new dictionaries
- auto spell correct works (as well as toggling it).
- disabling spellcheck works.
- custom words work (including adding in one renderer and immediately having it take effect in other renderers, for certain values of "immediate")

TODO:
- move spellchecker unit tests to test SpellCheck2
- add sync IPC for platform spellchecker; port to Mac
- add dictionary location fallback; port to Windows
- remove SpellChecker classes from browser/

BUG=25677

[Evan Stade, 2009-11-04 02:49:37]

I have not even tried to compile it on windows/mac, so for now just assume that
I've sprinkled ifdefs in all the right places.

[darin (slow to review), 2009-11-04 03:15:10]

Out of curiosity, what problems does this solve?
-Darin

On Tue, Nov 3, 2009 at 6:49 PM, <estade@chromium.org> wrote:

> Reviewers: John Abd-El-Malek, brettw, Peter Kasting,
>
> Message:
> I have not even tried to compile it on windows/mac, so for now just assume
> that
> I've sprinkled ifdefs in all the right places.
>
> Description:
> Move the spellchecker to the renderer.
>
> This breaks the SpellChecker class into two new classes.
> 1) On the browser side, we have SpellCheckUtil. This class handles
> browser-wide
> tasks, such as keeping the custom words list in sync with the on-disk
> custom
> words dictionary, downloading missing dictionaries, etc. On Posix, it also
> opens
> the bdic file since the renderer isn't allowed to open files.
> SpellCheckUtil is
> created and destroyed on the UI thread. It is initialized on the file
> thread.
> 2) On the renderer side, SpellChecker2. This class will one day be renamed
> SpellChecker. It handles actual checking of the words, memory maps the
> dictionary file, loads hunspell, etc. There is one SpellChecker2 per
> RenderThread (hence one per render process).
>
> My intention is for this patch to move Linux to this new approach, and
> follow up
> with ports for Windows (which will involve passing a dictionary file name
> rather
> than a file descriptor through to the renderer) and Mac (which will involve
> adding sync ViewHost IPC callsfor when the platform spellchecker is
> enabled).
> Note that anyone using the platform spellchecker rather than Hunspell will
> get
> no benefit out of this refactor.
>
> There should be no loss of functionality for Linux in this patch. The
> following
> should all still work:
> - dictionary is loaded lazily
> - hunspell is initialized lazily, per renderer
> - language changes work.
> - Dynamic downloading of new dictionaries
> - auto spell correct works (as well as toggling it).
> - disabling spellcheck works.
> - custom words work (including adding in one renderer and immediately
> having it
> take effect in other renderers, for certain values of "immediate")
>
> TODO:
> - move spellchecker unit tests to test SpellCheck2
> - add sync IPC for platform spellchecker; port to Mac
> - add dictionary location fallback; port to Windows
> - remove SpellChecker classes from browser/
>
> BUG=25677
>
>
> Please review this at http://codereview.chromium.org/357003
>
> SVN Base: svn://chrome-svn/chrome/trunk/src/
>
> Affected files:
>  M     base/file_util.h
>  M     base/file_util_posix.cc
>  M     chrome/browser/net/chrome_url_request_context.cc
>  M     chrome/browser/profile.h
>  M     chrome/browser/profile.cc
>  M     chrome/browser/renderer_host/browser_render_process_host.h
>  M     chrome/browser/renderer_host/browser_render_process_host.cc
>  M     chrome/browser/spellchecker.cc
>  M     chrome/browser/spellchecker_mac.mm
>  M     chrome/browser/tab_contents/render_view_context_menu.cc
>  M     chrome/chrome.gyp
>  M     chrome/common/notification_type.h
>  M     chrome/common/render_messages_internal.h
>  M     chrome/renderer/render_thread.h
>  M     chrome/renderer/render_thread.cc
>  A  +  chrome/renderer/spellchecker/spellcheck_worditerator.h
>  A  +  chrome/renderer/spellchecker/spellcheck_worditerator.cc
>  A  +  chrome/renderer/spellchecker/spellchecker2.h
>  A  +  chrome/renderer/spellchecker/spellchecker2.cc
>
>
>

[Evan Stade, 2009-11-04 03:47:27]

It removes the sync IPC on every call to the spellchecker. Also, currently we
spellcheck in the IO thread, which frequently needs to go to disk (in
particular, the entire spellcheck dictionary starts paged out), so this will
block just the single renderer when that happens, rather than the whole IO
thread.

http://groups.google.com/group/chromium-dev/browse_thread/thread/fef0ba204b29...

[darin (slow to review), 2009-11-04 04:21:35]

oh, that does sound like a huge win.  can you add this to the CL
description? :-)
-darin

On Tue, Nov 3, 2009 at 7:47 PM, <estade@chromium.org> wrote:

> It removes the sync IPC on every call to the spellchecker. Also, currently
> we
> spellcheck in the IO thread, which frequently needs to go to disk (in
> particular, the entire spellcheck dictionary starts paged out), so this
> will
> block just the single renderer when that happens, rather than the whole IO
> thread.
>
>
>
http://groups.google.com/group/chromium-dev/browse_thread/thread/fef0ba204b29...
>
>
>
> http://codereview.chromium.org/357003
>

[Peter Kasting, 2009-11-04 21:00:00]

Generally looks fine, although I didn't think hard about it.  The one thing you
haven't really addressed is what should be done with the old
DeleteSpellChecker() logic, which was added solely for the memory purger.  When
the spellchecker lived in the browser, deleting it could free the memory used
for the dictionary.  Now, presumably, the browser-side util object uses
virtually no memory, and if we want to free this, we'd need to tell all the
renderer-side objects to close their file handle.  Does that sound right?  If
so, you can rip the browser-side stuff out entirely and plumb a call to do this
from render_thread.cc:OnPurgeMemory(), which will get called in each renderer.

http://codereview.chromium.org/357003/diff/6001/7006
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/357003/diff/6001/7006#newcode1310
Line 1310: NotificationService::current()->Notify(
Nit: Maybe this should call SpellCheckUtilInitialized() and that function should
add a "spellcheck_util_ &&" to the front of its conditional?

http://codereview.chromium.org/357003/diff/6001/7010
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/357003/diff/6001/7010#newcode207
Line 207: 
Nit: No need for newline

http://codereview.chromium.org/357003/diff/6001/7010#newcode370
Line 370: #if defined(OS_LINUX)
Nit: Why not put this with the other three Init...() calls?

http://codereview.chromium.org/357003/diff/6001/7009
File chrome/browser/renderer_host/browser_render_process_host.h (right):

http://codereview.chromium.org/357003/diff/6001/7009#newcode149
Line 149: // The renderer has requested that we initialize it. This should
generally
Nit: "it" -> "its spellchecker"?

http://codereview.chromium.org/357003/diff/6001/7020
File chrome/browser/spellcheck_util.cc (right):

http://codereview.chromium.org/357003/diff/6001/7020#newcode28
Line 28: } g_supported_spellchecker_languages[] = {
Nit: Is there an order for these initializers?  It looks random.

http://codereview.chromium.org/357003/diff/6001/7020#newcode40
Line 40: // {"th", "th-TH"}, // Not to be included in Spellchecker as per
B=1277824
Nit: These EOL comments kind of suck, can they be replaced by one comment
somewhere which doesn't reference buganizer?

http://codereview.chromium.org/357003/diff/6001/7020#newcode68
Line 68: std::string input_language) {
Nit: No need to wrap this.  Also, should take a const&

http://codereview.chromium.org/357003/diff/6001/7020#newcode72
Line 72: g_supported_spellchecker_languages[i].language);
Nit: I'd just roll this into the conditional, the temp just adds noise

http://codereview.chromium.org/357003/diff/6001/7020#newcode73
Line 73: if (language ==  input_language)
Nit: extra space after ==

http://codereview.chromium.org/357003/diff/6001/7020#newcode153
Line 153: ChromeThread::FILE, FROM_HERE,
Nit: No need to wrap this first pair of args (several places)

http://codereview.chromium.org/357003/diff/6001/7020#newcode201
Line 201: for (size_t i = 0; i < list_of_words.size(); ++i) {
Nit: No {}

http://codereview.chromium.org/357003/diff/6001/7020#newcode234
Line 234: std::string word_to_add = word + "\n";
Nit: Constructor style?

http://codereview.chromium.org/357003/diff/6001/7021
File chrome/browser/spellcheck_util.h (right):

http://codereview.chromium.org/357003/diff/6001/7021#newcode14
Line 14: #include "chrome/browser/net/url_fetcher.h"
Nit: Could probably just forward-declare the necessary types from this header
and #include it in the .cc file

http://codereview.chromium.org/357003/diff/6001/7021#newcode41
Line 41: return custom_words_[custom_words_.size() - 1];
Nit: "return custom_words_.back();"

http://codereview.chromium.org/357003/diff/6001/7008
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/357003/diff/6001/7008#newcode24
Line 24: #include "chrome/browser/spellcheck_util.h"
Nit: I think platform-specific #includes go after the main list

http://codereview.chromium.org/357003/diff/6001/7013
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/357003/diff/6001/7013#newcode55
Line 55: #include "chrome/renderer/spellchecker/spellchecker2.h"
Nit: I think platform-specific includes go after the main list, looks like
there's another case of this above

http://codereview.chromium.org/357003/diff/6001/7017
File chrome/renderer/spellchecker/spellchecker2.cc (right):

http://codereview.chromium.org/357003/diff/6001/7017#newcode23
Line 23: // Wait till we check the first word before doing any initing.
Nit: "initing" -> "initialization"

http://codereview.chromium.org/357003/diff/6001/7017#newcode76
Line 76: bool word_ok = CheckSpelling(word, tag);
Nit: Just roll this temp into the conditional

http://codereview.chromium.org/357003/diff/6001/7018
File chrome/renderer/spellchecker/spellchecker2.h (right):

http://codereview.chromium.org/357003/diff/6001/7018#newcode118
Line 118: // This flags whether we have ever been inited, or have asked the
browser
Nit: "inited" -> "initialized", in variable name too

[jam, 2009-11-04 21:57:13]

http://codereview.chromium.org/357003/diff/8001/9006
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/357003/diff/8001/9006#newcode481
Line 481: #if defined(OS_LINUX)
Instead of having the code that uses the new spellchecker be based on OS_LINUX
define, why not add a new check like

#if defined(SPELLCHECKER_IN_RENDERER)

That way, it'll be easy to find out which code to update when it works on other
platforms (and to remove it all together when they all work).

http://codereview.chromium.org/357003/diff/8001/9010
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/357003/diff/8001/9010#newcode1100
Line 1100: void BrowserRenderProcessHost::InitSpellChecker() {
This function's name is confusing, since without looking how it's called, it
sounds like it's the one that causes initialization, which would be strange
since the file descriptors are returned synchronously.  How about
OnSpellCheckerInitialized?

http://codereview.chromium.org/357003/diff/8001/9020
File chrome/browser/spellcheck_util.cc (right):

http://codereview.chromium.org/357003/diff/8001/9020#newcode132
Line 132: SpellCheckUtil::SpellCheckUtil(Observer* observer,
"Util" is not a standard suffix for classes that exist in the browser process
and have a matching class in the renderer.  How about SpellCheckHost?

http://codereview.chromium.org/357003/diff/8001/9020#newcode159
Line 159: close(fd_.fd);
Two things:
-since the destructor does file access, can you add a trait to  this class so
that it's destructed on the FILE thread?  i.e.
SpellCheckUtil : public base::RefCountedThreadSafe<SpellCheckUtil,
ChromeThread::DeleteOnIOThread>

-since you know for sure that this class will be used by multiple platforms, I
think you might as well add the platform dependent abstractions now.

http://codereview.chromium.org/357003/diff/8001/9020#newcode162
Line 162: // Called on UI thread.
nit: instead of comments, better to do
DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));

http://codereview.chromium.org/357003/diff/8001/9021
File chrome/browser/spellcheck_util.h (right):

http://codereview.chromium.org/357003/diff/8001/9021#newcode27
Line 27: virtual ~SpellCheckUtil();
nit: make the destructor private and friend
base::RefCountedThreadSafe<SpellCheckUtil>

http://codereview.chromium.org/357003/diff/8001/9018
File chrome/renderer/spellchecker/spellchecker2.h (right):

http://codereview.chromium.org/357003/diff/8001/9018#newcode29
Line 29: class SpellChecker2 {
I think this should land the first time with the name we'll stick with.  How
about SpellCheck?  If so, then the browser object can be SpellCheckHost.  I'm
open to other names as well.

[Evan Stade, 2009-11-04 23:28:49]

http://codereview.chromium.org/357003/diff/6001/7006
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/357003/diff/6001/7006#newcode1310
Line 1310: NotificationService::current()->Notify(
On 2009/11/04 21:00:00, Peter Kasting wrote:
> Nit: Maybe this should call SpellCheckUtilInitialized() and that function
should
> add a "spellcheck_util_ &&" to the front of its conditional?

this grates on my senses but I guess it's ok. done.

http://codereview.chromium.org/357003/diff/6001/7010
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/357003/diff/6001/7010#newcode207
Line 207: 
On 2009/11/04 21:00:00, Peter Kasting wrote:
> Nit: No need for newline

Done.

http://codereview.chromium.org/357003/diff/6001/7010#newcode370
Line 370: #if defined(OS_LINUX)
On 2009/11/04 21:00:00, Peter Kasting wrote:
> Nit: Why not put this with the other three Init...() calls?

Done.

http://codereview.chromium.org/357003/diff/6001/7009
File chrome/browser/renderer_host/browser_render_process_host.h (right):

http://codereview.chromium.org/357003/diff/6001/7009#newcode149
Line 149: // The renderer has requested that we initialize it. This should
generally
On 2009/11/04 21:00:00, Peter Kasting wrote:
> Nit: "it" -> "its spellchecker"?

Done.

http://codereview.chromium.org/357003/diff/6001/7020
File chrome/browser/spellcheck_util.cc (right):

http://codereview.chromium.org/357003/diff/6001/7020#newcode28
Line 28: } g_supported_spellchecker_languages[] = {
On 2009/11/04 21:00:00, Peter Kasting wrote:
> Nit: Is there an order for these initializers?  It looks random.

this whole block is just copied. So, shrug.

http://codereview.chromium.org/357003/diff/6001/7020#newcode40
Line 40: // {"th", "th-TH"}, // Not to be included in Spellchecker as per
B=1277824
done, but I don't know why these languages are supposed to be excluded (the
buganizer link doesn't do much explaining).

http://codereview.chromium.org/357003/diff/6001/7020#newcode68
Line 68: std::string input_language) {
On 2009/11/04 21:00:00, Peter Kasting wrote:
> Nit: No need to wrap this.  Also, should take a const&

Done.

http://codereview.chromium.org/357003/diff/6001/7020#newcode72
Line 72: g_supported_spellchecker_languages[i].language);
On 2009/11/04 21:00:00, Peter Kasting wrote:
> Nit: I'd just roll this into the conditional, the temp just adds noise

Done.

http://codereview.chromium.org/357003/diff/6001/7020#newcode73
Line 73: if (language ==  input_language)
On 2009/11/04 21:00:00, Peter Kasting wrote:
> Nit: extra space after ==

Done.

http://codereview.chromium.org/357003/diff/6001/7020#newcode153
Line 153: ChromeThread::FILE, FROM_HERE,
On 2009/11/04 21:00:00, Peter Kasting wrote:
> Nit: No need to wrap this first pair of args (several places)

Done.

http://codereview.chromium.org/357003/diff/6001/7020#newcode201
Line 201: for (size_t i = 0; i < list_of_words.size(); ++i) {
On 2009/11/04 21:00:00, Peter Kasting wrote:
> Nit: No {}

Done.

http://codereview.chromium.org/357003/diff/6001/7020#newcode234
Line 234: std::string word_to_add = word + "\n";
On 2009/11/04 21:00:00, Peter Kasting wrote:
> Nit: Constructor style?

Done.

http://codereview.chromium.org/357003/diff/6001/7021
File chrome/browser/spellcheck_util.h (right):

http://codereview.chromium.org/357003/diff/6001/7021#newcode14
Line 14: #include "chrome/browser/net/url_fetcher.h"
On 2009/11/04 21:00:00, Peter Kasting wrote:
> Nit: Could probably just forward-declare the necessary types from this header
> and #include it in the .cc file

don't think so... need the delegate definition

http://codereview.chromium.org/357003/diff/6001/7021#newcode41
Line 41: return custom_words_[custom_words_.size() - 1];
On 2009/11/04 21:00:00, Peter Kasting wrote:
> Nit: "return custom_words_.back();"

Done.

http://codereview.chromium.org/357003/diff/6001/7008
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/357003/diff/6001/7008#newcode24
Line 24: #include "chrome/browser/spellcheck_util.h"
On 2009/11/04 21:00:00, Peter Kasting wrote:
> Nit: I think platform-specific #includes go after the main list

ditto

http://codereview.chromium.org/357003/diff/6001/7013
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/357003/diff/6001/7013#newcode55
Line 55: #include "chrome/renderer/spellchecker/spellchecker2.h"
On 2009/11/04 21:00:00, Peter Kasting wrote:
> Nit: I think platform-specific includes go after the main list, looks like
> there's another case of this above

changed to a different define

http://codereview.chromium.org/357003/diff/6001/7017
File chrome/renderer/spellchecker/spellchecker2.cc (right):

http://codereview.chromium.org/357003/diff/6001/7017#newcode23
Line 23: // Wait till we check the first word before doing any initing.
On 2009/11/04 21:00:00, Peter Kasting wrote:
> Nit: "initing" -> "initialization"

Done.

http://codereview.chromium.org/357003/diff/6001/7017#newcode76
Line 76: bool word_ok = CheckSpelling(word, tag);
On 2009/11/04 21:00:00, Peter Kasting wrote:
> Nit: Just roll this temp into the conditional

Done.

http://codereview.chromium.org/357003/diff/6001/7018
File chrome/renderer/spellchecker/spellchecker2.h (right):

http://codereview.chromium.org/357003/diff/6001/7018#newcode118
Line 118: // This flags whether we have ever been inited, or have asked the
browser
On 2009/11/04 21:00:00, Peter Kasting wrote:
> Nit: "inited" -> "initialized", in variable name too

Done.

http://codereview.chromium.org/357003/diff/8001/9006
File chrome/browser/profile.cc (right):

http://codereview.chromium.org/357003/diff/8001/9006#newcode481
Line 481: #if defined(OS_LINUX)
On 2009/11/04 21:57:13, John Abd-El-Malek wrote:
> Instead of having the code that uses the new spellchecker be based on OS_LINUX
> define, why not add a new check like
> 
> #if defined(SPELLCHECKER_IN_RENDERER)
> 
> That way, it'll be easy to find out which code to update when it works on
other
> platforms (and to remove it all together when they all work).

Done.

http://codereview.chromium.org/357003/diff/8001/9010
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/357003/diff/8001/9010#newcode1100
Line 1100: void BrowserRenderProcessHost::InitSpellChecker() {
On 2009/11/04 21:57:13, John Abd-El-Malek wrote:
> This function's name is confusing, since without looking how it's called, it
> sounds like it's the one that causes initialization, which would be strange
> since the file descriptors are returned synchronously.  How about
> OnSpellCheckerInitialized?

The action we are taking causes the spellchecker in the renderer to initialize
(at least partially). Another name could be OnSpellCheckHostLoaded(), but I
think this name is better.

http://codereview.chromium.org/357003/diff/8001/9020
File chrome/browser/spellcheck_util.cc (right):

http://codereview.chromium.org/357003/diff/8001/9020#newcode132
Line 132: SpellCheckUtil::SpellCheckUtil(Observer* observer,
On 2009/11/04 21:57:13, John Abd-El-Malek wrote:
> "Util" is not a standard suffix for classes that exist in the browser process
> and have a matching class in the renderer.  How about SpellCheckHost?

Done.

http://codereview.chromium.org/357003/diff/8001/9020#newcode159
Line 159: close(fd_.fd);
On 2009/11/04 21:57:13, John Abd-El-Malek wrote:
> Two things:
> -since the destructor does file access, can you add a trait to  this class so
> that it's destructed on the FILE thread?  i.e.
> SpellCheckUtil : public base::RefCountedThreadSafe<SpellCheckUtil,
> ChromeThread::DeleteOnIOThread>

done

> -since you know for sure that this class will be used by multiple platforms, I
> think you might as well add the platform dependent abstractions now.

I'd rather wait until I can check that my abstractions compile

http://codereview.chromium.org/357003/diff/8001/9020#newcode162
Line 162: // Called on UI thread.
On 2009/11/04 21:57:13, John Abd-El-Malek wrote:
> nit: instead of comments, better to do
> DCHECK(ChromeThread::CurrentlyOn(ChromeThread::UI));

Done.

http://codereview.chromium.org/357003/diff/8001/9021
File chrome/browser/spellcheck_util.h (right):

http://codereview.chromium.org/357003/diff/8001/9021#newcode27
Line 27: virtual ~SpellCheckUtil();
On 2009/11/04 21:57:13, John Abd-El-Malek wrote:
> nit: make the destructor private and friend
> base::RefCountedThreadSafe<SpellCheckUtil>

I can't get that to work:

./base/task.h:228:   instantiated from ‘void
RunnableMethodTraits<T>::ReleaseCallee(T*) [with T = SpellCheckHost]’
./base/task.h:287:   instantiated from ‘void RunnableMethod<T, Method,
Params>::ReleaseCallee() [with T = SpellCheckHost, Method = void
(SpellCheckHost::*)(const std::string&), Params = Tuple1<std::basic_string<char,
std::char_traits<char>, std::allocator<char> > >]’
./base/task.h:272:   instantiated from ‘RunnableMethod<T, Method,
Params>::~RunnableMethod() [with T = SpellCheckHost, Method = void
(SpellCheckHost::*)(const std::string&), Params = Tuple1<std::basic_string<char,
std::char_traits<char>, std::allocator<char> > >]’
chrome/browser/spellcheck_host.cc:277:   instantiated from here
chrome/browser/spellcheck_host.cc:152: error: ‘virtual
SpellCheckHost::~SpellCheckHost()’ is private
./chrome/browser/chrome_thread.h:134: error: within this context
chrome/browser/spellcheck_host.cc: In member function ‘void DeleteTask<T>::Run()
[with T = SpellCheckHost]’:
chrome/browser/spellcheck_host.cc:277:   instantiated from here
chrome/browser/spellcheck_host.cc:152: error: ‘virtual
SpellCheckHost::~SpellCheckHost()’ is private
./base/task.h:171: error: within this context

http://codereview.chromium.org/357003/diff/8001/9018
File chrome/renderer/spellchecker/spellchecker2.h (right):

http://codereview.chromium.org/357003/diff/8001/9018#newcode29
Line 29: class SpellChecker2 {
On 2009/11/04 21:57:13, John Abd-El-Malek wrote:
> I think this should land the first time with the name we'll stick with.  How
> about SpellCheck?  If so, then the browser object can be SpellCheckHost.  I'm
> open to other names as well.

We do this all the time, I don't see the problem. I see a difference between
SpellCheck{Util,Host,etc.} and this class, which is that this actually performs
the spellchecking, hence the "er".

[Peter Kasting, 2009-11-05 00:00:50]

http://codereview.chromium.org/357003/diff/6001/7020
File chrome/browser/spellcheck_util.cc (right):

http://codereview.chromium.org/357003/diff/6001/7020#newcode28
Line 28: } g_supported_spellchecker_languages[] = {
On 2009/11/04 23:28:49, Evan Stade wrote:
> On 2009/11/04 21:00:00, Peter Kasting wrote:
> > Nit: Is there an order for these initializers?  It looks random.
> 
> this whole block is just copied. So, shrug.

Yeah, I know... it seems like maybe we should go ahead and put in alphabetical
order so it will be easier to tell what languages are in the list and not later?

http://codereview.chromium.org/357003/diff/6001/7021
File chrome/browser/spellcheck_util.h (right):

http://codereview.chromium.org/357003/diff/6001/7021#newcode14
Line 14: #include "chrome/browser/net/url_fetcher.h"
On 2009/11/04 23:28:49, Evan Stade wrote:
> On 2009/11/04 21:00:00, Peter Kasting wrote:
> > Nit: Could probably just forward-declare the necessary types from this
header
> > and #include it in the .cc file
> 
> don't think so... need the delegate definition

Ah, missed that.  You're right.

[Evan Stade, 2009-11-05 19:13:49]

On 2009/11/05 00:00:50, Peter Kasting wrote:
> http://codereview.chromium.org/357003/diff/6001/7020
> File chrome/browser/spellcheck_util.cc (right):
> 
> http://codereview.chromium.org/357003/diff/6001/7020#newcode28
> Line 28: } g_supported_spellchecker_languages[] = {
> On 2009/11/04 23:28:49, Evan Stade wrote:
> > On 2009/11/04 21:00:00, Peter Kasting wrote:
> > > Nit: Is there an order for these initializers?  It looks random.
> > 
> > this whole block is just copied. So, shrug.
> 
> Yeah, I know... it seems like maybe we should go ahead and put in alphabetical
> order so it will be easier to tell what languages are in the list and not
later?

done

[Peter Kasting, 2009-11-05 19:40:14]

LGTM, basically

http://codereview.chromium.org/357003/diff/9046/9050
File chrome/browser/profile.h (right):

http://codereview.chromium.org/357003/diff/9046/9050#newcode360
Line 360: virtual void ReinitializeSpellCheckHost(bool force) = 0;
I see you added stubs to testing_profile.h, but don't you also need stubs in
automation_profile_impl.h?

http://codereview.chromium.org/357003/diff/9046/9056
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/357003/diff/9046/9056#newcode368
Line 368: if (profile()->GetSpellCheckHost())
It's not obvious why we should check GetSpellCheckHost() first, given that
InitSpellChecker() also checks it.  Are we just trying to save a renderer
message, or is it actually wrong to call InitSpellChecker() if there's no host
(and if so, why does it also check this)?

http://codereview.chromium.org/357003/diff/9046/9059
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/357003/diff/9046/9059#newcode629
Line 629: spellchecker_.reset(new SpellChecker2());
Nit: I'd put this above all the other cases above.  It definitely should be
above the FreeMemory() call at least.

http://codereview.chromium.org/357003/diff/9046/9063
File chrome/renderer/spellchecker/spellchecker2.cc (right):

http://codereview.chromium.org/357003/diff/9046/9063#newcode23
Line 23: // Wait till we check the first word before doing any intializing.
Nit: intializing -> initializing

[Evan Stade, 2009-11-05 19:55:47]

http://codereview.chromium.org/357003/diff/9046/9050
File chrome/browser/profile.h (right):

http://codereview.chromium.org/357003/diff/9046/9050#newcode360
Line 360: virtual void ReinitializeSpellCheckHost(bool force) = 0;
On 2009/11/05 19:40:14, Peter Kasting wrote:
> I see you added stubs to testing_profile.h, but don't you also need stubs in
> automation_profile_impl.h?

i dunno, the trybots seem fine without it (at least on linux---still waiting for
windows)

http://codereview.chromium.org/357003/diff/9046/9056
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/357003/diff/9046/9056#newcode368
Line 368: if (profile()->GetSpellCheckHost())
On 2009/11/05 19:40:14, Peter Kasting wrote:
> It's not obvious why we should check GetSpellCheckHost() first, given that
> InitSpellChecker() also checks it.  Are we just trying to save a renderer
> message, or is it actually wrong to call InitSpellChecker() if there's no host
> (and if so, why does it also check this)?

We don't want to init the spellchecker unless spellcheckhost has been created.
In InitSpellChecker(), we know if GetSpellCheckHost() is NULL then the
spellchecker has been turned off, but here, we don't know if it's been turned
off or just not loaded yet.

http://codereview.chromium.org/357003/diff/9046/9059
File chrome/renderer/render_thread.cc (right):

http://codereview.chromium.org/357003/diff/9046/9059#newcode629
Line 629: spellchecker_.reset(new SpellChecker2());
On 2009/11/05 19:40:14, Peter Kasting wrote:
> Nit: I'd put this above all the other cases above.  It definitely should be
> above the FreeMemory() call at least.

Done.

http://codereview.chromium.org/357003/diff/9046/9063
File chrome/renderer/spellchecker/spellchecker2.cc (right):

http://codereview.chromium.org/357003/diff/9046/9063#newcode23
Line 23: // Wait till we check the first word before doing any intializing.
On 2009/11/05 19:40:14, Peter Kasting wrote:
> Nit: intializing -> initializing

Done.

[Peter Kasting, 2009-11-05 20:13:28]

http://codereview.chromium.org/357003/diff/9046/9050
File chrome/browser/profile.h (right):

http://codereview.chromium.org/357003/diff/9046/9050#newcode360
Line 360: virtual void ReinitializeSpellCheckHost(bool force) = 0;
On 2009/11/05 19:55:47, Evan Stade wrote:
> On 2009/11/05 19:40:14, Peter Kasting wrote:
> > I see you added stubs to testing_profile.h, but don't you also need stubs in
> > automation_profile_impl.h?
> 
> i dunno, the trybots seem fine without it (at least on linux---still waiting
for
> windows)

Given that the automation profile has stubs for every single other function in
the main profile, I wouldn't trust the trybots to OK this.

http://codereview.chromium.org/357003/diff/9046/9056
File chrome/browser/renderer_host/browser_render_process_host.cc (right):

http://codereview.chromium.org/357003/diff/9046/9056#newcode368
Line 368: if (profile()->GetSpellCheckHost())
On 2009/11/05 19:55:47, Evan Stade wrote:
> On 2009/11/05 19:40:14, Peter Kasting wrote:
> > It's not obvious why we should check GetSpellCheckHost() first, given that
> > InitSpellChecker() also checks it.  Are we just trying to save a renderer
> > message, or is it actually wrong to call InitSpellChecker() if there's no
host
> > (and if so, why does it also check this)?
> 
> We don't want to init the spellchecker unless spellcheckhost has been created.
> In InitSpellChecker(), we know if GetSpellCheckHost() is NULL then the
> spellchecker has been turned off, but here, we don't know if it's been turned
> off or just not loaded yet.

Can you add a comment saying what you just said?  It was really clear.

[Evan Stade, 2009-11-05 22:33:57]

done and done. All trybots are go. John?

[jam, 2009-11-05 23:54:10]

http://codereview.chromium.org/357003/diff/8001/9021
File chrome/browser/spellcheck_util.h (right):

http://codereview.chromium.org/357003/diff/8001/9021#newcode27
Line 27: virtual ~SpellCheckUtil();
On 2009/11/04 23:28:49, Evan Stade wrote:
> On 2009/11/04 21:57:13, John Abd-El-Malek wrote:
> > nit: make the destructor private and friend
> > base::RefCountedThreadSafe<SpellCheckUtil>
> 
> I can't get that to work:
> 
> ./base/task.h:228:   instantiated from ‘void
> RunnableMethodTraits<T>::ReleaseCallee(T*) [with T = SpellCheckHost]’
> ./base/task.h:287:   instantiated from ‘void RunnableMethod<T, Method,
> Params>::ReleaseCallee() [with T = SpellCheckHost, Method = void
> (SpellCheckHost::*)(const std::string&), Params =
Tuple1<std::basic_string<char,
> std::char_traits<char>, std::allocator<char> > >]’
> ./base/task.h:272:   instantiated from ‘RunnableMethod<T, Method,
> Params>::~RunnableMethod() [with T = SpellCheckHost, Method = void
> (SpellCheckHost::*)(const std::string&), Params =
Tuple1<std::basic_string<char,
> std::char_traits<char>, std::allocator<char> > >]’
> chrome/browser/spellcheck_host.cc:277:   instantiated from here
> chrome/browser/spellcheck_host.cc:152: error: ‘virtual
> SpellCheckHost::~SpellCheckHost()’ is private
> ./chrome/browser/chrome_thread.h:134: error: within this context
> chrome/browser/spellcheck_host.cc: In member function ‘void
DeleteTask<T>::Run()
> [with T = SpellCheckHost]’:
> chrome/browser/spellcheck_host.cc:277:   instantiated from here
> chrome/browser/spellcheck_host.cc:152: error: ‘virtual
> SpellCheckHost::~SpellCheckHost()’ is private
> ./base/task.h:171: error: within this context
> 

It's worth investigating this further, because as a refcounted class only
base::RefCountedThreadSafe<SpellCheckHost> should be deleting it.  I've gone
through the entire codebase and made RefCounted objects have private
destructors, so let's try to keep it that way :)

http://codereview.chromium.org/357003/diff/8001/9018
File chrome/renderer/spellchecker/spellchecker2.h (right):

http://codereview.chromium.org/357003/diff/8001/9018#newcode29
Line 29: class SpellChecker2 {
On 2009/11/04 23:28:49, Evan Stade wrote:
> On 2009/11/04 21:57:13, John Abd-El-Malek wrote:
> > I think this should land the first time with the name we'll stick with.  How
> > about SpellCheck?  If so, then the browser object can be SpellCheckHost. 
I'm
> > open to other names as well.
> 
> We do this all the time, I don't see the problem. I see a difference between
> SpellCheck{Util,Host,etc.} and this class, which is that this actually
performs
> the spellchecking, hence the "er".

I guess I see this as causing more trouble for people going through svn history.
 Also, whatever this class is called (or will be called), it should match the
Host in the browser process.  So SpellChecker and SpellCheckHost don't match.

[Evan Stade, 2009-11-06 00:16:58]

ok, i figured out the incantations:

  friend class DeleteTask<SpellCheckHost>;
  friend class ChromeThread;

> I guess I see this as causing more trouble for people going through svn
history.
>  Also, whatever this class is called (or will be called), it should match the
> Host in the browser process.  So SpellChecker and SpellCheckHost don't match.

I disagree, because there is not a 1:1 relationship. RenderWidgetHost, for
example, does have a 1:1 relationship with RenderWidget, so it makes sense that
they share a name (down to "host"). In the spellcheck case, I don't see why
these two things need to share a name just because they are related to the same
feature. It's SpellCheckBla, because it's related to the functionality of spell
checking, and SpellCheck*er*, because it actually does the action.

[jam, 2009-11-06 00:21:22]

On Thu, Nov 5, 2009 at 4:16 PM, <estade@chromium.org> wrote:

> ok, i figured out the incantations:
>
>  friend class DeleteTask<SpellCheckHost>;
>  friend class ChromeThread;


This is incorrect.  A refcounted class should not be deleted using
DeleteSoon, which creates a DeleteTask.  It should only be Release'd.
 That's why you should only need to be friends with
base::RefCountedThreadSafe<SpellCheckHost>


>
>
>  I guess I see this as causing more trouble for people going through svn
>>
> history.
>
>>  Also, whatever this class is called (or will be called), it should match
>> the
>> Host in the browser process.  So SpellChecker and SpellCheckHost don't
>> match.
>>
>
> I disagree, because there is not a 1:1 relationship. RenderWidgetHost, for
> example, does have a 1:1 relationship with RenderWidget, so it makes sense
> that
> they share a name (down to "host").


It's not only for 1:1 host, i.e. see
ResourceDispatcher/ResourceDispatcherHost,
MessagePort/MessagePortDispatcherHost, Database, app cache, etc all have one
class per renderer and one class in the browser, and they all use this
naming convention.


> In the spellcheck case, I don't see why
> these two things need to share a name just because they are related to the
> same
> feature. It's SpellCheckBla, because it's related to the functionality of
> spell
> checking, and SpellCheck*er*, because it actually does the action.
>
>
> http://codereview.chromium.org/357003
>

[jam, 2009-11-06 00:31:01]

On Thu, Nov 5, 2009 at 4:21 PM, John Abd-El-Malek <jam@chromium.org> wrote:

>
>
> On Thu, Nov 5, 2009 at 4:16 PM, <estade@chromium.org> wrote:
>
>> ok, i figured out the incantations:
>>
>>  friend class DeleteTask<SpellCheckHost>;
>>  friend class ChromeThread;
>
>
> This is incorrect.  A refcounted class should not be deleted using
> DeleteSoon, which creates a DeleteTask.  It should only be Release'd.
>  That's why you should only need to be friends with
> base::RefCountedThreadSafe<SpellCheckHost>
>

btw I think it's because you call  spellcheck_host_->Release() directly in
profile.cc.  Instead, the pointer should be put in a scoped_refptr.  You can
do spellcheck_host_ = NULL to force it to release it.

>
>
>>
>>
>>  I guess I see this as causing more trouble for people going through svn
>>>
>> history.
>>
>>>  Also, whatever this class is called (or will be called), it should match
>>> the
>>> Host in the browser process.  So SpellChecker and SpellCheckHost don't
>>> match.
>>>
>>
>> I disagree, because there is not a 1:1 relationship. RenderWidgetHost, for
>> example, does have a 1:1 relationship with RenderWidget, so it makes sense
>> that
>> they share a name (down to "host").
>
>
> It's not only for 1:1 host, i.e. see
> ResourceDispatcher/ResourceDispatcherHost,
> MessagePort/MessagePortDispatcherHost, Database, app cache, etc all have one
> class per renderer and one class in the browser, and they all use this
> naming convention.
>
>
>> In the spellcheck case, I don't see why
>> these two things need to share a name just because they are related to the
>> same
>> feature. It's SpellCheckBla, because it's related to the functionality of
>> spell
>> checking, and SpellCheck*er*, because it actually does the action.
>>
>>
>> http://codereview.chromium.org/357003
>>
>
>

[Evan Stade, 2009-11-06 01:27:06]

renamed spellchecker2 to spellcheck, added scoped_refptr to profile, left friend
classes alone as discussed offline.

[jam, 2009-11-06 01:51:52]

lgtm

http://codereview.chromium.org/357003/diff/18003/18022
File chrome/browser/spellcheck_host.h (right):

http://codereview.chromium.org/357003/diff/18003/18022#newcode47
Line 47: friend class DeleteTask<SpellCheckHost>;
nit: order