Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(724)

Issue 13032002: Add RequestOSFileHandle as a private PPAPI (Closed)

Created:
7 years, 9 months ago by hamaji
Modified:
7 years, 8 months ago
CC:
chromium-reviews, yusukes+watch_chromium.org, jam, yzshen+watch_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, raymes+watch_chromium.org, piman+watch_chromium.org, ihf+watch_chromium.org, Yusuke Sato, dmichael (off chromium), bbudge, bsy_cr, Avi (use Gerrit), kinuko
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

Unlike GetOSFileDescriptor, this API is asynchronous. Both GetOSFileDescriptor and RequestOSFileHandle use GetOSFileDescriptor chrome IPC for now. I'm planning to remove call sites of GetOSFileDescriptor PPAPI and rename GetOSFileDescriptor chrome IPC to RequestOSFileHandle. - Add --allow-get-os-file-handle-api. With this flag, 1. browser_tests can test this API and 2. we can use this API even before this issue is resolved: http://crbug.com/224123 - Add TestRequestOSFileHandle in FileIO. This checks if read, write, lseek, and mmap work for FD fetched by this API. - PepperFileIOHost::OnHostMsgGetOSFileDescriptor use ShareHandleWithRemote to pass a file handle - Fix ShareHandleWithRemote for in-process API BUG=183015 TEST=trybots, browser_tests Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=191616

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : win build fix #

Total comments: 15

Patch Set 5 : WIP build check #

Patch Set 6 : WIP build check #

Patch Set 7 : WIP #

Patch Set 8 : #

Patch Set 9 : addressed comments #

Total comments: 36

Patch Set 10 : addressed comment #

Patch Set 11 : #

Patch Set 12 : forgot to update the whitelist #

Patch Set 13 : remove NOTREACHED #

Total comments: 4

Patch Set 14 : #

Total comments: 2

Patch Set 15 : Per kinuko's suggestion #

Patch Set 16 : Remove unnecessary #include #

Total comments: 2

Patch Set 17 : add a comment #

Patch Set 18 : add a TODO comment #

Patch Set 19 : have two IPCs #

