cc: Introduce BeginFrame sequence numbers and acknowledgements.

cc: Introduce BeginFrame sequence numbers and acknowledgements.

This patch modifies the existing BeginFrameSource::DidFinishFrame() into
a more robust acknowledgment mechanism, through which observers indicate
the results of a BeginFrame message. For this purpose, each BeginFrame
is identified through a source identifier and sequence number.

The acknowledgments are used by the DisplayScheduler to determine when
all updates for a BeginFrame have been received and trigger an early
deadline. This replaces the DisplayScheduler's original heuristics for
expected Surface damage. 

A BeginFrame acknowledgment indicates:
1) completion of a specific BeginFrame by the observer,
2) whether or not the observer produced updates, and
3) the oldest frame that was incorporated into the last update from the
observer.

2) and 3) are in preparation for DevTool's BeginFrameControl, see
http://bit.ly/bfc-v1 and https://codereview.chromium.org/2411793008/.

The patch also updates BeginFrameObservers to reliably call
DidFinishFrame and adds missing acknowledgment paths between renderer,
browser, and services. Further, it wires up the compositor's
BeginFrameSource on Android to RWHVAndroid, making it a
BeginFrameObserver. Compositor-less Android WebView is supported by
moving the BeginFrameSource from CompositorImpl up into WindowAndroid.

DO NOT SUBMIT: Tests missing, DisplaySchedulerTest needs updating.

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

[Eric Seckler, 2016-11-25 16:53:58]

Description was changed from

==========
cc: Introduce BeginFrame sequence numbers and acknowledgements.

DO NOT SUBMIT: Currently only linux; tests need updating.

BUG=
==========

to

==========
cc: Introduce BeginFrame sequence numbers and acknowledgements.

DO NOT SUBMIT: Currently only linux; tests need updating.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[Eric Seckler, 2016-11-25 16:53:58]

Description was changed from

==========
cc: Introduce BeginFrame sequence numbers and acknowledgements.

DO NOT SUBMIT: Currently only linux; tests need updating.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
cc: Introduce BeginFrame sequence numbers and acknowledgements.

DO NOT SUBMIT: Currently only linux; tests need updating.

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

[Eric Seckler, 2016-11-30 18:57:37]

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

[commit-bot: I haz the power, 2016-11-30 18:58:34]

Dry run: 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-11-30 19:02:46]

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

[commit-bot: I haz the power, 2016-11-30 19:02:47]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Eric Seckler, 2016-12-01 09:20:51]

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

[commit-bot: I haz the power, 2016-12-01 09:21:06]

Dry run: 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-01 09:28:37]

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

[commit-bot: I haz the power, 2016-12-01 09:28:38]

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

[Eric Seckler, 2016-12-01 11:20:09]

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

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

Dry run: 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-01 11:28:44]

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

[commit-bot: I haz the power, 2016-12-01 11:28:44]

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

[Eric Seckler, 2016-12-01 11:51:13]

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

[commit-bot: I haz the power, 2016-12-01 11:51:29]

Dry run: 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-01 11:58:59]

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

[commit-bot: I haz the power, 2016-12-01 11:59:00]

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

[Eric Seckler, 2016-12-01 14:49:25]

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

[commit-bot: I haz the power, 2016-12-01 14:49:45]

Dry run: 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-01 14:57:02]

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

[commit-bot: I haz the power, 2016-12-01 14:57:03]

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

[Eric Seckler, 2016-12-01 15:30:06]

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

[commit-bot: I haz the power, 2016-12-01 15:30:20]

Dry run: 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-01 15:38:09]

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

[commit-bot: I haz the power, 2016-12-01 15:38:10]

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

[Eric Seckler, 2016-12-01 16:43:02]

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

[commit-bot: I haz the power, 2016-12-01 16:43:17]

Dry run: 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-01 16:51:12]

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

[commit-bot: I haz the power, 2016-12-01 16:51:13]

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

[Eric Seckler, 2016-12-01 17:44:09]

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

[commit-bot: I haz the power, 2016-12-01 17:44:39]

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-01 17:45:12]

Patchset #10 (id:180001) has been deleted

[Eric Seckler, 2016-12-01 17:45:24]

Patchset #9 (id:160001) has been deleted

[commit-bot: I haz the power, 2016-12-01 17:46:44]

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

[Eric Seckler, 2016-12-01 17:55:44]

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

[commit-bot: I haz the power, 2016-12-01 17:56:03]

Dry run: 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-01 18:05:11]

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

[commit-bot: I haz the power, 2016-12-01 18:05:12]

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

[Eric Seckler, 2016-12-02 08:29:42]

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

[commit-bot: I haz the power, 2016-12-02 08:29:56]

Dry run: 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-02 08:38:22]

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

[commit-bot: I haz the power, 2016-12-02 08:38:23]

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

[Eric Seckler, 2016-12-02 11:22:42]

Patchset #11 (id:240001) has been deleted

[Eric Seckler, 2016-12-02 11:24:06]

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

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

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-02 11:28:39]

Patchset #1 (id:1) has been deleted

[Eric Seckler, 2016-12-02 11:28:58]

Patchset #1 (id:20001) has been deleted

[Eric Seckler, 2016-12-02 11:29:28]

Patchset #1 (id:40001) has been deleted

[Eric Seckler, 2016-12-02 11:29:45]

Patchset #1 (id:60001) has been deleted

[Eric Seckler, 2016-12-02 11:30:01]

Patchset #1 (id:80001) has been deleted

[Eric Seckler, 2016-12-02 11:30:17]

Patchset #1 (id:100001) has been deleted

[Eric Seckler, 2016-12-02 11:31:05]

Patchset #1 (id:120001) has been deleted

[Eric Seckler, 2016-12-02 11:31:20]

Patchset #1 (id:140001) has been deleted

[commit-bot: I haz the power, 2016-12-02 11:32:02]

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

[Eric Seckler, 2016-12-02 11:32:02]

Patchset #1 (id:200001) has been deleted

[commit-bot: I haz the power, 2016-12-02 11:32:02]

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

[Eric Seckler, 2016-12-02 11:32:20]

Patchset #1 (id:220001) has been deleted

[Eric Seckler, 2016-12-02 11:46:33]

Patchset #1 (id:260001) has been deleted

[Eric Seckler, 2016-12-02 11:47:10]

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

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

Dry run: 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-02 11:55:22]

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

[commit-bot: I haz the power, 2016-12-02 11:55:23]

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

[Eric Seckler, 2016-12-02 20:19:28]

Patchset #1 (id:280001) has been deleted

[Eric Seckler, 2016-12-02 20:19:44]

Patchset #1 (id:300001) has been deleted

