New method StatsObserver::OnCompleteReports, passing ownership.

New method StatsObserver::OnCompleteReports, passing ownership.

The new name, OnCompleteReports rather than OnComplete, is needed
because in C++ method lookup, overriding a method hides all otherwise
inherited methods with the same name, even if they have a different
signature. And here, the intention is that each subclass should
override one or the other of the two methods, and inherit the method it
doesn't override.

This cl is a prerequisite for
https://codereview.webrtc.org/2567143003/, because the Chrome glue
code needs to retain the stats report after the OnComplete method has
returned.

Currently, Chrome makes a copy of the stats mapping (which breaks when
changing ValuePtr from an rtc::linked_ptr to an std::unique_ptr). After
this cl, Chrome can be fixed to take ownership and no longer needs to
copy anything, unblocking cl 2567143003.

BUG=webrtc:6424
Review-Url: https://codereview.webrtc.org/2584553002
Cr-Commit-Position: refs/heads/master@{#15708}
Committed: https://chromium.googlesource.com/external/webrtc/+/b36ee8d498be2fa58fde3f3f3d69a74e4d3b817d

[nisse-webrtc, 2016-12-15 13:23:45]

nisse@webrtc.org changed reviewers:
+ hbos@webrtc.org, magjed@webrtc.org, solenberg@webrtc.org, tkchin@webrtc.org

[nisse-webrtc, 2016-12-15 13:23:46]

PTAL.

[hbos, 2016-12-15 13:39:41]

lgtm

https://codereview.webrtc.org/2584553002/diff/1/webrtc/api/peerconnectioninte...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2584553002/diff/1/webrtc/api/peerconnectioninte...
webrtc/api/peerconnectioninterface.h:118: OnComplete(*reports.get());
nit: Just *reports works too

[nisse-webrtc, 2016-12-15 14:02:31]

https://codereview.webrtc.org/2584553002/diff/1/webrtc/api/peerconnectioninte...
File webrtc/api/peerconnectioninterface.h (right):

https://codereview.webrtc.org/2584553002/diff/1/webrtc/api/peerconnectioninte...
webrtc/api/peerconnectioninterface.h:118: OnComplete(*reports.get());
On 2016/12/15 13:39:40, hbos wrote:
> nit: Just *reports works too

Done.

[the sun, 2016-12-15 14:39:41]

lgtm

[magjed_webrtc, 2016-12-19 12:56:20]

lgtm

Regarding that you have to name the new function OnCompleteReports and not
OnComplete, it sounds like you are hitting Woverloaded-virtual warnings? C++
certainly uses signatures for overload resolution and not just name, but this is
a name hiding issue you get warnings about. The current way of solving this name
hiding issue is to use an explicit using-declaration in the subclasses, but that
requires to change all subclasses, and I guess some of them are external in this
case? Anyway, the CL description is a bit confusing about this.

[nisse-webrtc, 2016-12-19 13:38:34]

On 2016/12/19 12:56:20, magjed_webrtc wrote:
> Regarding that you have to name the new function OnCompleteReports and not
> OnComplete, it sounds like you are hitting Woverloaded-virtual warnings? C++
> certainly uses signatures for overload resolution and not just name, but this
is
> a name hiding issue you get warnings about.

I discussed this with tommi@ and plundblad@ last time I was bitten by this. The
way I understood it, the compiler first looks up the class where the method is
implemented, based on name only. After identifying the class, it takes the
signature into account to find the right method. (But I fail to locate a good
reference for this behavior at the moment).

So if you have multiple methods with the same name but different signatures,
you're supposed to override all or none of them. And with virtual methods,
there's also the vtable at runtime, no idea what the compiler might do in case
the contents of the involved vtables conflict with this rule.

> The current way of solving this name
> hiding issue is to use an explicit using-declaration in the subclasses, but
that
> requires to change all subclasses, and I guess some of them are external in
this
> case?

Exactly, since this is a compatibility hack, each subclass should be able to
override one or the other of the two methods, and be blissfully unaware of the
method it's not overriding.

[magjed_webrtc, 2016-12-19 14:03:36]

On 2016/12/19 13:38:34, nisse-webrtc wrote:
> On 2016/12/19 12:56:20, magjed_webrtc wrote:
> > Regarding that you have to name the new function OnCompleteReports and not
> > OnComplete, it sounds like you are hitting Woverloaded-virtual warnings? C++
> > certainly uses signatures for overload resolution and not just name, but
this
> is
> > a name hiding issue you get warnings about.
> 
> I discussed this with tommi@ and plundblad@ last time I was bitten by this.
The
> way I understood it, the compiler first looks up the class where the method is
> implemented, based on name only. After identifying the class, it takes the
> signature into account to find the right method. (But I fail to locate a good
> reference for this behavior at the moment).
> 
> So if you have multiple methods with the same name but different signatures,
> you're supposed to override all or none of them. And with virtual methods,
> there's also the vtable at runtime, no idea what the compiler might do in case
> the contents of the involved vtables conflict with this rule.

How it is implemented by the compiler isn't really relevant. The rationale
behind name hiding is (like you mention) that it becomes too complex and
confusing to mix the set of overloaded methods with the mix of virtual methods.
The decision in C++ was made that every class should start as a clean slate with
regards to method resolution, and that's why you need to explicitly pull in the
function with an using-declaration in the subclass.

My point is that the first paragraph in the CL description that says that C++
uses names only for method resolution and not signatures is incorrect and
confusing, so I would rather you just remove that paragraph, or make a small
comment that the new naming is done to avoid Woverloaded-virtual warnings.

[nisse-webrtc, 2016-12-19 14:29:31]

On 2016/12/19 14:03:36, magjed_webrtc wrote:
> How it is implemented by the compiler isn't really relevant.

No, but it's also mandated by the language standard, if I understand it
correctly.

> My point is that the first paragraph in the CL description that says that C++
> uses names only for method resolution and not signatures is incorrect and
> confusing,

I'll try to rephrase it and refer to "hiding". This stack overflow post includes
a quote of the relevant paragraph of the standard:

----------8<------
The following steps define the result of name lookup in a class scope, C. First,
every declaration for the name in the class and in each of its base class
sub-objects is considered. A member name f in one sub- object B hides a member
name f in a sub-object A if A is a base class sub-object of B. Any declarations
that are so hidden are eliminated from consideration. Each of these declarations
that was introduced by a using-declaration is considered to be from each
sub-object of C that is of the type containing the declaration designated by the
using-declaration.96) If the resulting set of declarations are not all from
sub-objects of the same type, or the set has a nonstatic member and includes
members from distinct sub-objects, there is an ambiguity and the program is
ill-formed. Otherwise that set is the result of the lookup.
----------8<------

