[Presentation API] (4th)(1-UA blink side) Add WebPresentationConnection and WebPresentationConnecti…

[Presentation API] (4th)(1-UA blink side) Add WebPresentationConnection and WebPresentationConnectionProxy API for 1-UA messaging

(Splitting of https://codereview.chromium.org/2379703002/)

PresentationConnection in controlling frame should be able to send/receive message to/from PresentationConnection in receiver frame. We do it via PresentationConnectionProxy.

Relation between WebPresentationConnection, WebPresentationConnectionProxy, and WebPresentationConnectionClient:

PresentationConnection implements WebPresentationConnection; PresentationConnectionProxy implements WebPresentationConnectionProxy and blink::mojom::PresentationConnection. Sending message from PresentationConnectionA to PresentationConnectionB

PresentationConnectionA.sendString()
  --> PresentationConnectionProxyA.sendString()
      --> (mojo call) PresentationConnectionProxyB.onMessage()
          --> PresentationConnectionB.didReceiveTextMessage()

WebPresentationConnectionClient is not related to message sending, it stores id, url, and local proxy and is used by PresentationDispatcher class to create PresentationConnection.

More details in code comments and design doc (use chromium.org account): https://docs.google.com/document/d/1XM3jhMJTQyhEC5PDAAJFNIaKh6UUEihqZDz_ztEe4Co/edit#heading=h.hadpx5oi0gml

BUG=525660
Review-Url: https://codereview.chromium.org/2471263003
Cr-Commit-Position: refs/heads/master@{#446614}
Committed: https://chromium.googlesource.com/chromium/src/+/7e02242dcc6044c914b9de1a18cd0d3358769824

[zhaobin, 2016-11-03 19:03:25]

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

[mark a. foltz, 2016-11-16 01:16:40]

Overall looks good with minor comments fixed.  I won't approve until the design
for setting up the presentation receiver is complete.

https://codereview.chromium.org/2471263003/diff/20001/content/renderer/presen...
File content/renderer/presentation/presentation_connection_client.cc (right):

https://codereview.chromium.org/2471263003/diff/20001/content/renderer/presen...
content/renderer/presentation/presentation_connection_client.cc:16: :
PresentationConnectionClient(std::move(session_info), nullptr) {}
If this object is taking ownership of |session_info|, it's a little odd for it
to copy the contents and immediately destroy it after the ctor finishes.  I
would have expected it to keep |session_info| as a class member.

I guess that is okay though, it's an implementation detail what this class does
with it.

https://codereview.chromium.org/2471263003/diff/20001/content/renderer/presen...
File content/renderer/presentation/presentation_connection_client.h (right):

https://codereview.chromium.org/2471263003/diff/20001/content/renderer/presen...
content/renderer/presentation/presentation_connection_client.h:38: // |proxy_|
to calling object (PresentationConnection).
I would just say, "Transfers ownership of returned proxy to caller."

https://codereview.chromium.org/2471263003/diff/20001/content/renderer/presen...
File content/renderer/presentation/presentation_connection_proxy.h (right):

https://codereview.chromium.org/2471263003/diff/20001/content/renderer/presen...
content/renderer/presentation/presentation_connection_proxy.h:10: #include
"third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h"
Nit: Move to .cc

https://codereview.chromium.org/2471263003/diff/20001/content/renderer/presen...
content/renderer/presentation/presentation_connection_proxy.h:20: // object
owned by current frame.
What defines "current frame"?  Maybe just say "owned by one frame."

https://codereview.chromium.org/2471263003/diff/20001/content/renderer/presen...
content/renderer/presentation/presentation_connection_proxy.h:39: // changes
works the same.
Are state changes handled by this proxy or by a different API?

https://codereview.chromium.org/2471263003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationReceiverTest.cpp
(right):

https://codereview.chromium.org/2471263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationReceiverTest.cpp:89: 
Nit: extra newlines between declarations here

https://codereview.chromium.org/2471263003/diff/20001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h
(right):

https://codereview.chromium.org/2471263003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h:8:
#include "public/platform/WebCommon.h"
May not be needed.

https://codereview.chromium.org/2471263003/diff/20001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionClient.h
(right):

https://codereview.chromium.org/2471263003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionClient.h:7:

#include <unique_ptr>

[mark a. foltz, 2016-11-16 01:17:17]

>
https://codereview.chromium.org/2471263003/diff/20001/third_party/WebKit/publ...
> File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionClient.h
(right):
> 
>
https://codereview.chromium.org/2471263003/diff/20001/third_party/WebKit/publ...
>
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionClient.h:7:

> #include <unique_ptr>

Meant to say #include <memory> (for std::unique_ptr).

For some reason reitveld didn't let me edit this comment.

[zhaobin, 2016-11-23 20:19:57]

Patchset #4 (id:60001) has been deleted

[zhaobin, 2016-11-23 22:52:29]

Changed how to invoke PresentationConnectionProxy::Send:

Before
  PresentationConnection::Send() -> PresentationConnectionProxy::Send()

After
  PresentationConnection::Send() -> PresentationDispatcher::Send() ->
PresentationConnectionProxy::Send()

- PresentationConnection::OnMessage() takes a callback
- PresentationDispatcher::SendXXX() takes PresentationConnectionProxy as
parameter

- Added PresentationConnectionProxy::DidChangeState() function

https://codereview.chromium.org/2471263003/diff/20001/content/renderer/presen...
File content/renderer/presentation/presentation_connection_client.cc (right):

https://codereview.chromium.org/2471263003/diff/20001/content/renderer/presen...
content/renderer/presentation/presentation_connection_client.cc:16: :
PresentationConnectionClient(std::move(session_info), nullptr) {}
On 2016/11/16 01:16:39, mark a. foltz wrote:
> If this object is taking ownership of |session_info|, it's a little odd for it
> to copy the contents and immediately destroy it after the ctor finishes.  I
> would have expected it to keep |session_info| as a class member.
> 
> I guess that is okay though, it's an implementation detail what this class
does
> with it.

Done.

https://codereview.chromium.org/2471263003/diff/20001/content/renderer/presen...
File content/renderer/presentation/presentation_connection_client.h (right):

https://codereview.chromium.org/2471263003/diff/20001/content/renderer/presen...
content/renderer/presentation/presentation_connection_client.h:38: // |proxy_|
to calling object (PresentationConnection).
On 2016/11/16 01:16:39, mark a. foltz wrote:
> I would just say, "Transfers ownership of returned proxy to caller."

Done.

https://codereview.chromium.org/2471263003/diff/20001/content/renderer/presen...
File content/renderer/presentation/presentation_connection_proxy.h (right):

https://codereview.chromium.org/2471263003/diff/20001/content/renderer/presen...
content/renderer/presentation/presentation_connection_proxy.h:10: #include
"third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h"
On 2016/11/16 01:16:39, mark a. foltz wrote:
> Nit: Move to .cc

Done.

https://codereview.chromium.org/2471263003/diff/20001/content/renderer/presen...
content/renderer/presentation/presentation_connection_proxy.h:20: // object
owned by current frame.
On 2016/11/16 01:16:39, mark a. foltz wrote:
> What defines "current frame"?  Maybe just say "owned by one frame."
> 

Done.

https://codereview.chromium.org/2471263003/diff/20001/content/renderer/presen...
content/renderer/presentation/presentation_connection_proxy.h:39: // changes
works the same.
On 2016/11/16 01:16:39, mark a. foltz wrote:
> Are state changes handled by this proxy or by a different API?

Will add state change APIs to Proxy class in future patches.

Proxy will handle 'connected' state change for controller and receiver
Proxy will handle 'terminated' state change for receiver ('terminated' state
change for controller is handled by MRP)
close/reconnect not implemented yet, may be handled by proxy or by PSDImpl

https://codereview.chromium.org/2471263003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationReceiverTest.cpp
(right):

https://codereview.chromium.org/2471263003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationReceiverTest.cpp:89: 
On 2016/11/16 01:16:39, mark a. foltz wrote:
> Nit: extra newlines between declarations here

Done.

https://codereview.chromium.org/2471263003/diff/20001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h
(right):

https://codereview.chromium.org/2471263003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h:8:
#include "public/platform/WebCommon.h"
On 2016/11/16 01:16:39, mark a. foltz wrote:
> May not be needed.

Need this for uint8_t, size_t.

https://codereview.chromium.org/2471263003/diff/20001/third_party/WebKit/publ...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionClient.h
(right):

https://codereview.chromium.org/2471263003/diff/20001/third_party/WebKit/publ...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionClient.h:7:

On 2016/11/16 01:16:39, mark a. foltz wrote:
> #include <unique_ptr>

Done.

[haraken, 2016-11-24 04:49:55]

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

[haraken, 2016-11-24 04:49:56]

(Just a drive-by)

https://codereview.chromium.org/2471263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationReceiverTest.cpp
(right):

https://codereview.chromium.org/2471263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationReceiverTest.cpp:105:
new MockWebPresentationConnectionProxy());

Use wrapUnique.

[mark a. foltz, 2016-12-02 22:44:00]

Getting close - a few details around passing the proxy/connections I'd like to
clear up.

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_client.h (right):

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_connection_client.h:7: 
#include <memory> for unique_ptr

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_connection_client.h:12: #include
"url/gurl.h"
nit: I think GURL can be forward declared.

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.cc (right):

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:17:
source_connection_(nullptr) {}
If the proxy can make these values mandatory in the ctor, it would make the
class more robust: it looks like little or none of the API will work without
these set.

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:50:
DCHECK(source_connection_);
What guarantees that |source_connection_| is set before OnMessage is invoked?

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:51:
DCHECK(!callback.is_null());
What guarantees that |on_message_callback_| is set before OnMessage is invoked?

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:68: NOTREACHED();
callback.Run(false) here?

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:73:
callback.Run(true);
Unless you pass false to the callback somewhere, the callback doesn't need to
take a parameter.

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:78:
DCHECK(source_connection_);
What guarantees that |source_connection_| is set when DidChangeState is invoked?

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.h (right):

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:67: void
RegisterOnMessageCallback(const OnMessageCallback& callback);
Document this method: specifically, that |callback| is invoked once per message
received.

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:213:
base::Unretained(this), nullptr));
It looks like connection_proxy is passed around to the first invocation of
DoSendMessage(), but dropped in the loop to process the next message?

