Tweaks to PPB_VarArrayBuffer in preperation for taking out of Dev.

Tweaks to PPB_VarArrayBuffer in preperation for taking out of Dev.

* Add Unmap.
* Make ByteLength more consistent with the rest of PPAPI.
* Make C++ wrapper not cache the buffer.

BUG=
TEST=


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

[dmichael (off chromium), 2012-01-25 21:55:32]

darin: Please check the interface and C++ with an eye towards taking this out of
Dev.
bbudge:  Everything else :-)

Thanks!

[bbudge, 2012-01-25 22:19:07]

http://codereview.chromium.org/9169052/diff/2002/ppapi/api/dev/ppb_var_array_...
File ppapi/api/dev/ppb_var_array_buffer_dev.idl (right):

http://codereview.chromium.org/9169052/diff/2002/ppapi/api/dev/ppb_var_array_...
ppapi/api/dev/ppb_var_array_buffer_dev.idl:61: * @param[in] array The array
whose buffer should be returned.
Buffer in this case is the physical address?

http://codereview.chromium.org/9169052/diff/2002/ppapi/api/dev/ppb_var_array_...
ppapi/api/dev/ppb_var_array_buffer_dev.idl:76: void Unmap([in] PP_Var array);
Kind of confusing mix of Array, ArrayBuffer and VarArrayBuffer used to describe
the same thing. Could you use ArrayBuffer only?

http://codereview.chromium.org/9169052/diff/2002/ppapi/cpp/dev/var_array_buff...
File ppapi/cpp/dev/var_array_buffer_dev.cc (right):

http://codereview.chromium.org/9169052/diff/2002/ppapi/cpp/dev/var_array_buff...
ppapi/cpp/dev/var_array_buffer_dev.cc:58:
get_interface<PPB_VarArrayBuffer_Dev>()->Map(var_);
Shouldn't this call UnMap?

http://codereview.chromium.org/9169052/diff/2002/ppapi/cpp/dev/var_array_buff...
File ppapi/cpp/dev/var_array_buffer_dev.h (right):

http://codereview.chromium.org/9169052/diff/2002/ppapi/cpp/dev/var_array_buff...
ppapi/cpp/dev/var_array_buffer_dev.h:30: VarArrayBuffer_Dev(const
VarArrayBuffer_Dev& buffer) : Var(buffer) {}
pp::Var defines an assignment operator. Does it do the right thing?

http://codereview.chromium.org/9169052/diff/2002/ppapi/cpp/dev/var_array_buff...
ppapi/cpp/dev/var_array_buffer_dev.h:63: void Unmap();
same comment as for .idl, maybe standardize on VarArrayBuffer here.

http://codereview.chromium.org/9169052/diff/2002/ppapi/shared_impl/ppb_var_sh...
File ppapi/shared_impl/ppb_var_shared.cc (right):

http://codereview.chromium.org/9169052/diff/2002/ppapi/shared_impl/ppb_var_sh...
ppapi/shared_impl/ppb_var_shared.cc:81: return PP_FALSE;
byte_length is undefined in this case. You might want to zero it, or spec this
in the .idl.

[dmichael (off chromium), 2012-01-25 23:50:10]

http://codereview.chromium.org/9169052/diff/2002/ppapi/api/dev/ppb_var_array_...
File ppapi/api/dev/ppb_var_array_buffer_dev.idl (right):

http://codereview.chromium.org/9169052/diff/2002/ppapi/api/dev/ppb_var_array_...
ppapi/api/dev/ppb_var_array_buffer_dev.idl:61: * @param[in] array The array
whose buffer should be returned.
On 2012/01/25 22:19:07, bbudge1 wrote:
> Buffer in this case is the physical address?
Yes.

http://codereview.chromium.org/9169052/diff/2002/ppapi/api/dev/ppb_var_array_...
ppapi/api/dev/ppb_var_array_buffer_dev.idl:76: void Unmap([in] PP_Var array);
On 2012/01/25 22:19:07, bbudge1 wrote:
> Kind of confusing mix of Array, ArrayBuffer and VarArrayBuffer used to
describe
> the same thing. Could you use ArrayBuffer only?
Good point, done.