[Eric Seckler, 2016-12-02 20:19:55]

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

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

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-02 20:23:07]

Description was changed from

==========
cc: Introduce BeginFrame sequence numbers and acknowledgements.

DO NOT SUBMIT: Currently only linux; tests need updating.

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

to

==========
cc: Introduce BeginFrame sequence numbers and acknowledgements.

DO NOT SUBMIT: Tests missing, DisplaySchedulerTests need updating.

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

[commit-bot: I haz the power, 2016-12-02 20:31:33]

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

[commit-bot: I haz the power, 2016-12-02 20:31:34]

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

[Eric Seckler, 2016-12-05 12:39:21]

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

[commit-bot: I haz the power, 2016-12-05 12:39:38]

Dry run: 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-05 12:42:31]

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

[commit-bot: I haz the power, 2016-12-05 12:42:32]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Eric Seckler, 2016-12-05 15:33:29]

Patchset #2 (id:340001) has been deleted

[Eric Seckler, 2016-12-05 15:33:42]

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

[commit-bot: I haz the power, 2016-12-05 15:34:00]

Dry run: 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-05 15:43:59]

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

[commit-bot: I haz the power, 2016-12-05 15:44:00]

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

[Eric Seckler, 2016-12-05 16:23:16]

Description was changed from

==========
cc: Introduce BeginFrame sequence numbers and acknowledgements.

DO NOT SUBMIT: Tests missing, DisplaySchedulerTests need updating.

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

to

==========
cc: Introduce BeginFrame sequence numbers and acknowledgements.

This patch modifies the existing BeginFrameSource::DidFinishFrame() into
a more robust acknowledgment mechanism, through which observers indicate
the results of a BeginFrame message. For this purpose, each BeginFrame
is identified through a source identifier and sequence number.

The acknowledgments are used by the DisplayScheduler to determine when
all updates for a BeginFrame have been received and trigger an early
deadline. This replaces the DisplayScheduler's original heuristics for
expected Surface damage. 

A BeginFrame acknowledgment indicates:
1) completion of a specific BeginFrame by the observer,
2) whether or not the observer produced updates, and
3) the oldest frame that was incorporated into the last update from the
observer.

2) and 3) are in preparation for DevTool's BeginFrameControl, see
http://bit.ly/bfc-v1 and https://codereview.chromium.org/2411793008/.

The patch also updates BeginFrameObservers to reliably call
DidFinishFrame and adds missing acknowledgment paths between renderer,
browser, and services. Further, it wires up the compositor's
BeginFrameSource on Android to RWHVAndroid, making it a
BeginFrameObserver. Compositor-less Android WebView is supported by
moving the BeginFrameSource from CompositorImpl up into WindowAndroid.

DO NOT SUBMIT: Tests missing, DisplaySchedulerTest needs updating.

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

[Eric Seckler, 2016-12-05 16:54:08]

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

[commit-bot: I haz the power, 2016-12-05 16:54:32]

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-05 16:54:41]

Patchset #1 (id:320001) has been deleted

[Eric Seckler, 2016-12-05 16:59:44]

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

[Eric Seckler, 2016-12-05 16:59:46]

Hi Brian,

This is still missing some polish and tests (and I'm hunting some
Android-specific bug), but maybe you could take a look at the overall approach
already?

Let me know if you think we should split out the Android-related changes to
RWHVAndroid/WindowAndroid/CompositorImpl into a separate change.

Thanks!
Eric

[commit-bot: I haz the power, 2016-12-05 17:04:04]

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

[commit-bot: I haz the power, 2016-12-05 17:04:06]

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

[Eric Seckler, 2016-12-06 11:53:58]

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

[commit-bot: I haz the power, 2016-12-06 11:54:18]

Dry run: 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-06 12:02:35]

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

[commit-bot: I haz the power, 2016-12-06 12:02:36]

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

[Sami, 2016-12-06 12:41:04]

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

[Sami, 2016-12-06 12:41:07]

This looks excellent! Thanks for doing the hard work of untangling everything
here! For everyone's sanity splitting this into chunks is probably a good idea
if feasible.

https://codereview.chromium.org/2527283003/diff/380001/cc/blimp/layer_tree_ho...
File cc/blimp/layer_tree_host_remote.h (right):

https://codereview.chromium.org/2527283003/diff/380001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.h:125: int source_frame_number_ = 0;
aside: We might want to drop a TODO somewhere about replacing this with
begin_frame_number_ since I think it fills a similar use.

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

https://codereview.chromium.org/2527283003/diff/380001/cc/output/begin_frame_...
cc/output/begin_frame_args.h:122: bool has_updates;
|oldest_incorporated_frame| is only valid if this is true, right?

https://codereview.chromium.org/2527283003/diff/380001/cc/output/begin_frame_...
cc/output/begin_frame_args.h:131: uint64_t oldest_incorporated_frame;
Do we need to define an invalid/unknown value for this -- say if the BF source
changes and it's no longer meaningful to compare sequence numbers?

https://codereview.chromium.org/2527283003/diff/380001/cc/output/begin_frame_...
cc/output/begin_frame_args.h:134: uint32_t remaining_frames;
nit: might want to order these fields from largest to smallest to help with
packing a bit.

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.cc:60: uint64_t CreateSourceId() {
Fancy :)

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.cc:198: current_sequence_number_++;
Not sure why we don't just always increment this? IIRC that was needed to
distinguish otherwise duplicate frames?

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.cc:298: oldest_incorporated_frames_[obs] =
Does it make sense to look at this if !has_updates?

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.cc:308: current_sequence_number_ > 0 ?
current_sequence_number_ - 1 : 0;
nit: Alternatively we could start sequence numbering from 1 to avoid the edge
case.

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:132: virtual uint64_t source_id() const;
Does this need to be virtual?

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:251: bool ObserversFinishedFrame();
bikeshed: AllObserversFinishedFrame() / AnyObserverHadUpdates()? (also const)

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:265: std::set<BeginFrameObserver*> observers_;
nit: unordered sets?

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:858: current_frame_number_ > 0 ?
current_frame_number_ - 1 : 0;
Ditto about 1-based sequence numbers if we decide to start using them.

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.h:310: // These are used for tracing only.
This comment probably isn't accurate anymore?

https://codereview.chromium.org/2527283003/diff/380001/cc/surfaces/display_be...
File cc/surfaces/display_begin_frame_source.cc (right):

https://codereview.chromium.org/2527283003/diff/380001/cc/surfaces/display_be...
cc/surfaces/display_begin_frame_source.cc:49: }
Are we missing the case where the client stops needing frames?