Also, is connection_proxy actually used in this class?

[zhaobin, 2016-12-05 20:28:17]

https://codereview.chromium.org/2471263003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/presentation/PresentationReceiverTest.cpp
(right):

https://codereview.chromium.org/2471263003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/presentation/PresentationReceiverTest.cpp:105:
new MockWebPresentationConnectionProxy());
On 2016/11/24 04:49:55, haraken wrote:
> 
> Use wrapUnique.

Done.

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_client.h (right):

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_connection_client.h:7: 
On 2016/12/02 22:44:00, mark a. foltz wrote:
> #include <memory> for unique_ptr

Done.

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_connection_client.h:12: #include
"url/gurl.h"
On 2016/12/02 22:44:00, mark a. foltz wrote:
> nit: I think GURL can be forward declared.

Done.

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.cc (right):

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:17:
source_connection_(nullptr) {}
On 2016/12/02 22:44:00, mark a. foltz wrote:
> If the proxy can make these values mandatory in the ctor, it would make the
> class more robust: it looks like little or none of the API will work without
> these set.

Pass in on_message_callback.

For source_connection_, Proxy holds reference to PresentationConnection, and
PresentationConnection holds reference to Proxy as well. It seems that we need
to 1. create Proxy; 2. create PresentationConnection with Proxy; 3 set
proxy.source_connection_ in PresentationConnection ctor. (Or it will become 1.
create PresentationConnection; 2. create Proxy with PresentationConnection; 3.
set PresentationConnection.m_proxy in Proxy ctor. Will it be similar situation,
since we cannot garantee validity of PresentationConnection.m_proxy?)

target_connection_ (receiver connection proxy) for controller is not available
when creating controller connection proxy. Will have to set it later.

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:50:
DCHECK(source_connection_);
On 2016/12/02 22:44:00, mark a. foltz wrote:
> What guarantees that |source_connection_| is set before OnMessage is invoked?

(Sorry for the confusion, all invocation code are in patch 5) We sets
source_connection_ in PresentationConnection ctor (which will be right after
PresentationConnectionProxy ctor due to that cross reference issue...). When
this method is called, we should already have a PresentationConnection object,
and a valid source_connection_.

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:51:
DCHECK(!callback.is_null());
On 2016/12/02 22:44:00, mark a. foltz wrote:
> What guarantees that |on_message_callback_| is set before OnMessage is
invoked?

Done.

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:68: NOTREACHED();
On 2016/12/02 22:44:00, mark a. foltz wrote:
> callback.Run(false) here?

Done.

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:73:
callback.Run(true);
On 2016/12/02 22:44:00, mark a. foltz wrote:
> Unless you pass false to the callback somewhere, the callback doesn't need to
> take a parameter.

Done.

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:78:
DCHECK(source_connection_);
On 2016/12/02 22:44:00, mark a. foltz wrote:
> What guarantees that |source_connection_| is set when DidChangeState is
invoked?

Same as above.

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.h (right):

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:67: void
RegisterOnMessageCallback(const OnMessageCallback& callback);
On 2016/12/02 22:44:00, mark a. foltz wrote:
> Document this method: specifically, that |callback| is invoked once per
message
> received.

