Fold BeginFrameSource::SetClientReady into AddObserver

Fold BeginFrameSource::SetClientReady into AddObserver

As a part of https://codereview.chromium.org/1774323003, it became clear
that it needed to be possible to call SetClientReady repeatedly, which
some sources didn't support.

Instead of just making that part of the contract, instead fold the two
places where SetClientReady mattered into AddObserver (the first time
where a begin frame could actually be delivered) and remove
SetClientReady from the API.

R=skyostil@chromium.org,sunnyps@chomium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/de6bfca79029c410d2e7c58173e2119fd485cc28
Cr-Commit-Position: refs/heads/master@{#381612}

[enne (OOO), 2016-03-09 22:28:02]

Description was changed from

==========
Fold BeginFrameSource::SetClientReady into AddObserver

As a part of https://codereview.chromium.org/1774323003, it became clear
that it needed to be possible to call SetClientReady repeatedly, which
some sources didn't support.

Instead of just making that part of the contract, instead fold the two
places where SetClientReady mattered into AddObserver (the first time
where a begin frame could actually be delivered) and remove
SetClientReady from the API.

R=skyostil@chromium.org,sunnyps@chomium.org
==========

to

==========
Fold BeginFrameSource::SetClientReady into AddObserver

As a part of https://codereview.chromium.org/1774323003, it became clear
that it needed to be possible to call SetClientReady repeatedly, which
some sources didn't support.

Instead of just making that part of the contract, instead fold the two
places where SetClientReady mattered into AddObserver (the first time
where a begin frame could actually be delivered) and remove
SetClientReady from the API.

R=skyostil@chromium.org,sunnyps@chomium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Sami, 2016-03-10 19:35:19]

lgtm.

[sunnyps, 2016-03-10 22:43:57]

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

[sunnyps, 2016-03-10 22:43:58]

Actually SetClientReady is used for one-time initialization for the renderer's
external begin frame source and maybe for webview too. The idea is that the
begin frame source is created on the main thread and given to the impl thread
where SetClientReady should be called after it has been attached to the
scheduler. I think it should be the responsibility of the Proxy to do that and
this doesn't belong in the BeginFrameSource interface IMO. Can we remove it from
there and just have it in the BeginFrameSource subclasses that require it?

[danakj, 2016-03-10 22:45:56]

On Thu, Mar 10, 2016 at 2:43 PM, <sunnyps@chromium.org> wrote:

> Actually SetClientReady is used for one-time initialization for the
> renderer's
> external begin frame source and maybe for webview too. The idea is that the
> begin frame source is created on the main thread and given to the impl
> thread
> where SetClientReady should be called after it has been attached to the
> scheduler. I think it should be the responsibility of the Proxy to do that
> and
> this doesn't belong in the BeginFrameSource interface IMO. Can we remove
> it from
> there and just have it in the BeginFrameSource subclasses that require it?
>

That makes the 3rd way to tell the compositor to start working we've built.
We've had SetVisible, SetLayerTreeHostClientReady, and I guess
SetClientReady.

Can we remove it and just not SetVisible(true) until ready (the same way we
got rid of SetLTHClientReady)?


>
> https://codereview.chromium.org/1778853003/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[enne (OOO), 2016-03-10 22:55:32]

On 2016/03/10 at 22:43:58, sunnyps wrote:
> Actually SetClientReady is used for one-time initialization for the renderer's
external begin frame source and maybe for webview too. The idea is that the
begin frame source is created on the main thread and given to the impl thread
where SetClientReady should be called after it has been attached to the
scheduler. I think it should be the responsibility of the Proxy to do that and
this doesn't belong in the BeginFrameSource interface IMO. Can we remove it from
there and just have it in the BeginFrameSource subclasses that require it?

As far as I understand what you're getting at, this is still what this patch
does.  AddObserver is not called on a begin frame source until it's been
attached to the scheduler, at least in practice.  I think it would be incorrect
to add an observer to a begin frame source on a different thread.

I am a little confused about what you are proposing.  What code should move to
where?

[danakj, 2016-03-10 22:55:54]

Description was changed from

==========
Fold BeginFrameSource::SetClientReady into AddObserver

As a part of https://codereview.chromium.org/1774323003, it became clear
that it needed to be possible to call SetClientReady repeatedly, which
some sources didn't support.

Instead of just making that part of the contract, instead fold the two
places where SetClientReady mattered into AddObserver (the first time
where a begin frame could actually be delivered) and remove
SetClientReady from the API.

R=skyostil@chromium.org,sunnyps@chomium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fold BeginFrameSource::SetClientReady into AddObserver

As a part of https://codereview.chromium.org/1774323003, it became clear
that it needed to be possible to call SetClientReady repeatedly, which
some sources didn't support.

