WebRTCStats added, retaining type info when surfacing WebRTC stats into Blink

WebRTCStats added, retaining type info when surfacing WebRTC stats into Blink.

Previously RTCStatsResponseBase and impls had addReport and addStatistic
methods, adding stats as string key-string value pairs. These are replaced by
addStats(const WebRTCStats&). WebRTCStats describe a stats report and has a
member iterator.

The members' names and values can be converted to strings and be used like
addReport and addStatistic was previously used. RTCStatsResponse still uses this
functionality.

Additionally, the members are described with type information and has getters
for each type. The WebRTCStatsType and WebRTCStatsMemberName enums will be used
to identify individual stats. In follow-up CLs where .idl dictionaries are added
for each RTCStats in the spec, the member and type information will be used to
translate WebRTCStats into RTCStats-derived dictionaries.

Existing getStats tests use mocking. (blink_tests' RTCPeerConnection-stats.html
use a mock WebRTCPeerConnectionHandler, content_unittests'
RTCPeerConnectionHandlerTest.GetStats* use a mock LocalRTCStatsRequest). This CL
adds a getStats browser_tests that make sure getStats can be called and result
iterated without mocking components (integration test).

BUG=627816
Committed: https://crrev.com/36e436520d6835e4f4c48d0ab57b837197104d1d
Cr-Commit-Position: refs/heads/master@{#408957}

[hbos_chromium, 2016-07-27 08:56:54]

The CQ bit was checked by hbos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 08:57:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 09:06:19]

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

[commit-bot: I haz the power, 2016-07-27 09:06:20]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[hbos_chromium, 2016-07-27 10:01:52]

The CQ bit was checked by hbos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 10:02:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hbos_chromium, 2016-07-27 10:03:08]

Description was changed from

==========
WebRTCStats added, retaining type info when surfacing webrtc stats into blink.

BUG=
==========

to

==========
WebRTCStats added, retaining type info when surfacing WebRTC stats into Blink.

Previously RTCStatsResponseBase and impls had addReport and addStatistic
methods, adding stats as string key-string value pairs. These are replaced by
addStats(const WebRTCStats&). WebRTCStats describe a stats report and has a
member iterator.

The members' names and values can be converted to strings and be used like
addReport and addStatistic was previously used. RTCStatsResponse still uses this
functionality.

Additionally, the members are described with type information and has getters
for each type. The WebRTCStatsType and WebRTCStatsMemberName enums will be used
to identify individual stats. In follow-up CLs where .idl dictionaries are added
for each RTCStats in the spec, the member and type information will be used to
translate WebRTCStats into RTCStats-derived dictionaries.

Existing getStats tests use mocking. (blink_tests' RTCPeerConnection-stats.html
use a mock WebRTCPeerConnectionHandler, content_unittests'
RTCPeerConnectionHandlerTest.GetStats* use a mock LocalRTCStatsRequest). This CL
adds a getStats browser_tests that make sure getStats can be called and result
iterated without mocking components (integration test).

BUG=627816
==========

[commit-bot: I haz the power, 2016-07-27 10:32:14]

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

[commit-bot: I haz the power, 2016-07-27 10:32:15]

Dry run: Try jobs failed on following builders:
  win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)

[hbos_chromium, 2016-07-27 11:31:36]

The CQ bit was checked by hbos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 11:31:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hbos_chromium, 2016-07-27 12:03:13]

hbos@chromium.org changed reviewers:
+ mkwst@chromium.org, phoglund@chromium.org, tommi@chromium.org

[hbos_chromium, 2016-07-27 12:03:14]

PTAL phoglund, tommi, mkwst.

phoglund for webrtc_browsertest* and peerconnection.js.

tommi/mkwst for everything else.
(mkwst owner of components/test_runner, third_party/WebKit/Source/platform,
third_party/WebKit/public
tommi owner of webrtc stuff.)

[phoglund_chromium, 2016-07-27 12:52:10]