https://codereview.chromium.org/2527283003/diff/380001/cc/surfaces/display_be...
cc/surfaces/display_begin_frame_source.cc:54: if (!frame_active_)
Is someone calling this at a bad time?

https://codereview.chromium.org/2527283003/diff/380001/cc/surfaces/display_be...
cc/surfaces/display_begin_frame_source.cc:96: if (!observing_begin_frames_) {
Maybe fold this together with the logic in SetClientNeedsBeginFrames?

https://codereview.chromium.org/2527283003/diff/380001/cc/surfaces/display_be...
File cc/surfaces/display_begin_frame_source.h (right):

https://codereview.chromium.org/2527283003/diff/380001/cc/surfaces/display_be...
cc/surfaces/display_begin_frame_source.h:32: std::unique_ptr<BeginFrameSource>
target_source);
Sorry for the bikeshed but "target source" is breaking my brain :) How about
wrapped_source, underlying_source, main_source, original_source or
upstream_source or something like that? Feel free to tell me to get used to
target_source too.

https://codereview.chromium.org/2527283003/diff/380001/cc/surfaces/display_be...
cc/surfaces/display_begin_frame_source.h:41: bool ObserversFinishedFrame();
ditto: AllObservers...?

https://codereview.chromium.org/2527283003/diff/380001/cc/surfaces/display_be...
cc/surfaces/display_begin_frame_source.h:48: void FinishFrame(bool had_updates,
bool has_pending_updates);
Should we call this FinishClientFrame()? I'd like to be explicit since we're
mixing several types of frames here :)

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

https://codereview.chromium.org/2527283003/diff/380001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_android.cc:1942:
TRACE_EVENT0("cc,benchmark", "RenderWidgetHostViewAndroid::OnBeginFrame");
BTW if you see failing perf benchmarks when you land this, someone might be
relying on this being called "OnVSync". I didn't find any code doing that but
just thought I'd mention the possibility.

[Eric Seckler, 2016-12-06 13:45:25]

Patchset #3 (id:400001) has been deleted

[Eric Seckler, 2016-12-06 13:45:59]

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

[commit-bot: I haz the power, 2016-12-06 13:46:18]

Dry run: 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-06 13:56:05]

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

[commit-bot: I haz the power, 2016-12-06 13:56:07]

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

[Eric Seckler, 2016-12-06 17:27:40]

Patchset #3 (id:420001) has been deleted

[Eric Seckler, 2016-12-06 17:28:32]

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

[commit-bot: I haz the power, 2016-12-06 17:29:01]

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-06 17:34:01]

On 2016/12/06 12:41:07, Sami wrote:
> This looks excellent! Thanks for doing the hard work of untangling everything
> here! For everyone's sanity splitting this into chunks is probably a good idea
> if feasible.

Thanks for the comments! I agree, we should split this into portions somehow.
Will look into that once Brian had a peek at the approach :)

https://codereview.chromium.org/2527283003/diff/380001/cc/blimp/layer_tree_ho...
File cc/blimp/layer_tree_host_remote.h (right):

https://codereview.chromium.org/2527283003/diff/380001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.h:125: int source_frame_number_ = 0;
On 2016/12/06 12:41:06, Sami wrote:
> aside: We might want to drop a TODO somewhere about replacing this with
> begin_frame_number_ since I think it fills a similar use.

In general, the LayerTreeHost::SourceFrameNumber() seems to be incremented for
new main frames, rather than a BeginFrame. For LTHRemote that turns out to be
the same as the BeginFrame number used for its BeginFrameArgs creation, but
sign/width of the number are different - which is why I added another field. I'm
adding a TODO to the LTH interface to think about using BeginFrame numbers
instead of "source" frame numbers.

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

https://codereview.chromium.org/2527283003/diff/380001/cc/output/begin_frame_...
cc/output/begin_frame_args.h:122: bool has_updates;
On 2016/12/06 12:41:06, Sami wrote:
> |oldest_incorporated_frame| is only valid if this is true, right?

No :) I updated the comment below with a note, but here a longer explanation:

|oldest_incorporated_frame| remains valid even if there were no updates. There
are two situations where |has_updates| is false:
a) The observer didn't need to produce any updates.
b) The observer has pending updates but didn't manage to produce them (e.g. took
to long).

For a), |oldest_incorporated_frame| is incremented to the current frame (since
the current frame contributed its updates -- there just weren't any).

For b), |oldest_incorporated_frame| remains at the latest frame that
successfully contributed its updates.

If |has_updates| is true, this just means that a surface was indeed damaged / a
CompositorFrame was sent (rather than the frame just "acked").

https://codereview.chromium.org/2527283003/diff/380001/cc/output/begin_frame_...
cc/output/begin_frame_args.h:131: uint64_t oldest_incorporated_frame;
On 2016/12/06 12:41:06, Sami wrote:
> Do we need to define an invalid/unknown value for this -- say if the BF source
> changes and it's no longer meaningful to compare sequence numbers?

My current approach to this is to consider the consumer/observer up to date up
to the last frame of the new source. That way, the observer can indicate that it
is not up to date, but also doesn't propagate an invalid value. I think if we
start sequence numbering at 1, this would naturally work for the first source as
well (|oldest_incorporated_frame| would be initialized to frame 0).

https://codereview.chromium.org/2527283003/diff/380001/cc/output/begin_frame_...
cc/output/begin_frame_args.h:134: uint32_t remaining_frames;
On 2016/12/06 12:41:06, Sami wrote:
> nit: might want to order these fields from largest to smallest to help with
> packing a bit.

Done.

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.cc:60: uint64_t CreateSourceId() {
On 2016/12/06 12:41:06, Sami wrote:
> Fancy :)

I'd call this rather ugly ;)

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.cc:198: current_sequence_number_++;
On 2016/12/06 12:41:06, Sami wrote:
> Not sure why we don't just always increment this? IIRC that was needed to
> distinguish otherwise duplicate frames?

Not only. We also use the number to distinguish which frame an observer
acknowledges in DisplayBFS - to detect when all observers have responded and
then trigger an early deadline in DisplayScheduler.

We may end up here in two slightly different situations:
a) An observer adds themselves for the first time after no-one was listening for
a while. In this case, the missed frame it receives will be a "new" one that
wasn't sent to anyone else yet. In that case, we need a new sequence number.
b) An observer adds themselves while other observers were already listening. In
that case, we should send the same BeginFrame to the new observer (with same
sequence number) that the other observers already received. If we incremented in
that case, the other observers would answer with the old frame's number, even
though they are responding to the latest frame.

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.cc:298: oldest_incorporated_frames_[obs] =
On 2016/12/06 12:41:06, Sami wrote:
> Does it make sense to look at this if !has_updates?