Instead of just making that part of the contract, instead fold the two
places where SetClientReady mattered into AddObserver (the first time
where a begin frame could actually be delivered) and remove
SetClientReady from the API.

R=skyostil@chromium.org,sunnyps@chomium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[danakj, 2016-03-10 22:55:54]

danakj@chromium.org changed reviewers:
- sunnyps@chomium.org

[sunnyps, 2016-03-14 20:51:42]

On 2016/03/10 22:55:32, enne wrote:
> On 2016/03/10 at 22:43:58, sunnyps wrote:
> > Actually SetClientReady is used for one-time initialization for the
renderer's
> external begin frame source and maybe for webview too. The idea is that the
> begin frame source is created on the main thread and given to the impl thread
> where SetClientReady should be called after it has been attached to the
> scheduler. I think it should be the responsibility of the Proxy to do that and
> this doesn't belong in the BeginFrameSource interface IMO. Can we remove it
from
> there and just have it in the BeginFrameSource subclasses that require it?
> 
> As far as I understand what you're getting at, this is still what this patch
> does.  AddObserver is not called on a begin frame source until it's been
> attached to the scheduler, at least in practice.  I think it would be
incorrect
> to add an observer to a begin frame source on a different thread.
> 
> I am a little confused about what you are proposing.  What code should move to
> where?

Sorry, I thought you were keeping SetClientReady. LGTM

[enne (OOO), 2016-03-16 22:06:12]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2016-03-16 22:06:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1778853003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1778853003/1

[commit-bot: I haz the power, 2016-03-16 22:19:21]

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

[commit-bot: I haz the power, 2016-03-16 22:19:22]

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

[enne (OOO), 2016-03-16 22:26:29]

enne@chromium.org changed reviewers:
+ tedchoc@chromium.org

[enne (OOO), 2016-03-16 22:26:42]

+tedchoc for
content/renderer/android/synchronous_compositor_external_begin_frame_source.*
OWNERS

[Ted C, 2016-03-16 23:37:03]

tedchoc@chromium.org changed reviewers:
+ boliu@chromium.org

[Ted C, 2016-03-16 23:37:04]

boliu is a better OWNER than me

[boliu, 2016-03-16 23:55:51]

lgtm

[enne (OOO), 2016-03-17 00:38:27]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2016-03-17 00:39:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1778853003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1778853003/1

[commit-bot: I haz the power, 2016-03-17 00:45:10]

Description was changed from

==========
Fold BeginFrameSource::SetClientReady into AddObserver

As a part of https://codereview.chromium.org/1774323003, it became clear
that it needed to be possible to call SetClientReady repeatedly, which
some sources didn't support.

Instead of just making that part of the contract, instead fold the two
places where SetClientReady mattered into AddObserver (the first time
where a begin frame could actually be delivered) and remove
SetClientReady from the API.

R=skyostil@chromium.org,sunnyps@chomium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fold BeginFrameSource::SetClientReady into AddObserver

As a part of https://codereview.chromium.org/1774323003, it became clear
that it needed to be possible to call SetClientReady repeatedly, which
some sources didn't support.

Instead of just making that part of the contract, instead fold the two
places where SetClientReady mattered into AddObserver (the first time
where a begin frame could actually be delivered) and remove
SetClientReady from the API.

R=skyostil@chromium.org,sunnyps@chomium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-03-17 00:45:12]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-03-17 00:49:34]

Description was changed from

==========
Fold BeginFrameSource::SetClientReady into AddObserver

As a part of https://codereview.chromium.org/1774323003, it became clear
that it needed to be possible to call SetClientReady repeatedly, which
some sources didn't support.

Instead of just making that part of the contract, instead fold the two
places where SetClientReady mattered into AddObserver (the first time
where a begin frame could actually be delivered) and remove
SetClientReady from the API.

R=skyostil@chromium.org,sunnyps@chomium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Fold BeginFrameSource::SetClientReady into AddObserver

As a part of https://codereview.chromium.org/1774323003, it became clear
that it needed to be possible to call SetClientReady repeatedly, which
some sources didn't support.

Instead of just making that part of the contract, instead fold the two
places where SetClientReady mattered into AddObserver (the first time
where a begin frame could actually be delivered) and remove
SetClientReady from the API.

R=skyostil@chromium.org,sunnyps@chomium.org
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/de6bfca79029c410d2e7c58173e2119fd485cc28
Cr-Commit-Position: refs/heads/master@{#381612}
==========

[commit-bot: I haz the power, 2016-03-17 00:49:35]

Patchset 1 (id:??) landed as
https://crrev.com/de6bfca79029c410d2e7c58173e2119fd485cc28
Cr-Commit-Position: refs/heads/master@{#381612}