[cc] Adds source_id and sequence_number to BeginFrameArgs.

[cc] Adds source_id and sequence_number to BeginFrameArgs.

Also uses the sequence number in external BeginFrameSources to
decide whether a missed BeginFrame should be sent.

BUG=646774, 401331
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Committed: https://crrev.com/1ce4fd58dd1e43b5e033ab4d1a35028b04ac0224
Cr-Commit-Position: refs/heads/master@{#440382}

[Eric Seckler, 2016-12-15 11:58:58]

Description was changed from

==========
[cc] Adds source_id and sequence_number to BeginFrameArgs.

Also uses the sequence number in external BeginFrameSources to
decide whether a missed BeginFrame should be sent.

BUG=646774, 401331
==========

to

==========
[cc] Adds source_id and sequence_number to BeginFrameArgs.

Also uses the sequence number in external BeginFrameSources to
decide whether a missed BeginFrame should be sent.

BUG=646774, 401331
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Eric Seckler, 2016-12-15 11:59:36]

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

[commit-bot: I haz the power, 2016-12-15 11:59:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2016-12-15 13:23:00]

eseckler@chromium.org changed reviewers:
+ brianderson@chromium.org

[Eric Seckler, 2016-12-15 13:23:00]

Hi Brian, as promised, sequence numbers first! :)

Thanks!

[commit-bot: I haz the power, 2016-12-15 13:29:43]

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

[commit-bot: I haz the power, 2016-12-15 13:29:45]

Dry run: This issue passed the CQ dry run.

[Sami, 2016-12-15 13:53:00]

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

[Sami, 2016-12-15 13:53:01]

Non-owner lgtm.

https://codereview.chromium.org/2583483002/diff/1/cc/output/begin_frame_args.h
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/2583483002/diff/1/cc/output/begin_frame_args....
cc/output/begin_frame_args.h:94: uint32_t source_id;
nit: move source_id toward the end to help with packing.

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/begin_frame_so...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source.cc:29: // BeginFrameObserverBase
-------------------------------------------------
Very detail-oriented :)

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source.cc:53: static base::StaticAtomicSequenceNumber
next_source_id_;
nit: g_next_source_id since this isn't a member.

[Eric Seckler, 2016-12-15 14:59:38]

Thanks, Sami!

https://codereview.chromium.org/2583483002/diff/1/cc/output/begin_frame_args.h
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/2583483002/diff/1/cc/output/begin_frame_args....
cc/output/begin_frame_args.h:94: uint32_t source_id;
On 2016/12/15 13:53:00, Sami wrote:
> nit: move source_id toward the end to help with packing.

okay, moved both towards the end.

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/begin_frame_so...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source.cc:29: // BeginFrameObserverBase
-------------------------------------------------
On 2016/12/15 13:53:00, Sami wrote:
> Very detail-oriented :)

.. and maybe a touch of ocd :)

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source.cc:53: static base::StaticAtomicSequenceNumber
next_source_id_;
On 2016/12/15 13:53:00, Sami wrote:
> nit: g_next_source_id since this isn't a member.

Done.

[brianderson, 2016-12-15 20:30:36]

Only looked at the begin_frame_[source|args] files so far.

https://codereview.chromium.org/2583483002/diff/1/cc/output/begin_frame_args.h
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/2583483002/diff/1/cc/output/begin_frame_args....
cc/output/begin_frame_args.h:53: static const uint64_t kInvalidFrameNumber;
constexpr?

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/begin_frame_so...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source.cc:98: if
(pending_begin_frame_observers_.empty())
Is this change related? If it fixes a bug, please create a separate patch along
with a unit test.

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source.cc:118: BEGINFRAME_FROM_HERE, source_id(),
next_sequence_number_++, frame_time,
Please move the next_sequence_number_++ after the call for readability.

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source.cc:176: // Check if |current_begin_frame_args_|
are still valid. They may not be valid
"valid" is an overloaded term in this context. Can you talk about sequence
number specifically?

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source.cc:190:
CreateBeginFrameArgs(current_begin_frame_args_.sequence_number + 1,
"current_begin_frame_args_.sequence_number + 1" relies on the fact that the
invalid value is 0 and initial value is 1, which is brittle and difficult to
decode that's how this works for the first frame.

Can you do what you did in BackToBackBeginFrameSource instead? That might also
help avoid having a sequence number as an argument to CreateBegiFrameArgs.

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source.cc:257: last_args.sequence_number)) {
I think this if statement should stay the way it was before since monotonic
timestamps is the contract we have with the animation system.

The lines you added here should probably turn into a DCHECK instead.

[brianderson, 2016-12-15 22:10:33]

https://codereview.chromium.org/2583483002/diff/1/cc/blimp/layer_tree_host_re...
File cc/blimp/layer_tree_host_remote.cc (right):

https://codereview.chromium.org/2583483002/diff/1/cc/blimp/layer_tree_host_re...
cc/blimp/layer_tree_host_remote.cc:374: begin_frame_number_++, now, now +
kDefaultFrameInterval,
Increment after the call.

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/begin_frame_so...
File cc/scheduler/begin_frame_source_unittest.cc (right):

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source_unittest.cc:25: TEST_F(BeginFrameSourceTest,
SourceIdsAreUnique) {
I think this can just be TEST(), which gets rid of the need for line 23.

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/scheduler_unit...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:413: next_begin_frame_number_++, now_src());
Increment after.

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:1342: next_begin_frame_number_++, now_src());
Increment after.

