Issue 9677061: Pass dup'd handles to nacl::DescWrapper instead of bumping the ref count.

Issue 9677061: Pass dup'd handles to nacl::DescWrapper instead of bumping the ref count. (Closed)

Created:
8 years, 9 months ago by nfullagar

Modified:
8 years, 9 months ago

Reviewers:
sehr (please use chromium)

CC:
chromium-reviews

Base URL:
svn://svn.chromium.org/chrome/trunk/src/

Visibility:
Public.

More Reviews

Description

Pass dup'd handles to nacl::DescWrapper instead of bumping the ref count. In these cases, the original handle is owned outside of NaCl and shouldn't be closed by the NaCl Plugin. BUG=https://chromiumcodereview.appspot.com/9610008 TEST=ppapi_example_audio, ppb_image_data, ppb_graphics3d Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=127056

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : #

Patch Set 4 : #

Patch Set 5 : #

Patch Set 6 : #

Patch Set 7 : #

Patch Set 8 : #

Total comments: 10

Patch Set 9 : #

Patch Set 10 : #

Patch Set 11 : #

Patch Set 12 : #

Patch Set 13 : #

Created: 8 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+58 lines, -45 lines)			Patch
M	ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_audio_rpc_server.cc	View	1 2 3 4 5 6 7 8 9 10 11 12	2 chunks	+25 lines, -9 lines	0 comments	Download
M	ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_graphics_3d_rpc_server.cc	View	1 2 3 4 5 6 7 8 9 10 11 12	3 chunks	+17 lines, -11 lines	0 comments	Download
M	ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_image_data_rpc_server.cc	View	1 2 3 4 5 6 7 8 9 10 11 12	2 chunks	+16 lines, -25 lines	0 comments	Download

Messages

Total messages: 10 (0 generated)

Expand Messages | Collapse Messages

sehr (please use chromium)

A few style comments. http://codereview.chromium.org/9677061/diff/4007/ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_audio_rpc_server.cc File ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_audio_rpc_server.cc (right): http://codereview.chromium.org/9677061/diff/4007/ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_audio_rpc_server.cc#newcode86 ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_audio_rpc_server.cc:86: if (shared_memory_handle < 0) { ...

8 years, 9 months ago (2012-03-14 17:19:15 UTC) #2

nfullagar

thx, ptal https://chromiumcodereview.appspot.com/9677061/diff/4007/ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_audio_rpc_server.cc File ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_audio_rpc_server.cc (right): https://chromiumcodereview.appspot.com/9677061/diff/4007/ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_audio_rpc_server.cc#newcode86 ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_audio_rpc_server.cc:86: if (shared_memory_handle < 0) { On 2012/03/14 ...

8 years, 9 months ago (2012-03-14 18:36:28 UTC) #3

nfullagar

ugh. tweaking the casts to make windows trybot happy. On Wed, Mar 14, 2012 at ...

8 years, 9 months ago (2012-03-14 20:57:45 UTC) #4

ugh.  tweaking the casts to make windows trybot happy.

On Wed, Mar 14, 2012 at 11:36 AM, <nfullagar@google.com> wrote:

