Initial implementation of onbufferedamountlow

Initial implementation of onbufferedamountlow

BUG=496700
Committed: https://crrev.com/41bdc151d985678d2bbd91ffa883f4a40213b76d
git-svn-id: svn://svn.chromium.org/blink/trunk@199694 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[bemasc, 2015-06-05 19:29:47]

bemasc@chromium.org changed reviewers:
+ jiayl@chromium.org, lally@chromium.org, mkwst@chromium.org,
pthatcher@chromium.org, tommi@chromium.org

[bemasc, 2015-06-05 19:29:49]

tommi@: Please review for OWNERS on Source/modules/mediastream/RTCDatachannel.*

mkwst@: Please review for OWNERS on EventTypeNames.in and
public/platform/WebRTCDataChannelHandlerClient.h.

Note: Not to be committed until the corresponding WebRTC and Chromium changes
have also been reviewed.

[Mike West, 2015-06-07 13:48:16]

1. Did you send an intent to implement/ship?
2. Tests?

[bemasc, 2015-06-07 14:07:50]

1. I didn't send an intent-to-implement.  I figured this was a trivial
feature.  I also made sure it landed in the W3C spec first:
https://github.com/w3c/webrtc-pc/pull/233.

2. So far as I could tell, we do not have any framework for testing
bufferedamount behavior. Our implementation of bufferedamount is highly
nondeterministic and multithreaded, so testing it is nontrivial.  If you
have advice on an appropriate test pattern for this change, I'll be happy
to implement it.

On Sun, Jun 7, 2015 at 9:48 AM, <mkwst@chromium.org> wrote:

> 1. Did you send an intent to implement/ship?
> 2. Tests?
>
> https://codereview.chromium.org/1171553002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[tommi (sloooow) - chröme, 2015-06-08 08:57:10]

lgtm

[Mike West, 2015-06-08 09:28:46]

On 2015/06/07 at 14:07:50, bemasc wrote:
> 1. I didn't send an intent-to-implement.  I figured this was a trivial
> feature.  I also made sure it landed in the W3C spec first:
> https://github.com/w3c/webrtc-pc/pull/233.

Ok. If you and the module's OWNERS see it as falling under the general WebRTC
banner, that's fine.

> 2. So far as I could tell, we do not have any framework for testing
> bufferedamount behavior. Our implementation of bufferedamount is highly
> nondeterministic and multithreaded, so testing it is nontrivial.  If you
> have advice on an appropriate test pattern for this change, I'll be happy
> to implement it.

You could add unit tests in a new
`Source/modules/mediastream/RTCDataChannelTest.cpp` file which verify that
`scheduleDispatchEvent` is called at the right time when
`didDecreaseBufferedAmount` is called with various `previousAmount` and
`bufferedAmount()` values. Alternatively, you could add an internal API to the
test runner to trigger the condition in layout tests. I'd be happy either way.

LGTM when a test is in place.

[pthatcher2, 2015-06-08 22:35:27]

lgtm

[bemasc, 2015-06-09 15:48:57]

bemasc@chromium.org changed reviewers:
+ tkent@chromium.org

[bemasc, 2015-06-09 15:51:53]

On 2015/06/08 09:28:46, Mike West wrote:
> On 2015/06/07 at 14:07:50, bemasc wrote:
> > 1. I didn't send an intent-to-implement.  I figured this was a trivial
> > feature.  I also made sure it landed in the W3C spec first:
> > https://github.com/w3c/webrtc-pc/pull/233.
> 
> Ok. If you and the module's OWNERS see it as falling under the general WebRTC
> banner, that's fine.
> 
> > 2. So far as I could tell, we do not have any framework for testing
> > bufferedamount behavior. Our implementation of bufferedamount is highly
> > nondeterministic and multithreaded, so testing it is nontrivial.  If you
> > have advice on an appropriate test pattern for this change, I'll be happy
> > to implement it.
> 
> You could add unit tests in a new
> `Source/modules/mediastream/RTCDataChannelTest.cpp` file which verify that
> `scheduleDispatchEvent` is called at the right time when
> `didDecreaseBufferedAmount` is called with various `previousAmount` and
> `bufferedAmount()` values.

OK, I did this.  I think it should be uncontroversial, but someone should take a
look anyway.