Done.

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:213:
base::Unretained(this), nullptr));
On 2016/12/02 22:44:00, mark a. foltz wrote:
> It looks like connection_proxy is passed around to the first invocation of
> DoSendMessage(), but dropped in the loop to process the next message?
> 
> Also, is connection_proxy actually used in this class?

No, it is not used in this patch (proxy is used in patch 5). Put nullptr here so
patch 4 could be landed seperately without breaking things.

[mark a. foltz, 2016-12-05 22:07:29]

LGTM with comments addressed

One thing that I had a hard time getting from the design doc is what ultimately
owns the proxy objects.  In patch #5, it looks like it's the
WebPresentationClient/PresentationDispatcher objects for the controller and
receiver.  Is that correct?  Can you clarify in the design doc?

Thanks!  Nice work!

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.cc (right):

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:17:
source_connection_(nullptr) {}
On 2016/12/05 at 20:28:16, zhaobin wrote:
> On 2016/12/02 22:44:00, mark a. foltz wrote:
> > If the proxy can make these values mandatory in the ctor, it would make the
> > class more robust: it looks like little or none of the API will work without
> > these set.
> 
> Pass in on_message_callback.
> 
> For source_connection_, Proxy holds reference to PresentationConnection, and
PresentationConnection holds reference to Proxy as well. It seems that we need
to 1. create Proxy; 2. create PresentationConnection with Proxy; 3 set
proxy.source_connection_ in PresentationConnection ctor. (Or it will become 1.
create PresentationConnection; 2. create Proxy with PresentationConnection; 3.
set PresentationConnection.m_proxy in Proxy ctor. Will it be similar situation,
since we cannot garantee validity of PresentationConnection.m_proxy?)
> 
> target_connection_ (receiver connection proxy) for controller is not available
when creating controller connection proxy. Will have to set it later.

I slightly prefer the latter approach, since you can control the exposure of the
proxy via the PresentationConnection and guarantee the proxy has a source
connection set.

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:213:
base::Unretained(this), nullptr));
On 2016/12/05 at 20:28:17, zhaobin wrote:
> On 2016/12/02 22:44:00, mark a. foltz wrote:
> > It looks like connection_proxy is passed around to the first invocation of
> > DoSendMessage(), but dropped in the loop to process the next message?
> > 
> > Also, is connection_proxy actually used in this class?
> 
> No, it is not used in this patch (proxy is used in patch 5). Put nullptr here
so patch 4 could be landed seperately without breaking things.

Can you add a TODO here to set this with a followup patch?

[mlamouri (slow - plz ping), 2016-12-06 09:57:01]

lgtm

I only had a look at the third_part/WebKit/ code. Let me know if you wanted me
to review something else. I assumed mfoltz@ had a look at the general design.

https://codereview.chromium.org/2471263003/diff/120001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h
(right):

https://codereview.chromium.org/2471263003/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h:18:
virtual ~WebPresentationConnection() {}
nit: = default;

https://codereview.chromium.org/2471263003/diff/120001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionProxy.h
(right):

https://codereview.chromium.org/2471263003/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionProxy.h:18:
virtual ~WebPresentationConnectionProxy() {}
nit: = default

[zhaobin, 2016-12-08 02:41:10]

Make PresentationConnectionProxy ctor takes
blink::WebPresentationConnectionClient as parameter so source_connection_ should
always be valid.

Use PresentationConnectionClient to pass proxy ptr back to
PresentationDispatcher. Change unique_ptr<WebPresentationConnectionClient> and
WebPresentationConnectionClient* to WebPresentationConnectionClient&. Transfer
ownership seems not necessary here.

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2471263003/diff/100001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:213:
base::Unretained(this), nullptr));
On 2016/12/05 22:07:29, mark a. foltz wrote:
> On 2016/12/05 at 20:28:17, zhaobin wrote:
> > On 2016/12/02 22:44:00, mark a. foltz wrote:
> > > It looks like connection_proxy is passed around to the first invocation of
> > > DoSendMessage(), but dropped in the loop to process the next message?
> > > 
> > > Also, is connection_proxy actually used in this class?
> > 
> > No, it is not used in this patch (proxy is used in patch 5). Put nullptr
here
> so patch 4 could be landed seperately without breaking things.
> 
> Can you add a TODO here to set this with a followup patch?

Done.

https://codereview.chromium.org/2471263003/diff/120001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h
(right):

https://codereview.chromium.org/2471263003/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h:18:
virtual ~WebPresentationConnection() {}
On 2016/12/06 09:57:01, mlamouri wrote:
> nit: = default;

Done.

https://codereview.chromium.org/2471263003/diff/120001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionProxy.h
(right):

https://codereview.chromium.org/2471263003/diff/120001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionProxy.h:18:
virtual ~WebPresentationConnectionProxy() {}
On 2016/12/06 09:57:01, mlamouri wrote:
> nit: = default

Done.

[zhaobin, 2017-01-12 22:14:47]

Patchset #8 (id:160001) has been deleted

[zhaobin, 2017-01-12 22:31:11]

Patchset #8 (id:180001) has been deleted

[zhaobin, 2017-01-12 22:55:00]

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

[zhaobin, 2017-01-12 22:55:01]

Please review the latest patch against base. Major changes from previous
patches:

- split PresentationConnectionProxy into ControllerConnectionProxy and
ReceiverConnectionProxy to accomodate latest presentation.mojom API change

- make PresentationConnectionCallbacks return newly created
PresentationConnection object. Use returned connection object to create
connection proxy object in dispatcher (code in patch 5)

haraken@, could you please take a look at the newly added file under
third_party/WebKit/public/platform/ (again)?

dcheng@, FYI

[haraken, 2017-01-13 04:01:24]

LGTM

https://codereview.chromium.org/2471263003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/WebPresentationConnection.cpp
(right):

https://codereview.chromium.org/2471263003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebPresentationConnection.cpp:1: //
Copyright 2016 The Chromium Authors. All rights reserved.

2017

https://codereview.chromium.org/2471263003/diff/200001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/exported/WebPresentationConnectionCallbacks.cpp
(right):

https://codereview.chromium.org/2471263003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebPresentationConnectionCallbacks.cpp:1:
// Copyright 2016 The Chromium Authors. All rights reserved.

2017

https://codereview.chromium.org/2471263003/diff/200001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h
(right):

https://codereview.chromium.org/2471263003/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h:1:
// Copyright 2016 The Chromium Authors. All rights reserved.

2017

https://codereview.chromium.org/2471263003/diff/200001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionCallbacks.h
(right):

https://codereview.chromium.org/2471263003/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionCallbacks.h:1:
// Copyright 2016 The Chromium Authors. All rights reserved.

2017

https://codereview.chromium.org/2471263003/diff/200001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionProxy.h
(right):