> thx, ptal
>
>
> https://chromiumcodereview.**appspot.com/9677061/diff/4007/**
> ppapi/native_client/src/**shared/ppapi_proxy/browser_**
>
ppb_audio_rpc_server.cc<https://chromiumcodereview.appspot.com/9677061/diff/4007/ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_audio_rpc_server.cc>
> File
> ppapi/native_client/src/**shared/ppapi_proxy/browser_**
> ppb_audio_rpc_server.cc
> (right):
>
> https://chromiumcodereview.**appspot.com/9677061/diff/4007/**
> ppapi/native_client/src/**shared/ppapi_proxy/browser_**
>
ppb_audio_rpc_server.cc#**newcode86<https://chromiumcodereview.appspot.com/9677061/diff/4007/ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_audio_rpc_server.cc#newcode86>
>
> ppapi/native_client/src/**shared/ppapi_proxy/browser_**
> ppb_audio_rpc_server.cc:86:
> if (shared_memory_handle < 0) {
> On 2012/03/14 17:19:16, sehr wrote:
>
>> == nacl::kInvalidHandle here and below?
>>
>
> Done.
>
> https://chromiumcodereview.**appspot.com/9677061/diff/4007/**
> ppapi/native_client/src/**shared/ppapi_proxy/browser_**
>
ppb_graphics_3d_rpc_server.cc<https://chromiumcodereview.appspot.com/9677061/diff/4007/ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_graphics_3d_rpc_server.cc>
> File
> ppapi/native_client/src/**shared/ppapi_proxy/browser_**
> ppb_graphics_3d_rpc_server.cc
> (right):
>
> https://chromiumcodereview.**appspot.com/9677061/diff/4007/**
> ppapi/native_client/src/**shared/ppapi_proxy/browser_**
>
ppb_graphics_3d_rpc_server.cc#**newcode389<https://chromiumcodereview.appspot.com/9677061/diff/4007/ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_graphics_3d_rpc_server.cc#newcode389>
>
> ppapi/native_client/src/**shared/ppapi_proxy/browser_**
> ppb_graphics_3d_rpc_server.cc:**389:
> if (!transfer_buffer_aquired || native_handle < 0) {
> On 2012/03/14 17:19:16, sehr wrote:
>
>> again,
>>
>
>  native_handle == nacl::kInvalidHandle
>>
>
> Done.
>
> https://chromiumcodereview.**appspot.com/9677061/diff/4007/**
> ppapi/native_client/src/**shared/ppapi_proxy/browser_**
>
ppb_graphics_3d_rpc_server.cc#**newcode392<https://chromiumcodereview.appspot.com/9677061/diff/4007/ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_graphics_3d_rpc_server.cc#newcode392>
>
> ppapi/native_client/src/**shared/ppapi_proxy/browser_**
> ppb_graphics_3d_rpc_server.cc:**392:
> NaClHandle nacl_handle = (NaClHandle)native_handle;
> On 2012/03/14 17:19:16, sehr wrote:
>
>> static_cast<NaClHandle>>(...)
>>
>
> The c-style cast (from before) was required due to gcc & msvc not
> agreeing on whether this should be a static_cast vs. a reinterpret_cast.
>  The c-style cast satisfies them both without having to do #if ... #else
> ... #endif
>
> https://chromiumcodereview.**appspot.com/9677061/diff/4007/**
> ppapi/native_client/src/**shared/ppapi_proxy/browser_**
>
ppb_image_data_rpc_server.cc<https://chromiumcodereview.appspot.com/9677061/diff/4007/ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_image_data_rpc_server.cc>
> File
> ppapi/native_client/src/**shared/ppapi_proxy/browser_**
> ppb_image_data_rpc_server.cc
> (right):
>
> https://chromiumcodereview.**appspot.com/9677061/diff/4007/**
> ppapi/native_client/src/**shared/ppapi_proxy/browser_**
>
ppb_image_data_rpc_server.cc#**newcode134<https://chromiumcodereview.appspot.com/9677061/diff/4007/ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_image_data_rpc_server.cc#newcode134>
>
> ppapi/native_client/src/**shared/ppapi_proxy/browser_**
> ppb_image_data_rpc_server.cc:**134:
> if (native_handle >= 0) {
> On 2012/03/14 17:19:16, sehr wrote:
>
>> != nacl::kInvalidHandle
>>
>
> Done.
>
> https://chromiumcodereview.**appspot.com/9677061/diff/4007/**
> ppapi/native_client/src/**shared/ppapi_proxy/browser_**
>
ppb_image_data_rpc_server.cc#**newcode141<https://chromiumcodereview.appspot.com/9677061/diff/4007/ppapi/native_client/src/shared/ppapi_proxy/browser_ppb_image_data_rpc_server.cc#newcode141>
>
> ppapi/native_client/src/**shared/ppapi_proxy/browser_**
> ppb_image_data_rpc_server.cc:**141:
> NaClHandle nacl_handle = (NaClHandle)native_handle;
> On 2012/03/14 17:19:16, sehr wrote:
>
>> static_cast<NaClHandle>(...)
>>
> (see earlier reply)
>
>
https://chromiumcodereview.**appspot.com/9677061/<https://chromiumcodereview....
>

sehr (please use chromium)

On 2012/03/14 21:08:32, sehr wrote: > lgtm lgtm again.

8 years, 9 months ago (2012-03-14 22:40:27 UTC) #6

nfullagar

replaced c-style cast w/ c++ constructor On Wed, Mar 14, 2012 at 3:40 PM, <sehr@google.com> ...

8 years, 9 months ago (2012-03-14 23:34:15 UTC) #7

sehr (please use chromium)

On 2012/03/14 23:34:15, nfullagar wrote: > replaced c-style cast w/ c++ constructor > > On ...

8 years, 9 months ago (2012-03-15 16:08:31 UTC) #8

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/nfullagar@google.com/9677061/14004

8 years, 9 months ago (2012-03-15 20:52:03 UTC) #9

Change committed as 127056

Expand Messages | Collapse Messages