PPAPI: Implement PPB_FileMapping on POSIX

PPAPI: Implement PPB_FileMapping on POSIX

BUG=83774
R=bbudge@chromium.org

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

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

[dmichael (off chromium), 2014-01-23 20:19:18]

I think this is pretty much ready for review. Wasn't sure of a good way to break
it down further.

Note the API change CL hasn't landed yet, but the intent is there will be no
changes to those files here, so you can ignore those parts.

[teravest, 2014-01-23 21:40:57]

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
File ppapi/proxy/file_mapping_resource.cc (right):

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
boring nit: 2014 :(

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:7: #if defined(OS_POSIX)
It would be cool if there was an abstraction you could use for DoMap and DoUnmap
that put logic for different platforms in different files (like we have for ipc
channels or shared memory).

I understand if you don't want to bother with that, though.

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:78: return PP_ERROR_NOMEMORY;
I'd argue that this should be BADARGUMENT. But either way is ok I guess.

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:217: // TODO(dmichael): If the callback is
aborted, should we unmap?
I think we should unmap when the callback is aborted.

When the callback isn't invoked, the user won't have an address to use for
calling Unmap, so we should unmap in that case to prevent leaks.

https://codereview.chromium.org/69663002/diff/310001/ppapi/thunk/enter.cc
File ppapi/thunk/enter.cc (left):

https://codereview.chromium.org/69663002/diff/310001/ppapi/thunk/enter.cc#old...
ppapi/thunk/enter.cc:62: DCHECK(resource_ || !instance);
Why is this removed?

[bbudge, 2014-01-23 23:01:02]

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
File ppapi/proxy/file_mapping_resource.cc (right):

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:74: return PP_ERROR_BADARGUMENT;
Kind of a gray area, but this could also be PP_ERROR_FAILED.

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:162: NOTREACHED();
NOTIMPLEMENTED()?

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:203: map_result.result =
ErrnoToPPError(errno);
nit: Seems like you could access this inside ErrnoToPPError rather than at call
sites.

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:220: if (map_result.result == 0)
Since this is logically a PP_Error, PP_OK?

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
File ppapi/proxy/file_mapping_resource.h (right):

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
2014

https://codereview.chromium.org/69663002/diff/310001/ppapi/thunk/ppb_file_io_...
File ppapi/thunk/ppb_file_io_dev_thunk.cc (right):

https://codereview.chromium.org/69663002/diff/310001/ppapi/thunk/ppb_file_io_...
ppapi/thunk/ppb_file_io_dev_thunk.cc:5: // From dev/ppb_file_io_dev.idl modified
Thu Oct 17 12:25:57 2013.
Does this file even belong in the patch?

https://codereview.chromium.org/69663002/diff/310001/ppapi/thunk/ppb_file_map...
File ppapi/thunk/ppb_file_mapping_api.h (right):

https://codereview.chromium.org/69663002/diff/310001/ppapi/thunk/ppb_file_map...
ppapi/thunk/ppb_file_mapping_api.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
2014

[dmichael (off chromium), 2014-01-24 20:02:49]

Thanks, guys! Comments addressed.

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
File ppapi/proxy/file_mapping_resource.cc (right):

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2014/01/23 21:40:58, teravest wrote:
> boring nit: 2014 :(

Done.

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:7: #if defined(OS_POSIX)
On 2014/01/23 21:40:58, teravest wrote:
> It would be cool if there was an abstraction you could use for DoMap and
DoUnmap
> that put logic for different platforms in different files (like we have for
ipc
> channels or shared memory).
> 
> I understand if you don't want to bother with that, though.
Yeah...  I had already considered it, and was thinking maybe for next CL. But I
went ahead and did it this time. I think it's cleaner split up. Also did it for
GetMapPageSize, just for consistency.

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:74: return PP_ERROR_BADARGUMENT;
On 2014/01/23 23:01:03, bbudge wrote:
> Kind of a gray area, but this could also be PP_ERROR_FAILED.
I think of PP_ERROR_FAILED as a more generic, "we screwed up but don't know what
happened". I think this can happen if you pass a FileIO that's either not been
opened or has been closed, so that's why I was thinking BADARGUMENT. WDYT?

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:78: return PP_ERROR_NOMEMORY;
On 2014/01/23 21:40:58, teravest wrote:
> I'd argue that this should be BADARGUMENT. But either way is ok I guess.
This is for platform consistency. On 64-bit, values over 2^32 will still fit in
size_t, so we would pass it along to mmap, which will usually result in
PP_ERROR_NOMEMORY. So this check comes in to play on 32-bit, and I wanted it to
have the same failure code there. (This came up in my testing, where I pass
numeric_limits<int64_t>::max()).

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:162: NOTREACHED();
On 2014/01/23 23:01:03, bbudge wrote:
> NOTIMPLEMENTED()?
Oops, the other Windows functions just return PP_ERROR_NOTSUPPORTED without the
check...  so I just removed NOTREACHED here for consistency.

But if you think I should have the NOTIMPLEMENTED in all of these, I'm happy to
do that. I suspect it won't make a lot of difference either way.

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:203: map_result.result =
ErrnoToPPError(errno);
On 2014/01/23 23:01:03, bbudge wrote:
> nit: Seems like you could access this inside ErrnoToPPError rather than at
call
> sites.
Yeah...  I thought about that, but I already hate that it's a global variable,
and I thought this was a little cleaner/more flexible than relying on the global
state. E.g., you could stash an errno value to call it with at a later time
(though admittedly I don't use that flexibility).

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:217: // TODO(dmichael): If the callback is
aborted, should we unmap?
On 2014/01/23 21:40:58, teravest wrote:
> I think we should unmap when the callback is aborted.
> 
> When the callback isn't invoked, the user won't have an address to use for
> calling Unmap, so we should unmap in that case to prevent leaks.
Good point... they do get a PP_ERROR_ABORTED callback, but I wasn't thinking
about the fact that they don't get an address.

Done.

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:220: if (map_result.result == 0)
On 2014/01/23 23:01:03, bbudge wrote:
> Since this is logically a PP_Error, PP_OK?

Done.

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
File ppapi/proxy/file_mapping_resource.h (right):

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2014/01/23 23:01:03, bbudge wrote:
> 2014

Done.

https://codereview.chromium.org/69663002/diff/310001/ppapi/thunk/enter.cc
File ppapi/thunk/enter.cc (left):

https://codereview.chromium.org/69663002/diff/310001/ppapi/thunk/enter.cc#old...
ppapi/thunk/enter.cc:62: DCHECK(resource_ || !instance);
On 2014/01/23 21:40:58, teravest wrote:
> Why is this removed?
This case happens for singleton resources if you pass a bad resource, we just
had never tested that before. See the "Bad instance" part of my test; I pass
"PP_Instance(0xbadbad)", and expect to get a PP_ERROR_BADARGUMENT when the
instance can't be found. Instead, it was crashing in Debug because of this
DCHECK. (GetSingletonResource returns a NULL resource_, and instance is
non-zero).

https://codereview.chromium.org/69663002/diff/310001/ppapi/thunk/ppb_file_io_...
File ppapi/thunk/ppb_file_io_dev_thunk.cc (right):

https://codereview.chromium.org/69663002/diff/310001/ppapi/thunk/ppb_file_io_...
ppapi/thunk/ppb_file_io_dev_thunk.cc:5: // From dev/ppb_file_io_dev.idl modified
Thu Oct 17 12:25:57 2013.
On 2014/01/23 23:01:03, bbudge wrote:
> Does this file even belong in the patch?
No! Good catch.

https://codereview.chromium.org/69663002/diff/310001/ppapi/thunk/ppb_file_map...
File ppapi/thunk/ppb_file_mapping_api.h (right):

https://codereview.chromium.org/69663002/diff/310001/ppapi/thunk/ppb_file_map...
ppapi/thunk/ppb_file_mapping_api.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2014/01/23 23:01:03, bbudge wrote:
> 2014

Done.

[bbudge, 2014-01-24 21:41:21]

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
File ppapi/proxy/file_mapping_resource.cc (right):

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:74: return PP_ERROR_BADARGUMENT;
On 2014/01/24 20:02:50, dmichael wrote:
> On 2014/01/23 23:01:03, bbudge wrote:
> > Kind of a gray area, but this could also be PP_ERROR_FAILED.
> I think of PP_ERROR_FAILED as a more generic, "we screwed up but don't know
what
> happened". I think this can happen if you pass a FileIO that's either not been
> opened or has been closed, so that's why I was thinking BADARGUMENT. WDYT?

In retrospect, we should probably have defined a PP_ERROR_FILENOTOPEN.

Returning PP_ERROR_FAILED would be consistent with FileIO - we return it for any
operation when the file isn't open. It might be better - I don't know under what
conditions map/unmap might fail. If that is unusual then this might be better.

PP_ERROR_BADARGUMENT is also a reasonable choice here. I leave it to you.

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:103: base::Callback<MapResult()> map_op(
Maybe a better name is map_task or map_cb?
Same for unmap_op below.

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:162: NOTREACHED();
On 2014/01/24 20:02:50, dmichael wrote:
> On 2014/01/23 23:01:03, bbudge wrote:
> > NOTIMPLEMENTED()?
> Oops, the other Windows functions just return PP_ERROR_NOTSUPPORTED without
the
> check...  so I just removed NOTREACHED here for consistency.
> 
> But if you think I should have the NOTIMPLEMENTED in all of these, I'm happy
to
> do that. I suspect it won't make a lot of difference either way.

No big deal.

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:203: map_result.result =
ErrnoToPPError(errno);
On 2014/01/24 20:02:50, dmichael wrote:
> On 2014/01/23 23:01:03, bbudge wrote:
> > nit: Seems like you could access this inside ErrnoToPPError rather than at
> call
> > sites.
> Yeah...  I thought about that, but I already hate that it's a global variable,
> and I thought this was a little cleaner/more flexible than relying on the
global
> state. E.g., you could stash an errno value to call it with at a later time
> (though admittedly I don't use that flexibility).

OK

[dmichael (off chromium), 2014-01-24 22:05:12]

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
File ppapi/proxy/file_mapping_resource.cc (right):

https://codereview.chromium.org/69663002/diff/310001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:74: return PP_ERROR_BADARGUMENT;
On 2014/01/24 21:41:21, bbudge wrote:
> On 2014/01/24 20:02:50, dmichael wrote:
> > On 2014/01/23 23:01:03, bbudge wrote:
> > > Kind of a gray area, but this could also be PP_ERROR_FAILED.
> > I think of PP_ERROR_FAILED as a more generic, "we screwed up but don't know
> what
> > happened". I think this can happen if you pass a FileIO that's either not
been
> > opened or has been closed, so that's why I was thinking BADARGUMENT. WDYT?
> 
> In retrospect, we should probably have defined a PP_ERROR_FILENOTOPEN.
> 
> Returning PP_ERROR_FAILED would be consistent with FileIO - we return it for
any
> operation when the file isn't open.
Okay, I'm convinced. Done.

[bbudge, 2014-01-25 01:04:54]

https://codereview.chromium.org/69663002/diff/550001/ppapi/proxy/file_mapping...
File ppapi/proxy/file_mapping_resource.cc (right):

https://codereview.chromium.org/69663002/diff/550001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:56: // Ensure no bits that we don't
recognize are set.
This is hard for me to read. How about
// Ensure bits we don't recognize are zero.

https://codereview.chromium.org/69663002/diff/550001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:77: base::Callback<MapResult()> map_op(
The _op suffix seems to be a carryover from FileIOResource. Maybe _cb or _task
would be better.
Same comment below in Unmap.

https://codereview.chromium.org/69663002/diff/550001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:140: // TODO(dmichael): If the callback is
aborted, should we unmap?
remove comment now.

https://codereview.chromium.org/69663002/diff/550001/ppapi/proxy/file_mapping...
File ppapi/proxy/file_mapping_resource.h (right):

https://codereview.chromium.org/69663002/diff/550001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.h:59: // file_mapping_resource_win.cc for the
implementations of the Do* functions.
Rework. How about this?

  // These functions perform potentially blocking operations so they should not
  // be called on the main thread or while we hold the proxy lock. Their
  // implementation is platform specific. See file_mapping_resource_posix.cc and
  // file_mapping_resource_win.cc.

Hmm, you might also append _Blocking to the function names.

https://codereview.chromium.org/69663002/diff/550001/ppapi/proxy/file_mapping...
File ppapi/proxy/file_mapping_resource_posix.cc (right):

https://codereview.chromium.org/69663002/diff/550001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource_posix.cc:77: int result =
munmap(const_cast<void*>(address), static_cast<size_t>(length));
Since we don't otherwise use result could it just be if (munmap... ?

https://codereview.chromium.org/69663002/diff/550001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource_posix.cc:85: // TODO(dmichael): It might be
better to port sys_info_posix.cc to NaCl and
That file just calls getpagesize() and the rest is really non-portable to NaCl.
So I would remove the TODO.

[dmichael (off chromium), 2014-01-27 18:29:52]

https://codereview.chromium.org/69663002/diff/550001/ppapi/proxy/file_mapping...
File ppapi/proxy/file_mapping_resource.cc (right):

https://codereview.chromium.org/69663002/diff/550001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:56: // Ensure no bits that we don't
recognize are set.
On 2014/01/25 01:04:55, bbudge wrote:
> This is hard for me to read. How about
> // Ensure bits we don't recognize are zero.

Done.

https://codereview.chromium.org/69663002/diff/550001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:77: base::Callback<MapResult()> map_op(
On 2014/01/25 01:04:55, bbudge wrote:
> The _op suffix seems to be a carryover from FileIOResource. Maybe _cb or _task
> would be better.
> Same comment below in Unmap.

Done.

https://codereview.chromium.org/69663002/diff/550001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.cc:140: // TODO(dmichael): If the callback is
aborted, should we unmap?
On 2014/01/25 01:04:55, bbudge wrote:
> remove comment now.

Done.

https://codereview.chromium.org/69663002/diff/550001/ppapi/proxy/file_mapping...
File ppapi/proxy/file_mapping_resource.h (right):

https://codereview.chromium.org/69663002/diff/550001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.h:59: // file_mapping_resource_win.cc for the
implementations of the Do* functions.
On 2014/01/25 01:04:55, bbudge wrote:
> Rework. How about this?
> 
>   // These functions perform potentially blocking operations so they should
not
>   // be called on the main thread or while we hold the proxy lock. Their
>   // implementation is platform specific. See file_mapping_resource_posix.cc
and
>   // file_mapping_resource_win.cc.
> 
> Hmm, you might also append _Blocking to the function names.
Thanks, that's much better. Only downside is I don't do anything special for
GetMapPageSize, so I added a little extra comment before it and didn't append
"Blocking" there. Let me know what you think.

https://codereview.chromium.org/69663002/diff/550001/ppapi/proxy/file_mapping...
File ppapi/proxy/file_mapping_resource_posix.cc (right):

https://codereview.chromium.org/69663002/diff/550001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource_posix.cc:77: int result =
munmap(const_cast<void*>(address), static_cast<size_t>(length));
On 2014/01/25 01:04:55, bbudge wrote:
> Since we don't otherwise use result could it just be if (munmap... ?

Done.

https://codereview.chromium.org/69663002/diff/550001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource_posix.cc:85: // TODO(dmichael): It might be
better to port sys_info_posix.cc to NaCl and
On 2014/01/25 01:04:55, bbudge wrote:
> That file just calls getpagesize() and the rest is really non-portable to
NaCl.
> So I would remove the TODO.

Done.

[bbudge, 2014-01-27 18:56:43]

Nice work. Couple of nits. LGTM

https://codereview.chromium.org/69663002/diff/840001/ppapi/proxy/file_mapping...
File ppapi/proxy/file_mapping_resource.h (right):

https://codereview.chromium.org/69663002/diff/840001/ppapi/proxy/file_mapping...
ppapi/proxy/file_mapping_resource.h:34: uint32_t map_flags,
nit: These could be named 'protection' and 'flags'. Any reason why you prepended
map_, since the function name provides that context?

https://codereview.chromium.org/69663002/diff/840001/ppapi/tests/test_file_ma...
File ppapi/tests/test_file_mapping.cc (right):

https://codereview.chromium.org/69663002/diff/840001/ppapi/tests/test_file_ma...
ppapi/tests/test_file_mapping.cc:144: // FIXME(dmichael): Make sure our Pepper
documentation reflects this.
// TODO?

https://codereview.chromium.org/69663002/diff/840001/ppapi/tests/test_file_ma...
ppapi/tests/test_file_mapping.cc:204: //  - Unmapping partial regions
What would you test that isn't already tested in PartialRegions?

https://codereview.chromium.org/69663002/diff/840001/ppapi/tests/test_file_ma...
ppapi/tests/test_file_mapping.cc:415: // FIXME(dmichael) How to test FIXED??
// TODO?

[dmichael (off chromium), 2014-01-27 19:20:26]

Thanks, Bill! Comments addressed.

https://codereview.chromium.org/69663002/diff/840001/ppapi/tests/test_file_ma...
File ppapi/tests/test_file_mapping.cc (right):

https://codereview.chromium.org/69663002/diff/840001/ppapi/tests/test_file_ma...
ppapi/tests/test_file_mapping.cc:144: // FIXME(dmichael): Make sure our Pepper
documentation reflects this.
On 2014/01/27 18:56:44, bbudge wrote:
> // TODO?

Done.

https://codereview.chromium.org/69663002/diff/840001/ppapi/tests/test_file_ma...
ppapi/tests/test_file_mapping.cc:204: //  - Unmapping partial regions
On 2014/01/27 18:56:44, bbudge wrote:
> What would you test that isn't already tested in PartialRegions?
Oops, that's an old comment, thanks :)

https://codereview.chromium.org/69663002/diff/840001/ppapi/tests/test_file_ma...
ppapi/tests/test_file_mapping.cc:415: // FIXME(dmichael) How to test FIXED??
On 2014/01/27 18:56:44, bbudge wrote:
> // TODO?
The PartialRegions test actually does a FIXED mapping that works on POSIX. So I
think this is another obsolete FIXME.

[commit-bot: I haz the power, 2014-01-27 19:35:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/69663002/890001

[commit-bot: I haz the power, 2014-01-27 19:35:39]

Failed to apply patch for chrome/test/ppapi/ppapi_browsertest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/test/ppapi/ppapi_browsertest.cc
  Hunk #1 FAILED at 958.
  1 out of 1 hunk FAILED -- saving rejects to file
chrome/test/ppapi/ppapi_browsertest.cc.rej

Patch:       chrome/test/ppapi/ppapi_browsertest.cc
Index: chrome/test/ppapi/ppapi_browsertest.cc
diff --git a/chrome/test/ppapi/ppapi_browsertest.cc
b/chrome/test/ppapi/ppapi_browsertest.cc
index
65cc3b88bdff5ffddc877c2de1b48276fd707f05..c52d5fe279fa9f8015052893c55d9b57356bc2ce
100644
--- a/chrome/test/ppapi/ppapi_browsertest.cc
+++ b/chrome/test/ppapi/ppapi_browsertest.cc
@@ -958,6 +958,16 @@ IN_PROC_BROWSER_TEST_F(PPAPIPrivateNaClPNaClTest,
MAYBE_PNaCl_FileIO_Private) {
   );
 }
 
+// PPB_FileMapping is only implemented on POSIX currently.
+// http://crbug.com/83774
+#if defined(OS_WIN)
+#define MAYBE_FileMapping DISABLED_FileMapping
+#else
+#define MAYBE_FileMapping FileMapping
+#endif
+TEST_PPAPI_OUT_OF_PROCESS_VIA_HTTP(MAYBE_FileMapping)
+TEST_PPAPI_NACL(MAYBE_FileMapping)
+
 // Note, the FileRef tests are split in to two, because all of them together
 // sometimes takes too long on windows: crbug.com/336999
 IN_PROC_BROWSER_TEST_F(PPAPITest, FileRef1) {

[commit-bot: I haz the power, 2014-01-27 21:30:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/69663002/910001

[commit-bot: I haz the power, 2014-01-27 23:57:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/69663002/910001

[commit-bot: I haz the power, 2014-01-28 00:22:26]

Commit queue rejected this change because the description was changed
between the time the change entered the commit queue and the time it
was ready to commit. You can safely check the commit box again.

[commit-bot: I haz the power, 2014-01-28 00:24:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/69663002/930001

[commit-bot: I haz the power, 2014-01-28 02:53:03]

Retried try job too often on win_rel for step(s) telemetry_perf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[dmichael (off chromium), 2014-01-28 18:36:31]

Committed patchset #15 manually as r247473 (presubmit successful).

[haitaol1, 2014-01-28 18:47:53]

A revert of this CL has been created in
https://codereview.chromium.org/131543007/ by haitaol@chromium.org.

The reason for reverting is: Potential cause for build failure:
http://build.chromium.org/p/chromium.linux/builders/Linux%20Builder%20%28dbg%....

[commit-bot: I haz the power, 2014-01-28 19:25:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/69663002/920027

[commit-bot: I haz the power, 2014-01-28 21:40:03]

Retried try job too often on mac_rel for step(s) remoting_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2014-01-28 23:06:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/69663002/920027

[commit-bot: I haz the power, 2014-01-29 00:50:09]

Change committed as 247546