[Presentation API] Add layout test for connection.close() and fix test failures

[Presentation API] Add layout test for connection.close() and fix test failures

Add layout test.

1-UA connection.close() hits NOTREACHED() in PresentationConnection::didChangeState(). Use PresentationConnection::didClose() instead.

BUG=697719
Review-Url: https://codereview.chromium.org/2730123003
Cr-Commit-Position: refs/heads/master@{#455225}
Committed: https://chromium.googlesource.com/chromium/src/+/0c08ed56a3e5089b3cc4094e83daae196a6300c4

[zhaobin, 2017-03-03 21:40:12]

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

[zhaobin, 2017-03-03 21:41:12]

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

[commit-bot: I haz the power, 2017-03-03 21:41:32]

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-03-03 23:10:52]

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

[commit-bot: I haz the power, 2017-03-03 23:10:53]

Dry run: This issue passed the CQ dry run.

[imcheng, 2017-03-04 00:58:28]

lgtm

https://codereview.chromium.org/2730123003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/presentation/presentation-controller-close-connection.html
(right):

https://codereview.chromium.org/2730123003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/presentation/presentation-controller-close-connection.html:23:
assert_equals(connection.state, "closed");
Is there a specific reason / message we should be asserting here and in the
other test cases?

[mark a. foltz, 2017-03-06 18:57:55]

lgtm

Thank you for adding thorough layout tests.  Our layout test coverage is really
improving!

https://codereview.chromium.org/2730123003/diff/1/content/renderer/presentati...
File content/renderer/presentation/presentation_connection_proxy.cc (right):

https://codereview.chromium.org/2730123003/diff/1/content/renderer/presentati...
content/renderer/presentation/presentation_connection_proxy.cc:81:
target_connection_ptr_->DidChangeState(
Part of the connection refactoring should better align the Web* and the mojo
APIs for PresentationConnection.   For example, this could be
target_connection_ptr_->DidClose().

Not asking for changes here- I'm starting a design doc with some initial
thoughts and will share it post-perf :)

https://codereview.chromium.org/2730123003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/presentation/presentation-controller-close-connection.html
(right):

