[Presentation API] Allow PresentationConnection state change to "Connecting"

[Presentation API] Allow PresentationConnection state change to "Connecting"

As per the Presentation API spec [1], when we reconnect to a closed
PresentationConnection, its status changes from Closed to Connecting. This CL
removes a NOTREACHED() called at state change to Connecting.

This CL also replaces the use of the var keyword with const/let in
third_party/WebKit/LayoutTests/presentation/.

[1] https://w3c.github.io/presentation-api/#reconnecting-to-a-presentation

BUG=677540
Review-Url: https://codereview.chromium.org/2643473002
Cr-Commit-Position: refs/heads/master@{#446458}
Committed: https://chromium.googlesource.com/chromium/src/+/a8f908ca6ab0abe86af75c688099d8c9dee1685c

[takumif, 2017-01-17 20:41:58]

Patchset #1 (id:1) has been deleted

[takumif, 2017-01-17 20:42:03]

Patchset #1 (id:20001) has been deleted

[takumif, 2017-01-17 20:42:08]

Patchset #1 (id:40001) has been deleted

[takumif, 2017-01-17 20:46:32]

Description was changed from

==========
[Presentation API] Allow PresentationConnection state change to "connecting"

BUG=
==========

to

==========
[Presentation API] Allow PresentationConnection state change to "connecting"

As per the Presentation API spec [1], when we reconnect to a closed
PresentationConnection, its status changes from Closed to Connecting. This CL
removes a NOTREACHED() called at state change to Connecting.

[1] https://w3c.github.io/presentation-api/#reconnecting-to-a-presentation

BUG=677540
==========

[takumif, 2017-01-17 20:46:56]

Description was changed from

==========
[Presentation API] Allow PresentationConnection state change to "connecting"

As per the Presentation API spec [1], when we reconnect to a closed
PresentationConnection, its status changes from Closed to Connecting. This CL
removes a NOTREACHED() called at state change to Connecting.

[1] https://w3c.github.io/presentation-api/#reconnecting-to-a-presentation

BUG=677540
==========

to

==========
[Presentation API] Allow PresentationConnection state change to "Connecting"

As per the Presentation API spec [1], when we reconnect to a closed
PresentationConnection, its status changes from Closed to Connecting. This CL
removes a NOTREACHED() called at state change to Connecting.

[1] https://w3c.github.io/presentation-api/#reconnecting-to-a-presentation

BUG=677540
==========

[takumif, 2017-01-17 20:51:28]

takumif@chromium.org changed reviewers:
+ mfoltz@chromium.org

[takumif, 2017-01-17 20:51:29]

mfoltz@: please take a look.
zhaobin@: FYI, as this is related to a bug (677540) you've been working on.

Thanks!

[mark a. foltz, 2017-01-17 21:06:13]

On 2017/01/17 at 20:51:29, takumif wrote:
> mfoltz@: please take a look.
> zhaobin@: FYI, as this is related to a bug (677540) you've been working on.
> 
> Thanks!

It should be possible to write or update a layout test to verify that the
observed state is connecting, e.g.

https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/presentat...

Can you do that?

[takumif, 2017-01-18 19:29:08]

On 2017/01/17 21:06:13, mark a. foltz wrote:
> On 2017/01/17 at 20:51:29, takumif wrote:
> > mfoltz@: please take a look.
> > zhaobin@: FYI, as this is related to a bug (677540) you've been working on.
> > 
> > Thanks!
> 
> It should be possible to write or update a layout test to verify that the
> observed state is connecting, e.g.
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/presentat...
> 
> Can you do that?

Added!

[zhaobin, 2017-01-18 19:46:52]

zhaobin@chromium.org changed reviewers:
+ zhaobin@chromium.org

[zhaobin, 2017-01-18 19:46:52]

Could you please update existing presentation-reconnect.html instead of creating
a new file presentation-close-reconnect.html or just delete
presentation-reconnect.html? They seems testing the same thing :)

