Closed
Bug 1178091
Opened 10 years ago
Closed 10 years ago
Implement RTC-DataChannel-bufferedAmountLow event
Categories
(Core :: WebRTC: Networking, defect, P2)
Core
WebRTC: Networking
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
backlog | webrtc/webaudio+ |
People
(Reporter: shacharz, Assigned: jesup)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-complete)
Attachments
(2 files)
20.62 KB,
patch
|
smaug
:
review+
drno
:
review+
|
Details | Diff | Splinter Review |
4.13 KB,
patch
|
drno
:
review+
|
Details | Diff | Splinter Review |
Comply to spec:
https://github.com/w3c/webrtc-pc/pull/233/files
Chrome's equivalent issue:
https://code.google.com/p/chromium/issues/detail?id=496700
Updated•10 years ago
|
Component: Web Apps → WebRTC
Product: Firefox → Core
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → rjesup
Blocks: webrtc_spec
backlog: --- → webRTC+
Rank: 25
Component: WebRTC → WebRTC: Networking
Priority: -- → P2
Assignee | ||
Comment 1•10 years ago
|
||
tested locally and works with a modified data_test.html; a second patch is coming to add it to the tests in automation. Note that bufferedAmount can't currently include data buffered in the stack; Michael Tuexen has that on his todo list (see comments)
Attachment #8666376 -
Flags: review?(drno)
Attachment #8666376 -
Flags: review?(bugs)
Assignee | ||
Comment 2•10 years ago
|
||
Attachment #8666381 -
Flags: review?(drno)
Assignee | ||
Updated•10 years ago
|
Rank: 25 → 23
Assignee | ||
Comment 4•10 years ago
|
||
Updated•10 years ago
|
Keywords: dev-doc-needed
Comment 5•10 years ago
|
||
Comment on attachment 8666376 [details] [diff] [review]
Implement RTCDataChannel BufferedAmountLowThreshold and bufferedamountlow event
Please don't add any onfoo properties to .idl, only to .webidl.
And do we need attribute unsigned long bufferedAmountLowThreshold; in the .idl either? .webidl is the thing exposed to scripts these days.
(if you end up modifying .idl, update uuid there.)
Those fixed, r+
Didn't review netwerk/sctp/datachannel/DataChannel.cpp
Attachment #8666376 -
Flags: review?(bugs) → review+
Comment 6•10 years ago
|
||
Comment on attachment 8666376 [details] [diff] [review]
Implement RTCDataChannel BufferedAmountLowThreshold and bufferedamountlow event
Review of attachment 8666376 [details] [diff] [review]:
-----------------------------------------------------------------
LGTM
::: netwerk/sctp/datachannel/DataChannel.cpp
@@ +2640,5 @@
> DataChannel::GetBufferedAmount()
> {
> + size_t buffered = 0;
> + for (auto& buffer : mBufferedData) {
> + buffered += buffer->mLength;
With real paranoia this can already overflow.
Attachment #8666376 -
Flags: review?(drno) → review+
Comment 7•10 years ago
|
||
Comment on attachment 8666381 [details] [diff] [review]
tests for large transfers and onbufferedamountlow event
Review of attachment 8666381 [details] [diff] [review]:
-----------------------------------------------------------------
LGTM
::: dom/media/tests/mochitest/dataChannel.js
@@ +169,5 @@
> ];
>
> +var commandsCheckLargeXfer = [
> + function SEND_BIG_BUFFER(test) {
> + var buffer = new ArrayBuffer(1024*1024);
Would be nice to fill the buffer with some test data rather then just 0's.
Attachment #8666381 -
Flags: review?(drno) → review+
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/23cd3f0edf0a
https://hg.mozilla.org/mozilla-central/rev/40e2d33b759e
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Comment 10•8 years ago
|
||
This was already listed on Firefox 44 for developers, and bufferedAmountLowThreshold was documented, along with onbufferedamountlow, but now bufferedamountlow is documented too. This is now complete.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•