[PPAPI] Moving pp::VarResource_Dev API into pp::Var (now stable).

[PPAPI] Moving pp::VarResource_Dev API into pp::Var (now stable).

This API allows pp::Resource objects to be read and written to pp::Vars.

Moved the methods of PPB_VarResource_Dev (C API) to PPB_Var version 1.2.
Moved the methods of pp::VarResource_Dev (C++ API) to pp::Var.

Code written against the dev API will no longer compile, as references
to the API need to be updated to use Var instead of VarResource_Dev.
Binaries compiled against the dev API will now experience run-time
errors loading the old interface.

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

[Matt Giuca, 2014-02-04 08:39:41]

What a huge change just to rename this API! I think I have gotten everything. It
seems to work. Fortunately, most of the changes are highly mechanical. Thanks.

yzshen@chromium.org: ppapi/*
noelallen@chromium.org: native_client_sdk/*

[Matt Giuca, 2014-02-04 08:40:30]

+dmichael (if you would like to review instead of Yuzhu that is fine).

[Matt Giuca, 2014-02-04 08:42:02]

Please note the breakage this change will cause to existing apps already
compiled against the API.

Apps written against the C bindings will have get_interface fail. Apps written
against the C++ bindings will have null resources coming out (when reading) and
check fails (when writing).

We did discuss this at the time I put this into dev and I think you were OK with
it, but I just want to make sure.

[binji, 2014-02-04 17:26:31]

native_client_sdk lgtm

https://codereview.chromium.org/148213016/diff/1/native_client_sdk/src/exampl...
File native_client_sdk/src/examples/api/media_stream_video/media_stream_video.cc
(right):

https://codereview.chromium.org/148213016/diff/1/native_client_sdk/src/exampl...
native_client_sdk/src/examples/api/media_stream_video/media_stream_video.cc:135:
pp::Resource resource_track = pp::VarResource(var_track).AsResource();
Please remove the comment in "media_stream_video/index.html" saying that this
example uses dev interfaces.

[yzshen1, 2014-02-04 17:53:21]

https://codereview.chromium.org/148213016/diff/1/ppapi/api/ppb_var_resource.idl
File ppapi/api/ppb_var_resource.idl (right):

https://codereview.chromium.org/148213016/diff/1/ppapi/api/ppb_var_resource.i...
ppapi/api/ppb_var_resource.idl:16: interface PPB_VarResource {
I remember that we had considered the alternative of merging this into PPB_Var.
It seems more consistent with the current API: PPB_Var contains conversion
methods to/from other types.
Have we decided *not* to do that? (I don't remember. :))

https://codereview.chromium.org/148213016/diff/1/ppapi/thunk/interfaces_ppb_p...
File ppapi/thunk/interfaces_ppb_public_dev.h (right):

https://codereview.chromium.org/148213016/diff/1/ppapi/thunk/interfaces_ppb_p...
ppapi/thunk/interfaces_ppb_public_dev.h:25:
PROXIED_IFACE(PPB_VAR_RESOURCE_INTERFACE_1_0, PPB_VarResource_1_0)
This should be moved to interfaces_ppb_public_stable.h

[dmichael (off chromium), 2014-02-04 17:55:47]

https://codereview.chromium.org/148213016/diff/1/ppapi/api/ppb_var_resource.idl
File ppapi/api/ppb_var_resource.idl (right):

https://codereview.chromium.org/148213016/diff/1/ppapi/api/ppb_var_resource.i...
ppapi/api/ppb_var_resource.idl:16: interface PPB_VarResource {
On 2014/02/04 17:53:21, yzshen1 wrote:
> I remember that we had considered the alternative of merging this into
PPB_Var.
> It seems more consistent with the current API: PPB_Var contains conversion
> methods to/from other types.
> Have we decided *not* to do that? (I don't remember. :))
My recollection is that we had decided to merge it in to PPB_Var.

(I think I had voted for PPB_VarResource initially, but you all talked me in to
PPB_Var, and I'm still fine with that :-) )

[noelallen1, 2014-02-05 01:09:52]

Agree with yzshen and dmichael.  Adding new interfaces adds cost to the
developer.  Adding new functionality to existing interfaces would seem easier to
use.

[Matt Giuca, 2014-02-05 14:15:35]

Oh, now I remember that we decided to do that. Yes, I will move the methods into
Var.

[Matt Giuca, 2014-02-06 08:51:14]

PTAL

https://codereview.chromium.org/148213016/diff/1/native_client_sdk/src/exampl...
File native_client_sdk/src/examples/api/media_stream_video/media_stream_video.cc
(right):

https://codereview.chromium.org/148213016/diff/1/native_client_sdk/src/exampl...
native_client_sdk/src/examples/api/media_stream_video/media_stream_video.cc:135:
pp::Resource resource_track = pp::VarResource(var_track).AsResource();
On 2014/02/04 17:26:32, binji wrote:
> Please remove the comment in "media_stream_video/index.html" saying that this
> example uses dev interfaces.

Done.

https://codereview.chromium.org/148213016/diff/1/ppapi/api/ppb_var_resource.idl
File ppapi/api/ppb_var_resource.idl (right):

https://codereview.chromium.org/148213016/diff/1/ppapi/api/ppb_var_resource.i...
ppapi/api/ppb_var_resource.idl:16: interface PPB_VarResource {
On 2014/02/04 17:55:47, dmichael wrote:
> On 2014/02/04 17:53:21, yzshen1 wrote:
> > I remember that we had considered the alternative of merging this into
> PPB_Var.
> > It seems more consistent with the current API: PPB_Var contains conversion
> > methods to/from other types.
> > Have we decided *not* to do that? (I don't remember. :))
> My recollection is that we had decided to merge it in to PPB_Var.
> 
> (I think I had voted for PPB_VarResource initially, but you all talked me in
to
> PPB_Var, and I'm still fine with that :-) )

Done.

https://codereview.chromium.org/148213016/diff/1/ppapi/thunk/interfaces_ppb_p...
File ppapi/thunk/interfaces_ppb_public_dev.h (right):

https://codereview.chromium.org/148213016/diff/1/ppapi/thunk/interfaces_ppb_p...
ppapi/thunk/interfaces_ppb_public_dev.h:25:
PROXIED_IFACE(PPB_VAR_RESOURCE_INTERFACE_1_0, PPB_VarResource_1_0)
On 2014/02/04 17:53:21, yzshen1 wrote:
> This should be moved to interfaces_ppb_public_stable.h

No longer relevant.

[dmichael (off chromium), 2014-02-06 17:25:00]

https://codereview.chromium.org/148213016/diff/170001/native_client_sdk/src/e...
File native_client_sdk/src/examples/api/media_stream_video/index.html (left):

https://codereview.chromium.org/148213016/diff/170001/native_client_sdk/src/e...
native_client_sdk/src/examples/api/media_stream_video/index.html:21: or with
PNaCL.<br>
Actually, this is still almost true. The MediaStreamVideoTrack API is a dev
channel API, so it will only work on Dev channel and Canary.

https://codereview.chromium.org/148213016/diff/170001/ppapi/api/ppb_var.idl
File ppapi/api/ppb_var.idl (right):

https://codereview.chromium.org/148213016/diff/170001/ppapi/api/ppb_var.idl#n...
ppapi/api/ppb_var.idl:139: PP_Var VarFromResource([in] PP_Resource resource);
I can't remember, can this hold any PP_Resource, or only ones we support passing
over Messaging? I think it can hold any, in which case the comment is good.

If it can only hold *some*, that should be noted in the comment.

https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.cc
File ppapi/cpp/var.cc (right):

https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.cc#newcod...
ppapi/cpp/var.cc:126: get_interface<PPB_Var_1_0>()->AddRef(var_);
pp::Var was done before we decided to change our C++ version checking. Now, I
think the goal is that we will try the newest version first in all methods, then
fall back to the next, etc.

See file_io.cc for an example.

[yzshen1, 2014-02-06 18:02:57]

Mostly LG, only a few nits.
Thanks!

https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.cc
File ppapi/cpp/var.cc (right):

https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.cc#newcod...
ppapi/cpp/var.cc:136: if (has_interface<PPB_Var_1_0>())
In all the other cpp files, we use the latest version of API and then fallback
to old ones.

But it seems this file doesn't do that...

https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.cc#newcod...
ppapi/cpp/var.cc:238: pp::Resource Var::AsResource() const {
Maybe we should follow AsString to assert that it is_resource().
(And please update the comment.)

https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.h
File ppapi/cpp/var.h (right):

https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.h#newcode55
ppapi/cpp/var.h:55: explicit Var(const pp::Resource& resource);
Now that all the others are not 'explicit', maybe we should also support
implicit conversion in this case?

https://codereview.chromium.org/148213016/diff/170001/ppapi/shared_impl/ppb_v...
File ppapi/shared_impl/ppb_var_shared.cc (right):

https://codereview.chromium.org/148213016/diff/170001/ppapi/shared_impl/ppb_v...
ppapi/shared_impl/ppb_var_shared.cc:68: struct PP_Var
VarFromResource(PP_Resource resource) {
struct is not necessary

[dmichael (off chromium), 2014-02-06 18:33:13]

https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.h
File ppapi/cpp/var.h (right):

https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.h#newcode55
ppapi/cpp/var.h:55: explicit Var(const pp::Resource& resource);
On 2014/02/06 18:02:58, yzshen1 wrote:
> Now that all the others are not 'explicit', maybe we should also support
> implicit conversion in this case?
I think I prefer that the developer has to know that they're converting a
pp::Resource to a pp::Var, and aren't doing it by accident. (Admittedly, you
could say the same thing about the other ones, but I don't really like those
either :-p ).

[yzshen1, 2014-02-06 19:58:17]

https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.h
File ppapi/cpp/var.h (right):

https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.h#newcode55
ppapi/cpp/var.h:55: explicit Var(const pp::Resource& resource);
On 2014/02/06 18:33:14, dmichael wrote:
> On 2014/02/06 18:02:58, yzshen1 wrote:
> > Now that all the others are not 'explicit', maybe we should also support
> > implicit conversion in this case?
> I think I prefer that the developer has to know that they're converting a
> pp::Resource to a pp::Var, and aren't doing it by accident. (Admittedly, you
> could say the same thing about the other ones, but I don't really like those
> either :-p ).

I slightly prefer to be consistent. But I am fine with either approach. :)

[Matt Giuca, 2014-02-07 03:11:21]

OK, I'm just looking for an executive decision about two issues:

- Should Var be checking all interface versions, or just the oldest one that
introduced a particular API?
- Should Var(const pp::Resource&) be explicit?

Can you guys discuss and decide so I can have this ready by Monday? Thanks.

https://codereview.chromium.org/148213016/diff/170001/native_client_sdk/src/e...
File native_client_sdk/src/examples/api/media_stream_video/index.html (left):

https://codereview.chromium.org/148213016/diff/170001/native_client_sdk/src/e...
native_client_sdk/src/examples/api/media_stream_video/index.html:21: or with
PNaCL.<br>
On 2014/02/06 17:25:01, dmichael wrote:
> Actually, this is still almost true. The MediaStreamVideoTrack API is a dev
> channel API, so it will only work on Dev channel and Canary.

Reverted.

https://codereview.chromium.org/148213016/diff/170001/ppapi/api/ppb_var.idl
File ppapi/api/ppb_var.idl (right):

https://codereview.chromium.org/148213016/diff/170001/ppapi/api/ppb_var.idl#n...
ppapi/api/ppb_var.idl:139: PP_Var VarFromResource([in] PP_Resource resource);
Yes, it can hold any PP_Resource. It will only check if it's convertible when
you try to pass it.

https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.cc
File ppapi/cpp/var.cc (right):

https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.cc#newcod...
ppapi/cpp/var.cc:126: get_interface<PPB_Var_1_0>()->AddRef(var_);
:O is that necessary?

It doesn't seem scalable. It means that every time you add a new version, you
have to add new else blocks to every existing method, and duplicate the calls.

Do you want me to do this or not (given that, as Yuzhu points out, we don't
currently do it here).

https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.cc#newcod...
ppapi/cpp/var.cc:238: pp::Resource Var::AsResource() const {
On 2014/02/06 18:02:58, yzshen1 wrote:
> Maybe we should follow AsString to assert that it is_resource().
> (And please update the comment.)

Done.

https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.h
File ppapi/cpp/var.h (right):

https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.h#newcode55
ppapi/cpp/var.h:55: explicit Var(const pp::Resource& resource);
I briefly discussed this with sammc@ yesterday. He argued that if it's a managed
(ref-counted) type then it should be explicit, but primitives don't have to be.
That was the rationale for making this one explicit.

I don't have a strong preference, though. Can you guys make a decision?

(Note: we can remove "explicit" later without breaking apps, but we cannot add
"explicit".)

https://codereview.chromium.org/148213016/diff/170001/ppapi/shared_impl/ppb_v...
File ppapi/shared_impl/ppb_var_shared.cc (right):

https://codereview.chromium.org/148213016/diff/170001/ppapi/shared_impl/ppb_v...
ppapi/shared_impl/ppb_var_shared.cc:68: struct PP_Var
VarFromResource(PP_Resource resource) {
On 2014/02/06 18:02:58, yzshen1 wrote:
> struct is not necessary

Done.

[yzshen, 2014-02-07 04:54:50]

On Thu, Feb 6, 2014 at 7:11 PM, <mgiuca@chromium.org> wrote:

> OK, I'm just looking for an executive decision about two issues:
>
> - Should Var be checking all interface versions, or just the oldest one
> that
> introduced a particular API?
>
The team had a discussion before and preferred to check all versions, from
the latest to the oldest:
https://groups.google.com/a/chromium.org/forum/#!topic/pepper-dev/HrcIduc88ks

I personally think we should make the change.


- Should Var(const pp::Resource&) be explicit?
>
I am fine with making it explicit.


>
> Can you guys discuss and decide so I can have this ready by Monday? Thanks.
>
>
>
> https://codereview.chromium.org/148213016/diff/170001/
> native_client_sdk/src/examples/api/media_stream_video/index.html
> File native_client_sdk/src/examples/api/media_stream_video/index.html
> (left):
>
> https://codereview.chromium.org/148213016/diff/170001/
> native_client_sdk/src/examples/api/media_stream_video/index.html#oldcode21
> native_client_sdk/src/examples/api/media_stream_video/index.html:21: or
> with PNaCL.<br>
> On 2014/02/06 17:25:01, dmichael wrote:
>
>> Actually, this is still almost true. The MediaStreamVideoTrack API is
>>
> a dev
>
>> channel API, so it will only work on Dev channel and Canary.
>>
>
> Reverted.
>
>
> https://codereview.chromium.org/148213016/diff/170001/
> ppapi/api/ppb_var.idl
> File ppapi/api/ppb_var.idl (right):
>
> https://codereview.chromium.org/148213016/diff/170001/
> ppapi/api/ppb_var.idl#newcode139
> ppapi/api/ppb_var.idl:139: PP_Var VarFromResource([in] PP_Resource
> resource);
> Yes, it can hold any PP_Resource. It will only check if it's convertible
> when you try to pass it.
>
>
> https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.cc
> File ppapi/cpp/var.cc (right):
>
> https://codereview.chromium.org/148213016/diff/170001/
> ppapi/cpp/var.cc#newcode126
> ppapi/cpp/var.cc:126: get_interface<PPB_Var_1_0>()->AddRef(var_);
> :O is that necessary?
>
> It doesn't seem scalable. It means that every time you add a new
> version, you have to add new else blocks to every existing method, and
> duplicate the calls.
>
> Do you want me to do this or not (given that, as Yuzhu points out, we
> don't currently do it here).
>
>
> https://codereview.chromium.org/148213016/diff/170001/
> ppapi/cpp/var.cc#newcode238
> ppapi/cpp/var.cc:238: pp::Resource Var::AsResource() const {
> On 2014/02/06 18:02:58, yzshen1 wrote:
>
>> Maybe we should follow AsString to assert that it is_resource().
>> (And please update the comment.)
>>
>
> Done.
>
>
> https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.h
> File ppapi/cpp/var.h (right):
>
> https://codereview.chromium.org/148213016/diff/170001/
> ppapi/cpp/var.h#newcode55
> ppapi/cpp/var.h:55: explicit Var(const pp::Resource& resource);
> I briefly discussed this with sammc@ yesterday. He argued that if it's a
> managed (ref-counted) type then it should be explicit, but primitives
> don't have to be. That was the rationale for making this one explicit.
>
> I don't have a strong preference, though. Can you guys make a decision?
>
> (Note: we can remove "explicit" later without breaking apps, but we
> cannot add "explicit".)
>
>
> https://codereview.chromium.org/148213016/diff/170001/
> ppapi/shared_impl/ppb_var_shared.cc
> File ppapi/shared_impl/ppb_var_shared.cc (right):
>
> https://codereview.chromium.org/148213016/diff/170001/
> ppapi/shared_impl/ppb_var_shared.cc#newcode68
> ppapi/shared_impl/ppb_var_shared.cc:68: struct PP_Var
> VarFromResource(PP_Resource resource) {
> On 2014/02/06 18:02:58, yzshen1 wrote:
>
>> struct is not necessary
>>
>
> Done.
>
> https://codereview.chromium.org/148213016/
>



-- 
Best regards,
Yuzhu Shen.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[yzshen1, 2014-02-07 04:56:20]

(Resent from the right account.)


On Thu, Feb 6, 2014 at 7:11 PM, <mgiuca@chromium.org> wrote:

> OK, I'm just looking for an executive decision about two issues:
>
> - Should Var be checking all interface versions, or just the oldest one
> that
> introduced a particular API?
>
The team had a discussion before and preferred to check all versions, from
the latest to the oldest:
https://groups.google.com/a/chromium.org/forum/#!topic/pepper-dev/HrcIduc88ks

I personally think we should make the change.


> - Should Var(const pp::Resource&) be explicit?
>
I am fine with making it explicit.


>
> Can you guys discuss and decide so I can have this ready by Monday? Thanks.
>
>
>
> https://codereview.chromium.org/148213016/diff/170001/
> native_client_sdk/src/examples/api/media_stream_video/index.html
> File native_client_sdk/src/examples/api/media_stream_video/index.html
> (left):
>
> https://codereview.chromium.org/148213016/diff/170001/
> native_client_sdk/src/examples/api/media_stream_video/index.html#oldcode21
> native_client_sdk/src/examples/api/media_stream_video/index.html:21: or
> with PNaCL.<br>
> On 2014/02/06 17:25:01, dmichael wrote:
>
>> Actually, this is still almost true. The MediaStreamVideoTrack API is
>>
> a dev
>
>> channel API, so it will only work on Dev channel and Canary.
>>
>
> Reverted.
>
>
> https://codereview.chromium.org/148213016/diff/170001/
> ppapi/api/ppb_var.idl
> File ppapi/api/ppb_var.idl (right):
>
> https://codereview.chromium.org/148213016/diff/170001/
> ppapi/api/ppb_var.idl#newcode139
> ppapi/api/ppb_var.idl:139: PP_Var VarFromResource([in] PP_Resource
> resource);
> Yes, it can hold any PP_Resource. It will only check if it's convertible
> when you try to pass it.
>
>
> https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.cc
> File ppapi/cpp/var.cc (right):
>
> https://codereview.chromium.org/148213016/diff/170001/
> ppapi/cpp/var.cc#newcode126
> ppapi/cpp/var.cc:126: get_interface<PPB_Var_1_0>()->AddRef(var_);
> :O is that necessary?
>
> It doesn't seem scalable. It means that every time you add a new
> version, you have to add new else blocks to every existing method, and
> duplicate the calls.
>
> Do you want me to do this or not (given that, as Yuzhu points out, we
> don't currently do it here).
>
>
> https://codereview.chromium.org/148213016/diff/170001/
> ppapi/cpp/var.cc#newcode238
> ppapi/cpp/var.cc:238: pp::Resource Var::AsResource() const {
> On 2014/02/06 18:02:58, yzshen1 wrote:
>
>> Maybe we should follow AsString to assert that it is_resource().
>> (And please update the comment.)
>>
>
> Done.
>
>
> https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.h
> File ppapi/cpp/var.h (right):
>
> https://codereview.chromium.org/148213016/diff/170001/
> ppapi/cpp/var.h#newcode55
> ppapi/cpp/var.h:55: explicit Var(const pp::Resource& resource);
> I briefly discussed this with sammc@ yesterday. He argued that if it's a
> managed (ref-counted) type then it should be explicit, but primitives
> don't have to be. That was the rationale for making this one explicit.
>
> I don't have a strong preference, though. Can you guys make a decision?
>
> (Note: we can remove "explicit" later without breaking apps, but we
> cannot add "explicit".)
>
>
> https://codereview.chromium.org/148213016/diff/170001/
> ppapi/shared_impl/ppb_var_shared.cc
> File ppapi/shared_impl/ppb_var_shared.cc (right):
>
> https://codereview.chromium.org/148213016/diff/170001/
> ppapi/shared_impl/ppb_var_shared.cc#newcode68
> ppapi/shared_impl/ppb_var_shared.cc:68: struct PP_Var
> VarFromResource(PP_Resource resource) {
> On 2014/02/06 18:02:58, yzshen1 wrote:
>
>> struct is not necessary
>>
>
> Done.
>
> https://codereview.chromium.org/148213016/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmichael (off chromium), 2014-02-07 17:45:27]

https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.cc
File ppapi/cpp/var.cc (right):

https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.cc#newcod...
ppapi/cpp/var.cc:126: get_interface<PPB_Var_1_0>()->AddRef(var_);
On 2014/02/07 03:11:22, Matt Giuca wrote:
> :O is that necessary?
> 
> It doesn't seem scalable. It means that every time you add a new version, you
> have to add new else blocks to every existing method, and duplicate the calls.
> 
> Do you want me to do this or not (given that, as Yuzhu points out, we don't
> currently do it here).
Yes, we should do it here. It's definitely kind of ugly, and it's not *strictly*
necessary, since the backing implementation happens to be the same. But... we've
discussed this before a few times before coming to this approach. See for
example this thread:
https://groups.google.com/a/chromium.org/d/msg/pepper-dev/HrcIduc88ks/1nPPPVV...
...and also some discussions I had linked to in my first response on that
thread.

pp::Var was done before that decision, and we never got around to making it
consistent. I think we need to make it consistent now. The big advantage to the
always-try-the-new-one-first approach is it gives us more hope of deprecating
old versions of the C API eventually.

https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.h
File ppapi/cpp/var.h (right):

https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.h#newcode55
ppapi/cpp/var.h:55: explicit Var(const pp::Resource& resource);
I agree in thinking this one is a little different from primitives.
Additionally, we have a lot of functions that take pp::Var, and if we add this
constructor as implicit, it will suddenly be possible to accidentally pass a
pp::Resource in, and the compiler won't catch you. We'd be turning something
that currently is a compile-time mistake in to a runtime mistake. There's really
only 1 function where passing a pp::Resource as a pp::Var currently makes sense,
and that's PostMessage. I think the risk of making mistakes in other APIs
outweighs the benefits of being able to say "PostMessage(my_resource)" instead
of "pp::Var(my_resource)".

Yuzhu, are you okay with that direction?

[yzshen1, 2014-02-07 17:49:35]

https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.h
File ppapi/cpp/var.h (right):

https://codereview.chromium.org/148213016/diff/170001/ppapi/cpp/var.h#newcode55
ppapi/cpp/var.h:55: explicit Var(const pp::Resource& resource);
On 2014/02/07 17:45:28, dmichael wrote:
> I agree in thinking this one is a little different from primitives.
> Additionally, we have a lot of functions that take pp::Var, and if we add this
> constructor as implicit, it will suddenly be possible to accidentally pass a
> pp::Resource in, and the compiler won't catch you. We'd be turning something
> that currently is a compile-time mistake in to a runtime mistake. There's
really
> only 1 function where passing a pp::Resource as a pp::Var currently makes
sense,
> and that's PostMessage. I think the risk of making mistakes in other APIs
> outweighs the benefits of being able to say "PostMessage(my_resource)" instead
> of "pp::Var(my_resource)".
> 
> Yuzhu, are you okay with that direction?

Yeah. Your argument makes good sense. I agree we should do it with 'explicit'.
Thanks!

[Matt Giuca, 2014-02-10 01:52:19]

asvitkine: tools/metrics/histograms/histograms.xml

[Matt Giuca, 2014-02-10 02:02:52]

Other reviewers: I have updated Var as you asked (see
https://codereview.chromium.org/158563002/, and follow-up in this CL).

[Alexei Svitkine (slow), 2014-02-10 14:28:51]

https://codereview.chromium.org/148213016/diff/410001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/148213016/diff/410001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:29573: -  <int value="1465557812"
label="PPB_VarResource(Dev);0.1"/>
Don't remove existing entries from here, since old clients can still report
them. Just add the new ones.

[yzshen1, 2014-02-10 17:00:53]

https://codereview.chromium.org/148213016/diff/410001/ppapi/cpp/var.cc
File ppapi/cpp/var.cc (right):

https://codereview.chromium.org/148213016/diff/410001/ppapi/cpp/var.cc#newcode67
ppapi/cpp/var.cc:67: } else if (has_interface<PPB_Var_1_1>()) {
nit: According to style guide, it is preferred to omit all "else" in this case:
(multiple places in this file)
if (...) {
  return true;
}
if (...) {
  return true;
}
return false;

https://codereview.chromium.org/148213016/diff/410001/ppapi/cpp/var.cc#newcod...
ppapi/cpp/var.cc:221: default:  // Objects, arrays, dictionaries.
Please also add resource type here.

https://codereview.chromium.org/148213016/diff/410001/ppapi/cpp/var.cc#newcod...
ppapi/cpp/var.cc:315: } else {
please also add resource here.

[dmichael (off chromium), 2014-02-10 18:31:54]

lgtm, aside from Yuzhu's comments about resource. (And I'd like his feedback
about the "else if"...  if he still prefers "if" over "else if", I'm okay with
that; I don't want you to be held up)

https://codereview.chromium.org/148213016/diff/410001/ppapi/cpp/var.cc
File ppapi/cpp/var.cc (right):

https://codereview.chromium.org/148213016/diff/410001/ppapi/cpp/var.cc#newcode67
ppapi/cpp/var.cc:67: } else if (has_interface<PPB_Var_1_1>()) {
On 2014/02/10 17:00:53, yzshen1 wrote:
> nit: According to style guide, it is preferred to omit all "else" in this
case:
> (multiple places in this file)
> if (...) {
>   return true;
> }
> if (...) {
>   return true;
> }
> return false;
I wasn't aware of that; can you point me at the part of the style guide where it
says that? I think Matt got this format from file_io.cc, which I pointed him at.
Sorry if that was not right. Here's that example:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/file_io....

I personally find "else if" more readable, because you don't have to look inside
of the body of each "if" to know if the body of more than 1 "if" condition can
be executed. The existing way also is more consistent with how we've done other
files.
(See also
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/input_ev...
... and probably other examples)

I would be fine either way, except I do favor consistency.

https://codereview.chromium.org/148213016/diff/410001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/148213016/diff/410001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:29573: -  <int value="1465557812"
label="PPB_VarResource(Dev);0.1"/>
On 2014/02/10 14:28:53, Alexei Svitkine wrote:
> Don't remove existing entries from here, since old clients can still report
> them. Just add the new ones.
The old interface was not being used by any apps aside from internal testing, so
in this case it's safe to remove. It won't appear in the wild.

[Alexei Svitkine (slow), 2014-02-10 18:36:30]

histograms lgtm

https://codereview.chromium.org/148213016/diff/410001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/148213016/diff/410001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:29573: -  <int value="1465557812"
label="PPB_VarResource(Dev);0.1"/>
On 2014/02/10 18:31:56, dmichael wrote:
> On 2014/02/10 14:28:53, Alexei Svitkine wrote:
> > Don't remove existing entries from here, since old clients can still report
> > them. Just add the new ones.
> The old interface was not being used by any apps aside from internal testing,
so
> in this case it's safe to remove. It won't appear in the wild.

Normally, I'd still push back, but given this whole histogram was added recently
and indeed it seems we don't have any data for that entry[1], I guess it's okay.

[1]https://uma.googleplex.com/histograms/?dayCount=1&endDate=02-10-2014&version=Everything&histograms=Pepper.InterfaceUsed&otherVersions=true&filters

[yzshen1, 2014-02-10 19:44:12]

Thanks David. Please see inline comments.

https://codereview.chromium.org/148213016/diff/410001/ppapi/cpp/var.cc
File ppapi/cpp/var.cc (right):

https://codereview.chromium.org/148213016/diff/410001/ppapi/cpp/var.cc#newcode67
ppapi/cpp/var.cc:67: } else if (has_interface<PPB_Var_1_1>()) {
The style guide is at: http://www.chromium.org/developers/coding-style
(Please look at the part "Don't use else after return".)

There are quite some file using the no-else-after-return way. So I think we are
already not very consistent. :/ Hmm...


On 2014/02/10 18:31:56, dmichael wrote:
> On 2014/02/10 17:00:53, yzshen1 wrote:
> > nit: According to style guide, it is preferred to omit all "else" in this
> case:
> > (multiple places in this file)
> > if (...) {
> >   return true;
> > }
> > if (...) {
> >   return true;
> > }
> > return false;
> I wasn't aware of that; can you point me at the part of the style guide where
it
> says that? I think Matt got this format from file_io.cc, which I pointed him
at.
> Sorry if that was not right. Here's that example:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/file_io....
> 
> I personally find "else if" more readable, because you don't have to look
inside
> of the body of each "if" to know if the body of more than 1 "if" condition can
> be executed. The existing way also is more consistent with how we've done
other
> files.
> (See also
>
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/input_ev...
> ... and probably other examples)
> 
> I would be fine either way, except I do favor consistency.

[dmichael (off chromium), 2014-02-10 20:12:49]

https://codereview.chromium.org/148213016/diff/410001/ppapi/cpp/var.cc
File ppapi/cpp/var.cc (right):

https://codereview.chromium.org/148213016/diff/410001/ppapi/cpp/var.cc#newcode67
ppapi/cpp/var.cc:67: } else if (has_interface<PPB_Var_1_1>()) {
On 2014/02/10 19:44:14, yzshen1 wrote:
> The style guide is at: http://www.chromium.org/developers/coding-style
> (Please look at the part "Don't use else after return".)
Thanks, I was looking at the broader open source Google style guide that one
references.

I wouldn't have read that to apply to "else if", though I agree with you that it
would apply to the final "else" (like line 73-75).

I think:
if (...) {
}
if (...) {
}
is not as consistent with our other C++ wrapper fallback code I pointed to (I
didn't look at other examples that are in other parts of Pepper; I was trying to
find the closest analogue to this code).

But it's really not important to me; we can do it the way you prefer.

[yzshen1, 2014-02-10 22:38:40]

https://codereview.chromium.org/148213016/diff/410001/ppapi/cpp/var.cc
File ppapi/cpp/var.cc (right):

https://codereview.chromium.org/148213016/diff/410001/ppapi/cpp/var.cc#newcode67
ppapi/cpp/var.cc:67: } else if (has_interface<PPB_Var_1_1>()) {
I agree that it is not very important. :)
Please pick whatever you would like to do, Matt. I am fine with David's
approach, too.

Thanks!

On 2014/02/10 20:12:51, dmichael wrote:
> On 2014/02/10 19:44:14, yzshen1 wrote:
> > The style guide is at: http://www.chromium.org/developers/coding-style
> > (Please look at the part "Don't use else after return".)
> Thanks, I was looking at the broader open source Google style guide that one
> references.
> 
> I wouldn't have read that to apply to "else if", though I agree with you that
it
> would apply to the final "else" (like line 73-75).
> 
> I think:
> if (...) {
> }
> if (...) {
> }
> is not as consistent with our other C++ wrapper fallback code I pointed to (I
> didn't look at other examples that are in other parts of Pepper; I was trying
to
> find the closest analogue to this code).
> 
> But it's really not important to me; we can do it the way you prefer.

[Matt Giuca, 2014-02-10 22:47:51]

Thanks for the reviews. Before I go into these in too much detail, please look
at:
https://codereview.chromium.org/158563002/
This CL is blocked on that one now.

All the discussion about else if versus if should go in that CL (which is
transitioning Var to use that style and be consistent with the other
interfaces). This CL is just mechanically extending Var and should not be
changing its style.

For what it's worth, I agree that the style guide says that, but I was basing
mine off file_io.cc which uses the else if style. I think consistency here --
even across files -- is especially important here, since we're essentially doing
manual code generation and duplication, we at least should make it follow a
pattern across all files.

[dmichael (off chromium), 2014-02-10 22:59:40]

lgtm-ed that one, too, with the following nit:

I think the "else if"s are fine. I think (per the part of the styleguide
Yuzhu referenced), the final else should probably not be an else. Note this
is also consistent with other C++ wrapper fallback code:
https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/cpp/file_io....


On Mon, Feb 10, 2014 at 3:47 PM, <mgiuca@chromium.org> wrote:

> Thanks for the reviews. Before I go into these in too much detail, please
> look
> at:
> https://codereview.chromium.org/158563002/
> This CL is blocked on that one now.
>
> All the discussion about else if versus if should go in that CL (which is
> transitioning Var to use that style and be consistent with the other
> interfaces). This CL is just mechanically extending Var and should not be
> changing its style.
>
> For what it's worth, I agree that the style guide says that, but I was
> basing
> mine off file_io.cc which uses the else if style. I think consistency here
> --
> even across files -- is especially important here, since we're essentially
> doing
> manual code generation and duplication, we at least should make it follow a
> pattern across all files.
>
>
> https://codereview.chromium.org/148213016/
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to chromium-reviews+unsubscribe@chromium.org.
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Matt Giuca, 2014-02-11 02:41:16]

https://codereview.chromium.org/148213016/diff/410001/ppapi/cpp/var.cc
File ppapi/cpp/var.cc (right):

https://codereview.chromium.org/148213016/diff/410001/ppapi/cpp/var.cc#newcode67
ppapi/cpp/var.cc:67: } else if (has_interface<PPB_Var_1_1>()) {
I've changed the else but not the else-ifs.

https://codereview.chromium.org/148213016/diff/410001/ppapi/cpp/var.cc#newcod...
ppapi/cpp/var.cc:221: default:  // Objects, arrays, dictionaries.
On 2014/02/10 17:00:53, yzshen1 wrote:
> Please also add resource type here.

Done. (Oh no, this should have been there even before this CL.)

https://codereview.chromium.org/148213016/diff/410001/ppapi/cpp/var.cc#newcod...
ppapi/cpp/var.cc:315: } else {
On 2014/02/10 17:00:53, yzshen1 wrote:
> please also add resource here.

Done.

https://codereview.chromium.org/148213016/diff/410001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (left):

https://codereview.chromium.org/148213016/diff/410001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:29573: -  <int value="1465557812"
label="PPB_VarResource(Dev);0.1"/>
Note that external users can test out this API so we may still get data for this
in the future. But I don't think it's very important seeing as how we didn't
publicise it and it's going away now.

It depends on how critical it is -- UMA is not going to crash is it, if a data
point comes in that we don't have a handler for? I assume it just means we won't
record that data point, and since it will be at best a trickle, it can be safely
removed.

[yzshen1, 2014-02-11 04:50:19]

LGTM, Thanks!

[Matt Giuca, 2014-02-11 05:01:40]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-02-11 05:03:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/148213016/780001

[commit-bot: I haz the power, 2014-02-11 05:33:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-11 05:33:51]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[Matt Giuca, 2014-02-11 07:58:58]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-02-11 07:59:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/148213016/1120001

[Matt Giuca, 2014-02-11 08:00:35]

The CQ bit was unchecked by mgiuca@chromium.org

[Matt Giuca, 2014-02-11 08:01:02]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-02-11 08:01:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/148213016/1220001

[commit-bot: I haz the power, 2014-02-11 08:49:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-11 08:49:03]

Retried try job too often on mac for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[Matt Giuca, 2014-02-12 01:25:59]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-02-12 01:27:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/148213016/840002

[commit-bot: I haz the power, 2014-02-12 01:59:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-12 01:59:54]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Matt Giuca, 2014-02-12 03:33:53]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-02-12 03:54:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/148213016/840002

[commit-bot: I haz the power, 2014-02-12 08:48:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-12 08:48:10]

Retried try job too often on win_rel for step(s) ash_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Matt Giuca, 2014-02-13 00:27:06]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-02-13 00:30:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/148213016/2210001

[commit-bot: I haz the power, 2014-02-13 02:21:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-13 02:21:39]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, browser_tests, cacheinvalidation_unittests,
cc_unittests, check_deps, check_deps2git, chrome_elf_unittests,
chromedriver_unittests, components_unittests, compositor_unittests,
content_browsertests, content_unittests, crypto_unittests, events_unittests,
google_apis_unittests, gpu_unittests, installer_util_unittests, ipc_tests,
jingle_unittests, media_unittests, mini_installer_test, nacl_integration,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Matt Giuca, 2014-02-13 05:04:26]

The CQ bit was checked by mgiuca@chromium.org

[commit-bot: I haz the power, 2014-02-13 05:05:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mgiuca@chromium.org/148213016/2210001

[commit-bot: I haz the power, 2014-02-13 14:50:04]

Change committed as 251000