1328440 - Legacy PeerConnection.getStats should return a legacy stats compatible object

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Calling the legacy callback based PeerConnection.getStats returns a hybrid object that acts (mostly) like both a legacy and a spec compliant report object.  However, both the legacy and the spec compliant implementation have a forEach method. Currently, calling the forEach will produce spec compliant results. The type fields uses the new hyphenated type names, which may break code still using the legacy PeerConnection.getStats call. The returned object from a legacy call should not use hyphenated names.

Comment 1

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Attachment: Bug 1328440 - make legacy getStats stats use old type field names;

Review commit: https://reviewboard.mozilla.org/r/102282/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102282/

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8823766 [details]
Bug 1328440 - make legacy getStats stats use old type field names;

https://reviewboard.mozilla.org/r/102282/#review102702

Need to fix setting of _onGetStatsIsLegacy. The rest are nits.

::: dom/media/PeerConnection.js:308
(Diff revision 1)
>    setInternal: function(aKey, aObj) {
>      return this.__DOM_IMPL__.__set(aKey, aObj);
>    },
>  
>    // TODO: Remove legacy API eventually
> +  // see Bug 1328194 https://bugzilla.mozilla.org/show_bug.cgi?id=1328194

Bug # is sufficient.

::: dom/media/PeerConnection.js:341
(Diff revision 1)
> +  // TODO: remove legacy API eventually
> +  // see Bug 1328194 https://bugzilla.mozilla.org/show_bug.cgi?id=1328194
> +  //
> +  // Reverts the new style hyphenated type names back to the legacy
> +  // unhyphenated style.
> +  forceLegacyTypeNames: function() {
> +    for (let key in this._report) {
> +      let value = Cu.cloneInto(this._report[key], this._win);
> +      value.type = this._specToLegacyFieldMapping[value.type] || value.type;
> +      this.setInternal(key, value);

A bit of double code here. Passing in a boolean to makeStatsPublic would be an alternative.

Did you make this a separate function to make it easier to rip out later?

::: dom/media/PeerConnection.js:1244
(Diff revision 1)
>    getStats: function(selector, onSucc, onErr) {
> -    if (typeof onSucc == "function" &&
> +    this._onGetStatsIsLegacy = (typeof onSucc) == "function";

This needs to move to _getStats where it's protected by the chain. Otherwise I can mess up the state like this:

    pc.getStats(null, () => { /* expects legacy, won't get it because... */} );
    pc.getStats(); // this sets pc._onGetStatsIsLegacy = false for both calls

::: dom/media/tests/mochitest/pc.js:1530
(Diff revision 1)
> +    let types = "";
> +    stats.forEach(stat => {
> +      types = types + " " + stat.type;
> +    });

+= and lose the {}

Or you could do:

    let types = [...stats.values()].reduce((s, stat) => s + stat.type, "");

Comment 3

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8823766 [details]
Bug 1328440 - make legacy getStats stats use old type field names;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102282/diff/1-2/

Comment 4

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8823766 [details]
Bug 1328440 - make legacy getStats stats use old type field names;

https://reviewboard.mozilla.org/r/102282/#review102702

> A bit of double code here. Passing in a boolean to makeStatsPublic would be an alternative.
> 
> Did you make this a separate function to make it easier to rip out later?

Yep.

> += and lose the {}
> 
> Or you could do:
> 
>     let types = [...stats.values()].reduce((s, stat) => s + stat.type, "");

I agree that it definitely more concise. **re:** reduce, I wanted to be sure that the test was using forEach. Thoughts?

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8823766 [details]
Bug 1328440 - make legacy getStats stats use old type field names;

https://reviewboard.mozilla.org/r/102282/#review102702

> Yep.

Ok. Would probably be simpler and more efficient to use

    let type = this.get(key).type;
    this.setInternal(key, this._specToLegacyFieldMapping[type] || type);

in that case, to avoid cross-compartment cloning all the dictionaries twice.

> I agree that it definitely more concise. **re:** reduce, I wanted to be sure that the test was using forEach. Thoughts?

That's fine. forEach is provided by Map class here, so any way we verify the map is good.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8823766 [details]
Bug 1328440 - make legacy getStats stats use old type field names;

https://reviewboard.mozilla.org/r/102282/#review103244

Comment 7

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8823766 [details]
Bug 1328440 - make legacy getStats stats use old type field names;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102282/diff/2-3/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9468049db589
make legacy getStats stats use old type field names; r=jib

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9468049db589