http://codereview.chromium.org/9169052/diff/2002/ppapi/cpp/dev/var_array_buff...
File ppapi/cpp/dev/var_array_buffer_dev.cc (right):

http://codereview.chromium.org/9169052/diff/2002/ppapi/cpp/dev/var_array_buff...
ppapi/cpp/dev/var_array_buffer_dev.cc:58:
get_interface<PPB_VarArrayBuffer_Dev>()->Map(var_);
On 2012/01/25 22:19:07, bbudge1 wrote:
> Shouldn't this call UnMap?
Unmap is only an optimization for apps that want to save memory but still have
access to the buffer later.

http://codereview.chromium.org/9169052/diff/2002/ppapi/cpp/dev/var_array_buff...
File ppapi/cpp/dev/var_array_buffer_dev.h (right):

http://codereview.chromium.org/9169052/diff/2002/ppapi/cpp/dev/var_array_buff...
ppapi/cpp/dev/var_array_buffer_dev.h:30: VarArrayBuffer_Dev(const
VarArrayBuffer_Dev& buffer) : Var(buffer) {}
On 2012/01/25 22:19:07, bbudge1 wrote:
> pp::Var defines an assignment operator. Does it do the right thing?
Yes. I went ahead and added a simple VarArrayBuffer->VarArrayBuffer assignment
for consistency & convenience.

http://codereview.chromium.org/9169052/diff/2002/ppapi/cpp/dev/var_array_buff...
ppapi/cpp/dev/var_array_buffer_dev.h:63: void Unmap();
On 2012/01/25 22:19:07, bbudge1 wrote:
> same comment as for .idl, maybe standardize on VarArrayBuffer here.
Done.

http://codereview.chromium.org/9169052/diff/2002/ppapi/shared_impl/ppb_var_sh...
File ppapi/shared_impl/ppb_var_shared.cc (right):

http://codereview.chromium.org/9169052/diff/2002/ppapi/shared_impl/ppb_var_sh...
ppapi/shared_impl/ppb_var_shared.cc:81: return PP_FALSE;
On 2012/01/25 22:19:07, bbudge1 wrote:
> byte_length is undefined in this case. You might want to zero it, or spec this
> in the .idl.
Good point. My intent is that byte_length is guaranteed to be unchanged in that
case, so I updated the IDL.

[darin (slow to review), 2012-01-26 05:01:44]

http://codereview.chromium.org/9169052/diff/1018/ppapi/api/dev/ppb_var_array_...
File ppapi/api/dev/ppb_var_array_buffer_dev.idl (right):

http://codereview.chromium.org/9169052/diff/1018/ppapi/api/dev/ppb_var_array_...
ppapi/api/dev/ppb_var_array_buffer_dev.idl:34: * Retrieves the length of the
VarArrayBuffer in bytes. On success,
In a previous version, you had it so that ByteLength would fail if the
ArrayBuffer is not successfully mapped.  I think we shouldn't require the
ArrayBuffer to be mapped in order to report its length.  Since you don't specify
the behavior of calling ByteLength before Map, maybe it would be good to specify
what happens?  Hopefully, you agree that it should give a non-error :)

[dmichael (off chromium), 2012-01-26 05:43:38]

http://codereview.chromium.org/9169052/diff/1018/ppapi/api/dev/ppb_var_array_...
File ppapi/api/dev/ppb_var_array_buffer_dev.idl (right):

http://codereview.chromium.org/9169052/diff/1018/ppapi/api/dev/ppb_var_array_...
ppapi/api/dev/ppb_var_array_buffer_dev.idl:34: * Retrieves the length of the
VarArrayBuffer in bytes. On success,
On 2012/01/26 05:01:44, darin wrote:
> In a previous version, you had it so that ByteLength would fail if the
> ArrayBuffer is not successfully mapped.
That was a brief moment of insanity; the patch didn't work that way but I forgot
to fix the comment before uploading :-)
>  I think we shouldn't require the
> ArrayBuffer to be mapped in order to report its length.  Since you don't
specify
> the behavior of calling ByteLength before Map, maybe it would be good to
specify
> what happens?  Hopefully, you agree that it should give a non-error :)
Agreed, done.