https://codereview.chromium.org/2730123003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/presentation/presentation-controller-close-connection.html:19:
new PresentationRequest("http://example.com/").start().then(
Nit: Use https: to reflect upcoming deprecation on insecure contexts.

https://codereview.chromium.org/2730123003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/presentation/presentation-controller-close-connection.html:23:
assert_equals(connection.state, "closed");
On 2017/03/04 at 00:58:28, imcheng wrote:
> Is there a specific reason / message we should be asserting here and in the
other test cases?

The message is only required for the "error" close event reason - I don't think
it would add much to the other close reasons, we can add one later if there is
something I'm missing.

This test could assert an empty close message just to define the behavior.

Here's the spec language covering the close event message.
https://w3c.github.io/presentation-api/#sending-a-message-through-presentatio...

https://codereview.chromium.org/2730123003/diff/1/third_party/WebKit/public/p...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h
(right):

https://codereview.chromium.org/2730123003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h:36:
virtual void didClose(WebPresentationConnectionCloseReason,
Can you remove the message argument since it's always empty?

My thought is to have an API with a did() method for each of the close reasons,
especially since there is no message for the "Closed" or "WentAway" reasons.

e.g.

didError(message) => Error
didClose() => Closed 
didGoAway() => WentAway

(Not asking for this change here though.)

[zhaobin, 2017-03-06 19:52:04]

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

[zhaobin, 2017-03-06 19:52:23]

https://codereview.chromium.org/2730123003/diff/1/content/renderer/presentati...
File content/renderer/presentation/presentation_connection_proxy.cc (right):

https://codereview.chromium.org/2730123003/diff/1/content/renderer/presentati...
content/renderer/presentation/presentation_connection_proxy.cc:81:
target_connection_ptr_->DidChangeState(
On 2017/03/06 18:57:55, mark a. foltz wrote:
> Part of the connection refactoring should better align the Web* and the mojo
> APIs for PresentationConnection.   For example, this could be
> target_connection_ptr_->DidClose().
> 
> Not asking for changes here- I'm starting a design doc with some initial
> thoughts and will share it post-perf :)

Acknowledged.

https://codereview.chromium.org/2730123003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/presentation/presentation-controller-close-connection.html
(right):

https://codereview.chromium.org/2730123003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/presentation/presentation-controller-close-connection.html:19:
new PresentationRequest("http://example.com/").start().then(
On 2017/03/06 18:57:55, mark a. foltz wrote:
> Nit: Use https: to reflect upcoming deprecation on insecure contexts.

Done.

https://codereview.chromium.org/2730123003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/presentation/presentation-controller-close-connection.html:23:
assert_equals(connection.state, "closed");
On 2017/03/06 18:57:55, mark a. foltz wrote:
> On 2017/03/04 at 00:58:28, imcheng wrote:
> > Is there a specific reason / message we should be asserting here and in the
> other test cases?
> 
> The message is only required for the "error" close event reason - I don't
think
> it would add much to the other close reasons, we can add one later if there is
> something I'm missing.
> 
> This test could assert an empty close message just to define the behavior.
> 
> Here's the spec language covering the close event message.
>
https://w3c.github.io/presentation-api/#sending-a-message-through-presentatio...

Done.

https://codereview.chromium.org/2730123003/diff/1/third_party/WebKit/public/p...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h
(right):

https://codereview.chromium.org/2730123003/diff/1/third_party/WebKit/public/p...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h:36:
virtual void didClose(WebPresentationConnectionCloseReason,
On 2017/03/06 18:57:55, mark a. foltz wrote:
> Can you remove the message argument since it's always empty?
> 
> My thought is to have an API with a did() method for each of the close
reasons,
> especially since there is no message for the "Closed" or "WentAway" reasons.
> 
> e.g.
> 
> didError(message) => Error
> didClose() => Closed 
> didGoAway() => WentAway
> 
> (Not asking for this change here though.)

Done.

[commit-bot: I haz the power, 2017-03-06 19:52:58]

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-03-06 21:29:02]

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

[commit-bot: I haz the power, 2017-03-06 21:29:03]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2017-03-06 22:45:13]

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

[commit-bot: I haz the power, 2017-03-06 22:46:10]

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-03-07 00:46:49]

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

[commit-bot: I haz the power, 2017-03-07 00:46:50]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[zhaobin, 2017-03-07 18:24:25]

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

[commit-bot: I haz the power, 2017-03-07 18:25:15]

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-03-07 19:48:25]

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

[commit-bot: I haz the power, 2017-03-07 19:48:26]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2017-03-07 20:02:49]

The CQ bit was checked by zhaobin@chromium.org

[zhaobin, 2017-03-07 20:02:49]

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/2730123003/#ps40001
(title: "remove unnecessary forward declarations")

[commit-bot: I haz the power, 2017-03-07 20:05:08]

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-03-07 21:19:23]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488916969459760,
"parent_rev": "52efd28a984d8b4f5afdd9a3a6be1654499db1a3", "commit_rev":
"0c08ed56a3e5089b3cc4094e83daae196a6300c4"}

[commit-bot: I haz the power, 2017-03-07 21:20:05]

Description was changed from

==========
[Presentation API] Add layout test for connection.close() and fix test failures

Add layout test.

1-UA connection.close() hits NOTREACHED() in
PresentationConnection::didChangeState(). Use PresentationConnection::didClose()
instead.

BUG=697719
==========

to

==========
[Presentation API] Add layout test for connection.close() and fix test failures

Add layout test.

1-UA connection.close() hits NOTREACHED() in
PresentationConnection::didChangeState(). Use PresentationConnection::didClose()
instead.

BUG=697719

Review-Url: https://codereview.chromium.org/2730123003
Cr-Commit-Position: refs/heads/master@{#455225}
Committed:
https://chromium.googlesource.com/chromium/src/+/0c08ed56a3e5089b3cc4094e83da...
==========

[commit-bot: I haz the power, 2017-03-07 21:20:06]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/0c08ed56a3e5089b3cc4094e83da...