Fix up some types in the API.

Fix up some types in the API.

size_t -> int32_t, char* -> void* for buffers, void* -> const void* where needed.

We already use int32_t when we serialize size_t (and unlike pointers, size_ts do get serialized a lot).

char* for representing unstructured blocks of memory is weird, see "man 2 read" and friends. I'm removing it because it complicates our type system in IDL for no reason.

const void* should be obvious.

BUG=76271
TEST=none

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

[dmichael(do not use this one), 2011-03-21 21:53:24]

http://codereview.chromium.org/6711047/diff/1/ppapi/c/ppb_core.h
File ppapi/c/ppb_core.h (right):

http://codereview.chromium.org/6711047/diff/1/ppapi/c/ppb_core.h#newcode16
ppapi/c/ppb_core.h:16: #define PPB_CORE_INTERFACE "PPB_Core;0.4"
Are you going to put in any backwards compatibility for this?  Changing PPB_Core
is going to break pretty much all existing apps, I think.

http://codereview.chromium.org/6711047/diff/1/ppapi/c/ppb_core.h#newcode69
ppapi/c/ppb_core.h:69: void (*MemFree)(const void* ptr);
This isn't really that obvious to me.  I understand that it's okay to use a
const pointer since it's only freeing the memory (not modifying it like delete
w/destructor might).  But logically speaking, I would think a developer who
needs to free the memory should probably have a non-const pointer to it (or at
worst be willing to const-cast).

Did you run in to a place where you had a const pointer but needed to deallocate
it?

It sometimes even makes sense to null-out (or put garbage in) the given portion
of memory to smoke out bugs.  In that case, ptr would in fact be changed.  Once
the memory is freed, the developer can't make any assumptions about the value.

Not a big deal to me...  but it's not clear to me what you gain by making it
const.

http://codereview.chromium.org/6711047/diff/1/ppapi/cpp/core.h
File ppapi/cpp/core.h (left):

http://codereview.chromium.org/6711047/diff/1/ppapi/cpp/core.h#oldcode30
ppapi/cpp/core.h:30: void* MemAlloc(size_t num_bytes) {
size_t is 8 bytes on Linux 64.  It sounds like we've already decided we're okay
with that?  I think 32 bits should cover all our needs, but I want to make sure
it's been thought about already.

[neb, 2011-03-21 22:16:15]

http://codereview.chromium.org/6711047/diff/1/ppapi/c/ppb_core.h
File ppapi/c/ppb_core.h (right):

http://codereview.chromium.org/6711047/diff/1/ppapi/c/ppb_core.h#newcode16
ppapi/c/ppb_core.h:16: #define PPB_CORE_INTERFACE "PPB_Core;0.4"
On 2011/03/21 21:53:24, dmichael wrote:
> Are you going to put in any backwards compatibility for this?  Changing
PPB_Core
> is going to break pretty much all existing apps, I think.

Nope. Breakage will ensue.

http://codereview.chromium.org/6711047/diff/1/ppapi/c/ppb_core.h#newcode69
ppapi/c/ppb_core.h:69: void (*MemFree)(const void* ptr);
On 2011/03/21 21:53:24, dmichael wrote:
> This isn't really that obvious to me.  I understand that it's okay to use a
> const pointer since it's only freeing the memory (not modifying it like delete
> w/destructor might).  But logically speaking, I would think a developer who
> needs to free the memory should probably have a non-const pointer to it (or at
> worst be willing to const-cast).
> 
> Did you run in to a place where you had a const pointer but needed to
deallocate
> it?
> 
> It sometimes even makes sense to null-out (or put garbage in) the given
portion
> of memory to smoke out bugs.  In that case, ptr would in fact be changed. 
Once
> the memory is freed, the developer can't make any assumptions about the value.
> 
> Not a big deal to me...  but it's not clear to me what you gain by making it
> const.

Const-ness of pointer has to do with what the function is allowed to do with it.
In IDL, we mark in/out/inout params, and I was thinking of marking this ptr as
in.

But you're right in that it could be marked inout, as libc's free() does indeed
take non-const pointer.  I'll make ptr be inout and remove this change from the
CL.

Done.

http://codereview.chromium.org/6711047/diff/1/ppapi/cpp/core.h
File ppapi/cpp/core.h (left):

