Add counters for nonstandard uses of RTCPeerConnection

Add counters for nonstandard uses of RTCPeerConnection

BUG=564530
Committed: https://crrev.com/7a746bbf64928208672aadc0f3366011ae69c1b0
Cr-Commit-Position: refs/heads/master@{#363050}

[Guido Urdaneta, 2015-12-02 13:18:49]

guidou@chromium.org changed reviewers:
+ hta@chromium.org, jochen@chromium.org

[Guido Urdaneta, 2015-12-02 13:18:49]

Hi, PTAL

Intent to deprecate thread:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/WderCH-rAaY

[jochen (gone - plz use gerrit), 2015-12-02 13:23:27]

lgtm

[Guido Urdaneta, 2015-12-02 13:57:02]

Description was changed from

==========
Add counters for nonstandard uses of RTCPeerConnection

BUG=564530
==========

to

==========
Add counters and deprecation warnings for nonstandard uses of RTCPeerConnection

BUG=564530
==========

[Guido Urdaneta, 2015-12-02 15:34:32]

On 2015/12/02 13:23:27, jochen wrote:
> lgtm

I removed some tests that involve deprecated features and made some minor
changes to the mock RTCPeerConnection handler so that tests can avoid the
deprecated features.
Can you take another look?

[jochen (gone - plz use gerrit), 2015-12-02 15:38:20]

still lgtm

[Guido Urdaneta, 2015-12-02 16:03:48]

The CQ bit was checked by guidou@chromium.org

[commit-bot: I haz the power, 2015-12-02 16:04:55]

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

[hta - Chromium, 2015-12-02 16:24:11]

I am somewhat skeptical of removing the tests for a feature before we remove the
code for it.

Did the tests start failing when the use counters were added, or is there some
other reason to delete them now?

[Guido Urdaneta, 2015-12-02 16:26:31]

The CQ bit was unchecked by guidou@chromium.org

[Guido Urdaneta, 2015-12-02 16:36:27]

On 2015/12/02 16:24:11, hta - Chromium wrote:
> I am somewhat skeptical of removing the tests for a feature before we remove
the
> code for it.
> 
> Did the tests start failing when the use counters were added, or is there some
> other reason to delete them now?

The tests did not fail. 
In the case of the constructor it just produced deprecation warnings in the
output. I can add them back.
In the case of the createOffer test, the old MockRTCPeerConnectionHandler made
all correct uses of RTCOfferOptions fail. Only the fake (and now deprecated)
{mandatory:{"succeed":true}} constraints argument made createOffer succeed.
I changed it so that a valid RTCOfferOptions value makes createOffer succeed. I
can add a couple of tests back (together with the deprecation warning).

[Guido Urdaneta, 2015-12-02 16:53:24]

I put some of the deprecated tests back in.

[hta - Chromium, 2015-12-02 17:06:48]

lgtm

If any of the tests make trouble or cost more to support in mocks and so on than
it's worth, I'm fine with deleting them.
If we leave them in, we get early warning before we break something deprecated
that people depend on.

[philipj_slow, 2015-12-02 20:28:12]

philipj@opera.com changed reviewers:
+ philipj@opera.com

[philipj_slow, 2015-12-02 20:28:12]

https://codereview.chromium.org/1494543002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/1494543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:373:
UseCounter::count(context,
UseCounter::RTCPeerConnectionCreateOfferLegacyFailureCallback);
Is it really useful to measure when the error callback is present, separately
from the compliant bucket below? Measuring the specific non-compliant things as
well as a single compliant counter seems sufficient.

https://codereview.chromium.org/1494543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:412:
UseCounter::count(context,
UseCounter::RTCPeerConnectionCreateAnswerLegacyFailureCallback);
Same question about measuring this separately.

https://codereview.chromium.org/1494543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:439:
UseCounter::countDeprecation(context,
UseCounter::RTCPeerConnectionSetLocalDescriptionLegacyNoCallbacks);
You can omit just one of the callbacks, successCallback by passing
null/undefined, should you measure them separately? It seems far more likely
that the failure callback is omitted, and if the spec needs to change it'd be
nice to know that the success callback can stay non-optional.

https://codereview.chromium.org/1494543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:461: if
(successCallback && errorCallback)
Dito.

[Guido Urdaneta, 2015-12-03 10:51:34]

https://codereview.chromium.org/1494543002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/1494543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:373:
UseCounter::count(context,
UseCounter::RTCPeerConnectionCreateOfferLegacyFailureCallback);
On 2015/12/02 20:28:12, philipj wrote:
> Is it really useful to measure when the error callback is present, separately
> from the compliant bucket below? Measuring the specific non-compliant things
as
> well as a single compliant counter seems sufficient.

It is useful. If we find that users (almost) always provide the error callback,
we can add the promise-based version of the method even if most calls are still
noncompliant (e.g., due to passing constraints instead of offerOptions).

https://codereview.chromium.org/1494543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:412:
UseCounter::count(context,
UseCounter::RTCPeerConnectionCreateAnswerLegacyFailureCallback);
On 2015/12/02 20:28:12, philipj wrote:
> Same question about measuring this separately.

Same answer as for createOffer.

https://codereview.chromium.org/1494543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:439:
UseCounter::countDeprecation(context,
UseCounter::RTCPeerConnectionSetLocalDescriptionLegacyNoCallbacks);
On 2015/12/02 20:28:12, philipj wrote:
> You can omit just one of the callbacks, successCallback by passing
> null/undefined, should you measure them separately? It seems far more likely
> that the failure callback is omitted, and if the spec needs to change it'd be
> nice to know that the success callback can stay non-optional.

Done. This is actually a great idea for the same reason as the extra counter in
createOffer/createAnswer. If we find that people always provide the
successCallback, we can add the promise-based version even if the
failureCallback is not passed.

https://codereview.chromium.org/1494543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:461: if
(successCallback && errorCallback)
On 2015/12/02 20:28:12, philipj wrote:
> Dito.

Done.

[philipj_slow, 2015-12-03 11:25:58]

Use counter logic LGTM, but let's discuss further on blink-dev how the
deprecation should happen. (Will reply after lunch.)

https://codereview.chromium.org/1494543002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp
(right):

https://codereview.chromium.org/1494543002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:373:
UseCounter::count(context,
UseCounter::RTCPeerConnectionCreateOfferLegacyFailureCallback);
On 2015/12/03 10:51:34, Guido Urdaneta wrote:
> On 2015/12/02 20:28:12, philipj wrote:
> > Is it really useful to measure when the error callback is present,
separately
> > from the compliant bucket below? Measuring the specific non-compliant things
> as
> > well as a single compliant counter seems sufficient.
> 
> It is useful. If we find that users (almost) always provide the error
callback,
> we can add the promise-based version of the method even if most calls are
still
> noncompliant (e.g., due to passing constraints instead of offerOptions).

That makes sense!

[philipj_slow, 2015-12-03 12:54:03]

Do you have an idea for getStats as well? It's the only problematic API in
https://w3c.github.io/webrtc-pc/#legacy-interface-extensions not covered by
these counters, I think.

[blink-reviews, 2015-12-03 14:06:22]

I've deferred thinking about GetStats until I'm finished with
constraints.... and that's taking its time.


On Thu, Dec 3, 2015 at 1:54 PM, <philipj@opera.com> wrote:

> Do you have an idea for getStats as well? It's the only problematic API in
> https://w3c.github.io/webrtc-pc/#legacy-interface-extensions not covered
> by
> these counters, I think.
>
> https://codereview.chromium.org/1494543002/
>

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2015-12-03 14:06:23]

