Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
4 years, 5 months ago by hbos_chromium Modified:
4 years, 4 months ago Reviewers:
tommi (sloooow) - chröme, bokan, hta - Chromium, Alexei Svitkine (slow), haraken, Mike West CC:
blink-reviews, chromium-reviews, haraken Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPreparation CL for new Promise-based RTCPeerConnection.getStats.
The WebRTC spec defines the following getStats[1]:
partial interface RTCPeerConnection {
Promise<RTCStatsReport> getStats(optional MediaStreamTrack? selector = null);
};
interface RTCStatsReport {
readonly maplike<DOMString, object>; // object: RTCStats-derived dictionaries
};
dictionary RTCStats {
DOMHighResTimeStamp timestamp;
RTCStatsType type;
DOMString id;
};
There currently exists a callback-based getStats method and related interfaces
that are different from the spec. To not break existing usages, these will be
kept for a transition period.
Before this CL, what we call "RTCStatsReport" is something other than what the
spec refers to (and is more similar to the spec's RTCStats but different).
A big difference between old and new stats API is that the old stats are
presented as string-string maps and the new API as well defined and typed
dictionary members (each deriviation should have its own .idl file). This makes
the two APIs incompatible.
Changes:
- The old RTCStatsReport is renamed to RTCLegacyStatsReport to avoid a name
conflict for when adding the new RTCStatsReport in a follow-up CL. This should
be low-risk since its a NoInterfaceObject.
- The callback-based getStats is changed in ways necessary to support two
getStats functions. E.g. it now returns Promise<void> because the return
values both have to be Promise<Foo>.
- The promise-based getStats is added behind runtime enabled test feature
"RTCPeerConnectionNewGetStats". It is not implemented yet and always rejects
its promise for now.
- The new signature is tested in RTCPeerConnection-getStats-promise.html.
[1] https://www.w3.org/TR/2016/WD-webrtc-20160531/#sec.stats-model
BUG=627816, 629068
Committed: https://crrev.com/08fa67b0f718b342b8c47725f2018c63ade8b699
Cr-Commit-Position: refs/heads/master@{#408096}
Patch Set 1 #Patch Set 2 : Cleanup and bug referencing #Patch Set 3 : Replaced TODO /w comment what happens if function != 1 args #
Total comments: 16
Patch Set 4 : Addressed hta comments (testharness.js, updated rejections) #Patch Set 5 : Updated comment in RTCPeerConnection.idl #Patch Set 6 : Added UseCounter for the new getStats #
Total comments: 6
Patch Set 7 : Rebase with master #Patch Set 8 : Addressed hta's comments #
Total comments: 2
Patch Set 9 : Overloading getStats without custom code (bindings overload bug has been fixed) #Patch Set 10 : Rebase with master #Patch Set 11 : Rebase with master #
Created: 4 years, 4 months ago
Messages
Total messages: 47 (26 generated)
The CQ bit was checked by hbos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Preparation for new Promise-based RTCPeerConnection.getStats. BUG= ========== to ========== Preparation for new Promise-based RTCPeerConnection.getStats. BUG=627816, 629068 ==========
Patchset #2 (id:20001) has been deleted
Description was changed from
==========
Preparation for new Promise-based RTCPeerConnection.getStats.
BUG=627816, 629068
==========
to
==========
Preparation CL for new Promise-based RTCPeerConnection.getStats.
The WebRTC spec defines the following getStats[1]:
partial interface RTCPeerConnection {
Promise<RTCStatsReport> getStats(optional MediaStreamTrack? selector =
null);
};
interface RTCStatsReport {
readonly maplike<DOMString, object>; // object: RTCStats-derived
dictionaries
};
dictionary RTCStats {
DOMHighResTimeStamp timestamp;
RTCStatsType type;
DOMString id;
};
There currently exists a callback-based getStats method and related interfaces
that are different from the spec. To not break existing usages, these will be
kept for a transition period.
Before this CL, what we call "RTCStatsReport" is something other than what the
spec refers to (and is more similar to the spec's RTCStats but different).
A big difference between old and new stats API is that the old stats are
presented as string-string maps and the new API as well defined and typed
dictionary members (each deriviation should have its own .idl file). This makes
the two APIs incompatible.
Changes:
- The old RTCStatsReport is renamed to RTCLegacyStatsReport to avoid a name
conflict. This should be low-risk since its a NoInterfaceObject.
- The callback-based getStats is changed in ways necessary to support two
getStats functions. E.g. it now returns Promise<void> because the return
values both have to be Promise<Foo>.
- The promise-based getStats is added behind runtime enabled test feature
"RTCPeerConnectionNewGetStats". It is not implemented yet and always rejects
its promise for now.
- The new signature is tested in RTCPeerConnection-statsPromise.html.
Due to WebIDL limitations, callback interfaces and interfaces are
indistinguishable[2] when deciding which getStats function to call. This CL gets
around this by defining getStats(optional any value) in WebIDL and examining the
value's type information in C++. See crbug.com/629068.
[1] https://www.w3.org/TR/2016/WD-webrtc-20160531/#sec.stats-model
[2] https://heycam.github.io/webidl/#idl-overloading
BUG=627816, 629068
==========
hbos@chromium.org changed reviewers: + hta@chromium.org, mkwst@chromium.org, tommi@chromium.org
Please take a look, mkwst and hta. (And tommi when back from vacation.)
Description was changed from
==========
Preparation CL for new Promise-based RTCPeerConnection.getStats.
The WebRTC spec defines the following getStats[1]:
partial interface RTCPeerConnection {
Promise<RTCStatsReport> getStats(optional MediaStreamTrack? selector =
null);
};
interface RTCStatsReport {
readonly maplike<DOMString, object>; // object: RTCStats-derived
dictionaries
};
dictionary RTCStats {
DOMHighResTimeStamp timestamp;
RTCStatsType type;
DOMString id;
};
There currently exists a callback-based getStats method and related interfaces
that are different from the spec. To not break existing usages, these will be
kept for a transition period.
Before this CL, what we call "RTCStatsReport" is something other than what the
spec refers to (and is more similar to the spec's RTCStats but different).
A big difference between old and new stats API is that the old stats are
presented as string-string maps and the new API as well defined and typed
dictionary members (each deriviation should have its own .idl file). This makes
the two APIs incompatible.
Changes:
- The old RTCStatsReport is renamed to RTCLegacyStatsReport to avoid a name
conflict. This should be low-risk since its a NoInterfaceObject.
- The callback-based getStats is changed in ways necessary to support two
getStats functions. E.g. it now returns Promise<void> because the return
values both have to be Promise<Foo>.
- The promise-based getStats is added behind runtime enabled test feature
"RTCPeerConnectionNewGetStats". It is not implemented yet and always rejects
its promise for now.
- The new signature is tested in RTCPeerConnection-statsPromise.html.
Due to WebIDL limitations, callback interfaces and interfaces are
indistinguishable[2] when deciding which getStats function to call. This CL gets
around this by defining getStats(optional any value) in WebIDL and examining the
value's type information in C++. See crbug.com/629068.
[1] https://www.w3.org/TR/2016/WD-webrtc-20160531/#sec.stats-model
[2] https://heycam.github.io/webidl/#idl-overloading
BUG=627816, 629068
==========
to
==========
Preparation CL for new Promise-based RTCPeerConnection.getStats.
The WebRTC spec defines the following getStats[1]:
partial interface RTCPeerConnection {
Promise<RTCStatsReport> getStats(optional MediaStreamTrack? selector =
null);
};
interface RTCStatsReport {
readonly maplike<DOMString, object>; // object: RTCStats-derived
dictionaries
};
dictionary RTCStats {
DOMHighResTimeStamp timestamp;
RTCStatsType type;
DOMString id;
};
There currently exists a callback-based getStats method and related interfaces
that are different from the spec. To not break existing usages, these will be
kept for a transition period.
Before this CL, what we call "RTCStatsReport" is something other than what the
spec refers to (and is more similar to the spec's RTCStats but different).
A big difference between old and new stats API is that the old stats are
presented as string-string maps and the new API as well defined and typed
dictionary members (each deriviation should have its own .idl file). This makes
the two APIs incompatible.
Changes:
- The old RTCStatsReport is renamed to RTCLegacyStatsReport to avoid a name
conflict for when adding the new RTCStatsReport in a follow-up CL. This should
be low-risk since its a NoInterfaceObject.
- The callback-based getStats is changed in ways necessary to support two
getStats functions. E.g. it now returns Promise<void> because the return
values both have to be Promise<Foo>.
- The promise-based getStats is added behind runtime enabled test feature
"RTCPeerConnectionNewGetStats". It is not implemented yet and always rejects
its promise for now.
- The new signature is tested in RTCPeerConnection-statsPromise.html.
Due to WebIDL limitations, callback interfaces and interfaces are
indistinguishable[2] when deciding which getStats function to call. This CL gets
around this by defining getStats(optional any value) in WebIDL and examining the
value's type information in C++. See crbug.com/629068.
[1] https://www.w3.org/TR/2016/WD-webrtc-20160531/#sec.stats-model
[2] https://heycam.github.io/webidl/#idl-overloading
BUG=627816, 629068
==========
Good to see this code! Reviewed from the back of an IETF meeting, so don't assume I've caught everything. High points: - Verify your assumptions on IDL overloading rules - Use testharness.js I like the part about doing the move-the-old-stuff-out-of-the-way as a largely separate operation! https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html (right): https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html:4: <script src="../../resources/js-test.js"></script> This test is new. Can you make it a testharness.js-based test instead of js-test.js? I think the guidance is that all new tests should be testharness.js-based (which makes it possible to upstream them to the W3C test framework). https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html:15: navigator.webkitGetUserMedia(dictionary, callback, error); Use navigator.mediaDevices.getUserMedia => promise for all new stuff, even in tests. https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html:44: shouldNotThrow('pc.getStats().then(onResolve, onReject2)'); In general, when testing promises, it's better to write it as pc.getStats() .then(args => { stuff }) .catch(error => { stuff }); This saves you from defining a million oddly named helper functions. there's a style guide on formatting these, which looks odd, but appears to be the least unreasonable one. See the comments in the getConstraints tests, which uses this format. https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp (right): https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:387: // than one argument it is invoked with the subsequent arguments undefined. Is this just a description of how v8::Function works, or is there something special about this particular usage of v8::Function? If it's just general, I suggest removing it. https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:1004: resolver->reject(); Where do we set the error to be returned in this case? https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl (right): https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl:116: // dispatch to the correct |getStats| function based on the value type. I read heycam to say that an interface is distinguishable from another interface if "The two identified interfaces are not the same, it is not possible for a single platform object to implement both interfaces, and it is not the case that both are callback interfaces." Does the WebIDL compiler give you an error when you try to make them separate? https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl:124: // evaluate the value type ourselves in C++. crbug.com/629068, 627816. Can you add UMA counters here while you're at it?
The CQ bit was checked by hbos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL hta. https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html (right): https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html:4: <script src="../../resources/js-test.js"></script> On 2016/07/19 09:56:48, hta - Chromium wrote: > This test is new. Can you make it a testharness.js-based test instead of > js-test.js? > > I think the guidance is that all new tests should be testharness.js-based (which > makes it possible to upstream them to the W3C test framework). Done. Much nicer. https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html:15: navigator.webkitGetUserMedia(dictionary, callback, error); On 2016/07/19 09:56:48, hta - Chromium wrote: > Use navigator.mediaDevices.getUserMedia => promise for all new stuff, even in > tests. Done. https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-statsPromise.html:44: shouldNotThrow('pc.getStats().then(onResolve, onReject2)'); On 2016/07/19 09:56:48, hta - Chromium wrote: > In general, when testing promises, it's better to write it as > > pc.getStats() > .then(args => { > stuff > }) > .catch(error => { > stuff > }); > > This saves you from defining a million oddly named helper functions. > > there's a style guide on formatting these, which looks odd, but appears to be > the least unreasonable one. > See the comments in the getConstraints tests, which uses this format. Done. https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp (right): https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:387: // than one argument it is invoked with the subsequent arguments undefined. On 2016/07/19 09:56:48, hta - Chromium wrote: > Is this just a description of how v8::Function works, or is there something > special about this particular usage of v8::Function? > > If it's just general, I suggest removing it. Done. https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:1004: resolver->reject(); On 2016/07/19 09:56:48, hta - Chromium wrote: > Where do we set the error to be returned in this case? This error was undefined. I added an error and updated other rejection code to use rejectWithDOMException instead. https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl (right): https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl:116: // dispatch to the correct |getStats| function based on the value type. On 2016/07/19 09:56:48, hta - Chromium wrote: > I read heycam to say that an interface is distinguishable from another interface > if "The two identified interfaces are not the same, it is not possible for a > single platform object to implement both interfaces, and it is not the case that > both are callback interfaces." > > Does the WebIDL compiler give you an error when you try to make them separate? I might have misinterpreted something. I'm not sure if they *should* be distinguishable. (Could an object have a handleEvent function and still be considered a MediaStreamTrack?) I was able to get it to compile with both getStats, but due to the generated V8 code the wrong getStats function wound up being called, it didn't distinguish. (In line with spec or V8 gen bug?) I've spent a lot of time on this already. One idea was to change RTCStatsCallback to a callback function but this isn't fully supported (crbug.com/569301) and we would have to write a lot of V8 code manually instead. This seemed less painful. https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl:124: // evaluate the value type ourselves in C++. crbug.com/629068, 627816. On 2016/07/19 09:56:48, hta - Chromium wrote: > Can you add UMA counters here while you're at it? Good idea. I'd rather add and have that reviewed in a separate follow-up CL if that's ok. Added a TODO comment (in RTCPeerConnection.h).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from
==========
Preparation CL for new Promise-based RTCPeerConnection.getStats.
The WebRTC spec defines the following getStats[1]:
partial interface RTCPeerConnection {
Promise<RTCStatsReport> getStats(optional MediaStreamTrack? selector =
null);
};
interface RTCStatsReport {
readonly maplike<DOMString, object>; // object: RTCStats-derived
dictionaries
};
dictionary RTCStats {
DOMHighResTimeStamp timestamp;
RTCStatsType type;
DOMString id;
};
There currently exists a callback-based getStats method and related interfaces
that are different from the spec. To not break existing usages, these will be
kept for a transition period.
Before this CL, what we call "RTCStatsReport" is something other than what the
spec refers to (and is more similar to the spec's RTCStats but different).
A big difference between old and new stats API is that the old stats are
presented as string-string maps and the new API as well defined and typed
dictionary members (each deriviation should have its own .idl file). This makes
the two APIs incompatible.
Changes:
- The old RTCStatsReport is renamed to RTCLegacyStatsReport to avoid a name
conflict for when adding the new RTCStatsReport in a follow-up CL. This should
be low-risk since its a NoInterfaceObject.
- The callback-based getStats is changed in ways necessary to support two
getStats functions. E.g. it now returns Promise<void> because the return
values both have to be Promise<Foo>.
- The promise-based getStats is added behind runtime enabled test feature
"RTCPeerConnectionNewGetStats". It is not implemented yet and always rejects
its promise for now.
- The new signature is tested in RTCPeerConnection-statsPromise.html.
Due to WebIDL limitations, callback interfaces and interfaces are
indistinguishable[2] when deciding which getStats function to call. This CL gets
around this by defining getStats(optional any value) in WebIDL and examining the
value's type information in C++. See crbug.com/629068.
[1] https://www.w3.org/TR/2016/WD-webrtc-20160531/#sec.stats-model
[2] https://heycam.github.io/webidl/#idl-overloading
BUG=627816, 629068
==========
to
==========
Preparation CL for new Promise-based RTCPeerConnection.getStats.
The WebRTC spec defines the following getStats[1]:
partial interface RTCPeerConnection {
Promise<RTCStatsReport> getStats(optional MediaStreamTrack? selector =
null);
};
interface RTCStatsReport {
readonly maplike<DOMString, object>; // object: RTCStats-derived
dictionaries
};
dictionary RTCStats {
DOMHighResTimeStamp timestamp;
RTCStatsType type;
DOMString id;
};
There currently exists a callback-based getStats method and related interfaces
that are different from the spec. To not break existing usages, these will be
kept for a transition period.
Before this CL, what we call "RTCStatsReport" is something other than what the
spec refers to (and is more similar to the spec's RTCStats but different).
A big difference between old and new stats API is that the old stats are
presented as string-string maps and the new API as well defined and typed
dictionary members (each deriviation should have its own .idl file). This makes
the two APIs incompatible.
Changes:
- The old RTCStatsReport is renamed to RTCLegacyStatsReport to avoid a name
conflict for when adding the new RTCStatsReport in a follow-up CL. This should
be low-risk since its a NoInterfaceObject.
- The callback-based getStats is changed in ways necessary to support two
getStats functions. E.g. it now returns Promise<void> because the return
values both have to be Promise<Foo>.
- The promise-based getStats is added behind runtime enabled test feature
"RTCPeerConnectionNewGetStats". It is not implemented yet and always rejects
its promise for now.
- The new signature is tested in RTCPeerConnection-statsPromise.html.
When using the desired IDL, the generated V8 code is unable to properly
distinguish between the two [getStats| signatures. In order to be able to
support them both anyway, |getStats(optional any)| is used and we examine the
value type. Bug crbug.com/629068 has been filed.
[1] https://www.w3.org/TR/2016/WD-webrtc-20160531/#sec.stats-model
BUG=627816, 629068
==========
Description was changed from
==========
Preparation CL for new Promise-based RTCPeerConnection.getStats.
The WebRTC spec defines the following getStats[1]:
partial interface RTCPeerConnection {
Promise<RTCStatsReport> getStats(optional MediaStreamTrack? selector =
null);
};
interface RTCStatsReport {
readonly maplike<DOMString, object>; // object: RTCStats-derived
dictionaries
};
dictionary RTCStats {
DOMHighResTimeStamp timestamp;
RTCStatsType type;
DOMString id;
};
There currently exists a callback-based getStats method and related interfaces
that are different from the spec. To not break existing usages, these will be
kept for a transition period.
Before this CL, what we call "RTCStatsReport" is something other than what the
spec refers to (and is more similar to the spec's RTCStats but different).
A big difference between old and new stats API is that the old stats are
presented as string-string maps and the new API as well defined and typed
dictionary members (each deriviation should have its own .idl file). This makes
the two APIs incompatible.
Changes:
- The old RTCStatsReport is renamed to RTCLegacyStatsReport to avoid a name
conflict for when adding the new RTCStatsReport in a follow-up CL. This should
be low-risk since its a NoInterfaceObject.
- The callback-based getStats is changed in ways necessary to support two
getStats functions. E.g. it now returns Promise<void> because the return
values both have to be Promise<Foo>.
- The promise-based getStats is added behind runtime enabled test feature
"RTCPeerConnectionNewGetStats". It is not implemented yet and always rejects
its promise for now.
- The new signature is tested in RTCPeerConnection-statsPromise.html.
When using the desired IDL, the generated V8 code is unable to properly
distinguish between the two [getStats| signatures. In order to be able to
support them both anyway, |getStats(optional any)| is used and we examine the
value type. Bug crbug.com/629068 has been filed.
[1] https://www.w3.org/TR/2016/WD-webrtc-20160531/#sec.stats-model
BUG=627816, 629068
==========
to
==========
Preparation CL for new Promise-based RTCPeerConnection.getStats.
The WebRTC spec defines the following getStats[1]:
partial interface RTCPeerConnection {
Promise<RTCStatsReport> getStats(optional MediaStreamTrack? selector =
null);
};
interface RTCStatsReport {
readonly maplike<DOMString, object>; // object: RTCStats-derived
dictionaries
};
dictionary RTCStats {
DOMHighResTimeStamp timestamp;
RTCStatsType type;
DOMString id;
};
There currently exists a callback-based getStats method and related interfaces
that are different from the spec. To not break existing usages, these will be
kept for a transition period.
Before this CL, what we call "RTCStatsReport" is something other than what the
spec refers to (and is more similar to the spec's RTCStats but different).
A big difference between old and new stats API is that the old stats are
presented as string-string maps and the new API as well defined and typed
dictionary members (each deriviation should have its own .idl file). This makes
the two APIs incompatible.
Changes:
- The old RTCStatsReport is renamed to RTCLegacyStatsReport to avoid a name
conflict for when adding the new RTCStatsReport in a follow-up CL. This should
be low-risk since its a NoInterfaceObject.
- The callback-based getStats is changed in ways necessary to support two
getStats functions. E.g. it now returns Promise<void> because the return
values both have to be Promise<Foo>.
- The promise-based getStats is added behind runtime enabled test feature
"RTCPeerConnectionNewGetStats". It is not implemented yet and always rejects
its promise for now.
- The new signature is tested in RTCPeerConnection-getStats-promise.html.
When using the desired IDL, the generated V8 code is unable to properly
distinguish between the two [getStats| signatures. In order to be able to
support them both anyway, |getStats(optional any)| is used and we examine the
value type. Bug crbug.com/629068 has been filed.
[1] https://www.w3.org/TR/2016/WD-webrtc-20160531/#sec.stats-model
BUG=627816, 629068
==========
PTAL hta. I updataed the RTCPeerConnection.idl comments to be agnostic as to why it is unable to distinguish between the two signatures. https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl (right): https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl:116: // dispatch to the correct |getStats| function based on the value type. On 2016/07/19 13:59:35, hbos_chromium wrote: > On 2016/07/19 09:56:48, hta - Chromium wrote: > > I read heycam to say that an interface is distinguishable from another > interface > > if "The two identified interfaces are not the same, it is not possible for a > > single platform object to implement both interfaces, and it is not the case > that > > both are callback interfaces." > > > > Does the WebIDL compiler give you an error when you try to make them separate? > > I might have misinterpreted something. I'm not sure if they *should* be > distinguishable. (Could an object have a handleEvent function and still be > considered a MediaStreamTrack?) > > I was able to get it to compile with both getStats, but due to the generated V8 > code the wrong getStats function wound up being called, it didn't distinguish. > (In line with spec or V8 gen bug?) > > I've spent a lot of time on this already. One idea was to change > RTCStatsCallback to a callback function but this isn't fully supported > (crbug.com/569301) and we would have to write a lot of V8 code manually instead. > This seemed less painful. I've confirmed a second time (crbug.com/629068#c5) that it is unable to distinguish. We can discuss whether this is a bug or by design there, but in the meantime, is this an acceptable workaround?
hbos@chromium.org changed reviewers: + asvitkine@chromium.org
+asvitkine for UserCounter.h and histograms.xml (UseCounter::count /w UseCounter::RTCPeerConnectionGetStats called from RTCPeerConnection.cpp:1025).
Patchset #6 (id:120001) has been deleted
And +bokan also for UserCounter.h and histograms.xml (UseCounter::count /w UseCounter::RTCPeerConnectionGetStats called from RTCPeerConnection.cpp:1025). (asvitkine: histograms.xml owner, bokan: UseCounter.h owner) https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl (right): https://codereview.chromium.org/2156063002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.idl:124: // evaluate the value type ourselves in C++. crbug.com/629068, 627816. On 2016/07/19 13:59:35, hbos_chromium wrote: > On 2016/07/19 09:56:48, hta - Chromium wrote: > > Can you add UMA counters here while you're at it? > > Good idea. I'd rather add and have that reviewed in a separate follow-up CL if > that's ok. Added a TODO comment (in RTCPeerConnection.h). I looked into it and the change is just a couple of lines so I did it in the same CL. It requires more owners though.
hbos@chromium.org changed reviewers: + bokan@chromium.org
Oops, +bokan wasn't added. There.
UseCounter.h and usage lgtm
lgtm
haraken@chromium.org changed reviewers: + haraken@chromium.org
I might want to see a conclusion of the discussion on bug 629068 (https://bugs.chromium.org/p/chromium/issues/detail?id=629068#c9) before rushing to land the hand-coded overload resolution algorithm.
Agree that we should wait a day or two to see if Haraken's folks can manage to sort out the code generator before submitting this code. Wanted to get a few more comments on test formatting in - functional code seems OK with me. https://codereview.chromium.org/2156063002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html (right): https://codereview.chromium.org/2156063002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html:10: var pc = new webkitRTCPeerConnection(null); I don't think you should indent the Javascript due to the HTML tags. It wastes horizontal space. https://codereview.chromium.org/2156063002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html:13: return navigator.mediaDevices.getUserMedia({audio:true, video:true}).then(function(mediaStream) { Note that the examples in https://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style break .then on a new line. I think this is more readable. https://codereview.chromium.org/2156063002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html:33: assert_unreached('Expected promise to be rejected.'); Use form "The promise should be rejected".
On 2016/07/21 23:02:37, hta - Chromium wrote: > Agree that we should wait a day or two to see if Haraken's folks can manage to > sort out the code generator before submitting this code. Wanted to get a few > more comments on test formatting in - functional code seems OK with me. I agreed. They've already put a CL out. It made it possible to distinguish between callback interface as a function and the overload, but not callback interface as an interface, status at crbug.com/629068. https://codereview.chromium.org/2156063002/diff/140001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html (right): https://codereview.chromium.org/2156063002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html:10: var pc = new webkitRTCPeerConnection(null); On 2016/07/21 23:02:37, hta - Chromium wrote: > I don't think you should indent the Javascript due to the HTML tags. It wastes > horizontal space. Other RTCPeerConnection testharness tests look this way, but I agree with you, removing indentation. https://codereview.chromium.org/2156063002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html:13: return navigator.mediaDevices.getUserMedia({audio:true, video:true}).then(function(mediaStream) { On 2016/07/21 23:02:37, hta - Chromium wrote: > Note that the examples in > https://www.chromium.org/blink/serviceworker/testing#TOC-Layout-Tests-Style > break .then on a new line. I think this is more readable. Done. https://codereview.chromium.org/2156063002/diff/140001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/peerconnection/RTCPeerConnection-getStats-promise.html:33: assert_unreached('Expected promise to be rejected.'); On 2016/07/21 23:02:37, hta - Chromium wrote: > Use form "The promise should be rejected". Done.
lgtm https://codereview.chromium.org/2156063002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp (right): https://codereview.chromium.org/2156063002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:970: return getStats(scriptState, static_cast<MediaStreamTrack*>(nullptr)); you shouldn't need to cast a nullptr
Description was changed from
==========
Preparation CL for new Promise-based RTCPeerConnection.getStats.
The WebRTC spec defines the following getStats[1]:
partial interface RTCPeerConnection {
Promise<RTCStatsReport> getStats(optional MediaStreamTrack? selector =
null);
};
interface RTCStatsReport {
readonly maplike<DOMString, object>; // object: RTCStats-derived
dictionaries
};
dictionary RTCStats {
DOMHighResTimeStamp timestamp;
RTCStatsType type;
DOMString id;
};
There currently exists a callback-based getStats method and related interfaces
that are different from the spec. To not break existing usages, these will be
kept for a transition period.
Before this CL, what we call "RTCStatsReport" is something other than what the
spec refers to (and is more similar to the spec's RTCStats but different).
A big difference between old and new stats API is that the old stats are
presented as string-string maps and the new API as well defined and typed
dictionary members (each deriviation should have its own .idl file). This makes
the two APIs incompatible.
Changes:
- The old RTCStatsReport is renamed to RTCLegacyStatsReport to avoid a name
conflict for when adding the new RTCStatsReport in a follow-up CL. This should
be low-risk since its a NoInterfaceObject.
- The callback-based getStats is changed in ways necessary to support two
getStats functions. E.g. it now returns Promise<void> because the return
values both have to be Promise<Foo>.
- The promise-based getStats is added behind runtime enabled test feature
"RTCPeerConnectionNewGetStats". It is not implemented yet and always rejects
its promise for now.
- The new signature is tested in RTCPeerConnection-getStats-promise.html.
When using the desired IDL, the generated V8 code is unable to properly
distinguish between the two [getStats| signatures. In order to be able to
support them both anyway, |getStats(optional any)| is used and we examine the
value type. Bug crbug.com/629068 has been filed.
[1] https://www.w3.org/TR/2016/WD-webrtc-20160531/#sec.stats-model
BUG=627816, 629068
==========
to
==========
Preparation CL for new Promise-based RTCPeerConnection.getStats.
The WebRTC spec defines the following getStats[1]:
partial interface RTCPeerConnection {
Promise<RTCStatsReport> getStats(optional MediaStreamTrack? selector =
null);
};
interface RTCStatsReport {
readonly maplike<DOMString, object>; // object: RTCStats-derived
dictionaries
};
dictionary RTCStats {
DOMHighResTimeStamp timestamp;
RTCStatsType type;
DOMString id;
};
There currently exists a callback-based getStats method and related interfaces
that are different from the spec. To not break existing usages, these will be
kept for a transition period.
Before this CL, what we call "RTCStatsReport" is something other than what the
spec refers to (and is more similar to the spec's RTCStats but different).
A big difference between old and new stats API is that the old stats are
presented as string-string maps and the new API as well defined and typed
dictionary members (each deriviation should have its own .idl file). This makes
the two APIs incompatible.
Changes:
- The old RTCStatsReport is renamed to RTCLegacyStatsReport to avoid a name
conflict for when adding the new RTCStatsReport in a follow-up CL. This should
be low-risk since its a NoInterfaceObject.
- The callback-based getStats is changed in ways necessary to support two
getStats functions. E.g. it now returns Promise<void> because the return
values both have to be Promise<Foo>.
- The promise-based getStats is added behind runtime enabled test feature
"RTCPeerConnectionNewGetStats". It is not implemented yet and always rejects
its promise for now.
- The new signature is tested in RTCPeerConnection-getStats-promise.html.
[1] https://www.w3.org/TR/2016/WD-webrtc-20160531/#sec.stats-model
BUG=627816, 629068
==========
The CQ bit was checked by hbos@chromium.org to run a CQ dry run
The CQ bit was unchecked by hbos@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by hbos@chromium.org
PTAL hta, mkwst (and haraken?) With the overload bug fixed (https://codereview.chromium.org/2175463002) I was able to remove all custom overload algorithm code and use the correct .idl. https://codereview.chromium.org/2156063002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp (right): https://codereview.chromium.org/2156063002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/peerconnection/RTCPeerConnection.cpp:970: return getStats(scriptState, static_cast<MediaStreamTrack*>(nullptr)); On 2016/07/25 08:31:17, tommi-chrömium wrote: > you shouldn't need to cast a nullptr (Removed all of this code now that getStats overloading works with WebIDL.)
Bump mkwst
LGTM. Much nicer without the custom code, thanks!
The CQ bit was checked by hbos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org, asvitkine@chromium.org, tommi@chromium.org, mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/2156063002/#ps240001 (title: "Rebase with master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
hta is OOO. I have the LGTMs, landing.
Message was sent while issue was closed.
Description was changed from
==========
Preparation CL for new Promise-based RTCPeerConnection.getStats.
The WebRTC spec defines the following getStats[1]:
partial interface RTCPeerConnection {
Promise<RTCStatsReport> getStats(optional MediaStreamTrack? selector =
null);
};
interface RTCStatsReport {
readonly maplike<DOMString, object>; // object: RTCStats-derived
dictionaries
};
dictionary RTCStats {
DOMHighResTimeStamp timestamp;
RTCStatsType type;
DOMString id;
};
There currently exists a callback-based getStats method and related interfaces
that are different from the spec. To not break existing usages, these will be
kept for a transition period.
Before this CL, what we call "RTCStatsReport" is something other than what the
spec refers to (and is more similar to the spec's RTCStats but different).
A big difference between old and new stats API is that the old stats are
presented as string-string maps and the new API as well defined and typed
dictionary members (each deriviation should have its own .idl file). This makes
the two APIs incompatible.
Changes:
- The old RTCStatsReport is renamed to RTCLegacyStatsReport to avoid a name
conflict for when adding the new RTCStatsReport in a follow-up CL. This should
be low-risk since its a NoInterfaceObject.
- The callback-based getStats is changed in ways necessary to support two
getStats functions. E.g. it now returns Promise<void> because the return
values both have to be Promise<Foo>.
- The promise-based getStats is added behind runtime enabled test feature
"RTCPeerConnectionNewGetStats". It is not implemented yet and always rejects
its promise for now.
- The new signature is tested in RTCPeerConnection-getStats-promise.html.
[1] https://www.w3.org/TR/2016/WD-webrtc-20160531/#sec.stats-model
BUG=627816, 629068
==========
to
==========
Preparation CL for new Promise-based RTCPeerConnection.getStats.
The WebRTC spec defines the following getStats[1]:
partial interface RTCPeerConnection {
Promise<RTCStatsReport> getStats(optional MediaStreamTrack? selector =
null);
};
interface RTCStatsReport {
readonly maplike<DOMString, object>; // object: RTCStats-derived
dictionaries
};
dictionary RTCStats {
DOMHighResTimeStamp timestamp;
RTCStatsType type;
DOMString id;
};
There currently exists a callback-based getStats method and related interfaces
that are different from the spec. To not break existing usages, these will be
kept for a transition period.
Before this CL, what we call "RTCStatsReport" is something other than what the
spec refers to (and is more similar to the spec's RTCStats but different).
A big difference between old and new stats API is that the old stats are
presented as string-string maps and the new API as well defined and typed
dictionary members (each deriviation should have its own .idl file). This makes
the two APIs incompatible.
Changes:
- The old RTCStatsReport is renamed to RTCLegacyStatsReport to avoid a name
conflict for when adding the new RTCStatsReport in a follow-up CL. This should
be low-risk since its a NoInterfaceObject.
- The callback-based getStats is changed in ways necessary to support two
getStats functions. E.g. it now returns Promise<void> because the return
values both have to be Promise<Foo>.
- The promise-based getStats is added behind runtime enabled test feature
"RTCPeerConnectionNewGetStats". It is not implemented yet and always rejects
its promise for now.
- The new signature is tested in RTCPeerConnection-getStats-promise.html.
[1] https://www.w3.org/TR/2016/WD-webrtc-20160531/#sec.stats-model
BUG=627816, 629068
==========
Message was sent while issue was closed.
Committed patchset #11 (id:240001)
Message was sent while issue was closed.
Description was changed from
==========
Preparation CL for new Promise-based RTCPeerConnection.getStats.
The WebRTC spec defines the following getStats[1]:
partial interface RTCPeerConnection {
Promise<RTCStatsReport> getStats(optional MediaStreamTrack? selector =
null);
};
interface RTCStatsReport {
readonly maplike<DOMString, object>; // object: RTCStats-derived
dictionaries
};
dictionary RTCStats {
DOMHighResTimeStamp timestamp;
RTCStatsType type;
DOMString id;
};
There currently exists a callback-based getStats method and related interfaces
that are different from the spec. To not break existing usages, these will be
kept for a transition period.
Before this CL, what we call "RTCStatsReport" is something other than what the
spec refers to (and is more similar to the spec's RTCStats but different).
A big difference between old and new stats API is that the old stats are
presented as string-string maps and the new API as well defined and typed
dictionary members (each deriviation should have its own .idl file). This makes
the two APIs incompatible.
Changes:
- The old RTCStatsReport is renamed to RTCLegacyStatsReport to avoid a name
conflict for when adding the new RTCStatsReport in a follow-up CL. This should
be low-risk since its a NoInterfaceObject.
- The callback-based getStats is changed in ways necessary to support two
getStats functions. E.g. it now returns Promise<void> because the return
values both have to be Promise<Foo>.
- The promise-based getStats is added behind runtime enabled test feature
"RTCPeerConnectionNewGetStats". It is not implemented yet and always rejects
its promise for now.
- The new signature is tested in RTCPeerConnection-getStats-promise.html.
[1] https://www.w3.org/TR/2016/WD-webrtc-20160531/#sec.stats-model
BUG=627816, 629068
==========
to
==========
Preparation CL for new Promise-based RTCPeerConnection.getStats.
The WebRTC spec defines the following getStats[1]:
partial interface RTCPeerConnection {
Promise<RTCStatsReport> getStats(optional MediaStreamTrack? selector =
null);
};
interface RTCStatsReport {
readonly maplike<DOMString, object>; // object: RTCStats-derived
dictionaries
};
dictionary RTCStats {
DOMHighResTimeStamp timestamp;
RTCStatsType type;
DOMString id;
};
There currently exists a callback-based getStats method and related interfaces
that are different from the spec. To not break existing usages, these will be
kept for a transition period.
Before this CL, what we call "RTCStatsReport" is something other than what the
spec refers to (and is more similar to the spec's RTCStats but different).
A big difference between old and new stats API is that the old stats are
presented as string-string maps and the new API as well defined and typed
dictionary members (each deriviation should have its own .idl file). This makes
the two APIs incompatible.
Changes:
- The old RTCStatsReport is renamed to RTCLegacyStatsReport to avoid a name
conflict for when adding the new RTCStatsReport in a follow-up CL. This should
be low-risk since its a NoInterfaceObject.
- The callback-based getStats is changed in ways necessary to support two
getStats functions. E.g. it now returns Promise<void> because the return
values both have to be Promise<Foo>.
- The promise-based getStats is added behind runtime enabled test feature
"RTCPeerConnectionNewGetStats". It is not implemented yet and always rejects
its promise for now.
- The new signature is tested in RTCPeerConnection-getStats-promise.html.
[1] https://www.w3.org/TR/2016/WD-webrtc-20160531/#sec.stats-model
BUG=627816, 629068
Committed: https://crrev.com/08fa67b0f718b342b8c47725f2018c63ade8b699
Cr-Commit-Position: refs/heads/master@{#408096}
==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/08fa67b0f718b342b8c47725f2018c63ade8b699 Cr-Commit-Position: refs/heads/master@{#408096}
Message was sent while issue was closed.
lgtm post-submit lgtm. |
