Proxy PPB_Var, fix o-o-p string var id tracking.

Proxy PPB_Var, fix o-o-p string var id tracking.

Note this doesn't need to use IPC at all, so it's a little strange.

Made test for pp::Var/PPB_Var that does only strings (copied from test_var_deprecated.cc).  Fixed string var tracking so test can pass out-of-process (aside from invalid UTF8 checking, which is still not implemented o-o-p).

BUG=85236
TEST=test_var.cc, run tests manually out-of-process.

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

[piman, 2011-06-08 19:31:29]

LGTM, couple of nits.
Adding Brett in CC because he wrote most of this for VarDeprecated.
Eventually we should remove the redundant functions from PPB_VarDeprecated, but
not for this CL.

http://codereview.chromium.org/6995083/diff/10001/ppapi/proxy/ppb_var_proxy.h
File ppapi/proxy/ppb_var_proxy.h (right):

http://codereview.chromium.org/6995083/diff/10001/ppapi/proxy/ppb_var_proxy.h...
ppapi/proxy/ppb_var_proxy.h:24: return reinterpret_cast<const
PPB_Var*>(target_interface());
nit: static_cast ?

http://codereview.chromium.org/6995083/diff/10001/ppapi/tests/test_var.cc
File ppapi/tests/test_var.cc (right):

http://codereview.chromium.org/6995083/diff/10001/ppapi/tests/test_var.cc#new...
ppapi/tests/test_var.cc:28: var_interface_ = reinterpret_cast<PPB_Var const*>(
nit: static_cast ? Also, for consistency, we use "const PP_Var*" across the
board.

http://codereview.chromium.org/6995083/diff/10001/ppapi/tests/test_var.cc#new...
ppapi/tests/test_var.cc:48: const char kStr[kStrLen + 1] = "Hello";
nit: you can declare kStr as a char[] and deduce kStrLen from
array_size(kStr)-1.

http://codereview.chromium.org/6995083/diff/10001/ppapi/tests/test_var.cc#new...
ppapi/tests/test_var.cc:106: return "Non-UTF8 string permitted.";
error message is slightly confusing. Mind changing it to "Non-UTF8 strings
should not be permitted." ?

[dmichael (off chromium), 2011-06-08 20:09:39]

Thanks for the review. Think it's okay to land prior to brettw having a chance
to look at it?  I was afraid this might block you porting to
InstancePrivate/VarPrivate.

http://codereview.chromium.org/6995083/diff/10001/ppapi/proxy/ppb_var_proxy.h
File ppapi/proxy/ppb_var_proxy.h (right):

http://codereview.chromium.org/6995083/diff/10001/ppapi/proxy/ppb_var_proxy.h...
ppapi/proxy/ppb_var_proxy.h:24: return reinterpret_cast<const
PPB_Var*>(target_interface());
On 2011/06/08 19:31:29, piman wrote:
> nit: static_cast ?
Done here and in ppb_var_deprecated_proxy.h.

http://codereview.chromium.org/6995083/diff/10001/ppapi/tests/test_var.cc
File ppapi/tests/test_var.cc (right):

http://codereview.chromium.org/6995083/diff/10001/ppapi/tests/test_var.cc#new...
ppapi/tests/test_var.cc:28: var_interface_ = reinterpret_cast<PPB_Var const*>(
On 2011/06/08 19:31:29, piman wrote:
> nit: static_cast ? Also, for consistency, we use "const PP_Var*" across the
> board.
Done (here and in test_var_deprecated.cc)

http://codereview.chromium.org/6995083/diff/10001/ppapi/tests/test_var.cc#new...
ppapi/tests/test_var.cc:48: const char kStr[kStrLen + 1] = "Hello";
On 2011/06/08 19:31:29, piman wrote:
> nit: you can declare kStr as a char[] and deduce kStrLen from
> array_size(kStr)-1.
Done (here and in test_var_deprecated.cc)

http://codereview.chromium.org/6995083/diff/10001/ppapi/tests/test_var.cc#new...
ppapi/tests/test_var.cc:106: return "Non-UTF8 string permitted.";
On 2011/06/08 19:31:29, piman wrote:
> error message is slightly confusing. Mind changing it to "Non-UTF8 strings
> should not be permitted." ?
Since it's failure output, I think it should be phrased in terms of the
unexpected behavior.  How does "Non-UTF8 string was permitted erroneously."
sound?

Done here and in test_var_deprecated.cc

[piman, 2011-06-08 20:12:04]

On Wed, Jun 8, 2011 at 1:09 PM, <dmichael@chromium.org> wrote:

> Thanks for the review. Think it's okay to land prior to brettw having a
> chance
> to look at it?  I was afraid this might block you porting to
> InstancePrivate/VarPrivate.


Yes, go ahead. I just cc: brett if he wants to do a post commit review.

>
>
>
>
> http://codereview.chromium.org/6995083/diff/10001/ppapi/proxy/ppb_var_proxy.h
> File ppapi/proxy/ppb_var_proxy.h (right):
>
>
>
http://codereview.chromium.org/6995083/diff/10001/ppapi/proxy/ppb_var_proxy.h...
> ppapi/proxy/ppb_var_proxy.h:24: return reinterpret_cast<const
> PPB_Var*>(target_interface());
> On 2011/06/08 19:31:29, piman wrote:
>
>> nit: static_cast ?
>>
> Done here and in ppb_var_deprecated_proxy.h.
>
>
> http://codereview.chromium.org/6995083/diff/10001/ppapi/tests/test_var.cc
> File ppapi/tests/test_var.cc (right):
>
>
>
http://codereview.chromium.org/6995083/diff/10001/ppapi/tests/test_var.cc#new...
> ppapi/tests/test_var.cc:28: var_interface_ = reinterpret_cast<PPB_Var
> const*>(
> On 2011/06/08 19:31:29, piman wrote:
>
>> nit: static_cast ? Also, for consistency, we use "const PP_Var*"
>>
> across the
>
>> board.
>>
> Done (here and in test_var_deprecated.cc)
>
>
>
>
http://codereview.chromium.org/6995083/diff/10001/ppapi/tests/test_var.cc#new...
> ppapi/tests/test_var.cc:48: const char kStr[kStrLen + 1] = "Hello";
> On 2011/06/08 19:31:29, piman wrote:
>
>> nit: you can declare kStr as a char[] and deduce kStrLen from
>> array_size(kStr)-1.
>>
> Done (here and in test_var_deprecated.cc)
>
>
>
>
http://codereview.chromium.org/6995083/diff/10001/ppapi/tests/test_var.cc#new...
> ppapi/tests/test_var.cc:106: return "Non-UTF8 string permitted.";
> On 2011/06/08 19:31:29, piman wrote:
>
>> error message is slightly confusing. Mind changing it to "Non-UTF8
>>
> strings
>
>> should not be permitted." ?
>>
> Since it's failure output, I think it should be phrased in terms of the
> unexpected behavior.  How does "Non-UTF8 string was permitted
> erroneously." sound?
>

Sounds fine.


>
> Done here and in test_var_deprecated.cc
>
>
> http://codereview.chromium.org/6995083/
>