[chrome.displaySource] Use WDS for Wi-Fi Display implementation

[chrome.displaySource] Use WDS for Wi-Fi Display implementation

The WDS library implements a Wi-Fi Display dialect of RTSP
that includes the parser, actual negotiation logic for sink
and source, and the related data structures.

This patch contains initial implementation of WDS library
embedder interfaces.

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

[Mikhail, 2016-02-12 15:58:09]

Description was changed from

==========
[chrome.displaySource] Use WDS for Wi-Fi Display implementation

The WDS library implements a Wi-Fi Display dialect of RTSP that includes the
parser,
actual negotiation logic for sink and source, and the related data structures.

This patch contains initial implementation of WDS library embedder interfaces in
the
Wi-Fi Display implementation of 'chrome.displaySource' API.

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

to

==========
[chrome.displaySource] Use WDS for Wi-Fi Display implementation

The WDS library implements a Wi-Fi Display dialect of RTSP
that includes the parser, actual negotiation logic for sink
and source, and the related data structures.

This patch contains initial implementation of WDS library
embedder interfaces.

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

[Mikhail, 2016-02-12 15:58:09]

mikhail.pozdnyakov@intel.com changed reviewers:
+ alexander.shalamov@intel.com, asargent@chromium.org

[Mikhail, 2016-02-12 15:59:27]

PTAL

[shalamov, 2016-02-15 10:29:02]

https://codereview.chromium.org/1698473004/diff/1/extensions/renderer/api/dis...
File extensions/renderer/api/display_source/wifi_display/wifi_display_session.cc
(right):

https://codereview.chromium.org/1698473004/diff/1/extensions/renderer/api/dis...
extensions/renderer/api/display_source/wifi_display/wifi_display_session.cc:119:
static int sCseq = 0;

Make cseq a member.

https://codereview.chromium.org/1698473004/diff/1/extensions/renderer/api/dis...
extensions/renderer/api/display_source/wifi_display/wifi_display_session.cc:128:
static uint sTimerId = 0;

Make timerId as a class member.

[Mikhail, 2016-02-15 12:54:29]

Patchset #2 (id:20001) has been deleted

[shalamov, 2016-02-15 12:55:28]

lgtm

[Mikhail, 2016-02-16 13:38:44]

Antony, could you have a look? thanks

[asargent_no_longer_on_chrome, 2016-02-18 07:47:27]

Sorry for the delay in responding. 

I'm not sure if we want the top-level extensions/ module to take a static
dependency on the WDS library; we might want to push this into
chrome/{browser|common}/extensions instead. Remind me, is the intention that
this API will work on all chrome platforms where extensions run, or just on
ChromeOS?

[Mikhail, 2016-02-18 08:39:18]

On 2016/02/18 07:47:27, Antony Sargent wrote:
> Sorry for the delay in responding. 
> 
> I'm not sure if we want the top-level extensions/ module to take a static
> dependency on the WDS library; we might want to push this into
> chrome/{browser|common}/extensions instead. Remind me, is the intention that
> this API will work on all chrome platforms where extensions run, or just on
> ChromeOS?

Implementation on ChromeOS is a primary target at the moment, however all the
code (except networking, encapsulated within 'DisplaySourceConnectionDelegate'
interface implementation) is platform-independent so we're considering enabling
WiFi Display functionality (via chrome.displaySource API) on other platform as
well.

[Mikhail, 2016-02-19 18:35:21]

On 2016/02/18 08:39:18, Mikhail wrote:
> On 2016/02/18 07:47:27, Antony Sargent wrote:
> > Sorry for the delay in responding. 
> > 
> > I'm not sure if we want the top-level extensions/ module to take a static
> > dependency on the WDS library; we might want to push this into
> > chrome/{browser|common}/extensions instead. Remind me, is the intention that
> > this API will work on all chrome platforms where extensions run, or just on
> > ChromeOS?
> 
> Implementation on ChromeOS is a primary target at the moment, however all the
> code (except networking, encapsulated within 'DisplaySourceConnectionDelegate'
> interface implementation) is platform-independent so we're considering
enabling
> WiFi Display functionality (via chrome.displaySource API) on other platform as
> well.

