[Presentation API] change initial state of PresentationConnection to 'connecting'

[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}

[zhaobin, 2016-11-02 01:51:28]

zhaobin@chromium.org changed reviewers:
+ imcheng@chromium.org, mfoltz@chromium.org

[imcheng, 2016-11-02 21:04:55]

Can we add some tests for this change? I was thinking integration tests but feel
free to add others you deem appropriate.

https://codereview.chromium.org/2470023002/diff/1/content/browser/presentatio...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/2470023002/diff/1/content/browser/presentatio...
content/browser/presentation/presentation_service_impl.cc:568:
ListenForConnectionStateChange(connection);
I believe we also need to add the OnConnectionStateChanged call here.

It's not great that we have logic to return a connection to
PresentationDispatcher in 3 different places... for now, refactoring
ListenForConnectionStateChange + OnConnectionStateChanged into a common method
might help readability.

[mark a. foltz, 2016-11-03 18:15:03]

Looks good once Derek's comments were addressed.

Wanted to make sure the following sequence of events happens when a presentation
is started, as this is the current intention of the spec (awaiting resolution of
https://github.com/w3c/presentation-api/issues/360).

1. Site calls PresentationRequest.start()
2. User selects screen from the dialog
3. Promise from start() resolves with connection.state == 'connecting' (in one
event loop)
4. connectionavailable fires on the request with connection.state ==
'connecting' (in one event loop)
5. 'connect' fires on connection (and connection.state changes to 'connected')
(in one event loop)

Is there a web platform test that checks this?

[zhaobin, 2016-11-03 18:24:32]

@Derek, trying to figure out how to write an integration test for this.

@Mark, we do have one web test verifying onconnect event is fired. It does not
cover the complete sequence you mentioned though (lacking 3, 4).
http://w3c-test.org/presentation-api/controlling-ua/PresentationConnection_on...

https://codereview.chromium.org/2470023002/diff/1/content/browser/presentatio...
File content/browser/presentation/presentation_service_impl.cc (right):

https://codereview.chromium.org/2470023002/diff/1/content/browser/presentatio...
content/browser/presentation/presentation_service_impl.cc:568:
ListenForConnectionStateChange(connection);
On 2016/11/02 21:04:55, imcheng wrote:
> I believe we also need to add the OnConnectionStateChanged call here.
> 
> It's not great that we have logic to return a connection to
> PresentationDispatcher in 3 different places... for now, refactoring
> ListenForConnectionStateChange + OnConnectionStateChanged into a common method
> might help readability.

Done.

[mark a. foltz, 2016-11-03 19:36:56]

On 2016/11/03 at 18:24:32, zhaobin wrote:
> @Derek, trying to figure out how to write an integration test for this.

Here's what I think Derek had in mind.  The "basic" integration test covers
starting a presentation.  It can run against a test media route provider or the
Cast MRP.

https://cs.chromium.org/chromium/src/content/public/test/browser_test_base.h?...

You'll want to add an assertion that the connection is in the connecting /
connected states.

https://cs.chromium.org/chromium/src/chrome/test/media_router/resources/commo...
 
It looks like test coverage is weak for presentationConnection.state in our
integration tests so any additional fixes are welcome :)

> @Mark, we do have one web test verifying onconnect event is fired. It does not
cover the complete sequence you mentioned though (lacking 3, 4).
http://w3c-test.org/presentation-api/controlling-ua/PresentationConnection_on...

It looks like there are tests for different steps in isolation but not a
specific sequence.  I'll file an issue in GitHub to cover this.

> 
>
https://codereview.chromium.org/2470023002/diff/1/content/browser/presentatio...
> File content/browser/presentation/presentation_service_impl.cc (right):
> 
>
https://codereview.chromium.org/2470023002/diff/1/content/browser/presentatio...
> content/browser/presentation/presentation_service_impl.cc:568:
ListenForConnectionStateChange(connection);
> On 2016/11/02 21:04:55, imcheng wrote:
> > I believe we also need to add the OnConnectionStateChanged call here.
> > 
> > It's not great that we have logic to return a connection to
> > PresentationDispatcher in 3 different places... for now, refactoring
> > ListenForConnectionStateChange + OnConnectionStateChanged into a common
method
> > might help readability.
> 
> Done.

[mark a. foltz, 2016-11-03 19:37:49]

First link should be:

https://cs.chromium.org/chromium/src/chrome/test/media_router/media_router_in...

[mark a. foltz, 2016-11-03 19:44:49]

On 2016/11/03 at 19:37:49, mark a. foltz wrote:
> First link should be:
> 
>
https://cs.chromium.org/chromium/src/chrome/test/media_router/media_router_in...

Filed https://github.com/w3c/web-platform-tests/issues/4168

[mark a. foltz, 2016-11-03 21:40:14]

lgtm

[zhaobin, 2016-11-04 19:08:41]

Patchset #3 (id:40001) has been deleted

[zhaobin, 2016-11-04 19:16:26]

zhaobin@chromium.org changed reviewers:
+ cliffordcheng@chromium.org, leilei@chromium.org

[zhaobin, 2016-11-04 19:16:27]

Modify our integraton test to check for 'connecting' state and onconnect event.

[imcheng, 2016-11-09 00:38:25]

lgtm % 1 nit

https://codereview.chromium.org/2470023002/diff/60001/chrome/test/media_route...
File chrome/test/media_router/resources/common.js (right):

https://codereview.chromium.org/2470023002/diff/60001/chrome/test/media_route...
chrome/test/media_router/resources/common.js:86: sendResult(false, 'Failed to
start session: encounter exception ' + e);
s/encounter/encountered

[zhaobin, 2016-11-09 02:45:02]

The CQ bit was checked by zhaobin@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-09 02:45:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 04:48:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-09 04:48:10]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2016-11-09 18:20:00]

The CQ bit was checked by zhaobin@chromium.org

[zhaobin, 2016-11-09 18:20:01]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org,
imcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2470023002/#ps80001
(title: "resolve code review comments from Derek and rebase with master")

[commit-bot: I haz the power, 2016-11-09 18:20:26]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-09 18:26:18]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-11-09 18:46:58]

Description was changed from

==========
[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
==========

to

==========
[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

Committed: https://crrev.com/badd595e3a5263107e93fea0e1bd943186291153
Cr-Commit-Position: refs/heads/master@{#430983}
==========

[commit-bot: I haz the power, 2016-11-09 18:46:59]

Patchset 4 (id:??) landed as
https://crrev.com/badd595e3a5263107e93fea0e1bd943186291153
Cr-Commit-Position: refs/heads/master@{#430983}

[zhaobin, 2016-11-10 18:23:56]

Description was changed from

==========
[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

Committed: https://crrev.com/badd595e3a5263107e93fea0e1bd943186291153
Cr-Commit-Position: refs/heads/master@{#430983}
==========

to

==========
[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}
==========