https://codereview.chromium.org/2583483002/diff/1/cc/surfaces/display_schedul...
File cc/surfaces/display_scheduler_unittest.cc (right):

https://codereview.chromium.org/2583483002/diff/1/cc/surfaces/display_schedul...
cc/surfaces/display_scheduler_unittest.cc:105: uint64_t
next_begin_frame_number_;
In this case, does it make sense for this to be tracked by the
FakeExternalBeginFrameSource?

https://codereview.chromium.org/2583483002/diff/1/cc/test/begin_frame_source_...
File cc/test/begin_frame_source_test.cc (right):

https://codereview.chromium.org/2583483002/diff/1/cc/test/begin_frame_source_...
cc/test/begin_frame_source_test.cc:39: BeginFrameArgs::kStartingFrameNumber,
Should this be "Invalid" since the rest of the values in the list correspond to
and invalid BeginFrameArgs?

https://codereview.chromium.org/2583483002/diff/20001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/2583483002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:49: // TODO(eseckler): Consider replacing this with
BeginFrame sequence numbers.
I think we can delete this TODO. SourceFrameNumber increments for every commit,
but BeginFrame sequence numbers increment for every BeginFrame sent.

A lot of tests rely on SourceFrameNumber to wait for a specific commit to
complete. Relying on BeginFrame numbers for that would not be reliable since
there could be multiple BeginFrames between commits.

https://codereview.chromium.org/2583483002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2583483002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:492:
begin_frame_args.sequence_number++;
Why isn't this incremented afterwards?

https://codereview.chromium.org/2583483002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:10526:
InputHandler::SCROLL_ON_IMPL_THREAD,
There's a lot of unrelated whitespace / formatting changes in this file. Don't
worry about removing it at this point, but try to keep larger formatting patches
separate in the future which makes review easier.

https://codereview.chromium.org/2583483002/diff/20001/ui/android/window_andro...
File ui/android/window_android.cc (right):

https://codereview.chromium.org/2583483002/diff/20001/ui/android/window_andro...
ui/android/window_android.cc:70: last_args.sequence_number <
last_begin_frame_args_.sequence_number) {
Keep this frame_time and convert sequence check to DCHECK.

[Eric Seckler, 2016-12-16 17:25:58]

Thank you for the fantastic turn-around, Brian :D

https://codereview.chromium.org/2583483002/diff/1/cc/blimp/layer_tree_host_re...
File cc/blimp/layer_tree_host_remote.cc (right):

https://codereview.chromium.org/2583483002/diff/1/cc/blimp/layer_tree_host_re...
cc/blimp/layer_tree_host_remote.cc:374: begin_frame_number_++, now, now +
kDefaultFrameInterval,
On 2016/12/15 22:10:33, brianderson wrote:
> Increment after the call.

Done.

https://codereview.chromium.org/2583483002/diff/1/cc/output/begin_frame_args.h
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/2583483002/diff/1/cc/output/begin_frame_args....
cc/output/begin_frame_args.h:53: static const uint64_t kInvalidFrameNumber;
On 2016/12/15 20:30:35, brianderson wrote:
> constexpr?

Done.

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/begin_frame_so...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source.cc:98: if
(pending_begin_frame_observers_.empty())
On 2016/12/15 20:30:36, brianderson wrote:
> Is this change related? If it fixes a bug, please create a separate patch
along
> with a unit test.

ok, will do.

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source.cc:118: BEGINFRAME_FROM_HERE, source_id(),
next_sequence_number_++, frame_time,
On 2016/12/15 20:30:35, brianderson wrote:
> Please move the next_sequence_number_++ after the call for readability.

Done.

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source.cc:176: // Check if |current_begin_frame_args_|
are still valid. They may not be valid
On 2016/12/15 20:30:36, brianderson wrote:
> "valid" is an overloaded term in this context. Can you talk about sequence
> number specifically?

Done.

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source.cc:190:
CreateBeginFrameArgs(current_begin_frame_args_.sequence_number + 1,
On 2016/12/15 20:30:36, brianderson wrote:
> "current_begin_frame_args_.sequence_number + 1" relies on the fact that the
> invalid value is 0 and initial value is 1, which is brittle and difficult to
> decode that's how this works for the first frame.
> 
> Can you do what you did in BackToBackBeginFrameSource instead? That might also
> help avoid having a sequence number as an argument to CreateBegiFrameArgs.

Done.

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source.cc:257: last_args.sequence_number)) {
On 2016/12/15 20:30:36, brianderson wrote:
> I think this if statement should stay the way it was before since monotonic
> timestamps is the contract we have with the animation system.
> 
> The lines you added here should probably turn into a DCHECK instead.

Hm, okay. Will do. I was thinking ahead, considering situations where we might
want to retry a frame with the same timestamp. That'd be useful for headless
when a frame fails for some reason but we still want a deterministic result.
Anyway, seems like this requires some more thinking on its own :)

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/begin_frame_so...
File cc/scheduler/begin_frame_source_unittest.cc (right):

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source_unittest.cc:25: TEST_F(BeginFrameSourceTest,
SourceIdsAreUnique) {
On 2016/12/15 22:10:33, brianderson wrote:
> I think this can just be TEST(), which gets rid of the need for line 23.

Done.

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/scheduler_unit...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:413: next_begin_frame_number_++, now_src());
On 2016/12/15 22:10:33, brianderson wrote:
> Increment after.