https://codereview.chromium.org/2471263003/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionProxy.h:1:
// Copyright 2016 The Chromium Authors. All rights reserved.

2017

[dcheng, 2017-01-17 23:31:49]

LGTM with a few minor nits.

https://codereview.chromium.org/2471263003/diff/200001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.cc (right):

https://codereview.chromium.org/2471263003/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:100:
binding_.Bind(std::move(target_conn_request));
Nit: conn => connection

https://codereview.chromium.org/2471263003/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:103: void
ReceiverConnectionProxy::SetTargetConnection(
Nit: consider calling this BindTargetConnection as a hint that's only supposed
to be called once (this is a weak convention, but I personally like it)

https://codereview.chromium.org/2471263003/diff/200001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.h (right):

https://codereview.chromium.org/2471263003/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:27: //
PresentationConnection object owned by a different frame.
Nit: update these comments? I found them helpful while trying to understand the
initial patch.

https://codereview.chromium.org/2471263003/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:63:
PresentationConnectionProxy(blink::WebPresentationConnection*);
Nit: explicit (also, please name the constructor parameter)

https://codereview.chromium.org/2471263003/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:75:
ControllerConnectionProxy(blink::WebPresentationConnection*);
Ditto with explicit here.

https://codereview.chromium.org/2471263003/diff/200001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h
(right):

https://codereview.chromium.org/2471263003/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h:23:
virtual void setProxy(std::unique_ptr<WebPresentationConnectionProxy>) = 0;
Nit: consider calling this bindProxy(), as the impl looks like it's only
supposed to be called once (setProxy() makes it look like it's ok to call
multiple times)

[zhaobin, 2017-01-18 18:25:47]

https://codereview.chromium.org/2471263003/diff/200001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.cc (right):

https://codereview.chromium.org/2471263003/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:100:
binding_.Bind(std::move(target_conn_request));
On 2017/01/17 23:31:49, dcheng wrote:
> Nit: conn => connection

Done.

https://codereview.chromium.org/2471263003/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:103: void
ReceiverConnectionProxy::SetTargetConnection(
On 2017/01/17 23:31:49, dcheng wrote:
> Nit: consider calling this BindTargetConnection as a hint that's only supposed
> to be called once (this is a weak convention, but I personally like it)

Done.

https://codereview.chromium.org/2471263003/diff/200001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.h (right):

https://codereview.chromium.org/2471263003/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:27: //
PresentationConnection object owned by a different frame.
On 2017/01/17 23:31:49, dcheng wrote:
> Nit: update these comments? I found them helpful while trying to understand
the
> initial patch.

Done.

https://codereview.chromium.org/2471263003/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:63:
PresentationConnectionProxy(blink::WebPresentationConnection*);
On 2017/01/17 23:31:49, dcheng wrote:
> Nit: explicit (also, please name the constructor parameter)

Done.

https://codereview.chromium.org/2471263003/diff/200001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:75:
ControllerConnectionProxy(blink::WebPresentationConnection*);
On 2017/01/17 23:31:49, dcheng wrote:
> Ditto with explicit here.

Done.

https://codereview.chromium.org/2471263003/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/exported/WebPresentationConnection.cpp
(right):

https://codereview.chromium.org/2471263003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebPresentationConnection.cpp:1: //
Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/01/13 04:01:24, haraken wrote:
> 
> 2017

Done.

https://codereview.chromium.org/2471263003/diff/200001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/exported/WebPresentationConnectionCallbacks.cpp
(right):

https://codereview.chromium.org/2471263003/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/exported/WebPresentationConnectionCallbacks.cpp:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/01/13 04:01:24, haraken wrote:
> 
> 2017

Done.

https://codereview.chromium.org/2471263003/diff/200001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h
(right):

https://codereview.chromium.org/2471263003/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/01/13 04:01:24, haraken wrote:
> 
> 2017

Done.

https://codereview.chromium.org/2471263003/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h:23:
virtual void setProxy(std::unique_ptr<WebPresentationConnectionProxy>) = 0;
On 2017/01/17 23:31:49, dcheng wrote:
> Nit: consider calling this bindProxy(), as the impl looks like it's only
> supposed to be called once (setProxy() makes it look like it's ok to call
> multiple times)

Done.

https://codereview.chromium.org/2471263003/diff/200001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionCallbacks.h
(right):

https://codereview.chromium.org/2471263003/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionCallbacks.h:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/01/13 04:01:24, haraken wrote:
> 
> 2017

Done.

https://codereview.chromium.org/2471263003/diff/200001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionProxy.h
(right):

https://codereview.chromium.org/2471263003/diff/200001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnectionProxy.h:1:
// Copyright 2016 The Chromium Authors. All rights reserved.
On 2017/01/13 04:01:24, haraken wrote:
> 
> 2017

Done.

[imcheng, 2017-01-20 20:15:43]

Code structure looks good overall. Just some questions. Adding more
documentations to the new methods / interfaces would be helpful.

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.cc (right):

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:29:
DCHECK(target_connection_);
What prevents this method from being called before target_connection_ is
created? Is it the fact that Blink only forwards messages to here when state is
connected?

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:36:
DCHECK(source_connection_);
This DCHECK already exists in the constructor. Not needed?

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:62: void
PresentationConnectionProxy::DidChangeState(
If I were to close() one side of a 1-UA presentation, does the state change go
through PresentationService / PresentationDispatcher?

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:64:
DCHECK(source_connection_);
ditto on DCHECK

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:69: //
|DidChangeState| should only handles state transition from connecting ->
s/handles/handle

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.h (right):

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:33: //      
ControllerConnectionProxy::MakeRemoveRequest() {
s/Remove/Remote

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:44: //       
ReceiverConnectionProxy::Bind(
nit: This statement doesn't parse for me. Is this supposed to be just
ReceiverConnectionProxy::Bind(target_connection_request); ?

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:76: virtual void
SendConnectionMessage(
Does this need to be virtual?

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:92:
blink::WebPresentationConnection* source_connection_;
Can the pointer be const? Please also document ownership.

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:103: virtual
blink::mojom::PresentationConnectionPtr Bind();
ditto on virtual here and below. Please also include documentation on these
functions.

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:439: if (!receiver_) {
revert?

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
File content/renderer/presentation/presentation_dispatcher.h (right):

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.h:79: const
blink::WebPresentationConnectionProxy* connection_proxy;
Is this used in this patch? Please document what this field is for. Also, what
happens when |connection_proxy| is destroyed?

https://codereview.chromium.org/2471263003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp
(right):

https://codereview.chromium.org/2471263003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:323:
client->sendString(m_url, m_id, message->text, nullptr);
Should this be |m_proxy.get()| here and below?

https://codereview.chromium.org/2471263003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/PresentationConnection.h
(right):

https://codereview.chromium.org/2471263003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationConnection.h:51: void
bindProxy(std::unique_ptr<WebPresentationConnectionProxy>) override;
nit: group the override methods together and comment:
// WebPresentationConnection implementation.

https://codereview.chromium.org/2471263003/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp
(right):

https://codereview.chromium.org/2471263003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp:41:
m_resolver->getExecutionContext()->isContextDestroyed())
nit: braces and empty line for consistency

https://codereview.chromium.org/2471263003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp:44:
m_connection = nullptr;
not needed since it's already set to nullptr?

https://codereview.chromium.org/2471263003/diff/220001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationClient.h
(right):

https://codereview.chromium.org/2471263003/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/WebPresentationClient.h:54:
// session.
Please document the proxy argument.

https://codereview.chromium.org/2471263003/diff/220001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h
(right):

https://codereview.chromium.org/2471263003/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h:23:
virtual void bindProxy(std::unique_ptr<WebPresentationConnectionProxy>) = 0;
Please add comments.

[zhaobin, 2017-01-23 19:38:50]

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.cc (right):

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:29:
DCHECK(target_connection_);
On 2017/01/20 20:15:43, imcheng wrote:
> What prevents this method from being called before target_connection_ is
> created? Is it the fact that Blink only forwards messages to here when state
is
> connected?

Yes.

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:36:
DCHECK(source_connection_);
On 2017/01/20 20:15:43, imcheng wrote:
> This DCHECK already exists in the constructor. Not needed?

Done.

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:62: void
PresentationConnectionProxy::DidChangeState(
On 2017/01/20 20:15:43, imcheng wrote:
> If I were to close() one side of a 1-UA presentation, does the state change go
> through PresentationService / PresentationDispatcher?

Yes.

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:64:
DCHECK(source_connection_);
On 2017/01/20 20:15:43, imcheng wrote:
> ditto on DCHECK

Done.

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:69: //
|DidChangeState| should only handles state transition from connecting ->
On 2017/01/20 20:15:43, imcheng wrote:
> s/handles/handle

Done.

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.h (right):

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:33: //      
ControllerConnectionProxy::MakeRemoveRequest() {
On 2017/01/20 20:15:43, imcheng wrote:
> s/Remove/Remote

Done.

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:44: //       
ReceiverConnectionProxy::Bind(
On 2017/01/20 20:15:43, imcheng wrote:
> nit: This statement doesn't parse for me. Is this supposed to be just
> ReceiverConnectionProxy::Bind(target_connection_request); ?

Done.

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:76: virtual void
SendConnectionMessage(
On 2017/01/20 20:15:43, imcheng wrote:
> Does this need to be virtual?

Used in unit test in 5th patch. (Cannot reference
blink::mojom::ConnectionMessagePtr and add SendConnectionMessage to
WebPresentationConnectionProxy interface)

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:92:
blink::WebPresentationConnection* source_connection_;
On 2017/01/20 20:15:43, imcheng wrote:
> Can the pointer be const? Please also document ownership.

Done.

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:103: virtual
blink::mojom::PresentationConnectionPtr Bind();
On 2017/01/20 20:15:43, imcheng wrote:
> ditto on virtual here and below. Please also include documentation on these
> functions.

Done.

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:439: if (!receiver_) {
On 2017/01/20 20:15:43, imcheng wrote:
> revert?

Done.

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
File content/renderer/presentation/presentation_dispatcher.h (right):

https://codereview.chromium.org/2471263003/diff/220001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.h:79: const
blink::WebPresentationConnectionProxy* connection_proxy;
On 2017/01/20 20:15:43, imcheng wrote:
> Is this used in this patch? Please document what this field is for. Also, what
> happens when |connection_proxy| is destroyed?

Done.

https://codereview.chromium.org/2471263003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp
(right):

https://codereview.chromium.org/2471263003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:323:
client->sendString(m_url, m_id, message->text, nullptr);
On 2017/01/20 20:15:43, imcheng wrote:
> Should this be |m_proxy.get()| here and below?

Done.

https://codereview.chromium.org/2471263003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/PresentationConnection.h
(right):

https://codereview.chromium.org/2471263003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationConnection.h:51: void
bindProxy(std::unique_ptr<WebPresentationConnectionProxy>) override;
On 2017/01/20 20:15:43, imcheng wrote:
> nit: group the override methods together and comment:
> // WebPresentationConnection implementation.

Done.

https://codereview.chromium.org/2471263003/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp
(right):

https://codereview.chromium.org/2471263003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp:41:
m_resolver->getExecutionContext()->isContextDestroyed())
On 2017/01/20 20:15:43, imcheng wrote:
> nit: braces and empty line for consistency

Done.

https://codereview.chromium.org/2471263003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationConnectionCallbacks.cpp:44:
m_connection = nullptr;
On 2017/01/20 20:15:43, imcheng wrote:
> not needed since it's already set to nullptr?

Set m_connection to nullptr so following sequence works:

callback->onSuccess() // set m_connection
callback->onError() // m_connection = nullptr (quite unlikely since we reset
callback everytime)
callback->getConnection() // return nullptr

Leave it there to be safe?

https://codereview.chromium.org/2471263003/diff/220001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationClient.h
(right):

https://codereview.chromium.org/2471263003/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/WebPresentationClient.h:54:
// session.
On 2017/01/20 20:15:43, imcheng wrote:
> Please document the proxy argument.

Done.

https://codereview.chromium.org/2471263003/diff/220001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h
(right):

https://codereview.chromium.org/2471263003/diff/220001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h:23:
virtual void bindProxy(std::unique_ptr<WebPresentationConnectionProxy>) = 0;
On 2017/01/20 20:15:43, imcheng wrote:
> Please add comments.

Done.

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

Still LGTM with comments addressed.

The design has evolved somewhat since the last review by mlamouri@.  It may be a
good idea to get a re-review of the third_party/WebKit code, depending on how
many substantial changes there have been.

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.cc (right):

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:29:
DCHECK(target_connection_);
It seems like the following three things have to be true for this to succeed:
(1) Source connection is in connected state.
(2) Target connection is in connected state.
(3) Target connection is set (BindTargetConnection has been called).

How do you check (1) and (2)?

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:67: //
|DidChangeState| should only handle state transition from connecting ->
Long term, I'd rather have one code path to handle state transitions - this is
part of a larger conversation about where state changes should be managed w.r.t.
connection lifetime.

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.h (right):

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:59: //   
SendString().
I don't see SendString() defined below.  Did you mean SendConnectionMessage()?

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:78:
blink::mojom::ConnectionMessagePtr session_message,
s/session_message/connection_message/ here and elsewhere ConnectionMessagePtr is
used

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:88:
blink::WebPresentationConnection* connection);
source_connection

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:90:
mojo::InterfacePtr<blink::mojom::PresentationConnection> target_connection_;
Slight preference for naming this target_connection_ptr_ (which is a Mojo
InterfacePtr) to distinguish it from source_connection_ (which is a Blink
connection object).

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:93: // Raw pointer
to blink connection object owning this proxy object. Does not
s/blink/Blink/

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:103:
blink::WebPresentationConnection* connection);
s/connection/controller_connection/

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:115:
blink::WebPresentationConnection* connection);
s/connection/receiver_connection/

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:119:
blink::mojom::PresentationConnectionRequest target_connection_request);
Could this be receiver_connection_request? The request is *for* a receiver
connection by the controller frame if I understand correctly?

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:120: void
BindTargetConnection(blink::mojom::PresentationConnectionPtr connection);
Can this be BindControllerConnection() with controller_connection_ptr?

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy_unittest.cc
(right):

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy_unittest.cc:33:
class PresentationConnectionProxyTest : public ::testing::Test {
Nice unit test :-)

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy_unittest.cc:49: void
SetUpPresentationConnectionProxy() {
Should this just be part of SetUp()?

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
File content/renderer/presentation/presentation_dispatcher.h (right):

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.h:53: // from
PresentationDispatcher to PresentationConnectionProxy.
That would be great.  Eventually we want to get rid of
content/renderer/presentation and that would help.

Can you file a crbug to track this and change this to TODO(crbug/XXX)?

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.h:81: // blink connection.
Blink connection is destroyed after
s/blink/Blink/ throughout in comments

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
File content/renderer/presentation/presentation_dispatcher_unittest.cc (right):

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher_unittest.cc:124: class
TestPresentationConnection : public blink::WebPresentationConnection {
Is this identical to the one in presentation_connection_proxy_unittest.cc?  If
so, can it be factored out into a test support class?

https://codereview.chromium.org/2471263003/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/presentation/ExistingPresentationConnectionCallbacks.h
(right):

https://codereview.chromium.org/2471263003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/ExistingPresentationConnectionCallbacks.h:24:
// TODO(zhaobin): combine this class with PresentationConnectionCallbacks.
Can you file a crbug to track this and change this to TODO(crbug/XXX)?

https://codereview.chromium.org/2471263003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/PresentationReceiverTest.cpp
(right):

https://codereview.chromium.org/2471263003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationReceiverTest.cpp:106:
class MockWebPresentationConnectionProxy
Are tests that exercise this mock coming in patch #5?

[imcheng, 2017-01-23 22:02:35]

lgtm after comments addressed.

https://codereview.chromium.org/2471263003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp
(right):

https://codereview.chromium.org/2471263003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:316:
if (!client || !m_proxy)
2-ua connections, which don't have m_proxy, still uses this code path for
messaging, right?

https://codereview.chromium.org/2471263003/diff/240001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h
(right):

https://codereview.chromium.org/2471263003/diff/240001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h:26:
// Notifies the presentation about new message.
nit: blank line before comment, here and below.

[zhaobin, 2017-01-24 01:23:24]

mlamouri@, could you please re-review the third_party/Webkit code since the
design has evolved somewhat?

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.cc (right):

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:29:
DCHECK(target_connection_);
On 2017/01/23 20:18:41, mark a. foltz wrote:
> It seems like the following three things have to be true for this to succeed:
> (1) Source connection is in connected state.
> (2) Target connection is in connected state.
> (3) Target connection is set (BindTargetConnection has been called).
> 
> How do you check (1) and (2)?

For (1), we will call into PresentationConnectionProxy::SendConnectionMessage()
only if Blink connection (source_connection_) is in 'connected' state because
PresentationConnection::send() checks Blink connection state.

For (2), in 5th patch, we are going to remove 'connected' state transition in
PSImpl so the only place changing 'connecting' to 'connected' is
ReceiverConnectionProxy::BindTargetConnection.

If ReceiverConnectionProxy::BindTargetConnection() has been called before
sending message:

target_connection_ = std::move(connection); // set receiver proxy's
target_connection_ (3)
target_connection_->DidChangeState(blink::mojom::PresentationConnectionState::CONNECTED);
// controller connection changes to 'connected'
DidChangeState(blink::mojom::PresentationConnectionState::CONNECTED); //
receiver connection changes to 'connected'

sending message from receiver -> controller: mojo call
target_connection_->OnMessage() will be queued after
target_connection_->DidChangeState(CONNECTED)
sending message from controller -> receiver: mojo call 
target_connection_->OnMessage() will be sent after local call
DidChangeState(CONNECTED)

For (3), make sure ReceiverConnectionProxy::Bind() and
ReceiverConnectionProxy::BindTargetConnection() are called before sending
messages.

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:67: //
|DidChangeState| should only handle state transition from connecting ->
On 2017/01/23 20:18:41, mark a. foltz wrote:
> Long term, I'd rather have one code path to handle state transitions - this is
> part of a larger conversation about where state changes should be managed
w.r.t.
> connection lifetime.  

crbug.com/588874 tracks this. Added a TODO. Currently, close/terminate state
transition is sent by MR extension (cast_provider and cloud_provider). Connected
state transition is sent by PSImpl (by PresentationConnectionProxy in 5th
patch). We can remove this state change if we find a place in MR extension (cast
provider and cloud provider) where we can signal connected state properly.

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.h (right):

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:59: //   
SendString().
On 2017/01/23 20:18:42, mark a. foltz wrote:
> I don't see SendString() defined below.  Did you mean SendConnectionMessage()?

Done.

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:78:
blink::mojom::ConnectionMessagePtr session_message,
On 2017/01/23 20:18:41, mark a. foltz wrote:
> s/session_message/connection_message/ here and elsewhere ConnectionMessagePtr
is
> used

Done.

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:88:
blink::WebPresentationConnection* connection);
On 2017/01/23 20:18:42, mark a. foltz wrote:
> source_connection

Done.

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:90:
mojo::InterfacePtr<blink::mojom::PresentationConnection> target_connection_;
On 2017/01/23 20:18:42, mark a. foltz wrote:
> Slight preference for naming this target_connection_ptr_ (which is a Mojo
> InterfacePtr) to distinguish it from source_connection_ (which is a Blink
> connection object).

Done.

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:93: // Raw pointer
to blink connection object owning this proxy object. Does not
On 2017/01/23 20:18:42, mark a. foltz wrote:
> s/blink/Blink/

Done.

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:103:
blink::WebPresentationConnection* connection);
On 2017/01/23 20:18:42, mark a. foltz wrote:
> s/connection/controller_connection/

Done.

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:115:
blink::WebPresentationConnection* connection);
On 2017/01/23 20:18:42, mark a. foltz wrote:
> s/connection/receiver_connection/

Done.

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:119:
blink::mojom::PresentationConnectionRequest target_connection_request);
On 2017/01/23 20:18:41, mark a. foltz wrote:
> Could this be receiver_connection_request? The request is *for* a receiver
> connection by the controller frame if I understand correctly?

Done.

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:120: void
BindTargetConnection(blink::mojom::PresentationConnectionPtr connection);
On 2017/01/23 20:18:42, mark a. foltz wrote:
> Can this be BindControllerConnection() with controller_connection_ptr?

Done.

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy_unittest.cc
(right):

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy_unittest.cc:49: void
SetUpPresentationConnectionProxy() {
On 2017/01/23 20:18:42, mark a. foltz wrote:
> Should this just be part of SetUp()?

Done.

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
File content/renderer/presentation/presentation_dispatcher.h (right):

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.h:53: // from
PresentationDispatcher to PresentationConnectionProxy.
On 2017/01/23 20:18:42, mark a. foltz wrote:
> That would be great.  Eventually we want to get rid of
> content/renderer/presentation and that would help.
> 
> Can you file a crbug to track this and change this to TODO(crbug/XXX)?

Done.

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.h:81: // blink connection.
Blink connection is destroyed after
On 2017/01/23 20:18:42, mark a. foltz wrote:
> s/blink/Blink/ throughout in comments

Done.

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
File content/renderer/presentation/presentation_dispatcher_unittest.cc (right):

https://codereview.chromium.org/2471263003/diff/240001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher_unittest.cc:124: class
TestPresentationConnection : public blink::WebPresentationConnection {
On 2017/01/23 20:18:42, mark a. foltz wrote:
> Is this identical to the one in presentation_connection_proxy_unittest.cc?  If
> so, can it be factored out into a test support class?

Done.

https://codereview.chromium.org/2471263003/diff/240001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/modules/presentation/ExistingPresentationConnectionCallbacks.h
(right):

https://codereview.chromium.org/2471263003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/ExistingPresentationConnectionCallbacks.h:24:
// TODO(zhaobin): combine this class with PresentationConnectionCallbacks.
On 2017/01/23 20:18:42, mark a. foltz wrote:
> Can you file a crbug to track this and change this to TODO(crbug/XXX)?

Done.

https://codereview.chromium.org/2471263003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp
(right):

https://codereview.chromium.org/2471263003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationConnection.cpp:316:
if (!client || !m_proxy)
On 2017/01/23 22:02:35, imcheng wrote:
> 2-ua connections, which don't have m_proxy, still uses this code path for
> messaging, right?

2-ua will have m_proxy as well. In this patch, m_proxy is a dummy object passed
from blink connection to PresentationDispatcher for both 1-ua and 2-ua. We still
use presentation_service_->SendConnectionMessage() to send message. In 5th
patch, we will use m_proxy to send message: 1-ua from ControllerConnectionProxy
to ReceiverConnectionProxy; 2-ua from ControllerConnectionProxy to
BrowserConnectionProxy.

https://codereview.chromium.org/2471263003/diff/240001/third_party/WebKit/Sou...
File third_party/WebKit/Source/modules/presentation/PresentationReceiverTest.cpp
(right):

https://codereview.chromium.org/2471263003/diff/240001/third_party/WebKit/Sou...
third_party/WebKit/Source/modules/presentation/PresentationReceiverTest.cpp:106:
class MockWebPresentationConnectionProxy
On 2017/01/23 20:18:43, mark a. foltz wrote:
> Are tests that exercise this mock coming in patch #5?

Removed. Not needed here.

https://codereview.chromium.org/2471263003/diff/240001/third_party/WebKit/pub...
File
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h
(right):

https://codereview.chromium.org/2471263003/diff/240001/third_party/WebKit/pub...
third_party/WebKit/public/platform/modules/presentation/WebPresentationConnection.h:26:
// Notifies the presentation about new message.
On 2017/01/23 22:02:35, imcheng wrote:
> nit: blank line before comment, here and below.

Done.

[zhaobin, 2017-01-25 19:29:13]

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

[commit-bot: I haz the power, 2017-01-25 19:30:04]

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-25 20:22:42]

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

[commit-bot: I haz the power, 2017-01-25 20:22:44]

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

[zhaobin, 2017-01-26 02:13:19]

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

[commit-bot: I haz the power, 2017-01-26 02:14:19]

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-26 05:44:43]

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

[commit-bot: I haz the power, 2017-01-26 05:44:44]

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

[zhaobin, 2017-01-26 19:15:38]

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

[commit-bot: I haz the power, 2017-01-26 19:16:34]

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-26 19:59:33]

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

[commit-bot: I haz the power, 2017-01-26 19:59:33]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2017-01-26 22:18:03]

My comments are mostly on the surface. It looks okay but I didn't dig very deep
but I assume the design was reviewed already :)