http://codereview.chromium.org/6711047/diff/1/ppapi/cpp/core.h#oldcode30
ppapi/cpp/core.h:30: void* MemAlloc(size_t num_bytes) {
On 2011/03/21 21:53:24, dmichael wrote:
> size_t is 8 bytes on Linux 64.  It sounds like we've already decided we're
okay
> with that?  I think 32 bits should cover all our needs, but I want to make
sure
> it's been thought about already.

Yeah, we talked about it a bit. We currently serialize this as 32 bits anyhow, I
don't see us making 4Gb resources any time soon.

[dmichael(do not use this one), 2011-03-21 22:26:58]

It might be good to do a heads-up to nacl-eng and/or pepper-team about the
ppb_core interface rev just so people are aware.

Thanks for fixing the const;  did you mean to also take it off of
plugin_module.cc?

O/W LGTM

http://codereview.chromium.org/6711047/diff/6004/webkit/plugins/ppapi/plugin_...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/6711047/diff/6004/webkit/plugins/ppapi/plugin_...
webkit/plugins/ppapi/plugin_module.cc:146: free(const_cast<void*>(ptr));
I think you want to take const off here, too.

[David Springer, 2011-03-21 22:31:29]

http://codereview.chromium.org/6711047/diff/1/ppapi/c/ppb_core.h
File ppapi/c/ppb_core.h (right):

http://codereview.chromium.org/6711047/diff/1/ppapi/c/ppb_core.h#newcode16
ppapi/c/ppb_core.h:16: #define PPB_CORE_INTERFACE "PPB_Core;0.4"
On 2011/03/21 22:16:15, neb wrote:
> On 2011/03/21 21:53:24, dmichael wrote:
> > Are you going to put in any backwards compatibility for this?  Changing
> PPB_Core
> > is going to break pretty much all existing apps, I think.
> 
> Nope. Breakage will ensue.

This will, in particular, break all the NaCl gallery examples which are on a
public code page.  This is a pain in the a-- but we can deal with it - we just
need to track this and update the gallery when appropriate.  Note that NaCl is
behind a flag and therefore subject to random breakage without notice, so this
is not a huge problem.  We just need a heads-up as this change bubble through
the proxy, etc.

[neb, 2011-03-21 23:22:25]

http://codereview.chromium.org/6711047/diff/6004/webkit/plugins/ppapi/plugin_...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/6711047/diff/6004/webkit/plugins/ppapi/plugin_...
webkit/plugins/ppapi/plugin_module.cc:146: free(const_cast<void*>(ptr));
On 2011/03/21 22:26:58, dmichael wrote:
> I think you want to take const off here, too.

Nice catch, I spaced out a bit.

I've removed it also from the C++ bindings and the proxy.

Done.

[dmichael(do not use this one), 2011-03-22 14:35:47]

In case you didn't already see them, it looks like you still have some build
errors.

[neb, 2011-03-22 20:02:18]

On 2011/03/21 22:31:29, David Springer wrote:
> http://codereview.chromium.org/6711047/diff/1/ppapi/c/ppb_core.h
> File ppapi/c/ppb_core.h (right):
> 
> http://codereview.chromium.org/6711047/diff/1/ppapi/c/ppb_core.h#newcode16
> ppapi/c/ppb_core.h:16: #define PPB_CORE_INTERFACE "PPB_Core;0.4"
> On 2011/03/21 22:16:15, neb wrote:
> > On 2011/03/21 21:53:24, dmichael wrote:
> > > Are you going to put in any backwards compatibility for this?  Changing
> > PPB_Core
> > > is going to break pretty much all existing apps, I think.
> > 
> > Nope. Breakage will ensue.
> 
> This will, in particular, break all the NaCl gallery examples which are on a
> public code page.  This is a pain in the a-- but we can deal with it - we just
> need to track this and update the gallery when appropriate.  Note that NaCl is
> behind a flag and therefore subject to random breakage without notice, so this
> is not a huge problem.  We just need a heads-up as this change bubble through
> the proxy, etc.

By breaking, you mean they will need to be recompiled?

[David Springer, 2011-03-22 20:08:19]

On Tue, Mar 22, 2011 at 2:02 PM, <neb@chromium.org> wrote:

> On 2011/03/21 22:31:29, David Springer wrote:
>
>> http://codereview.chromium.org/6711047/diff/1/ppapi/c/ppb_core.h
>> File ppapi/c/ppb_core.h (right):
>>
>
>
>> http://codereview.chromium.org/6711047/diff/1/ppapi/c/ppb_core.h#newcode16
>> ppapi/c/ppb_core.h:16: #define PPB_CORE_INTERFACE "PPB_Core;0.4"
>> On 2011/03/21 22:16:15, neb wrote:
>> > On 2011/03/21 21:53:24, dmichael wrote:
>> > > Are you going to put in any backwards compatibility for this?
>>  Changing
>> > PPB_Core
>> > > is going to break pretty much all existing apps, I think.
>> >
>> > Nope. Breakage will ensue.
>>
>
>  This will, in particular, break all the NaCl gallery examples which are on
>> a
>> public code page.  This is a pain in the a-- but we can deal with it - we
>> just
>> need to track this and update the gallery when appropriate.  Note that
>> NaCl is
>> behind a flag and therefore subject to random breakage without notice, so
>> this
>> is not a huge problem.  We just need a heads-up as this change bubble
>> through
>> the proxy, etc.
>>
>
> By breaking, you mean they will need to be recompiled?
>

Yes.  Plus, this change means that recompiled .nexes in the gallery will no
longer work in M10 stable or M11.  NaCl is still behind a flag, so this is
ok, it's just a pain.  When NaCl is on by default these kinds of changes
will meet with MUCH greater resistance.  So now is the time to do it.


>
> http://codereview.chromium.org/6711047/
>



-- 
- Dave.S

[piman, 2011-03-24 03:41:36]

I'm not sure I agree with the decision to switch from char* to void*. char* is
allowed to alias with every other type, but still provides pointer arithmetic
which is quite convenient for buffers of bytes. And especially when the said
buffers are actually strings, it's even more convenient to use char*. What's the
rationale ?

http://codereview.chromium.org/6711047/diff/15001/ppapi/lib/gl/egl/egldriver.c
File ppapi/lib/gl/egl/egldriver.c (right):

http://codereview.chromium.org/6711047/diff/15001/ppapi/lib/gl/egl/egldriver....
ppapi/lib/gl/egl/egldriver.c:7: #include
"third_party/mesa/MesaLib/src/egl/main/egldriver.h"
This include should be first

http://codereview.chromium.org/6711047/diff/15001/ppapi/lib/gl/gles2/gl2ext_p...
File ppapi/lib/gl/gles2/gl2ext_ppapi.c (right):

http://codereview.chromium.org/6711047/diff/15001/ppapi/lib/gl/gles2/gl2ext_p...
ppapi/lib/gl/gles2/gl2ext_ppapi.c:6: #include
"ppapi/lib/gl/gles2/gl2ext_ppapi.h"
This include should be first.

http://codereview.chromium.org/6711047/diff/15001/ppapi/proxy/ppb_url_request...
File ppapi/proxy/ppb_url_request_info_proxy.cc (right):

http://codereview.chromium.org/6711047/diff/15001/ppapi/proxy/ppb_url_request...
ppapi/proxy/ppb_url_request_info_proxy.cc:90: std::string(reinterpret_cast<const
char*>(data), len)));
static_cast

