Moving deadline adjustment from SyntheticBeginFrameSource into BeginFrameArgs.

Moving deadline adjustment from SyntheticBeginFrameSource into BeginFrameArgs.

The correct place for this adjustment seems to be in BeginFrame (or maybe
AdjustedBeginImplFrameDeadline?) but definately not in the
SyntheticBeginFrameSource.

Currently the code does;
  base::TimeDelta parent_draw_time =
      output_surface_->capabilities().adjust_deadline_for_parent
          ? BeginFrameArgs::DefaultDeadlineAdjustment()
          : base::TimeDelta();
  client_->SetEstimatedParentDrawTime(parent_draw_time);

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

[mithro-old, 2014-06-10 03:21:25]

Hi Brian,

Another small CL related to the begin frame source refactor. I'm unsure if it is
worth landing this if the refactor is going to be landed soon after, but it does
give a clearer idea of the progression of changes.

Tim

[brianderson, 2014-06-10 18:24:23]

https://codereview.chromium.org/328603002/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/328603002/diff/1/cc/scheduler/scheduler.cc#ne...
cc/scheduler/scheduler.cc:401: adjusted_args.deadline -=
EstimatedParentDrawTime();
I had some reservations about putting the adjustment inside of the
SyntheticBeginFrameSource as well, but I think it still makes the most sense
because, when we are using a real BeginFrame message, the sender is supposed to
adjust the deadline based on it's own expected draw time.

I agree that it's a bit confusing to apply the EstimatedParentDrawTime in some
cases but not others. I wonder if we can clarify the usage and reasoning with
better naming or if we need to explain things better with comments.

[mithro-old, 2014-06-11 02:02:42]

On 2014/06/10 at 18:24:23, brianderson wrote:
> https://codereview.chromium.org/328603002/diff/1/cc/scheduler/scheduler.cc
> File cc/scheduler/scheduler.cc (right):
> 
>
https://codereview.chromium.org/328603002/diff/1/cc/scheduler/scheduler.cc#ne...
> cc/scheduler/scheduler.cc:401: adjusted_args.deadline -=
EstimatedParentDrawTime();
> I had some reservations about putting the adjustment inside of the
SyntheticBeginFrameSource as well, but I think it still makes the most sense
because, when we are using a real BeginFrame message, the sender is supposed to
adjust the deadline based on it's own expected draw time.
> 
> I agree that it's a bit confusing to apply the EstimatedParentDrawTime in some
cases but not others. I wonder if we can clarify the usage and reasoning with
better naming or if we need to explain things better with comments.

My theory was if the parent is doing the adjustment, EstimatedParentDrawTime is
zero, hence it is still safe to do the adjustment. It also means that we can be
consistent through all cases.

I do think it probably makes better sense in the AdjustedBeginImplFrameDeadline
method though?

The FrameSource code doesn't have a access to the scheduler, so this will have
to move at that point.

Tim

[brianderson, 2014-06-11 02:08:09]

Ok. I understand why this needs to come out then. Let's leave the adjustment as
you currently have it in the BeginFrame method so that we know the target
deadline in traces.

lgtm.

[brianderson, 2014-06-11 02:08:31]

please reference a bug btw.

[mithro-old, 2014-06-11 02:22:37]

The CQ bit was checked by mithro@mithis.com

[commit-bot: I haz the power, 2014-06-11 02:25:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mithro@mithis.com/328603002/1

[commit-bot: I haz the power, 2014-06-11 08:30:52]

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

[commit-bot: I haz the power, 2014-06-11 11:04:15]

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

[commit-bot: I haz the power, 2014-06-11 11:04:16]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[mithro-old, 2014-06-11 11:11:21]

The CQ bit was checked by mithro@mithis.com

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

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mithro@mithis.com/328603002/1

[commit-bot: I haz the power, 2014-06-11 13:27:51]

Change committed as 276378