Implement some WiFi Display elementary stream descriptors.

[chrome.displaySource] Implement some elementary stream descriptors.

WiFi Display elementary stream descriptors are used for passing descriptive
information about elementary streams to a WiFi Display transport stream
packetizer which then packetizes that information so that it can be passed
to a remote sink.

This is part of a WiFi Display packetizer patch series:
 * https://codereview.chromium.org/1796123002/
   WiFi Display elementary stream packetizer
 * https://codereview.chromium.org/1800493003/  <-- this CL
   WiFi Display elementary stream descriptors
 * https://codereview.chromium.org/1797953002/
   WiFi Display transport stream packetizer
 * https://codereview.chromium.org/1796073003/
   WiFi Display media packetizer

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

[e_hakkinen, 2016-03-14 10:26:00]

Description was changed from

==========
Implement some WiFi Display elementary stream descriptors.
==========

to

==========
Implement some WiFi Display elementary stream descriptors.

This is part of a WiFi Display packetizer patch series:
 * https://codereview.chromium.org/1796123002/
   WiFi Display elementary stream packetizer
 * https://codereview.chromium.org/1800493003/
   WiFi Display elementary stream descriptors
 * https://codereview.chromium.org/1797953002/
   WiFi Display transport stream packetizer
 * https://codereview.chromium.org/1796073003/
   WiFi Display media packetizer
==========

[e_hakkinen, 2016-03-14 10:26:00]

eero.hakkinen@intel.com changed reviewers:
+ alexander.shalamov@intel.com, mikhail.pozdnyakov@intel.com

[e_hakkinen, 2016-03-14 11:18:47]

Description was changed from

==========
Implement some WiFi Display elementary stream descriptors.

This is part of a WiFi Display packetizer patch series:
 * https://codereview.chromium.org/1796123002/
   WiFi Display elementary stream packetizer
 * https://codereview.chromium.org/1800493003/
   WiFi Display elementary stream descriptors
 * https://codereview.chromium.org/1797953002/
   WiFi Display transport stream packetizer
 * https://codereview.chromium.org/1796073003/
   WiFi Display media packetizer
==========

to

==========
[chrome.displaySource] Implement some elementary stream descriptors.

WiFi Display elementary stream descriptors are used for passing descriptive
information about elementary streams to a WiFi Display transport stream
packetizer which then packetizes that information so that it can be passed
to a remote sink.

This is part of a WiFi Display packetizer patch series:
 * https://codereview.chromium.org/1796123002/
   WiFi Display elementary stream packetizer
 * https://codereview.chromium.org/1800493003/  <-- this CL
   WiFi Display elementary stream descriptors
 * https://codereview.chromium.org/1797953002/
   WiFi Display transport stream packetizer
 * https://codereview.chromium.org/1796073003/
   WiFi Display media packetizer

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

[e_hakkinen, 2016-03-14 14:25:09]

Patchset #1 (id:1) has been deleted

[e_hakkinen, 2016-03-14 14:46:22]

eero.hakkinen@intel.com changed reviewers:
+ asargent@chromium.org

[e_hakkinen, 2016-03-14 14:46:23]

PTAL.

Although I put this CL to be the second one in the series, this one does not
does not depend on the first one as the two are independent from each other.

[asargent_no_longer_on_chrome, 2016-03-16 19:56:23]

A few minor style nits. Also make sure to run "git cl lint" to make sure there
isn't anything I missed that the tool catches.

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

https://codereview.chromium.org/1800493003/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_elementary_stream_descriptor.h:32:
typedef uint8_t* pointer;
nit: I don't think these typedefs are helpful/needed. 

Aside: we're also supposed to be using aliases instead of typedefs according to
http://chromium-cpp.appspot.com/

https://codereview.chromium.org/1800493003/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_elementary_stream_descriptor.h:46:
const_iterator end() const { return data() + size(); }
nit: "Simple accessors should generally be the only inline functions" - consider
whether this one meets that bar (we also do make exceptions for some situations,
eg very performance critical code)

http://dev.chromium.org/developers/coding-style#TOC-Inline-functions
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...

(here and other occurrences below like the bit oriented functions in
WiFiDisplayElementaryStreamDescriptor::LPCMAudioStream)

https://codereview.chromium.org/1800493003/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_elementary_stream_descriptor.h:80:
class WiFiDisplayElementaryStreamDescriptor::LPCMAudioStream
nit: needs class description

https://google.github.io/styleguide/cppguide.html#Class_Comments

