Convert base::MemoryMappedFile to use File instead of PlatformFile.

Convert base::MemoryMappedFile to use File instead of PlatformFile.

This also modifies consumers of MemoryMappedFile and fixes double handle
close on MediaFileChecker, media_file_checker_unittest and data_pack_unittests.

BUG=322664
TEST=unit tests

R=cpu@chromium.org, dalecurtis@chromium.org (media)

TBR (owners):
tony@chromium.org (resource)
jochen@chromium.org (chrome-content)
thakis@chromium.org (base)

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

[rvargas (doing something else), 2013-12-20 20:40:11]

PTAL

[cpu_(ooo_6.6-7.5), 2013-12-21 22:59:33]

I have reviewed upto hunspell. I don't see any red flags so far.

https://codereview.chromium.org/109273002/diff/60001/base/files/file.cc
File base/files/file.cc (right):

https://codereview.chromium.org/109273002/diff/60001/base/files/file.cc#newco...
base/files/file.cc:41: created_(false),
if they pass you a handle then the created_ = true ?

https://codereview.chromium.org/109273002/diff/60001/base/files/file_posix.cc
File base/files/file_posix.cc (right):

https://codereview.chromium.org/109273002/diff/60001/base/files/file_posix.cc...
base/files/file_posix.cc:350: }
ignorance: is seeking 3 times the way of the posix ? the internets say fstat.

nb: looks at us discussing posix, how quaint.

https://codereview.chromium.org/109273002/diff/60001/base/files/memory_mapped...
File base/files/memory_mapped_file_win.cc (left):

https://codereview.chromium.org/109273002/diff/60001/base/files/memory_mapped...
base/files/memory_mapped_file_win.cc:60:
UMA_HISTOGRAM_ENUMERATION("MemoryMappedFile.CreateFileMapping",
I so hope this two umas are gone gone, not just moved.

[cpu_(ooo_6.6-7.5), 2013-12-21 23:06:28]

https://codereview.chromium.org/109273002/diff/60001/chrome/app/chrome_main_d...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/109273002/diff/60001/chrome/app/chrome_main_d...
chrome/app/chrome_main_delegate.cc:671: false);
so locale_pak_fd will be closed at this point since the temporary File object of
line 670 went out of scope. Is that ok?

[groby-ooo-7-16, 2013-12-27 19:52:25]

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

https://codereview.chromium.org/109273002/diff/60001/chrome/renderer/spellche...
chrome/renderer/spellchecker/hunspell_engine.cc:65: // bdict_file_.
If you want to address the TODO: file_, bdict_file_, initialized_ and
dictionary_requested_ together are used to coordinate a multi-stage
initialization. Relinquishing file_ might be tricky :)

(Stages are bare object, request dictionary, memory map dictionary & init
hunspell. The last 2 are on demand, when InitializeIfNeeded is called)

https://codereview.chromium.org/109273002/diff/60001/chrome/renderer/spellche...
chrome/renderer/spellchecker/hunspell_engine.cc:66: if
(bdict_file_->Initialize(base::File(file_))) {
Probably want file_ to be base::File, for clearer ownership semantics. (Also,
this fixes a file leak if InitializeHunspell is never called :)

[rvargas (doing something else), 2013-12-27 23:54:39]

Thanks.

https://codereview.chromium.org/109273002/diff/60001/base/files/file.cc
File base/files/file.cc (right):

https://codereview.chromium.org/109273002/diff/60001/base/files/file.cc#newco...
base/files/file.cc:41: created_(false),
On 2013/12/21 22:59:33, cpu wrote:
> if they pass you a handle then the created_ = true ?

In this case there is not enough information to determine what happened.
created_ means that the file (on disk) is a new file as opposed to just opening
an old file.

https://codereview.chromium.org/109273002/diff/60001/base/files/file_posix.cc
File base/files/file_posix.cc (right):

https://codereview.chromium.org/109273002/diff/60001/base/files/file_posix.cc...
base/files/file_posix.cc:350: }
On 2013/12/21 22:59:33, cpu wrote:
> ignorance: is seeking 3 times the way of the posix ? the internets say fstat.
> 
> nb: looks at us discussing posix, how quaint.

definitely my bad.

