[webkit-reviews] review granted: [Bug 170024] Fix framesEncoded/framesDecoded RTC stats : [Attachment 305226] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted youenn fablet
<youennf at gmail.com>'s request for review:
Bug 170024: Fix framesEncoded/framesDecoded RTC stats
https://bugs.webkit.org/show_bug.cgi?id=170024

Attachment 305226: Patch

https://bugs.webkit.org/attachment.cgi?id=305226&action=review




--- Comment #2 from Eric Carlson <eric.carlson at apple.com> ---
Comment on attachment 305226
  --> https://bugs.webkit.org/attachment.cgi?id=305226
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305226&action=review

> LayoutTests/webrtc/video-stats.html:86
> +	   return waitFor(1000);

1) hard coded timeout seems like a recipe for a flakey test, and 2) this means
the test will always take a full second to run. Is there any way to have it
check for completion instead of always waiting a second?

> LayoutTests/webrtc/video-stats.html:95
> +	   assert_true(stats.timestamp > statsFirstConnection.timestamp,
"Timestamp should have increased");
> +	   assert_true(stats.framesEncoded >
statsFirstConnection.framesEncoded, "Number of encoded frames should have
increased");
> +	   return getInboundRTPStats(secondConnection);
> +    }).then((stats) => {
> +	   assert_true(stats.timestamp > statsSecondConnection.timestamp,
"Timestamp should have increased");
> +	   assert_true(stats.framesDecoded >
statsSecondConnection.framesDecoded, "Number of decoded frames should have
increased");

Having the same failure message for two different tests will make this harder
to debug if it fails. Can you make the messages different, eg. "Timestamp
should have increased in first connection stats"?