Replace uses of raw uint32's with a type-checked RtpTimeTicks data type.

Replace uses of raw uint32's with a type-checked RtpTimeTicks data type.

Introduces RtpTimeDelta and RtpTimeTicks to media::cast, modeled after
base::TimeDelta and base::TimeTicks, that track RTP time and provide a
strict set of valid math and comparison operators, bit truncation and
re-expansion, and timebase conversion.

All instances of raw uint32 used to represent RTP timestamps has been
replaced with the new data type.

BUG=530839
NOPRESUBMIT=true

Committed: https://crrev.com/c938d4441a3fe1c3851858fba68e017aef424f86
Cr-Commit-Position: refs/heads/master@{#367106}

[miu, 2015-12-09 02:21:02]

Patchset #1 (id:1) has been deleted

[miu, 2015-12-09 02:22:41]

miu@chromium.org changed reviewers:
+ isheriff@chromium.org

[miu, 2015-12-09 02:22:42]

irfan: PTAL.  Sorry for the huge CL, but it's mostly search-and-replace, so
should be quick to get through.  Please start with the new files in
media/cast/common so that the rest makes sense.

[Irfan, 2015-12-09 21:24:28]

https://codereview.chromium.org/1515433002/diff/20001/chrome/common/cast_mess...
File chrome/common/cast_messages.h (right):

https://codereview.chromium.org/1515433002/diff/20001/chrome/common/cast_mess...
chrome/common/cast_messages.h:6: // Multiply-included message file, hence no
include guard.
Perhaps remove this comment given it does not apply for everything.

For my understanding, why is this done ?

https://codereview.chromium.org/1515433002/diff/20001/chrome/common/cast_mess...
chrome/common/cast_messages.h:20: struct
IPC::ParamTraits<media::cast::RtpTimeTicks> {
I wonder if this belongs to its own header ?

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/expan...
File media/cast/common/expanded_value_base.h (right):

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/expan...
media/cast/common/expanded_value_base.h:17: // RtpTimeTicks, FrameId, or
PacketId which generally increment/decrement in
Suggest expanding further here with some examples if possible (and explaining
the design limitation)

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/expan...
media/cast/common/expanded_value_base.h:21: // FullPrecisionInterger should be a
signed integer POD type that is of
Typo: FullPrecisionInteger

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/expan...
media/cast/common/expanded_value_base.h:23: // the the class inheriting the
common functionality provided in this template,
s/the the/the

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/expan...
media/cast/common/expanded_value_base.h:28:
static_assert(std::numeric_limits<FullPrecisionInteger>::is_signed,
also do an integer check like below ?

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/expan...
media/cast/common/expanded_value_base.h:32: bool is_zero() const { return value_
== 0; }
we need this ?

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/expan...
media/cast/common/expanded_value_base.h:68: const FullPrecisionInteger pivot =
short_max / 2;
pivot = max_distance_for_expansion ?

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/expan...
media/cast/common/expanded_value_base.h:102: static FullPrecisionInteger
max_distance_for_expansion() {
private ?

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/expan...
File media/cast/common/expanded_value_base_unittest.cc (right):

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/expan...
media/cast/common/expanded_value_base_unittest.cc:70: const TestValue
original_value(bias + i - 256);
am a little confused by the need for -256. perhaps explain here.

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/rtp_t...
File media/cast/common/rtp_time.cc (right):

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/rtp_t...
media/cast/common/rtp_time.cc:20: 0.5 /* rounding */;
nit: alignment ?

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/rtp_t...
File media/cast/common/rtp_time.h (right):

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/rtp_t...
media/cast/common/rtp_time.h:67: // Multiplicative operators (with other
deltas).
Perhaps group all operators with deltas together ?

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/rtp_t...
media/cast/common/rtp_time.h:135: void OutputToLog(std::ostream& out) const;
Do we need this public method given << ?

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/rtp_t...
File media/cast/common/rtp_time_unittest.cc (right):

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/rtp_t...
media/cast/common/rtp_time_unittest.cc:14: TEST(RtpTimeDeltaTest,
ConversionToAndFromTimeDelta) {
perhaps cover some operator tests ?

https://codereview.chromium.org/1515433002/diff/20001/media/cast/logging/enco...
File media/cast/logging/encoding_event_subscriber_unittest.cc (right):

https://codereview.chromium.org/1515433002/diff/20001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber_unittest.cc:306: RtpTimeTicks
rtp_timestamp1 = RtpTimeTicks().Expand(UINT32_C(100));
Should we add an explicit constructor so we can do RtpTimeTicks(100u) instead of
RtpTimeTicks().Expand(UINT32_C(100)) here and else where ?

https://codereview.chromium.org/1515433002/diff/20001/media/cast/logging/simp...
File media/cast/logging/simple_event_subscriber_unittest.cc (right):

https://codereview.chromium.org/1515433002/diff/20001/media/cast/logging/simp...
media/cast/logging/simple_event_subscriber_unittest.cc:47:
encode_event->rtp_timestamp = RtpTimeTicks().Expand(UINT32_C(100));
Would Expand(100u) work here and elsewhere ?

https://codereview.chromium.org/1515433002/diff/20001/media/cast/net/rtcp/rtcp.h
File media/cast/net/rtcp/rtcp.h (right):

https://codereview.chromium.org/1515433002/diff/20001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp.h:141: RtcpParser parser_;
probably add a comment here that parser now remembers state that helps with
future parsing

https://codereview.chromium.org/1515433002/diff/20001/media/cast/net/rtcp/rtc...
File media/cast/net/rtcp/rtcp_builder_unittest.cc (right):

https://codereview.chromium.org/1515433002/diff/20001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_builder_unittest.cc:225: packet_event.rtp_timestamp =
RtpTimeTicks().Expand(kRtpTimestamp);
consider making kRtpTimestamp a RtpTimeTicks value and avoid the expansion code
in a number of places ?

[miu, 2015-12-10 00:38:37]

Thanks for the very quick turnaround!  :)

PTAL.

https://codereview.chromium.org/1515433002/diff/20001/chrome/common/cast_mess...
File chrome/common/cast_messages.h (right):

https://codereview.chromium.org/1515433002/diff/20001/chrome/common/cast_mess...
chrome/common/cast_messages.h:6: // Multiply-included message file, hence no
include guard.
On 2015/12/09 21:24:27, Irfan wrote:
> Perhaps remove this comment given it does not apply for everything.
> 
> For my understanding, why is this done ?

Done.  I've kept the comment I added on line 47 that states this fact.

Throughout Chromium, the IPC infrastructure is mostly code-generated by putting
all the messaging definitions in a header file like this one.  There's a file
somewhere that includes this file and is written like so:

  #define IPC_MACRO_1 <something for generating struct declarations>
  #define IPC_MACRO_2 <something for generating struct declarations>
  ...
  #include "cast_messages.h"
  #undef IPC_MACRO_1
  #undef IPC_MACRO_2
  ...
  #define IPC_MACRO_1 <something for generating struct method impls>
  #define IPC_MACRO_2 <something for generating struct method impls>
  ...
  #include "cast_messages.h"

So, basically, the file that includes this file changes the macro definitions
and includes cast_messages.h again to get different code generated.

https://codereview.chromium.org/1515433002/diff/20001/chrome/common/cast_mess...
chrome/common/cast_messages.h:20: struct
IPC::ParamTraits<media::cast::RtpTimeTicks> {
On 2015/12/09 21:24:27, Irfan wrote:
> I wonder if this belongs to its own header ?

I looked at a number of examples elsewhere, and this is how they did it.

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/expan...
File media/cast/common/expanded_value_base.h (right):

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/expan...
media/cast/common/expanded_value_base.h:17: // RtpTimeTicks, FrameId, or
PacketId which generally increment/decrement in
On 2015/12/09 21:24:27, Irfan wrote:
> Suggest expanding further here with some examples if possible (and explaining
> the design limitation)

Done.  Elaborated on where to find usage info.  Also, added examples to
rtp_time.h, since a concrete impl is needed.

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/expan...
media/cast/common/expanded_value_base.h:21: // FullPrecisionInterger should be a
signed integer POD type that is of
On 2015/12/09 21:24:27, Irfan wrote:
> Typo: FullPrecisionInteger

Done.

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/expan...
media/cast/common/expanded_value_base.h:23: // the the class inheriting the
common functionality provided in this template,
On 2015/12/09 21:24:27, Irfan wrote:
> s/the the/the

Done.

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/expan...
media/cast/common/expanded_value_base.h:28:
static_assert(std::numeric_limits<FullPrecisionInteger>::is_signed,
On 2015/12/09 21:24:27, Irfan wrote:
> also do an integer check like below ?

Done.

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/expan...
media/cast/common/expanded_value_base.h:32: bool is_zero() const { return value_
== 0; }
On 2015/12/09 21:24:27, Irfan wrote:
> we need this ?

Nope.  Removed.

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/expan...
media/cast/common/expanded_value_base.h:68: const FullPrecisionInteger pivot =
short_max / 2;
On 2015/12/09 21:24:27, Irfan wrote:
> pivot = max_distance_for_expansion ?

Done.

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/expan...
media/cast/common/expanded_value_base.h:102: static FullPrecisionInteger
max_distance_for_expansion() {
On 2015/12/09 21:24:27, Irfan wrote:
> private ?

In a follow-up CL (where I introduce FrameId), I want to use this to replace
kMaxUnackedFrames (here:
https://code.google.com/p/chromium/codesearch#chromium/src/media/cast/constan...).

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/expan...
File media/cast/common/expanded_value_base_unittest.cc (right):

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/expan...
media/cast/common/expanded_value_base_unittest.cc:70: const TestValue
original_value(bias + i - 256);
On 2015/12/09 21:24:27, Irfan wrote:
> am a little confused by the need for -256. perhaps explain here.

I clarified with comments and a little extra code.  I think calling out
"wrap-around" in the comments made things confusing.  The logic is all about
finding the closest value to |reference| whose lower 8-bits match |truncated|.

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/rtp_t...
File media/cast/common/rtp_time.cc (right):

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/rtp_t...
media/cast/common/rtp_time.cc:20: 0.5 /* rounding */;
On 2015/12/09 21:24:28, Irfan wrote:
> nit: alignment ?

This is what `git cl format media` produced.  ;-)

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/rtp_t...
File media/cast/common/rtp_time.h (right):

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/rtp_t...
media/cast/common/rtp_time.h:67: // Multiplicative operators (with other
deltas).
On 2015/12/09 21:24:28, Irfan wrote:
> Perhaps group all operators with deltas together ?

Done.

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/rtp_t...
media/cast/common/rtp_time.h:135: void OutputToLog(std::ostream& out) const;
On 2015/12/09 21:24:28, Irfan wrote:
> Do we need this public method given << ?

Done.  Removed method, and friended the operator<<() functions.  Had to move
order-of-declarations around a little to make it all work.

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/rtp_t...
File media/cast/common/rtp_time_unittest.cc (right):

https://codereview.chromium.org/1515433002/diff/20001/media/cast/common/rtp_t...
media/cast/common/rtp_time_unittest.cc:14: TEST(RtpTimeDeltaTest,
ConversionToAndFromTimeDelta) {
On 2015/12/09 21:24:28, Irfan wrote:
> perhaps cover some operator tests ?

I thought about this, but the tests would just be confirming integer arithmetic.
 ;-)  If I don't have any typos in any of the one-liner operator methods, we
should be fine.

https://codereview.chromium.org/1515433002/diff/20001/media/cast/logging/enco...
File media/cast/logging/encoding_event_subscriber_unittest.cc (right):

https://codereview.chromium.org/1515433002/diff/20001/media/cast/logging/enco...
media/cast/logging/encoding_event_subscriber_unittest.cc:306: RtpTimeTicks
rtp_timestamp1 = RtpTimeTicks().Expand(UINT32_C(100));
On 2015/12/09 21:24:28, Irfan wrote:
> Should we add an explicit constructor so we can do RtpTimeTicks(100u) instead
of
> RtpTimeTicks().Expand(UINT32_C(100)) here and else where ?

No.  I explicitly want to avoid this since it would lead to people writing code
that does a bunch of time math and THEN construct the RtpTimeTicks.  This is
modeled after the way base::TimeTicks works (for the same reason).

In the test code, it looks a little ugly to create RtpTimeTicks values, but
that's okay given it would also look ugly in non-test code; and a code reviewer
would then be able to easily detect and point out that non-test code shouldn't
be doing this.

https://codereview.chromium.org/1515433002/diff/20001/media/cast/logging/simp...
File media/cast/logging/simple_event_subscriber_unittest.cc (right):

https://codereview.chromium.org/1515433002/diff/20001/media/cast/logging/simp...
media/cast/logging/simple_event_subscriber_unittest.cc:47:
encode_event->rtp_timestamp = RtpTimeTicks().Expand(UINT32_C(100));
On 2015/12/09 21:24:28, Irfan wrote:
> Would Expand(100u) work here and elsewhere ?

Yes, but the interpretation of 100u would be different when using different
compilers/platforms (i.e., is it a 32-bit unsigned int, or a 64-bit one?). 
IIRC, there was a past discussion on chromium-dev@ encouraging us to use the
cross-platform macros (e.g., UINT32_C() and friends) rather than 100u.

https://codereview.chromium.org/1515433002/diff/20001/media/cast/net/rtcp/rtcp.h
File media/cast/net/rtcp/rtcp.h (right):

https://codereview.chromium.org/1515433002/diff/20001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp.h:141: RtcpParser parser_;
On 2015/12/09 21:24:28, Irfan wrote:
> probably add a comment here that parser now remembers state that helps with
> future parsing

Done.

https://codereview.chromium.org/1515433002/diff/20001/media/cast/net/rtcp/rtc...
File media/cast/net/rtcp/rtcp_builder_unittest.cc (right):

https://codereview.chromium.org/1515433002/diff/20001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_builder_unittest.cc:225: packet_event.rtp_timestamp =
RtpTimeTicks().Expand(kRtpTimestamp);
On 2015/12/09 21:24:28, Irfan wrote:
> consider making kRtpTimestamp a RtpTimeTicks value and avoid the expansion
code
> in a number of places ?

Done.

[Irfan, 2015-12-10 01:40:38]

lgtm

https://codereview.chromium.org/1515433002/diff/20001/chrome/common/cast_mess...
File chrome/common/cast_messages.h (right):

https://codereview.chromium.org/1515433002/diff/20001/chrome/common/cast_mess...
chrome/common/cast_messages.h:6: // Multiply-included message file, hence no
include guard.
On 2015/12/10 00:38:36, miu wrote:
> On 2015/12/09 21:24:27, Irfan wrote:
> > Perhaps remove this comment given it does not apply for everything.
> > 
> > For my understanding, why is this done ?
> 
> Done.  I've kept the comment I added on line 47 that states this fact.
> 
> Throughout Chromium, the IPC infrastructure is mostly code-generated by
putting
> all the messaging definitions in a header file like this one.  There's a file
> somewhere that includes this file and is written like so:
> 
>   #define IPC_MACRO_1 <something for generating struct declarations>
>   #define IPC_MACRO_2 <something for generating struct declarations>
>   ...
>   #include "cast_messages.h"
>   #undef IPC_MACRO_1
>   #undef IPC_MACRO_2
>   ...
>   #define IPC_MACRO_1 <something for generating struct method impls>
>   #define IPC_MACRO_2 <something for generating struct method impls>
>   ...
>   #include "cast_messages.h"
> 
> So, basically, the file that includes this file changes the macro definitions
> and includes cast_messages.h again to get different code generated.

Thanks for explaining

https://codereview.chromium.org/1515433002/diff/80001/media/cast/common/expan...
File media/cast/common/expanded_value_base_unittest.cc (right):

https://codereview.chromium.org/1515433002/diff/80001/media/cast/common/expan...
media/cast/common/expanded_value_base_unittest.cc:94: // |reference| than -192.
Really good explanation, thanks!

[miu, 2015-12-10 01:59:59]

miu@chromium.org changed reviewers:
+ hubbe@chromium.org, mkwst@chromium.org

[miu, 2015-12-10 01:59:59]

hubbe: Need "full committer" lgtm stamp.

mkwst: Need IPC security review of chrome/common/cast_messages.h.

[Mike West, 2015-12-10 08:54:46]

https://codereview.chromium.org/1515433002/diff/100001/chrome/common/cast_mes...
File chrome/common/cast_messages.h (right):

https://codereview.chromium.org/1515433002/diff/100001/chrome/common/cast_mes...
chrome/common/cast_messages.h:21: struct ParamTraits<media::cast::RtpTimeTicks>
{
Please don't inline the definitions of these methods. Sticking them into a new
`case_messages.cc` would be preferable.

https://codereview.chromium.org/1515433002/diff/100001/chrome/common/cast_mes...
chrome/common/cast_messages.h:25: auto serialized = p.SerializeForIPC();
Why `auto` and not `uint64_t`? I'd prefer clarity. Being explicit about the type
also allows you to avoid the strange construction on line #31.

https://codereview.chromium.org/1515433002/diff/100001/media/cast/common/rtp_...
File media/cast/common/rtp_time.h (right):

https://codereview.chromium.org/1515433002/diff/100001/media/cast/common/rtp_...
media/cast/common/rtp_time.h:27: // The difference between two RtpTimeTickses. 
This data type is modeled off of
Nit: "Tickses" can't possibly be the right pluralization. :)

[miu, 2015-12-11 23:00:29]

mkwst: lgty now?

https://codereview.chromium.org/1515433002/diff/100001/chrome/common/cast_mes...
File chrome/common/cast_messages.h (right):

https://codereview.chromium.org/1515433002/diff/100001/chrome/common/cast_mes...
chrome/common/cast_messages.h:21: struct ParamTraits<media::cast::RtpTimeTicks>
{
On 2015/12/10 08:54:46, Mike West wrote:
> Please don't inline the definitions of these methods. Sticking them into a new
> `case_messages.cc` would be preferable.

Done.

https://codereview.chromium.org/1515433002/diff/100001/chrome/common/cast_mes...
chrome/common/cast_messages.h:25: auto serialized = p.SerializeForIPC();
On 2015/12/10 08:54:46, Mike West wrote:
> Why `auto` and not `uint64_t`? I'd prefer clarity. Being explicit about the
type
> also allows you to avoid the strange construction on line #31.

Done.  I was originally flip-flopping between representations while I was
authoring this change.  I'll just use uint64_t here.

https://codereview.chromium.org/1515433002/diff/100001/media/cast/common/rtp_...
File media/cast/common/rtp_time.h (right):

https://codereview.chromium.org/1515433002/diff/100001/media/cast/common/rtp_...
media/cast/common/rtp_time.h:27: // The difference between two RtpTimeTickses. 
This data type is modeled off of
On 2015/12/10 08:54:46, Mike West wrote:
> Nit: "Tickses" can't possibly be the right pluralization. :)

Done...but it was cuter the way I had it.  ;)

[miu, 2015-12-11 23:02:38]

miu@chromium.org changed reviewers:
+ apacible@chromium.org
- hubbe@chromium.org

[miu, 2015-12-11 23:02:39]

apacible: Need full committer rubber stamp.  (Basically, this CL is reviewed,
but feel free to take a look at media/cast/common/* for the "meat" of this
change.)

[apacible, 2015-12-11 23:54:59]

rs lgtm

https://codereview.chromium.org/1515433002/diff/120001/chrome/browser/media/c...
File chrome/browser/media/cast_transport_host_filter_unittest.cc (right):

https://codereview.chromium.org/1515433002/diff/120001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter_unittest.cc:111:
media::cast::RtpTimeTicks() + media::cast::RtpTimeDelta::FromTicks(47);
Is the 47 here related to kSamples?

https://codereview.chromium.org/1515433002/diff/120001/media/cast/common/rtp_...
File media/cast/common/rtp_time.cc (right):

https://codereview.chromium.org/1515433002/diff/120001/media/cast/common/rtp_...
media/cast/common/rtp_time.cc:21: 0.5 /* rounding */;
nit: indent to align with previous line

[miu, 2015-12-12 00:23:30]

https://codereview.chromium.org/1515433002/diff/120001/chrome/browser/media/c...
File chrome/browser/media/cast_transport_host_filter_unittest.cc (right):

https://codereview.chromium.org/1515433002/diff/120001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter_unittest.cc:111:
media::cast::RtpTimeTicks() + media::cast::RtpTimeDelta::FromTicks(47);
On 2015/12/11 23:54:59, apacible wrote:
> Is the 47 here related to kSamples?

Yes, good catch.  Fixed.

https://codereview.chromium.org/1515433002/diff/120001/media/cast/common/rtp_...
File media/cast/common/rtp_time.cc (right):

https://codereview.chromium.org/1515433002/diff/120001/media/cast/common/rtp_...
media/cast/common/rtp_time.cc:21: 0.5 /* rounding */;
On 2015/12/11 23:54:59, apacible wrote:
> nit: indent to align with previous line

Hahahah.  Irfan caught this too.  This is what `git cl format media` produced.

Okay, so I'm going to change this and continue the "hanging indentation" scheme
(by indenting 4 spaces to the right of the prior line).

BTW--There's a side discussion among the src/media owners about whether we
like/hate the indentation changes `git cl format` is making in the code.  The
guidance right now is to "do what you think is best."

[Mike West, 2015-12-13 07:39:02]

On 2015/12/11 at 23:00:29, miu wrote:
> mkwst: lgty now?

LGTM, thanks.

[miu, 2015-12-14 19:56:29]

The CQ bit was checked by miu@chromium.org

[miu, 2015-12-14 19:56:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
isheriff@chromium.org, apacible@chromium.org
Link to the patchset: https://codereview.chromium.org/1515433002/#ps140001
(title: "Addressed apacible's comments.  Fixed minor build dep issue after
adding cast_messages.cc.")

[commit-bot: I haz the power, 2015-12-14 19:57:26]

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

[commit-bot: I haz the power, 2015-12-14 20:11:17]

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

[commit-bot: I haz the power, 2015-12-14 20:11:18]

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

[miu, 2015-12-29 02:06:04]

The CQ bit was checked by miu@chromium.org

[miu, 2015-12-29 02:06:04]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
isheriff@chromium.org, apacible@chromium.org
Link to the patchset: https://codereview.chromium.org/1515433002/#ps160001
(title: "REBASE after holidays")

[commit-bot: I haz the power, 2015-12-29 02:06:10]

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

[commit-bot: I haz the power, 2015-12-29 02:11:07]

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

[commit-bot: I haz the power, 2015-12-29 02:11:09]

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

[miu, 2015-12-29 02:49:52]

miu@chromium.org changed reviewers:
+ thestig@chromium.org

[miu, 2015-12-29 02:49:53]

thestig: Need OWNERS rubber stamp on chrome/common/cast_messages.cc (mkwst@
already did IPC review, but presubmit wants the .cc file reviewed by someone
else).

[Lei Zhang, 2015-12-29 02:57:23]

lgtm

https://codereview.chromium.org/1515433002/diff/180001/chrome/common/cast_mes...
File chrome/common/cast_messages.h (right):

https://codereview.chromium.org/1515433002/diff/180001/chrome/common/cast_mes...
chrome/common/cast_messages.h:206: CastHostMsg_Delete, int32_t /* channel_id */)
Not sure why this is needed, but ok.

[miu, 2015-12-29 03:14:30]

The CQ bit was checked by miu@chromium.org

[miu, 2015-12-29 03:14:30]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org,
isheriff@chromium.org, apacible@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1515433002/#ps200001
(title: "Fix unintended indentation change in cast_messages.h")

[commit-bot: I haz the power, 2015-12-29 03:14:36]

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

[miu, 2015-12-29 03:14:51]

https://codereview.chromium.org/1515433002/diff/180001/chrome/common/cast_mes...
File chrome/common/cast_messages.h (right):

https://codereview.chromium.org/1515433002/diff/180001/chrome/common/cast_mes...
chrome/common/cast_messages.h:206: CastHostMsg_Delete, int32_t /* channel_id */)
On 2015/12/29 02:57:23, Lei Zhang wrote:
> Not sure why this is needed, but ok.

Reverted.  This was just an artifact left over from rebasing (some intermediate
change re-styled this whole file).

[commit-bot: I haz the power, 2015-12-29 03:23:43]

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

[commit-bot: I haz the power, 2015-12-29 03:23:44]

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

[miu, 2015-12-29 20:28:36]

Description was changed from

==========
Replace uses of raw uint32's with a type-checked RtpTimeTicks data type.

Introduces RtpTimeDelta and RtpTimeTicks to media::cast, modeled after
base::TimeDelta and base::TimeTicks, that track RTP time and provide a
strict set of valid math and comparison operators, bit truncation and
re-expansion, and timebase conversion.

All instances of raw uint32 used to represent RTP timestamps has been
replaced with the new data type.

BUG=530839
==========

to

==========
Replace uses of raw uint32's with a type-checked RtpTimeTicks data type.

Introduces RtpTimeDelta and RtpTimeTicks to media::cast, modeled after
base::TimeDelta and base::TimeTicks, that track RTP time and provide a
strict set of valid math and comparison operators, bit truncation and
re-expansion, and timebase conversion.

All instances of raw uint32 used to represent RTP timestamps has been
replaced with the new data type.

BUG=530839
NOPRESUBMIT=true
==========

[miu, 2015-12-29 20:53:01]

Patchset #11 (id:220001) has been deleted

[miu, 2015-12-29 20:56:30]

For the record, I'm committing with NOPRESUBMIT=true due to problems with `git
cl format media` in the media/ subdir.  There is an ongoing e-mail discussion
about resolving the issue.  For this change, I've accepted most of the format
script's changes, but have rejected the ones that make things unreadable.

[miu, 2015-12-29 20:56:37]

The CQ bit was checked by miu@chromium.org

[miu, 2015-12-29 20:56:38]

The patchset sent to the CQ was uploaded after l-g-t-m from
thestig@chromium.org, mkwst@chromium.org, isheriff@chromium.org,
apacible@chromium.org
Link to the patchset: https://codereview.chromium.org/1515433002/#ps240001
(title: "Speculative workaround fix for win8_chromium_ng compile error.")

[commit-bot: I haz the power, 2015-12-29 20:56:51]

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

[commit-bot: I haz the power, 2015-12-29 22:05:04]

Description was changed from

==========
Replace uses of raw uint32's with a type-checked RtpTimeTicks data type.

Introduces RtpTimeDelta and RtpTimeTicks to media::cast, modeled after
base::TimeDelta and base::TimeTicks, that track RTP time and provide a
strict set of valid math and comparison operators, bit truncation and
re-expansion, and timebase conversion.

All instances of raw uint32 used to represent RTP timestamps has been
replaced with the new data type.

BUG=530839
NOPRESUBMIT=true
==========

to

==========
Replace uses of raw uint32's with a type-checked RtpTimeTicks data type.

Introduces RtpTimeDelta and RtpTimeTicks to media::cast, modeled after
base::TimeDelta and base::TimeTicks, that track RTP time and provide a
strict set of valid math and comparison operators, bit truncation and
re-expansion, and timebase conversion.

All instances of raw uint32 used to represent RTP timestamps has been
replaced with the new data type.

BUG=530839
NOPRESUBMIT=true
==========

[commit-bot: I haz the power, 2015-12-29 22:05:05]

Committed patchset #11 (id:240001)

[commit-bot: I haz the power, 2015-12-29 22:05:56]

Description was changed from

==========
Replace uses of raw uint32's with a type-checked RtpTimeTicks data type.

Introduces RtpTimeDelta and RtpTimeTicks to media::cast, modeled after
base::TimeDelta and base::TimeTicks, that track RTP time and provide a
strict set of valid math and comparison operators, bit truncation and
re-expansion, and timebase conversion.

All instances of raw uint32 used to represent RTP timestamps has been
replaced with the new data type.

BUG=530839
NOPRESUBMIT=true
==========

to

==========
Replace uses of raw uint32's with a type-checked RtpTimeTicks data type.

Introduces RtpTimeDelta and RtpTimeTicks to media::cast, modeled after
base::TimeDelta and base::TimeTicks, that track RTP time and provide a
strict set of valid math and comparison operators, bit truncation and
re-expansion, and timebase conversion.

All instances of raw uint32 used to represent RTP timestamps has been
replaced with the new data type.

BUG=530839
NOPRESUBMIT=true

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

[commit-bot: I haz the power, 2015-12-29 22:05:57]

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

[dcheng, 2015-12-30 04:05:37]

On 2015/12/29 at 20:56:30, miu wrote:
> For the record, I'm committing with NOPRESUBMIT=true due to problems with `git
cl format media` in the media/ subdir.  There is an ongoing e-mail discussion
about resolving the issue.  For this change, I've accepted most of the format
script's changes, but have rejected the ones that make things unreadable.

Next time, please run the presubmit locally still before reverting objectionable
formatting changes. This change introduced new calls to Pass(), which I'm
actively trying to remove.