Add the descriptor support needed for Pepper in a way that does...

Add the descriptor support needed for Pepper in a way that does
not include chrome/common or base/ files from the NaCl tree.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=41864

[sehr (please use chromium), 2010-03-17 04:43:49]

Here's a first attempt at addressing the file inclusion issue.

[darin (slow to review), 2010-03-17 07:17:33]

Looks good.  I just have nits:

http://codereview.chromium.org/969006/diff/1/3
File chrome/renderer/nacl_desc_wrapper_chrome.cc (right):

http://codereview.chromium.org/969006/diff/1/3#newcode16
chrome/renderer/nacl_desc_wrapper_chrome.cc:16: base::SharedMemory* shm =
reinterpret_cast<base::SharedMemory*>(shm_int);
nit: indent by 2 spaces

http://codereview.chromium.org/969006/diff/1/3#newcode47
chrome/renderer/nacl_desc_wrapper_chrome.cc:47: ss_desc =
reinterpret_cast<NaClDescSyncSocket*>(
nit: technically this could be a static_cast

http://codereview.chromium.org/969006/diff/1/3#newcode53
chrome/renderer/nacl_desc_wrapper_chrome.cc:53: delete ss_desc;
shouldn't you use free instead of delete here to match calloc?  does
NaClDescSyncSocketCtor require memory to be initialized to zeros?

http://codereview.chromium.org/969006/diff/1/3#newcode57
chrome/renderer/nacl_desc_wrapper_chrome.cc:57: wrapper = new(std::nothrow)
nit: it is not necessary to specify nothrow in chromium code

http://codereview.chromium.org/969006/diff/1/3#newcode58
chrome/renderer/nacl_desc_wrapper_chrome.cc:58: DescWrapper(common_data_,
reinterpret_cast<struct NaClDesc*>(ss_desc));
nit: &ss_desc->base could be used here as well, right?  or, is it more
conventional to use the reinterpret_cast?

http://codereview.chromium.org/969006/diff/1/3#newcode66
chrome/renderer/nacl_desc_wrapper_chrome.cc:66:
NaClDescSafeUnref(reinterpret_cast<struct NaClDesc*>(ss_desc));
ditto: &ss_desc->base

note: i'm not a big fan of using gotos for cleanup, so if we are going to have a
lot of code in chromium that uses nacl, perhaps it would be wise to create a
variant of scoped_ptr_malloc that calls NaClDescSafeUnref automatically.

[sehr (please use chromium), 2010-03-17 17:20:08]

Darin,

Thanks for the review.  New version uploaded, awaiting trybots for submission.

David

http://codereview.chromium.org/969006/diff/1/3
File chrome/renderer/nacl_desc_wrapper_chrome.cc (right):

http://codereview.chromium.org/969006/diff/1/3#newcode16
chrome/renderer/nacl_desc_wrapper_chrome.cc:16: base::SharedMemory* shm =
reinterpret_cast<base::SharedMemory*>(shm_int);
On 2010/03/17 07:17:33, darin wrote:
> nit: indent by 2 spaces

Done.

http://codereview.chromium.org/969006/diff/1/3#newcode47
chrome/renderer/nacl_desc_wrapper_chrome.cc:47: ss_desc =
reinterpret_cast<NaClDescSyncSocket*>(
On 2010/03/17 07:17:33, darin wrote:
> nit: technically this could be a static_cast

It used to be a new call, and this was missed in code review before.  Thanks. 
Done.

http://codereview.chromium.org/969006/diff/1/3#newcode53
chrome/renderer/nacl_desc_wrapper_chrome.cc:53: delete ss_desc;
On 2010/03/17 07:17:33, darin wrote:
> shouldn't you use free instead of delete here to match calloc?  does
> NaClDescSyncSocketCtor require memory to be initialized to zeros?

Ugh.  Also missed during prior code review.  Done.

http://codereview.chromium.org/969006/diff/1/3#newcode57
chrome/renderer/nacl_desc_wrapper_chrome.cc:57: wrapper = new(std::nothrow)
On 2010/03/17 07:17:33, darin wrote:
> nit: it is not necessary to specify nothrow in chromium code

NaCl coding conventions v. Chrome.  Done.

http://codereview.chromium.org/969006/diff/1/3#newcode58
chrome/renderer/nacl_desc_wrapper_chrome.cc:58: DescWrapper(common_data_,
reinterpret_cast<struct NaClDesc*>(ss_desc));
On 2010/03/17 07:17:33, darin wrote:
> nit: &ss_desc->base could be used here as well, right?  or, is it more
> conventional to use the reinterpret_cast?

Done.

http://codereview.chromium.org/969006/diff/1/3#newcode66
chrome/renderer/nacl_desc_wrapper_chrome.cc:66:
NaClDescSafeUnref(reinterpret_cast<struct NaClDesc*>(ss_desc));
On 2010/03/17 07:17:33, darin wrote:
> ditto: &ss_desc->base
> 
> note: i'm not a big fan of using gotos for cleanup, so if we are going to have
a
> lot of code in chromium that uses nacl, perhaps it would be wise to create a
> variant of scoped_ptr_malloc that calls NaClDescSafeUnref automatically.

I think the scoped_ptr variant should go in the header file, which resides in
the native client repository.  I have added a TODO and will circle back along
with the nacl inclusions of base/basictypes.h.

[darin (slow to review), 2010-03-17 17:32:52]

LGTM

[appamatto, 2010-03-20 10:36:23]

This appears to break the disable_nacl=1 build, because
nacl_desc_wrapper_chrome.cc is unconditionally included in the sources list.