Done.

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/scheduler_unit...
cc/scheduler/scheduler_unittest.cc:1342: next_begin_frame_number_++, now_src());
On 2016/12/15 22:10:33, brianderson wrote:
> Increment after.

Done.

https://codereview.chromium.org/2583483002/diff/1/cc/surfaces/display_schedul...
File cc/surfaces/display_scheduler_unittest.cc (right):

https://codereview.chromium.org/2583483002/diff/1/cc/surfaces/display_schedul...
cc/surfaces/display_scheduler_unittest.cc:105: uint64_t
next_begin_frame_number_;
On 2016/12/15 22:10:33, brianderson wrote:
> In this case, does it make sense for this to be tracked by the
> FakeExternalBeginFrameSource?

true, that's nicer :) Done.

https://codereview.chromium.org/2583483002/diff/1/cc/test/begin_frame_source_...
File cc/test/begin_frame_source_test.cc (right):

https://codereview.chromium.org/2583483002/diff/1/cc/test/begin_frame_source_...
cc/test/begin_frame_source_test.cc:39: BeginFrameArgs::kStartingFrameNumber,
On 2016/12/15 22:10:33, brianderson wrote:
> Should this be "Invalid" since the rest of the values in the list correspond
to
> and invalid BeginFrameArgs?

I added a DCHECK to the BeginFrameArgs constructor with field values, which
verifies that the provided sequence_number is valid. I can get rid of it to
allow a "bad" sequence number here if you'd prefer that?

https://codereview.chromium.org/2583483002/diff/20001/cc/trees/layer_tree_host.h
File cc/trees/layer_tree_host.h (right):

https://codereview.chromium.org/2583483002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host.h:49: // TODO(eseckler): Consider replacing this with
BeginFrame sequence numbers.
On 2016/12/15 22:10:33, brianderson wrote:
> I think we can delete this TODO. SourceFrameNumber increments for every
commit,
> but BeginFrame sequence numbers increment for every BeginFrame sent.
> 
> A lot of tests rely on SourceFrameNumber to wait for a specific commit to
> complete. Relying on BeginFrame numbers for that would not be reliable since
> there could be multiple BeginFrames between commits.

Ok, sounds good! Thanks :)

https://codereview.chromium.org/2583483002/diff/20001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/2583483002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:492:
begin_frame_args.sequence_number++;
On 2016/12/15 22:10:33, brianderson wrote:
> Why isn't this incremented afterwards?

Yeah, this is actually buggy since it copies |begin_frame_args|. Moved it out of
the method.

(I increment before BeginImplFrameAndAnimate now b/c the args are initialized
with the last used sequence number in the tests.)

https://codereview.chromium.org/2583483002/diff/20001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_impl_unittest.cc:10526:
InputHandler::SCROLL_ON_IMPL_THREAD,
On 2016/12/15 22:10:33, brianderson wrote:
> There's a lot of unrelated whitespace / formatting changes in this file. Don't
> worry about removing it at this point, but try to keep larger formatting
patches
> separate in the future which makes review easier.

