Add PPB_VarArray_Dev support - part 1.

Add PPB_VarArray_Dev support - part 1.

It includes:
- C/C++ interface implementation.
- Conversions between PP_Var and base::Value.

It dones't include:
- Serialization code for IPC.

BUG=None
TEST=None


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=188883

[yzshen1, 2013-03-04 18:49:21]

Hi, Raymes and David.

Would you please take a look? Thanks!

Raymes: everything.
David: high-level design.

[raymes, 2013-03-05 18:19:16]

Looks good! Mainly nits.

https://codereview.chromium.org/12388083/diff/1/ppapi/cpp/dev/var_array_dev.h
File ppapi/cpp/dev/var_array_dev.h (right):

https://codereview.chromium.org/12388083/diff/1/ppapi/cpp/dev/var_array_dev.h...
ppapi/cpp/dev/var_array_dev.h:24: /// methods specific to array.
to "arrays"?

https://codereview.chromium.org/12388083/diff/1/ppapi/shared_impl/array_var.cc
File ppapi/shared_impl/array_var.cc (right):

https://codereview.chromium.org/12388083/diff/1/ppapi/shared_impl/array_var.c...
ppapi/shared_impl/array_var.cc:63: if (elements_.size() >
std::numeric_limits<uint32_t>::max()) {
This is being very careful :) I was wondering whether it makes more sense to
have this before a resize operation...but I think it's fine here.

https://codereview.chromium.org/12388083/diff/1/ppapi/shared_impl/array_var.h
File ppapi/shared_impl/array_var.h (right):

https://codereview.chromium.org/12388083/diff/1/ppapi/shared_impl/array_var.h...
ppapi/shared_impl/array_var.h:34: // The returned PP_Var has been added ref on
behalf of the caller.
I think this needs to be re-worded. Maybe:
// The returned PP_Var has had a ref added on behalf of the caller.

https://codereview.chromium.org/12388083/diff/1/ppapi/shared_impl/var_value_c...
File ppapi/shared_impl/var_value_conversions_unittest.cc (right):

https://codereview.chromium.org/12388083/diff/1/ppapi/shared_impl/var_value_c...
ppapi/shared_impl/var_value_conversions_unittest.cc:99: var_iter !=
array_var->elements().end();
indentation here looks wrong

https://codereview.chromium.org/12388083/diff/1/ppapi/shared_impl/var_value_c...
ppapi/shared_impl/var_value_conversions_unittest.cc:184:
ASSERT_TRUE(array_var->Set(1, PP_MakeBool(PP_TRUE)));
Seems like we could have some more simple conversion tests for array (e.g. 0
element array, 1 element array) both to Var and from Var.

[yzshen1, 2013-03-14 22:36:48]

Thanks, Raymes!

Please take another look.

https://codereview.chromium.org/12388083/diff/1/ppapi/cpp/dev/var_array_dev.h
File ppapi/cpp/dev/var_array_dev.h (right):

https://codereview.chromium.org/12388083/diff/1/ppapi/cpp/dev/var_array_dev.h...
ppapi/cpp/dev/var_array_dev.h:24: /// methods specific to array.
On 2013/03/05 18:19:16, raymes wrote:
> to "arrays"?

Done.

https://codereview.chromium.org/12388083/diff/1/ppapi/shared_impl/array_var.cc
File ppapi/shared_impl/array_var.cc (right):

https://codereview.chromium.org/12388083/diff/1/ppapi/shared_impl/array_var.c...
ppapi/shared_impl/array_var.cc:63: if (elements_.size() >
std::numeric_limits<uint32_t>::max()) {
On 2013/03/05 18:19:16, raymes wrote:
> This is being very careful :) I was wondering whether it makes more sense to
> have this before a resize operation...but I think it's fine here.

Done. :)

https://codereview.chromium.org/12388083/diff/1/ppapi/shared_impl/array_var.h
File ppapi/shared_impl/array_var.h (right):