https://codereview.chromium.org/1800493003/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_elementary_stream_descriptor.h:132:
static const Bits kByte1NumberOfChannelsBits;
nit: it could improve readability to sort these 4 lines

https://codereview.chromium.org/1800493003/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_elementary_stream_descriptor.h:137:
// the inherited data.
Can you add to this comment an brief explanation of why we need to have this
restriction? It's not immediately obvious to me.

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

https://codereview.chromium.org/1800493003/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_elementary_stream_descriptor_unittest.cc:17:
T Copy(const T& t) {
nit: include a comment here about why you need this (I guess it's to test the
copy constructor below?)

https://codereview.chromium.org/1800493003/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_elementary_stream_descriptor_unittest.cc:74:
WiFiDisplayElementaryStreamDescriptor::LPCMAudioStream;
nit: the most common style I've seen, at least in extensions code, is to have
using declarations towards the top of the file, close to the includes. I'm not
sure if this is an actual style guide rule or not, but I guess the reasoning for
it is that it's nice to have them close to the includes so all the "foreign
stuff" is contained in one place.

[e_hakkinen, 2016-03-17 13:20:56]

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

https://codereview.chromium.org/1800493003/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_elementary_stream_descriptor.h:32:
typedef uint8_t* pointer;
On 2016/03/16 19:56:23, Antony Sargent wrote:
> nit: I don't think these typedefs are helpful/needed. 
> 
> Aside: we're also supposed to be using aliases instead of typedefs according
to
> http://chromium-cpp.appspot.com/

Unit tests (or Google Test to be more precise) need iterator typedefs.
I'll drop pointer typedef, add a comment and use aliases for iterator types.

https://codereview.chromium.org/1800493003/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_elementary_stream_descriptor.h:46:
const_iterator end() const { return data() + size(); }
On 2016/03/16 19:56:23, Antony Sargent wrote:
> nit: "Simple accessors should generally be the only inline functions" -
consider
> whether this one meets that bar (we also do make exceptions for some
situations,
> eg very performance critical code)
> 
> http://dev.chromium.org/developers/coding-style#TOC-Inline-functions
>
http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-in...
> 
> (here and other occurrences below like the bit oriented functions in
> WiFiDisplayElementaryStreamDescriptor::LPCMAudioStream)

Done for data(), for size() and for bit orientied functions in LPCMAudioStream.
The rest are really simple.

https://codereview.chromium.org/1800493003/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_elementary_stream_descriptor.h:80:
class WiFiDisplayElementaryStreamDescriptor::LPCMAudioStream
On 2016/03/16 19:56:23, Antony Sargent wrote:
> nit: needs class description
> 
> https://google.github.io/styleguide/cppguide.html#Class_Comments

Done.

https://codereview.chromium.org/1800493003/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_elementary_stream_descriptor.h:132:
static const Bits kByte1NumberOfChannelsBits;
On 2016/03/16 19:56:23, Antony Sargent wrote:
> nit: it could improve readability to sort these 4 lines

Aren't they already sorted?
Anyway, as I moved the definitions of the bit oriented functions to the .cc
file, there is no need for keeping these in a header file, any more. So I
removed them.

https://codereview.chromium.org/1800493003/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_elementary_stream_descriptor.h:137:
// the inherited data.
On 2016/03/16 19:56:23, Antony Sargent wrote:
> Can you add to this comment an brief explanation of why we need to have this
> restriction? It's not immediately obvious to me. 
> 

Done.

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

https://codereview.chromium.org/1800493003/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_elementary_stream_descriptor_unittest.cc:17:
T Copy(const T& t) {
On 2016/03/16 19:56:23, Antony Sargent wrote:
> nit: include a comment here about why you need this (I guess it's to test the
> copy constructor below?)

Done.

https://codereview.chromium.org/1800493003/diff/40001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_elementary_stream_descriptor_unittest.cc:74:
WiFiDisplayElementaryStreamDescriptor::LPCMAudioStream;
On 2016/03/16 19:56:23, Antony Sargent wrote:
> nit: the most common style I've seen, at least in extensions code, is to have
> using declarations towards the top of the file, close to the includes. I'm not
> sure if this is an actual style guide rule or not, but I guess the reasoning
for
> it is that it's nice to have them close to the includes so all the "foreign
> stuff" is contained in one place. 
>  

Done.

[e_hakkinen, 2016-03-17 13:25:43]

On 2016/03/16 19:56:23, Antony Sargent wrote:
> A few minor style nits. Also make sure to run "git cl lint" to make sure there
> isn't anything I missed that the tool catches.

I did that, too. The tool catches only false positives:
 * Move constructors are allowed even though they use rvalue references.
 * testing::tuple is not std::tuple so there is no need to include <tuple>.

[asargent_no_longer_on_chrome, 2016-03-18 18:24:48]

lgtm

https://codereview.chromium.org/1800493003/diff/60001/extensions/renderer/api...
File
extensions/renderer/api/display_source/wifi_display/wifi_display_elementary_stream_descriptor.h
(right):

https://codereview.chromium.org/1800493003/diff/60001/extensions/renderer/api...
extensions/renderer/api/display_source/wifi_display/wifi_display_elementary_stream_descriptor.h:29:
// Make Google Test to treat this class as a container.
grammar nit: "Make Google Test to treat" should be just "Make Google Test treat"

[e_hakkinen, 2016-03-21 04:35:38]

The CQ bit was checked by eero.hakkinen@intel.com

[e_hakkinen, 2016-03-21 04:35:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/1800493003/#ps100001
(title: "Grammar nits")

[commit-bot: I haz the power, 2016-03-21 04:35:46]

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

[commit-bot: I haz the power, 2016-03-21 07:59:56]

Description was changed from

==========
[chrome.displaySource] Implement some elementary stream descriptors.

WiFi Display elementary stream descriptors are used for passing descriptive
information about elementary streams to a WiFi Display transport stream
packetizer which then packetizes that information so that it can be passed
to a remote sink.

This is part of a WiFi Display packetizer patch series:
 * https://codereview.chromium.org/1796123002/
   WiFi Display elementary stream packetizer
 * https://codereview.chromium.org/1800493003/  <-- this CL
   WiFi Display elementary stream descriptors
 * https://codereview.chromium.org/1797953002/
   WiFi Display transport stream packetizer
 * https://codereview.chromium.org/1796073003/
   WiFi Display media packetizer

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

to

==========
[chrome.displaySource] Implement some elementary stream descriptors.

WiFi Display elementary stream descriptors are used for passing descriptive
information about elementary streams to a WiFi Display transport stream
packetizer which then packetizes that information so that it can be passed
to a remote sink.

This is part of a WiFi Display packetizer patch series:
 * https://codereview.chromium.org/1796123002/
   WiFi Display elementary stream packetizer
 * https://codereview.chromium.org/1800493003/  <-- this CL
   WiFi Display elementary stream descriptors
 * https://codereview.chromium.org/1797953002/
   WiFi Display transport stream packetizer
 * https://codereview.chromium.org/1796073003/
   WiFi Display media packetizer

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

[commit-bot: I haz the power, 2016-03-21 07:59:57]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2016-03-21 08:01:12]

Description was changed from

==========
[chrome.displaySource] Implement some elementary stream descriptors.

WiFi Display elementary stream descriptors are used for passing descriptive
information about elementary streams to a WiFi Display transport stream
packetizer which then packetizes that information so that it can be passed
to a remote sink.

This is part of a WiFi Display packetizer patch series:
 * https://codereview.chromium.org/1796123002/
   WiFi Display elementary stream packetizer
 * https://codereview.chromium.org/1800493003/  <-- this CL
   WiFi Display elementary stream descriptors
 * https://codereview.chromium.org/1797953002/
   WiFi Display transport stream packetizer
 * https://codereview.chromium.org/1796073003/
   WiFi Display media packetizer

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

to

==========
[chrome.displaySource] Implement some elementary stream descriptors.

WiFi Display elementary stream descriptors are used for passing descriptive
information about elementary streams to a WiFi Display transport stream
packetizer which then packetizes that information so that it can be passed
to a remote sink.

This is part of a WiFi Display packetizer patch series:
 * https://codereview.chromium.org/1796123002/
   WiFi Display elementary stream packetizer
 * https://codereview.chromium.org/1800493003/  <-- this CL
   WiFi Display elementary stream descriptors
 * https://codereview.chromium.org/1797953002/
   WiFi Display transport stream packetizer
 * https://codereview.chromium.org/1796073003/
   WiFi Display media packetizer

BUG=242107

Committed: https://crrev.com/2c13e7ad077ec639b6ca0e76f0a1a530a398cb9d
Cr-Commit-Position: refs/heads/master@{#382245}
==========

[commit-bot: I haz the power, 2016-03-21 08:01:13]

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