I've added tkent@ as a reviewer, because the unit tests require using "friend
class", and it looks like tkent@ is in charge of efforts to use FRIEND_TEST
instead, but that doesn't appear to be ready.

> Alternatively, you could add an internal API to the
> test runner to trigger the condition in layout tests. I'd be happy either way.
> 
> LGTM when a test is in place.

[tkent, 2015-06-09 23:42:40]

On 2015/06/07 14:07:50, bemasc wrote:
> 1. I didn't send an intent-to-implement.  I figured this was a trivial
> feature.  I also made sure it landed in the W3C spec first:
> https://github.com/w3c/webrtc-pc/pull/233.

Please add an entry of https://www.chromestatus.com/features.

[tkent, 2015-06-09 23:47:20]

On 2015/06/09 15:51:53, bemasc wrote:
> I've added tkent@ as a reviewer, because the unit tests require using "friend
> class", and it looks like tkent@ is in charge of efforts to use FRIEND_TEST
> instead, but that doesn't appear to be ready.

Yeah, it's not ready.  Actually we should use base/gtest_prod_util.h, but Blink
shouldn't depend on base yet.

[tkent, 2015-06-09 23:47:27]

lgtm

https://codereview.chromium.org/1171553002/diff/20001/Source/modules/mediastr...
File Source/modules/mediastream/RTCDataChannelTest.cpp (right):

https://codereview.chromium.org/1171553002/diff/20001/Source/modules/mediastr...
Source/modules/mediastream/RTCDataChannelTest.cpp:33: virtual void
setClient(WebRTCDataChannelHandlerClient* client) { m_client = client; }
Remove |virtual|, and add |override| or |final|.
Same for other virtual member functions.

https://codereview.chromium.org/1171553002/diff/20001/Source/modules/mediastr...
Source/modules/mediastream/RTCDataChannelTest.cpp:72: private:
nit: add a blank line before |private:|

[bemasc, 2015-06-10 14:21:13]

> Please add an entry of https://www.chromestatus.com/features.

Done.  https://www.chromestatus.com/features/5515010235170816

On 2015/06/09 23:47:27, tkent wrote:
> lgtm
> 
>
https://codereview.chromium.org/1171553002/diff/20001/Source/modules/mediastr...
> File Source/modules/mediastream/RTCDataChannelTest.cpp (right):
> 
>
https://codereview.chromium.org/1171553002/diff/20001/Source/modules/mediastr...
> Source/modules/mediastream/RTCDataChannelTest.cpp:33: virtual void
> setClient(WebRTCDataChannelHandlerClient* client) { m_client = client; }
> Remove |virtual|, and add |override| or |final|.
> Same for other virtual member functions.

Done.  I went with |override|.  (The class itself is |final|.)

>
https://codereview.chromium.org/1171553002/diff/20001/Source/modules/mediastr...
> Source/modules/mediastream/RTCDataChannelTest.cpp:72: private:
> nit: add a blank line before |private:|

Done.

[tommi (sloooow) - chröme, 2015-06-10 14:28:27]

lgtm

[lally, 2015-06-10 17:59:41]

lgtm as-is, with suggestions.

https://codereview.chromium.org/1171553002/diff/40001/Source/modules/mediastr...
File Source/modules/mediastream/RTCDataChannelTest.cpp (right):

https://codereview.chromium.org/1171553002/diff/40001/Source/modules/mediastr...
Source/modules/mediastream/RTCDataChannelTest.cpp:42:
WebRTCDataChannelHandlerClient::ReadyState state() const override { return
m_state; }
80col.

https://codereview.chromium.org/1171553002/diff/40001/Source/modules/mediastr...
Source/modules/mediastream/RTCDataChannelTest.cpp:104:
EXPECT_EQ("bufferedamountlow",
std::string(channel->m_scheduledEvents.last()->type().utf8().data()));
Consider checking the buffered amount. Also 80col.

[bemasc, 2015-06-10 18:23:16]

On 2015/06/10 17:59:41, lally wrote:
> lgtm as-is, with suggestions.
> 
>
https://codereview.chromium.org/1171553002/diff/40001/Source/modules/mediastr...
> File Source/modules/mediastream/RTCDataChannelTest.cpp (right):
> 
>
https://codereview.chromium.org/1171553002/diff/40001/Source/modules/mediastr...
> Source/modules/mediastream/RTCDataChannelTest.cpp:42:
> WebRTCDataChannelHandlerClient::ReadyState state() const override { return
> m_state; }
> 80col.