https://codereview.chromium.org/12388083/diff/1/ppapi/shared_impl/array_var.h...
ppapi/shared_impl/array_var.h:34: // The returned PP_Var has been added ref on
behalf of the caller.
Thanks. It sounds better this way.
On 2013/03/05 18:19:16, raymes wrote:
> I think this needs to be re-worded. Maybe:
> // The returned PP_Var has had a ref added on behalf of the caller.

https://codereview.chromium.org/12388083/diff/1/ppapi/shared_impl/var_value_c...
File ppapi/shared_impl/var_value_conversions_unittest.cc (right):

https://codereview.chromium.org/12388083/diff/1/ppapi/shared_impl/var_value_c...
ppapi/shared_impl/var_value_conversions_unittest.cc:99: var_iter !=
array_var->elements().end();
It is 4 spaces further than "value_iter != list_value.end()".
Every way I tried seemed weird. :) (4 more spaces than "for" / "(" /
"value_iter").
Which one do you prefer?

On 2013/03/05 18:19:16, raymes wrote:
> indentation here looks wrong

https://codereview.chromium.org/12388083/diff/1/ppapi/shared_impl/var_value_c...
ppapi/shared_impl/var_value_conversions_unittest.cc:184:
ASSERT_TRUE(array_var->Set(1, PP_MakeBool(PP_TRUE)));
On 2013/03/05 18:19:16, raymes wrote:
> Seems like we could have some more simple conversion tests for array (e.g. 0
> element array, 1 element array) both to Var and from Var.

Done.

[yzshen1, 2013-03-18 19:18:56]

Hi, Raymes.

Would you please take another look? Thanks!

[raymes, 2013-03-18 20:41:08]

lgtm with 1 comment

https://codereview.chromium.org/12388083/diff/8001/ppapi/shared_impl/array_va...
File ppapi/shared_impl/array_var.cc (right):

https://codereview.chromium.org/12388083/diff/8001/ppapi/shared_impl/array_va...
ppapi/shared_impl/array_var.cc:77: elements_.resize(length);
Probably need to add the check here too.

[yzshen1, 2013-03-18 20:50:42]

Thanks, Raymes!

https://codereview.chromium.org/12388083/diff/8001/ppapi/shared_impl/array_va...
File ppapi/shared_impl/array_var.cc (right):

https://codereview.chromium.org/12388083/diff/8001/ppapi/shared_impl/array_va...
ppapi/shared_impl/array_var.cc:77: elements_.resize(length);
Because the input type is uint32_t already, I thought it doesn't need a check.
Maybe I have misunderstood anything?

On 2013/03/18 20:41:09, raymes wrote:
> Probably need to add the check here too.

[raymes, 2013-03-18 20:57:02]

Oh nevermind I didn't notice. lgtm


On Mon, Mar 18, 2013 at 1:50 PM, <yzshen@chromium.org> wrote:

> Thanks, Raymes!
>
>
>
> https://codereview.chromium.**org/12388083/diff/8001/ppapi/**
>
shared_impl/array_var.cc<https://codereview.chromium.org/12388083/diff/8001/ppapi/shared_impl/array_var.cc>
> File ppapi/shared_impl/array_var.cc (right):
>
> https://codereview.chromium.**org/12388083/diff/8001/ppapi/**
>
shared_impl/array_var.cc#**newcode77<https://codereview.chromium.org/12388083/diff/8001/ppapi/shared_impl/array_var.cc#newcode77>
> ppapi/shared_impl/array_var.**cc:77: elements_.resize(length);
> Because the input type is uint32_t already, I thought it doesn't need a
> check. Maybe I have misunderstood anything?
>
>
> On 2013/03/18 20:41:09, raymes wrote:
>
>> Probably need to add the check here too.
>>
>
>
https://codereview.chromium.**org/12388083/<https://codereview.chromium.org/1...
>

[commit-bot: I haz the power, 2013-03-18 21:15:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yzshen@chromium.org/12388083/36001

[commit-bot: I haz the power, 2013-03-19 00:29:05]

Change committed as 188883