[Presentation API] Use connection proxy to change connection state to 'closed'

[Presentation API] Use connection proxy to change connection state to 'closed'

blink connection.close()
  connection_proxy.close()
    ---> mojo call
      target_connection_proxy.OnClose() // change both sides' blink connection to 'closed'

For 1-UA, it works for both controller and receiver connection.
For 2-UA, browser_connection_proxy has no-op OnClose(), and state change is still handled by MRP.

Also change message_request_queue_ to std::deque and erase pending send message requests from message queue when closing connection.

BUG=688233
Review-Url: https://codereview.chromium.org/2714693002
Cr-Commit-Position: refs/heads/master@{#454391}
Committed: https://chromium.googlesource.com/chromium/src/+/fb7c75d163e22589351f06fbef424f94d6e216f8

[zhaobin, 2017-02-23 04:12:36]

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

[mark a. foltz, 2017-03-01 06:14:03]

LGTM

Nice patch :-)

It would be good to add a layout test for controller <--> receiver (a separate
patch is fine).

https://codereview.chromium.org/2714693002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/browser_presentation_connection_proxy.h
(right):

https://codereview.chromium.org/2714693002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/browser_presentation_connection_proxy.h:65: //
Underlying media route is always connected. Media route class does not
While it's true the route is always connected (as long as it exists) there
should be a way to close an individual connection to the route... This should
get addressed with larger refactoring of connections / routes.  No change
requested here.

[imcheng, 2017-03-02 00:03:30]

lgtm. It would be good to have followups for:

- Replacing the shared queue in PresentationDispatcher with per-connection queue
- Plumb close() through PresentationConnection for 2-UA

[zhaobin, 2017-03-02 03:33:41]

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

[zhaobin, 2017-03-02 03:33:42]

+dcheng@ for mojo related change:

third_party/WebKit/public/platform/modules/presentation/presentation.mojom

Following bugs track comments from imcheng@ and mfoltz@:

crbug.com/684116 [Presentation API] Move message queue logic from
PresentationDispatcher to PresentationConnectionProxy
crbug.com/588874 Ensure legal PresentationConnection state transitions
crbug.com/697719 [Presentation API] Add layout test for controller
connection.close() and receiver connection.close()

https://codereview.chromium.org/2714693002/diff/1/chrome/browser/media/router...
File chrome/browser/media/router/browser_presentation_connection_proxy.h
(right):

https://codereview.chromium.org/2714693002/diff/1/chrome/browser/media/router...
chrome/browser/media/router/browser_presentation_connection_proxy.h:65: //
Underlying media route is always connected. Media route class does not
On 2017/03/01 06:14:03, mark a. foltz wrote:
> While it's true the route is always connected (as long as it exists) there
> should be a way to close an individual connection to the route... This should
> get addressed with larger refactoring of connections / routes.  No change
> requested here.

Acknowledged.

[dcheng, 2017-03-02 07:31:08]

mojo lgtm

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

https://codereview.chromium.org/2714693002/diff/1/content/renderer/presentati...
content/renderer/presentation/presentation_connection_proxy.cc:68:
blink::WebPresentationConnectionState::Connected);
Kind of feels like it'd be easier to just pass in the Blink type as the
argument, and then pass that through. That doesn't have to be done in this CL
though.

[zhaobin, 2017-03-02 20:10:30]

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

[commit-bot: I haz the power, 2017-03-02 20:11:00]

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-02 21:51:39]

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

[commit-bot: I haz the power, 2017-03-02 21:51:40]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2017-03-02 21:52:21]

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

https://codereview.chromium.org/2714693002/diff/1/content/renderer/presentati...
content/renderer/presentation/presentation_connection_proxy.cc:68:
blink::WebPresentationConnectionState::Connected);
On 2017/03/02 07:31:07, dcheng wrote:
> Kind of feels like it'd be easier to just pass in the Blink type as the
> argument, and then pass that through. That doesn't have to be done in this CL
> though.

Created crbug.com/698000.

[zhaobin, 2017-03-02 21:52:49]

The CQ bit was checked by zhaobin@chromium.org

[zhaobin, 2017-03-02 21:52:49]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org,
imcheng@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2714693002/#ps40001
(title: "fix unittest errors")

[commit-bot: I haz the power, 2017-03-02 21:53:36]

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-02 21:58:55]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488491569611650,
"parent_rev": "375b5de438487e3fe5b52014a5962822b59f0d19", "commit_rev":
"fb7c75d163e22589351f06fbef424f94d6e216f8"}

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

Description was changed from

==========
[Presentation API] Use connection proxy to change connection state to 'closed'

blink connection.close()
  connection_proxy.close()
    ---> mojo call
      target_connection_proxy.OnClose() // change both sides' blink connection
to 'closed'

For 1-UA, it works for both controller and receiver connection.
For 2-UA, browser_connection_proxy has no-op OnClose(), and state change is
still handled by MRP.

Also change message_request_queue_ to std::deque and erase pending send message
requests from message queue when closing connection.

BUG=688233
==========

to

==========
[Presentation API] Use connection proxy to change connection state to 'closed'

blink connection.close()
  connection_proxy.close()
    ---> mojo call
      target_connection_proxy.OnClose() // change both sides' blink connection
to 'closed'

For 1-UA, it works for both controller and receiver connection.
For 2-UA, browser_connection_proxy has no-op OnClose(), and state change is
still handled by MRP.

Also change message_request_queue_ to std::deque and erase pending send message
requests from message queue when closing connection.

BUG=688233

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

[commit-bot: I haz the power, 2017-03-02 21:59:35]

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