https://codereview.chromium.org/2186853002/diff/60001/chrome/browser/media/we...
File chrome/browser/media/webrtc_browsertest.cc (right):

https://codereview.chromium.org/2186853002/diff/60001/chrome/browser/media/we...
chrome/browser/media/webrtc_browsertest.cc:60: bool get_stats = false) {
Instead of adding a new parameter, maybe inline this function body to your new
test and just call stats verification directly from there. It's so few lines of
code it's not worth polluting this helper with more parameters.

https://codereview.chromium.org/2186853002/diff/60001/chrome/browser/media/we...
File chrome/browser/media/webrtc_browsertest_base.h (right):

https://codereview.chromium.org/2186853002/diff/60001/chrome/browser/media/we...
chrome/browser/media/webrtc_browsertest_base.h:169: void
GetStats(content::WebContents* tab) const;
The name is misleading since it doesn't really get stats (at least not from the
perspective of the c++ test). It rather verifies that at stats reports are
generated correctly. Maybe call it VerifyStatsGenerated?

https://codereview.chromium.org/2186853002/diff/60001/chrome/test/data/webrtc...
File chrome/test/data/webrtc/peerconnection.js (right):

https://codereview.chromium.org/2186853002/diff/60001/chrome/test/data/webrtc...
chrome/test/data/webrtc/peerconnection.js:305: statsStr += '\nReport #' + i + ':
' + report.id + ' (type: ' +
Maybe call it statsText or result? It appears this is only used for debugging. I
think you should either analyze the string and check it contains what you
expected, or drop the whole thing from this test. The purpose of this test isn't
to debug stats but rather watch for regressions. I think you're better served by
a test page for debugging.

https://codereview.chromium.org/2186853002/diff/60001/chrome/test/data/webrtc...
chrome/test/data/webrtc/peerconnection.js:318: returnToTest('ok');
ok-got-stats perhaps?

[hbos_chromium, 2016-07-27 13:50:24]

The CQ bit was checked by hbos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-27 13:50:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hbos_chromium, 2016-07-27 13:50:32]

PTAL phoglund.

https://codereview.chromium.org/2186853002/diff/60001/chrome/browser/media/we...
File chrome/browser/media/webrtc_browsertest.cc (right):

https://codereview.chromium.org/2186853002/diff/60001/chrome/browser/media/we...
chrome/browser/media/webrtc_browsertest.cc:60: bool get_stats = false) {
On 2016/07/27 12:52:10, phoglund_chrome wrote:
> Instead of adding a new parameter, maybe inline this function body to your new
> test and just call stats verification directly from there. It's so few lines
of
> code it's not worth polluting this helper with more parameters.

Done.

https://codereview.chromium.org/2186853002/diff/60001/chrome/browser/media/we...
File chrome/browser/media/webrtc_browsertest_base.h (right):

https://codereview.chromium.org/2186853002/diff/60001/chrome/browser/media/we...
chrome/browser/media/webrtc_browsertest_base.h:169: void
GetStats(content::WebContents* tab) const;
On 2016/07/27 12:52:10, phoglund_chrome wrote:
> The name is misleading since it doesn't really get stats (at least not from
the
> perspective of the c++ test). It rather verifies that at stats reports are
> generated correctly. Maybe call it VerifyStatsGenerated?

Done.

https://codereview.chromium.org/2186853002/diff/60001/chrome/test/data/webrtc...
File chrome/test/data/webrtc/peerconnection.js (right):