still lgtm

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.cc (right):

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:54: break;
nit: it's a NOTREACHED() but you might want to return instead of `break` because
otherwise, you will call `callback.Run()` twice and that might not work very
well :)

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:65: if (state ==
blink::mojom::PresentationConnectionState::CONNECTED) {
nit:
```
if (state != blink::mojom:...::CONNECTED) {
  NOTREACHED();
  return;
}

// real stuff
```

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:79:
ControllerConnectionProxy::~ControllerConnectionProxy() {}
= default;

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:96:
ReceiverConnectionProxy::~ReceiverConnectionProxy() {}
ditto

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.h (right):

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:19: using
OnMessageCallback = base::Callback<void(bool)>;
Can you move this inside the class? Otherwise, you are defining
content::OnMessageCallback for everyone who includes this header.

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:70: //
presentations.
That documentation looks great! :)

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy_unittest.cc
(right):

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy_unittest.cc:23:
~PresentationConnectionProxyTest() override {}
= default for both?

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:518:
connection->bindProxy(base::WrapUnique(controller_connection_proxy));
nit: could have this in one instruction instead of two with some `auto*` in the
middle.

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:531:
connection->bindProxy(base::WrapUnique(receiver_connection_proxy));
ditto

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
File content/renderer/presentation/test_presentation_connection.cc (right):

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
content/renderer/presentation/test_presentation_connection.cc:15:
TestPresentationConnection::~TestPresentationConnection() {}
`= default` for both?

