Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1460133002:
Allow Mach shm to be passed to NaCl. (Closed)
|
Created:
5 years, 1 month ago by erikchen Modified:
5 years ago Reviewers:
Mark Seaborn CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAllow Mach shm to be passed to NaCl.
BUG=547246
Committed: https://crrev.com/3dc5fdfb9ab7fbc13b583e07a6ef56636e0ed806
Cr-Commit-Position: refs/heads/master@{#361148}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Comments from mseaborn. #Patch Set 3 : Typos. #Patch Set 4 : More compile errors. #
Total comments: 3
Messages
Total messages: 15 (4 generated)
erikchen@chromium.org changed reviewers: + mseaborn@chromium.org
mseaborn: Please review. Note that this is dependent on a NaCl roll that is currently stalled: https://codereview.chromium.org/1460063003/
https://codereview.chromium.org/1460133002/diff/1/components/nacl/loader/nacl... File components/nacl/loader/nacl_ipc_adapter.cc (right): https://codereview.chromium.org/1460133002/diff/1/components/nacl/loader/nacl... components/nacl/loader/nacl_ipc_adapter.cc:544: const base::SharedMemoryHandle& shm_handle = iter->shmem(); For readability, how about splitting out this conversion into a function like this one? -- NaClDescWrapper MakeShmNaClDesc(const base::SharedMemoryHandle& handle, uint32_t size); https://codereview.chromium.org/1460133002/diff/1/components/nacl/loader/nacl... components/nacl/loader/nacl_ipc_adapter.cc:546: #if defined(OS_MACOSX) && !defined(OS_IOS) This file shouldn't be getting compiled for iOS. It would be better to omit the check for iOS so as not to mislead readers about what this code supports. https://codereview.chromium.org/1460133002/diff/1/components/nacl/loader/nacl... components/nacl/loader/nacl_ipc_adapter.cc:554: nacl_desc.reset(new NaClDescWrapper(NaClDescImcShmMake( This is the same as the code in the #else, so you could reduce duplication with: #if defined(OS_MACOSX) if (shm_handle.GetType() == base::SharedMemoryHandle::MACH) { return new NaClDescWrapper(...); } CHECK_EQ(shm_handle.GetType(), base::SharedMemoryHandle::POSIX); #endif // Generic case...
mseaborn: PTAL https://codereview.chromium.org/1460133002/diff/1/components/nacl/loader/nacl... File components/nacl/loader/nacl_ipc_adapter.cc (right): https://codereview.chromium.org/1460133002/diff/1/components/nacl/loader/nacl... components/nacl/loader/nacl_ipc_adapter.cc:544: const base::SharedMemoryHandle& shm_handle = iter->shmem(); On 2015/11/20 18:55:54, Mark Seaborn wrote: > For readability, how about splitting out this conversion into a function like > this one? -- > > NaClDescWrapper MakeShmNaClDesc(const base::SharedMemoryHandle& handle, uint32_t > size); Done. https://codereview.chromium.org/1460133002/diff/1/components/nacl/loader/nacl... components/nacl/loader/nacl_ipc_adapter.cc:546: #if defined(OS_MACOSX) && !defined(OS_IOS) On 2015/11/20 18:55:54, Mark Seaborn wrote: > This file shouldn't be getting compiled for iOS. It would be better to omit the > check for iOS so as not to mislead readers about what this code supports. Done. https://codereview.chromium.org/1460133002/diff/1/components/nacl/loader/nacl... components/nacl/loader/nacl_ipc_adapter.cc:554: nacl_desc.reset(new NaClDescWrapper(NaClDescImcShmMake( On 2015/11/20 18:55:54, Mark Seaborn wrote: > This is the same as the code in the #else, so you could reduce duplication with: > > #if defined(OS_MACOSX) > if (shm_handle.GetType() == base::SharedMemoryHandle::MACH) { > return new NaClDescWrapper(...); > } > CHECK_EQ(shm_handle.GetType(), base::SharedMemoryHandle::POSIX); > #endif > // Generic case... Done.
LGTM, thanks https://codereview.chromium.org/1460133002/diff/60001/components/nacl/loader/... File components/nacl/loader/nacl_ipc_adapter.cc (right): https://codereview.chromium.org/1460133002/diff/60001/components/nacl/loader/... components/nacl/loader/nacl_ipc_adapter.cc:564: nacl_desc = Nit: the other cases do nacl_desc.reset() (though presumably it's equivalent)
https://codereview.chromium.org/1460133002/diff/60001/components/nacl/loader/... File components/nacl/loader/nacl_ipc_adapter.cc (right): https://codereview.chromium.org/1460133002/diff/60001/components/nacl/loader/... components/nacl/loader/nacl_ipc_adapter.cc:564: nacl_desc = On 2015/11/20 21:57:19, Mark Seaborn wrote: > Nit: the other cases do nacl_desc.reset() (though presumably it's equivalent) nacl_desc.reset() is being used on a raw pointer. We can use reset() here a well, but we'd need to call .Pass() or .release() on MakeShmNaClDesc() to convert the return value back to a raw pointer. Seems like additional effort for no benefit?
https://codereview.chromium.org/1460133002/diff/60001/components/nacl/loader/... File components/nacl/loader/nacl_ipc_adapter.cc (right): https://codereview.chromium.org/1460133002/diff/60001/components/nacl/loader/... components/nacl/loader/nacl_ipc_adapter.cc:564: nacl_desc = On 2015/11/20 22:00:46, erikchen wrote: > On 2015/11/20 21:57:19, Mark Seaborn wrote: > > Nit: the other cases do nacl_desc.reset() (though presumably it's equivalent) > > nacl_desc.reset() is being used on a raw pointer. We can use reset() here a > well, but we'd need to call .Pass() or .release() on MakeShmNaClDesc() to > convert the return value back to a raw pointer. Seems like additional effort for > no benefit? OK, please leave it as it is, then. I hadn't realised the difference.
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1460133002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1460133002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by erikchen@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1460133002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1460133002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3dc5fdfb9ab7fbc13b583e07a6ef56636e0ed806 Cr-Commit-Position: refs/heads/master@{#361148} |
