[chrome.displaySource] Add WiFi Display session class skeleton and mojo service

Add WiFi Display session class skeleton and mojo service

This patch introduces a WiFi Display session class
skeleton and mojo service which provides network
access for the render-hosted session.

The introduced code is compiled only if a newly added
'enable_wifi_display' build option is set.

Besides, some minor changes were applied to the
DisplaySourceConnectionDelegate interface in order
to better define its methods behavior.

BUG=242107
Committed: https://crrev.com/61a28e373720d8f205d9fc37c09580804a36b038
Cr-Commit-Position: refs/heads/master@{#369415}

[Mikhail, 2015-12-18 14:35:14]

Patchset #1 (id:1) has been deleted

[Mikhail, 2015-12-18 14:37:37]

Description was changed from

==========
Add WiFi Display session class skeleton and mojo service

This patch introduces a WiFi Display session class skeleton and mojo service
which provides network access for the render-hosted session.
The introduced code is compiled only if a newly added 'enable_wifi_display'
build option is set.

Besides, some minor changes were applied to the DisplaySourceConnectionDelegate
interface in order to better define its methods behavior.

BUG=242107
==========

to

==========
Add WiFi Display session class skeleton and mojo service

This patch introduces a WiFi Display session class skeleton and mojo service
which provides network access for the render-hosted session.
The introduced code is compiled only if a newly added 'enable_wifi_display'
build option is set.

Besides, some minor changes were applied to the DisplaySourceConnectionDelegate
interface in order to better define its methods behavior.

BUG=242107
==========

[Mikhail, 2015-12-18 14:37:37]

mikhail.pozdnyakov@intel.com changed reviewers:
+ asargent@chromium.org, jochen@chromium.org, rdevlin.cronin@chromium.org

[Mikhail, 2015-12-18 14:40:07]

Please take a look.

[Mikhail, 2015-12-30 16:44:40]

On 2015/12/18 14:40:07, Mikhail wrote:
> Please take a look.

[Devlin, 2016-01-04 18:19:13]

On 2015/12/30 16:44:40, Mikhail wrote:
> On 2015/12/18 14:40:07, Mikhail wrote:
> > Please take a look.

asargent@'s more familiar with the wifi display API, so leaving this to him.

[asargent_no_longer_on_chrome, 2016-01-04 23:28:13]

asargent@chromium.org changed reviewers:
+ rockot@chromium.org

[asargent_no_longer_on_chrome, 2016-01-04 23:28:14]

+cc rockot

I found a couple of minor issues, but I have a larger overall question: this is
the first API implementation I've reviewed that uses mojo services instead of
our standard extension function IPC infrastructure. Did you discuss implementing
it this way with Ken and/or Devlin, or just pick that way on your own? I'm not
sure what the "state of readiness" is considered to be for using mojo services
to implement extension APIs. I see that we've made some limited use of them here
and there, but wasn't sure if we're at a point where it's considered a preferred
approach for new APIs. (Hopefully Ken can comment on this)

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
File extensions/browser/api/display_source/display_source_connection_delegate.h
(right):

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
extensions/browser/api/display_source/display_source_connection_delegate.h:22:
// API.
nit: while you're in here, can these last two lines of the comment be merged
into one line?

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
File
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.cc
(right):

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.cc:30:
binding_.set_connection_error_handler([this]() {
The C++11 in chromium style guide says "Do not bind or store lambdas; use
base::Bind and base::Callback instead, because they offer protection against a
large class of object lifetime mistakes." - given that
set_connection_error_handler does store the lambda you're passing in, my reading
is that this usage of lambdas is not allowed, and you need to switch to using
base::Bind with a separate method on the class. 

Also, there's a comment on StrongBinding::set_connection_error_handler that says
"Note: The error handler must not delete the interface implementation.", which
you seem to be violating.

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.cc:35:
if (auto connection = delegate_->connection()) {
nit: Having variable declaration/assignment inside a conditional expression is
pretty unusual in chrome code in my experience - I'm not sure if it's actually
banned by the style guide, but I typically prefer to separate it like the
following: 

auto connection = delegate_->connection();
if (connection) {
  ...
}

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.cc:41:
delegate_->RemoveObserver(this);
optional: consider using ScopedObserver

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/mojo...
File extensions/browser/mojo/service_registration.cc (right):

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/mojo...
extensions/browser/mojo/service_registration.cc:51: 
did you intend to add this blank line?

[Ken Rockot(use gerrit already), 2016-01-05 17:21:37]

Since there's already a service registry in place for extensions-related
services, with a good pattern established for checking permissions at
service-binding-time, I think this is just fine conceptually.

Some inline comments below.

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
File
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.cc
(right):

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.cc:30:
binding_.set_connection_error_handler([this]() {
On 2016/01/04 at 23:28:14, Antony Sargent wrote:
> The C++11 in chromium style guide says "Do not bind or store lambdas; use
base::Bind and base::Callback instead, because they offer protection against a
large class of object lifetime mistakes." - given that
set_connection_error_handler does store the lambda you're passing in, my reading
is that this usage of lambdas is not allowed, and you need to switch to using
base::Bind with a separate method on the class. 
> 

There is some precedent for doing this with mojo APIs: specifically in cases
where |this| is guaranteed to live as long as the lambda (because it indirectly
owns the lambda), it's safe. You're right though, it is technically against the
style guide recommendations...

> Also, there's a comment on StrongBinding::set_connection_error_handler that
says "Note: The error handler must not delete the interface implementation.",
which you seem to be violating.

And this is true, and makes the above part irrelevant :)  This will in fact
always double-free AFAICT, because as soon as you delete this, the StrongBinding
will run its own error handler which also deletes this.

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.cc:46:
void WiFiDisplaySessionServiceImpl::BindToRequest(
Please annotate as "// static" on the above line

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.cc:59:
client_ = std::move(client);
You probably want to set a connection error handler on client_ to detect a
broken pipe

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.cc:97:
// Render should have been notified already.
nit: "Renderer"?

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
File
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.h
(right):

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.h:20:
class WiFiDisplaySessionServiceImpl
Please document this class

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.h:59:
};
nit: DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/1540563002/diff/20001/extensions/renderer/api...
File extensions/renderer/api/display_source/wifi_display/wifi_display_session.cc
(right):

https://codereview.chromium.org/1540563002/diff/20001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_session.cc:20:
CHECK(params_.render_frame);
In general these CHECKs should probably just be DCHECKs

https://codereview.chromium.org/1540563002/diff/20001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_session.cc:28:
service_->SetClient(std::move(client_ptr));
You probably want to set a connection error handler on service_ and binding_ to
detect a broken pipe

https://codereview.chromium.org/1540563002/diff/20001/extensions/renderer/api...
File extensions/renderer/api/display_source/wifi_display/wifi_display_session.h
(right):

https://codereview.chromium.org/1540563002/diff/20001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_session.h:42:
};
nit: DISALLOW_COPY_AND_ASSIGN

[Ken Rockot(use gerrit already), 2016-01-05 17:23:54]

On 2016/01/05 at 17:21:37, Ken Rockot wrote:
> Since there's already a service registry in place for extensions-related
services, with a good pattern established for checking permissions at
service-binding-time, I think this is just fine conceptually.
> 
> Some inline comments below.
> 
>
https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
> File
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.cc
(right):
> 
>
https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
>
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.cc:30:
binding_.set_connection_error_handler([this]() {
> On 2016/01/04 at 23:28:14, Antony Sargent wrote:
> > The C++11 in chromium style guide says "Do not bind or store lambdas; use
base::Bind and base::Callback instead, because they offer protection against a
large class of object lifetime mistakes." - given that
set_connection_error_handler does store the lambda you're passing in, my reading
is that this usage of lambdas is not allowed, and you need to switch to using
base::Bind with a separate method on the class. 
> > 
> 
> There is some precedent for doing this with mojo APIs: specifically in cases
where |this| is guaranteed to live as long as the lambda (because it indirectly
owns the lambda), it's safe. You're right though, it is technically against the
style guide recommendations...
> 
> > Also, there's a comment on StrongBinding::set_connection_error_handler that
says "Note: The error handler must not delete the interface implementation.",
which you seem to be violating.
> 
> And this is true, and makes the above part irrelevant :)  This will in fact
always double-free AFAICT, because as soon as you delete this, the StrongBinding
will run its own error handler which also deletes this.

Actually a minor correction: it won't *always* double free (which is probably
why it doesn't crash when testing locally), but it will definitely double-free
in the event of an actual pipe error occurring. So in any case the point
remains: please remove this deletion.

> 
>
https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
>
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.cc:46:
void WiFiDisplaySessionServiceImpl::BindToRequest(
> Please annotate as "// static" on the above line
> 
>
https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
>
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.cc:59:
client_ = std::move(client);
> You probably want to set a connection error handler on client_ to detect a
broken pipe
> 
>
https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
>
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.cc:97:
// Render should have been notified already.
> nit: "Renderer"?
> 
>
https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
> File
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.h
(right):
> 
>
https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
>
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.h:20:
class WiFiDisplaySessionServiceImpl
> Please document this class
> 
>
https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
>
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.h:59:
};
> nit: DISALLOW_COPY_AND_ASSIGN
> 
>
https://codereview.chromium.org/1540563002/diff/20001/extensions/renderer/api...
> File
extensions/renderer/api/display_source/wifi_display/wifi_display_session.cc
(right):
> 
>
https://codereview.chromium.org/1540563002/diff/20001/extensions/renderer/api...
>
extensions/renderer/api/display_source/wifi_display/wifi_display_session.cc:20:
CHECK(params_.render_frame);
> In general these CHECKs should probably just be DCHECKs
> 
>
https://codereview.chromium.org/1540563002/diff/20001/extensions/renderer/api...
>
extensions/renderer/api/display_source/wifi_display/wifi_display_session.cc:28:
service_->SetClient(std::move(client_ptr));
> You probably want to set a connection error handler on service_ and binding_
to detect a broken pipe
> 
>
https://codereview.chromium.org/1540563002/diff/20001/extensions/renderer/api...
> File
extensions/renderer/api/display_source/wifi_display/wifi_display_session.h
(right):
> 
>
https://codereview.chromium.org/1540563002/diff/20001/extensions/renderer/api...
> extensions/renderer/api/display_source/wifi_display/wifi_display_session.h:42:
};
> nit: DISALLOW_COPY_AND_ASSIGN

[Mikhail, 2016-01-11 19:39:41]

Thanks for your comments!

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
File extensions/browser/api/display_source/display_source_connection_delegate.h
(right):

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
extensions/browser/api/display_source/display_source_connection_delegate.h:22:
// API.
On 2016/01/04 23:28:14, Antony Sargent wrote:
> nit: while you're in here, can these last two lines of the comment be merged
> into one line?
sure :-)

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
File
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.cc
(right):

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.cc:30:
binding_.set_connection_error_handler([this]() {
On 2016/01/04 23:28:14, Antony Sargent wrote:
> The C++11 in chromium style guide says "Do not bind or store lambdas; use
> base::Bind and base::Callback instead, because they offer protection against a
> large class of object lifetime mistakes." - given that
> set_connection_error_handler does store the lambda you're passing in, my
reading
> is that this usage of lambdas is not allowed, and you need to switch to using
> base::Bind with a separate method on the class. 
> 
> Also, there's a comment on StrongBinding::set_connection_error_handler that
says
> "Note: The error handler must not delete the interface implementation.", which
> you seem to be violating. 
> 
Right, thanks for catching this!

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.cc:35:
if (auto connection = delegate_->connection()) {
On 2016/01/04 23:28:14, Antony Sargent wrote:
> nit: Having variable declaration/assignment inside a conditional expression is
> pretty unusual in chrome code in my experience - I'm not sure if it's actually
> banned by the style guide, but I typically prefer to separate it like the
> following: 
> 
> auto connection = delegate_->connection();
> if (connection) {
>   ...
> }

Done.

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.cc:41:
delegate_->RemoveObserver(this);
On 2016/01/04 23:28:14, Antony Sargent wrote:
> optional: consider using ScopedObserver
I prefer the existing symmetric calls in ctor/dtor, these are pretty simple..

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.cc:46:
void WiFiDisplaySessionServiceImpl::BindToRequest(
On 2016/01/05 17:21:36, Ken Rockot wrote:
> Please annotate as "// static" on the above line

Done.

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.cc:59:
client_ = std::move(client);
On 2016/01/05 17:21:37, Ken Rockot wrote:
> You probably want to set a connection error handler on client_ to detect a
> broken pipe

Done.

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.cc:97:
// Render should have been notified already.
On 2016/01/05 17:21:37, Ken Rockot wrote:
> nit: "Renderer"?

Done.

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
File
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.h
(right):

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.h:20:
class WiFiDisplaySessionServiceImpl
On 2016/01/05 17:21:37, Ken Rockot wrote:
> Please document this class

Done.

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/api/...
extensions/browser/api/display_source/wifi_display/wifi_display_session_service_impl.h:59:
};
On 2016/01/05 17:21:37, Ken Rockot wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/mojo...
File extensions/browser/mojo/service_registration.cc (right):

https://codereview.chromium.org/1540563002/diff/20001/extensions/browser/mojo...
extensions/browser/mojo/service_registration.cc:51: 
On 2016/01/04 23:28:14, Antony Sargent wrote:
> did you intend to add this blank line?
removed.

https://codereview.chromium.org/1540563002/diff/20001/extensions/renderer/api...
File extensions/renderer/api/display_source/wifi_display/wifi_display_session.cc
(right):

https://codereview.chromium.org/1540563002/diff/20001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_session.cc:20:
CHECK(params_.render_frame);
On 2016/01/05 17:21:37, Ken Rockot wrote:
> In general these CHECKs should probably just be DCHECKs

Done.

https://codereview.chromium.org/1540563002/diff/20001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_session.cc:28:
service_->SetClient(std::move(client_ptr));
On 2016/01/05 17:21:37, Ken Rockot wrote:
> You probably want to set a connection error handler on service_ and binding_
to
> detect a broken pipe

Done.

https://codereview.chromium.org/1540563002/diff/20001/extensions/renderer/api...
File extensions/renderer/api/display_source/wifi_display/wifi_display_session.h
(right):

https://codereview.chromium.org/1540563002/diff/20001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_session.h:42:
};
On 2016/01/05 17:21:37, Ken Rockot wrote:
> nit: DISALLOW_COPY_AND_ASSIGN

Done.

[Mikhail, 2016-01-13 19:52:46]

gentle ping for review

[Ken Rockot(use gerrit already), 2016-01-13 22:21:29]

lgtm

[asargent_no_longer_on_chrome, 2016-01-14 00:18:08]

nit: can you format the CL description to wrap at 80 (or even better, 72)
characters? 

otherwise, lgtm

[Mikhail, 2016-01-14 11:07:26]

@jochen, could you pls have a look at build/* changes?

[Mikhail, 2016-01-14 11:08:27]

Description was changed from

==========
Add WiFi Display session class skeleton and mojo service

This patch introduces a WiFi Display session class skeleton and mojo service
which provides network access for the render-hosted session.
The introduced code is compiled only if a newly added 'enable_wifi_display'
build option is set.

Besides, some minor changes were applied to the DisplaySourceConnectionDelegate
interface in order to better define its methods behavior.

BUG=242107
==========

to

==========
Add WiFi Display session class skeleton and mojo service

This patch introduces a WiFi Display session class
skeleton and mojo service which provides network
access for the render-hosted session.

The introduced code is compiled only if a newly added
'enable_wifi_display' build option is set.

Besides, some minor changes were applied to the
DisplaySourceConnectionDelegate interface in order
to better define its methods behavior.

BUG=242107
==========

[jochen (gone - plz use gerrit), 2016-01-14 13:58:08]

lgtm

[Mikhail, 2016-01-14 14:00:24]

The CQ bit was checked by mikhail.pozdnyakov@intel.com

[commit-bot: I haz the power, 2016-01-14 14:00:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1540563002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1540563002/40001

[commit-bot: I haz the power, 2016-01-14 15:24:15]

Description was changed from

==========
Add WiFi Display session class skeleton and mojo service

This patch introduces a WiFi Display session class
skeleton and mojo service which provides network
access for the render-hosted session.

The introduced code is compiled only if a newly added
'enable_wifi_display' build option is set.

Besides, some minor changes were applied to the
DisplaySourceConnectionDelegate interface in order
to better define its methods behavior.

BUG=242107
==========

to

==========
Add WiFi Display session class skeleton and mojo service

This patch introduces a WiFi Display session class
skeleton and mojo service which provides network
access for the render-hosted session.

The introduced code is compiled only if a newly added
'enable_wifi_display' build option is set.

Besides, some minor changes were applied to the
DisplaySourceConnectionDelegate interface in order
to better define its methods behavior.

BUG=242107
==========

[commit-bot: I haz the power, 2016-01-14 15:24:16]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-01-14 15:26:46]

Description was changed from

==========
Add WiFi Display session class skeleton and mojo service

This patch introduces a WiFi Display session class
skeleton and mojo service which provides network
access for the render-hosted session.

The introduced code is compiled only if a newly added
'enable_wifi_display' build option is set.

Besides, some minor changes were applied to the
DisplaySourceConnectionDelegate interface in order
to better define its methods behavior.

BUG=242107
==========

to

==========
Add WiFi Display session class skeleton and mojo service

This patch introduces a WiFi Display session class
skeleton and mojo service which provides network
access for the render-hosted session.

The introduced code is compiled only if a newly added
'enable_wifi_display' build option is set.

Besides, some minor changes were applied to the
DisplaySourceConnectionDelegate interface in order
to better define its methods behavior.

BUG=242107

Committed: https://crrev.com/61a28e373720d8f205d9fc37c09580804a36b038
Cr-Commit-Position: refs/heads/master@{#369415}
==========

[commit-bot: I haz the power, 2016-01-14 15:26:47]

Patchset 2 (id:??) landed as
https://crrev.com/61a28e373720d8f205d9fc37c09580804a36b038
Cr-Commit-Position: refs/heads/master@{#369415}