Support for onbufferedamountlow

Support for onbufferedamountlow

Original review at https://webrtc-codereview.appspot.com/54679004/

BUG=https://code.google.com/p/chromium/issues/detail?id=496700

Committed: https://crrev.com/0edd50ccb34cc2dc4746137fdce1f5cf66808274
Cr-Commit-Position: refs/heads/master@{#9527}

[bemasc2, 2015-06-24 18:46:59]

bemasc@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[bemasc2, 2015-06-24 18:47:00]

PTAL

[pthatcher1, 2015-06-24 21:44:27]

lgtm

[pthatcher1, 2015-06-24 21:44:30]

The CQ bit was checked by pthatcher@webrtc.org

[commit-bot: I haz the power, 2015-06-24 21:44:44]

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

[commit-bot: I haz the power, 2015-06-24 21:44:45]

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

[commit-bot: I haz the power, 2015-06-24 21:44:46]

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[bemasc2, 2015-06-25 18:23:57]

bemasc@webrtc.org changed reviewers:
+ juberti@chromium.org

[bemasc2, 2015-06-25 18:23:57]

Looks like this needs a Chromium committer bit?  Seems like a bug...

Over to juberti's chromium account for rubber stamp.

[juberti1, 2015-06-25 18:56:21]

juberti@webrtc.org changed reviewers:
+ jiayl@chromium.org, juberti@webrtc.org

[juberti1, 2015-06-25 18:56:23]

lgtm with one style comment.
Jiayang can land for you

https://codereview.webrtc.org/1207613006/diff/20001/talk/app/webrtc/datachann...
File talk/app/webrtc/datachannel_unittest.cc (right):

https://codereview.webrtc.org/1207613006/diff/20001/talk/app/webrtc/datachann...
talk/app/webrtc/datachannel_unittest.cc:39: on_buffered_amount_change_count_(0)
{}
fix indent - 1 initializer per line

[bemasc2, 2015-06-25 19:56:09]

https://codereview.webrtc.org/1207613006/diff/20001/talk/app/webrtc/datachann...
File talk/app/webrtc/datachannel_unittest.cc (right):

https://codereview.webrtc.org/1207613006/diff/20001/talk/app/webrtc/datachann...
talk/app/webrtc/datachannel_unittest.cc:39: on_buffered_amount_change_count_(0)
{}
On 2015/06/25 18:56:22, juberti1 wrote:
> fix indent - 1 initializer per line

Done.

[bemasc2, 2015-06-25 19:56:17]

The CQ bit was checked by bemasc@webrtc.org

[bemasc2, 2015-06-25 19:56:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
pthatcher@webrtc.org, juberti@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1207613006/#ps40001 (title:
"Improve indentation in FakeDataChannel")

[commit-bot: I haz the power, 2015-06-25 19:56:26]

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

[commit-bot: I haz the power, 2015-06-25 19:58:37]

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

[commit-bot: I haz the power, 2015-06-25 19:58:38]

Exceeded global retry quota

[bemasc2, 2015-06-25 20:01:38]

On 2015/06/25 19:58:38, commit-bot: I haz the power wrote:
> Exceeded global retry quota

Looks like this failure is because of missing changes to the Objective C
bindings.  I will investigate.

[bemasc2, 2015-06-29 17:27:44]

bemasc@webrtc.org changed reviewers:
+ tkchin@webrtc.org

[bemasc2, 2015-06-29 17:28:43]

On 2015/06/25 20:01:38, bemasc2 wrote:
> On 2015/06/25 19:58:38, commit-bot: I haz the power wrote:
> > Exceeded global retry quota
> 
> Looks like this failure is because of missing changes to the Objective C
> bindings.  I will investigate.

Added basic Objective C bindings.  tkchin@, PTAL.  Is this going to cause an API
break for library users?

[tkchin_webrtc, 2015-06-29 22:18:30]

just some style nits.

https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:563: "onBufferedAmountChange",
"(J)V")),
formatting here seems odd. Did you run git clang-format?

https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/objc/RTCD...
File talk/app/webrtc/objc/RTCDataChannel.mm (right):