AFAIK chrome/{browser|common}/extensions is a better place for the APis tied to
the browser UI (like tab API), and 'displaySorce' does not have UI dependencies,
so think it's more natural to place it within 'src/extensions/' folder.

[Mikhail, 2016-02-24 12:54:02]

Gentle ping for review

[asargent_no_longer_on_chrome, 2016-02-25 18:13:41]

Sorry for delay - I was out of office travelling for a few days. 

How much does the WDS library contribute to the binary size of chrome?

I think the best plan would be to add a gyp/gn flag which controls whether the
API and library is compiled/linked, and have that default to on for chromeos and
off elsewhere until there's agreement that we want this API enabled for other
desktop platforms. Have you been in contact with the folks working on
chromecast? I imagine they'd have input on this.

[asargent_no_longer_on_chrome, 2016-02-25 22:09:39]

My apologies - I realized that I didn't look closely enough at the gyp files,
and I see that you're already doing the conditional compilation/linking that I
requested. 

I just noticed a few minor problems to fix.

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

https://codereview.chromium.org/1698473004/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_session.cc:25:
LOG(ERROR) << "[WDS] " << buffer;
nit: the general policy is not to leave in logging statements that might be
useful in the future but aren't being actively used to help track down a
specific bug.  Instead the advice is to prefer things like DVLOG or VLOG which
allow selective enabling as needed, with DVLOG being best because it doesn't
bloat release binaries. 

http://dev.chromium.org/developers/coding-style#TOC-Logging

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

https://codereview.chromium.org/1698473004/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_session.h:24:
class WiFiDisplaySession: public DisplaySourceSession,
nit: this class could use a brief overview comment describing its overall role

https://codereview.chromium.org/1698473004/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_session.h:67:
mutable int cseq_;
nits: 
-"cseq" isn't a very descriptive name - at a minimum this needs a descriptive
comment describing what it is, and if it's an acronym it's probably better to
use the complete name as it's not a very commonly known acronym. 
-do you really need to use mutable here? It's use is allowed but somewhat
discouraged due to thread safety issues.

https://codereview.chromium.org/1698473004/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_session.h:68:
unsigned timer_id_;
nit: we usually prefer to use either int, size_t, or one of the specific integer
type definitions, and actively discourage using unsigned integers to represent
"integers that should be non-negative"; see
https://google.github.io/styleguide/cppguide.html#Integer_Types

[Mikhail, 2016-02-26 14:42:46]

Thanks for your comments. I've fixed the issues in the latest patch, besides it
also contains the necessary GN config files modifications which I had failed to
do before.

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

https://codereview.chromium.org/1698473004/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_session.h:24:
class WiFiDisplaySession: public DisplaySourceSession,
On 2016/02/25 22:09:39, Antony Sargent wrote:
> nit: this class could use a brief overview comment describing its overall role

Done.

https://codereview.chromium.org/1698473004/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_session.h:67:
mutable int cseq_;
On 2016/02/25 22:09:39, Antony Sargent wrote:
> nits: 
> -"cseq" isn't a very descriptive name - at a minimum this needs a descriptive
> comment describing what it is, and if it's an acronym it's probably better to
> use the complete name as it's not a very commonly known acronym. 
> -do you really need to use mutable here? It's use is allowed but somewhat
> discouraged due to thread safety issues. 
Added a description comment. This member is mutable because it is modified in a
const method.

https://codereview.chromium.org/1698473004/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_session.h:68:
unsigned timer_id_;
On 2016/02/25 22:09:39, Antony Sargent wrote:
> nit: we usually prefer to use either int, size_t, or one of the specific
integer
> type definitions, and actively discourage using unsigned integers to represent
> "integers that should be non-negative"; see
> https://google.github.io/styleguide/cppguide.html#Integer_Types
'unsigned' was used due to the inherited WDS interface, but I believe it should
be fixed in WDS (will switch interface to using 'int' type in the following WDS
version), thanks.