http://codereview.chromium.org/6711047/diff/15001/webkit/plugins/ppapi/ppb_ur...
File webkit/plugins/ppapi/ppb_url_loader_impl.cc (right):

http://codereview.chromium.org/6711047/diff/15001/webkit/plugins/ppapi/ppb_ur...
webkit/plugins/ppapi/ppb_url_loader_impl.cc:325: user_buffer_ =
reinterpret_cast<char*>(buffer);
static_cast

http://codereview.chromium.org/6711047/diff/15001/webkit/plugins/ppapi/ppb_ur...
File webkit/plugins/ppapi/ppb_url_request_info_impl.cc (right):

http://codereview.chromium.org/6711047/diff/15001/webkit/plugins/ppapi/ppb_ur...
webkit/plugins/ppapi/ppb_url_request_info_impl.cc:138: reinterpret_cast<const
char*>(data), len)));
static_cast

[neb, 2011-03-25 21:26:41]

On 2011/03/24 03:41:36, piman wrote:
> I'm not sure I agree with the decision to switch from char* to void*. char* is
> allowed to alias with every other type, but still provides pointer arithmetic
> which is quite convenient for buffers of bytes. And especially when the said
> buffers are actually strings, it's even more convenient to use char*. What's
the
> rationale ?

The rationale is to unify they types we use in PPAPI interfaces. We have a type
that we called mem_t in IDL, which we use when we pass and receive chunks of
memory. Request/response info is an example, but so is memfree and so forth, and
I don't think that we should use multiple types to mean chunk of memory. So what
type should it be? I dislike char because it implies that there is a structure
to the memory, which there isn't. void* can be converted to whatever you need,
like char*. Our PPAPI code always goes through a trampoline anyhow and we
serialize this as char[], so we don't get any pointer arithmetic benefits. Also,
char* conversions only go one way
(http://stackoverflow.com/questions/262379/when-is-char-safe-for-strict-pointe...)
which means it's awkward to use it for a return type.

[neb, 2011-03-25 21:26:41]

On 2011/03/24 03:41:36, piman wrote:
> I'm not sure I agree with the decision to switch from char* to void*. char* is
> allowed to alias with every other type, but still provides pointer arithmetic
> which is quite convenient for buffers of bytes. And especially when the said
> buffers are actually strings, it's even more convenient to use char*. What's
the
> rationale ?

The rationale is to unify they types we use in PPAPI interfaces. We have a type
that we called mem_t in IDL, which we use when we pass and receive chunks of
memory. Request/response info is an example, but so is memfree and so forth, and
I don't think that we should use multiple types to mean chunk of memory. So what
type should it be? I dislike char because it implies that there is a structure
to the memory, which there isn't. void* can be converted to whatever you need,
like char*. Our PPAPI code always goes through a trampoline anyhow and we
serialize this as char[], so we don't get any pointer arithmetic benefits. Also,
char* conversions only go one way
(http://stackoverflow.com/questions/262379/when-is-char-safe-for-strict-pointe...)
which means it's awkward to use it for a return type.

[neb, 2011-03-25 21:27:10]

http://codereview.chromium.org/6711047/diff/15001/ppapi/lib/gl/egl/egldriver.c
File ppapi/lib/gl/egl/egldriver.c (right):

http://codereview.chromium.org/6711047/diff/15001/ppapi/lib/gl/egl/egldriver....
ppapi/lib/gl/egl/egldriver.c:7: #include
"third_party/mesa/MesaLib/src/egl/main/egldriver.h"
On 2011/03/24 03:41:36, piman wrote:
> This include should be first

Unfortunately, Mesa's egldriver.h relies on NULL being defined although it never
includes a file that defines that. We used to define it in pp_stdint.h by
including <stddef.h>, and now we don't include stddef.h anymore. egldriver.h
includes PPAPI's egl.h -> eglplatform.h -> khrplatform.h, but I don't think NULL
belongs there. The proper thing would be to fix Mesa to include what they use.
This is a hack.

http://codereview.chromium.org/6711047/diff/15001/ppapi/lib/gl/gles2/gl2ext_p...
File ppapi/lib/gl/gles2/gl2ext_ppapi.c (right):

http://codereview.chromium.org/6711047/diff/15001/ppapi/lib/gl/gles2/gl2ext_p...
ppapi/lib/gl/gles2/gl2ext_ppapi.c:6: #include
"ppapi/lib/gl/gles2/gl2ext_ppapi.h"
On 2011/03/24 03:41:36, piman wrote:
> This include should be first.

Done.

http://codereview.chromium.org/6711047/diff/15001/ppapi/proxy/ppb_url_request...
File ppapi/proxy/ppb_url_request_info_proxy.cc (right):

http://codereview.chromium.org/6711047/diff/15001/ppapi/proxy/ppb_url_request...
ppapi/proxy/ppb_url_request_info_proxy.cc:90: std::string(reinterpret_cast<const
char*>(data), len)));
On 2011/03/24 03:41:36, piman wrote:
> static_cast

Done.

http://codereview.chromium.org/6711047/diff/15001/webkit/plugins/ppapi/ppb_ur...
File webkit/plugins/ppapi/ppb_url_loader_impl.cc (right):

http://codereview.chromium.org/6711047/diff/15001/webkit/plugins/ppapi/ppb_ur...
webkit/plugins/ppapi/ppb_url_loader_impl.cc:325: user_buffer_ =
reinterpret_cast<char*>(buffer);
On 2011/03/24 03:41:36, piman wrote:
> static_cast

Done.

http://codereview.chromium.org/6711047/diff/15001/webkit/plugins/ppapi/ppb_ur...
File webkit/plugins/ppapi/ppb_url_request_info_impl.cc (right):

http://codereview.chromium.org/6711047/diff/15001/webkit/plugins/ppapi/ppb_ur...
webkit/plugins/ppapi/ppb_url_request_info_impl.cc:138: reinterpret_cast<const
char*>(data), len)));
On 2011/03/24 03:41:36, piman wrote:
> static_cast

Done.

[piman, 2011-03-26 01:26:55]

Ok, LGTM I guess, I don't want to hold this up.