Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 2 months ago by hbos Modified:
4 years, 2 months ago Reviewers:
hta-webrtc, Taylor Brandstetter CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionDataChannel[Interface]::[message/bytes]_[sent/received]() added.
These are required for the RTCDataChannelStats[1] that will be collected
in a follow-up CL.
[1] https://w3c.github.io/webrtc-stats/#dcstats-dict*
BUG=chromium:654927, chromium:627816
Committed: https://crrev.com/84ffdee879c9d13f50288dbf6a2a050b0da95170
Cr-Commit-Position: refs/heads/master@{#14616}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Addressed comments #
Total comments: 4
Patch Set 3 : Addressed comments #
Created: 4 years, 2 months ago
Messages
Total messages: 23 (14 generated)
Description was changed from ========== DataChannel[Interface]::[message/bytes]_[sent/received] added. These are required for the RTCDataChannelStats[1] that will be collected in a follow-up CL. [1] https://w3c.github.io/webrtc-stats/#dcstats-dict* BUG=chromium:654927, chromium:627816 ========== to ========== DataChannel[Interface]::[message/bytes]_[sent/received]() added. These are required for the RTCDataChannelStats[1] that will be collected in a follow-up CL. [1] https://w3c.github.io/webrtc-stats/#dcstats-dict* BUG=chromium:654927, chromium:627816 ==========
hbos@webrtc.org changed reviewers: + deadbeef@webrtc.org, hta@webrtc.org
The CQ bit was checked by hbos@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Please take a look, deadbeef and hta.
lgtm % comments https://codereview.webrtc.org/2413803002/diff/1/webrtc/api/datachannel_unitte... File webrtc/api/datachannel_unittest.cc (right): https://codereview.webrtc.org/2413803002/diff/1/webrtc/api/datachannel_unitte... webrtc/api/datachannel_unittest.cc:213: provider_->set_send_blocked(false); check that messages_sent and bytes_sent are zero at start too. https://codereview.webrtc.org/2413803002/diff/1/webrtc/api/datachannel_unitte... webrtc/api/datachannel_unittest.cc:218: EXPECT_EQ(0U, webrtc_data_channel_->buffered_amount()); Is this 100% reliable, or do we need it to be EXPECT_EQ_WAIT? (i'm fine with it being EXPECT_EQ if you have looked at the implementation and concluded that Send() will always empty the buffer when it's wired like it is. Just afraid of flaky tests.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Please take a look, deadybeef https://codereview.webrtc.org/2413803002/diff/1/webrtc/api/datachannel_unitte... File webrtc/api/datachannel_unittest.cc (right): https://codereview.webrtc.org/2413803002/diff/1/webrtc/api/datachannel_unitte... webrtc/api/datachannel_unittest.cc:213: provider_->set_send_blocked(false); On 2016/10/12 17:25:28, hta-webrtc wrote: > check that messages_sent and bytes_sent are zero at start too. Done. Did the same for messages_received and bytes_received in the other test. https://codereview.webrtc.org/2413803002/diff/1/webrtc/api/datachannel_unitte... webrtc/api/datachannel_unittest.cc:218: EXPECT_EQ(0U, webrtc_data_channel_->buffered_amount()); On 2016/10/12 17:25:28, hta-webrtc wrote: > Is this 100% reliable, or do we need it to be EXPECT_EQ_WAIT? > > (i'm fine with it being EXPECT_EQ if you have looked at the implementation and > concluded that Send() will always empty the buffer when it's wired like it is. > Just afraid of flaky tests.) Yes, but there are interfaces and stuff involved so I changed it to EXPECT_EQ_WAIT to avoid assumptions.
The CQ bit was checked by hbos@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with couple small suggestions https://codereview.webrtc.org/2413803002/diff/20001/webrtc/api/datachannel_un... File webrtc/api/datachannel_unittest.cc (right): https://codereview.webrtc.org/2413803002/diff/20001/webrtc/api/datachannel_un... webrtc/api/datachannel_unittest.cc:222: EXPECT_EQ_WAIT(0U, webrtc_data_channel_->buffered_amount(), 1000); I'd use kDefaultTimeout. Any time a test doesn't actually care about the time - just that something happens eventually in the future - it's good to use a long timeout so the test doesn't fail if running on a slow bot. https://codereview.webrtc.org/2413803002/diff/20001/webrtc/api/datachannel_un... webrtc/api/datachannel_unittest.cc:445: // Receive three buffers while blocked. I'd clarify that this is "while the data channel isn't open", rather than "while blocked".
https://codereview.webrtc.org/2413803002/diff/20001/webrtc/api/datachannel_un... File webrtc/api/datachannel_unittest.cc (right): https://codereview.webrtc.org/2413803002/diff/20001/webrtc/api/datachannel_un... webrtc/api/datachannel_unittest.cc:222: EXPECT_EQ_WAIT(0U, webrtc_data_channel_->buffered_amount(), 1000); On 2016/10/12 19:43:58, Taylor Brandstetter wrote: > I'd use kDefaultTimeout. Any time a test doesn't actually care about the time - > just that something happens eventually in the future - it's good to use a long > timeout so the test doesn't fail if running on a slow bot. Done. https://codereview.webrtc.org/2413803002/diff/20001/webrtc/api/datachannel_un... webrtc/api/datachannel_unittest.cc:445: // Receive three buffers while blocked. On 2016/10/12 19:43:58, Taylor Brandstetter wrote: > I'd clarify that this is "while the data channel isn't open", rather than "while > blocked". Done.
The CQ bit was checked by hbos@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from hta@webrtc.org, deadbeef@webrtc.org Link to the patchset: https://codereview.webrtc.org/2413803002/#ps40001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== DataChannel[Interface]::[message/bytes]_[sent/received]() added. These are required for the RTCDataChannelStats[1] that will be collected in a follow-up CL. [1] https://w3c.github.io/webrtc-stats/#dcstats-dict* BUG=chromium:654927, chromium:627816 ========== to ========== DataChannel[Interface]::[message/bytes]_[sent/received]() added. These are required for the RTCDataChannelStats[1] that will be collected in a follow-up CL. [1] https://w3c.github.io/webrtc-stats/#dcstats-dict* BUG=chromium:654927, chromium:627816 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== DataChannel[Interface]::[message/bytes]_[sent/received]() added. These are required for the RTCDataChannelStats[1] that will be collected in a follow-up CL. [1] https://w3c.github.io/webrtc-stats/#dcstats-dict* BUG=chromium:654927, chromium:627816 ========== to ========== DataChannel[Interface]::[message/bytes]_[sent/received]() added. These are required for the RTCDataChannelStats[1] that will be collected in a follow-up CL. [1] https://w3c.github.io/webrtc-stats/#dcstats-dict* BUG=chromium:654927, chromium:627816 Committed: https://crrev.com/84ffdee879c9d13f50288dbf6a2a050b0da95170 Cr-Commit-Position: refs/heads/master@{#14616} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/84ffdee879c9d13f50288dbf6a2a050b0da95170 Cr-Commit-Position: refs/heads/master@{#14616}
Message was sent while issue was closed.
This caused chromium fyi breakage, fix instead of reverting: https://codereview.webrtc.org/2414613003 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