Patch Set 20 : fix test #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+588 lines, -19 lines) Patch
M chrome/browser/chrome_content_browser_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/common/chrome_switches.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +7 lines, -2 lines 0 comments Download
M chrome/renderer/chrome_content_renderer_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +10 lines, -0 lines 0 comments Download
M chrome/renderer/chrome_content_renderer_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +38 lines, -0 lines 0 comments Download
M chrome/renderer/chrome_content_renderer_client_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +36 lines, -1 line 0 comments Download
M chrome/test/ppapi/ppapi_browsertest.cc View 1 2 3 4 5 6 7 8 9 5 chunks +5 lines, -0 lines 0 comments Download
M chrome/test/ppapi/ppapi_test.cc View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -0 lines 0 comments Download
M content/public/renderer/content_renderer_client.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +3 lines, -0 lines 0 comments Download
M content/public/renderer/content_renderer_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +5 lines, -0 lines 0 comments Download
M content/renderer/pepper/pepper_file_io_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +4 lines, -1 line 0 comments Download
M content/renderer/pepper/pepper_file_io_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 4 chunks +37 lines, -7 lines 1 comment Download
M content/renderer/pepper/renderer_ppapi_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 2 chunks +9 lines, -3 lines 0 comments Download
A ppapi/api/private/ppb_file_io_private.idl View 1 2 3 4 5 6 7 8 9 1 chunk +26 lines, -0 lines 0 comments Download
M ppapi/api/trusted/ppb_file_io_trusted.idl View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -0 lines 0 comments Download
M ppapi/c/pp_macros.h View 1 2 3 4 5 6 7 8 9 1 chunk +2 lines, -2 lines 0 comments Download
A ppapi/c/private/ppb_file_io_private.h View 1 2 3 4 5 6 7 8 9 1 chunk +49 lines, -0 lines 0 comments Download
M ppapi/c/trusted/ppb_file_io_trusted.h View 1 2 3 4 5 6 7 8 9 2 chunks +3 lines, -1 line 0 comments Download
A ppapi/cpp/private/file_io_private.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +42 lines, -0 lines 0 comments Download
A ppapi/cpp/private/file_io_private.cc View 1 2 3 4 5 6 7 8 9 1 chunk +39 lines, -0 lines 0 comments Download
M ppapi/native_client/src/untrusted/pnacl_irt_shim/pnacl_shim.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 6 chunks +23 lines, -0 lines 0 comments Download
M ppapi/ppapi_shared.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -0 lines 0 comments Download
M ppapi/ppapi_sources.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +2 lines, -0 lines 0 comments Download
M ppapi/proxy/file_io_resource.h View 1 2 3 4 5 6 7 8 9 4 chunks +8 lines, -1 line 0 comments Download
M ppapi/proxy/file_io_resource.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 2 chunks +40 lines, -1 line 0 comments Download
M ppapi/proxy/interface_list.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -0 lines 0 comments Download
M ppapi/proxy/ppapi_messages.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +2 lines, -0 lines 0 comments Download
M ppapi/tests/all_c_includes.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -0 lines 0 comments Download
M ppapi/tests/test_file_io.h View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M ppapi/tests/test_file_io.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 3 chunks +130 lines, -0 lines 0 comments Download
M ppapi/thunk/interfaces_ppb_private_no_permissions.h View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -0 lines 0 comments Download
M ppapi/thunk/ppb_file_io_api.h View 1 2 3 4 5 6 7 8 9 2 chunks +6 lines, -0 lines 0 comments Download
A ppapi/thunk/ppb_file_io_private_thunk.cc View 1 2 3 4 5 6 7 8 9 1 chunk +44 lines, -0 lines 0 comments Download
M webkit/plugins/ppapi/plugin_module.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 40 (0 generated)
hamaji
I've never worked for PPAPI (and this is my first meaningful CL in ~30 months!). ...
7 years, 9 months ago (2013-03-23 01:18:18 UTC) #1
yzshen1
Hi, Shinichiro. Thanks for working on this! Please see below for some high-level comments and ...
7 years, 9 months ago (2013-03-25 19:28:44 UTC) #2
hamaji
I think I followed your suggestions. One change I made without discussing with you is ...
7 years, 9 months ago (2013-03-27 08:48:10 UTC) #3
dmichael (off chromium)
https://codereview.chromium.org/13032002/diff/42001/content/renderer/DEPS File content/renderer/DEPS (right): https://codereview.chromium.org/13032002/diff/42001/content/renderer/DEPS#newcode5 content/renderer/DEPS:5: "+extensions/common/constants.h", You don't appear to be using this. Please ...
7 years, 9 months ago (2013-03-27 19:40:53 UTC) #4
victorhsieh
(Remove myself and leave the code review to experts.)
7 years, 9 months ago (2013-03-27 20:32:29 UTC) #5
yzshen1
Thanks Shinichiro! https://codereview.chromium.org/13032002/diff/19001/ppapi/api/private/ppb_file_io_private.idl File ppapi/api/private/ppb_file_io_private.idl (right): https://codereview.chromium.org/13032002/diff/19001/ppapi/api/private/ppb_file_io_private.idl#newcode16 ppapi/api/private/ppb_file_io_private.idl:16: * descriptor. The FileIO object must have ...
7 years, 9 months ago (2013-03-27 20:47:58 UTC) #6
yzshen
(I realized that I haven't answered this question) On Wed, Mar 27, 2013 at 1:48 ...
7 years, 9 months ago (2013-03-27 21:53:28 UTC) #7
hamaji
Adding cevans@ and joi@ as reviewer of this CL as they are suggested by git ...
7 years, 9 months ago (2013-03-28 00:17:28 UTC) #8
hamaji
https://codereview.chromium.org/13032002/diff/42001/content/renderer/pepper/pepper_file_io_host.cc File content/renderer/pepper/pepper_file_io_host.cc (right): https://codereview.chromium.org/13032002/diff/42001/content/renderer/pepper/pepper_file_io_host.cc#newcode466 content/renderer/pepper/pepper_file_io_host.cc:466: DCHECK(file_system_url_.SchemeIsFileSystem()); On 2013/03/28 00:17:29, hamaji wrote: > On 2013/03/27 ...
7 years, 9 months ago (2013-03-28 00:42:56 UTC) #9
cevans
[+aedla] Jüri, would you be interested in looking at this? It's in an area where ...
7 years, 9 months ago (2013-03-28 01:18:03 UTC) #10
yzshen1
LGTM with a few comments. Thanks for the nice work! https://codereview.chromium.org/13032002/diff/42001/content/public/common/content_switches.cc File content/public/common/content_switches.cc (right): https://codereview.chromium.org/13032002/diff/42001/content/public/common/content_switches.cc#newcode15 ...
7 years, 9 months ago (2013-03-28 20:59:49 UTC) #11
hamaji
https://codereview.chromium.org/13032002/diff/42001/content/renderer/pepper/renderer_ppapi_host_impl.cc File content/renderer/pepper/renderer_ppapi_host_impl.cc (right): https://codereview.chromium.org/13032002/diff/42001/content/renderer/pepper/renderer_ppapi_host_impl.cc#newcode250 content/renderer/pepper/renderer_ppapi_host_impl.cc:250: return BrokerGetFileHandleForProcess(handle, On 2013/03/28 20:59:49, yzshen1 wrote: > I ...
7 years, 9 months ago (2013-03-28 21:37:15 UTC) #12
hamaji
A few more info for non pepper OWNERs: cevans and aedla (ppapi/proxy/ppapi_messages.h): Now we use ...
7 years, 9 months ago (2013-03-28 21:50:20 UTC) #13
hamaji
Adding avi as joi seems to be OOO. Avi, could you review this? I need ...
7 years, 8 months ago (2013-03-29 15:17:42 UTC) #14
Avi (use Gerrit)
Code lgtm. John wants to look at all content API changes; adding him as a ...
7 years, 8 months ago (2013-03-29 15:32:32 UTC) #15
hamaji
On 2013/03/29 15:32:32, Avi wrote: > Code lgtm. John wants to look at all content ...
7 years, 8 months ago (2013-03-29 16:44:37 UTC) #16
yzshen
On Fri, Mar 29, 2013 at 9:44 AM, <hamaji@chromium.org> wrote: > On 2013/03/29 15:32:32, Avi ...
7 years, 8 months ago (2013-03-29 16:47:48 UTC) #17
yzshen
Forgot to add: That is especially true for IPC changes, because security team owns *_messages.h ...
7 years, 8 months ago (2013-03-29 16:49:57 UTC) #18
jam
https://codereview.chromium.org/13032002/diff/92001/content/public/common/content_switches.cc File content/public/common/content_switches.cc (right): https://codereview.chromium.org/13032002/diff/92001/content/public/common/content_switches.cc#newcode13 content/public/common/content_switches.cc:13: // Specifies command-separated list of extension ids or hosts ...
7 years, 8 months ago (2013-03-29 16:52:22 UTC) #19
hamaji
Ah, I thought the security review is something like one in google3. We've already had ...
7 years, 8 months ago (2013-03-29 17:13:59 UTC) #20
kinuko
https://codereview.chromium.org/13032002/diff/92001/content/renderer/pepper/pepper_file_io_host.cc File content/renderer/pepper/pepper_file_io_host.cc (right): https://codereview.chromium.org/13032002/diff/92001/content/renderer/pepper/pepper_file_io_host.cc#newcode462 content/renderer/pepper/pepper_file_io_host.cc:462: bool PepperFileIOHost::IsGetOSFileDescriptorAllowed() const { I guess this part is ...
7 years, 8 months ago (2013-03-29 17:17:13 UTC) #21
kinuko
On 2013/03/29 17:17:13, kinuko wrote: > https://codereview.chromium.org/13032002/diff/92001/content/renderer/pepper/pepper_file_io_host.cc > File content/renderer/pepper/pepper_file_io_host.cc (right): > > https://codereview.chromium.org/13032002/diff/92001/content/renderer/pepper/pepper_file_io_host.cc#newcode462 > ...
7 years, 8 months ago (2013-03-29 17:19:28 UTC) #22
hamaji
Hi, I've updated this CL following kinuko's suggestion as it sounds like a right direction ...
7 years, 8 months ago (2013-03-29 18:26:03 UTC) #23
jam
lgtm, thanks https://codereview.chromium.org/13032002/diff/65001/content/renderer/pepper/pepper_file_io_host.cc File content/renderer/pepper/pepper_file_io_host.cc (right): https://codereview.chromium.org/13032002/diff/65001/content/renderer/pepper/pepper_file_io_host.cc#newcode476 content/renderer/pepper/pepper_file_io_host.cc:476: if (url.SchemeIs("chrome-extension")) { I realize this code ...
7 years, 8 months ago (2013-03-29 19:57:20 UTC) #24
hamaji
https://codereview.chromium.org/13032002/diff/65001/content/renderer/pepper/pepper_file_io_host.cc File content/renderer/pepper/pepper_file_io_host.cc (right): https://codereview.chromium.org/13032002/diff/65001/content/renderer/pepper/pepper_file_io_host.cc#newcode476 content/renderer/pepper/pepper_file_io_host.cc:476: if (url.SchemeIs("chrome-extension")) { I see. Thanks! On 2013/03/29 19:57:20, ...
7 years, 8 months ago (2013-03-29 20:06:50 UTC) #25
Chris Evans
On 2013/03/29 20:06:50, hamaji wrote: > https://codereview.chromium.org/13032002/diff/65001/content/renderer/pepper/pepper_file_io_host.cc > File content/renderer/pepper/pepper_file_io_host.cc (right): > > https://codereview.chromium.org/13032002/diff/65001/content/renderer/pepper/pepper_file_io_host.cc#newcode476 > ...
7 years, 8 months ago (2013-03-29 21:54:00 UTC) #26
hamaji
On 2013/03/29 21:54:00, Chris Evans wrote: > On 2013/03/29 20:06:50, hamaji wrote: > > > ...
7 years, 8 months ago (2013-03-29 22:27:36 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hamaji@chromium.org/13032002/113001
7 years, 8 months ago (2013-03-29 22:41:14 UTC) #28
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) nacl_integration http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=129373
7 years, 8 months ago (2013-03-30 05:09:58 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hamaji@chromium.org/13032002/113001
7 years, 8 months ago (2013-03-30 06:35:42 UTC) #30
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) nacl_integration http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=129397
7 years, 8 months ago (2013-03-30 09:05:30 UTC) #31
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hamaji@chromium.org/13032002/113001
7 years, 8 months ago (2013-03-30 17:00:33 UTC) #32
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) nacl_integration http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=129415
7 years, 8 months ago (2013-03-30 20:36:54 UTC) #33
hamaji
A single test in nacl_integration was always failing on windows. I made several guesses but ...
7 years, 8 months ago (2013-03-31 11:09:09 UTC) #34
hamaji
On 2013/03/31 11:09:09, hamaji wrote: > A single test in nacl_integration was always failing on ...
7 years, 8 months ago (2013-03-31 11:09:45 UTC) #35
aedla
> > I'll try eliminating GetOSFileDescriptor after this CL is landed. So if I understand ...
7 years, 8 months ago (2013-04-01 09:27:28 UTC) #36
hamaji
Thanks for the review! On 2013/04/01 09:27:28, aedla wrote: > > > I'll try eliminating ...
7 years, 8 months ago (2013-04-01 15:28:45 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/hamaji@chromium.org/13032002/98003
7 years, 8 months ago (2013-04-01 15:29:19 UTC) #38
commit-bot: I haz the power
Change committed as 191616
7 years, 8 months ago (2013-04-01 16:51:15 UTC) #39
bsy
7 years, 8 months ago (2013-04-01 17:17:44 UTC) #40
Message was sent while issue was closed.
https://codereview.chromium.org/13032002/diff/98003/content/renderer/pepper/p...
File content/renderer/pepper/pepper_file_io_host.cc (right):

https://codereview.chromium.org/13032002/diff/98003/content/renderer/pepper/p...
content/renderer/pepper/pepper_file_io_host.cc:474:
ppapi::proxy::SerializedHandle::FILE, file));
is the file opened with GENERIC_EXEC on windows?  this is needed for
CreateFileMapping / MapViewOfFile with PAGE_EXECUTE_* and FILE_MAP_EXECUTE.

Powered by Google App Engine
This is Rietveld 408576698