Spellchecker:...

Spellchecker:

Move file accesses during initialization to file thread. Also fix a rare (and harmless) memory leak. Also introduce a rare (and harmless) memory leak.

TEST=added dchecks, ran spellcheck unit tests, clicked around, changed dictionaries, added words
BUG=22984
BUG=21924
BUG=1123096
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=28512

[Evan Stade, 2009-10-08 06:09:06]

-thestig, +sidchat

[tony, 2009-10-08 17:41:27]

LGTM, mostly just nits.

http://codereview.chromium.org/269020/diff/1/2
File chrome/browser/spellchecker.cc (right):

http://codereview.chromium.org/269020/diff/1/2#newcode223
Line 223: ui_loop_(spellchecker->ui_loop_),
FWIW, there's now a ChromeThread::UI so we don't have to keep passing the ui
loop around all the time anymore.

http://codereview.chromium.org/269020/diff/1/2#newcode272
Line 272: ui_loop_->PostTask(FROM_HERE, new UIProxyForIOTask(task,
spellchecker_));
Huh, tricky crash case.

http://codereview.chromium.org/269020/diff/1/2#newcode512
Line 512: // thread thatSpellCheckWord is called on, so we verify that they were
all the
Nit: Missing space in thatSpellCheckWord

http://codereview.chromium.org/269020/diff/1/2#newcode567
Line 567: DCHECK(worker_loop_ == MessageLoop::current());
When do we hit this else case (i.e., can Initialize get called twice?  Maybe
instead DCHECK that worker_loop_ is NULL before assigning it
MessageLoop::current()?

http://codereview.chromium.org/269020/diff/1/2#newcode630
Line 630: Release();
Hmm, I wonder if you could use:
  scoped_refptr<SpellChecker> releaser;
  releaser.swap(this);
or something to get the release at method exit?

http://codereview.chromium.org/269020/diff/1/2#newcode635
Line 635: TimeTicks::Now() - hunspell_init_start_);
Is this histogram still meaningful?  It seems like it will be all over the board
depending on how busy disk is, but maybe we care about outliers?

[Evan Stade, 2009-10-08 18:57:04]

http://codereview.chromium.org/269020/diff/1/2
File chrome/browser/spellchecker.cc (right):

http://codereview.chromium.org/269020/diff/1/2#newcode223
Line 223: ui_loop_(spellchecker->ui_loop_),
On 2009/10/08 17:41:27, tony wrote:
> FWIW, there's now a ChromeThread::UI so we don't have to keep passing the ui
> loop around all the time anymore.

Done.

http://codereview.chromium.org/269020/diff/1/2#newcode272
Line 272: ui_loop_->PostTask(FROM_HERE, new UIProxyForIOTask(task,
spellchecker_));
On 2009/10/08 17:41:27, tony wrote:
> Huh, tricky crash case.

yes, thankfully someone else ran into it first :)

http://codereview.chromium.org/269020/diff/1/2#newcode512
Line 512: // thread thatSpellCheckWord is called on, so we verify that they were
all the
On 2009/10/08 17:41:27, tony wrote:
> Nit: Missing space in thatSpellCheckWord

Done.

http://codereview.chromium.org/269020/diff/1/2#newcode567
Line 567: DCHECK(worker_loop_ == MessageLoop::current());
On 2009/10/08 17:41:27, tony wrote:
> When do we hit this else case (i.e., can Initialize get called twice?  Maybe
> instead DCHECK that worker_loop_ is NULL before assigning it
> MessageLoop::current()?

Yes, initialize gets called multiple times by design.

http://codereview.chromium.org/269020/diff/1/2#newcode630
Line 630: Release();
On 2009/10/08 17:41:27, tony wrote:
> Hmm, I wonder if you could use:
>   scoped_refptr<SpellChecker> releaser;
>   releaser.swap(this);
> or something to get the release at method exit?

my template-foo is middling but it doesn't look like you can do that with a
RefCountedThreadSafe

http://codereview.chromium.org/269020/diff/1/2#newcode635
Line 635: TimeTicks::Now() - hunspell_init_start_);
On 2009/10/08 17:41:27, tony wrote:
> Is this histogram still meaningful?  It seems like it will be all over the
board
> depending on how busy disk is, but maybe we care about outliers?

hmm, yes, that's true. It should be moved to the ReadDictionaryTask, but I was
worried about whether writing histograms was thread safe, so I tried to leave it
on the io thread.

But now that I actually read the code it seems it should be fine as long as a
given histogram is only accessed from one thread.

[Evan Stade, 2009-10-08 20:52:20]

ping sidchat.

[sidchat (Google), 2009-10-08 21:05:40]

LGTM from my side. Thanks for making this work - it has been on hold for a long
time.


On 2009/10/08 20:52:20, Evan Stade wrote:
> ping sidchat.