https://codereview.chromium.org/2643473002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/presentation/presentation-close-reconnect.html
(right):

https://codereview.chromium.org/2643473002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentation-close-reconnect.html:47:
connection.close();
assert_true(connection.state === 'closed'); ?

[takumif, 2017-01-18 19:56:14]

Patchset #3 (id:100001) has been deleted

[takumif, 2017-01-18 19:57:27]

https://codereview.chromium.org/2643473002/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/presentation/presentation-close-reconnect.html
(right):

https://codereview.chromium.org/2643473002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/presentation/presentation-close-reconnect.html:47:
connection.close();
On 2017/01/18 19:46:52, zhaobin wrote:
> assert_true(connection.state === 'closed'); ?

Added the check to the onclose callback, as the state change happens
asynchronously.

[mark a. foltz, 2017-01-18 20:30:17]

https://codereview.chromium.org/2643473002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/presentation/presentation-close-reconnect.html
(right):

https://codereview.chromium.org/2643473002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/presentation/presentation-close-reconnect.html:43:
assert_true(connection.state === 'connecting' ||
Shouldn't state always be 'connecting' in the resolver for reconnect()?

https://codereview.chromium.org/2643473002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/presentation/presentation-close-reconnect.html:52:
}, "Test that Presentation.reconnect() resolves with a closed presentation
connection and updates its state.");
s/closed/connecting/

[takumif, 2017-01-18 20:53:22]

https://codereview.chromium.org/2643473002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/presentation/presentation-close-reconnect.html
(right):

https://codereview.chromium.org/2643473002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/presentation/presentation-close-reconnect.html:43:
assert_true(connection.state === 'connecting' ||
On 2017/01/18 20:30:17, mark a. foltz wrote:
> Shouldn't state always be 'connecting' in the resolver for reconnect()?

I wasn't sure because there was a browser test [1] that seemed to be expecting
'connecting' or 'connected' in its resolver, but according to the spec it should
always be 'connecting.' I'll go change that browser test as well.

[1]
https://cs.chromium.org/chromium/src/chrome/test/media_router/resources/commo...

https://codereview.chromium.org/2643473002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/presentation/presentation-close-reconnect.html:52:
}, "Test that Presentation.reconnect() resolves with a closed presentation
connection and updates its state.");
On 2017/01/18 20:30:17, mark a. foltz wrote:
> s/closed/connecting/

I wanted to say that the connection given is the one that was closed. Changed
the wording.

[mark a. foltz, 2017-01-20 18:48:23]

LGTM with comments addressed.  Will the browser test be updated in your other
patch?

https://codereview.chromium.org/2643473002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/presentation/presentation-close-reconnect.html
(right):

