Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2654003002:
[MR tests] Assert initial state / state change for reconnect case. (Closed)
Description[MR tests] Assert initial state / state change for reconnect case.
Followup to https://codereview.chromium.org/2653683006/.
Refactor logic that asserts initial state of a PresentationConnection
into a helper method and also use it in the reconnect success case.
BUG=684664
Review-Url: https://codereview.chromium.org/2654003002
Cr-Commit-Position: refs/heads/master@{#447184}
Committed: https://chromium.googlesource.com/chromium/src/+/54e7828cdcb38fda68c7a5bbc7ea1d7896f96588
Patch Set 1 #Patch Set 2 : #
Total comments: 3
Patch Set 3 : rebase #Messages
Total messages: 19 (12 generated)
The CQ bit was checked by imcheng@chromium.org to run a CQ dry run
Description was changed from ========== [MR tests] Assert initial state / state change for reconnect case. Refactor logic that asserts initial state of a PresentationConnection into a helper method and also use it in the reconnect success case. BUG=684664 ========== to ========== [MR tests] Assert initial state / state change for reconnect case. Followup to https://codereview.chromium.org/2653683006/. Refactor logic that asserts initial state of a PresentationConnection into a helper method and also use it in the reconnect success case. BUG=684664 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
imcheng@chromium.org changed reviewers: + mfoltz@chromium.org, takumif@chromium.org
Takumi / Mark: PTAL. This may conflict with Takumi's integration test patch. I can wait for it to land first.
On 2017/01/24 22:48:08, imcheng wrote: > Takumi / Mark: PTAL. This may conflict with Takumi's integration test patch. I > can wait for it to land first. LGTM. You can land this first, since I need to change the behavior of the TestProvider before I land my integration tests, as discussed. FWIW, I ran MANUAL_Fail_ReconnectSession with this patch 30 times on Mac and it didn't fail, so the onconnect bug doesn't seem to happen for that test.
https://codereview.chromium.org/2654003002/diff/20001/chrome/test/media_route... File chrome/test/media_router/resources/common.js (right): https://codereview.chromium.org/2654003002/diff/20001/chrome/test/media_route... chrome/test/media_router/resources/common.js:91: if (connection.state == 'connected') { When would this be 'connected'? The state must be 'connecting' in the start() Promise resolver. https://w3c.github.io/presentation-api/#starting-a-presentation-connection
https://codereview.chromium.org/2654003002/diff/20001/chrome/test/media_route... File chrome/test/media_router/resources/common.js (right): https://codereview.chromium.org/2654003002/diff/20001/chrome/test/media_route... chrome/test/media_router/resources/common.js:91: if (connection.state == 'connected') { On 2017/01/26 19:32:23, mark a. foltz wrote: > When would this be 'connected'? The state must be 'connecting' in the start() > Promise resolver. > > https://w3c.github.io/presentation-api/#starting-a-presentation-connection In checkSession() we're passing in a resolver function to |startSessionPromise| which had previously been returned. So wouldn't it be possible for the connection state to have changed to 'connected' before we pass in the resolver? This is the browser test I was referring to in another CL [1] regarding this issue, but now I think it's possible for the state to be 'connected' here, unless I'm misunderstanding how promises work. [1] https://codereview.chromium.org/2643473002/
lgtm https://codereview.chromium.org/2654003002/diff/20001/chrome/test/media_route... File chrome/test/media_router/resources/common.js (right): https://codereview.chromium.org/2654003002/diff/20001/chrome/test/media_route... chrome/test/media_router/resources/common.js:91: if (connection.state == 'connected') { On 2017/01/26 at 21:28:29, takumif wrote: > On 2017/01/26 19:32:23, mark a. foltz wrote: > > When would this be 'connected'? The state must be 'connecting' in the start() > > Promise resolver. > > > > https://w3c.github.io/presentation-api/#starting-a-presentation-connection > > In checkSession() we're passing in a resolver function to |startSessionPromise| which had previously been returned. So wouldn't it be possible for the connection state to have changed to 'connected' before we pass in the resolver? > > This is the browser test I was referring to in another CL [1] regarding this issue, but now I think it's possible for the state to be 'connected' here, unless I'm misunderstanding how promises work. > > [1] https://codereview.chromium.org/2643473002/ Yes, if the Promise is already settled the connection could be in any state.
The CQ bit was checked by imcheng@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by imcheng@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, takumif@chromium.org Link to the patchset: https://codereview.chromium.org/2654003002/#ps40001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485834836327090,
"parent_rev": "b0dbfec9aa34c8f33e547f04cb8a7d4079e6b122", "commit_rev":
"54e7828cdcb38fda68c7a5bbc7ea1d7896f96588"}
Message was sent while issue was closed.
Description was changed from ========== [MR tests] Assert initial state / state change for reconnect case. Followup to https://codereview.chromium.org/2653683006/. Refactor logic that asserts initial state of a PresentationConnection into a helper method and also use it in the reconnect success case. BUG=684664 ========== to ========== [MR tests] Assert initial state / state change for reconnect case. Followup to https://codereview.chromium.org/2653683006/. Refactor logic that asserts initial state of a PresentationConnection into a helper method and also use it in the reconnect success case. BUG=684664 Review-Url: https://codereview.chromium.org/2654003002 Cr-Commit-Position: refs/heads/master@{#447184} Committed: https://chromium.googlesource.com/chromium/src/+/54e7828cdcb38fda68c7a5bbc7ea... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/54e7828cdcb38fda68c7a5bbc7ea... |
