Remove PlatformFile from spell checker

Remove PlatformFile from spell checker

BUG=322664
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=259163

[rvargas (doing something else), 2014-03-24 19:03:06]

PTAL

+cpu for the ipc code.

[groby-ooo-7-16, 2014-03-24 19:44:56]

spellcheck LGTM w/ tiny wish.

https://codereview.chromium.org/209193002/diff/20001/chrome/renderer/spellche...
File chrome/renderer/spellchecker/hunspell_engine.cc (right):

https://codereview.chromium.org/209193002/diff/20001/chrome/renderer/spellche...
chrome/renderer/spellchecker/hunspell_engine.cc:54: file_ = file.Pass();
There's a small window here where IsEnabled returns false even though file_ is
valid...

IIRC, that is fine. But I'd appreciate a DCHECK in IsEnabled. Something like
this:

// It is impossible to have a valid file while hunspell is disabled.
DCHECK(!file_.IsValid() || hunspell_enabled_)

[rvargas (doing something else), 2014-03-24 23:53:30]

https://codereview.chromium.org/209193002/diff/20001/chrome/renderer/spellche...
File chrome/renderer/spellchecker/hunspell_engine.cc (right):

https://codereview.chromium.org/209193002/diff/20001/chrome/renderer/spellche...
chrome/renderer/spellchecker/hunspell_engine.cc:54: file_ = file.Pass();
On 2014/03/24 19:44:57, groby wrote:
> There's a small window here where IsEnabled returns false even though file_ is
> valid...

The logic is confusing to me.

IsEnabled() returns true if Init() was called with a valid file, regardless of
failures inside InitializeHunspell().

I could make IsEnabled keep exactly that logic.

Note that another change in behavior is that InitializeHunspell currently keeps
calling 
bdict_file_->Initialize when it fails, but it uses a broken handle after the
first one.
> 
> IIRC, that is fine. But I'd appreciate a DCHECK in IsEnabled. Something like
> this:
> 
> // It is impossible to have a valid file while hunspell is disabled.
> DCHECK(!file_.IsValid() || hunspell_enabled_)

I don't get that. Do you want to add a dcheck that would fail when calling
IsEnabled() between Init() and InitializeHunspell()? That means:
if (InitializeIfNeeded()){
  Wait a while so that Init is called;
  IsEnabled()
}
would trigger it, right?

[groby-ooo-7-16, 2014-03-25 00:42:26]

https://codereview.chromium.org/209193002/diff/20001/chrome/renderer/spellche...
File chrome/renderer/spellchecker/hunspell_engine.cc (right):

https://codereview.chromium.org/209193002/diff/20001/chrome/renderer/spellche...
chrome/renderer/spellchecker/hunspell_engine.cc:54: file_ = file.Pass();
On 2014/03/24 23:53:30, rvargas wrote:
> On 2014/03/24 19:44:57, groby wrote:
> > There's a small window here where IsEnabled returns false even though file_
is
> > valid...
> 
> The logic is confusing to me.
> 
> IsEnabled() returns true if Init() was called with a valid file, regardless of
> failures inside InitializeHunspell().

Yes. 

> I could make IsEnabled keep exactly that logic.

That might be the better choice, because the whole "Initialize" chain is...
tricky.

> Note that another change in behavior is that InitializeHunspell currently
keeps
> calling 
> bdict_file_->Initialize when it fails, but it uses a broken handle after the
> first one.

Wait, in the current version file_ is never reset/passed, so wouldn't it reuse
the correct handle?

In the _new_ version, it would use a broken handle. (Due to file_.Pass()), but
InitializeIfNeeded guards against that.

Or am I misunderstanding?
> > 
> > IIRC, that is fine. But I'd appreciate a DCHECK in IsEnabled. Something like
> > this:
> > 
> > // It is impossible to have a valid file while hunspell is disabled.
> > DCHECK(!file_.IsValid() || hunspell_enabled_)
> 
> I don't get that. Do you want to add a dcheck that would fail when calling
> IsEnabled() between Init() and InitializeHunspell()? That means:
> if (InitializeIfNeeded()){
>   Wait a while so that Init is called;
>   IsEnabled()
> }
> would trigger it, right?

Yes, that is correct. We should never do that, essentially.

Looking over this again: In the interest of preserving the existing madness,
you're probably best off setting hunspell_enabled_ = file_.IsValid() in Init().

We will lose the retry behavior if bdict_file_->Initialize fails. I suppose that
might make sense when memory is temporarily very tight, but I think that's not a
common problem. And the failure mode is "don't spellcheck", which I don't
consider a huge problem, either.

Does that make sense?

[rvargas (doing something else), 2014-03-25 01:07:10]

Reverted the meaning of hunspell_enabled_. Please take another look.

https://codereview.chromium.org/209193002/diff/20001/chrome/renderer/spellche...
File chrome/renderer/spellchecker/hunspell_engine.cc (right):

https://codereview.chromium.org/209193002/diff/20001/chrome/renderer/spellche...
chrome/renderer/spellchecker/hunspell_engine.cc:54: file_ = file.Pass();
On 2014/03/25 00:42:26, groby wrote:
> On 2014/03/24 23:53:30, rvargas wrote:
> > On 2014/03/24 19:44:57, groby wrote:
> > > There's a small window here where IsEnabled returns false even though
file_
> is
> > > valid...
> > 
> > The logic is confusing to me.
> > 
> > IsEnabled() returns true if Init() was called with a valid file, regardless
of
> > failures inside InitializeHunspell().
> 
> Yes. 
> 
> > I could make IsEnabled keep exactly that logic.
> 
> That might be the better choice, because the whole "Initialize" chain is...
> tricky.
> 
> > Note that another change in behavior is that InitializeHunspell currently
> keeps
> > calling 
> > bdict_file_->Initialize when it fails, but it uses a broken handle after the
> > first one.
> 
> Wait, in the current version file_ is never reset/passed, so wouldn't it reuse
> the correct handle?
> 
> In the _new_ version, it would use a broken handle. (Due to file_.Pass()), but
> InitializeIfNeeded guards against that.
> 
> Or am I misunderstanding?

The current source has a PlatformFile and instantiates a File with it.
bdict_file_->Initialize() takes ownership of that file regardless of failure so
if initialization fails the handle is closed and the value kept by this code
becomes stale. Next time this code will be passing a random object to
bdict_file_.

To maintain retry behavior bdict_file_ code has to change to explicitly return
the file if the operation fails.

The new code is fine even without an explicit guard because bdict_file_ can
easily see that if it receives an invalid File.

[cpu_(ooo_6.6-7.5), 2014-03-25 01:07:42]

lgtm on IPC change.

[groby-ooo-7-16, 2014-03-25 01:18:50]

Spellcheck LGTM

[rvargas (doing something else), 2014-03-25 01:41:10]

The CQ bit was checked by rvargas@chromium.org

[commit-bot: I haz the power, 2014-03-25 01:42:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rvargas@chromium.org/209193002/60001

[commit-bot: I haz the power, 2014-03-25 06:02:02]

Change committed as 259163