[zhaobin, 2017-01-26 22:47:50]

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

[zhaobin, 2017-01-26 22:48:14]

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.cc (right):

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:54: break;
On 2017/01/26 22:18:02, mlamouri (slow) wrote:
> nit: it's a NOTREACHED() but you might want to return instead of `break`
because
> otherwise, you will call `callback.Run()` twice and that might not work very
> well :)

Done.

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:65: if (state ==
blink::mojom::PresentationConnectionState::CONNECTED) {
On 2017/01/26 22:18:02, mlamouri (slow) wrote:
> nit:
> ```
> if (state != blink::mojom:...::CONNECTED) {
>   NOTREACHED();
>   return;
> }
> 
> // real stuff
> ```

Done.

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:79:
ControllerConnectionProxy::~ControllerConnectionProxy() {}
On 2017/01/26 22:18:02, mlamouri (slow) wrote:
> = default;

Done.

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.cc:96:
ReceiverConnectionProxy::~ReceiverConnectionProxy() {}
On 2017/01/26 22:18:02, mlamouri (slow) wrote:
> ditto

Done.

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy.h (right):

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy.h:19: using
OnMessageCallback = base::Callback<void(bool)>;
On 2017/01/26 22:18:02, mlamouri (slow) wrote:
> Can you move this inside the class? Otherwise, you are defining
> content::OnMessageCallback for everyone who includes this header.

Done.

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
File content/renderer/presentation/presentation_connection_proxy_unittest.cc
(right):

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
content/renderer/presentation/presentation_connection_proxy_unittest.cc:23:
~PresentationConnectionProxyTest() override {}
On 2017/01/26 22:18:02, mlamouri (slow) wrote:
> = default for both?

Done.

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
File content/renderer/presentation/presentation_dispatcher.cc (right):

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:518:
connection->bindProxy(base::WrapUnique(controller_connection_proxy));
On 2017/01/26 22:18:02, mlamouri (slow) wrote:
> nit: could have this in one instruction instead of two with some `auto*` in
the
> middle.

Done.

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
content/renderer/presentation/presentation_dispatcher.cc:531:
connection->bindProxy(base::WrapUnique(receiver_connection_proxy));
On 2017/01/26 22:18:02, mlamouri (slow) wrote:
> ditto

|receiver_connection_proxy| is used in following lines.

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
File content/renderer/presentation/test_presentation_connection.cc (right):

https://codereview.chromium.org/2471263003/diff/300001/content/renderer/prese...
content/renderer/presentation/test_presentation_connection.cc:15:
TestPresentationConnection::~TestPresentationConnection() {}
On 2017/01/26 22:18:03, mlamouri (slow) wrote:
> `= default` for both?

Done.

[commit-bot: I haz the power, 2017-01-26 22:48:45]

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-27 01:04:52]

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