https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/objc/RTCD...
talk/app/webrtc/objc/RTCDataChannel.mm:46: void OnBufferedAmountChange(uint64
previous_amount) override {
For .mm files naming style isn't exactly fixed, but in this case would say to
use camel casing (previousAmount) to match rest of ObjC calls.

https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/objc/publ...
File talk/app/webrtc/objc/public/RTCDataChannel.h (right):

https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/objc/publ...
talk/app/webrtc/objc/public/RTCDataChannel.h:82: -
(void)channelDidChangeBufferedAmount:(RTCDataChannel*)channel
Update comment?
Style:
- (void)channel:(RTCDataChannel*)channel
didChangeBufferedAmount:(NSUInteger)amount;

https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/objctests...
File talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.m (right):

https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/objctests...
talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.m:235: NSAssert([channel
bufferedAmount] != previousAmount,
Style: properties should be accessed using dot syntax.
NSAssert(channel.bufferedAmount != previousAmount, ...);

https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/test/mock...
File talk/app/webrtc/test/mockpeerconnectionobservers.h (right):

https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/test/mock...
talk/app/webrtc/test/mockpeerconnectionobservers.h:101: virtual void
OnBufferedAmountChange(uint64 previous_amount) { }
Update methods to use override instead of virtual?

[tkchin_webrtc, 2015-06-29 22:19:41]

On 2015/06/29 22:18:30, tkchin_webrtc wrote:
> just some style nits.
> 
>
https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/java/jni/...
> File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):
> 
>
https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/java/jni/...
> talk/app/webrtc/java/jni/peerconnection_jni.cc:563: "onBufferedAmountChange",
> "(J)V")),
> formatting here seems odd. Did you run git clang-format?
> 
>
https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/objc/RTCD...
> File talk/app/webrtc/objc/RTCDataChannel.mm (right):
> 
>
https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/objc/RTCD...
> talk/app/webrtc/objc/RTCDataChannel.mm:46: void OnBufferedAmountChange(uint64
> previous_amount) override {
> For .mm files naming style isn't exactly fixed, but in this case would say to
> use camel casing (previousAmount) to match rest of ObjC calls.
> 
>
https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/objc/publ...
> File talk/app/webrtc/objc/public/RTCDataChannel.h (right):
> 
>
https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/objc/publ...
> talk/app/webrtc/objc/public/RTCDataChannel.h:82: -
> (void)channelDidChangeBufferedAmount:(RTCDataChannel*)channel
> Update comment?
> Style:
> - (void)channel:(RTCDataChannel*)channel
> didChangeBufferedAmount:(NSUInteger)amount;
> 
>
https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/objctests...
> File talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.m (right):
> 
>
https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/objctests...
> talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.m:235:
NSAssert([channel
> bufferedAmount] != previousAmount,
> Style: properties should be accessed using dot syntax.
> NSAssert(channel.bufferedAmount != previousAmount, ...);
> 
>
https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/test/mock...
> File talk/app/webrtc/test/mockpeerconnectionobservers.h (right):
> 
>
https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/test/mock...
> talk/app/webrtc/test/mockpeerconnectionobservers.h:101: virtual void
> OnBufferedAmountChange(uint64 previous_amount) { }
> Update methods to use override instead of virtual?

And yes, this will cause an API break because you're adding a non-optional
method on the protocol.

[bemasc2, 2015-06-29 22:53:01]

On 2015/06/29 22:19:41, tkchin_webrtc wrote:
> On 2015/06/29 22:18:30, tkchin_webrtc wrote:
> > just some style nits.
> > 
> >
>
https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/java/jni/...
> > File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):
> > 
> >
>
https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/java/jni/...
> > talk/app/webrtc/java/jni/peerconnection_jni.cc:563:
"onBufferedAmountChange",
> > "(J)V")),
> > formatting here seems odd. Did you run git clang-format?
> > 
> >
>
https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/objc/RTCD...
> > File talk/app/webrtc/objc/RTCDataChannel.mm (right):
> > 
> >
>
https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/objc/RTCD...
> > talk/app/webrtc/objc/RTCDataChannel.mm:46: void
OnBufferedAmountChange(uint64
> > previous_amount) override {
> > For .mm files naming style isn't exactly fixed, but in this case would say
to
> > use camel casing (previousAmount) to match rest of ObjC calls.
> > 
> >
>
https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/objc/publ...
> > File talk/app/webrtc/objc/public/RTCDataChannel.h (right):
> > 
> >
>
https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/objc/publ...
> > talk/app/webrtc/objc/public/RTCDataChannel.h:82: -
> > (void)channelDidChangeBufferedAmount:(RTCDataChannel*)channel
> > Update comment?
> > Style:
> > - (void)channel:(RTCDataChannel*)channel
> > didChangeBufferedAmount:(NSUInteger)amount;
> > 
> >
>
https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/objctests...
> > File talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.m (right):
> > 
> >
>
https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/objctests...
> > talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.m:235:
> NSAssert([channel
> > bufferedAmount] != previousAmount,
> > Style: properties should be accessed using dot syntax.
> > NSAssert(channel.bufferedAmount != previousAmount, ...);
> > 
> >
>
https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/test/mock...
> > File talk/app/webrtc/test/mockpeerconnectionobservers.h (right):
> > 
> >
>
https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/test/mock...
> > talk/app/webrtc/test/mockpeerconnectionobservers.h:101: virtual void
> > OnBufferedAmountChange(uint64 previous_amount) { }
> > Update methods to use override instead of virtual?
> 
> And yes, this will cause an API break because you're adding a non-optional
> method on the protocol.

OK, I've changed that protocol method to be optional, and after some thought
I've also made DataChannelObserver::OnBufferedAmountChange optional in
datachannelinterface.h, to minimize breakage of any third parties who might be
using this API.

I guess the Java API has the same issue, but since DataChannelObserver is an
Interface, I don't see any way to avoid the break there.

[bemasc2, 2015-06-29 22:53:17]

https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/java/jni/...
File talk/app/webrtc/java/jni/peerconnection_jni.cc (right):

https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/java/jni/...
talk/app/webrtc/java/jni/peerconnection_jni.cc:563: "onBufferedAmountChange",
"(J)V")),
On 2015/06/29 22:18:30, tkchin_webrtc wrote:
> formatting here seems odd. Did you run git clang-format?

