Remove PlatformFile from spell checker

chrome/renderer/spellchecker/hunspell_engine.cc

Index: chrome/renderer/spellchecker/hunspell_engine.cc
diff --git a/chrome/renderer/spellchecker/hunspell_engine.cc b/chrome/renderer/spellchecker/hunspell_engine.cc
index c2cdd29f560f2567965120a686c6fe39cb09900e..772c7f3f08486a984c743775c2abf7ad825c3af7 100644
--- a/chrome/renderer/spellchecker/hunspell_engine.cc
+++ b/chrome/renderer/spellchecker/hunspell_engine.cc
@@ -29,7 +29,7 @@ namespace {
 
   COMPILE_ASSERT(kMaxCheckedLen <= size_t(MAXWORDLEN), MaxCheckedLen_too_long);
   COMPILE_ASSERT(kMaxSuggestLen <= kMaxCheckedLen, MaxSuggestLen_too_long);
-}
+}  // namespace
 
 #if !defined(OS_MACOSX)
 SpellingEngine* CreateNativeSpellingEngine() {
@@ -38,7 +38,7 @@ SpellingEngine* CreateNativeSpellingEngine() {
 #endif
 
 HunspellEngine::HunspellEngine()
-    : file_(base::kInvalidPlatformFileValue),
+    : hunspell_enabled_(false),
       initialized_(false),
       dictionary_requested_(false) {
   // Wait till we check the first word before doing any initializing.
@@ -47,11 +47,11 @@ HunspellEngine::HunspellEngine()
 HunspellEngine::~HunspellEngine() {
 }
 
-void HunspellEngine::Init(base::PlatformFile file) {
+void HunspellEngine::Init(base::File file) {
   initialized_ = true;
   hunspell_.reset();
   bdict_file_.reset();
-  file_ = file;
+  file_ = file.Pass();

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

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]

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.
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]

Yes. 
That might be the better choice, because the whole "Initialize" chain is...
tricky.
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?
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]

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.

   // Delay the actual initialization of hunspell until it is needed.
 }
 
@@ -61,13 +61,11 @@ void HunspellEngine::InitializeHunspell() {
 
   bdict_file_.reset(new base::MemoryMappedFile);
 
-  // TODO(rvargas): This object should not keep file_ after passing it to
-  // bdict_file_.
-  if (bdict_file_->Initialize(base::File(file_))) {
+  if (bdict_file_->Initialize(file_.Pass())) {
     TimeTicks debug_start_time = base::Histogram::DebugNow();
 
-    hunspell_.reset(
-        new Hunspell(bdict_file_->data(), bdict_file_->length()));
+    hunspell_.reset(new Hunspell(bdict_file_->data(), bdict_file_->length()));
+    hunspell_enabled_ = true;
 
     DHISTOGRAM_TIMES("Spellcheck.InitTime",
                      base::Histogram::DebugNow() - debug_start_time);
@@ -134,12 +132,12 @@ bool HunspellEngine::InitializeIfNeeded() {
   }
 
   // Don't initialize if hunspell is disabled.
-  if (file_ != base::kInvalidPlatformFileValue)
+  if (file_.IsValid())
     InitializeHunspell();
 
   return !initialized_;
 }
 
 bool HunspellEngine::IsEnabled() {
-  return file_ != base::kInvalidPlatformFileValue;
+  return hunspell_enabled_;
 }