http://stackoverflow.com/questions/411103/function-with-same-name-but-differe...

Note that the first step in the process is based on name only, and that was what
I was trying to refer to.

[nisse-webrtc, 2016-12-19 14:36:19]

Description was changed from

==========
New method StatsObserver::OnCompleteReports, passing ownership.

The new name, OnCompleteReports rather than OnComplete, is needed
because C++ message resolution is based on name only, not signature,
and if we use the same method name, gcc warns when a subclass overrides
only one of the two variants of the method.

This cl is a prerequisite for
https://codereview.webrtc.org/2567143003/, because the Chrome glue
code needs to retain the stats report after the OnComplete method has
returned.

Currently, Chrome makes a copy of the stats mapping (which breaks when
changing ValuePtr from an rtc::linked_ptr to an std::unique_ptr). After
this cl, Chrome can be fixed to take ownership and no longer needs to
copy anything, unblocking cl 2567143003.

BUG=webrtc:6424
==========

to

==========
New method StatsObserver::OnCompleteReports, passing ownership.

The new name, OnCompleteReports rather than OnComplete, is needed
because in C++ method lookup, overriding a method hides all otherwise inherited
methods with the same name, even if they have a different signature. And here,
the intention is each subclass should override one or the other of the two
methods, and inherit the method it doesn't override.

This cl is a prerequisite for
https://codereview.webrtc.org/2567143003/, because the Chrome glue
code needs to retain the stats report after the OnComplete method has
returned.

Currently, Chrome makes a copy of the stats mapping (which breaks when
changing ValuePtr from an rtc::linked_ptr to an std::unique_ptr). After
this cl, Chrome can be fixed to take ownership and no longer needs to
copy anything, unblocking cl 2567143003.

BUG=webrtc:6424
==========

[tkchin_webrtc, 2016-12-19 23:41:41]

On 2016/12/19 14:29:31, nisse-webrtc wrote:
> On 2016/12/19 14:03:36, magjed_webrtc wrote:
> > How it is implemented by the compiler isn't really relevant.
> 
> No, but it's also mandated by the language standard, if I understand it
> correctly.
> 
> > My point is that the first paragraph in the CL description that says that
C++
> > uses names only for method resolution and not signatures is incorrect and
> > confusing,
> 
> I'll try to rephrase it and refer to "hiding". This stack overflow post
includes
> a quote of the relevant paragraph of the standard:
> 
> ----------8<------
> The following steps define the result of name lookup in a class scope, C.
First,
> every declaration for the name in the class and in each of its base class
> sub-objects is considered. A member name f in one sub- object B hides a member
> name f in a sub-object A if A is a base class sub-object of B. Any
declarations
> that are so hidden are eliminated from consideration. Each of these
declarations
> that was introduced by a using-declaration is considered to be from each
> sub-object of C that is of the type containing the declaration designated by
the
> using-declaration.96) If the resulting set of declarations are not all from
> sub-objects of the same type, or the set has a nonstatic member and includes
> members from distinct sub-objects, there is an ambiguity and the program is
> ill-formed. Otherwise that set is the result of the lookup.
> ----------8<------
> 
>
http://stackoverflow.com/questions/411103/function-with-same-name-but-differe...
> 
> Note that the first step in the process is based on name only, and that was
what
> I was trying to refer to.