Done. Ran clang-format.

https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/objc/RTCD...
File talk/app/webrtc/objc/RTCDataChannel.mm (right):

https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/objc/RTCD...
talk/app/webrtc/objc/RTCDataChannel.mm:46: void OnBufferedAmountChange(uint64
previous_amount) override {
On 2015/06/29 22:18:30, tkchin_webrtc wrote:
> For .mm files naming style isn't exactly fixed, but in this case would say to
> use camel casing (previousAmount) to match rest of ObjC calls.

Done.

https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/objc/publ...
File talk/app/webrtc/objc/public/RTCDataChannel.h (right):

https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/objc/publ...
talk/app/webrtc/objc/public/RTCDataChannel.h:82: -
(void)channelDidChangeBufferedAmount:(RTCDataChannel*)channel
On 2015/06/29 22:18:30, tkchin_webrtc wrote:
> Update comment?

Done.

> Style:
> - (void)channel:(RTCDataChannel*)channel
> didChangeBufferedAmount:(NSUInteger)amount;

clang-format had other ideas :)

https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/objctests...
File talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.m (right):

https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/objctests...
talk/app/webrtc/objctests/RTCPeerConnectionSyncObserver.m:235: NSAssert([channel
bufferedAmount] != previousAmount,
On 2015/06/29 22:18:30, tkchin_webrtc wrote:
> Style: properties should be accessed using dot syntax.
> NSAssert(channel.bufferedAmount != previousAmount, ...);

Done.

https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/test/mock...
File talk/app/webrtc/test/mockpeerconnectionobservers.h (right):

https://codereview.webrtc.org/1207613006/diff/60001/talk/app/webrtc/test/mock...
talk/app/webrtc/test/mockpeerconnectionobservers.h:101: virtual void
OnBufferedAmountChange(uint64 previous_amount) { }
On 2015/06/29 22:18:30, tkchin_webrtc wrote:
> Update methods to use override instead of virtual?

Done.

[tkchin_webrtc, 2015-06-30 00:24:11]

I don't have a strong opinion on breakage prevention because of the branch cut
bit I mentioned. Optional observer methods sound reasonable to me (basically
only listen to the events you care about).

That being said it's weird to have one language do something different than the
others API-wise, if it breaks one it would seem like it should break all three.
I'll leave this up to you though.

https://codereview.webrtc.org/1207613006/diff/100001/talk/app/webrtc/objc/RTC...
File talk/app/webrtc/objc/RTCDataChannel.mm (right):

https://codereview.webrtc.org/1207613006/diff/100001/talk/app/webrtc/objc/RTC...
talk/app/webrtc/objc/RTCDataChannel.mm:46: void OnBufferedAmountChange(uint64
previousAmount) override {
nit:
this might look cleaner with a local var (and it'd be more correct too - when
accessing weak ptr twice in a row the second time may return nil while the first
doesn't).
RTCDataChannel* channel = _channel;  (converts to strong ptr)
id<RTCDataChannelDelegate> delegate = channel.delegate;
if ([delegate respondsTo....]) {
etc.
}

https://codereview.webrtc.org/1207613006/diff/100001/talk/app/webrtc/objc/pub...
File talk/app/webrtc/objc/public/RTCDataChannel.h (right):

https://codereview.webrtc.org/1207613006/diff/100001/talk/app/webrtc/objc/pub...
talk/app/webrtc/objc/public/RTCDataChannel.h:88: -
(void)channelDidChangeBufferedAmount:(RTCDataChannel*)channel
Wait, clang-format complained about the method naming? If so it's wrong.
(Wouldn't be the first time)
Please use
- (void)channel:(RTCDataChannel*)channel
didChangeBufferedAmount:(NSUInteger)amount;

That properly conforms to style guide. (See delegate naming in
https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Coding...)

[bemasc2, 2015-06-30 14:33:25]

On 2015/06/30 00:24:11, tkchin_webrtc wrote:
> I don't have a strong opinion on breakage prevention because of the branch cut
> bit I mentioned. Optional observer methods sound reasonable to me (basically
> only listen to the events you care about).

I must have missed your comment about a branch cut...

> That being said it's weird to have one language do something different than
the
> others API-wise, if it breaks one it would seem like it should break all
three.
> I'll leave this up to you though.

At the moment I don't feel like we should break the C++ and ObjC APIs just
because Java Interfaces don't support optional methods.

>
https://codereview.webrtc.org/1207613006/diff/100001/talk/app/webrtc/objc/RTC...
> File talk/app/webrtc/objc/RTCDataChannel.mm (right):
> 
>
https://codereview.webrtc.org/1207613006/diff/100001/talk/app/webrtc/objc/RTC...
> talk/app/webrtc/objc/RTCDataChannel.mm:46: void OnBufferedAmountChange(uint64
> previousAmount) override {
> nit:
> this might look cleaner with a local var (and it'd be more correct too - when
> accessing weak ptr twice in a row the second time may return nil while the
first
> doesn't).
> RTCDataChannel* channel = _channel;  (converts to strong ptr)
> id<RTCDataChannelDelegate> delegate = channel.delegate;
> if ([delegate respondsTo....]) {
> etc.
> }

Done.

> 
>
https://codereview.webrtc.org/1207613006/diff/100001/talk/app/webrtc/objc/pub...
> File talk/app/webrtc/objc/public/RTCDataChannel.h (right):
> 
>
https://codereview.webrtc.org/1207613006/diff/100001/talk/app/webrtc/objc/pub...
> talk/app/webrtc/objc/public/RTCDataChannel.h:88: -
> (void)channelDidChangeBufferedAmount:(RTCDataChannel*)channel
> Wait, clang-format complained about the method naming? If so it's wrong.

Oops, I thought you were asking for a change in spacing.
Done.

> (Wouldn't be the first time)
> Please use
> - (void)channel:(RTCDataChannel*)channel
> didChangeBufferedAmount:(NSUInteger)amount;
> 
> That properly conforms to style guide. (See delegate naming in
>
https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/Coding...)

[tkchin_webrtc, 2015-06-30 17:53:47]

lgtm

oops, I could've sworn I had a comment along the lines of - since a bunch of
webrtc consumers only take branch cuts, they're expecting to deal with API
breakage anyway. My bad.

https://codereview.webrtc.org/1207613006/diff/120001/talk/app/webrtc/objc/RTC...
File talk/app/webrtc/objc/RTCDataChannel.mm (right):

https://codereview.webrtc.org/1207613006/diff/120001/talk/app/webrtc/objc/RTC...
talk/app/webrtc/objc/RTCDataChannel.mm:47: RTCDataChannel* channel = _channel; 
// Converts weak ptr to strong ptr.
don't need the comment, fairly standard practice.

[bemasc2, 2015-06-30 18:29:45]

https://codereview.webrtc.org/1207613006/diff/120001/talk/app/webrtc/objc/RTC...
File talk/app/webrtc/objc/RTCDataChannel.mm (right):

https://codereview.webrtc.org/1207613006/diff/120001/talk/app/webrtc/objc/RTC...
talk/app/webrtc/objc/RTCDataChannel.mm:47: RTCDataChannel* channel = _channel; 
// Converts weak ptr to strong ptr.
On 2015/06/30 17:53:47, tkchin_webrtc wrote:
> don't need the comment, fairly standard practice.

OK, removed comment.

[bemasc2, 2015-06-30 18:29:55]

The CQ bit was checked by bemasc@webrtc.org

[bemasc2, 2015-06-30 18:29:55]

The patchset sent to the CQ was uploaded after l-g-t-m from juberti@webrtc.org,
pthatcher@webrtc.org, tkchin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1207613006/#ps140001 (title:
"Remove unnecessary comment")

[commit-bot: I haz the power, 2015-06-30 18:30:24]

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

[commit-bot: I haz the power, 2015-06-30 19:10:22]

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

[commit-bot: I haz the power, 2015-06-30 19:10:24]

Exceeded global retry quota

[bemasc2, 2015-07-01 19:35:50]

The CQ bit was checked by bemasc@webrtc.org

[commit-bot: I haz the power, 2015-07-01 19:36:03]

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

[commit-bot: I haz the power, 2015-07-01 20:34:38]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-07-01 20:34:54]

Patchset 8 (id:??) landed as
https://crrev.com/0edd50ccb34cc2dc4746137fdce1f5cf66808274
Cr-Commit-Position: refs/heads/master@{#9527}