https://codereview.chromium.org/2186853002/diff/60001/chrome/test/data/webrtc...
chrome/test/data/webrtc/peerconnection.js:305: statsStr += '\nReport #' + i + ':
' + report.id + ' (type: ' +
On 2016/07/27 12:52:10, phoglund_chrome wrote:
> Maybe call it statsText or result? It appears this is only used for debugging.
I
> think you should either analyze the string and check it contains what you
> expected, or drop the whole thing from this test. The purpose of this test
isn't
> to debug stats but rather watch for regressions. I think you're better served
by
> a test page for debugging.

Done.

https://codereview.chromium.org/2186853002/diff/60001/chrome/test/data/webrtc...
chrome/test/data/webrtc/peerconnection.js:318: returnToTest('ok');
On 2016/07/27 12:52:10, phoglund_chrome wrote:
> ok-got-stats perhaps?

Done.

[commit-bot: I haz the power, 2016-07-27 14:50:45]

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

[commit-bot: I haz the power, 2016-07-27 14:50:46]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tommi (sloooow) - chröme, 2016-07-27 18:38:40]

rs lgtm once patrik is happy and below nits fixed

https://codereview.chromium.org/2186853002/diff/100001/chrome/browser/media/w...
File chrome/browser/media/webrtc_browsertest_base.cc (right):

https://codereview.chromium.org/2186853002/diff/100001/chrome/browser/media/w...
chrome/browser/media/webrtc_browsertest_base.cc:491: EXPECT_EQ("ok-got-stats",
response) << "Failed to get stats: " << response;
doesn't EXPECT_EQ already print out the contents of the variables?

https://codereview.chromium.org/2186853002/diff/100001/chrome/test/data/webrt...
File chrome/test/data/webrtc/peerconnection.js (right):

https://codereview.chromium.org/2186853002/diff/100001/chrome/test/data/webrt...
chrome/test/data/webrtc/peerconnection.js:307: if (typeof statValue !==
'string')
!= should be enough.  !== is used to do equality+type checks.  The typeof
operator always returns a string, so verifying the return type of 'typeof' is
misleading.  It's actually not about checking statValue.

https://codereview.chromium.org/2186853002/diff/100001/components/test_runner...
File components/test_runner/mock_webrtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/2186853002/diff/100001/components/test_runner...
components/test_runner/mock_webrtc_peer_connection_handler.cc:43: const
std::vector<std::pair<std::string, std::string>>& values)
I think that in cases like this, it's more appropriate to pass in a const* than
a const&

https://codereview.chromium.org/2186853002/diff/100001/content/renderer/media...
File content/renderer/media/rtc_peer_connection_handler_unittest.cc (right):

https://codereview.chromium.org/2186853002/diff/100001/content/renderer/media...
content/renderer/media/rtc_peer_connection_handler_unittest.cc:79:
stats.iterator()); !member->isEnd(); member->next()) {
is this what git cl format did?

[phoglund_chromium, 2016-07-28 06:24:22]

lgtm

[phoglund_chromium, 2016-07-28 06:25:08]

Note, tommi, I only reviewed the tests here.

[tommi (sloooow) - chröme, 2016-07-28 07:35:01]

On 2016/07/28 06:25:08, phoglund_chrome wrote:
> Note, tommi, I only reviewed the tests here.

yup, I took a look at the rest.

[hbos_chromium, 2016-07-28 08:26:11]

The CQ bit was checked by hbos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-28 08:26:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hbos_chromium, 2016-07-28 08:27:45]

Please take a look, Mike West.

https://codereview.chromium.org/2186853002/diff/100001/chrome/browser/media/w...
File chrome/browser/media/webrtc_browsertest_base.cc (right):

https://codereview.chromium.org/2186853002/diff/100001/chrome/browser/media/w...
chrome/browser/media/webrtc_browsertest_base.cc:491: EXPECT_EQ("ok-got-stats",
response) << "Failed to get stats: " << response;
On 2016/07/27 18:38:40, tommi-chrömium wrote:
> doesn't EXPECT_EQ already print out the contents of the variables?

You're right, fixed here and above while I'm at it.

https://codereview.chromium.org/2186853002/diff/100001/chrome/test/data/webrt...
File chrome/test/data/webrtc/peerconnection.js (right):

