Bug 170024

Created attachment 305226 [details] Patch

Comment on attachment 305226 [details] 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"?

Thanks for the 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? We need to let some time for the "cached stats report" to be obsolete and a new one to be created. But we also need to ensure there are some new frames decoded and encoded to ensure we are not flaky. 1000 might be a bit too large, 100 is a minimum. I will reduce it to 200. > > > 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"? Right, will do

Created attachment 305285 [details] Patch for landing

Comment on attachment 305285 [details] Patch for landing Clearing flags on attachment: 305285 Committed r214348: <http://trac.webkit.org/changeset/214348>

All reviewed patches have been landed. Closing bug.