Yup, see comment in begin_frame_args.h.

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.cc:308: current_sequence_number_ > 0 ?
current_sequence_number_ - 1 : 0;
On 2016/12/06 12:41:06, Sami wrote:
> nit: Alternatively we could start sequence numbering from 1 to avoid the edge
> case.

Done.

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:132: virtual uint64_t source_id() const;
On 2016/12/06 12:41:07, Sami wrote:
> Does this need to be virtual?

Yup, for the DelegatingBFS.

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:251: bool ObserversFinishedFrame();
On 2016/12/06 12:41:07, Sami wrote:
> bikeshed: AllObserversFinishedFrame() / AnyObserverHadUpdates()? (also const)

Done.

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:265: std::set<BeginFrameObserver*> observers_;
On 2016/12/06 12:41:06, Sami wrote:
> nit: unordered sets?

That would make the AllObserversFinishedFrame calculation more cumbersome, since
base::STLIncludes() requires ordered sets. Leaving as-is.

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine.cc (right):

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.cc:858: current_frame_number_ > 0 ?
current_frame_number_ - 1 : 0;
On 2016/12/06 12:41:07, Sami wrote:
> Ditto about 1-based sequence numbers if we decide to start using them.

Done.

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_state_machine.h (right):

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/scheduler...
cc/scheduler/scheduler_state_machine.h:310: // These are used for tracing only.
On 2016/12/06 12:41:07, Sami wrote:
> This comment probably isn't accurate anymore?

Done.

https://codereview.chromium.org/2527283003/diff/380001/cc/surfaces/display_be...
File cc/surfaces/display_begin_frame_source.cc (right):

https://codereview.chromium.org/2527283003/diff/380001/cc/surfaces/display_be...
cc/surfaces/display_begin_frame_source.cc:49: }
On 2016/12/06 12:41:07, Sami wrote:
> Are we missing the case where the client stops needing frames?

You're right. Should now be covered in UpdateObservingBeginFrames().

https://codereview.chromium.org/2527283003/diff/380001/cc/surfaces/display_be...
cc/surfaces/display_begin_frame_source.cc:54: if (!frame_active_)
On 2016/12/06 12:41:07, Sami wrote:
> Is someone calling this at a bad time?

Shouldn't :) Changed to a DCHECK.

https://codereview.chromium.org/2527283003/diff/380001/cc/surfaces/display_be...
cc/surfaces/display_begin_frame_source.cc:96: if (!observing_begin_frames_) {
On 2016/12/06 12:41:07, Sami wrote:
> Maybe fold this together with the logic in SetClientNeedsBeginFrames?

Extracted common bits into UpdateObservingBeginFrames() / GetMissedArgs().

https://codereview.chromium.org/2527283003/diff/380001/cc/surfaces/display_be...
File cc/surfaces/display_begin_frame_source.h (right):

https://codereview.chromium.org/2527283003/diff/380001/cc/surfaces/display_be...
cc/surfaces/display_begin_frame_source.h:32: std::unique_ptr<BeginFrameSource>
target_source);
On 2016/12/06 12:41:07, Sami wrote:
> Sorry for the bikeshed but "target source" is breaking my brain :) How about
> wrapped_source, underlying_source, main_source, original_source or
> upstream_source or something like that? Feel free to tell me to get used to
> target_source too.

Done.

https://codereview.chromium.org/2527283003/diff/380001/cc/surfaces/display_be...
cc/surfaces/display_begin_frame_source.h:41: bool ObserversFinishedFrame();
On 2016/12/06 12:41:07, Sami wrote:
> ditto: AllObservers...?

Done.

https://codereview.chromium.org/2527283003/diff/380001/cc/surfaces/display_be...
cc/surfaces/display_begin_frame_source.h:48: void FinishFrame(bool had_updates,
bool has_pending_updates);
On 2016/12/06 12:41:07, Sami wrote:
> Should we call this FinishClientFrame()? I'd like to be explicit since we're
> mixing several types of frames here :)

Done.

[commit-bot: I haz the power, 2016-12-06 17:40:03]

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

[commit-bot: I haz the power, 2016-12-06 17:40:04]

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

[Eric Seckler, 2016-12-07 10:48:08]

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

[Eric Seckler, 2016-12-07 10:48:17]

Patchset #3 (id:440001) has been deleted

[commit-bot: I haz the power, 2016-12-07 10:48:20]

Dry run: 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-07 10:52:25]

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

[commit-bot: I haz the power, 2016-12-07 10:52:27]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Eric Seckler, 2016-12-07 11:03:28]

Patchset #3 (id:460001) has been deleted

[Eric Seckler, 2016-12-07 11:03:40]

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

[commit-bot: I haz the power, 2016-12-07 11:03:57]

Dry run: 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-07 11:14:43]

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

[commit-bot: I haz the power, 2016-12-07 11:14:44]

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

[Eric Seckler, 2016-12-07 13:09:30]

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

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

Dry run: 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-07 13:20:06]

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

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

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

[Eric Seckler, 2016-12-07 15:24:02]

Patchset #3 (id:480001) has been deleted

[Eric Seckler, 2016-12-07 15:24:15]

Patchset #3 (id:500001) has been deleted

[Eric Seckler, 2016-12-07 15:24:29]

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

[commit-bot: I haz the power, 2016-12-07 15:24:46]

Dry run: 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-07 15:35:31]

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

[commit-bot: I haz the power, 2016-12-07 15:35:33]

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

[Eric Seckler, 2016-12-07 16:52:27]

Patchset #1 (id:360001) has been deleted

[Sami, 2016-12-07 16:59:40]

Thanks for the improvements!

Brian, could you have a look too please?

https://codereview.chromium.org/2527283003/diff/380001/cc/blimp/layer_tree_ho...
File cc/blimp/layer_tree_host_remote.h (right):

https://codereview.chromium.org/2527283003/diff/380001/cc/blimp/layer_tree_ho...
cc/blimp/layer_tree_host_remote.h:125: int source_frame_number_ = 0;
On 2016/12/06 17:33:59, Eric Seckler wrote:
> On 2016/12/06 12:41:06, Sami wrote:
> > aside: We might want to drop a TODO somewhere about replacing this with
> > begin_frame_number_ since I think it fills a similar use.
> 
> In general, the LayerTreeHost::SourceFrameNumber() seems to be incremented for
> new main frames, rather than a BeginFrame. For LTHRemote that turns out to be
> the same as the BeginFrame number used for its BeginFrameArgs creation, but
> sign/width of the number are different - which is why I added another field.
I'm
> adding a TODO to the LTH interface to think about using BeginFrame numbers
> instead of "source" frame numbers.