I've deferred thinking about GetStats until I'm finished with
constraints.... and that's taking its time.


On Thu, Dec 3, 2015 at 1:54 PM, <philipj@opera.com> wrote:

> Do you have an idea for getStats as well? It's the only problematic API in
> https://w3c.github.io/webrtc-pc/#legacy-interface-extensions not covered
> by
> these counters, I think.
>
> https://codereview.chromium.org/1494543002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[philipj_slow, 2015-12-03 14:19:27]

On 2015/12/03 14:06:23, chromium-reviews wrote:
> I've deferred thinking about GetStats until I'm finished with
> constraints.... and that's taking its time.

Fair enough. FWIW, I don't have any good ideas for getStats at this time either.

[Guido Urdaneta, 2015-12-03 16:02:12]

I just removed all deprecation warnings so that we can start getting data ASAP.
I also added a counter for GetStats since that data can also be useful, even if
we don't have a concrete plan for it yet.

[Guido Urdaneta, 2015-12-03 16:34:05]

Description was changed from

==========
Add counters and deprecation warnings for nonstandard uses of RTCPeerConnection

BUG=564530
==========

to

==========
Add counters for nonstandard uses of RTCPeerConnection

BUG=564530
==========

[Guido Urdaneta, 2015-12-03 17:32:18]

guidou@chromium.org changed reviewers:
+ isherman@chromium.org

[Guido Urdaneta, 2015-12-03 17:33:15]

isherman@: please review changes to histograms.xml

[Ilya Sherman, 2015-12-03 17:38:10]

histograms.xml lgtm

[Guido Urdaneta, 2015-12-03 19:42:31]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2015-12-03 19:42:32]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
hta@chromium.org, philipj@opera.com
Link to the patchset: https://codereview.chromium.org/1494543002/#ps160001
(title: "Update histograms.xml")

[commit-bot: I haz the power, 2015-12-03 19:43:05]

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

[commit-bot: I haz the power, 2015-12-03 20:45:44]

Description was changed from

==========
Add counters for nonstandard uses of RTCPeerConnection

BUG=564530
==========

to

==========
Add counters for nonstandard uses of RTCPeerConnection

BUG=564530
==========

[commit-bot: I haz the power, 2015-12-03 20:45:45]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2015-12-03 20:46:20]

Description was changed from

==========
Add counters for nonstandard uses of RTCPeerConnection

BUG=564530
==========

to

==========
Add counters for nonstandard uses of RTCPeerConnection

BUG=564530

Committed: https://crrev.com/7a746bbf64928208672aadc0f3366011ae69c1b0
Cr-Commit-Position: refs/heads/master@{#363050}
==========

[commit-bot: I haz the power, 2015-12-03 20:46:21]

Patchset 9 (id:??) landed as
https://crrev.com/7a746bbf64928208672aadc0f3366011ae69c1b0
Cr-Commit-Position: refs/heads/master@{#363050}