Avoid DCHECK in ExternalBeginFrameSource::GetMissedBeginFrameArgs caused by out of order frame time

Avoid DCHECK in ExternalBeginFrameSource::GetMissedBeginFrameArgs caused by out of order frame time

DCHECK was caused by incorrectly inverting the logical check in 
ExternalBeginFrameSource::AddObserver when refactoring the code.

Before the change:

if (!last_args.IsValid() ||
   (last_begin_frame_args_.frame_time > last_args.frame_time)) { 
  // Generate MISSED args here
  ...
}

After the change:

if (last_args.IsValid() &&
    last_begin_frame_args_.frame_time == last_args.frame_time) {
  // SKIP generating MISSED args
  return BeginFrameArgs();
}

last_begin_frame_args_.frame_time which is smaller than last_args.frame_time,
while arguably invalid, would now allow this code to generate a MISSED args
and trigger a DCHECK.

I've fixed this by changing the condition above to
last_begin_frame_args_.frame_time <= last_args.frame_time.

I've added a new unit test to cover this specific case.

BUG=730218
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2930813002
Cr-Commit-Position: refs/heads/master@{#477846}
Committed: https://chromium.googlesource.com/chromium/src/+/25d005fb3364df055d7773cffe7039cfd3d33eca

[stanisc, 2017-06-07 23:02:11]

Description was changed from

==========
Fix DCHECK in ExternalBeginFrameSource::GetMissedBeginFrameArgs

BUG=730218
==========

to

==========
Fix DCHECK in ExternalBeginFrameSource::GetMissedBeginFrameArgs

BUG=730218
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[stanisc, 2017-06-07 23:02:29]

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

[commit-bot: I haz the power, 2017-06-07 23:03:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[stanisc, 2017-06-07 23:09:05]

Description was changed from

==========
Fix DCHECK in ExternalBeginFrameSource::GetMissedBeginFrameArgs

BUG=730218
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Fix DCHECK in ExternalBeginFrameSource::GetMissedBeginFrameArgs

DCHECK was caused by incorrectly inverting the logical check in 
ExternalBeginFrameSource::AddObserver when refactoring the code.

Before the change:

if (!last_args.IsValid() ||
   (last_begin_frame_args_.frame_time > last_args.frame_time)) { 
  // Generate MISSED args here
  ...
}

After the change:

if (last_args.IsValid() &&
    last_begin_frame_args_.frame_time == last_args.frame_time) {
  // SKIP generating MISSED args
  return BeginFrameArgs();
}

last_begin_frame_args_.frame_time which is smaller than last_args.frame_time,
while arguably invalid, would now allow this code to generate a MISSED args
and trigger a DCHECK.

I've fixed this by changing the condition above to
last_begin_frame_args_.frame_time <= last_args.frame_time.

I've added a new unit test to cover this specific case.

BUG=730218
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[stanisc, 2017-06-07 23:09:33]

stanisc@chromium.org changed reviewers:
+ sunnyps@chromium.org

[stanisc, 2017-06-07 23:09:33]

PTAL!

[sunnyps, 2017-06-07 23:22:24]

lgtm % nits

nit: The CL description says "Fix DCHECK" but it's not actually a DCHECK right?
It's actually fixing the frame time out of order early out in
GetMissedBeginFrameArgs.

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

https://codereview.chromium.org/2930813002/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source_unittest.cc:596: EXPECT_CALL(*obs_,
OnBeginFrame(CreateBeginFrameArgsForTesting(
nit: EXPECT_CALL(*obs_, _) instead? We don't want any begin frame no matter how
the args are calculated.

[stanisc, 2017-06-07 23:32:42]

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

[commit-bot: I haz the power, 2017-06-07 23:33:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[stanisc, 2017-06-07 23:36:11]

Description was changed from

==========
Fix DCHECK in ExternalBeginFrameSource::GetMissedBeginFrameArgs

DCHECK was caused by incorrectly inverting the logical check in 
ExternalBeginFrameSource::AddObserver when refactoring the code.

Before the change:

if (!last_args.IsValid() ||
   (last_begin_frame_args_.frame_time > last_args.frame_time)) { 
  // Generate MISSED args here
  ...
}

After the change:

if (last_args.IsValid() &&
    last_begin_frame_args_.frame_time == last_args.frame_time) {
  // SKIP generating MISSED args
  return BeginFrameArgs();
}

last_begin_frame_args_.frame_time which is smaller than last_args.frame_time,
while arguably invalid, would now allow this code to generate a MISSED args
and trigger a DCHECK.

I've fixed this by changing the condition above to
last_begin_frame_args_.frame_time <= last_args.frame_time.

I've added a new unit test to cover this specific case.

BUG=730218
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Avoid DCHECK in ExternalBeginFrameSource::GetMissedBeginFrameArgs caused by out
of order frame time

DCHECK was caused by incorrectly inverting the logical check in 
ExternalBeginFrameSource::AddObserver when refactoring the code.

Before the change:

if (!last_args.IsValid() ||
   (last_begin_frame_args_.frame_time > last_args.frame_time)) { 
  // Generate MISSED args here
  ...
}

After the change:

if (last_args.IsValid() &&
    last_begin_frame_args_.frame_time == last_args.frame_time) {
  // SKIP generating MISSED args
  return BeginFrameArgs();
}

last_begin_frame_args_.frame_time which is smaller than last_args.frame_time,
while arguably invalid, would now allow this code to generate a MISSED args
and trigger a DCHECK.

I've fixed this by changing the condition above to
last_begin_frame_args_.frame_time <= last_args.frame_time.

I've added a new unit test to cover this specific case.

BUG=730218
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[stanisc, 2017-06-07 23:36:33]

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

https://codereview.chromium.org/2930813002/diff/1/cc/scheduler/begin_frame_so...
cc/scheduler/begin_frame_source_unittest.cc:596: EXPECT_CALL(*obs_,
OnBeginFrame(CreateBeginFrameArgsForTesting(
On 2017/06/07 23:22:24, sunnyps wrote:
> nit: EXPECT_CALL(*obs_, _) instead? We don't want any begin frame no matter
how
> the args are calculated.

Done. I tried that initially but couldn't make it work because I was
missing a using statement at the top.

[commit-bot: I haz the power, 2017-06-08 01:05:06]

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

[commit-bot: I haz the power, 2017-06-08 01:05:07]

Dry run: This issue passed the CQ dry run.

[stanisc, 2017-06-08 01:07:54]

The CQ bit was checked by stanisc@chromium.org

[stanisc, 2017-06-08 01:07:54]

The patchset sent to the CQ was uploaded after l-g-t-m from sunnyps@chromium.org
Link to the patchset: https://codereview.chromium.org/2930813002/#ps20001
(title: "Addressed CR feedback")

[commit-bot: I haz the power, 2017-06-08 01:08:29]

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, 2017-06-08 01:13:29]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1496884074357290,
"parent_rev": "bdc4a31b6e88e8a12b0b7bb298a03b020b1c61de", "commit_rev":
"25d005fb3364df055d7773cffe7039cfd3d33eca"}

[commit-bot: I haz the power, 2017-06-08 01:13:40]

Description was changed from

==========
Avoid DCHECK in ExternalBeginFrameSource::GetMissedBeginFrameArgs caused by out
of order frame time

DCHECK was caused by incorrectly inverting the logical check in 
ExternalBeginFrameSource::AddObserver when refactoring the code.

Before the change:

if (!last_args.IsValid() ||
   (last_begin_frame_args_.frame_time > last_args.frame_time)) { 
  // Generate MISSED args here
  ...
}

After the change:

if (last_args.IsValid() &&
    last_begin_frame_args_.frame_time == last_args.frame_time) {
  // SKIP generating MISSED args
  return BeginFrameArgs();
}

last_begin_frame_args_.frame_time which is smaller than last_args.frame_time,
while arguably invalid, would now allow this code to generate a MISSED args
and trigger a DCHECK.

I've fixed this by changing the condition above to
last_begin_frame_args_.frame_time <= last_args.frame_time.

I've added a new unit test to cover this specific case.

BUG=730218
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Avoid DCHECK in ExternalBeginFrameSource::GetMissedBeginFrameArgs caused by out
of order frame time

DCHECK was caused by incorrectly inverting the logical check in 
ExternalBeginFrameSource::AddObserver when refactoring the code.

Before the change:

if (!last_args.IsValid() ||
   (last_begin_frame_args_.frame_time > last_args.frame_time)) { 
  // Generate MISSED args here
  ...
}

After the change:

if (last_args.IsValid() &&
    last_begin_frame_args_.frame_time == last_args.frame_time) {
  // SKIP generating MISSED args
  return BeginFrameArgs();
}

last_begin_frame_args_.frame_time which is smaller than last_args.frame_time,
while arguably invalid, would now allow this code to generate a MISSED args
and trigger a DCHECK.

I've fixed this by changing the condition above to
last_begin_frame_args_.frame_time <= last_args.frame_time.

I've added a new unit test to cover this specific case.

BUG=730218
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2930813002
Cr-Commit-Position: refs/heads/master@{#477846}
Committed:
https://chromium.googlesource.com/chromium/src/+/25d005fb3364df055d7773cffe70...
==========

[commit-bot: I haz the power, 2017-06-08 01:13:42]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/25d005fb3364df055d7773cffe70...