Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1523213002:
Make sure peer connection states are updated properly (Closed)
DescriptionMake sure that peer connection states are updated properly.
Update the state right before the respective javascript events are fired.
This fixed the problem that when two state changes happen in quick succession, the js client may see two events both with the same last-changed state.
BUG=371804
Committed: https://crrev.com/d869cbd0afc9c62114e4ce9e16c0b67e08dcfc68
Cr-Commit-Position: refs/heads/master@{#365666}
Patch Set 1 : #Patch Set 2 : #
Total comments: 4
Patch Set 3 : Added comments #
Total comments: 1
Messages
Total messages: 29 (14 generated)
Description was changed from ========== Make sure update state calls to be synchronous BUG= ========== to ========== Make sure update state calls to be synchronous BUG=371804 ==========
Description was changed from ========== Make sure update state calls to be synchronous BUG=371804 ========== to ========== Make sure that peer connection states are updated properly. They are updated right before the respective javascript events are fired. BUG=371804 ==========
Description was changed from ========== Make sure that peer connection states are updated properly. They are updated right before the respective javascript events are fired. BUG=371804 ========== to ========== Make sure that peer connection states are updated properly. Update the state right before the respective javascript events are fired. BUG=371804 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
honghaiz@chromium.org changed reviewers: + honghaiz@chromium.org, pthatcher@chromium.org, tommi@chromium.org
Description was changed from ========== Make sure that peer connection states are updated properly. Update the state right before the respective javascript events are fired. BUG=371804 ========== to ========== Make sure that peer connection states are updated properly. Update the state right before the respective javascript events are fired. This fixed the problem that when two state changes happen in quick succession, the js client may see two events both with the same last-changed state. BUG=371804 ==========
lgtm
Fixing the unittest errors. Basically I reverted the changes on the signalingstate because signaling states are often changed by the js and js need to access the changed state right away (instead of waiting just before the event is fired).
This looks really good. But is there a way to write a unit test for it? https://codereview.chromium.org/1523213002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1523213002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:970: } Can you fix the signaling state as well? I don't mind if it's in a second CL, but I think we should fix it too. https://codereview.chromium.org/1523213002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h (right): https://codereview.chromium.org/1523213002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h:184: bool setIceConnectionState(WebRTCPeerConnectionHandlerClient::ICEConnectionState); Can you leave a comment saying something like "setX changes the state immediately and doesn't fire an event. changeX changes the state asynchronously and does fire an event immediately after changing the state"?.
I do not think there is an easy way to add unittest for this. This dir does not have any unittest file and none of the previous CL in the same dir has tests. https://codereview.chromium.org/1523213002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp (right): https://codereview.chromium.org/1523213002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.cpp:970: } On 2015/12/15 23:59:20, pthatcher2 wrote: > Can you fix the signaling state as well? I don't mind if it's in a second CL, > but I think we should fix it too. I tried to do the same thing for the signaling (in my first patch) but it created a bunch of test failures. The reason is that the signaling state was changed by the js code, and the state change should not be delayed (Many tests and perhaps some apps will want the state is changed right away once the js call is issued. So I think we'd better keep that as is. https://codereview.chromium.org/1523213002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h (right): https://codereview.chromium.org/1523213002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h:184: bool setIceConnectionState(WebRTCPeerConnectionHandlerClient::ICEConnectionState); On 2015/12/15 23:59:20, pthatcher2 wrote: > Can you leave a comment saying something like "setX changes the state > immediately and doesn't fire an event. changeX changes the state asynchronously > and does fire an event immediately after changing the state"?. Done.
The CQ bit was checked by honghaiz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@chromium.org Link to the patchset: https://codereview.chromium.org/1523213002/#ps80001 (title: "Added comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1523213002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1523213002/80001
The CQ bit was unchecked by honghaiz@chromium.org
The CQ bit was checked by honghaiz@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1523213002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1523213002/80001
Message was sent while issue was closed.
Description was changed from ========== Make sure that peer connection states are updated properly. Update the state right before the respective javascript events are fired. This fixed the problem that when two state changes happen in quick succession, the js client may see two events both with the same last-changed state. BUG=371804 ========== to ========== Make sure that peer connection states are updated properly. Update the state right before the respective javascript events are fired. This fixed the problem that when two state changes happen in quick succession, the js client may see two events both with the same last-changed state. BUG=371804 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Make sure that peer connection states are updated properly. Update the state right before the respective javascript events are fired. This fixed the problem that when two state changes happen in quick succession, the js client may see two events both with the same last-changed state. BUG=371804 ========== to ========== Make sure that peer connection states are updated properly. Update the state right before the respective javascript events are fired. This fixed the problem that when two state changes happen in quick succession, the js client may see two events both with the same last-changed state. BUG=371804 Committed: https://crrev.com/d869cbd0afc9c62114e4ce9e16c0b67e08dcfc68 Cr-Commit-Position: refs/heads/master@{#365666} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/d869cbd0afc9c62114e4ce9e16c0b67e08dcfc68 Cr-Commit-Position: refs/heads/master@{#365666}
Message was sent while issue was closed.
haraken@chromium.org changed reviewers: + haraken@chromium.org
Message was sent while issue was closed.
This CL broke oilpan's build. Uploaded a fix: https://codereview.chromium.org/1532463005/ https://codereview.chromium.org/1523213002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h (right): https://codereview.chromium.org/1523213002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h:169: PassOwnPtrWillBeRawPtr<BoolFunction> m_setupFunction; This is wrong even on non-oilpan... This must be an OwnPtr.
Message was sent while issue was closed.
haraken@, Thanks for the fixing! On Wed, Dec 16, 2015 at 7:03 PM, <haraken@chromium.org> wrote: > This CL broke oilpan's build. > > Uploaded a fix: https://codereview.chromium.org/1532463005/ > > > > > https://codereview.chromium.org/1523213002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h > (right): > > > https://codereview.chromium.org/1523213002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h:169: > PassOwnPtrWillBeRawPtr<BoolFunction> m_setupFunction; > > This is wrong even on non-oilpan... This must be an OwnPtr. > > https://codereview.chromium.org/1523213002/ > -- 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.
Message was sent while issue was closed.
haraken@, Thanks for the fixing! On Wed, Dec 16, 2015 at 7:03 PM, <haraken@chromium.org> wrote: > This CL broke oilpan's build. > > Uploaded a fix: https://codereview.chromium.org/1532463005/ > > > > > https://codereview.chromium.org/1523213002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h > (right): > > > https://codereview.chromium.org/1523213002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h:169: > PassOwnPtrWillBeRawPtr<BoolFunction> m_setupFunction; > > This is wrong even on non-oilpan... This must be an OwnPtr. > > https://codereview.chromium.org/1523213002/ > -- 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.
Message was sent while issue was closed.
For those of us who don't work in that area of the code every day, is there a good guide somewhere on the differences between the different pointer types and when to use them or not use them (PassOwnPtrWillBeRawPtr vs. PassOwnPtr vs. OwnPtr; WillBeHeapVector vs. HeapVector<Member>)? On Thu, Dec 17, 2015 at 9:24 AM, Honghai Zhang <honghaiz@google.com> wrote: > haraken@, > > Thanks for the fixing! > > On Wed, Dec 16, 2015 at 7:03 PM, <haraken@chromium.org> wrote: > >> This CL broke oilpan's build. >> >> Uploaded a fix: https://codereview.chromium.org/1532463005/ >> >> >> >> >> https://codereview.chromium.org/1523213002/diff/80001/third_party/WebKit/Sour... >> File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h >> (right): >> >> >> https://codereview.chromium.org/1523213002/diff/80001/third_party/WebKit/Sour... >> third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h:169: >> PassOwnPtrWillBeRawPtr<BoolFunction> m_setupFunction; >> >> This is wrong even on non-oilpan... This must be an OwnPtr. >> >> https://codereview.chromium.org/1523213002/ >> > > -- 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.
Message was sent while issue was closed.
For those of us who don't work in that area of the code every day, is there a good guide somewhere on the differences between the different pointer types and when to use them or not use them (PassOwnPtrWillBeRawPtr vs. PassOwnPtr vs. OwnPtr; WillBeHeapVector vs. HeapVector<Member>)? On Thu, Dec 17, 2015 at 9:24 AM, Honghai Zhang <honghaiz@google.com> wrote: > haraken@, > > Thanks for the fixing! > > On Wed, Dec 16, 2015 at 7:03 PM, <haraken@chromium.org> wrote: > >> This CL broke oilpan's build. >> >> Uploaded a fix: https://codereview.chromium.org/1532463005/ >> >> >> >> >> https://codereview.chromium.org/1523213002/diff/80001/third_party/WebKit/Sour... >> File third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h >> (right): >> >> >> https://codereview.chromium.org/1523213002/diff/80001/third_party/WebKit/Sour... >> third_party/WebKit/Source/modules/mediastream/RTCPeerConnection.h:169: >> PassOwnPtrWillBeRawPtr<BoolFunction> m_setupFunction; >> >> This is wrong even on non-oilpan... This must be an OwnPtr. >> >> https://codereview.chromium.org/1523213002/ >> > > -- 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.
Message was sent while issue was closed.
On 2015/12/17 19:38:11, chromium-reviews wrote: > For those of us who don't work in that area of the code every day, is there > a good guide somewhere on the differences between the different pointer > types and when to use them or not use them (PassOwnPtrWillBeRawPtr > vs. PassOwnPtr vs. OwnPtr; WillBeHeapVector vs. HeapVector<Member>)? This is a tutorial: https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