https://codereview.chromium.org/2186853002/diff/100001/chrome/test/data/webrt...
chrome/test/data/webrtc/peerconnection.js:307: if (typeof statValue !==
'string')
On 2016/07/27 18:38:40, tommi-chrömium wrote:
> != should be enough.  !== is used to do equality+type checks.  The typeof
> operator always returns a string, so verifying the return type of 'typeof' is
> misleading.  It's actually not about checking statValue.

Done.

https://codereview.chromium.org/2186853002/diff/100001/components/test_runner...
File components/test_runner/mock_webrtc_peer_connection_handler.cc (right):

https://codereview.chromium.org/2186853002/diff/100001/components/test_runner...
components/test_runner/mock_webrtc_peer_connection_handler.cc:43: const
std::vector<std::pair<std::string, std::string>>& values)
On 2016/07/27 18:38:40, tommi-chrömium wrote:
> I think that in cases like this, it's more appropriate to pass in a const*
than
> a const&

Done.

https://codereview.chromium.org/2186853002/diff/100001/content/renderer/media...
File content/renderer/media/rtc_peer_connection_handler_unittest.cc (right):

https://codereview.chromium.org/2186853002/diff/100001/content/renderer/media...
content/renderer/media/rtc_peer_connection_handler_unittest.cc:79:
stats.iterator()); !member->isEnd(); member->next()) {
On 2016/07/27 18:38:40, tommi-chrömium wrote:
> is this what git cl format did?

No. Done. I ran git cl format now and it put a newline after ...iterator());

[commit-bot: I haz the power, 2016-07-28 10:14:51]

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

[commit-bot: I haz the power, 2016-07-28 10:14:52]

Dry run: This issue passed the CQ dry run.

[hbos_chromium, 2016-07-29 14:51:52]

Friendly ping, mkwst.

[Mike West, 2016-08-01 11:42:40]

LGTM.

[hbos_chromium, 2016-08-01 11:57:43]

The CQ bit was checked by hbos@chromium.org

[hbos_chromium, 2016-08-01 11:57:43]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org,
phoglund@chromium.org
Link to the patchset: https://codereview.chromium.org/2186853002/#ps120001
(title: "Addressed tommi's comments")

[commit-bot: I haz the power, 2016-08-01 11:57:56]

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

[commit-bot: I haz the power, 2016-08-01 13:10:27]

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

[commit-bot: I haz the power, 2016-08-01 13:10:29]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[hbos_chromium, 2016-08-01 13:48:45]

The CQ bit was checked by hbos@chromium.org

[commit-bot: I haz the power, 2016-08-01 13:48:56]

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

[commit-bot: I haz the power, 2016-08-01 14:18:30]

Description was changed from

==========
WebRTCStats added, retaining type info when surfacing WebRTC stats into Blink.

Previously RTCStatsResponseBase and impls had addReport and addStatistic
methods, adding stats as string key-string value pairs. These are replaced by
addStats(const WebRTCStats&). WebRTCStats describe a stats report and has a
member iterator.

The members' names and values can be converted to strings and be used like
addReport and addStatistic was previously used. RTCStatsResponse still uses this
functionality.

Additionally, the members are described with type information and has getters
for each type. The WebRTCStatsType and WebRTCStatsMemberName enums will be used
to identify individual stats. In follow-up CLs where .idl dictionaries are added
for each RTCStats in the spec, the member and type information will be used to
translate WebRTCStats into RTCStats-derived dictionaries.

Existing getStats tests use mocking. (blink_tests' RTCPeerConnection-stats.html
use a mock WebRTCPeerConnectionHandler, content_unittests'
RTCPeerConnectionHandlerTest.GetStats* use a mock LocalRTCStatsRequest). This CL
adds a getStats browser_tests that make sure getStats can be called and result
iterated without mocking components (integration test).

BUG=627816
==========

to

==========
WebRTCStats added, retaining type info when surfacing WebRTC stats into Blink.