True, I was just thinking that since the only way to get a new main frame is via
a BeginFrame, so another counter doesn't really add any new information --
unless we want main frame numbers to be consecutive for some reason. In any case
I think we can defer this decision.

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

https://codereview.chromium.org/2527283003/diff/380001/cc/output/begin_frame_...
cc/output/begin_frame_args.h:122: bool has_updates;
On 2016/12/06 17:33:59, Eric Seckler wrote:
> On 2016/12/06 12:41:06, Sami wrote:
> > |oldest_incorporated_frame| is only valid if this is true, right?
> 
> No :) I updated the comment below with a note, but here a longer explanation:
> 
> |oldest_incorporated_frame| remains valid even if there were no updates. There
> are two situations where |has_updates| is false:
> a) The observer didn't need to produce any updates.
> b) The observer has pending updates but didn't manage to produce them (e.g.
took
> to long).
> 
> For a), |oldest_incorporated_frame| is incremented to the current frame (since
> the current frame contributed its updates -- there just weren't any).
> 
> For b), |oldest_incorporated_frame| remains at the latest frame that
> successfully contributed its updates.
> 
> If |has_updates| is true, this just means that a surface was indeed damaged /
a
> CompositorFrame was sent (rather than the frame just "acked").

Sorry, I think the explanation is still a bit confusing. The text below says
"Sequence number of the oldest frame that contributed contents to the last
update the consumer sent", but does an ack with has_updates=false count as an
update or not? :) Maybe we need a different word for ack vs. damage?

I agree that distinguishing (a) and (b) feels important. Let's just make it
crystal clear how to do that :)

https://codereview.chromium.org/2527283003/diff/380001/cc/output/begin_frame_...
cc/output/begin_frame_args.h:131: uint64_t oldest_incorporated_frame;
On 2016/12/06 17:33:59, Eric Seckler wrote:
> On 2016/12/06 12:41:06, Sami wrote:
> > Do we need to define an invalid/unknown value for this -- say if the BF
source
> > changes and it's no longer meaningful to compare sequence numbers?
> 
> My current approach to this is to consider the consumer/observer up to date up
> to the last frame of the new source. That way, the observer can indicate that
it
> is not up to date, but also doesn't propagate an invalid value. I think if we
> start sequence numbering at 1, this would naturally work for the first source
as
> well (|oldest_incorporated_frame| would be initialized to frame 0).

Makes sense. If we decide to do that, let's add a named constant for 0 too.

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.cc:60: uint64_t CreateSourceId() {
On 2016/12/06 17:34:00, Eric Seckler wrote:
> On 2016/12/06 12:41:06, Sami wrote:
> > Fancy :)
> 
> I'd call this rather ugly ;)

Let's settle on "elaborate".

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.cc:198: current_sequence_number_++;
On 2016/12/06 17:33:59, Eric Seckler wrote:
> On 2016/12/06 12:41:06, Sami wrote:
> > Not sure why we don't just always increment this? IIRC that was needed to
> > distinguish otherwise duplicate frames?
> 
> Not only. We also use the number to distinguish which frame an observer
> acknowledges in DisplayBFS - to detect when all observers have responded and
> then trigger an early deadline in DisplayScheduler.
> 
> We may end up here in two slightly different situations:
> a) An observer adds themselves for the first time after no-one was listening
for
> a while. In this case, the missed frame it receives will be a "new" one that
> wasn't sent to anyone else yet. In that case, we need a new sequence number.
> b) An observer adds themselves while other observers were already listening.
In
> that case, we should send the same BeginFrame to the new observer (with same
> sequence number) that the other observers already received. If we incremented
in
> that case, the other observers would answer with the old frame's number, even
> though they are responding to the latest frame.

Thanks for the explanation, that makes sense. However I'm not sure if comparing
against current_begin_frame_time_ is equivalent to what you wrote. Could we
instead more explicitly check what the state it at the time the observer was
added? In any case I think this deserves a comment :)

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:132: virtual uint64_t source_id() const;
On 2016/12/06 17:34:00, Eric Seckler wrote:
> On 2016/12/06 12:41:07, Sami wrote:
> > Does this need to be virtual?
> 
> Yup, for the DelegatingBFS.

Oh I see, it needs to lie about its id.

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:265: std::set<BeginFrameObserver*> observers_;
On 2016/12/06 17:34:00, Eric Seckler wrote:
> On 2016/12/06 12:41:06, Sami wrote:
> > nit: unordered sets?
> 
> That would make the AllObserversFinishedFrame calculation more cumbersome,
since
> base::STLIncludes() requires ordered sets. Leaving as-is.

Alright.

https://codereview.chromium.org/2527283003/diff/520001/cc/output/begin_frame_...
File cc/output/begin_frame_args.cc (right):

https://codereview.chromium.org/2527283003/diff/520001/cc/output/begin_frame_...
cc/output/begin_frame_args.cc:43: sequence_number(sequence_number),
DCHECK(sequence_number > 0)?

https://codereview.chromium.org/2527283003/diff/520001/cc/output/begin_frame_...
cc/output/begin_frame_args.cc:122: has_updates(has_updates) {}
DCHECK(sequence_number > 0)?

https://codereview.chromium.org/2527283003/diff/520001/cc/scheduler/begin_fra...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/2527283003/diff/520001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:118: virtual void
DidFinishFrame(BeginFrameObserver* obs,
Idle thought: should we have some debug safeguard that makes sure this is
actually called? Of course it's possible that the renderer crashes and we never
hear back.

https://codereview.chromium.org/2527283003/diff/520001/content/browser/render...
File content/browser/renderer_host/browser_compositor_view_mac.mm (right):

https://codereview.chromium.org/2527283003/diff/520001/content/browser/render...
content/browser/renderer_host/browser_compositor_view_mac.mm:489:
args.sequence_number > 0 ? args.sequence_number - 1 : 0;
Zero-check not needed anymore? (Could be a DCHECK still I guess)

https://codereview.chromium.org/2527283003/diff/520001/content/browser/render...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/2527283003/diff/520001/content/browser/render...
content/browser/renderer_host/compositor_impl_android.cc:658:
std::unique_ptr<cc::DelegatingBeginFrameSource> begin_frame_source(
Dumb question: why do we need delegation here instead of just using the original
source directly?

https://codereview.chromium.org/2527283003/diff/520001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/2527283003/diff/520001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.h:170: void
OnBeginFrameDidNotDraw(cc::BeginFrameAck ack) override;
nit: const ref&

[Eric Seckler, 2016-12-08 12:23:55]

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

[commit-bot: I haz the power, 2016-12-08 12:24:04]

Dry run: 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-08 12:32:19]

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

[commit-bot: I haz the power, 2016-12-08 12:32:21]

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

[Eric Seckler, 2016-12-08 13:09:00]

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

[commit-bot: I haz the power, 2016-12-08 13:09:11]

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-08 13:11:16]