https://codereview.chromium.org/2643473002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/presentation/presentation-close-reconnect.html:11:
function waitForClick(callback) {
waitForClick() is already defined in presentation-service-mock.js.

https://codereview.chromium.org/2643473002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/presentation/presentation-close-reconnect.html:29:
var connection = null;
I would prefer to use const/let in these tests, but check other tests and remain
consistent.  We can update them all at once later.

https://codereview.chromium.org/2643473002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/presentation/resources/presentation-service-mock.js
(right):

https://codereview.chromium.org/2643473002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/presentation/resources/presentation-service-mock.js:13:
let [ presentation, bindings ] = mojo.modules;
nit: Can you leave this as presentationService, so it's not ambiguous with
navigator.presentation?

[zhaobin, 2017-01-20 19:27:50]

lgtm, please cc avayvod@ if you are going to change browser test.

https://codereview.chromium.org/2643473002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/presentation/presentation-close-reconnect.html
(right):

https://codereview.chromium.org/2643473002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/presentation/presentation-close-reconnect.html:43:
assert_true(connection.state === 'connecting' ||
On 2017/01/18 20:53:22, takumif wrote:
> On 2017/01/18 20:30:17, mark a. foltz wrote:
> > Shouldn't state always be 'connecting' in the resolver for reconnect()?
> 
> I wasn't sure because there was a browser test [1] that seemed to be expecting
> 'connecting' or 'connected' in its resolver, but according to the spec it
should
> always be 'connecting.' I'll go change that browser test as well.
> 
> [1]
>
https://cs.chromium.org/chromium/src/chrome/test/media_router/resources/commo...

Adding 'connected' state to fix crbug.com/664629

We should be able to remove the check after
https://codereview.chromium.org/2547143002. The test fails flankily though, it
might be better to do that in a seperate patch and cc avayvod@.

[takumif, 2017-01-23 19:52:52]

Patchset #5 (id:160001) has been deleted

[takumif, 2017-01-23 19:55:28]

Description was changed from

==========
[Presentation API] Allow PresentationConnection state change to "Connecting"

As per the Presentation API spec [1], when we reconnect to a closed
PresentationConnection, its status changes from Closed to Connecting. This CL
removes a NOTREACHED() called at state change to Connecting.

[1] https://w3c.github.io/presentation-api/#reconnecting-to-a-presentation

BUG=677540
==========

to

==========
[Presentation API] Allow PresentationConnection state change to "Connecting"

As per the Presentation API spec [1], when we reconnect to a closed
PresentationConnection, its status changes from Closed to Connecting. This CL
removes a NOTREACHED() called at state change to Connecting.

This CL also replaces the use of the var keyword with const/let in
third_party/WebKit/LayoutTests/presentation/.

[1] https://w3c.github.io/presentation-api/#reconnecting-to-a-presentation

BUG=677540
==========

[takumif, 2017-01-23 19:58:13]

Thanks for reviewing. Regarding the browser test that currently accepts both
'connecting' and 'connected' states in the resolver of
PresentationRequest.start(), I'll change it in a patch of its own if it's not
flaky anymore.

https://codereview.chromium.org/2643473002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/presentation/presentation-close-reconnect.html
(right):

https://codereview.chromium.org/2643473002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/presentation/presentation-close-reconnect.html:43:
assert_true(connection.state === 'connecting' ||
On 2017/01/20 19:27:50, zhaobin wrote:
> On 2017/01/18 20:53:22, takumif wrote:
> > On 2017/01/18 20:30:17, mark a. foltz wrote:
> > > Shouldn't state always be 'connecting' in the resolver for reconnect()?
> > 
> > I wasn't sure because there was a browser test [1] that seemed to be
expecting
> > 'connecting' or 'connected' in its resolver, but according to the spec it
> should
> > always be 'connecting.' I'll go change that browser test as well.
> > 
> > [1]
> >
>
https://cs.chromium.org/chromium/src/chrome/test/media_router/resources/commo...
> 
> Adding 'connected' state to fix crbug.com/664629
> 
> We should be able to remove the check after
> https://codereview.chromium.org/2547143002. The test fails flankily though, it
> might be better to do that in a seperate patch and cc avayvod@.

Is the browser test still flaky after https://codereview.chromium.org/2547143002
? If not, I'll go change the browser test in another patch.

https://codereview.chromium.org/2643473002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/presentation/presentation-close-reconnect.html
(right):

https://codereview.chromium.org/2643473002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/presentation/presentation-close-reconnect.html:11:
function waitForClick(callback) {
On 2017/01/20 18:48:22, mark a. foltz wrote:
> waitForClick() is already defined in presentation-service-mock.js.

Removed.

https://codereview.chromium.org/2643473002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/presentation/presentation-close-reconnect.html:29:
var connection = null;
On 2017/01/20 18:48:22, mark a. foltz wrote:
> I would prefer to use const/let in these tests, but check other tests and
remain
> consistent.  We can update them all at once later.

Changed all the files in this directory to use const/let.

https://codereview.chromium.org/2643473002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/presentation/resources/presentation-service-mock.js
(right):

https://codereview.chromium.org/2643473002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/presentation/resources/presentation-service-mock.js:13:
let [ presentation, bindings ] = mojo.modules;
On 2017/01/20 18:48:22, mark a. foltz wrote:
> nit: Can you leave this as presentationService, so it's not ambiguous with
> navigator.presentation?

Done.

[takumif, 2017-01-23 19:58:22]

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

[commit-bot: I haz the power, 2017-01-23 19:58:49]

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, 2017-01-23 20:09:58]

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

[commit-bot: I haz the power, 2017-01-23 20:10:00]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[takumif, 2017-01-23 20:21:25]

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

[commit-bot: I haz the power, 2017-01-23 20:21:54]

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, 2017-01-23 20:26:48]

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

[commit-bot: I haz the power, 2017-01-23 20:26:49]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[takumif, 2017-01-24 00:51:47]

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

[commit-bot: I haz the power, 2017-01-24 00:52:49]

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, 2017-01-24 01:36:55]

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

[commit-bot: I haz the power, 2017-01-24 01:36:56]

Dry run: This issue passed the CQ dry run.

[takumif, 2017-01-26 18:16:51]

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

[commit-bot: I haz the power, 2017-01-26 18:17:23]

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

[takumif, 2017-01-26 18:27:23]

I touched many files replacing var with const/let, so it'd be great if you could
take another look. Thanks.

[mark a. foltz, 2017-01-26 19:20:49]

lgtm

[commit-bot: I haz the power, 2017-01-26 20:43:20]

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

[commit-bot: I haz the power, 2017-01-26 20:43:21]

Dry run: This issue passed the CQ dry run.

[takumif, 2017-01-26 21:34:13]

The CQ bit was checked by takumif@chromium.org

[takumif, 2017-01-26 21:34:13]

The patchset sent to the CQ was uploaded after l-g-t-m from zhaobin@chromium.org
Link to the patchset: https://codereview.chromium.org/2643473002/#ps180001
(title: "Address Mark and Bin's comments")

[commit-bot: I haz the power, 2017-01-26 21:35:04]

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, 2017-01-26 21:47:53]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1485466453390820,
"parent_rev": "27df167d082e2049382618e582f773b973c6e305", "commit_rev":
"a8f908ca6ab0abe86af75c688099d8c9dee1685c"}