Previously RTCStatsResponseBase and impls had addReport and addStatistic
methods, adding stats as string key-string value pairs. These are replaced by
addStats(const WebRTCStats&). WebRTCStats describe a stats report and has a
member iterator.

The members' names and values can be converted to strings and be used like
addReport and addStatistic was previously used. RTCStatsResponse still uses this
functionality.

Additionally, the members are described with type information and has getters
for each type. The WebRTCStatsType and WebRTCStatsMemberName enums will be used
to identify individual stats. In follow-up CLs where .idl dictionaries are added
for each RTCStats in the spec, the member and type information will be used to
translate WebRTCStats into RTCStats-derived dictionaries.

Existing getStats tests use mocking. (blink_tests' RTCPeerConnection-stats.html
use a mock WebRTCPeerConnectionHandler, content_unittests'
RTCPeerConnectionHandlerTest.GetStats* use a mock LocalRTCStatsRequest). This CL
adds a getStats browser_tests that make sure getStats can be called and result
iterated without mocking components (integration test).

BUG=627816
==========

[commit-bot: I haz the power, 2016-08-01 14:18:31]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-08-01 14:20:03]

Description was changed from

==========
WebRTCStats added, retaining type info when surfacing WebRTC stats into Blink.

Previously RTCStatsResponseBase and impls had addReport and addStatistic
methods, adding stats as string key-string value pairs. These are replaced by
addStats(const WebRTCStats&). WebRTCStats describe a stats report and has a
member iterator.

The members' names and values can be converted to strings and be used like
addReport and addStatistic was previously used. RTCStatsResponse still uses this
functionality.

Additionally, the members are described with type information and has getters
for each type. The WebRTCStatsType and WebRTCStatsMemberName enums will be used
to identify individual stats. In follow-up CLs where .idl dictionaries are added
for each RTCStats in the spec, the member and type information will be used to
translate WebRTCStats into RTCStats-derived dictionaries.

Existing getStats tests use mocking. (blink_tests' RTCPeerConnection-stats.html
use a mock WebRTCPeerConnectionHandler, content_unittests'
RTCPeerConnectionHandlerTest.GetStats* use a mock LocalRTCStatsRequest). This CL
adds a getStats browser_tests that make sure getStats can be called and result
iterated without mocking components (integration test).

BUG=627816
==========

to

==========
WebRTCStats added, retaining type info when surfacing WebRTC stats into Blink.

Previously RTCStatsResponseBase and impls had addReport and addStatistic
methods, adding stats as string key-string value pairs. These are replaced by
addStats(const WebRTCStats&). WebRTCStats describe a stats report and has a
member iterator.

The members' names and values can be converted to strings and be used like
addReport and addStatistic was previously used. RTCStatsResponse still uses this
functionality.

Additionally, the members are described with type information and has getters
for each type. The WebRTCStatsType and WebRTCStatsMemberName enums will be used
to identify individual stats. In follow-up CLs where .idl dictionaries are added
for each RTCStats in the spec, the member and type information will be used to
translate WebRTCStats into RTCStats-derived dictionaries.

Existing getStats tests use mocking. (blink_tests' RTCPeerConnection-stats.html
use a mock WebRTCPeerConnectionHandler, content_unittests'
RTCPeerConnectionHandlerTest.GetStats* use a mock LocalRTCStatsRequest). This CL
adds a getStats browser_tests that make sure getStats can be called and result
iterated without mocking components (integration test).

BUG=627816

Committed: https://crrev.com/36e436520d6835e4f4c48d0ab57b837197104d1d
Cr-Commit-Position: refs/heads/master@{#408957}
==========

[commit-bot: I haz the power, 2016-08-01 14:20:05]

Patchset 7 (id:??) landed as
https://crrev.com/36e436520d6835e4f4c48d0ab57b837197104d1d
Cr-Commit-Position: refs/heads/master@{#408957}