Sorry about that, must have accidentally formatted the whole file.

https://codereview.chromium.org/2583483002/diff/20001/ui/android/window_andro...
File ui/android/window_android.cc (right):

https://codereview.chromium.org/2583483002/diff/20001/ui/android/window_andro...
ui/android/window_android.cc:70: last_args.sequence_number <
last_begin_frame_args_.sequence_number) {
On 2016/12/15 22:10:33, brianderson wrote:
> Keep this frame_time and convert sequence check to DCHECK.

Done.

[Eric Seckler, 2016-12-16 17:32:09]

eseckler@chromium.org changed reviewers:
+ nasko@chromium.org, tedchoc@chromium.org

[Eric Seckler, 2016-12-16 17:32:09]

+tedchoc@ for ui/android/window_android.cc

Nasko, would you be happy to look at content/ and the IPC cc/ipc changes?

Thanks!

[brianderson, 2016-12-16 17:53:38]

lgtm!

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/begin_frame_so...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/2583483002/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source.cc:257: last_args.sequence_number)) {
On 2016/12/16 17:25:57, Eric Seckler wrote:
> On 2016/12/15 20:30:36, brianderson wrote:
> > I think this if statement should stay the way it was before since monotonic
> > timestamps is the contract we have with the animation system.
> > 
> > The lines you added here should probably turn into a DCHECK instead.
> 
> Hm, okay. Will do. I was thinking ahead, considering situations where we might
> want to retry a frame with the same timestamp. That'd be useful for headless
> when a frame fails for some reason but we still want a deterministic result.
> Anyway, seems like this requires some more thinking on its own :)

I see. Maybe the animation system is okay with monotonic rather than strictly
monotonic timestamps - but I imagine there could be weird corner cases to debug.
It definitely didn't work when we ticked backwards in time!

https://codereview.chromium.org/2583483002/diff/1/cc/test/begin_frame_source_...
File cc/test/begin_frame_source_test.cc (right):

https://codereview.chromium.org/2583483002/diff/1/cc/test/begin_frame_source_...
cc/test/begin_frame_source_test.cc:39: BeginFrameArgs::kStartingFrameNumber,
On 2016/12/16 17:25:58, Eric Seckler wrote:
> On 2016/12/15 22:10:33, brianderson wrote:
> > Should this be "Invalid" since the rest of the values in the list correspond
> to
> > and invalid BeginFrameArgs?
> 
> I added a DCHECK to the BeginFrameArgs constructor with field values, which
> verifies that the provided sequence_number is valid. I can get rid of it to
> allow a "bad" sequence number here if you'd prefer that?

This is okay as is then. Thanks for the explanation.

https://codereview.chromium.org/2583483002/diff/40001/ui/android/window_andro...
File ui/android/window_android.cc (right):

https://codereview.chromium.org/2583483002/diff/40001/ui/android/window_andro...
ui/android/window_android.cc:71: DCHECK(last_args.sequence_number <
Is it possible for the source id to change?
If yes: Does this DCHECK need to be guarded by a source id check?
If no: Please add a DCHECK that the source_id does not change.

[Eric Seckler, 2016-12-20 12:50:34]

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

[commit-bot: I haz the power, 2016-12-20 12:50:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eric Seckler, 2016-12-20 12:52:28]

eseckler@chromium.org changed reviewers:
+ twellington@chromium.org
- tedchoc@chromium.org

[Eric Seckler, 2016-12-20 12:52:29]

nasko@ friendly ping :)

Ted is OOO, adding twellington@ for ui/android/window_android.cc instead.

https://codereview.chromium.org/2583483002/diff/40001/ui/android/window_andro...
File ui/android/window_android.cc (right):