lgtm

[nisse-webrtc, 2016-12-20 07:36:18]

Description was changed from

==========
New method StatsObserver::OnCompleteReports, passing ownership.

The new name, OnCompleteReports rather than OnComplete, is needed
because in C++ method lookup, overriding a method hides all otherwise inherited
methods with the same name, even if they have a different signature. And here,
the intention is each subclass should override one or the other of the two
methods, and inherit the method it doesn't override.

This cl is a prerequisite for
https://codereview.webrtc.org/2567143003/, because the Chrome glue
code needs to retain the stats report after the OnComplete method has
returned.

Currently, Chrome makes a copy of the stats mapping (which breaks when
changing ValuePtr from an rtc::linked_ptr to an std::unique_ptr). After
this cl, Chrome can be fixed to take ownership and no longer needs to
copy anything, unblocking cl 2567143003.

BUG=webrtc:6424
==========

to

==========
New method StatsObserver::OnCompleteReports, passing ownership.

The new name, OnCompleteReports rather than OnComplete, is needed
because in C++ method lookup, overriding a method hides all otherwise
inherited methods with the same name, even if they have a different
signature. And here, the intention is that each subclass should
override one or the other of the two methods, and inherit the method it
doesn't override.

This cl is a prerequisite for
https://codereview.webrtc.org/2567143003/, because the Chrome glue
code needs to retain the stats report after the OnComplete method has
returned.

Currently, Chrome makes a copy of the stats mapping (which breaks when
changing ValuePtr from an rtc::linked_ptr to an std::unique_ptr). After
this cl, Chrome can be fixed to take ownership and no longer needs to
copy anything, unblocking cl 2567143003.

BUG=webrtc:6424
==========

[nisse-webrtc, 2016-12-20 07:37:40]

Thanks for the review. Magnus, is the cl description good enough?

[nisse-webrtc, 2016-12-20 10:43:06]

The CQ bit was checked by nisse@webrtc.org

[nisse-webrtc, 2016-12-20 10:43:07]

The patchset sent to the CQ was uploaded after l-g-t-m from hbos@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2584553002/#ps20001 (title:
"Drop unneeded ".get()".")

[commit-bot: I haz the power, 2016-12-20 10:43:17]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-12-20 11:30:01]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1482230586915470,
"parent_rev": "5206667dadd69d30309dc69cc866925299b0efe1", "commit_rev":
"b36ee8d498be2fa58fde3f3f3d69a74e4d3b817d"}

[commit-bot: I haz the power, 2016-12-20 11:30:03]

Description was changed from

==========
New method StatsObserver::OnCompleteReports, passing ownership.

The new name, OnCompleteReports rather than OnComplete, is needed
because in C++ method lookup, overriding a method hides all otherwise
inherited methods with the same name, even if they have a different
signature. And here, the intention is that each subclass should
override one or the other of the two methods, and inherit the method it
doesn't override.

This cl is a prerequisite for
https://codereview.webrtc.org/2567143003/, because the Chrome glue
code needs to retain the stats report after the OnComplete method has
returned.

Currently, Chrome makes a copy of the stats mapping (which breaks when
changing ValuePtr from an rtc::linked_ptr to an std::unique_ptr). After
this cl, Chrome can be fixed to take ownership and no longer needs to
copy anything, unblocking cl 2567143003.

BUG=webrtc:6424
==========

to

==========
New method StatsObserver::OnCompleteReports, passing ownership.

The new name, OnCompleteReports rather than OnComplete, is needed
because in C++ method lookup, overriding a method hides all otherwise
inherited methods with the same name, even if they have a different
signature. And here, the intention is that each subclass should
override one or the other of the two methods, and inherit the method it
doesn't override.

This cl is a prerequisite for
https://codereview.webrtc.org/2567143003/, because the Chrome glue
code needs to retain the stats report after the OnComplete method has
returned.

Currently, Chrome makes a copy of the stats mapping (which breaks when
changing ValuePtr from an rtc::linked_ptr to an std::unique_ptr). After
this cl, Chrome can be fixed to take ownership and no longer needs to
copy anything, unblocking cl 2567143003.

BUG=webrtc:6424

Review-Url: https://codereview.webrtc.org/2584553002
Cr-Commit-Position: refs/heads/master@{#15708}
Committed:
https://chromium.googlesource.com/external/webrtc/+/b36ee8d498be2fa58fde3f3f3...
==========

[commit-bot: I haz the power, 2016-12-20 11:30:04]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/b36ee8d498be2fa58fde3f3f3...

[nisse-webrtc, 2017-01-18 09:20:12]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.webrtc.org/2641783002/ by nisse@webrtc.org.

The reason for reverting is: The new method doesn't work as intended. 

It can't pass ownership, because the StatsReports is a vector of raw pointers to
StatReport objects owned by the StatsCollector..