RTCStatsReport: Take ownership of other's stats with TakeMembersFrom

RTCStatsReport: Take ownership of other's stats with TakeMembersFrom

This would make it possible to gather stats on multiple threads, store
the results in multiple reports and to merge the results.

Added rtcstatsreport_unittest.cc, moving a RTCStatsReport-related test
from rtcstats_unittest.cc. Added more unittests covering the order of
stats and TakeMembersFrom.

Also changed RTCStatsReport[] to RTCStatsReport::Get to avoid
confusion with other usages of the [] operator.

BUG=chromium:627816
NOTRY=True

Committed: https://crrev.com/6d183ac27a6755497e35f197b8aaf41d6319aeaf
Cr-Commit-Position: refs/heads/master@{#13957}

[hbos, 2016-08-24 12:46:00]

hbos@webrtc.org changed reviewers:
+ glaznev@webrtc.org, hta@webrtc.org

[hbos, 2016-08-24 12:46:01]

Please take a look, hta and glaznev.

[hta-webrtc, 2016-08-24 13:54:55]

Some nits, and something I should have spotted earlier.

https://codereview.webrtc.org/2278433003/diff/1/webrtc/api/rtcstatsreport.h
File webrtc/api/rtcstatsreport.h (right):

https://codereview.webrtc.org/2278433003/diff/1/webrtc/api/rtcstatsreport.h#n...
webrtc/api/rtcstatsreport.h:62: void Scavenge(rtc::scoped_refptr<RTCStatsReport>
victim);
Suggest using less carnivorous language - what about MoveMembersFrom(victim) or
TakeMembersFrom(victim)?

https://codereview.webrtc.org/2278433003/diff/1/webrtc/stats/rtcstatsreport.cc
File webrtc/stats/rtcstatsreport.cc (right):

https://codereview.webrtc.org/2278433003/diff/1/webrtc/stats/rtcstatsreport.c...
webrtc/stats/rtcstatsreport.cc:69: const RTCStats*
RTCStatsReport::operator[](const std::string& id) const {
(missed this previously) strongly suggest replacing [] with a simple getter. The
semantics of [] in other places in C++ are bad enough that we shouldn't be using
the symbol unless that's the precise semantics we want.

https://codereview.webrtc.org/2278433003/diff/1/webrtc/stats/rtcstatsreport_u...
File webrtc/stats/rtcstatsreport_unittest.cc (right):

https://codereview.webrtc.org/2278433003/diff/1/webrtc/stats/rtcstatsreport_u...
webrtc/stats/rtcstatsreport_unittest.cc:42: RTCStatsMember<int32_t> dummy;
Suggest using different member variables in the different types.

[hbos, 2016-08-24 14:58:18]

Description was changed from

==========
RTCStatsReport: Take ownership of another report's stats with Scavenge

This would make it possible to gather stats on multiple threads, store
the results in multiple reports and to merge the results.

Added rtcstatsreport_unittest.cc, moving a RTCStatsReport-related test
from rtcstats_unittest.cc. Added more unittests covering the order of
stats and scavenging.

BUG=chromium:627816
==========

to

==========
RTCStatsReport: Take ownership of other's stats with TakeMembersFrom

This would make it possible to gather stats on multiple threads, store
the results in multiple reports and to merge the results.

Added rtcstatsreport_unittest.cc, moving a RTCStatsReport-related test
from rtcstats_unittest.cc. Added more unittests covering the order of
stats and TakeMembersFrom.

BUG=chromium:627816
==========

[hbos, 2016-08-24 14:59:31]

Description was changed from

==========
RTCStatsReport: Take ownership of other's stats with TakeMembersFrom

This would make it possible to gather stats on multiple threads, store
the results in multiple reports and to merge the results.

Added rtcstatsreport_unittest.cc, moving a RTCStatsReport-related test
from rtcstats_unittest.cc. Added more unittests covering the order of
stats and TakeMembersFrom.

BUG=chromium:627816
==========

to

==========
RTCStatsReport: Take ownership of other's stats with TakeMembersFrom

This would make it possible to gather stats on multiple threads, store
the results in multiple reports and to merge the results.

Added rtcstatsreport_unittest.cc, moving a RTCStatsReport-related test
from rtcstats_unittest.cc. Added more unittests covering the order of
stats and TakeMembersFrom.

Also changed RTCStatsReport[] to RTCStatsReport::Get to avoid
confusion with other usages of the [] operator.

BUG=chromium:627816
==========

[hbos, 2016-08-24 15:01:30]

Please take (another) look hta, glaznev.

https://codereview.webrtc.org/2278433003/diff/1/webrtc/api/rtcstatsreport.h
File webrtc/api/rtcstatsreport.h (right):

https://codereview.webrtc.org/2278433003/diff/1/webrtc/api/rtcstatsreport.h#n...
webrtc/api/rtcstatsreport.h:62: void Scavenge(rtc::scoped_refptr<RTCStatsReport>
victim);
On 2016/08/24 13:54:55, hta-webrtc wrote:
> Suggest using less carnivorous language - what about MoveMembersFrom(victim)
or
> TakeMembersFrom(victim)?

NomNomNom(victim) could have worked too. But OK, TakeMembersFrom(victim) it is.

https://codereview.webrtc.org/2278433003/diff/1/webrtc/stats/rtcstatsreport.cc
File webrtc/stats/rtcstatsreport.cc (right):

https://codereview.webrtc.org/2278433003/diff/1/webrtc/stats/rtcstatsreport.c...
webrtc/stats/rtcstatsreport.cc:69: const RTCStats*
RTCStatsReport::operator[](const std::string& id) const {
On 2016/08/24 13:54:55, hta-webrtc wrote:
> (missed this previously) strongly suggest replacing [] with a simple getter.
The
> semantics of [] in other places in C++ are bad enough that we shouldn't be
using
> the symbol unless that's the precise semantics we want.

Changed to "Get".

https://codereview.webrtc.org/2278433003/diff/1/webrtc/stats/rtcstatsreport_u...
File webrtc/stats/rtcstatsreport_unittest.cc (right):

https://codereview.webrtc.org/2278433003/diff/1/webrtc/stats/rtcstatsreport_u...
webrtc/stats/rtcstatsreport_unittest.cc:42: RTCStatsMember<int32_t> dummy;
On 2016/08/24 13:54:55, hta-webrtc wrote:
> Suggest using different member variables in the different types.
> 

Done.

[hta-webrtc, 2016-08-26 08:50:31]

lgtm

https://codereview.webrtc.org/2278433003/diff/20001/webrtc/stats/rtcstatsrepo...
File webrtc/stats/rtcstatsreport_unittest.cc (right):

https://codereview.webrtc.org/2278433003/diff/20001/webrtc/stats/rtcstatsrepo...
webrtc/stats/rtcstatsreport_unittest.cc:94: TEST(RTCStatsReport, StatsOrder) {
This test verifies a property of RTCStatsReport that isn't mentioned in its API.
Can you add a statement to the .h file that the elements are returned in
lexicographic order on key?

(if we were to use a hashmap to hold them rather than std::map, this property
would change - but this is documented by implementation leaking into interface
due to C++ limitations, I'd prefer it to be explict).

[AlexG, 2016-08-26 17:49:32]

lgtm

[hbos, 2016-08-29 09:52:46]

https://codereview.webrtc.org/2278433003/diff/20001/webrtc/stats/rtcstatsrepo...
File webrtc/stats/rtcstatsreport_unittest.cc (right):

https://codereview.webrtc.org/2278433003/diff/20001/webrtc/stats/rtcstatsrepo...
webrtc/stats/rtcstatsreport_unittest.cc:94: TEST(RTCStatsReport, StatsOrder) {
On 2016/08/26 08:50:31, hta-webrtc wrote:
> This test verifies a property of RTCStatsReport that isn't mentioned in its
API.
> Can you add a statement to the .h file that the elements are returned in
> lexicographic order on key?
> 
> (if we were to use a hashmap to hold them rather than std::map, this property
> would change - but this is documented by implementation leaking into interface
> due to C++ limitations, I'd prefer it to be explict).

"Objects implementing an interface that is declared to be maplike represent an
ordered list of key–value pairs known as its map entries."
https://heycam.github.io/webidl/#idl-maplike

I've interpreted ordered list of key-value pairs as ordered by key, and the key
being a string as lexiographical order. Is this not what is meant?

[hbos, 2016-08-29 11:58:42]

https://codereview.webrtc.org/2278433003/diff/20001/webrtc/stats/rtcstatsrepo...
File webrtc/stats/rtcstatsreport_unittest.cc (right):

https://codereview.webrtc.org/2278433003/diff/20001/webrtc/stats/rtcstatsrepo...
webrtc/stats/rtcstatsreport_unittest.cc:94: TEST(RTCStatsReport, StatsOrder) {
On 2016/08/29 09:52:45, hbos wrote:
> On 2016/08/26 08:50:31, hta-webrtc wrote:
> > This test verifies a property of RTCStatsReport that isn't mentioned in its
> API.
> > Can you add a statement to the .h file that the elements are returned in
> > lexicographic order on key?
> > 
> > (if we were to use a hashmap to hold them rather than std::map, this
property
> > would change - but this is documented by implementation leaking into
interface
> > due to C++ limitations, I'd prefer it to be explict).
> 
> "Objects implementing an interface that is declared to be maplike represent an
> ordered list of key–value pairs known as its map entries."
> https://heycam.github.io/webidl/#idl-maplike
> 
> I've interpreted ordered list of key-value pairs as ordered by key, and the
key
> being a string as lexiographical order. Is this not what is meant?

Done, added a comment. Talked offline, ordered in that context typically just
means that the order is consistent, without specifying how it is ordered.

[hbos, 2016-08-29 11:58:47]

The CQ bit was checked by hbos@webrtc.org

[hbos, 2016-08-29 11:58:47]

The patchset sent to the CQ was uploaded after l-g-t-m from glaznev@webrtc.org,
hta@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2278433003/#ps40001 (title:
"Added comment")

[commit-bot: I haz the power, 2016-08-29 11:58:52]

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-08-29 13:59:33]

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

[commit-bot: I haz the power, 2016-08-29 13:59:34]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[hbos, 2016-08-29 14:13:00]

Description was changed from

==========
RTCStatsReport: Take ownership of other's stats with TakeMembersFrom

This would make it possible to gather stats on multiple threads, store
the results in multiple reports and to merge the results.

Added rtcstatsreport_unittest.cc, moving a RTCStatsReport-related test
from rtcstats_unittest.cc. Added more unittests covering the order of
stats and TakeMembersFrom.

Also changed RTCStatsReport[] to RTCStatsReport::Get to avoid
confusion with other usages of the [] operator.

BUG=chromium:627816
==========

to

==========
RTCStatsReport: Take ownership of other's stats with TakeMembersFrom

This would make it possible to gather stats on multiple threads, store
the results in multiple reports and to merge the results.

Added rtcstatsreport_unittest.cc, moving a RTCStatsReport-related test
from rtcstats_unittest.cc. Added more unittests covering the order of
stats and TakeMembersFrom.

Also changed RTCStatsReport[] to RTCStatsReport::Get to avoid
confusion with other usages of the [] operator.

BUG=chromium:627816
NOTRY=True
==========

[hbos, 2016-08-29 14:13:09]

The CQ bit was checked by hbos@webrtc.org

[commit-bot: I haz the power, 2016-08-29 14:13: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-08-29 14:20:37]

Description was changed from

==========
RTCStatsReport: Take ownership of other's stats with TakeMembersFrom

This would make it possible to gather stats on multiple threads, store
the results in multiple reports and to merge the results.

Added rtcstatsreport_unittest.cc, moving a RTCStatsReport-related test
from rtcstats_unittest.cc. Added more unittests covering the order of
stats and TakeMembersFrom.

Also changed RTCStatsReport[] to RTCStatsReport::Get to avoid
confusion with other usages of the [] operator.

BUG=chromium:627816
NOTRY=True
==========

to

==========
RTCStatsReport: Take ownership of other's stats with TakeMembersFrom

This would make it possible to gather stats on multiple threads, store
the results in multiple reports and to merge the results.

Added rtcstatsreport_unittest.cc, moving a RTCStatsReport-related test
from rtcstats_unittest.cc. Added more unittests covering the order of
stats and TakeMembersFrom.

Also changed RTCStatsReport[] to RTCStatsReport::Get to avoid
confusion with other usages of the [] operator.

BUG=chromium:627816
NOTRY=True
==========

[commit-bot: I haz the power, 2016-08-29 14:20:39]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-29 14:20:46]

Description was changed from

==========
RTCStatsReport: Take ownership of other's stats with TakeMembersFrom

This would make it possible to gather stats on multiple threads, store
the results in multiple reports and to merge the results.

Added rtcstatsreport_unittest.cc, moving a RTCStatsReport-related test
from rtcstats_unittest.cc. Added more unittests covering the order of
stats and TakeMembersFrom.

Also changed RTCStatsReport[] to RTCStatsReport::Get to avoid
confusion with other usages of the [] operator.

BUG=chromium:627816
NOTRY=True
==========

to

==========
RTCStatsReport: Take ownership of other's stats with TakeMembersFrom

This would make it possible to gather stats on multiple threads, store
the results in multiple reports and to merge the results.

Added rtcstatsreport_unittest.cc, moving a RTCStatsReport-related test
from rtcstats_unittest.cc. Added more unittests covering the order of
stats and TakeMembersFrom.

Also changed RTCStatsReport[] to RTCStatsReport::Get to avoid
confusion with other usages of the [] operator.

BUG=chromium:627816
NOTRY=True

Committed: https://crrev.com/6d183ac27a6755497e35f197b8aaf41d6319aeaf
Cr-Commit-Position: refs/heads/master@{#13957}
==========

[commit-bot: I haz the power, 2016-08-29 14:20:47]

Patchset 3 (id:??) landed as
https://crrev.com/6d183ac27a6755497e35f197b8aaf41d6319aeaf
Cr-Commit-Position: refs/heads/master@{#13957}