cc: Fix negative EstimatedParentDrawTimes

cc: Fix negative EstimatedParentDrawTimes

BUG=380987
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276369

[brianderson, 2014-06-06 22:39:53]

Ptal. Not sure this needs a test since it's "just" an incorrectly set constant.

[danakj, 2014-06-06 22:45:45]

Were we using a positive value in the scheduler tests then that would cover use
of this value?

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

https://codereview.chromium.org/311263016/diff/1/cc/output/begin_frame_args.c...
cc/output/begin_frame_args.cc:55: base::TimeDelta
BeginFrameArgs::DefaultEstimatedParentDrawTime() {
+1 to the new name

[brianderson, 2014-06-07 00:49:24]

I added a DCHECK as a poor man's test.

I was in the middle of adding Scheduler tests that exercise the
EstimatedParentDrawTime, but I need to fix
https://code.google.com/p/chromium/issues/detail?id=380889 first.

I talked to some Intel folks, and apparently this bug causes a 20% regression in
one of their WebGL use cases so I would like to land and cherry pick this patch
separately from improving test coverage.

I've opened https://code.google.com/p/chromium/issues/detail?id=381972 to add
EstimatedParentDrawTime coverage.

[danakj, 2014-06-07 13:59:53]

On 2014/06/07 00:49:24, brianderson wrote:
> I added a DCHECK as a poor man's test.
> 
> I was in the middle of adding Scheduler tests that exercise the
> EstimatedParentDrawTime, but I need to fix
> https://code.google.com/p/chromium/issues/detail?id=380889 first.
> 
> I talked to some Intel folks, and apparently this bug causes a 20% regression
in
> one of their WebGL use cases so I would like to land and cherry pick this
patch
> separately from improving test coverage.
> 
> I've opened https://code.google.com/p/chromium/issues/detail?id=381972 to add
> EstimatedParentDrawTime coverage.

OK LGTM

[Sami, 2014-06-09 09:42:44]

Thanks for fixing this Brian, lgtm.

[brianderson, 2014-06-09 18:05:03]

The CQ bit was checked by brianderson@chromium.org

[commit-bot: I haz the power, 2014-06-09 18:06:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/brianderson@chromium.org/311263016/20001

[commit-bot: I haz the power, 2014-06-10 08:44:55]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-10 12:22:48]

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

[commit-bot: I haz the power, 2014-06-10 12:22:49]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[brianderson, 2014-06-11 01:31:46]

Sami, I fixed the race in LayerTreeHostAnimationTestAddAnimationAfterAnimating.
Please let me know if I changed the test such that it's no longer testing
something that it was testing before.

I'm pretty confident in the fix though, so I'm going to hit the cq again.

[brianderson, 2014-06-11 01:31:53]

The CQ bit was checked by brianderson@chromium.org

[commit-bot: I haz the power, 2014-06-11 01:35:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/brianderson@chromium.org/311263016/40001

[commit-bot: I haz the power, 2014-06-11 12:48:27]

Change committed as 276369

[Sami, 2014-06-11 14:57:30]

Ok, that fix lgtm.