DelegatedFrameHost should not create an ExternalBeginFrameSource

DelegatedFrameHost should not create an ExternalBeginFrameSource

There is currently some unnecessary complexity in the code because
DelegatedFrameHost creates an ExternalBeginFrameSource which
its client use to get BeginFrames instead of sending BeginFrames
directly to its clients.

Review-Url: https://codereview.chromium.org/2776973002
Cr-Commit-Position: refs/heads/master@{#459863}
Committed: https://chromium.googlesource.com/chromium/src/+/ba666778f77e419a4983f1a7b9e59d7bef0654d9

[Saman Sami, 2017-03-26 22:02:23]

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

[commit-bot: I haz the power, 2017-03-26 22:02:38]

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

[Saman Sami, 2017-03-26 22:04:38]

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

[commit-bot: I haz the power, 2017-03-26 22:04:45]

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

[Saman Sami, 2017-03-26 22:14:34]

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

[commit-bot: I haz the power, 2017-03-26 22:14:44]

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

[Saman Sami, 2017-03-26 22:30:47]

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

[commit-bot: I haz the power, 2017-03-26 22:31: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, 2017-03-26 22:53:02]

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

[commit-bot: I haz the power, 2017-03-26 22:53:03]

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

[Saman Sami, 2017-03-26 22:59:28]

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

[commit-bot: I haz the power, 2017-03-26 22:59:32]

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, 2017-03-26 23:59:52]

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

[commit-bot: I haz the power, 2017-03-26 23:59:52]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2017-03-27 11:43:01]

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

[commit-bot: I haz the power, 2017-03-27 11:43:13]

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

[Saman Sami, 2017-03-27 11:50:56]

Description was changed from

==========
Test
==========

to

==========
DelegatedFrameHost should not create an ExternalBeginFrameSource

There is currently some unnecessary complexity in the code because
DelegatedFrameHost creates an ExternalBeginFrameSource which
its client use to get BeginFrames instead of sending BeginFrames
directly to its clients.
==========

[Saman Sami, 2017-03-27 12:04:18]

samans@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Saman Sami, 2017-03-27 12:04:28]

PTAL

[Saman Sami, 2017-03-27 12:36:26]

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

[commit-bot: I haz the power, 2017-03-27 12:38:23]

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

[commit-bot: I haz the power, 2017-03-27 12:38:24]

Dry run: This issue passed the CQ dry run.

[Fady Samuel, 2017-03-27 12:45:35]

LGTM! Yay for cleanups! Please pass along to piman@ for final review! Thanks!

[Saman Sami, 2017-03-27 12:47:27]

samans@chromium.org changed reviewers:
+ piman@chromium.org

[Saman Sami, 2017-03-27 12:49:57]

Thanks for the LGTM!
eseckler@: Can you please check that I haven't broken BeginFrames?
piman@: Please take a look.

[Eric Seckler, 2017-03-27 13:05:03]

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

https://codereview.chromium.org/2776973002/diff/100001/content/browser/render...
content/browser/renderer_host/delegated_frame_host.cc:238: if (ack.source_id !=
latest_confirmed_begin_frame_source_id_) {
The code that was previously OnDidFinishFrame() still needs to be executed after
SwapDelegatedFrame, so that
latest_confirmed_begin_frame_source_id_/sequence_number_ are updated.

You'll probably need to keep OnDidFinishFrame (feel free to rename), which you
call from here and after SwapDelegatedFrame.

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

https://codereview.chromium.org/2776973002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:658:
delegated_frame_host_->SetNeedsBeginFrames(needs_begin_frames_ ||
I'd prefer to keep UpdateNeedsBeginFramesInternal, so that we define this
condition (needs_begin_frames_ || needs_flush_input_) in one place only :)

[Saman Sami, 2017-03-27 16:56:57]

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

[commit-bot: I haz the power, 2017-03-27 16:57:42]

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

[Saman Sami, 2017-03-27 17:04:33]

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

[commit-bot: I haz the power, 2017-03-27 17:05:09]

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

[Saman Sami, 2017-03-27 17:10:23]

PTAL

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

https://codereview.chromium.org/2776973002/diff/100001/content/browser/render...
content/browser/renderer_host/delegated_frame_host.cc:238: if (ack.source_id !=
latest_confirmed_begin_frame_source_id_) {
On 2017/03/27 13:05:03, Eric Seckler wrote:
> The code that was previously OnDidFinishFrame() still needs to be executed
after
> SwapDelegatedFrame, so that
> latest_confirmed_begin_frame_source_id_/sequence_number_ are updated.
> 
> You'll probably need to keep OnDidFinishFrame (feel free to rename), which you
> call from here and after SwapDelegatedFrame.

Done.

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

https://codereview.chromium.org/2776973002/diff/100001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:658:
delegated_frame_host_->SetNeedsBeginFrames(needs_begin_frames_ ||
On 2017/03/27 13:05:03, Eric Seckler wrote:
> I'd prefer to keep UpdateNeedsBeginFramesInternal, so that we define this
> condition (needs_begin_frames_ || needs_flush_input_) in one place only :)

Done.

[piman, 2017-03-27 18:08:46]

lgtm

[commit-bot: I haz the power, 2017-03-27 18:15:46]

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

[commit-bot: I haz the power, 2017-03-27 18:15:47]

Dry run: This issue passed the CQ dry run.

[Eric Seckler, 2017-03-27 18:16:09]

lgtm % nit. Thanks for simplifying this!

https://codereview.chromium.org/2776973002/diff/160001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2776973002/diff/160001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:919:
delegated_frame_host_->DidFinishFrame(ack);
Nit: I'd keep DelegatedFrameHost::BeginFrameDidNotSwap (and make
DFH::DidFinishFrame private), otherwise it's not very clear in RWHVA why we
don't call DidFinishFrame after SwapDelegatedFrame from RWHVA.

[Saman Sami, 2017-03-27 19:02:35]

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

[commit-bot: I haz the power, 2017-03-27 19:03: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, 2017-03-27 19:51:22]

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

[commit-bot: I haz the power, 2017-03-27 19:51:23]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2017-03-27 19:55:46]

The CQ bit was checked by samans@chromium.org

[Saman Sami, 2017-03-27 19:55:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, eseckler@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2776973002/#ps180001
(title: "c")

[commit-bot: I haz the power, 2017-03-27 19:56:22]

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-03-27 20:05:41]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1490644546776630,
"parent_rev": "4347e0851af0acb9c6e0d5c17cec03ef6a167749", "commit_rev":
"ba666778f77e419a4983f1a7b9e59d7bef0654d9"}

[commit-bot: I haz the power, 2017-03-27 20:06:25]

Description was changed from

==========
DelegatedFrameHost should not create an ExternalBeginFrameSource

There is currently some unnecessary complexity in the code because
DelegatedFrameHost creates an ExternalBeginFrameSource which
its client use to get BeginFrames instead of sending BeginFrames
directly to its clients.
==========

to

==========
DelegatedFrameHost should not create an ExternalBeginFrameSource

There is currently some unnecessary complexity in the code because
DelegatedFrameHost creates an ExternalBeginFrameSource which
its client use to get BeginFrames instead of sending BeginFrames
directly to its clients.

Review-Url: https://codereview.chromium.org/2776973002
Cr-Commit-Position: refs/heads/master@{#459863}
Committed:
https://chromium.googlesource.com/chromium/src/+/ba666778f77e419a4983f1a7b9e5...
==========

[commit-bot: I haz the power, 2017-03-27 20:06:26]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/ba666778f77e419a4983f1a7b9e5...