Patchset #4 (id:560001) has been deleted

[commit-bot: I haz the power, 2016-12-08 13:12:05]

Dry run: Try jobs failed on following builders:
  linux_trusty_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_trusty_blink_rel/b...)
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Eric Seckler, 2016-12-08 13:13:47]

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

[commit-bot: I haz the power, 2016-12-08 13:13:55]

Dry run: 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-08 13:21:39]

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

[commit-bot: I haz the power, 2016-12-08 13:21:40]

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

[Eric Seckler, 2016-12-08 17:39:53]

Patchset #3 (id:540001) has been deleted

[Eric Seckler, 2016-12-08 17:54:29]

The new DCHECKs still fail some additional tests - I think primarily b/c some of
them create CompositorFrames without begin_frame_numbers. Will look into that
tomorrow.

I also realized that Mus doesn't hook up the window server's BeginFrameSource to
the browser's cc::Display. I've prototyped how we could solve that in
GpuProcessTransportFactory. We'll see how many things break if we do that.

In Mus, the browser's cc::Display submits damages as OutputSurfaceFrame to
MusBrowserCompositorOutputSurface, which submits a CompositorFrame to a
WindowCompositorFrameSink and eventually to the window server's cc::Display.
Because we include the BeginFrameAck for a frame with damages in the
CompositorFrame, this setup also requires us to pass on the ack through the
OutputSurfaceFrame via MusBCOS.

Given the additional complexity that is required to package the BeginFrameAck
into CompositorFrame / OutputSurfaceFrame, I'm considering if it would make more
sense to send explicit BeginFrameAcks as separate IPCs (after submitting the
CompositorFrame) instead. That would decouple the BeginFrameAck logic from the
damage submission a little more, at the cost of additional IPCs. However, it
might actually be more complicated to sort out the ordering of the
SwapCompositorFrame + BeginFrameAck IPCs in that case. WDYT?

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

https://codereview.chromium.org/2527283003/diff/380001/cc/output/begin_frame_...
cc/output/begin_frame_args.h:122: bool has_updates;
On 2016/12/07 16:59:40, Sami wrote:
> On 2016/12/06 17:33:59, Eric Seckler wrote:
> > On 2016/12/06 12:41:06, Sami wrote:
> > > |oldest_incorporated_frame| is only valid if this is true, right?
> > 
> > No :) I updated the comment below with a note, but here a longer
explanation:
> > 
> > |oldest_incorporated_frame| remains valid even if there were no updates.
There
> > are two situations where |has_updates| is false:
> > a) The observer didn't need to produce any updates.
> > b) The observer has pending updates but didn't manage to produce them (e.g.
> took
> > to long).
> > 
> > For a), |oldest_incorporated_frame| is incremented to the current frame
(since
> > the current frame contributed its updates -- there just weren't any).
> > 
> > For b), |oldest_incorporated_frame| remains at the latest frame that
> > successfully contributed its updates.
> > 
> > If |has_updates| is true, this just means that a surface was indeed damaged
/
> a
> > CompositorFrame was sent (rather than the frame just "acked").
> 
> Sorry, I think the explanation is still a bit confusing. The text below says
> "Sequence number of the oldest frame that contributed contents to the last
> update the consumer sent", but does an ack with has_updates=false count as an
> update or not? :) Maybe we need a different word for ack vs. damage?
> 
> I agree that distinguishing (a) and (b) feels important. Let's just make it
> crystal clear how to do that :)

Renamed and updated the comment again after discussion with Sami: We're now
calling this the |latest_confirmed_frame|, which is the last BeginFrame for
which all pending updates successfully led to damages - or where no pending
updates existed (and thus, no damages needed to be produced). We also replaced
|has_updates| by |has_damages| to distinguish better between pending updates and
resulting damages.

https://codereview.chromium.org/2527283003/diff/380001/cc/output/begin_frame_...
cc/output/begin_frame_args.h:131: uint64_t oldest_incorporated_frame;
On 2016/12/07 16:59:40, Sami wrote:
> On 2016/12/06 17:33:59, Eric Seckler wrote:
> > On 2016/12/06 12:41:06, Sami wrote:
> > > Do we need to define an invalid/unknown value for this -- say if the BF
> source
> > > changes and it's no longer meaningful to compare sequence numbers?
> > 
> > My current approach to this is to consider the consumer/observer up to date
up
> > to the last frame of the new source. That way, the observer can indicate
that
> it
> > is not up to date, but also doesn't propagate an invalid value. I think if
we
> > start sequence numbering at 1, this would naturally work for the first
source
> as
> > well (|oldest_incorporated_frame| would be initialized to frame 0).
> 
> Makes sense. If we decide to do that, let's add a named constant for 0 too.

Done, also for 1 as starting frame number.

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.cc:198: current_sequence_number_++;
On 2016/12/07 16:59:40, Sami wrote:
> On 2016/12/06 17:33:59, Eric Seckler wrote:
> > On 2016/12/06 12:41:06, Sami wrote:
> > > Not sure why we don't just always increment this? IIRC that was needed to
> > > distinguish otherwise duplicate frames?
> > 
> > Not only. We also use the number to distinguish which frame an observer
> > acknowledges in DisplayBFS - to detect when all observers have responded and
> > then trigger an early deadline in DisplayScheduler.
> > 
> > We may end up here in two slightly different situations:
> > a) An observer adds themselves for the first time after no-one was listening
> for
> > a while. In this case, the missed frame it receives will be a "new" one that
> > wasn't sent to anyone else yet. In that case, we need a new sequence number.
> > b) An observer adds themselves while other observers were already listening.
> In
> > that case, we should send the same BeginFrame to the new observer (with same
> > sequence number) that the other observers already received. If we
incremented
> in
> > that case, the other observers would answer with the old frame's number,
even
> > though they are responding to the latest frame.
> 
> Thanks for the explanation, that makes sense. However I'm not sure if
comparing
> against current_begin_frame_time_ is equivalent to what you wrote. Could we
> instead more explicitly check what the state it at the time the observer was
> added? In any case I think this deserves a comment :)

Updated the code & comment. I think it's a bit clearer now...

We only discover the state ("is the last frame we sent still active?") during
AddObserver(), so the only way to figure out if we should send that same frame
or a different one is to compare frame times there. I'm now also checking that
the interval hasn't changed between frames to ensure that we don't send
BeginFrames with different intervals and the same sequence number.

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/2527283003/diff/380001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:132: virtual uint64_t source_id() const;
On 2016/12/07 16:59:40, Sami wrote:
> On 2016/12/06 17:34:00, Eric Seckler wrote:
> > On 2016/12/06 12:41:07, Sami wrote:
> > > Does this need to be virtual?
> > 
> > Yup, for the DelegatingBFS.
> 
> Oh I see, it needs to lie about its id.

