cc: Make BackToBackBeginFrameSource a SyntheticBeginFrameSource.

cc: Make BackToBackBeginFrameSource a SyntheticBeginFrameSource.

This means that we can pass a single BeginFrameSource to the
Display's OutputSurface, and to the Display, and they agree on which
BeginFrameSource they are using.

This is a prereq for https://codereview.chromium.org/2029323004/

R=brianderson@chromium.org, enne
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/15ec524d5a9a6ef343c838d55b713fb492b4250f
Cr-Commit-Position: refs/heads/master@{#400025}

[danakj, 2016-06-14 18:11:36]

Description was changed from

==========
cc: Make BackToBackBeginFrameSource a SyntheticBeginFrameSource.

This means that we can pass a single BeginFrameSource to the
Display's OutputSurface, and to the Display, and they agree on which
BeginFrameSource they are using.

This is a prereq for https://codereview.chromium.org/2029323004/

R=brianderson@chromium.org, enne
BUG=487471
==========

to

==========
cc: Make BackToBackBeginFrameSource a SyntheticBeginFrameSource.

This means that we can pass a single BeginFrameSource to the
Display's OutputSurface, and to the Display, and they agree on which
BeginFrameSource they are using.

This is a prereq for https://codereview.chromium.org/2029323004/

R=brianderson@chromium.org, enne
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[danakj, 2016-06-14 18:12:04]

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

[danakj, 2016-06-14 18:12:19]

danakj@chromium.org changed reviewers:
+ fsamuel@google.com

[danakj, 2016-06-14 18:12:20]

+fsamuel for mus

[commit-bot: I haz the power, 2016-06-14 18:12:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2061273002/1

[danakj, 2016-06-14 18:12:36]

Description was changed from

==========
cc: Make BackToBackBeginFrameSource a SyntheticBeginFrameSource.

This means that we can pass a single BeginFrameSource to the
Display's OutputSurface, and to the Display, and they agree on which
BeginFrameSource they are using.

This is a prereq for https://codereview.chromium.org/2029323004/

R=brianderson@chromium.org, enne
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Make BackToBackBeginFrameSource a SyntheticBeginFrameSource.

This means that we can pass a single BeginFrameSource to the
Display's OutputSurface, and to the Display, and they agree on which
BeginFrameSource they are using.

This is a prereq for https://codereview.chromium.org/2029323004/

R=brianderson@chromium.org, enne
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[danakj, 2016-06-14 18:12:36]

danakj@chromium.org changed reviewers:
+ fsamuel@chromium.org
- fsamuel@google.com

[commit-bot: I haz the power, 2016-06-14 18:14:37]

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

[commit-bot: I haz the power, 2016-06-14 18:14:38]

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

[danakj, 2016-06-14 18:25:52]

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

[commit-bot: I haz the power, 2016-06-14 18:26:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2061273002/20001

[commit-bot: I haz the power, 2016-06-14 18:35:43]

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

[commit-bot: I haz the power, 2016-06-14 18:35:44]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[danakj, 2016-06-14 18:36:41]

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

[commit-bot: I haz the power, 2016-06-14 18:37:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2061273002/40001

[Fady Samuel, 2016-06-14 18:45:11]

lgtm

[danakj, 2016-06-14 19:59:43]

The changed tests are all in begin_frame_source_unittests.cc for BackToBack. I
added a comment on every change to explain it, hopefully they are okay. If not
plz let me know I can try to maintain older behaviour.

[danakj, 2016-06-14 19:59:48]

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

[commit-bot: I haz the power, 2016-06-14 20:00:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2061273002/80001

[enne (OOO), 2016-06-14 21:25:18]

I do feel like this really complicates SyntheticBeginFrameSource, but as long as
the tests keep passing, I think that's better than pushing complexity elsewhere.
 I think there's some things that could be done to abstract it apart better, but
when there's only two types that's also probably overkill.

https://codereview.chromium.org/2061273002/diff/80001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/2061273002/diff/80001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:124: // TODO(brianderson): We should not be
receiving 0 intervals.
I love how this comment has proliferated.

https://codereview.chromium.org/2061273002/diff/80001/cc/test/scheduler_test_...
File cc/test/scheduler_test_common.h (right):

https://codereview.chromium.org/2061273002/diff/80001/cc/test/scheduler_test_...
cc/test/scheduler_test_common.h:99: class TestBackToBackBeginFrameSource :
public BeginFrameSourceBase {
Why can't this be a BackToBackBeginFrameSource? It looks like you replicate some
of the logic in the test class, which is a bit <_<

[commit-bot: I haz the power, 2016-06-14 21:40:00]

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

[commit-bot: I haz the power, 2016-06-14 21:40:01]

Dry run: This issue passed the CQ dry run.

[danakj, 2016-06-14 21:53:24]

https://codereview.chromium.org/2061273002/diff/80001/cc/test/scheduler_test_...
File cc/test/scheduler_test_common.h (right):

https://codereview.chromium.org/2061273002/diff/80001/cc/test/scheduler_test_...
cc/test/scheduler_test_common.h:99: class TestBackToBackBeginFrameSource :
public BeginFrameSourceBase {
On 2016/06/14 21:25:17, enne wrote:
> Why can't this be a BackToBackBeginFrameSource? It looks like you replicate
some
> of the logic in the test class, which is a bit <_<

Oh, I did that first so I could ignore some tests. Let me try that.

[danakj, 2016-06-14 21:55:37]

On Tue, Jun 14, 2016 at 2:25 PM, enne@chromium.org via
codereview.chromium.org <reply@chromiumcodereview-hr.appspotmail.com> wrote:

> I do feel like this really complicates SyntheticBeginFrameSource, but as
> long as
> the tests keep passing, I think that's better than pushing complexity
> elsewhere.
> I think there's some things that could be done to abstract it apart
> better, but
> when there's only two types that's also probably overkill.
>

I'd be interested in abstracting it apart better. I struggled to get this
to work in a reasonable way and didn't really see nicer ways to do it at
that point.


>
>
>
>
https://codereview.chromium.org/2061273002/diff/80001/cc/scheduler/begin_fram...
> File cc/scheduler/begin_frame_source.cc (right):
>
>
>
https://codereview.chromium.org/2061273002/diff/80001/cc/scheduler/begin_fram...
> cc/scheduler/begin_frame_source.cc:124: // TODO(brianderson): We should
> not be receiving 0 intervals.
> I love how this comment has proliferated.
>
>
>
https://codereview.chromium.org/2061273002/diff/80001/cc/test/scheduler_test_...
> File cc/test/scheduler_test_common.h (right):
>
>
>
https://codereview.chromium.org/2061273002/diff/80001/cc/test/scheduler_test_...
> cc/test/scheduler_test_common.h:99: class TestBackToBackBeginFrameSource
> : public BeginFrameSourceBase {
> Why can't this be a BackToBackBeginFrameSource? It looks like you
> replicate some of the logic in the test class, which is a bit <_<
>
> https://codereview.chromium.org/2061273002/
>

-- 
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.

[brianderson, 2016-06-14 21:59:49]

Is the intention to eventually make throttling vs. not a setting that can change
at runtime?

If yes: Complicating SyntheticBeginFrame source is probably okay if it is the
only class that needs the runtime setting.

If not, why can't we initialize things so that everyone uses the same source
(throttled or not)?

https://codereview.chromium.org/2061273002/diff/80001/cc/scheduler/begin_fram...
File cc/scheduler/begin_frame_source.cc (right):

https://codereview.chromium.org/2061273002/diff/80001/cc/scheduler/begin_fram...
cc/scheduler/begin_frame_source.cc:124: // TODO(brianderson): We should not be
receiving 0 intervals.
On 2016/06/14 21:25:17, enne wrote:
> I love how this comment has proliferated.

I have so many fewer TODO's now! =)

[danakj, 2016-06-14 22:00:16]

https://codereview.chromium.org/2061273002/diff/80001/cc/test/scheduler_test_...
File cc/test/scheduler_test_common.h (right):

https://codereview.chromium.org/2061273002/diff/80001/cc/test/scheduler_test_...
cc/test/scheduler_test_common.h:99: class TestBackToBackBeginFrameSource :
public BeginFrameSourceBase {
On 2016/06/14 21:53:23, danakj wrote:
> On 2016/06/14 21:25:17, enne wrote:
> > Why can't this be a BackToBackBeginFrameSource? It looks like you replicate
> some
> > of the logic in the test class, which is a bit <_<
> 
> Oh, I did that first so I could ignore some tests. Let me try that.

It is now deleted.

[danakj, 2016-06-14 22:04:18]

On Tue, Jun 14, 2016 at 2:59 PM, <brianderson@chromium.org> wrote:

> Is the intention to eventually make throttling vs. not a setting that can
> change
> at runtime?
>

Yes, enne was pushing me to go that way as people have wanted to achieve
this in the past.


>
> If yes: Complicating SyntheticBeginFrame source is probably okay if it is
> the
> only class that needs the runtime setting.
>

The only other complication was that DelayBasedTimeSource can now use a 0
interval instead of DCHECKing that, but that's probably okay?


> If not, why can't we initialize things so that everyone uses the same
> source
> (throttled or not)?
>

Otherwise, we could make a subclass of Synthetic that is back to back (and
optionally a different subclass that isn't).. yah.

But I think we want it to be runtime.


>
>
>
>
>
https://codereview.chromium.org/2061273002/diff/80001/cc/scheduler/begin_fram...
> File cc/scheduler/begin_frame_source.cc (right):
>
>
>
https://codereview.chromium.org/2061273002/diff/80001/cc/scheduler/begin_fram...
> cc/scheduler/begin_frame_source.cc:124: // TODO(brianderson): We should
> not be receiving 0 intervals.
> On 2016/06/14 21:25:17, enne wrote:
> > I love how this comment has proliferated.
>
> I have so many fewer TODO's now! =)
>
> https://codereview.chromium.org/2061273002/
>

-- 
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.

[brianderson, 2016-06-14 22:13:00]

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

[brianderson, 2016-06-14 22:23:08]

On 2016/06/14 22:04:18, danakj wrote:
> On Tue, Jun 14, 2016 at 2:59 PM, <mailto:brianderson@chromium.org> wrote:
> 
> > Is the intention to eventually make throttling vs. not a setting that can
> > change
> > at runtime?
> >
> 
> Yes, enne was pushing me to go that way as people have wanted to achieve
> this in the past.
> 
> 
> >
> > If yes: Complicating SyntheticBeginFrame source is probably okay if it is
> > the
> > only class that needs the runtime setting.
> >
> 
> The only other complication was that DelayBasedTimeSource can now use a 0
> interval instead of DCHECKing that, but that's probably okay?
> 
> 
> > If not, why can't we initialize things so that everyone uses the same
> > source
> > (throttled or not)?
> >
> 
> Otherwise, we could make a subclass of Synthetic that is back to back (and
> optionally a different subclass that isn't).. yah.
> 
> But I think we want it to be runtime.

+bajones to comment on the runtime vsync option, since he was the one initially
looking into it. Not sure if anyone else has been looking into it since then.

If we aren't going to do it soon, maybe punt on this? enne just removed
complicated code in BeginFrameSourceMultiplexer that was intended for background
ticking and a runtime vsync option. There's some hairy monotonicity issues
BeginFrameSourceMultiplexer handled that we'd need to make sure aren't an issue
here.

Instead of subclassing Synthetic, can we change places that assume Synthetic to
use BeginFrameSource?

> 
> 
> >
> >
> >
> >
> >
>
https://codereview.chromium.org/2061273002/diff/80001/cc/scheduler/begin_fram...
> > File cc/scheduler/begin_frame_source.cc (right):
> >
> >
> >
>
https://codereview.chromium.org/2061273002/diff/80001/cc/scheduler/begin_fram...
> > cc/scheduler/begin_frame_source.cc:124: // TODO(brianderson): We should
> > not be receiving 0 intervals.
> > On 2016/06/14 21:25:17, enne wrote:
> > > I love how this comment has proliferated.
> >
> > I have so many fewer TODO's now! =)
> >
> > https://codereview.chromium.org/2061273002/
> >
> 
> -- 
> 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 mailto:chromium-reviews+unsubscribe@chromium.org.

[bajones, 2016-06-14 22:45:20]

On 2016/06/14 22:23:08, brianderson wrote:
> +bajones to comment on the runtime vsync option, since he was the one
initially
> looking into it. Not sure if anyone else has been looking into it since then.

There's two cases that I was aware of for changing this at run time. One was for
dev tools, where developers wanted to be able to test the throughput of their
apps (usually something WebGL based) without re-launching the browser with the
vsync disabled flags. That effort stalled when we hit some questions about how
to keep the setting localized to only one renderer, given that we were tweaking
values in the GPU process as well. I don't know when we'll come back around to
that again, but it's not really on my roadmap.

The other scenario is for desktop VR, where the headsets run at high rates
(90Hz). In order to render to them effectively we'll need to be able to update
the refresh rate on the fly. That's not an immediate need, though. Realistically
that feature is probably targeting around M55?

[danakj, 2016-06-14 22:59:17]

On Tue, Jun 14, 2016 at 3:23 PM, <brianderson@chromium.org> wrote:

> On 2016/06/14 22:04:18, danakj wrote:
> > On Tue, Jun 14, 2016 at 2:59 PM, <mailto:brianderson@chromium.org>
> wrote:
> >
> > > Is the intention to eventually make throttling vs. not a setting that
> can
> > > change
> > > at runtime?
> > >
> >
> > Yes, enne was pushing me to go that way as people have wanted to achieve
> > this in the past.
> >
> >
> > >
> > > If yes: Complicating SyntheticBeginFrame source is probably okay if it
> is
> > > the
> > > only class that needs the runtime setting.
> > >
> >
> > The only other complication was that DelayBasedTimeSource can now use a 0
> > interval instead of DCHECKing that, but that's probably okay?
> >
> >
> > > If not, why can't we initialize things so that everyone uses the same
> > > source
> > > (throttled or not)?
> > >
> >
> > Otherwise, we could make a subclass of Synthetic that is back to back
> (and
> > optionally a different subclass that isn't).. yah.
> >
> > But I think we want it to be runtime.
>
> +bajones to comment on the runtime vsync option, since he was the one
> initially
> looking into it. Not sure if anyone else has been looking into it since
> then.
>
> If we aren't going to do it soon, maybe punt on this? enne just removed
> complicated code in BeginFrameSourceMultiplexer that was intended for
> background
> ticking and a runtime vsync option. There's some hairy monotonicity issues
> BeginFrameSourceMultiplexer handled that we'd need to make sure aren't an
> issue
> here.
>

OK I can punt if you like.


>
> Instead of subclassing Synthetic, can we change places that assume
> Synthetic to
> use BeginFrameSource?
>

Can't do that without changing the API of BeginFrameSource. The Display
output surface is calling UpdateVsyncParams on the BeginFrameSource, which
is a function of SyntheticBFS. It's not clear that should be part of the
BFS API. And BackToBack *is* a type of synthetic BFS so it seemed to make
more sense that way.

>
>
> >
> >
> > >
> > >
> > >
> > >
> > >
> >
>
>
https://codereview.chromium.org/2061273002/diff/80001/cc/scheduler/begin_fram...
> > > File cc/scheduler/begin_frame_source.cc (right):
> > >
> > >
> > >
> >
>
>
https://codereview.chromium.org/2061273002/diff/80001/cc/scheduler/begin_fram...
> > > cc/scheduler/begin_frame_source.cc:124: // TODO(brianderson): We should
> > > not be receiving 0 intervals.
> > > On 2016/06/14 21:25:17, enne wrote:
> > > > I love how this comment has proliferated.
> > >
> > > I have so many fewer TODO's now! =)
> > >
> > > https://codereview.chromium.org/2061273002/
> > >
> >
> > --
> > 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 mailto:chromium-reviews+unsubscribe@chromium.org.
>
>
>
> https://codereview.chromium.org/2061273002/
>

-- 
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-06-14 23:13:44]

lgtm

https://codereview.chromium.org/2061273002/diff/100001/cc/scheduler/delay_bas...
File cc/scheduler/delay_based_time_source.cc (right):

https://codereview.chromium.org/2061273002/diff/100001/cc/scheduler/delay_bas...
cc/scheduler/delay_based_time_source.cc:85: // DCHECK_GT(interval,
base::TimeDelta());
??

[danakj, 2016-06-14 23:15:24]

https://codereview.chromium.org/2061273002/diff/100001/cc/scheduler/delay_bas...
File cc/scheduler/delay_based_time_source.cc (right):

https://codereview.chromium.org/2061273002/diff/100001/cc/scheduler/delay_bas...
cc/scheduler/delay_based_time_source.cc:85: // DCHECK_GT(interval,
base::TimeDelta());
On 2016/06/14 23:13:44, enne wrote:
> ??

It must be deleted, we support 0 now. Yay

[danakj, 2016-06-15 00:54:02]

The latest patch set makes BackToBackBFS and DelayBasedBFS both subclasses of a
SyntheticBeginFrameSource interface. The BackToBackBFS ignores calls to the
synthetic stuff tho. :)

I left BackToBack using a DelayBasedTimeSource to do its post-tasking, so that
tests can change Now on that time source instead of having magic overridden
methods on the B2BBFS class, which leaves the test expectation changes as they
were in the last patch set.

I had to make BFSBase a template to do this.. but I have a feeling I can just
kill that class, let me see.

[danakj, 2016-06-15 18:48:21]

PTAL.

BeginFrameSource is pure virtual.
SynthethicBeginFrameSource is a subclass that is pure virtual.

BeginFrameSourceBase is gone. Each implementation of BFS just implements the
things it needs from the 2 above APIs.

It's not clear to me why the tracing in BFSBase was for debug builds only. I
maintained that here but I'd like to know why.

[danakj, 2016-06-15 19:22:51]

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

[danakj, 2016-06-15 19:22:52]

+piman for content/

[enne (OOO), 2016-06-15 19:46:56]

I like this approach better than the previous patches.  Getting rid of
BeginFrameSourceBase is nice.

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

https://codereview.chromium.org/2061273002/diff/160001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.cc:71: // Generate a new BeginFrame tick.
// ... if needed?

(or no comment at all?)

https://codereview.chromium.org/2061273002/diff/160001/cc/scheduler/begin_fra...
File cc/scheduler/begin_frame_source.h (left):

https://codereview.chromium.org/2061273002/diff/160001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.h:150: virtual void
OnNeedsBeginFramesChanged(bool needs_begin_frames) {}
Just moving this logic up into each class is nice cleanup.

https://codereview.chromium.org/2061273002/diff/160001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/2061273002/diff/160001/cc/scheduler/scheduler...
cc/scheduler/scheduler_unittest.cc:203: class BeginSourceToSchedulerClientProxy
class BeginSourceToSchedulerClientProxy : public
FakeExternalBeginFrameSource::Client makes my head hurt.

Maybe at least Begin*Frame*Source, even though it's wordier or
SchedulerObserverTracker?

[piman, 2016-06-15 19:48:14]

https://codereview.chromium.org/2061273002/diff/160001/content/renderer/gpu/c...
File content/renderer/gpu/compositor_external_begin_frame_source.cc (right):

https://codereview.chromium.org/2061273002/diff/160001/content/renderer/gpu/c...
content/renderer/gpu/compositor_external_begin_frame_source.cc:107:
std::set<cc::BeginFrameObserver*> observers(observers_);
Would it make sense to use ObserverList + FOR_EACH_OBSERVER instead?
It deals with an observer removing itself from the list while called back, and
is probably generally cheaper (and more idiomatic) than a std::set.

[danakj, 2016-06-15 19:56:05]

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

https://codereview.chromium.org/2061273002/diff/160001/cc/scheduler/begin_fra...
cc/scheduler/begin_frame_source.cc:71: // Generate a new BeginFrame tick.
On 2016/06/15 19:46:56, enne wrote:
> // ... if needed?
> 
> (or no comment at all?)

Well, so the time source is only active 1 tick at a time, which is what I want
to get across. Hm. I can make that more explicit. I moved it to the place where
we set the 0 interval.

https://codereview.chromium.org/2061273002/diff/160001/content/renderer/gpu/c...
File content/renderer/gpu/compositor_external_begin_frame_source.cc (right):

https://codereview.chromium.org/2061273002/diff/160001/content/renderer/gpu/c...
content/renderer/gpu/compositor_external_begin_frame_source.cc:107:
std::set<cc::BeginFrameObserver*> observers(observers_);
On 2016/06/15 19:48:14, piman wrote:
> Would it make sense to use ObserverList + FOR_EACH_OBSERVER instead?
> It deals with an observer removing itself from the list while called back, and
> is probably generally cheaper (and more idiomatic) than a std::set.

I think it didn't work because of OnBeginFrame being const. I looked at some
patches to change, or add another, ObserverList when the original code for this
went into BFSBase.

[danakj, 2016-06-15 19:58:00]

https://codereview.chromium.org/2061273002/diff/160001/content/renderer/gpu/c...
File content/renderer/gpu/compositor_external_begin_frame_source.cc (right):

https://codereview.chromium.org/2061273002/diff/160001/content/renderer/gpu/c...
content/renderer/gpu/compositor_external_begin_frame_source.cc:107:
std::set<cc::BeginFrameObserver*> observers(observers_);
On 2016/06/15 19:56:05, danakj wrote:
> On 2016/06/15 19:48:14, piman wrote:
> > Would it make sense to use ObserverList + FOR_EACH_OBSERVER instead?
> > It deals with an observer removing itself from the list while called back,
and
> > is probably generally cheaper (and more idiomatic) than a std::set.
> 
> I think it didn't work because of OnBeginFrame being const. I looked at some
> patches to change, or add another, ObserverList when the original code for
this
> went into BFSBase.

OK.. so I found the email and it said:

"The important part is that inside a **const method** on
ThingWhichHasObservers we want to be able to loop over the observer
list and call const methods on the observers.

As observers are normally allowed to modify the observer list while
you are iterating over them (so they can remove themselves after being
notified) the ObserverList::Iterator currently only works on non-const
ObserverLists. This means you can't loop over ObserverLists in a const
method!"

But these methods aren't const. So ya let me try that.

[danakj, 2016-06-15 20:23:41]

https://codereview.chromium.org/2061273002/diff/160001/content/renderer/gpu/c...
File content/renderer/gpu/compositor_external_begin_frame_source.cc (right):

https://codereview.chromium.org/2061273002/diff/160001/content/renderer/gpu/c...
content/renderer/gpu/compositor_external_begin_frame_source.cc:107:
std::set<cc::BeginFrameObserver*> observers(observers_);
On 2016/06/15 19:58:00, danakj wrote:
> On 2016/06/15 19:56:05, danakj wrote:
> > On 2016/06/15 19:48:14, piman wrote:
> > > Would it make sense to use ObserverList + FOR_EACH_OBSERVER instead?
> > > It deals with an observer removing itself from the list while called back,
> and
> > > is probably generally cheaper (and more idiomatic) than a std::set.
> > 
> > I think it didn't work because of OnBeginFrame being const. I looked at some
> > patches to change, or add another, ObserverList when the original code for
> this
> > went into BFSBase.
> 
> OK.. so I found the email and it said:
> 
> "The important part is that inside a **const method** on
> ThingWhichHasObservers we want to be able to loop over the observer
> list and call const methods on the observers.
> 
> As observers are normally allowed to modify the observer list while
> you are iterating over them (so they can remove themselves after being
> notified) the ObserverList::Iterator currently only works on non-const
> ObserverLists. This means you can't loop over ObserverLists in a const
> method!"
> 
> But these methods aren't const. So ya let me try that.

It gets sketchy around "if the list was empty then do this after adding". We can
check "might_have_observers()", but if this is being added during iteration,
then it might return true when there wasn't and we fail to act correctly, or
after removing it checks if it's now empty. I think I prefer to leave it using
set().

[danakj, 2016-06-15 20:35:57]

https://codereview.chromium.org/2061273002/diff/160001/cc/scheduler/scheduler...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/2061273002/diff/160001/cc/scheduler/scheduler...
cc/scheduler/scheduler_unittest.cc:203: class BeginSourceToSchedulerClientProxy
On 2016/06/15 19:46:56, enne wrote:
> class BeginSourceToSchedulerClientProxy : public
> FakeExternalBeginFrameSource::Client makes my head hurt.
> 
> Maybe at least Begin*Frame*Source, even though it's wordier or
> SchedulerObserverTracker?

OK I made FakeSchedulerClient also be a FakeExternalBeginFrameSource::Client.

[danakj, 2016-06-15 20:36:26]

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

[commit-bot: I haz the power, 2016-06-15 20:36:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2061273002/200001

[piman, 2016-06-15 20:41:08]

On 2016/06/15 20:23:41, danakj wrote:
>
https://codereview.chromium.org/2061273002/diff/160001/content/renderer/gpu/c...
> File content/renderer/gpu/compositor_external_begin_frame_source.cc (right):
> 
>
https://codereview.chromium.org/2061273002/diff/160001/content/renderer/gpu/c...
> content/renderer/gpu/compositor_external_begin_frame_source.cc:107:
> std::set<cc::BeginFrameObserver*> observers(observers_);
> On 2016/06/15 19:58:00, danakj wrote:
> > On 2016/06/15 19:56:05, danakj wrote:
> > > On 2016/06/15 19:48:14, piman wrote:
> > > > Would it make sense to use ObserverList + FOR_EACH_OBSERVER instead?
> > > > It deals with an observer removing itself from the list while called
back,
> > and
> > > > is probably generally cheaper (and more idiomatic) than a std::set.
> > > 
> > > I think it didn't work because of OnBeginFrame being const. I looked at
some
> > > patches to change, or add another, ObserverList when the original code for
> > this
> > > went into BFSBase.
> > 
> > OK.. so I found the email and it said:
> > 
> > "The important part is that inside a **const method** on
> > ThingWhichHasObservers we want to be able to loop over the observer
> > list and call const methods on the observers.
> > 
> > As observers are normally allowed to modify the observer list while
> > you are iterating over them (so they can remove themselves after being
> > notified) the ObserverList::Iterator currently only works on non-const
> > ObserverLists. This means you can't loop over ObserverLists in a const
> > method!"
> > 
> > But these methods aren't const. So ya let me try that.
> 
> It gets sketchy around "if the list was empty then do this after adding". We
can
> check "might_have_observers()", but if this is being added during iteration,
> then it might return true when there wasn't and we fail to act correctly, or
> after removing it checks if it's now empty. I think I prefer to leave it using
> set().

Ok... how about unordered_set?
LGTM either way.

[danakj, 2016-06-15 20:44:27]

On Wed, Jun 15, 2016 at 1:41 PM, <piman@chromium.org> wrote:

> On 2016/06/15 20:23:41, danakj wrote:
> >
>
>
https://codereview.chromium.org/2061273002/diff/160001/content/renderer/gpu/c...
> > File content/renderer/gpu/compositor_external_begin_frame_source.cc
> (right):
> >
> >
>
>
https://codereview.chromium.org/2061273002/diff/160001/content/renderer/gpu/c...
> > content/renderer/gpu/compositor_external_begin_frame_source.cc:107:
> > std::set<cc::BeginFrameObserver*> observers(observers_);
> > On 2016/06/15 19:58:00, danakj wrote:
> > > On 2016/06/15 19:56:05, danakj wrote:
> > > > On 2016/06/15 19:48:14, piman wrote:
> > > > > Would it make sense to use ObserverList + FOR_EACH_OBSERVER
> instead?
> > > > > It deals with an observer removing itself from the list while
> called
> back,
> > > and
> > > > > is probably generally cheaper (and more idiomatic) than a std::set.
> > > >
> > > > I think it didn't work because of OnBeginFrame being const. I looked
> at
> some
> > > > patches to change, or add another, ObserverList when the original
> code for
> > > this
> > > > went into BFSBase.
> > >
> > > OK.. so I found the email and it said:
> > >
> > > "The important part is that inside a **const method** on
> > > ThingWhichHasObservers we want to be able to loop over the observer
> > > list and call const methods on the observers.
> > >
> > > As observers are normally allowed to modify the observer list while
> > > you are iterating over them (so they can remove themselves after being
> > > notified) the ObserverList::Iterator currently only works on non-const
> > > ObserverLists. This means you can't loop over ObserverLists in a const
> > > method!"
> > >
> > > But these methods aren't const. So ya let me try that.
> >
> > It gets sketchy around "if the list was empty then do this after
> adding". We
> can
> > check "might_have_observers()", but if this is being added during
> iteration,
> > then it might return true when there wasn't and we fail to act
> correctly, or
> > after removing it checks if it's now empty. I think I prefer to leave it
> using
> > set().
>
> Ok... how about unordered_set?
> LGTM either way.
>

Good idea! That didn't exist when this was first written.

-- 
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.

[danakj, 2016-06-15 20:46:15]

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

[commit-bot: I haz the power, 2016-06-15 20:46:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2061273002/220001

[danakj, 2016-06-15 20:49:35]

After talking with enne@ it seems like the DEBUG_FRAMES was something used
during development but probably not anymore - since it's not shipped to users we
can't use it to get data from them anyways. So I just removed it instead of
propogating this macro out to more places.

[danakj, 2016-06-15 20:49:48]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-06-15 20:49:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, enne@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2061273002/#ps240001
(title: "syntheticbeginframesource: delete-DEBUG_FRAMES")

[commit-bot: I haz the power, 2016-06-15 20:50:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2061273002/240001

[commit-bot: I haz the power, 2016-06-15 22:13:54]

Description was changed from

==========
cc: Make BackToBackBeginFrameSource a SyntheticBeginFrameSource.

This means that we can pass a single BeginFrameSource to the
Display's OutputSurface, and to the Display, and they agree on which
BeginFrameSource they are using.

This is a prereq for https://codereview.chromium.org/2029323004/

R=brianderson@chromium.org, enne
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Make BackToBackBeginFrameSource a SyntheticBeginFrameSource.

This means that we can pass a single BeginFrameSource to the
Display's OutputSurface, and to the Display, and they agree on which
BeginFrameSource they are using.

This is a prereq for https://codereview.chromium.org/2029323004/

R=brianderson@chromium.org, enne
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-06-15 22:13:57]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2016-06-15 22:14:10]

CQ bit was unchecked

[commit-bot: I haz the power, 2016-06-15 22:15:30]

Description was changed from

==========
cc: Make BackToBackBeginFrameSource a SyntheticBeginFrameSource.

This means that we can pass a single BeginFrameSource to the
Display's OutputSurface, and to the Display, and they agree on which
BeginFrameSource they are using.

This is a prereq for https://codereview.chromium.org/2029323004/

R=brianderson@chromium.org, enne
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Make BackToBackBeginFrameSource a SyntheticBeginFrameSource.

This means that we can pass a single BeginFrameSource to the
Display's OutputSurface, and to the Display, and they agree on which
BeginFrameSource they are using.

This is a prereq for https://codereview.chromium.org/2029323004/

R=brianderson@chromium.org, enne
BUG=487471
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/15ec524d5a9a6ef343c838d55b713fb492b4250f
Cr-Commit-Position: refs/heads/master@{#400025}
==========

[commit-bot: I haz the power, 2016-06-15 22:15:32]

Patchset 13 (id:??) landed as
https://crrev.com/15ec524d5a9a6ef343c838d55b713fb492b4250f
Cr-Commit-Position: refs/heads/master@{#400025}

[brianderson, 2016-06-16 18:47:34]

Was out yesterday, but had a change to take a look at this today. lgtm. Thanks
for the cleanups!