[commit-bot: I haz the power, 2017-01-27 01:04:54]

Dry run: This issue passed the CQ dry run.

[zhaobin, 2017-01-27 01:07:10]

The CQ bit was checked by zhaobin@chromium.org

[zhaobin, 2017-01-27 01:07:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, dcheng@chromium.org, mfoltz@chromium.org,
imcheng@chromium.org, mlamouri@chromium.org
Link to the patchset: https://codereview.chromium.org/2471263003/#ps320001
(title: "resolve code review comments from mlamouri")

[commit-bot: I haz the power, 2017-01-27 01:08: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-01-27 01:34:24]

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

[commit-bot: I haz the power, 2017-01-27 01:34:26]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[zhaobin, 2017-01-27 01:37:16]

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

[zhaobin, 2017-01-27 01:37:17]

+jam@ to review content/renderer/BUILD.gn

[jam1, 2017-01-27 04:26:14]

jam@google.com changed reviewers:
+ jam@google.com

[jam1, 2017-01-27 04:26:16]

lgtm

[jam, 2017-01-27 06:25:05]

lgtm

[zhaobin, 2017-01-27 06:27:43]

The CQ bit was checked by zhaobin@chromium.org

[commit-bot: I haz the power, 2017-01-27 06:28:11]

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-27 06:39:02]