Actually, given that the DelegatingBFS doesn't produce BeginFrames, and I don't
support the source_id() accessor for ExternalBFS or DisplayBFS, I'm getting rid
of the virtual here. source_id() should only be used to create BeginFrames.
Updated the comment, too.

https://codereview.chromium.org/2527283003/diff/520001/cc/output/begin_frame_...
File cc/output/begin_frame_args.cc (right):

https://codereview.chromium.org/2527283003/diff/520001/cc/output/begin_frame_...
cc/output/begin_frame_args.cc:43: sequence_number(sequence_number),
On 2016/12/07 16:59:40, Sami wrote:
> DCHECK(sequence_number > 0)?

Done.

https://codereview.chromium.org/2527283003/diff/520001/cc/output/begin_frame_...
cc/output/begin_frame_args.cc:122: has_updates(has_updates) {}
On 2016/12/07 16:59:40, Sami wrote:
> DCHECK(sequence_number > 0)?

Done.

https://codereview.chromium.org/2527283003/diff/520001/cc/scheduler/begin_fra...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/2527283003/diff/520001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:118: virtual void
DidFinishFrame(BeginFrameObserver* obs,
On 2016/12/07 16:59:40, Sami wrote:
> Idle thought: should we have some debug safeguard that makes sure this is
> actually called? Of course it's possible that the renderer crashes and we
never
> hear back.

Good question. Not sure if a check is easily enforceable, since it could take a
while for the ack to come back (even post the next BeginFrame).

https://codereview.chromium.org/2527283003/diff/520001/content/browser/render...
File content/browser/renderer_host/browser_compositor_view_mac.mm (right):

https://codereview.chromium.org/2527283003/diff/520001/content/browser/render...
content/browser/renderer_host/browser_compositor_view_mac.mm:489:
args.sequence_number > 0 ? args.sequence_number - 1 : 0;
On 2016/12/07 16:59:40, Sami wrote:
> Zero-check not needed anymore? (Could be a DCHECK still I guess)

Done, also in RWHVAura.

https://codereview.chromium.org/2527283003/diff/520001/content/browser/render...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/2527283003/diff/520001/content/browser/render...
content/browser/renderer_host/compositor_impl_android.cc:658:
std::unique_ptr<cc::DelegatingBeginFrameSource> begin_frame_source(
On 2016/12/07 16:59:40, Sami wrote:
> Dumb question: why do we need delegation here instead of just using the
original
> source directly?

I thought this is the simplest solution to the ownership problems we'd otherwise
have. (The Display currently wants ownership of the source.)

https://codereview.chromium.org/2527283003/diff/520001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.h (right):

https://codereview.chromium.org/2527283003/diff/520001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.h:170: void
OnBeginFrameDidNotDraw(cc::BeginFrameAck ack) override;
On 2016/12/07 16:59:40, Sami wrote:
> nit: const ref&

Done.

[Eric Seckler, 2016-12-08 17:55:55]

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

[commit-bot: I haz the power, 2016-12-08 17:56:18]

Dry run: 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-08 18:10:22]

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

[commit-bot: I haz the power, 2016-12-08 18:10:24]

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

[brianderson, 2016-12-09 01:08:11]

A large patch, so I only looked at the header files I'm most familiar with so
far. I like the way this patch is headed though!

https://codereview.chromium.org/2527283003/diff/520001/content/browser/render...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/2527283003/diff/520001/content/browser/render...
content/browser/renderer_host/compositor_impl_android.cc:658:
std::unique_ptr<cc::DelegatingBeginFrameSource> begin_frame_source(
On 2016/12/08 17:54:28, Eric Seckler wrote:
> On 2016/12/07 16:59:40, Sami wrote:
> > Dumb question: why do we need delegation here instead of just using the
> original
> > source directly?
> 
> I thought this is the simplest solution to the ownership problems we'd
otherwise
> have. (The Display currently wants ownership of the source.)

Is it hard / or non sensical to move ownership up a level on platforms other
than Android?

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

https://codereview.chromium.org/2527283003/diff/600001/cc/output/begin_frame_...
cc/output/begin_frame_args.h:117: // Source identifier of the BeginFrame that is
acknowledged.
In this comment, can you explain why the source_id is needed?

https://codereview.chromium.org/2527283003/diff/600001/cc/scheduler/begin_fra...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/2527283003/diff/600001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:33: // greater than or equal to the previous
call.
To "Further, |args|.frame_time is guaranteed to be greater than or equal to the
previous call", can you add: "even if the source_id changes.".

I'm assuming that's still true after this patch?

https://codereview.chromium.org/2527283003/diff/600001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:131: // process, and very likely unique across
processes. This is used to create
"Guaranteed unique within a process, but not across processes." since we don't
want to rely on that.

https://codereview.chromium.org/2527283003/diff/600001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:268: bool observers_had_damages_;
observers_had_damage_

https://codereview.chromium.org/2527283003/diff/600001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:309: BeginFrameObserverAckTracker
ack_tracker_;
For my education, is this something that is actually used by this class at ToT?
i.e. do we ever have more than one observer for an ExternalBeginFrameSource?

You don't have to remove this. I'm just wondering if anything has changed since
I last looked at the code.

If it's dead code, maybe we should revisit how soon it'll become alive.

https://codereview.chromium.org/2527283003/diff/600001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:315: // A BeginFrameSource that delegates to
another given source.
Can you add a comment explaining why this class is needed as opposed to using
the delegate directly.

[Eric Seckler, 2016-12-09 14:51:43]

Description was changed from

==========
cc: Introduce BeginFrame sequence numbers and acknowledgements.

This patch modifies the existing BeginFrameSource::DidFinishFrame() into
a more robust acknowledgment mechanism, through which observers indicate
the results of a BeginFrame message. For this purpose, each BeginFrame
is identified through a source identifier and sequence number.

The acknowledgments are used by the DisplayScheduler to determine when
all updates for a BeginFrame have been received and trigger an early
deadline. This replaces the DisplayScheduler's original heuristics for
expected Surface damage. 

A BeginFrame acknowledgment indicates:
1) completion of a specific BeginFrame by the observer,
2) whether or not the observer produced updates, and
3) the oldest frame that was incorporated into the last update from the
observer.

2) and 3) are in preparation for DevTool's BeginFrameControl, see
http://bit.ly/bfc-v1 and https://codereview.chromium.org/2411793008/.