[darin (slow to review), 2012-01-26 05:55:36]

LGTM for IDL and C++ bindings.

[bbudge, 2012-01-26 17:20:26]

http://codereview.chromium.org/9169052/diff/5004/ppapi/api/dev/ppb_var_array_...
File ppapi/api/dev/ppb_var_array_buffer_dev.idl (right):

http://codereview.chromium.org/9169052/diff/5004/ppapi/api/dev/ppb_var_array_...
ppapi/api/dev/ppb_var_array_buffer_dev.idl:28: * @return A PP_Var which
represents an VarArrayBuffer of the requested size
an/a

http://codereview.chromium.org/9169052/diff/5004/ppapi/cpp/dev/var_array_buff...
File ppapi/cpp/dev/var_array_buffer_dev.cc (right):

http://codereview.chromium.org/9169052/diff/5004/ppapi/cpp/dev/var_array_buff...
ppapi/cpp/dev/var_array_buffer_dev.cc:42: Var::operator=(other);
Isn't it a problem that you can assign a non-ArrayBuffer var to an ArrayBuffer
(using the base operator=)? Seems like operator= needs to be virtual on the base
so you can check that it's the right type (VarArrayBuffer) in your override.

http://codereview.chromium.org/9169052/diff/5004/ppapi/cpp/dev/var_array_buff...
ppapi/cpp/dev/var_array_buffer_dev.cc:64:
get_interface<PPB_VarArrayBuffer_Dev>()->Map(var_);
Reading the code, it still seems weird to see Unmap call Map. Maybe a comment
explaining why?

[dmichael (off chromium), 2012-01-26 18:33:47]

Thanks for the fast/quality reviews guys. bbudge, I think I addressed your
concerns, PTAL.

http://codereview.chromium.org/9169052/diff/5004/ppapi/api/dev/ppb_var_array_...
File ppapi/api/dev/ppb_var_array_buffer_dev.idl (right):

http://codereview.chromium.org/9169052/diff/5004/ppapi/api/dev/ppb_var_array_...
ppapi/api/dev/ppb_var_array_buffer_dev.idl:28: * @return A PP_Var which
represents an VarArrayBuffer of the requested size
On 2012/01/26 17:20:26, bbudge1 wrote:
> an/a

Done.

http://codereview.chromium.org/9169052/diff/5004/ppapi/cpp/dev/var_array_buff...
File ppapi/cpp/dev/var_array_buffer_dev.cc (right):

http://codereview.chromium.org/9169052/diff/5004/ppapi/cpp/dev/var_array_buff...
ppapi/cpp/dev/var_array_buffer_dev.cc:42: Var::operator=(other);
On 2012/01/26 17:20:26, bbudge1 wrote:
> Isn't it a problem that you can assign a non-ArrayBuffer var to an ArrayBuffer
> (using the base operator=)? Seems like operator= needs to be virtual on the
base
> so you can check that it's the right type (VarArrayBuffer) in your override.
Good point; I was only really thinking about static types. Done.

http://codereview.chromium.org/9169052/diff/5004/ppapi/cpp/dev/var_array_buff...
ppapi/cpp/dev/var_array_buffer_dev.cc:64:
get_interface<PPB_VarArrayBuffer_Dev>()->Map(var_);
On 2012/01/26 17:20:26, bbudge1 wrote:
> Reading the code, it still seems weird to see Unmap call Map. Maybe a comment
> explaining why?
Gah. You were completely right the first time. I read it as if you were
commenting on the example code in the header for some reason, rather than
looking at the actual context of your comment. Sorry, and thanks for keeping me
honest. Fixed.

[bbudge, 2012-01-26 18:37:47]

lgtm

[bbudge, 2012-01-26 18:37:49]

lgtm

[bbudge, 2012-01-26 18:39:03]

On 2012/01/26 18:37:49, bbudge1 wrote:
> lgtm

Whoa, a double el-gee-tee-em!

[commit-bot: I haz the power, 2012-01-26 19:13:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/9169052/4018

[commit-bot: I haz the power, 2012-01-26 21:04:11]

Change committed as 119286