Blink doesn't have an 80col rule.

>
https://codereview.chromium.org/1171553002/diff/40001/Source/modules/mediastr...
> Source/modules/mediastream/RTCDataChannelTest.cpp:104:
> EXPECT_EQ("bufferedamountlow",
> std::string(channel->m_scheduledEvents.last()->type().utf8().data()));
> Consider checking the buffered amount. Also 80col.

OK, I added checks for bufferedAmount() throughout.

[bemasc, 2015-07-20 17:31:34]

The CQ bit was checked by bemasc@chromium.org

[bemasc, 2015-07-20 17:31:35]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
pthatcher@chromium.org, tkent@chromium.org, lally@chromium.org,
tommi@chromium.org
Link to the patchset: https://codereview.chromium.org/1171553002/#ps60001
(title: "Check bufferedAmount() throughout RTCDataChannelTest")

[commit-bot: I haz the power, 2015-07-20 17:31:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1171553002/60001

[commit-bot: I haz the power, 2015-07-20 17:34:48]

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

[commit-bot: I haz the power, 2015-07-20 17:34:49]

Try jobs failed on following builders:
  mac_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/63392)

[bemasc, 2015-07-27 19:53:32]

The CQ bit was checked by bemasc@chromium.org

[bemasc, 2015-07-27 19:53:32]

The patchset sent to the CQ was uploaded after l-g-t-m from lally@chromium.org,
tommi@chromium.org, tkent@chromium.org, pthatcher@chromium.org,
mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1171553002/#ps80001
(title: "Merge intervening changes")

[commit-bot: I haz the power, 2015-07-27 19:53:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1171553002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1171553002/80001

[commit-bot: I haz the power, 2015-07-27 20:28:51]

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

[commit-bot: I haz the power, 2015-07-27 20:28:53]

Try jobs failed on following builders:
  win_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[bemasc, 2015-07-28 16:05:29]

The CQ bit was checked by bemasc@chromium.org

[bemasc, 2015-07-28 16:05:29]

The patchset sent to the CQ was uploaded after l-g-t-m from lally@chromium.org,
tommi@chromium.org, tkent@chromium.org, pthatcher@chromium.org,
mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1171553002/#ps100001
(title: "Add MODULES_EXPORT annotation to RTCDataChannel")

[commit-bot: I haz the power, 2015-07-28 16:05:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1171553002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1171553002/100001

[commit-bot: I haz the power, 2015-07-28 16:27:36]

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

[commit-bot: I haz the power, 2015-07-28 16:27:38]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...)

[bemasc, 2015-07-28 18:21:34]

The CQ bit was checked by bemasc@chromium.org

[bemasc, 2015-07-28 18:21:34]

The patchset sent to the CQ was uploaded after l-g-t-m from lally@chromium.org,
tommi@chromium.org, tkent@chromium.org, pthatcher@chromium.org,
mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1171553002/#ps120001
(title: "Minor changes to make trybots pass")

[commit-bot: I haz the power, 2015-07-28 18:22:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1171553002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1171553002/120001

[commit-bot: I haz the power, 2015-07-28 19:02:46]

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

[commit-bot: I haz the power, 2015-07-28 19:02:47]

Try jobs failed on following builders:
  win_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[bemasc, 2015-07-30 00:00:12]

The CQ bit was checked by bemasc@chromium.org

[bemasc, 2015-07-30 00:00:13]

The patchset sent to the CQ was uploaded after l-g-t-m from lally@chromium.org,
tommi@chromium.org, tkent@chromium.org, pthatcher@chromium.org,
mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1171553002/#ps160001
(title: "Remove reference to file that no longer exists")

[commit-bot: I haz the power, 2015-07-30 00:00:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1171553002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1171553002/160001

[commit-bot: I haz the power, 2015-07-30 01:26:21]

Committed patchset #9 (id:160001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=199694

[commit-bot: I haz the power, 2015-09-23 11:54:49]

Patchset 9 (id:??) landed as
https://crrev.com/41bdc151d985678d2bbd91ffa883f4a40213b76d