The patch also updates BeginFrameObservers to reliably call
DidFinishFrame and adds missing acknowledgment paths between renderer,
browser, and services. Further, it wires up the compositor's
BeginFrameSource on Android to RWHVAndroid, making it a
BeginFrameObserver. Compositor-less Android WebView is supported by
moving the BeginFrameSource from CompositorImpl up into WindowAndroid.

DO NOT SUBMIT: Tests missing, DisplaySchedulerTest needs updating.

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

to

==========
cc: Introduce BeginFrame sequence numbers and acknowledgements.

This patch modifies the existing BeginFrameSource::DidFinishFrame() into
a more robust acknowledgment mechanism, through which observers indicate
the results of a BeginFrame message. For this purpose, each BeginFrame
is identified through a source identifier and sequence number.

The acknowledgments are used by the DisplayScheduler to determine when
all updates for a BeginFrame have been received and trigger an early
deadline. This replaces the DisplayScheduler's original heuristics for
expected Surface damage. 

A BeginFrame acknowledgment indicates:
1) completion of a specific BeginFrame by the observer,
2) whether or not the observer produced updates, and
3) the oldest frame that was incorporated into the last update from the
observer.

2) and 3) are in preparation for DevTool's BeginFrameControl, see
http://bit.ly/bfc-v1 and https://codereview.chromium.org/2411793008/.

The patch also updates BeginFrameObservers to reliably call
DidFinishFrame and adds missing acknowledgment paths between renderer,
browser, and services. Further, it wires up the compositor's
BeginFrameSource on Android to RWHVAndroid, making it a
BeginFrameObserver. Compositor-less Android WebView is supported by
moving the BeginFrameSource from CompositorImpl up into WindowAndroid.

DO NOT SUBMIT: Tests missing, DisplaySchedulerTest needs updating.

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

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

Thanks, Brian! :) If you don't have any high-level objections, I'm going to
split this up into smaller chunks. I guess we can figure out the details in
those patches then.

I'll start with the Display's BeginFrameSource ownership and the changes to
BeginFrameSource hookup in Android and Mus. Then sequence numbers, then acks
(probably in multiple patches), and eventually the DisplayScheduler changes.

https://codereview.chromium.org/2527283003/diff/520001/content/browser/render...
File content/browser/renderer_host/compositor_impl_android.cc (right):

https://codereview.chromium.org/2527283003/diff/520001/content/browser/render...
content/browser/renderer_host/compositor_impl_android.cc:658:
std::unique_ptr<cc::DelegatingBeginFrameSource> begin_frame_source(
On 2016/12/09 01:08:11, brianderson wrote:
> On 2016/12/08 17:54:28, Eric Seckler wrote:
> > On 2016/12/07 16:59:40, Sami wrote:
> > > Dumb question: why do we need delegation here instead of just using the
> > original
> > > source directly?
> > 
> > I thought this is the simplest solution to the ownership problems we'd
> otherwise
> > have. (The Display currently wants ownership of the source.)
> 
> Is it hard / or non sensical to move ownership up a level on platforms other
> than Android?

I think you're right, that might be a better solution. I'm sending a patch that
does this, here: https://codereview.chromium.org/2565783002/.

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

https://codereview.chromium.org/2527283003/diff/600001/cc/output/begin_frame_...
cc/output/begin_frame_args.h:117: // Source identifier of the BeginFrame that is
acknowledged.
On 2016/12/09 01:08:11, brianderson wrote:
> In this comment, can you explain why the source_id is needed?

Done. Also added a comment to BeginFrameArgs.

https://codereview.chromium.org/2527283003/diff/600001/cc/scheduler/begin_fra...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/2527283003/diff/600001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:33: // greater than or equal to the previous
call.
On 2016/12/09 01:08:11, brianderson wrote:
> To "Further, |args|.frame_time is guaranteed to be greater than or equal to
the
> previous call", can you add: "even if the source_id changes.".
> 
> I'm assuming that's still true after this patch?

Yes, done!

https://codereview.chromium.org/2527283003/diff/600001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:131: // process, and very likely unique across
processes. This is used to create
On 2016/12/09 01:08:11, brianderson wrote:
> "Guaranteed unique within a process, but not across processes." since we don't
> want to rely on that.

Okay. I guess the question is whether there may be a situation where the
"origin" BeginFrameSource (the one that initiates the BeginFrames) of an
observer might be changed to a different one in a different process. But this
shouldn't happen, since those BeginFrameSources typically only live in the
browser process (or the window server in Mus), right?

If we agree that we don't need cross-process identities, I'll get rid of the
process id mangling in CreateSourceId().

https://codereview.chromium.org/2527283003/diff/600001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:268: bool observers_had_damages_;
On 2016/12/09 01:08:11, brianderson wrote:
> observers_had_damage_

Done.

https://codereview.chromium.org/2527283003/diff/600001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:309: BeginFrameObserverAckTracker
ack_tracker_;
On 2016/12/09 01:08:11, brianderson wrote:
> For my education, is this something that is actually used by this class at
ToT?
> i.e. do we ever have more than one observer for an ExternalBeginFrameSource?
> 
> You don't have to remove this. I'm just wondering if anything has changed
since
> I last looked at the code.
> 
> If it's dead code, maybe we should revisit how soon it'll become alive.

Mus uses ExternalBeginFrameSources in WindowCompositorFrameSink, which (aren't
currently but) should be hooked up to a Display. So I'd say, yeah, there'll be
such a use, at least until Mus get's rid of the additional Display in the
browser.

https://codereview.chromium.org/2527283003/diff/600001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:315: // A BeginFrameSource that delegates to
another given source.
On 2016/12/09 01:08:11, brianderson wrote:
> Can you add a comment explaining why this class is needed as opposed to using
> the delegate directly.

I think moving the ownership of the BeginFrameSource out of Display would render
the DelegatingBFS unnecessary. I'll get rid of it then.

[Sami, 2016-12-09 18:36:37]

+1 to the idea of adjusting the ownership so delegation isn't needed.

https://codereview.chromium.org/2527283003/diff/600001/cc/scheduler/begin_fra...
File cc/scheduler/begin_frame_source.h (right):

https://codereview.chromium.org/2527283003/diff/600001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:268: bool observers_had_damages_;
On 2016/12/09 16:47:17, Eric Seckler wrote:
> On 2016/12/09 01:08:11, brianderson wrote:
> > observers_had_damage_
> 
> Done.

Should we do a s/damages/damage/g overall?

[Eric Seckler, 2016-12-12 10:05:27]

On 2016/12/09 18:36:37, Sami wrote:
> Should we do a s/damages/damage/g overall?

Done.