[commit-bot: I haz the power, 2017-01-26 21:48:29]

Description was changed from

==========
[Presentation API] Allow PresentationConnection state change to "Connecting"

As per the Presentation API spec [1], when we reconnect to a closed
PresentationConnection, its status changes from Closed to Connecting. This CL
removes a NOTREACHED() called at state change to Connecting.

This CL also replaces the use of the var keyword with const/let in
third_party/WebKit/LayoutTests/presentation/.

[1] https://w3c.github.io/presentation-api/#reconnecting-to-a-presentation

BUG=677540
==========

to

==========
[Presentation API] Allow PresentationConnection state change to "Connecting"

As per the Presentation API spec [1], when we reconnect to a closed
PresentationConnection, its status changes from Closed to Connecting. This CL
removes a NOTREACHED() called at state change to Connecting.

This CL also replaces the use of the var keyword with const/let in
third_party/WebKit/LayoutTests/presentation/.

[1] https://w3c.github.io/presentation-api/#reconnecting-to-a-presentation

BUG=677540

Review-Url: https://codereview.chromium.org/2643473002
Cr-Commit-Position: refs/heads/master@{#446458}
Committed:
https://chromium.googlesource.com/chromium/src/+/a8f908ca6ab0abe86af75c688099...
==========

[commit-bot: I haz the power, 2017-01-26 21:48:30]

Committed patchset #5 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/a8f908ca6ab0abe86af75c688099...

[suzyh_UTC10 (ex-contributor), 2017-01-27 03:39:59]

A revert of this CL (patchset #5 id:180001) has been created in
https://codereview.chromium.org/2655073007/ by suzyh@chromium.org.

The reason for reverting is: presentation/presentation-close-reconnect.html is
timing out consistently on e.g. WebKit Linux Trusty
https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Trusty....