Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(30)

Issue 2470023002: [Presentation API] change initial state of PresentationConnection to 'connecting' (Closed)

Created:
4 years, 1 month ago by zhaobin
Modified:
4 years, 1 month ago
CC:
blink-reviews, chromium-reviews, darin-cc_chromium.org, haraken, jam
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

[Presentation API] change initial state of PresentationConnection to 'connecting' - change initial state to 'connecting' - transition to 'connected' state in PresentationServiceImpl (browser side) as a work around - verified Youtube and CastHelloVideo works fine for Cast; Youtube works fine for DIAL BUG=659423, 574232 Committed: https://crrev.com/badd595e3a5263107e93fea0e1bd943186291153 Cr-Commit-Position: refs/heads/master@{#430983}

Patch Set 1 #

Total comments: 2

Patch Set 2 : resolve code review comments from Derek #

Patch Set 3 : add integration test to check connection state #

Total comments: 1

Patch Set 4 : resolve code review comments from Derek and rebase with master #

Unified diffs Side-by-side diffs Delta from patch set Stats (+38 lines, -19 lines) Patch
M chrome/test/media_router/resources/common.js View 1 2 3 1 chunk +11 lines, -4 lines 0 comments Download
M content/browser/presentation/presentation_service_impl.h View 1 2 chunks +4 lines, -3 lines 0 comments Download
M content/browser/presentation/presentation_service_impl.cc View 1 5 chunks +10 lines, -4 lines 0 comments Download
M content/browser/presentation/presentation_service_impl_unittest.cc View 1 2 chunks +12 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp View 1 chunk +1 line, -2 lines 0 comments Download

Messages

Total messages: 24 (11 generated)
zhaobin
4 years, 1 month ago (2016-11-02 01:51:28 UTC) #2
imcheng
Can we add some tests for this change? I was thinking integration tests but feel ...
4 years, 1 month ago (2016-11-02 21:04:55 UTC) #3
mark a. foltz
Looks good once Derek's comments were addressed. Wanted to make sure the following sequence of ...
4 years, 1 month ago (2016-11-03 18:15:03 UTC) #4
zhaobin
@Derek, trying to figure out how to write an integration test for this. @Mark, we ...
4 years, 1 month ago (2016-11-03 18:24:32 UTC) #5
mark a. foltz
On 2016/11/03 at 18:24:32, zhaobin wrote: > @Derek, trying to figure out how to write ...
4 years, 1 month ago (2016-11-03 19:36:56 UTC) #6
mark a. foltz
First link should be: https://cs.chromium.org/chromium/src/chrome/test/media_router/media_router_integration_browsertest.cc?rcl=0&l=408
4 years, 1 month ago (2016-11-03 19:37:49 UTC) #7
mark a. foltz
On 2016/11/03 at 19:37:49, mark a. foltz wrote: > First link should be: > > ...
4 years, 1 month ago (2016-11-03 19:44:49 UTC) #8
mark a. foltz
lgtm
4 years, 1 month ago (2016-11-03 21:40:14 UTC) #9
zhaobin
Modify our integraton test to check for 'connecting' state and onconnect event.
4 years, 1 month ago (2016-11-04 19:16:27 UTC) #12
imcheng
lgtm % 1 nit https://codereview.chromium.org/2470023002/diff/60001/chrome/test/media_router/resources/common.js File chrome/test/media_router/resources/common.js (right): https://codereview.chromium.org/2470023002/diff/60001/chrome/test/media_router/resources/common.js#newcode86 chrome/test/media_router/resources/common.js:86: sendResult(false, 'Failed to start session: ...
4 years, 1 month ago (2016-11-09 00:38:25 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2470023002/80001
4 years, 1 month ago (2016-11-09 18:20:26 UTC) #20
commit-bot: I haz the power
Committed patchset #4 (id:80001)
4 years, 1 month ago (2016-11-09 18:26:18 UTC) #21
commit-bot: I haz the power
4 years, 1 month ago (2016-11-09 18:46:59 UTC) #23
Message was sent while issue was closed.
Patchset 4 (id:??) landed as
https://crrev.com/badd595e3a5263107e93fea0e1bd943186291153
Cr-Commit-Position: refs/heads/master@{#430983}

Powered by Google App Engine
This is Rietveld 408576698