[asargent_no_longer_on_chrome, 2016-02-26 17:01:35]

lgtm

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

https://codereview.chromium.org/1698473004/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_session.h:67:
mutable int cseq_;
On 2016/02/26 14:42:46, Mikhail wrote:
> On 2016/02/25 22:09:39, Antony Sargent wrote:
> > nits: 
> > -"cseq" isn't a very descriptive name - at a minimum this needs a
descriptive
> > comment describing what it is, and if it's an acronym it's probably better
to
> > use the complete name as it's not a very commonly known acronym. 
> > -do you really need to use mutable here? It's use is allowed but somewhat
> > discouraged due to thread safety issues. 
> Added a description comment. This member is mutable because it is modified in
a
> const method.

To be clear, the point I was trying to make is as follows: Does the modification
of cseq change the externally visible state of the object? If so the preferred
solution is to use non-const ref/ptr wherever you need to call a method that
results in cseq changing, and make that method non-const. I'd however cseq is
something like a cache that gets lazily populated on an otherwise const method,
then using mutable is appropriate.

[Mikhail, 2016-02-29 07:58:33]

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

[Mikhail, 2016-02-29 07:58:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
alexander.shalamov@intel.com
Link to the patchset: https://codereview.chromium.org/1698473004/#ps60001
(title: "Comments from Antony + rebase + GN build fix")

[commit-bot: I haz the power, 2016-02-29 07:58:41]

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

[Mikhail, 2016-02-29 08:09:16]

On 2016/02/26 17:01:35, Antony Sargent wrote:
> 
> To be clear, the point I was trying to make is as follows: Does the
modification
> of cseq change the externally visible state of the object? If so the preferred
> solution is to use non-const ref/ptr wherever you need to call a method that
> results in cseq changing, and make that method non-const. I'd however cseq is
> something like a cache that gets lazily populated on an otherwise const
method,
> then using mutable is appropriate.

This is exactly the second case: cseq is just lazily populated and it does not
really change the class state.

[commit-bot: I haz the power, 2016-02-29 08:59:40]

Description was changed from

==========
[chrome.displaySource] Use WDS for Wi-Fi Display implementation

The WDS library implements a Wi-Fi Display dialect of RTSP
that includes the parser, actual negotiation logic for sink
and source, and the related data structures.

This patch contains initial implementation of WDS library
embedder interfaces.

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

to

==========
[chrome.displaySource] Use WDS for Wi-Fi Display implementation

The WDS library implements a Wi-Fi Display dialect of RTSP
that includes the parser, actual negotiation logic for sink
and source, and the related data structures.

This patch contains initial implementation of WDS library
embedder interfaces.

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

[commit-bot: I haz the power, 2016-02-29 08:59:41]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-02-29 09:00:55]

Description was changed from

==========
[chrome.displaySource] Use WDS for Wi-Fi Display implementation

The WDS library implements a Wi-Fi Display dialect of RTSP
that includes the parser, actual negotiation logic for sink
and source, and the related data structures.

This patch contains initial implementation of WDS library
embedder interfaces.

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

to

==========
[chrome.displaySource] Use WDS for Wi-Fi Display implementation

The WDS library implements a Wi-Fi Display dialect of RTSP
that includes the parser, actual negotiation logic for sink
and source, and the related data structures.

This patch contains initial implementation of WDS library
embedder interfaces.

BUG=242107

Committed: https://crrev.com/da8ee699b156b3d8d82b16285039f7bf7d80b720
Cr-Commit-Position: refs/heads/master@{#378182}
==========

[commit-bot: I haz the power, 2016-02-29 09:00:56]

Patchset 3 (id:??) landed as
https://crrev.com/da8ee699b156b3d8d82b16285039f7bf7d80b720
Cr-Commit-Position: refs/heads/master@{#378182}