https://codereview.chromium.org/109273002/diff/60001/base/files/memory_mapped...
File base/files/memory_mapped_file_win.cc (left):

https://codereview.chromium.org/109273002/diff/60001/base/files/memory_mapped...
base/files/memory_mapped_file_win.cc:60:
UMA_HISTOGRAM_ENUMERATION("MemoryMappedFile.CreateFileMapping",
On 2013/12/21 22:59:33, cpu wrote:
> I so hope this two umas are gone gone, not just moved.

gone for good

https://codereview.chromium.org/109273002/diff/60001/chrome/app/chrome_main_d...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/109273002/diff/60001/chrome/app/chrome_main_d...
chrome/app/chrome_main_delegate.cc:671: false);
On 2013/12/21 23:06:28, cpu wrote:
> so locale_pak_fd will be closed at this point since the temporary File object
of
> line 670 went out of scope. Is that ok?

The file is effectively passed to ResourceBundle, and from there to DataPack who
uses it to initialize a MemoryMappedFile (it will not be closed when this line
goes out of scope).

What I have to verify with someone familiar with this code is that
base::GlobalDescriptors::GetInstance()->MaybeGet is OK with the file being
closed by someone else (it would be better to return File(s) instead)... but
this particular change is not modifying that behavior.

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

https://codereview.chromium.org/109273002/diff/60001/chrome/renderer/spellche...
chrome/renderer/spellchecker/hunspell_engine.cc:66: if
(bdict_file_->Initialize(base::File(file_))) {
On 2013/12/27 19:52:26, groby wrote:
> Probably want file_ to be base::File, for clearer ownership semantics. (Also,
> this fixes a file leak if InitializeHunspell is never called :)

That sounds correct. I didn't do it because it probably means changing Init (and
the callers) to pass File instead, and I don't know what would be the logic for
IsEnabled (cannot be file_.IsValid()). Besides, I'm trying to contain the scope
of this change.

Keeping the current type for file_ at least makes clear that this code has to be
updated.

[groby-ooo-7-16, 2013-12-27 23:57:37]

Might want to file a bug against spellcheck, then. (Also, removed myself from
reviewers - spellcheck LG to me)

[cpu_(ooo_6.6-7.5), 2013-12-28 01:40:52]

lgtm

ps: find out about GetInstance()->MaybeGet()

https://codereview.chromium.org/109273002/diff/180001/media/base/media_file_c...
File media/base/media_file_checker.h (left):

https://codereview.chromium.org/109273002/diff/180001/media/base/media_file_c...
media/base/media_file_checker.h:34: base::ScopedPlatformFileCloser file_closer_;
33 + 34 above look awful. Shame on us.

https://codereview.chromium.org/109273002/diff/180001/ui/base/resource/data_p...
File ui/base/resource/data_pack.cc (right):

https://codereview.chromium.org/109273002/diff/180001/ui/base/resource/data_p...
ui/base/resource/data_pack.cc:89: if (!mmap_->Initialize(file.Pass())) {
oh I had missed this .Pass()

[rvargas (doing something else), 2013-12-28 02:30:37]

+Dale for the media files (which I think had bugs)

+owners for the rest.

https://codereview.chromium.org/109273002/diff/60001/chrome/app/chrome_main_d...
File chrome/app/chrome_main_delegate.cc (right):

https://codereview.chromium.org/109273002/diff/60001/chrome/app/chrome_main_d...
chrome/app/chrome_main_delegate.cc:671: false);
Filed bug 330930, because GlobalDescriptors doesn't consider that case.

[tony, 2014-01-01 02:34:31]

ui/base/resource LGTM

[DaleCurtis, 2014-01-02 18:38:52]

media/ lgtm.

That code is only used by the Media Galleries API, which I'm not sure who is
using: http://developer.chrome.com/apps/mediaGalleries.html

cc: vandebo who wrote the original code.

[jochen (gone - plz use gerrit), 2014-01-03 11:37:33]

lgtm

[commit-bot: I haz the power, 2014-01-03 18:56:44]

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

[commit-bot: I haz the power, 2014-01-03 19:13:19]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Nico, 2014-01-03 19:14:30]

lgtm

[commit-bot: I haz the power, 2014-01-03 19:14:46]

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

[commit-bot: I haz the power, 2014-01-03 22:14:17]

Change committed as 242937