https://codereview.chromium.org/2583483002/diff/40001/ui/android/window_andro...
ui/android/window_android.cc:71: DCHECK(last_args.sequence_number <
On 2016/12/16 17:53:38, brianderson wrote:
> Is it possible for the source id to change?
> If yes: Does this DCHECK need to be guarded by a source id check?
> If no: Please add a DCHECK that the source_id does not change.

I think you're right, the observer could switch windows, thus last_args may have
a different source_id. Adding the source_id check.

[commit-bot: I haz the power, 2016-12-20 13:49:28]

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

[commit-bot: I haz the power, 2016-12-20 13:49:29]

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

[Eric Seckler, 2016-12-20 14:57:16]

Patchset #4 (id:60001) has been deleted

[Eric Seckler, 2016-12-20 14:57:26]

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

[commit-bot: I haz the power, 2016-12-20 14:57:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Theresa, 2016-12-20 16:35:20]

twellington@chromium.org changed reviewers:
+ boliu@chromium.org

[Theresa, 2016-12-20 16:35:21]

+boliu@ to look at window_android.cc, then I'll do an OWNERS stamp.

[boliu, 2016-12-20 16:38:02]

On 2016/12/20 16:35:21, Theresa Wellington wrote:
> +boliu@ to look at window_android.cc, then I'll do an OWNERS stamp.

window_android lgtm

[Theresa, 2016-12-20 16:38:28]

lgtm

[commit-bot: I haz the power, 2016-12-20 17:32:52]

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

[commit-bot: I haz the power, 2016-12-20 17:32:53]

Dry run: This issue passed the CQ dry run.

[nasko, 2016-12-21 17:31:45]

IPC and content/ LGTM with a couple of nits.

https://codereview.chromium.org/2583483002/diff/100001/cc/output/begin_frame_...
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/2583483002/diff/100001/cc/output/begin_frame_...
cc/output/begin_frame_args.h:99: uint32_t source_id;
nit: order the same way you have them in the constructor and the Create method.

https://codereview.chromium.org/2583483002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2583483002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1929: //
crbug.com/675289: Update |last_begin_frame_args_| before handling
nit: https:// so code search can linkify it.

[Eric Seckler, 2016-12-22 09:17:33]

Patchset #6 (id:120001) has been deleted

[Eric Seckler, 2016-12-22 09:20:27]

Thanks Nasko!

https://codereview.chromium.org/2583483002/diff/100001/cc/output/begin_frame_...
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/2583483002/diff/100001/cc/output/begin_frame_...
cc/output/begin_frame_args.h:99: uint32_t source_id;
On 2016/12/21 17:31:45, nasko wrote:
> nit: order the same way you have them in the constructor and the Create
method.

The ordering here is taking packing of the struct into account, see earlier
comments on the patch by Sami. I'll leave the ordering in the constructor /
Create method as-is, since it makes more sense semantically, though. Updating
the order of fields/methods in ipc-related files to match field order.

Added a short comment here as well.

https://codereview.chromium.org/2583483002/diff/100001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2583483002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1929: //
crbug.com/675289: Update |last_begin_frame_args_| before handling
On 2016/12/21 17:31:45, nasko wrote:
> nit: https:// so code search can linkify it.

Uh, this must have snuck back during rebase. Shouldn't have been here at all.
Removed.

[Eric Seckler, 2016-12-22 09:21:49]

The CQ bit was checked by eseckler@chromium.org

[Eric Seckler, 2016-12-22 09:21:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
skyostil@chromium.org, brianderson@chromium.org, twellington@chromium.org,
boliu@chromium.org, nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2583483002/#ps140001
(title: "fix field ordering")

[commit-bot: I haz the power, 2016-12-22 09:22:07]

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, 2016-12-22 11:23:44]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1482398509986910,
"parent_rev": "7f80c288b820d35aab125e1509cefbb8fc651d93", "commit_rev":
"c0878d896f5e6acf15567582fa8fa20c45583d9b"}

[commit-bot: I haz the power, 2016-12-22 11:24:12]

Description was changed from

==========
[cc] Adds source_id and sequence_number to BeginFrameArgs.

Also uses the sequence number in external BeginFrameSources to
decide whether a missed BeginFrame should be sent.

BUG=646774, 401331
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
[cc] Adds source_id and sequence_number to BeginFrameArgs.

Also uses the sequence number in external BeginFrameSources to
decide whether a missed BeginFrame should be sent.

BUG=646774, 401331
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2583483002
==========

[commit-bot: I haz the power, 2016-12-22 11:24:14]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-12-22 11:28:15]

Description was changed from

==========
[cc] Adds source_id and sequence_number to BeginFrameArgs.

Also uses the sequence number in external BeginFrameSources to
decide whether a missed BeginFrame should be sent.

BUG=646774, 401331
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2583483002
==========

to

==========
[cc] Adds source_id and sequence_number to BeginFrameArgs.

Also uses the sequence number in external BeginFrameSources to
decide whether a missed BeginFrame should be sent.

BUG=646774, 401331
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Committed: https://crrev.com/1ce4fd58dd1e43b5e033ab4d1a35028b04ac0224
Cr-Commit-Position: refs/heads/master@{#440382}
==========

[commit-bot: I haz the power, 2016-12-22 11:28:17]

Patchset 6 (id:??) landed as
https://crrev.com/1ce4fd58dd1e43b5e033ab4d1a35028b04ac0224
Cr-Commit-Position: refs/heads/master@{#440382}