CQ is committing da patch.

Bot data: {"patchset_id": 320001, "attempt_start_ts": 1485498463841160,
"parent_rev": "e8c75f95e39c0cc7cb1a0079abdad0a26e1abe0a", "commit_rev":
"7e02242dcc6044c914b9de1a18cd0d3358769824"}

[commit-bot: I haz the power, 2017-01-27 06:39:53]

Description was changed from

==========
[Presentation API] (4th)(1-UA blink side) Add WebPresentationConnection and
WebPresentationConnectionProxy API for 1-UA messaging

(Splitting of https://codereview.chromium.org/2379703002/)

PresentationConnection in controlling frame should be able to send/receive
message to/from PresentationConnection in receiver frame. We do it via
PresentationConnectionProxy.

Relation between WebPresentationConnection, WebPresentationConnectionProxy, and
WebPresentationConnectionClient:

PresentationConnection implements WebPresentationConnection;
PresentationConnectionProxy implements WebPresentationConnectionProxy and
blink::mojom::PresentationConnection. Sending message from
PresentationConnectionA to PresentationConnectionB

PresentationConnectionA.sendString()
  --> PresentationConnectionProxyA.sendString()
      --> (mojo call) PresentationConnectionProxyB.onMessage()
          --> PresentationConnectionB.didReceiveTextMessage()

WebPresentationConnectionClient is not related to message sending, it stores id,
url, and local proxy and is used by PresentationDispatcher class to create
PresentationConnection.

More details in code comments and design doc (use chromium.org account):
https://docs.google.com/document/d/1XM3jhMJTQyhEC5PDAAJFNIaKh6UUEihqZDz_ztEe4...

BUG=525660
==========

to

==========
[Presentation API] (4th)(1-UA blink side) Add WebPresentationConnection and
WebPresentationConnectionProxy API for 1-UA messaging

(Splitting of https://codereview.chromium.org/2379703002/)

PresentationConnection in controlling frame should be able to send/receive
message to/from PresentationConnection in receiver frame. We do it via
PresentationConnectionProxy.

Relation between WebPresentationConnection, WebPresentationConnectionProxy, and
WebPresentationConnectionClient:

PresentationConnection implements WebPresentationConnection;
PresentationConnectionProxy implements WebPresentationConnectionProxy and
blink::mojom::PresentationConnection. Sending message from
PresentationConnectionA to PresentationConnectionB

PresentationConnectionA.sendString()
  --> PresentationConnectionProxyA.sendString()
      --> (mojo call) PresentationConnectionProxyB.onMessage()
          --> PresentationConnectionB.didReceiveTextMessage()

WebPresentationConnectionClient is not related to message sending, it stores id,
url, and local proxy and is used by PresentationDispatcher class to create
PresentationConnection.

More details in code comments and design doc (use chromium.org account):
https://docs.google.com/document/d/1XM3jhMJTQyhEC5PDAAJFNIaKh6UUEihqZDz_ztEe4...

BUG=525660

Review-Url: https://codereview.chromium.org/2471263003
Cr-Commit-Position: refs/heads/master@{#446614}
Committed:
https://chromium.googlesource.com/chromium/src/+/7e02242dcc6044c914b9de1a18cd...
==========

[commit-bot: I haz the power, 2017-01-27 06:39:55]

Committed patchset #14 (id:320001) as
https://chromium.googlesource.com/chromium/src/+/7e02242dcc6044c914b9de1a18cd...