Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1661493002:
Add promise-based addIceCandidate, setLocalDescription and setRemoteDescription to RTCPeerConnection (Closed)
|
Created:
4 years, 10 months ago by Guido Urdaneta Modified:
4 years, 10 months ago CC:
chromium-reviews, blink-reviews, tommyw+watchlist_chromium.org, hta - Chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd promise-based addIceCandidate, setLocalDescription and setRemoteDescription to RTCPeerConnection
Intent to Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/zVlp8jIb2XU
BUG=499126
Committed: https://crrev.com/e16c96f5a5b517a64fdbc5be75f05e359e04b461
Cr-Commit-Position: refs/heads/master@{#376163}
Patch Set 1 #Patch Set 2 : Replace "TBD" with "InternalError" #
Total comments: 35
Patch Set 3 : Remove fulfilled TODO and rebase #Patch Set 4 : Some philipj's comments #
Total comments: 4
Patch Set 5 : Use microtask to run callbacks asynchronously #Patch Set 6 : rebase #
Total comments: 16
Patch Set 7 : yhirano's comments #Patch Set 8 : yhirano's comments #
Total comments: 9
Patch Set 9 : Return resolved promises with undefined #
Total comments: 8
Patch Set 10 : philipj's comments #Patch Set 11 : remove unncessary include #Patch Set 12 : remove activedomobjects check #Patch Set 13 : stop promise resolver when the promise must remain unresolved #
Total comments: 4
Patch Set 14 : yhirano's comments #
Total comments: 27
Patch Set 15 : haraken and philipj's comments #
Total comments: 2
Patch Set 16 : rebase, philipj's comments and execution-order tests for legacy functions #Patch Set 17 : return ScriptPromise(undefined) instead of ScriptPromise() #
Total comments: 1
Patch Set 18 : Use ScriptPromiseResolver::detach() to leave the promise unresolved. #
Total comments: 4
Patch Set 19 : philipj's comments on tests #
Total comments: 2
Messages
Total messages: 87 (17 generated)
The CQ bit was checked by guidou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1661493002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1661493002/1
guidou@chromium.org changed reviewers: + peter@chromium.org
Hi, PTAL
Description was changed from ========== Add promise-based addIceCandidate, setLocalDescription and setRemoteDescription to RTCPeerConnection Intent to Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/zVlp8jIb2XU BUG=499126 ========== to ========== Add promise-based addIceCandidate, setLocalDescription and setRemoteDescription to RTCPeerConnection Intent to Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/zVlp8jIb2XU BUG=499126 ==========
peter@chromium.org changed reviewers: + avayvod@chromium.org
+avayvod
The CQ bit was checked by guidou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1661493002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1661493002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Friendly ping :)
guidou@chromium.org changed reviewers: + philipj@opera.com - avayvod@chromium.org, peter@chromium.org
philipj: can you take a look?
hta@chromium.org changed reviewers: + hta@chromium.org
To me it looks good. https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl (right): https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl:84: // TODO(guidou): addIceCandidate() should return a Promise. Delete fulfilled TODO.
This is mostly new code to me, pardon my confusion :) https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:89: const char kDefaultErrorName[] = "InternalError"; I see "Ask the DOM team to extend their list with the following errors" issue in the spec, can you make sure that an issue is filed on http://heycam.github.io/webidl/#idl-DOMException-error-names ? It seems pretty likely to me that you'll be asked to use an existing generic error name instead of InternalError, at least. For things that you expect to be added to DOMException, instead add them to ExceptionCode and then use in the same way as InvalidStateError and friends. The spec also talks about DOMError, which shouldn't be used for anything new, but fortunately it looks like it isn't actually used, MediaErrorState::raiseException instead throws a DOMExceptions. Are these error conditions new, or why don't they appear anywhere in the existing code? https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:102: static bool callErrorCallbackIfSignalingStateClosed(RTCPeerConnection::SignalingState state, RTCErrorCallback* errorCallback) I think you don't need static inside this anonymous namespace. If not, drop it above as well. https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:113: extra newline https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:125: WebRTCICECandidate ConvertToWebRTCIceCandidate(const RTCIceCandidateInitOrRTCIceCandidate& candidate) Lower-case convertTo https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:528: if (callErrorCallbackIfSignalingStateClosed(m_signalingState, errorCallback)) This can synchronously invoke the error callback inside the setLocalDescriptionCall(), which I suspect isn't what was intended by the spec. It defines this in terms of the promise variant, but says "Upon rejection of p with reason r, invoke failureCallback with r as the argument" which is too imprecise to say what it means. Either it means to add steps to the call sites that rejects the promise, or it means that the callback should be called at the same time as promise rejection callbacks are invoked. I think only the second interpretation makes sense, because it is what could be implemented using JavaScript on top of the promise API, and it avoids invoking the error callback synchronously. https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:693: resolver->reject(DOMException::create(NotSupportedError, "This method is not yet implemented.")); Can this actually happen? The whole source code is under your control, no? :) https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:710: errorCallback->handleEvent("This method is not yet implemented."); This is also a synchronous error callback, but in a different shape. https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl (right): https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl:103: // TODO(guidou): The failureCallback argument should be non-optional. Can you first make the arguments non-optional in a separate CL, so that in the case of regressions it's more clear which change is to blame? https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp (right): https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:36: bool shouldFireCallback = m_requester ? m_requester->shouldFireDefaultCallbacks() : false; true/false in ternary expressions can always be rewritten, here to m_requester && m_requested->shouldFireDefaultCallbacks() which I find slightly clearer. https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:47: m_resolver->reject(DOMError::create(m_errorName, error)); Ah, DOMError. Whatever the spec may say, we shouldn't be using DOMError anywhere new, it's gone from most specs and jsbell@ and I have been trying to eliminate it. https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.h (right): https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.h:18: WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(RTCVoidRequestPromiseImpl); Looks like this class will only work with Oilpan, so skip the transitional macro? https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.h:25: void requestFailed(const String& error) override; This seems a bit strange, shouldn't the type of error also be in this callback and not in the constructor? How can the constructor know in which way it will fail? https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.h:27: // ActiveDOMObject What happens if this is not an ActiveDOMObject and is there test case coverage for that?
Comment on the error thing: In the spec discussions, this is under active review (Dom made a bunch of proposals). So we are aiming mainly to return the same errors we did last week, so that we do not surprise users by changing the stuff twice (once to what-we-think-will-happen and once to what-actually-happens). Agree that it seems cleaner to include the code as a parameter to the error method, even if all the callers return "internal error" this week. The lower layer code doesn't distinguish error codes yet, it only returns different messages. On Tue, Feb 9, 2016 at 1:02 PM, <philipj@opera.com> wrote: > This is mostly new code to me, pardon my confusion :) > > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp > (right): > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:89 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > const char kDefaultErrorName[] = "InternalError"; > I see "Ask the DOM team to extend their list with the following errors" > issue in the spec, can you make sure that an issue is filed onhttp://heycam.github.io/webidl/#idl-DOMException-error-names ? > > It seems pretty likely to me that you'll be asked to use an existing > generic error name instead of InternalError, at least. > > For things that you expect to be added to DOMException, instead add them > to ExceptionCode and then use in the same way as InvalidStateError and > friends. > > The spec also talks about DOMError, which shouldn't be used for anything > new, but fortunately it looks like it isn't actually used, > MediaErrorState::raiseException instead throws a DOMExceptions. > > Are these error conditions new, or why don't they appear anywhere in the > existing code? > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:102 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > static bool > callErrorCallbackIfSignalingStateClosed(RTCPeerConnection::SignalingState > state, RTCErrorCallback* errorCallback) > I think you don't need static inside this anonymous namespace. If not, > drop it above as well. > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:113 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > > extra newline > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:125 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > WebRTCICECandidate ConvertToWebRTCIceCandidate(const > RTCIceCandidateInitOrRTCIceCandidate& candidate) > Lower-case convertTo > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:528 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > if (callErrorCallbackIfSignalingStateClosed(m_signalingState, > errorCallback)) > This can synchronously invoke the error callback inside the > setLocalDescriptionCall(), which I suspect isn't what was intended by > the spec. It defines this in terms of the promise variant, but says > "Upon rejection of p with reason r, invoke failureCallback with r as the > argument" which is too imprecise to say what it means. > > Either it means to add steps to the call sites that rejects the promise, > or it means that the callback should be called at the same time as > promise rejection callbacks are invoked. I think only the second > interpretation makes sense, because it is what could be implemented > using JavaScript on top of the promise API, and it avoids invoking the > error callback synchronously. > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:693 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > resolver->reject(DOMException::create(NotSupportedError, "This method is > not yet implemented.")); > Can this actually happen? The whole source code is under your control, > no? :) > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:710 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > errorCallback->handleEvent("This method is not yet implemented."); > This is also a synchronous error callback, but in a different shape. > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl > (right): > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl:103 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > // TODO(guidou): The failureCallback argument should be non-optional. > Can you first make the arguments non-optional in a separate CL, so that > in the case of regressions it's more clear which change is to blame? > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > File > third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp > (right): > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:36 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > bool shouldFireCallback = m_requester ? > m_requester->shouldFireDefaultCallbacks() : false; > true/false in ternary expressions can always be rewritten, here to > m_requester && m_requested->shouldFireDefaultCallbacks() which I find > slightly clearer. > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:47 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > m_resolver->reject(DOMError::create(m_errorName, error)); > Ah, DOMError. Whatever the spec may say, we shouldn't be using DOMError > anywhere new, it's gone from most specs and jsbell@ and I have been > trying to eliminate it. > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > File > third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.h > (right): > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.h:18 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(RTCVoidRequestPromiseImpl); > Looks like this class will only work with Oilpan, so skip the > transitional macro? > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.h:25 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > void requestFailed(const String& error) override; > This seems a bit strange, shouldn't the type of error also be in this > callback and not in the constructor? How can the constructor know in > which way it will fail? > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.h:27 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > // ActiveDOMObject > What happens if this is not an ActiveDOMObject and is there test case > coverage for that? > https://codereview.chromium.org/1661493002/ > > -- 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.
Comment on the error thing: In the spec discussions, this is under active review (Dom made a bunch of proposals). So we are aiming mainly to return the same errors we did last week, so that we do not surprise users by changing the stuff twice (once to what-we-think-will-happen and once to what-actually-happens). Agree that it seems cleaner to include the code as a parameter to the error method, even if all the callers return "internal error" this week. The lower layer code doesn't distinguish error codes yet, it only returns different messages. On Tue, Feb 9, 2016 at 1:02 PM, <philipj@opera.com> wrote: > This is mostly new code to me, pardon my confusion :) > > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp > (right): > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:89 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > const char kDefaultErrorName[] = "InternalError"; > I see "Ask the DOM team to extend their list with the following errors" > issue in the spec, can you make sure that an issue is filed onhttp://heycam.github.io/webidl/#idl-DOMException-error-names ? > > It seems pretty likely to me that you'll be asked to use an existing > generic error name instead of InternalError, at least. > > For things that you expect to be added to DOMException, instead add them > to ExceptionCode and then use in the same way as InvalidStateError and > friends. > > The spec also talks about DOMError, which shouldn't be used for anything > new, but fortunately it looks like it isn't actually used, > MediaErrorState::raiseException instead throws a DOMExceptions. > > Are these error conditions new, or why don't they appear anywhere in the > existing code? > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:102 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > static bool > callErrorCallbackIfSignalingStateClosed(RTCPeerConnection::SignalingState > state, RTCErrorCallback* errorCallback) > I think you don't need static inside this anonymous namespace. If not, > drop it above as well. > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:113 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > > extra newline > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:125 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > WebRTCICECandidate ConvertToWebRTCIceCandidate(const > RTCIceCandidateInitOrRTCIceCandidate& candidate) > Lower-case convertTo > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:528 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > if (callErrorCallbackIfSignalingStateClosed(m_signalingState, > errorCallback)) > This can synchronously invoke the error callback inside the > setLocalDescriptionCall(), which I suspect isn't what was intended by > the spec. It defines this in terms of the promise variant, but says > "Upon rejection of p with reason r, invoke failureCallback with r as the > argument" which is too imprecise to say what it means. > > Either it means to add steps to the call sites that rejects the promise, > or it means that the callback should be called at the same time as > promise rejection callbacks are invoked. I think only the second > interpretation makes sense, because it is what could be implemented > using JavaScript on top of the promise API, and it avoids invoking the > error callback synchronously. > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:693 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > resolver->reject(DOMException::create(NotSupportedError, "This method is > not yet implemented.")); > Can this actually happen? The whole source code is under your control, > no? :) > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:710 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > errorCallback->handleEvent("This method is not yet implemented."); > This is also a synchronous error callback, but in a different shape. > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl > (right): > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl:103 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > // TODO(guidou): The failureCallback argument should be non-optional. > Can you first make the arguments non-optional in a separate CL, so that > in the case of regressions it's more clear which change is to blame? > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > File > third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp > (right): > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:36 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > bool shouldFireCallback = m_requester ? > m_requester->shouldFireDefaultCallbacks() : false; > true/false in ternary expressions can always be rewritten, here to > m_requester && m_requested->shouldFireDefaultCallbacks() which I find > slightly clearer. > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:47 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > m_resolver->reject(DOMError::create(m_errorName, error)); > Ah, DOMError. Whatever the spec may say, we shouldn't be using DOMError > anywhere new, it's gone from most specs and jsbell@ and I have been > trying to eliminate it. > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > File > third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.h > (right): > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.h:18 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(RTCVoidRequestPromiseImpl); > Looks like this class will only work with Oilpan, so skip the > transitional macro? > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.h:25 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > void requestFailed(const String& error) override; > This seems a bit strange, shouldn't the type of error also be in this > callback and not in the constructor? How can the constructor know in > which way it will fail? > https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.h:27 <https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour...>: > // ActiveDOMObject > What happens if this is not an ActiveDOMObject and is there test case > coverage for that? > https://codereview.chromium.org/1661493002/ > > -- 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.
On 2016/02/09 12:40:17, chromium-reviews wrote: > Comment on the error thing: In the spec discussions, this is under active > review (Dom made a bunch of proposals). So we are aiming mainly to return > the same errors we did last week, so that we do not surprise users by > changing the stuff twice (once to what-we-think-will-happen and once to > what-actually-happens). > > Agree that it seems cleaner to include the code as a parameter to the error > method, even if all the callers return "internal error" this week. The > lower layer code doesn't distinguish error codes yet, it only returns > different messages. (That was hta@) OK, "lower layer code doesn't distinguish error codes yet" explains the function signature. About errors, I agree it would make sense to change this only once. Was DOMError and these things that look new in the CL already somewhere else that I have missed, or should those bits be trimmed from this CL?
> OK, "lower layer code doesn't distinguish error codes yet" explains the function > signature. > > About errors, I agree it would make sense to change this only once. Was DOMError > and these things that look new in the CL already somewhere else that I have > missed, or should those bits be trimmed from this CL? The old code reports errors as strings, so it uses neither DOMError nor DOMException. The latest version of the spec indicates that one should use DOMError with error name TBD (there is a spec bug about the TBD part). I used "InternalError" which sounded less bad than TBD to me, but until the spec clarifies this I guess we can use any value that makes sense. What would you suggest?
https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:89: const char kDefaultErrorName[] = "InternalError"; On 2016/02/09 12:02:03, philipj_UTC7 wrote: > I see "Ask the DOM team to extend their list with the following errors" issue in > the spec, can you make sure that an issue is filed on > http://heycam.github.io/webidl/#idl-DOMException-error-names ? > > It seems pretty likely to me that you'll be asked to use an existing generic > error name instead of InternalError, at least. > > For things that you expect to be added to DOMException, instead add them to > ExceptionCode and then use in the same way as InvalidStateError and friends. > > The spec also talks about DOMError, which shouldn't be used for anything new, > but fortunately it looks like it isn't actually used, > MediaErrorState::raiseException instead throws a DOMExceptions. > > Are these error conditions new, or why don't they appear anywhere in the > existing code? The spec says to use DOMError with error name TBD. There is a bug filed about the TBD part. I used InternalError just because it is a value used elsewhere in the spec. What value would you suggest? https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:102: static bool callErrorCallbackIfSignalingStateClosed(RTCPeerConnection::SignalingState state, RTCErrorCallback* errorCallback) On 2016/02/09 12:02:03, philipj_UTC7 wrote: > I think you don't need static inside this anonymous namespace. If not, drop it > above as well. Done. https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:113: On 2016/02/09 12:02:03, philipj_UTC7 wrote: > extra newline Done. https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:125: WebRTCICECandidate ConvertToWebRTCIceCandidate(const RTCIceCandidateInitOrRTCIceCandidate& candidate) On 2016/02/09 12:02:03, philipj_UTC7 wrote: > Lower-case convertTo Done. https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:528: if (callErrorCallbackIfSignalingStateClosed(m_signalingState, errorCallback)) On 2016/02/09 12:02:03, philipj_UTC7 wrote: > This can synchronously invoke the error callback inside the > setLocalDescriptionCall(), which I suspect isn't what was intended by the spec. > It defines this in terms of the promise variant, but says "Upon rejection of p > with reason r, invoke failureCallback with r as the argument" which is too > imprecise to say what it means. > > Either it means to add steps to the call sites that rejects the promise, or it > means that the callback should be called at the same time as promise rejection > callbacks are invoked. I think only the second interpretation makes sense, > because it is what could be implemented using JavaScript on top of the promise > API, and it avoids invoking the error callback synchronously. Now calling the error callback asynchronously. https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:693: resolver->reject(DOMException::create(NotSupportedError, "This method is not yet implemented.")); On 2016/02/09 12:02:03, philipj_UTC7 wrote: > Can this actually happen? The whole source code is under your control, no? :) Apparently, it can. https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... I left NotSupportedError because that is the exception that was being thrown in the legacy. Looking at the spec, we should return DOMError(TBD), or perhaps DOMError("InvalidCandidate"), as can be inferred from the spec bug. https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:710: errorCallback->handleEvent("This method is not yet implemented."); On 2016/02/09 12:02:03, philipj_UTC7 wrote: > This is also a synchronous error callback, but in a different shape. Now called asynchronously. https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl (right): https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl:84: // TODO(guidou): addIceCandidate() should return a Promise. On 2016/02/09 10:59:08, hta - Chromium wrote: > Delete fulfilled TODO. Done. https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl:103: // TODO(guidou): The failureCallback argument should be non-optional. On 2016/02/09 12:02:03, philipj_UTC7 wrote: > Can you first make the arguments non-optional in a separate CL, so that in the > case of regressions it's more clear which change is to blame? Done. https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp (right): https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:36: bool shouldFireCallback = m_requester ? m_requester->shouldFireDefaultCallbacks() : false; On 2016/02/09 12:02:03, philipj_UTC7 wrote: > true/false in ternary expressions can always be rewritten, here to m_requester > && m_requested->shouldFireDefaultCallbacks() which I find slightly clearer. Done. https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:47: m_resolver->reject(DOMError::create(m_errorName, error)); On 2016/02/09 12:02:03, philipj_UTC7 wrote: > Ah, DOMError. Whatever the spec may say, we shouldn't be using DOMError anywhere > new, it's gone from most specs and jsbell@ and I have been trying to eliminate > it. Should I use DOMException instead then? https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.h (right): https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.h:18: WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(RTCVoidRequestPromiseImpl); On 2016/02/09 12:02:04, philipj_UTC7 wrote: > Looks like this class will only work with Oilpan, so skip the transitional > macro? Done. https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.h:25: void requestFailed(const String& error) override; On 2016/02/09 12:02:04, philipj_UTC7 wrote: > This seems a bit strange, shouldn't the type of error also be in this callback > and not in the constructor? How can the constructor know in which way it will > fail? Already explained by hta. https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.h:27: // ActiveDOMObject On 2016/02/09 12:02:04, philipj_UTC7 wrote: > What happens if this is not an ActiveDOMObject and is there test case coverage > for that? I removed the inheritance from ActiveDOMObject and instead am checking that ActiveDOMObjects are not stopped in the resolver's execution context when the promise is about to be accepted or rejected.
There's a chance I'll be OOO for the next round, I'll update my nick if so. https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:89: const char kDefaultErrorName[] = "InternalError"; On 2016/02/09 16:51:01, Guido Urdaneta wrote: > On 2016/02/09 12:02:03, philipj_UTC7 wrote: > > I see "Ask the DOM team to extend their list with the following errors" issue > in > > the spec, can you make sure that an issue is filed on > > http://heycam.github.io/webidl/#idl-DOMException-error-names ? > > > > It seems pretty likely to me that you'll be asked to use an existing generic > > error name instead of InternalError, at least. > > > > For things that you expect to be added to DOMException, instead add them to > > ExceptionCode and then use in the same way as InvalidStateError and friends. > > > > The spec also talks about DOMError, which shouldn't be used for anything new, > > but fortunately it looks like it isn't actually used, > > MediaErrorState::raiseException instead throws a DOMExceptions. > > > > Are these error conditions new, or why don't they appear anywhere in the > > existing code? > > The spec says to use DOMError with error name TBD. There is a bug filed about > the TBD part. I used InternalError just because it is a value used elsewhere in > the spec. What value would you suggest? Is there a WebIDL issue filed for adding any of the new names? I can't see any in https://github.com/heycam/webidl/issues, can you do that? Or poke someone? I think using DOMException with the best matching of the existing types would make sense. What that is depends on what usually causes InternalError. I could imagine NotSupportedError, InvalidStateError, SyntaxError, NetworkError, AbortError, TimeoutError, ConstraintError or perhaps OperationError. https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:528: if (callErrorCallbackIfSignalingStateClosed(m_signalingState, errorCallback)) On 2016/02/09 16:51:01, Guido Urdaneta wrote: > On 2016/02/09 12:02:03, philipj_UTC7 wrote: > > This can synchronously invoke the error callback inside the > > setLocalDescriptionCall(), which I suspect isn't what was intended by the > spec. > > It defines this in terms of the promise variant, but says "Upon rejection of p > > with reason r, invoke failureCallback with r as the argument" which is too > > imprecise to say what it means. > > > > Either it means to add steps to the call sites that rejects the promise, or it > > means that the callback should be called at the same time as promise rejection > > callbacks are invoked. I think only the second interpretation makes sense, > > because it is what could be implemented using JavaScript on top of the promise > > API, and it avoids invoking the error callback synchronously. > > Now calling the error callback asynchronously. Thanks, see new comments for more timing fun :) https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:693: resolver->reject(DOMException::create(NotSupportedError, "This method is not yet implemented.")); On 2016/02/09 16:51:01, Guido Urdaneta wrote: > On 2016/02/09 12:02:03, philipj_UTC7 wrote: > > Can this actually happen? The whole source code is under your control, no? :) > > Apparently, it can. > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > I left NotSupportedError because that is the exception that was being thrown in > the legacy. Looking at the spec, we should return DOMError(TBD), or perhaps > DOMError("InvalidCandidate"), as can be inferred from the spec bug. OK, it's not obvious from RTCPeerConnectionHandler::addICECandidate that returning false means a missing implementation, there are other error cases in WebRtcSession::ProcessIceMessage. But, this isn't new code. Maybe add a TODO to make it more truthful about the cause? https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp (right): https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:47: m_resolver->reject(DOMError::create(m_errorName, error)); On 2016/02/09 16:51:01, Guido Urdaneta wrote: > On 2016/02/09 12:02:03, philipj_UTC7 wrote: > > Ah, DOMError. Whatever the spec may say, we shouldn't be using DOMError > anywhere > > new, it's gone from most specs and jsbell@ and I have been trying to eliminate > > it. > > Should I use DOMException instead then? I would recommend that, yes. Is https://github.com/w3c/webrtc-pc/issues/319 the right place to point out that DOMError should not be used, or is there a separate spec issue? Should I file one? https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.h (right): https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.h:27: // ActiveDOMObject On 2016/02/09 16:51:01, Guido Urdaneta wrote: > On 2016/02/09 12:02:04, philipj_UTC7 wrote: > > What happens if this is not an ActiveDOMObject and is there test case coverage > > for that? > > I removed the inheritance from ActiveDOMObject and instead am checking that > ActiveDOMObjects are not stopped in the resolver's execution context when the > promise is about to be accepted or rejected. Makes sense! https://codereview.chromium.org/1661493002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1661493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:118: Timer<ErrorCallbackRunner> m_timer; To get the same timing as a promise rejection, what you need is a microtask, which runs after this task has finished but before returning to the event loop. Scripts can tell the difference, if they do something that does "queue a task to fire an event named ... at ..." right before making the call that creates this ErrorCallbackRunner. The order of the event listener and the error callback will be reversed. When updating the test for the legacy variants, you'll probably want to sandwich the failure between two new Promise() rejections of your own, and see that the callback order is the same. This may seem nitpicky, but I think it's what the spec is trying to say. Would be very simply to implement as JS, but that's already been discussed on blink-dev and doesn't look promising right now. https://codereview.chromium.org/1661493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:762: if (!implemented) { Drop added { }?
Oh, right. In the "Intent to Remove: Support for optional error handler in RTCPeerConnection's createOffer() and createAnswer()" it was said that "We believe it is a good idea to let these changes in the legacy API stabilize during one release cycle, giving developers some time to update their applications." Is this still the plan, should landing this wait until after the next branch point?
We decided that it was possible to ship these 3 functions as promises in this version, and let the ones that are blocked on a deprecation (createOffer and createAnswer) wait until the next version. This will create some more diversity in the API for a while, but also means that we'll not introduce too many changes at the same time. On Tue, Feb 9, 2016 at 8:03 PM, <philipj@opera.com> wrote: > Oh, right. In the "Intent to Remove: Support for optional error handler in > RTCPeerConnection's createOffer() and createAnswer()" it was said that "We > believe it is a good idea to let these changes in the legacy API stabilize > during one release cycle, giving developers some time to update their > applications." > > Is this still the plan, should landing this wait until after the next branch > point? > https://codereview.chromium.org/1661493002/ > > -- 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.
We decided that it was possible to ship these 3 functions as promises in this version, and let the ones that are blocked on a deprecation (createOffer and createAnswer) wait until the next version. This will create some more diversity in the API for a while, but also means that we'll not introduce too many changes at the same time. On Tue, Feb 9, 2016 at 8:03 PM, <philipj@opera.com> wrote: > Oh, right. In the "Intent to Remove: Support for optional error handler in > RTCPeerConnection's createOffer() and createAnswer()" it was said that "We > believe it is a good idea to let these changes in the legacy API stabilize > during one release cycle, giving developers some time to update their > applications." > > Is this still the plan, should landing this wait until after the next branch > point? > https://codereview.chromium.org/1661493002/ > > -- 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.
> Is there a WebIDL issue filed for adding any of the new names? I can't see any > in https://github.com/heycam/webidl/issues, can you do that? Or poke someone? > I would not know which new name to add there, since the WebRTC spec says TBD. > I think using DOMException with the best matching of the existing types would > make sense. What that is depends on what usually causes InternalError. I could > imagine NotSupportedError, InvalidStateError, SyntaxError, NetworkError, > AbortError, TimeoutError, ConstraintError or perhaps OperationError. I'll do that in the meantime. > > > Ah, DOMError. Whatever the spec may say, we shouldn't be using DOMError > > anywhere > > > new, it's gone from most specs and jsbell@ and I have been trying to > eliminate > > > it. > > > > Should I use DOMException instead then? > > I would recommend that, yes. Is https://github.com/w3c/webrtc-pc/issues/319 the > right place to point out that DOMError should not be used, or is there a > separate spec issue? Should I file one? > https://github.com/w3c/webrtc-pc/issues/319 would be the right place.
On 2016/02/09 19:49:11, hta wrote: > On Tue, Feb 9, 2016 at 8:03 PM, philipj wrote: > > > Oh, right. In the "Intent to Remove: Support for optional error handler in > > RTCPeerConnection's createOffer() and createAnswer()" it was said that "We > > believe it is a good idea to let these changes in the legacy API stabilize > > during one release cycle, giving developers some time to update their > > applications." > > > > Is this still the plan, should landing this wait until after the next branch > > point? > > https://codereview.chromium.org/1661493002/ > > We decided that it was possible to ship these 3 functions as promises in > this version, and let the ones that are blocked on a deprecation > (createOffer and createAnswer) wait until the next version. > This will create some more diversity in the API for a while, but also means > that we'll not introduce too many changes at the same time. Oh, sorry, I conflated the two groups of methods.
On 2016/02/09 21:24:30, Guido Urdaneta wrote: > > Is there a WebIDL issue filed for adding any of the new names? I can't see any > > in https://github.com/heycam/webidl/issues, can you do that? Or poke someone? > > > > I would not know which new name to add there, since the WebRTC spec says TBD. I've filed https://github.com/w3c/webrtc-pc/issues/495 > > I think using DOMException with the best matching of the existing types would > > make sense. What that is depends on what usually causes InternalError. I could > > imagine NotSupportedError, InvalidStateError, SyntaxError, NetworkError, > > AbortError, TimeoutError, ConstraintError or perhaps OperationError. > > I'll do that in the meantime. If you like the types you pick, the spec should probably just do the same :) > > > > Ah, DOMError. Whatever the spec may say, we shouldn't be using DOMError > > > anywhere > > > > new, it's gone from most specs and jsbell@ and I have been trying to > > eliminate > > > > it. > > > > > > Should I use DOMException instead then? > > > > I would recommend that, yes. Is https://github.com/w3c/webrtc-pc/issues/319 > the > > right place to point out that DOMError should not be used, or is there a > > separate spec issue? Should I file one? > > > > https://github.com/w3c/webrtc-pc/issues/319 would be the right place. I found https://github.com/w3c/webrtc-pc/issues/335 and https://github.com/w3c/webrtc-pc/pull/468 so the DOMError problem is fixed.
PTAL. I think I addressed all comments, except the part about updating the tests to verify execution order. https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:89: const char kDefaultErrorName[] = "InternalError"; On 2016/02/09 18:59:17, philipj_UTC7 wrote: > On 2016/02/09 16:51:01, Guido Urdaneta wrote: > > On 2016/02/09 12:02:03, philipj_UTC7 wrote: > > > I see "Ask the DOM team to extend their list with the following errors" > issue > > in > > > the spec, can you make sure that an issue is filed on > > > http://heycam.github.io/webidl/#idl-DOMException-error-names ? > > > > > > It seems pretty likely to me that you'll be asked to use an existing generic > > > error name instead of InternalError, at least. > > > > > > For things that you expect to be added to DOMException, instead add them to > > > ExceptionCode and then use in the same way as InvalidStateError and friends. > > > > > > The spec also talks about DOMError, which shouldn't be used for anything > new, > > > but fortunately it looks like it isn't actually used, > > > MediaErrorState::raiseException instead throws a DOMExceptions. > > > > > > Are these error conditions new, or why don't they appear anywhere in the > > > existing code? > > > > The spec says to use DOMError with error name TBD. There is a bug filed about > > the TBD part. I used InternalError just because it is a value used elsewhere > in > > the spec. What value would you suggest? > > Is there a WebIDL issue filed for adding any of the new names? I can't see any > in https://github.com/heycam/webidl/issues, can you do that? Or poke someone? > > I think using DOMException with the best matching of the existing types would > make sense. What that is depends on what usually causes InternalError. I could > imagine NotSupportedError, InvalidStateError, SyntaxError, NetworkError, > AbortError, TimeoutError, ConstraintError or perhaps OperationError. Done. Using DOMException("AbortError") until the webrtc spec defines the error codes to use. https://codereview.chromium.org/1661493002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:693: resolver->reject(DOMException::create(NotSupportedError, "This method is not yet implemented.")); On 2016/02/09 18:59:17, philipj_UTC7 wrote: > On 2016/02/09 16:51:01, Guido Urdaneta wrote: > > On 2016/02/09 12:02:03, philipj_UTC7 wrote: > > > Can this actually happen? The whole source code is under your control, no? > :) > > > > Apparently, it can. > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m... > > > > I left NotSupportedError because that is the exception that was being thrown > in > > the legacy. Looking at the spec, we should return DOMError(TBD), or perhaps > > DOMError("InvalidCandidate"), as can be inferred from the spec bug. > > OK, it's not obvious from RTCPeerConnectionHandler::addICECandidate that > returning false means a missing implementation, there are other error cases in > WebRtcSession::ProcessIceMessage. > > But, this isn't new code. Maybe add a TODO to make it more truthful about the > cause? Done. https://codereview.chromium.org/1661493002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1661493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:118: Timer<ErrorCallbackRunner> m_timer; On 2016/02/09 18:59:17, philipj_UTC7 wrote: > To get the same timing as a promise rejection, what you need is a microtask, > which runs after this task has finished but before returning to the event loop. > > Scripts can tell the difference, if they do something that does "queue a task to > fire an event named ... at ..." right before making the call that creates this > ErrorCallbackRunner. The order of the event listener and the error callback will > be reversed. When updating the test for the legacy variants, you'll probably > want to sandwich the failure between two new Promise() rejections of your own, > and see that the callback order is the same. > > This may seem nitpicky, but I think it's what the spec is trying to say. Would > be very simply to implement as JS, but that's already been discussed on > blink-dev and doesn't look promising right now. Done. Layout test still needs fixing to verify timing. https://codereview.chromium.org/1661493002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:762: if (!implemented) { On 2016/02/09 18:59:17, philipj_UTC7 wrote: > Drop added { }? Done.
guidou@chromium.org changed reviewers: + haraken@chromium.org - philipj@opera.com
haraken@, can you take a look at this CL?
haraken@chromium.org changed reviewers: + yhirano@chromium.org
+yhirano: Would you take a look at the promise part? Also I want to have hta@ confirm this change since I'm not familiar with RTC's implementation.
I have followed closely, and believe it is good. 10. feb. 2016 14:46 skrev <haraken@chromium.org>: > > +yhirano: Would you take a look at the promise part? > > Also I want to have hta@ confirm this change since I'm not familiar with RTC's > implementation. > > https://codereview.chromium.org/1661493002/ > > -- 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.
I have followed closely, and believe it is good. 10. feb. 2016 14:46 skrev <haraken@chromium.org>: > > +yhirano: Would you take a look at the promise part? > > Also I want to have hta@ confirm this change since I'm not familiar with RTC's > implementation. > > https://codereview.chromium.org/1661493002/ > > -- 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.
https://codereview.chromium.org/1661493002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/MediaErrorState.cpp (right): https://codereview.chromium.org/1661493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaErrorState.cpp:111: break; Is this "break" needed? https://codereview.chromium.org/1661493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaErrorState.cpp:118: return String("Unsatisfiable constraint " + m_constraint); return "Unsatisfiable constant" + m_constraint; https://codereview.chromium.org/1661493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaErrorState.cpp:119: break; ditto https://codereview.chromium.org/1661493002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1661493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:37: #include "bindings/core/v8/ScriptPromiseResolver.h" +#include "bindings/core/v8/ScriptState.h" https://codereview.chromium.org/1661493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:525: ScriptPromise RTCPeerConnection::setLocalDescription(ExecutionContext* context, RTCSessionDescription* sessionDescription, VoidCallback* successCallback, RTCErrorCallback* errorCallback) How about implement this function using the above one? The implementation would be essentially: return setLocalDescription(scriptState, init).then(successCallback, errorCallback); https://codereview.chromium.org/1661493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:567: ScriptPromise RTCPeerConnection::setRemoteDescription(ExecutionContext* context, RTCSessionDescription* sessionDescription, VoidCallback* successCallback, RTCErrorCallback* errorCallback) ditto https://codereview.chromium.org/1661493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:707: ScriptPromise RTCPeerConnection::addIceCandidate(RTCIceCandidate* iceCandidate, VoidCallback* successCallback, RTCErrorCallback* errorCallback) ditto https://codereview.chromium.org/1661493002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h (right): https://codereview.chromium.org/1661493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h:58: class RTCStatsCallback; +class ScriptState;
https://codereview.chromium.org/1661493002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/MediaErrorState.cpp (right): https://codereview.chromium.org/1661493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaErrorState.cpp:111: break; On 2016/02/10 18:13:22, yhirano wrote: > Is this "break" needed? Done. https://codereview.chromium.org/1661493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaErrorState.cpp:118: return String("Unsatisfiable constraint " + m_constraint); On 2016/02/10 18:13:22, yhirano wrote: > return "Unsatisfiable constant" + m_constraint; Done. https://codereview.chromium.org/1661493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaErrorState.cpp:119: break; On 2016/02/10 18:13:22, yhirano wrote: > ditto Done. https://codereview.chromium.org/1661493002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1661493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:37: #include "bindings/core/v8/ScriptPromiseResolver.h" On 2016/02/10 18:13:22, yhirano wrote: > +#include "bindings/core/v8/ScriptState.h" Done. https://codereview.chromium.org/1661493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:525: ScriptPromise RTCPeerConnection::setLocalDescription(ExecutionContext* context, RTCSessionDescription* sessionDescription, VoidCallback* successCallback, RTCErrorCallback* errorCallback) On 2016/02/10 18:13:22, yhirano wrote: > How about implement this function using the above one? > > The implementation would be essentially: > > return setLocalDescription(scriptState, init).then(successCallback, > errorCallback); Actually, that's what the spec says. However, our legacy functions are from an older version of the spec and have some slight differences. For example, the new versions return DOMExceptions in case of error, but legacy error callbacks expect a string. Input can also be slightly different (RTCSessionDescription vs RTCSessionDescriptionInit). In addition, for legacy setLocalDescription() and setRemoteDescription() we have a bunch of counters we want to keep. Once the legacy versions become less used, our plan is to implement them in terms of the new versions. https://codereview.chromium.org/1661493002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h (right): https://codereview.chromium.org/1661493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h:58: class RTCStatsCallback; On 2016/02/10 18:13:23, yhirano wrote: > +class ScriptState; Done.
https://codereview.chromium.org/1661493002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1661493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:525: ScriptPromise RTCPeerConnection::setLocalDescription(ExecutionContext* context, RTCSessionDescription* sessionDescription, VoidCallback* successCallback, RTCErrorCallback* errorCallback) On 2016/02/10 20:21:28, Guido Urdaneta wrote: > On 2016/02/10 18:13:22, yhirano wrote: > > How about implement this function using the above one? > > > > The implementation would be essentially: > > > > return setLocalDescription(scriptState, init).then(successCallback, > > errorCallback); > > Actually, that's what the spec says. However, our legacy functions are from an > older version of the spec and have some slight differences. > For example, the new versions return DOMExceptions in case of error, but legacy > error callbacks expect a string. Input can also be slightly different > (RTCSessionDescription vs RTCSessionDescriptionInit). In addition, for legacy > setLocalDescription() and setRemoteDescription() we have a bunch of counters we > want to keep. > Once the legacy versions become less used, our plan is to implement them in > terms of the new versions. I see. But I don't like to return an empty ScriptPromise - Can you return a valid promise? I think returning ScriptPromise::cast(undefined) is OK here because the reason you returns a promise is that all or none of overloaded functions must return a promise in IDL. Adding a comment in the IDL for the intention would be good, too. Regarding the legacy interface, there are three functions, 1. Promise-based interface implemented by this CL 2. legacy new interface taking RTCSessionDescriptionInit 3. legacy old interface taking RTCSessionDescription right? I'm not sure what "implement them in terms of the new versions" means. Does that mean you will remove 3 and add 2?
https://codereview.chromium.org/1661493002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1661493002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:525: ScriptPromise RTCPeerConnection::setLocalDescription(ExecutionContext* context, RTCSessionDescription* sessionDescription, VoidCallback* successCallback, RTCErrorCallback* errorCallback) On 2016/02/10 22:29:19, yhirano wrote: > On 2016/02/10 20:21:28, Guido Urdaneta wrote: > > On 2016/02/10 18:13:22, yhirano wrote: > > > How about implement this function using the above one? > > > > > > The implementation would be essentially: > > > > > > return setLocalDescription(scriptState, init).then(successCallback, > > > errorCallback); > > > > Actually, that's what the spec says. However, our legacy functions are from an > > older version of the spec and have some slight differences. > > For example, the new versions return DOMExceptions in case of error, but > legacy > > error callbacks expect a string. Input can also be slightly different > > (RTCSessionDescription vs RTCSessionDescriptionInit). In addition, for legacy > > setLocalDescription() and setRemoteDescription() we have a bunch of counters > we > > want to keep. > > Once the legacy versions become less used, our plan is to implement them in > > terms of the new versions. > > I see. > But I don't like to return an empty ScriptPromise - Can you return a valid promise? Done. > I think returning ScriptPromise::cast(undefined) is OK here because the > reason you returns a promise is that all or none of overloaded functions must > return a promise in IDL. Adding a comment in the IDL for the intention would be > good, too. > Done > Regarding the legacy interface, there are three functions, > > 1. Promise-based interface implemented by this CL > 2. legacy new interface taking RTCSessionDescriptionInit > 3. legacy old interface taking RTCSessionDescription > > right? > > I'm not sure what "implement them in terms of the new versions" means. Does that > mean you will remove 3 and add 2? I meant that according to the spec there should be only 1 and 2, and 2 should be implemented as you proposed in your previous comment (return setLocalDescription(scriptState, init).then(successCallback, errorCallback);), and our goal is to eventually match the spec. For now, however, we plan to have 1 and 3. The strategy to remove 3 and add 2 has not been fully decided yet beyond the TODOs in the IDL file. However, but we have to keep supporting 3 until 1 is widely adopted. Note that there are 5 methods and each legacy version departs from the latest spec in different ways.
https://codereview.chromium.org/1661493002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1661493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:540: return ScriptPromise::cast(scriptState, ScriptValue()); ScriptPromise::cast(scriptState, v8::Undefined(scriptState->isolate()); https://codereview.chromium.org/1661493002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp (right): https://codereview.chromium.org/1661493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:35: bool shouldFireCallback = m_requester && m_requester->shouldFireDefaultCallbacks() && m_resolver->executionContext() && !m_resolver->executionContext()->activeDOMObjectsAreStopped(); When keepAliveWhilePending is called, the resolver is kept alive until resolved, rejected or ActiveDOMObjects are stopped. Why is it OK to not call resolve when shouldFireDefaultCallbacks()?
https://codereview.chromium.org/1661493002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1661493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:540: return ScriptPromise::cast(scriptState, ScriptValue()); On 2016/02/11 01:58:17, yhirano wrote: > ScriptPromise::cast(scriptState, v8::Undefined(scriptState->isolate()); Done. https://codereview.chromium.org/1661493002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp (right): https://codereview.chromium.org/1661493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:35: bool shouldFireCallback = m_requester && m_requester->shouldFireDefaultCallbacks() && m_resolver->executionContext() && !m_resolver->executionContext()->activeDOMObjectsAreStopped(); On 2016/02/11 01:58:17, yhirano wrote: > When keepAliveWhilePending is called, the resolver is kept alive until resolved, > rejected or ActiveDOMObjects are stopped. Why is it OK to not call resolve when > shouldFireDefaultCallbacks()? With regards to the ActiveDOMObjectsAreStopped() check, is it OK? is it unnecessary because I called keepAliveWhilePending()? should I instead not call keepAliveWhilePending()? With regards to shouldFireDefaultCallbacks(), that one returns false if the RTCPeerConnection is closed/stopped. According to the spec, we should neither resolve nor reject the promise in that case.
philipj@opera.com changed reviewers: + philipj@opera.com
A few more comments on the code, but I haven't reviewed the tests at all. hta@, have you carefully reviewed those against the spec? https://codereview.chromium.org/1661493002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1661493002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:68: #include "platform/Timer.h" platform/Timer.h no longer needed? https://codereview.chromium.org/1661493002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:93: const char kDefaultErrorName[] = "AbortError"; See comment about DOMException and ExceptionCode, neither this nor the below ought to be string constants I think. https://codereview.chromium.org/1661493002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:109: Microtask::enqueueMicrotask(WTF::bind(&RTCErrorCallback::handleEvent, errorCallback, errorMsg)); I think this probably will not keep the RTCErrorCallback alive and ought to be a crash. Can you write a test that triggers a GC right after this line is run and see if it crashes? Oilpan experts ought to have advice on best practices here, but I suspect it will involve Persistent<RTCErrorCallback>. https://codereview.chromium.org/1661493002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp (right): https://codereview.chromium.org/1661493002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:46: m_resolver->reject(DOMException::create(error, m_errorName)); This is invoking the constructor documented as "exposed to script" which isn't used internally elsewhere. Can you the variant that takes ExceptionCode instead?
https://codereview.chromium.org/1661493002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1661493002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:93: const char kDefaultErrorName[] = "AbortError"; On 2016/02/11 14:31:01, philipj_OOO_til_Feb15 wrote: > See comment about DOMException and ExceptionCode, neither this nor the below > ought to be string constants I think. Done. https://codereview.chromium.org/1661493002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:109: Microtask::enqueueMicrotask(WTF::bind(&RTCErrorCallback::handleEvent, errorCallback, errorMsg)); On 2016/02/11 14:31:01, philipj_OOO_til_Feb15 wrote: > I think this probably will not keep the RTCErrorCallback alive and ought to be a > crash. Can you write a test that triggers a GC right after this line is run and > see if it crashes? Oilpan experts ought to have advice on best practices here, > but I suspect it will involve Persistent<RTCErrorCallback>. I think you are right. Rewritten using a Task that contains a Persistent<RTCErrorCallback> https://codereview.chromium.org/1661493002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp (right): https://codereview.chromium.org/1661493002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:46: m_resolver->reject(DOMException::create(error, m_errorName)); On 2016/02/11 14:31:01, philipj_OOO_til_Feb15 wrote: > This is invoking the constructor documented as "exposed to script" which isn't > used internally elsewhere. Can you the variant that takes ExceptionCode instead? Done.
https://codereview.chromium.org/1661493002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp (right): https://codereview.chromium.org/1661493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:35: bool shouldFireCallback = m_requester && m_requester->shouldFireDefaultCallbacks() && m_resolver->executionContext() && !m_resolver->executionContext()->activeDOMObjectsAreStopped(); On 2016/02/11 10:47:08, Guido Urdaneta wrote: > On 2016/02/11 01:58:17, yhirano wrote: > > When keepAliveWhilePending is called, the resolver is kept alive until > resolved, > > rejected or ActiveDOMObjects are stopped. Why is it OK to not call resolve > when > > shouldFireDefaultCallbacks()? > > With regards to the ActiveDOMObjectsAreStopped() check, is it OK? is it > unnecessary because I called keepAliveWhilePending()? should I instead not call > keepAliveWhilePending()? > > With regards to shouldFireDefaultCallbacks(), that one returns false if the > RTCPeerConnection is closed/stopped. According to the spec, we should neither > resolve nor reject the promise in that case. I'd recommend calling resolve or reject unconditionally, because they are no-op when already resolved or rejected, or active dom objects are stopped. You don't need to clear the resolver pointer in clear() because all pointers held in the resolver will be cleared when resolved, rejected or stopped.
https://codereview.chromium.org/1661493002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp (right): https://codereview.chromium.org/1661493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:35: bool shouldFireCallback = m_requester && m_requester->shouldFireDefaultCallbacks() && m_resolver->executionContext() && !m_resolver->executionContext()->activeDOMObjectsAreStopped(); On 2016/02/11 18:28:31, yhirano wrote: > On 2016/02/11 10:47:08, Guido Urdaneta wrote: > > On 2016/02/11 01:58:17, yhirano wrote: > > > When keepAliveWhilePending is called, the resolver is kept alive until > > resolved, > > > rejected or ActiveDOMObjects are stopped. Why is it OK to not call resolve > > when > > > shouldFireDefaultCallbacks()? > > > > With regards to the ActiveDOMObjectsAreStopped() check, is it OK? is it > > unnecessary because I called keepAliveWhilePending()? should I instead not > call > > keepAliveWhilePending()? > > > > With regards to shouldFireDefaultCallbacks(), that one returns false if the > > RTCPeerConnection is closed/stopped. According to the spec, we should neither > > resolve nor reject the promise in that case. > > I'd recommend calling resolve or reject unconditionally, because they are no-op > when already resolved or rejected, or active dom objects are stopped. > You don't need to clear the resolver pointer in clear() because all pointers > held in the resolver will be cleared when resolved, rejected or stopped. Removed the unnecessary activeDOMObjects check. However, did not remove the other check because the spec says the promise should be left unresolved if the RTCPeerConnection is closed. https://codereview.chromium.org/1661493002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1661493002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:68: #include "platform/Timer.h" On 2016/02/11 14:31:01, philipj_OOO_til_Feb15 wrote: > platform/Timer.h no longer needed? Done.
https://codereview.chromium.org/1661493002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp (right): https://codereview.chromium.org/1661493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:35: bool shouldFireCallback = m_requester && m_requester->shouldFireDefaultCallbacks() && m_resolver->executionContext() && !m_resolver->executionContext()->activeDOMObjectsAreStopped(); On 2016/02/11 20:00:05, Guido Urdaneta wrote: > On 2016/02/11 18:28:31, yhirano wrote: > > On 2016/02/11 10:47:08, Guido Urdaneta wrote: > > > On 2016/02/11 01:58:17, yhirano wrote: > > > > When keepAliveWhilePending is called, the resolver is kept alive until > > > resolved, > > > > rejected or ActiveDOMObjects are stopped. Why is it OK to not call resolve > > > when > > > > shouldFireDefaultCallbacks()? > > > > > > With regards to the ActiveDOMObjectsAreStopped() check, is it OK? is it > > > unnecessary because I called keepAliveWhilePending()? should I instead not > > call > > > keepAliveWhilePending()? > > > > > > With regards to shouldFireDefaultCallbacks(), that one returns false if the > > > RTCPeerConnection is closed/stopped. According to the spec, we should > neither > > > resolve nor reject the promise in that case. > > > > I'd recommend calling resolve or reject unconditionally, because they are > no-op > > when already resolved or rejected, or active dom objects are stopped. > > You don't need to clear the resolver pointer in clear() because all pointers > > held in the resolver will be cleared when resolved, rejected or stopped. > > Removed the unnecessary activeDOMObjects check. > However, did not remove the other check because the spec says the promise should > be left unresolved if the RTCPeerConnection is closed. It's problematic because the resolver continues to be kept alive in such a case. Can you call m_resolver->stop() if shouldFireCallback is false? I would like some comments here and the below, for example "This is needed to have the resolver release its internal resources while leaving the associated promise pending as specified".
https://codereview.chromium.org/1661493002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp (right): https://codereview.chromium.org/1661493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:35: bool shouldFireCallback = m_requester && m_requester->shouldFireDefaultCallbacks() && m_resolver->executionContext() && !m_resolver->executionContext()->activeDOMObjectsAreStopped(); On 2016/02/11 21:56:30, yhirano wrote: > On 2016/02/11 20:00:05, Guido Urdaneta wrote: > > On 2016/02/11 18:28:31, yhirano wrote: > > > On 2016/02/11 10:47:08, Guido Urdaneta wrote: > > > > On 2016/02/11 01:58:17, yhirano wrote: > > > > > When keepAliveWhilePending is called, the resolver is kept alive until > > > > resolved, > > > > > rejected or ActiveDOMObjects are stopped. Why is it OK to not call > resolve > > > > when > > > > > shouldFireDefaultCallbacks()? > > > > > > > > With regards to the ActiveDOMObjectsAreStopped() check, is it OK? is it > > > > unnecessary because I called keepAliveWhilePending()? should I instead not > > > call > > > > keepAliveWhilePending()? > > > > > > > > With regards to shouldFireDefaultCallbacks(), that one returns false if > the > > > > RTCPeerConnection is closed/stopped. According to the spec, we should > > neither > > > > resolve nor reject the promise in that case. > > > > > > I'd recommend calling resolve or reject unconditionally, because they are > > no-op > > > when already resolved or rejected, or active dom objects are stopped. > > > You don't need to clear the resolver pointer in clear() because all pointers > > > held in the resolver will be cleared when resolved, rejected or stopped. > > > > Removed the unnecessary activeDOMObjects check. > > However, did not remove the other check because the spec says the promise > should > > be left unresolved if the RTCPeerConnection is closed. > > It's problematic because the resolver continues to be kept alive in such a case. > Can you call m_resolver->stop() if shouldFireCallback is false? I would like > some comments here and the below, for example "This is needed to have the > resolver release its internal resources while leaving the associated promise > pending as specified". Done.
https://codereview.chromium.org/1661493002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp (right): https://codereview.chromium.org/1661493002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:26: m_resolver->keepAliveWhilePending(); Sorry I didn't notice this before, but is this call needed? If the RTCVoidRequestPromiseImpl is properly kept alive, you don't need to call this function. https://codereview.chromium.org/1661493002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:39: // while leaving the associated promise pending as specified". You don't need the last double quotation.
https://codereview.chromium.org/1661493002/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp (right): https://codereview.chromium.org/1661493002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:26: m_resolver->keepAliveWhilePending(); On 2016/02/11 23:32:50, yhirano wrote: > Sorry I didn't notice this before, but is this call needed? If the > RTCVoidRequestPromiseImpl is properly kept alive, you don't need to call this > function. I wasn't completely sure it was needed. Removed now. https://codereview.chromium.org/1661493002/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:39: // while leaving the associated promise pending as specified". On 2016/02/11 23:32:50, yhirano wrote: > You don't need the last double quotation. Done.
lgtm
https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/MediaErrorState.cpp (right): https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaErrorState.cpp:117: return "Unsatisfiable constraint " + m_constraint; Shall we add: default: ASSERT_NOT_REACHED(); ? https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:91: const char kSignalingStateClosedMsg[] = "The RTCPeerConnection's signalingState is 'closed'."; Msg => Message (Blink prefers a fully qualified name.) https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:110: static PassOwnPtr<ErrorCallbackTask> create(RTCErrorCallback* errorCallback, const String& errorMsg) Msg => Message The same comment for other parts in this file. https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:125: , m_errorMsg(errorMsg) {} Shall we add ASSERT(m_errorCallback)? https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:551: ScriptPromise RTCPeerConnection::setLocalDescription(ScriptState* scriptState, RTCSessionDescription* sessionDescription, VoidCallback* successCallback, RTCErrorCallback* errorCallback) This method always returns an undefined ScriptPromise. Is it expected? Or is it going to be changed in the future? https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:564: return ScriptPromise::cast(scriptState, v8::Undefined(scriptState->isolate())); Can we just use ScriptPromise()? https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp (right): https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:16: return request; return new RTCVoidRequestPromiseImpl(...) https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:39: m_resolver->stop(); ActiveDOMObject::resume/suspend/stop should not be called by other than LifecycleNotifer. yhirano@: Would it be ok to make ScriptPromiseResolver::clear a public method and call clear here?
https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp (right): https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:39: m_resolver->stop(); On 2016/02/13 15:07:18, haraken wrote: > > ActiveDOMObject::resume/suspend/stop should not be called by other than > LifecycleNotifer. > > yhirano@: Would it be ok to make ScriptPromiseResolver::clear a public method > and call clear here? Yes. You need to stop the timer as well as in stop().
On 2016/02/13 16:13:37, yhirano wrote: > https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp > (right): > > https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:39: > m_resolver->stop(); > On 2016/02/13 15:07:18, haraken wrote: > > > > ActiveDOMObject::resume/suspend/stop should not be called by other than > > LifecycleNotifer. > > > > yhirano@: Would it be ok to make ScriptPromiseResolver::clear a public method > > and call clear here? > > Yes. You need to stop the timer as well as in stop(). https://codereview.chromium.org/1696553005/
https://codereview.chromium.org/1661493002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp (right): https://codereview.chromium.org/1661493002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:35: bool shouldFireCallback = m_requester && m_requester->shouldFireDefaultCallbacks() && m_resolver->executionContext() && !m_resolver->executionContext()->activeDOMObjectsAreStopped(); On 2016/02/11 20:00:05, Guido Urdaneta wrote: > On 2016/02/11 18:28:31, yhirano wrote: > > On 2016/02/11 10:47:08, Guido Urdaneta wrote: > > > On 2016/02/11 01:58:17, yhirano wrote: > > > > When keepAliveWhilePending is called, the resolver is kept alive until > > > resolved, > > > > rejected or ActiveDOMObjects are stopped. Why is it OK to not call resolve > > > when > > > > shouldFireDefaultCallbacks()? > > > > > > With regards to the ActiveDOMObjectsAreStopped() check, is it OK? is it > > > unnecessary because I called keepAliveWhilePending()? should I instead not > > call > > > keepAliveWhilePending()? > > > > > > With regards to shouldFireDefaultCallbacks(), that one returns false if the > > > RTCPeerConnection is closed/stopped. According to the spec, we should > neither > > > resolve nor reject the promise in that case. > > > > I'd recommend calling resolve or reject unconditionally, because they are > no-op > > when already resolved or rejected, or active dom objects are stopped. > > You don't need to clear the resolver pointer in clear() because all pointers > > held in the resolver will be cleared when resolved, rejected or stopped. > > Removed the unnecessary activeDOMObjects check. > However, did not remove the other check because the spec says the promise should > be left unresolved if the RTCPeerConnection is closed. It sounds weird that a promise is left without getting resolved or rejected. (I believe that the contract of Promise is that it must be resolved or rejected.) Maybe is it worth revisiting the spec?
It seems that nowhere is it written explicitly that a Promise must eventually be resolved or rejected, and Mozilla has stated that they don't ever resolve the dummy promise for callback-style calls. The latest WEBRTC discussion in a bug is here: https://github.com/w3c/webrtc-pc/issues/150 On Mon, Feb 15, 2016 at 2:31 AM, <haraken@chromium.org> wrote: > > https://codereview.chromium.org/1661493002/diff/140001/third_party/WebKit/Sou... > File > third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp > (right): > https://codereview.chromium.org/1661493002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:35 <https://codereview.chromium.org/1661493002/diff/140001/third_party/WebKit/Sou...>: > bool shouldFireCallback = m_requester && > m_requester->shouldFireDefaultCallbacks() && > m_resolver->executionContext() && > !m_resolver->executionContext()->activeDOMObjectsAreStopped(); > On 2016/02/11 20:00:05, Guido Urdaneta wrote: > > On 2016/02/11 18:28:31, yhirano wrote: > > > On 2016/02/11 10:47:08, Guido Urdaneta wrote: > > > > On 2016/02/11 01:58:17, yhirano wrote: > > > > > When keepAliveWhilePending is called, the resolver is kept alive > until > > > > resolved, > > > > > rejected or ActiveDOMObjects are stopped. Why is it OK to not > call resolve > > > > when > > > > > shouldFireDefaultCallbacks()? > > > > > > > > With regards to the ActiveDOMObjectsAreStopped() check, is it OK? > is it > > > > unnecessary because I called keepAliveWhilePending()? should I > instead not > > > call > > > > keepAliveWhilePending()? > > > > > > > > With regards to shouldFireDefaultCallbacks(), that one returns > false if the > > > > RTCPeerConnection is closed/stopped. According to the spec, we > should > > neither > > > > resolve nor reject the promise in that case. > > > > > > I'd recommend calling resolve or reject unconditionally, because > they are > > no-op > > > when already resolved or rejected, or active dom objects are > stopped. > > > You don't need to clear the resolver pointer in clear() because all > pointers > > > held in the resolver will be cleared when resolved, rejected or > stopped. > > > > Removed the unnecessary activeDOMObjects check. > > However, did not remove the other check because the spec says the > promise should > > be left unresolved if the RTCPeerConnection is closed. > > It sounds weird that a promise is left without getting resolved or > rejected. (I believe that the contract of Promise is that it must be > resolved or rejected.) Maybe is it worth revisiting the spec? > https://codereview.chromium.org/1661493002/ > > -- 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.
It seems that nowhere is it written explicitly that a Promise must eventually be resolved or rejected, and Mozilla has stated that they don't ever resolve the dummy promise for callback-style calls. The latest WEBRTC discussion in a bug is here: https://github.com/w3c/webrtc-pc/issues/150 On Mon, Feb 15, 2016 at 2:31 AM, <haraken@chromium.org> wrote: > > https://codereview.chromium.org/1661493002/diff/140001/third_party/WebKit/Sou... > File > third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp > (right): > https://codereview.chromium.org/1661493002/diff/140001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:35 <https://codereview.chromium.org/1661493002/diff/140001/third_party/WebKit/Sou...>: > bool shouldFireCallback = m_requester && > m_requester->shouldFireDefaultCallbacks() && > m_resolver->executionContext() && > !m_resolver->executionContext()->activeDOMObjectsAreStopped(); > On 2016/02/11 20:00:05, Guido Urdaneta wrote: > > On 2016/02/11 18:28:31, yhirano wrote: > > > On 2016/02/11 10:47:08, Guido Urdaneta wrote: > > > > On 2016/02/11 01:58:17, yhirano wrote: > > > > > When keepAliveWhilePending is called, the resolver is kept alive > until > > > > resolved, > > > > > rejected or ActiveDOMObjects are stopped. Why is it OK to not > call resolve > > > > when > > > > > shouldFireDefaultCallbacks()? > > > > > > > > With regards to the ActiveDOMObjectsAreStopped() check, is it OK? > is it > > > > unnecessary because I called keepAliveWhilePending()? should I > instead not > > > call > > > > keepAliveWhilePending()? > > > > > > > > With regards to shouldFireDefaultCallbacks(), that one returns > false if the > > > > RTCPeerConnection is closed/stopped. According to the spec, we > should > > neither > > > > resolve nor reject the promise in that case. > > > > > > I'd recommend calling resolve or reject unconditionally, because > they are > > no-op > > > when already resolved or rejected, or active dom objects are > stopped. > > > You don't need to clear the resolver pointer in clear() because all > pointers > > > held in the resolver will be cleared when resolved, rejected or > stopped. > > > > Removed the unnecessary activeDOMObjects check. > > However, did not remove the other check because the spec says the > promise should > > be left unresolved if the RTCPeerConnection is closed. > > It sounds weird that a promise is left without getting resolved or > rejected. (I believe that the contract of Promise is that it must be > resolved or rejected.) Maybe is it worth revisiting the spec? > https://codereview.chromium.org/1661493002/ > > -- 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.
https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:93: // TODO(guidou): Update exception codes once the error-handling aspects of the spec are finalized. crbug.com/585621 AFAICT the spec now uses only existing DOMException names: https://github.com/w3c/webrtc-pc/issues/495#issuecomment-184057583 https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:551: ScriptPromise RTCPeerConnection::setLocalDescription(ScriptState* scriptState, RTCSessionDescription* sessionDescription, VoidCallback* successCallback, RTCErrorCallback* errorCallback) On 2016/02/13 15:07:18, haraken wrote: > > This method always returns an undefined ScriptPromise. Is it expected? Or is it > going to be changed in the future? That is expected. The methods that take success/error callbacks return a dummy promises because they have the same name as the newer promise-vending versions. I can't find where WebIDL says that it must be so, but it does somewhere and the bindings generator checks for it.
I've taken a look at the test too now. There's "the part about updating the tests to verify execution order" left. I'll put off final review until all the promise bits are in order just in case that changes something I care about/understand. https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-ice-promise.html (right): https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-ice-promise.html:42: shouldBe(error.name, "TypeError") "error.name" so that the output makes more sense? Something like in RTCPeerConnection-localDescription.html https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-localDescription.html (right): https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-localDescription.html:47: errorReason = error; window.error = error is more obvious than a missing 'var' and allows you to use the same name. https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/MediaErrorState.cpp (right): https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaErrorState.cpp:109: case DOMError: Also s/DOMError/DOMException/ here? https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl (right): https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.idl:100: // These methods return or will be changed to return Promise<void> because Oh, here's the answer to haraken@'s question too.
https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-ice-promise.html (right): https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-ice-promise.html:42: shouldBe(error.name, "TypeError") On 2016/02/15 10:22:21, philipj_UTC7 wrote: > "error.name" so that the output makes more sense? Something like in > RTCPeerConnection-localDescription.html Done. https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-localDescription.html (right): https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-localDescription.html:47: errorReason = error; On 2016/02/15 10:22:21, philipj_UTC7 wrote: > window.error = error is more obvious than a missing 'var' and allows you to use > the same name. Done. https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/MediaErrorState.cpp (right): https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaErrorState.cpp:109: case DOMError: On 2016/02/15 10:22:21, philipj_UTC7 wrote: > Also s/DOMError/DOMException/ here? Done. https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/MediaErrorState.cpp:117: return "Unsatisfiable constraint " + m_constraint; On 2016/02/13 15:07:18, haraken wrote: > > Shall we add: > > default: > ASSERT_NOT_REACHED(); > > ? Done. https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:91: const char kSignalingStateClosedMsg[] = "The RTCPeerConnection's signalingState is 'closed'."; On 2016/02/13 15:07:18, haraken wrote: > > Msg => Message (Blink prefers a fully qualified name.) Done. https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:93: // TODO(guidou): Update exception codes once the error-handling aspects of the spec are finalized. crbug.com/585621 On 2016/02/15 10:00:44, philipj_UTC7 wrote: > AFAICT the spec now uses only existing DOMException names: > https://github.com/w3c/webrtc-pc/issues/495#issuecomment-184057583 Done. https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:110: static PassOwnPtr<ErrorCallbackTask> create(RTCErrorCallback* errorCallback, const String& errorMsg) On 2016/02/13 15:07:18, haraken wrote: > > Msg => Message > > The same comment for other parts in this file. Done. https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:125: , m_errorMsg(errorMsg) {} On 2016/02/13 15:07:18, haraken wrote: > > Shall we add ASSERT(m_errorCallback)? Done. https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:551: ScriptPromise RTCPeerConnection::setLocalDescription(ScriptState* scriptState, RTCSessionDescription* sessionDescription, VoidCallback* successCallback, RTCErrorCallback* errorCallback) On 2016/02/15 10:00:44, philipj_UTC7 wrote: > On 2016/02/13 15:07:18, haraken wrote: > > > > This method always returns an undefined ScriptPromise. Is it expected? Or is > it > > going to be changed in the future? > > That is expected. The methods that take success/error callbacks return a dummy > promises because they have the same name as the newer promise-vending versions. > I can't find where WebIDL says that it must be so, but it does somewhere and the > bindings generator checks for it. To further clarify, the reason all methods with the same name have to return promises is that if the method was called arguments that are invalid for all overloads, it would not be possible to decide whether to throw a TypeError or return a rejected Promise. https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:564: return ScriptPromise::cast(scriptState, v8::Undefined(scriptState->isolate())); On 2016/02/13 15:07:18, haraken wrote: > > Can we just use ScriptPromise()? Done. That is what the CL had originally. https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp (right): https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:16: return request; On 2016/02/13 15:07:18, haraken wrote: > > return new RTCVoidRequestPromiseImpl(...) Done. https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:39: m_resolver->stop(); On 2016/02/13 16:13:37, yhirano wrote: > On 2016/02/13 15:07:18, haraken wrote: > > > > ActiveDOMObject::resume/suspend/stop should not be called by other than > > LifecycleNotifer. > > > > yhirano@: Would it be ok to make ScriptPromiseResolver::clear a public method > > and call clear here? > > Yes. You need to stop the timer as well as in stop(). What should I do? Stay with stop() for now or wait until clear() is public?
On 2016/02/15 16:56:52, Guido Urdaneta wrote: > https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Lay... > File > third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-ice-promise.html > (right): > > https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-ice-promise.html:42: > shouldBe(error.name, "TypeError") > On 2016/02/15 10:22:21, philipj_UTC7 wrote: > > "error.name" so that the output makes more sense? Something like in > > RTCPeerConnection-localDescription.html > > Done. > > https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Lay... > File > third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-localDescription.html > (right): > > https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-localDescription.html:47: > errorReason = error; > On 2016/02/15 10:22:21, philipj_UTC7 wrote: > > window.error = error is more obvious than a missing 'var' and allows you to > use > > the same name. > > Done. > > https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/mediastream/MediaErrorState.cpp (right): > > https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/mediastream/MediaErrorState.cpp:109: case > DOMError: > On 2016/02/15 10:22:21, philipj_UTC7 wrote: > > Also s/DOMError/DOMException/ here? > > Done. > > https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/mediastream/MediaErrorState.cpp:117: return > "Unsatisfiable constraint " + m_constraint; > On 2016/02/13 15:07:18, haraken wrote: > > > > Shall we add: > > > > default: > > ASSERT_NOT_REACHED(); > > > > ? > > Done. > > https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp > (right): > > https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:91: const > char kSignalingStateClosedMsg[] = "The RTCPeerConnection's signalingState is > 'closed'."; > On 2016/02/13 15:07:18, haraken wrote: > > > > Msg => Message (Blink prefers a fully qualified name.) > > Done. > > https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:93: // > TODO(guidou): Update exception codes once the error-handling aspects of the spec > are finalized. crbug.com/585621 > On 2016/02/15 10:00:44, philipj_UTC7 wrote: > > AFAICT the spec now uses only existing DOMException names: > > https://github.com/w3c/webrtc-pc/issues/495#issuecomment-184057583 > > Done. > > https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:110: static > PassOwnPtr<ErrorCallbackTask> create(RTCErrorCallback* errorCallback, const > String& errorMsg) > On 2016/02/13 15:07:18, haraken wrote: > > > > Msg => Message > > > > The same comment for other parts in this file. > > Done. > > https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:125: , > m_errorMsg(errorMsg) {} > On 2016/02/13 15:07:18, haraken wrote: > > > > Shall we add ASSERT(m_errorCallback)? > > Done. > > https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:551: > ScriptPromise RTCPeerConnection::setLocalDescription(ScriptState* scriptState, > RTCSessionDescription* sessionDescription, VoidCallback* successCallback, > RTCErrorCallback* errorCallback) > On 2016/02/15 10:00:44, philipj_UTC7 wrote: > > On 2016/02/13 15:07:18, haraken wrote: > > > > > > This method always returns an undefined ScriptPromise. Is it expected? Or is > > it > > > going to be changed in the future? > > > > That is expected. The methods that take success/error callbacks return a dummy > > promises because they have the same name as the newer promise-vending > versions. > > I can't find where WebIDL says that it must be so, but it does somewhere and > the > > bindings generator checks for it. > > To further clarify, the reason all methods with the same name have to return > promises is that if the method was called arguments that are invalid for all > overloads, it would not be possible to decide whether to throw a TypeError or > return a rejected Promise. > > https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:564: return > ScriptPromise::cast(scriptState, v8::Undefined(scriptState->isolate())); > On 2016/02/13 15:07:18, haraken wrote: > > > > Can we just use ScriptPromise()? > > Done. That is what the CL had originally. > > https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... > File third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp > (right): > > https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:16: > return request; > On 2016/02/13 15:07:18, haraken wrote: > > > > return new RTCVoidRequestPromiseImpl(...) > > Done. > > https://codereview.chromium.org/1661493002/diff/260001/third_party/WebKit/Sou... > third_party/WebKit/Source/modules/mediastream/RTCVoidRequestPromiseImpl.cpp:39: > m_resolver->stop(); > On 2016/02/13 16:13:37, yhirano wrote: > > On 2016/02/13 15:07:18, haraken wrote: > > > > > > ActiveDOMObject::resume/suspend/stop should not be called by other than > > > LifecycleNotifer. > > > > > > yhirano@: Would it be ok to make ScriptPromiseResolver::clear a public > method > > > and call clear here? > > > > Yes. You need to stop the timer as well as in stop(). > > What should I do? Stay with stop() for now or wait until clear() is public? yhirano@ already has a CL for ScriptPromiseResolver::detach(): https://codereview.chromium.org/1696553005/ However, I want to make sure that the WebRTC spec really wants that behavior.
https://codereview.chromium.org/1661493002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1661493002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:93: const ExceptionCode kDefaultErrorName = OperationError; Why not simply remove kDefaultErrorName and kSessionDescriptionErrorName and use ExceptionCode values directly? That makes it more obvious that each place does what the spec says, without indirection.
Spec references for leaving promises dangling: http://w3c.github.io/webrtc-pc/#widl-RTCPeerConnection-close-void - the "close" method does not resolve or reject outstanding promises. http://w3c.github.io/webrtc-pc/#set-description (as an example) - the process to resolve the promise aborts if the state is "closed", without rejecting the promise. This has been discussed in the WG. I happen to dislike the resolution myself, but it's clear that this is what was decided to write.
philipj: added execution order tests for asynchronous execution of error callback. waiting for ScriptPromiseResolver::detach() to land. https://codereview.chromium.org/1661493002/diff/280001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1661493002/diff/280001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:93: const ExceptionCode kDefaultErrorName = OperationError; On 2016/02/16 05:44:08, philipj_UTC7 wrote: > Why not simply remove kDefaultErrorName and kSessionDescriptionErrorName and use > ExceptionCode values directly? That makes it more obvious that each place does > what the spec says, without indirection. Done.
The CQ bit was checked by guidou@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1661493002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1661493002/300001
I don't like returning undefined from a Promise returning function, per https://www.w3.org/2001/tag/doc/promises-guide#always-return-promises.
On 2016/02/16 05:44:22, hta - Chromium wrote: > Spec references for leaving promises dangling: > > http://w3c.github.io/webrtc-pc/#widl-RTCPeerConnection-close-void - the "close" > method does not resolve or reject outstanding promises. > > http://w3c.github.io/webrtc-pc/#set-description (as an example) - the process to > resolve the promise aborts if the state is "closed", without rejecting the > promise. > > This has been discussed in the WG. I happen to dislike the resolution myself, > but it's clear that this is what was decided to write. Thanks for the clarification! Then let's land ScriptPromiseResolver::detach and this CL.
LGTM
On 2016/02/16 18:54:01, haraken wrote: > LGTM (Please address yhirano's comments and wait for ScriptPromiseResolver::detach() before landing this CL.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/02/16 18:55:53, haraken wrote: > On 2016/02/16 18:54:01, haraken wrote: > > LGTM > > (Please address yhirano's comments and wait for ScriptPromiseResolver::detach() > before landing this CL.) I think I addressed all of yhirano's comments. The only one I reverted was returning ScriptPromise() instead of ScriptPromise(undefined), based on your suggestion. yhirano: is this still lgtm with you? Apart from detach(), I will also wait for philipj's comments on the tests.
On 2016/02/16 19:30:02, Guido Urdaneta wrote: > On 2016/02/16 18:55:53, haraken wrote: > > On 2016/02/16 18:54:01, haraken wrote: > > > LGTM > > > > (Please address yhirano's comments and wait for > ScriptPromiseResolver::detach() > > before landing this CL.) > > I think I addressed all of yhirano's comments. The only one I reverted was > returning ScriptPromise() instead of ScriptPromise(undefined), based on your > suggestion. > yhirano: is this still lgtm with you? > No, as said at https://codereview.chromium.org/1661493002/#msg68. > Apart from detach(), I will also wait for philipj's comments on the tests. I'm landing my CL.
CL updated to use ScriptPromiseResolver::detach()
https://codereview.chromium.org/1661493002/diff/320001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1661493002/diff/320001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:563: return ScriptPromise::cast(scriptState, v8::Undefined(scriptState->isolate())); Is this change observable? I thought it was the same thing, but if yhirano@ says this is the way to do it, so it it. A test for the difference would be nice, though. https://codereview.chromium.org/1661493002/diff/340001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-ice.html (right): https://codereview.chromium.org/1661493002/diff/340001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-ice.html:55: orderCounter = 0; add var or let here, or it ends up on window https://codereview.chromium.org/1661493002/diff/340001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-ice.html:66: shouldBe('error', '"The RTCPeerConnection\'s signalingState is \'closed\'."'); This is testing the right thing, the timing of asyncCallErrorCallback, but it doesn't look like it would fail with the previous Timer implementation. What I mean is that the call that internally reaches asyncCallErrorCallback and enqueues the microtask needs to be sandwiched in between two other things that enqueue microtasks. Here's an example using another API: http://software.hixie.ch/utilities/js/live-dom-viewer/saved/3894 That case rejects the promise because of missing arguments, but the timing of the error callback should be the same unless I've misunderstood the spec. So your test can probably be something like: ... localPeerConnection.close(); Promise.resolve().then(_ => orderCounter++); localPeerConnection.addIceCandidate(/* check orderCounter in error callback */) Promise.resolve().then(_ => orderCounter++); Or slightly better, maintain an array of events and verify when the last promise is resolved that the order is what you expect. Should be easier to debug when it goes wrong.
LGTM assuming the test changes are made, but if it's unclear what I mean or if I'm just wrong I'm happy to take another look.
https://codereview.chromium.org/1661493002/diff/340001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-ice.html (right): https://codereview.chromium.org/1661493002/diff/340001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-ice.html:55: orderCounter = 0; On 2016/02/18 02:31:56, philipj_UTC7 wrote: > add var or let here, or it ends up on window If I use var, I cannot check its value with shouldBe() functions. https://codereview.chromium.org/1661493002/diff/340001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-ice.html:66: shouldBe('error', '"The RTCPeerConnection\'s signalingState is \'closed\'."'); On 2016/02/18 02:31:56, philipj_UTC7 wrote: > This is testing the right thing, the timing of asyncCallErrorCallback, but it > doesn't look like it would fail with the previous Timer implementation. It does fail with the Timer implementation. > What I > mean is that the call that internally reaches asyncCallErrorCallback and > enqueues the microtask needs to be sandwiched in between two other things that > enqueue microtasks. Here's an example using another API: > http://software.hixie.ch/utilities/js/live-dom-viewer/saved/3894 > > That case rejects the promise because of missing arguments, but the timing of > the error callback should be the same unless I've misunderstood the spec. > > So your test can probably be something like: > > ... > localPeerConnection.close(); > Promise.resolve().then(_ => orderCounter++); > localPeerConnection.addIceCandidate(/* check orderCounter in error callback */) > Promise.resolve().then(_ => orderCounter++); > This tests the same thing, but yours is much more elegant :) > Or slightly better, maintain an array of events and verify when the last promise > is resolved that the order is what you expect. Should be easier to debug when it > goes wrong. Done.
The CQ bit was checked by guidou@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yhirano@chromium.org, haraken@chromium.org, philipj@opera.com Link to the patchset: https://codereview.chromium.org/1661493002/#ps360001 (title: "philipj's comments on tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1661493002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1661493002/360001
Message was sent while issue was closed.
Description was changed from ========== Add promise-based addIceCandidate, setLocalDescription and setRemoteDescription to RTCPeerConnection Intent to Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/zVlp8jIb2XU BUG=499126 ========== to ========== Add promise-based addIceCandidate, setLocalDescription and setRemoteDescription to RTCPeerConnection Intent to Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/zVlp8jIb2XU BUG=499126 ==========
Message was sent while issue was closed.
Committed patchset #19 (id:360001)
Message was sent while issue was closed.
Description was changed from ========== Add promise-based addIceCandidate, setLocalDescription and setRemoteDescription to RTCPeerConnection Intent to Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/zVlp8jIb2XU BUG=499126 ========== to ========== Add promise-based addIceCandidate, setLocalDescription and setRemoteDescription to RTCPeerConnection Intent to Ship: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/zVlp8jIb2XU BUG=499126 Committed: https://crrev.com/e16c96f5a5b517a64fdbc5be75f05e359e04b461 Cr-Commit-Position: refs/heads/master@{#376163} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/e16c96f5a5b517a64fdbc5be75f05e359e04b461 Cr-Commit-Position: refs/heads/master@{#376163}
Message was sent while issue was closed.
https://codereview.chromium.org/1661493002/diff/360001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-ice.html (right): https://codereview.chromium.org/1661493002/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-ice.html:57: events = []; could set window.events for clarity, like window.error, in some future CL poking around this. https://codereview.chromium.org/1661493002/diff/360001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/fast/mediastream/RTCPeerConnection-ice.html:67: shouldBe('events', '[1,2,3]'); Looks good! |
