cc: Implement unified BeginFrame on aura

cc: Unified BeginFrame on aura

This cl implements unified BeginFrame on Aura platform.
With this, child scheduler(renderer-side cc scheduler) uses external BeginFrameSource
instead of synthetic BeignFrameSource.
ui::CompositorVSyncManager is also removed because aura platform doesn't need vsync message.

Design docs (https://docs.google.com/a/chromium.org/document/d/13xtO-_NSSnNZRRS1Xq3xGNKZawKc8HQxOid5boBUyX8/edit#)

R=brianderson@chromium.org, mithro@mithis.com
BUG=372086, 441577
TEST=cc_unittests, compositor_unittests

[simonhong, 2014-12-06 02:32:49]

Patchset #1 (id:1) has been deleted

[simonhong, 2014-12-06 02:37:31]

simonhong@chromium.org changed reviewers:
+ brianderson@chromium.org, mithro@mithis.com

[simonhong, 2014-12-06 02:40:32]

Yesterday, ui::Compositor starts to act as a SchedulerClient.
So, we can enable unified BeginFrame scheduling on aura and mac.
Unified BeginFrame will be enabled on Mac, too after landing on Aura.
PTAL!

[danakj, 2014-12-06 18:02:13]

fwiw that CL was reverted and will likely take a few tries to stick, so
please be patient with us :)

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brianderson, 2014-12-09 03:00:32]

https://codereview.chromium.org/775143003/diff/40001/cc/output/begin_frame_ar...
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/775143003/diff/40001/cc/output/begin_frame_ar...
cc/output/begin_frame_args.h:88: interval == other.interval);
Probably should include "type" in the checks here.

https://codereview.chromium.org/775143003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/775143003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:2531:
DCHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
Should probably make this a simple dcheck ona bool or just remove the dcheck.

https://codereview.chromium.org/775143003/diff/40001/ui/base/compositor/compo...
File ui/base/compositor/compositor_begin_frame_observer.h (right):

https://codereview.chromium.org/775143003/diff/40001/ui/base/compositor/compo...
ui/base/compositor/compositor_begin_frame_observer.h:14: class
CompositorBeginFrameObserver {
Do we need this now that we have cc::BeginFrameObserver?

https://codereview.chromium.org/775143003/diff/40001/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/775143003/diff/40001/ui/compositor/compositor...
ui/compositor/compositor.cc:319: last_begin_frame_args_.deadline >
base::TimeTicks::Now())
Let's not do the deadline check here and just let the Renderer decide what it
wants to do with the expired BeginFrame.

https://codereview.chromium.org/775143003/diff/40001/ui/compositor/compositor...
ui/compositor/compositor.cc:320:
observer->OnSendBeginFrame(last_begin_frame_args_);
In this case, can you set the BeginFrame "type" to MISSED.

[simonhong, 2014-12-09 17:09:27]

brianderson@, thanks for review :)

https://codereview.chromium.org/775143003/diff/40001/cc/output/begin_frame_ar...
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/775143003/diff/40001/cc/output/begin_frame_ar...
cc/output/begin_frame_args.h:88: interval == other.interval);
On 2014/12/09 03:00:31, brianderson wrote:
> Probably should include "type" in the checks here.

Done.

https://codereview.chromium.org/775143003/diff/40001/content/browser/renderer...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/775143003/diff/40001/content/browser/renderer...
content/browser/renderer_host/render_widget_host_view_aura.cc:2531:
DCHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
On 2014/12/09 03:00:31, brianderson wrote:
> Should probably make this a simple dcheck ona bool or just remove the dcheck.

I think this can be removed because DelegatedFrameHost::OnSetNeedsBeginFrames()
also check it.

https://codereview.chromium.org/775143003/diff/40001/ui/base/compositor/compo...
File ui/base/compositor/compositor_begin_frame_observer.h (right):

https://codereview.chromium.org/775143003/diff/40001/ui/base/compositor/compo...
ui/base/compositor/compositor_begin_frame_observer.h:14: class
CompositorBeginFrameObserver {
On 2014/12/09 03:00:31, brianderson wrote:
> Do we need this now that we have cc::BeginFrameObserver?

IMO, both are doing different role and living in different layer although they
have a similar interface.
This is for *sending" BeginFrame where as cc::BeginFrameObserver::OnBeginFrame
is for starting
new BeginFrame.

https://codereview.chromium.org/775143003/diff/40001/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/775143003/diff/40001/ui/compositor/compositor...
ui/compositor/compositor.cc:319: last_begin_frame_args_.deadline >
base::TimeTicks::Now())
On 2014/12/09 03:00:32, brianderson wrote:
> Let's not do the deadline check here and just let the Renderer decide what it
> wants to do with the expired BeginFrame.

Yep.
If this is already expired, it will be dropped by Scheduler.

https://codereview.chromium.org/775143003/diff/40001/ui/compositor/compositor...
ui/compositor/compositor.cc:320:
observer->OnSendBeginFrame(last_begin_frame_args_);
On 2014/12/09 03:00:32, brianderson wrote:
> In this case, can you set the BeginFrame "type" to MISSED.

Done.

[brianderson, 2014-12-09 21:56:32]

https://codereview.chromium.org/775143003/diff/60001/content/browser/composit...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/775143003/diff/60001/content/browser/composit...
content/browser/compositor/delegated_frame_host.cc:949: host->Send(new
ViewMsg_BeginFrame(host->GetRoutingID(), args));
Would be nice if the BeginFrame and SetNeedsBeginFrames messages are
sent/received by the same class. Can you move the actual sending to
RenderWidgetHostViewAura?

https://codereview.chromium.org/775143003/diff/60001/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/775143003/diff/60001/ui/compositor/compositor...
ui/compositor/compositor.cc:319: last_begin_frame_args_.type =
cc::BeginFrameArgs::MISSED;
What about calling last_begin_frame_args_ missed_begin_frame_args_ and setting
the type to MISSED once where the args are stored initially.

https://codereview.chromium.org/775143003/diff/60001/ui/compositor/compositor.h
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/775143003/diff/60001/ui/compositor/compositor...
ui/compositor/compositor.h:232: bool
HasBeginFrameObserver(CompositorBeginFrameObserver* observer);
Is HasBeginFrameObserver needed outside of sanity checks? Maybe we can remove it
or move the sanity checks to be internal to ui::Compositor.

The ObserverList class already has DCHECKs to make sure observers aren't added
twice. I see that it doesn't have checks for double removal though the removal
check might be worth keeping.

[simonhong, 2014-12-11 17:45:48]

brian, comments are addressed.
Also SetAuthoritativeVSyncInterval() is added into Compositor, LTH, Proxy and
Scheduler.
I found that weiliangc@ is working on enabling STP on CrOS browser
compositor(https://codereview.chromium.org/779223007/)
I think this add will help weilianc@'s work.

https://codereview.chromium.org/775143003/diff/60001/content/browser/composit...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/775143003/diff/60001/content/browser/composit...
content/browser/compositor/delegated_frame_host.cc:949: host->Send(new
ViewMsg_BeginFrame(host->GetRoutingID(), args));
On 2014/12/09 21:56:32, brianderson wrote:
> Would be nice if the BeginFrame and SetNeedsBeginFrames messages are
> sent/received by the same class. Can you move the actual sending to
> RenderWidgetHostViewAura?

Done.

https://codereview.chromium.org/775143003/diff/60001/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/775143003/diff/60001/ui/compositor/compositor...
ui/compositor/compositor.cc:319: last_begin_frame_args_.type =
cc::BeginFrameArgs::MISSED;
On 2014/12/09 21:56:32, brianderson wrote:
> What about calling last_begin_frame_args_ missed_begin_frame_args_ and setting
> the type to MISSED once where the args are stored initially.

Done.

https://codereview.chromium.org/775143003/diff/60001/ui/compositor/compositor.h
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/775143003/diff/60001/ui/compositor/compositor...
ui/compositor/compositor.h:232: bool
HasBeginFrameObserver(CompositorBeginFrameObserver* observer);
On 2014/12/09 21:56:32, brianderson wrote:
> Is HasBeginFrameObserver needed outside of sanity checks? Maybe we can remove
it
> or move the sanity checks to be internal to ui::Compositor.
> 
> The ObserverList class already has DCHECKs to make sure observers aren't added
> twice. I see that it doesn't have checks for double removal though the removal
> check might be worth keeping.

Done.

[brianderson, 2014-12-12 01:17:02]

Simon, do you mind keeping the AuthoritativeVSyncInterval changes separate to
simplify the review? @weiliangc is landing most of the changes that applies to
the STP work in https://codereview.chromium.org/791373003, but we'll still need
to delete the CompositorVSyncManager after this patch lands for which I've
opened https://code.google.com/p/chromium/issues/detail?id=441577 .

https://codereview.chromium.org/775143003/diff/60001/ui/compositor/compositor.cc
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/775143003/diff/60001/ui/compositor/compositor...
ui/compositor/compositor.cc:319: last_begin_frame_args_.type =
cc::BeginFrameArgs::MISSED;
On 2014/12/11 17:45:48, simonhong wrote:
> On 2014/12/09 21:56:32, brianderson wrote:
> > What about calling last_begin_frame_args_ missed_begin_frame_args_ and
setting
> > the type to MISSED once where the args are stored initially.
> 
> Done.

Thanks. I realize now that the if statement above would have always failed if we
didn't set the type to MISSED before hand.

https://codereview.chromium.org/775143003/diff/80001/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/775143003/diff/80001/cc/base/switches.cc#newc...
cc/base/switches.cc:68: const char kEnableBeginFrameScheduling[] =
"enable-begin-frame-scheduling";
Where is this flag set for Aura? Is it just command line right now?

https://codereview.chromium.org/775143003/diff/80001/content/browser/composit...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/775143003/diff/80001/content/browser/composit...
content/browser/compositor/delegated_frame_host.cc:946: TRACE_EVENT0("cc",
"DelegatedFrameHost::OnSendBeginFrame");
I don't think we need a trace per Renderer. A trace in ui::Compositor is
probably good enough. Ditto for the other trace you added to this file.

https://codereview.chromium.org/775143003/diff/80001/content/browser/composit...
File content/browser/compositor/delegated_frame_host.h (right):

https://codereview.chromium.org/775143003/diff/80001/content/browser/composit...
content/browser/compositor/delegated_frame_host.h:334: cc::BeginFrameArgs
last_begin_frame_args_;
last_sent_begin_frame_args_?

[simonhong, 2014-12-15 15:28:49]

Patchset #5 (id:100001) has been deleted

[simonhong, 2014-12-15 15:37:27]

brian, I think this cl works well with
(https://codereview.chromium.org/802353002/).
But, I found one issue with dev tools.
When dev tools is  toggled, new RWHVAura is created.
The new RWHVAura's window is not connected with Compositor because
it is not the child of root window.
So, it can't receive BeginFrame message from Compositor.
Could you give any clue why this RWHV's window is not a child of root window and
how to solve this situation?

https://codereview.chromium.org/775143003/diff/80001/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/775143003/diff/80001/cc/base/switches.cc#newc...
cc/base/switches.cc:68: const char kEnableBeginFrameScheduling[] =
"enable-begin-frame-scheduling";
On 2014/12/12 01:17:02, brianderson wrote:
> Where is this flag set for Aura? Is it just command line right now?

This feature is only enabled by command line.
Should we need to add this in about://flags page?

https://codereview.chromium.org/775143003/diff/80001/content/browser/composit...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/775143003/diff/80001/content/browser/composit...
content/browser/compositor/delegated_frame_host.cc:946: TRACE_EVENT0("cc",
"DelegatedFrameHost::OnSendBeginFrame");
On 2014/12/12 01:17:02, brianderson wrote:
> I don't think we need a trace per Renderer. A trace in ui::Compositor is
> probably good enough. Ditto for the other trace you added to this file.

Removed.

https://codereview.chromium.org/775143003/diff/80001/content/browser/composit...
File content/browser/compositor/delegated_frame_host.h (right):

https://codereview.chromium.org/775143003/diff/80001/content/browser/composit...
content/browser/compositor/delegated_frame_host.h:334: cc::BeginFrameArgs
last_begin_frame_args_;
On 2014/12/12 01:17:02, brianderson wrote:
> last_sent_begin_frame_args_?

Yep, it is more clear!

[brianderson, 2014-12-16 00:06:53]

I'm not sure what to do about the dev tools issue. I saw your email to
chromium-dev. Hopefully someone will respond. If not, I can dig around.

https://codereview.chromium.org/775143003/diff/80001/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/775143003/diff/80001/cc/base/switches.cc#newc...
cc/base/switches.cc:68: const char kEnableBeginFrameScheduling[] =
"enable-begin-frame-scheduling";
On 2014/12/15 15:37:27, simonhong wrote:
> On 2014/12/12 01:17:02, brianderson wrote:
> > Where is this flag set for Aura? Is it just command line right now?
> 
> This feature is only enabled by command line.
> Should we need to add this in about://flags page?

We eventually want to just get rid of this command-line flag. Can we just set
the relevant settings from platform specific code, rather than relying on the
command-line flag?

[jbauman, 2014-12-16 05:28:40]

On 2014/12/15 15:37:27, simonhong wrote:
> brian, I think this cl works well with
> (https://codereview.chromium.org/802353002/).
> But, I found one issue with dev tools.
> When dev tools is  toggled, new RWHVAura is created.
> The new RWHVAura's window is not connected with Compositor because
> it is not the child of root window.
> So, it can't receive BeginFrame message from Compositor.
> Could you give any clue why this RWHV's window is not a child of root window
and
> how to solve this situation?

Are you sure it's not connected to a root window after it asks (and probably
receives) a begin frame? Devtools windows seem to have some complexity with the
timing of when they dock and undock, so I wouldn't be surprised if it requires a
requestAnimationFrame or similar before it docks to a window. FYI, the devtools
window is pretty much owned by DevToolsWindow. And evtools_web_view_ on
BrowserView should be a view::WebView.

Could you immediately trigger a BeginFrame if a renderer requests one and isn't
connected to a compositor?
> 
> https://codereview.chromium.org/775143003/diff/80001/cc/base/switches.cc
> File cc/base/switches.cc (right):
> 
>
https://codereview.chromium.org/775143003/diff/80001/cc/base/switches.cc#newc...
> cc/base/switches.cc:68: const char kEnableBeginFrameScheduling[] =
> "enable-begin-frame-scheduling";
> On 2014/12/12 01:17:02, brianderson wrote:
> > Where is this flag set for Aura? Is it just command line right now?
> 
> This feature is only enabled by command line.
> Should we need to add this in about://flags page?
> 
>
https://codereview.chromium.org/775143003/diff/80001/content/browser/composit...
> File content/browser/compositor/delegated_frame_host.cc (right):
> 
>
https://codereview.chromium.org/775143003/diff/80001/content/browser/composit...
> content/browser/compositor/delegated_frame_host.cc:946: TRACE_EVENT0("cc",
> "DelegatedFrameHost::OnSendBeginFrame");
> On 2014/12/12 01:17:02, brianderson wrote:
> > I don't think we need a trace per Renderer. A trace in ui::Compositor is
> > probably good enough. Ditto for the other trace you added to this file.
> 
> Removed.
> 
>
https://codereview.chromium.org/775143003/diff/80001/content/browser/composit...
> File content/browser/compositor/delegated_frame_host.h (right):
> 
>
https://codereview.chromium.org/775143003/diff/80001/content/browser/composit...
> content/browser/compositor/delegated_frame_host.h:334: cc::BeginFrameArgs
> last_begin_frame_args_;
> On 2014/12/12 01:17:02, brianderson wrote:
> > last_sent_begin_frame_args_?
> 
> Yep, it is more clear!

[mithro-old, 2014-12-16 10:50:58]

Hi Simon,

I haven't had a chance to read through the existing reviews or look at older
versions of this patch but wanted to add the following comments first.

Tim

https://codereview.chromium.org/775143003/diff/120001/cc/output/begin_frame_a...
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/775143003/diff/120001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:86: bool operator==(const BeginFrameArgs& other)
const {
Why have these been defined? We have explicitly kept these operators only
available in testing so people use them in production.

https://codereview.chromium.org/775143003/diff/120001/ui/base/compositor/comp...
File ui/base/compositor/compositor_begin_frame_observer.h (right):

https://codereview.chromium.org/775143003/diff/120001/ui/base/compositor/comp...
ui/base/compositor/compositor_begin_frame_observer.h:3: // found in the LICENSE
file.
Is this being added because BeginFrameObserver in
cc/scheduler/begin_frame_source.h can't be used in ui/base?

Can cc/ depend on ui/base - if so we should probably just move the file from
cc/scheduler into ui/base/compositor

https://codereview.chromium.org/775143003/diff/120001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/775143003/diff/120001/ui/compositor/composito...
ui/compositor/compositor.cc:320:
observer->OnSendBeginFrame(missed_begin_frame_args_);
I don't think this is correct, if the observer was a proper BeginFrameObserver
the code would read something like;

if (!new_args.IsValid())
  return;

// If the last begin frame is invalid, then any new begin frame is valid.
if (observer->LastUsedBeginFrameArgs().IsValid())
  OnSendBeginFrame(new_args);

// Only allow new args have a *strictly bigger* frame_time value
if (new_args.frame_time >= observer->LastUsedBeginFrameArgs().frame_time)
  OnSendBeginFrame(new_args);

https://codereview.chromium.org/775143003/diff/120001/ui/compositor/compositor.h
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/775143003/diff/120001/ui/compositor/composito...
ui/compositor/compositor.h:334: // Used to send to any new
CompositorBeginFrameObserver immediately.
BTW Do we actually want this functionality?

[simonhong, 2014-12-16 15:13:29]

simonhong@chromium.org changed reviewers:
+ jbauman@chromium.org

[simonhong, 2014-12-16 15:13:30]

jbauman@, thanks for pointing out.
You're thought is right. BeginFrame is requested earlier than it is added into
the root window tree. This problem is fixed.
Also, I modified to send BeginFrame message before it is registered as an
BeginFrameObserver.

mithro@, please check my comments.
brianderson@, this feature is on in the platform specific define.

https://codereview.chromium.org/775143003/diff/80001/cc/base/switches.cc
File cc/base/switches.cc (right):

https://codereview.chromium.org/775143003/diff/80001/cc/base/switches.cc#newc...
cc/base/switches.cc:68: const char kEnableBeginFrameScheduling[] =
"enable-begin-frame-scheduling";
On 2014/12/16 00:06:52, brianderson wrote:
> On 2014/12/15 15:37:27, simonhong wrote:
> > On 2014/12/12 01:17:02, brianderson wrote:
> > > Where is this flag set for Aura? Is it just command line right now?
> > 
> > This feature is only enabled by command line.
> > Should we need to add this in about://flags page?
> 
> We eventually want to just get rid of this command-line flag. Can we just set
> the relevant settings from platform specific code, rather than relying on the
> command-line flag?

Changed to set this in the platform specific define.

https://codereview.chromium.org/775143003/diff/120001/cc/output/begin_frame_a...
File cc/output/begin_frame_args.h (right):

https://codereview.chromium.org/775143003/diff/120001/cc/output/begin_frame_a...
cc/output/begin_frame_args.h:86: bool operator==(const BeginFrameArgs& other)
const {
On 2014/12/16 10:50:57, mithro wrote:
> Why have these been defined? We have explicitly kept these operators only
> available in testing so people use them in production.

In ui::Compositor, I need to compare two BeginFrameArgs.
So, I moved this from testing code to here.

https://codereview.chromium.org/775143003/diff/120001/ui/base/compositor/comp...
File ui/base/compositor/compositor_begin_frame_observer.h (right):

https://codereview.chromium.org/775143003/diff/120001/ui/base/compositor/comp...
ui/base/compositor/compositor_begin_frame_observer.h:3: // found in the LICENSE
file.
On 2014/12/16 10:50:58, mithro wrote:
> Is this being added because BeginFrameObserver in
> cc/scheduler/begin_frame_source.h can't be used in ui/base?
> 
> Can cc/ depend on ui/base - if so we should probably just move the file from
> cc/scheduler into ui/base/compositor

I thought cc::BeginFrameObserver and ui:CompositorBeginFrameObserver has
different source.
Former is for BeginFrameSource and latter is for Compositor.
Both are living different layer and target.
Do we need to use same interface just because both have a similar interface?

https://codereview.chromium.org/775143003/diff/120001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/775143003/diff/120001/ui/compositor/composito...
ui/compositor/compositor.cc:320:
observer->OnSendBeginFrame(missed_begin_frame_args_);
On 2014/12/16 10:50:58, mithro wrote:
> I don't think this is correct, if the observer was a proper BeginFrameObserver
> the code would read something like;
> 
> if (!new_args.IsValid())
>   return;
> 
> // If the last begin frame is invalid, then any new begin frame is valid.
> if (observer->LastUsedBeginFrameArgs().IsValid())
>   OnSendBeginFrame(new_args);
> 
> // Only allow new args have a *strictly bigger* frame_time value
> if (new_args.frame_time >= observer->LastUsedBeginFrameArgs().frame_time)
>   OnSendBeginFrame(new_args);

This code can be effective when observer is removed and added again and
|missed_begin_frame_args_| is still effective.
In this case, |missed_begin_frame_args_| can be delivered immediately to
renderer.
Of course, |missed_begin_frame_args_| 's deadline can be passed.
I and brian thought that this can be handled in the Scheduler side.
Old BeginFrameArgs with MISSED type will be dropped by RetroFrame handler.
So we are sending |missed_begin_frame_args_| to renderer without much
inspection.

https://codereview.chromium.org/775143003/diff/120001/ui/compositor/compositor.h
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/775143003/diff/120001/ui/compositor/composito...
ui/compositor/compositor.h:334: // Used to send to any new
CompositorBeginFrameObserver immediately.
On 2014/12/16 10:50:58, mithro wrote:
> BTW Do we actually want this functionality?

I think this is useful when new observer is added.
With this, new observer can deliver BeginFrame without waiting next BeginFrame
if this is still effective.

[danakj, 2014-12-16 15:44:11]

https://codereview.chromium.org/775143003/diff/120001/ui/base/compositor/comp...
File ui/base/compositor/compositor_begin_frame_observer.h (right):

https://codereview.chromium.org/775143003/diff/120001/ui/base/compositor/comp...
ui/base/compositor/compositor_begin_frame_observer.h:3: // found in the LICENSE
file.
On 2014/12/16 10:50:58, mithro wrote:
> Is this being added because BeginFrameObserver in
> cc/scheduler/begin_frame_source.h can't be used in ui/base?
> 
> Can cc/ depend on ui/base - if so we should probably just move the file from
> cc/scheduler into ui/base/compositor

Ui depends on cc so can't go the other way. It should stay in cc/ if its used by
cc and ui. If cc only needs a base class (interface) then it should move

[brianderson, 2014-12-16 23:23:52]

https://codereview.chromium.org/775143003/diff/120001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/775143003/diff/120001/ui/compositor/composito...
ui/compositor/compositor.cc:320:
observer->OnSendBeginFrame(missed_begin_frame_args_);
On 2014/12/16 15:13:30, simonhong wrote:
> On 2014/12/16 10:50:58, mithro wrote:
> > I don't think this is correct, if the observer was a proper
BeginFrameObserver
> > the code would read something like;
> > 
> > if (!new_args.IsValid())
> >   return;
> > 
> > // If the last begin frame is invalid, then any new begin frame is valid.
> > if (observer->LastUsedBeginFrameArgs().IsValid())
> >   OnSendBeginFrame(new_args);
> > 
> > // Only allow new args have a *strictly bigger* frame_time value
> > if (new_args.frame_time >= observer->LastUsedBeginFrameArgs().frame_time)
> >   OnSendBeginFrame(new_args);
> 
> This code can be effective when observer is removed and added again and
> |missed_begin_frame_args_| is still effective.
> In this case, |missed_begin_frame_args_| can be delivered immediately to
> renderer.
> Of course, |missed_begin_frame_args_| 's deadline can be passed.
> I and brian thought that this can be handled in the Scheduler side.
> Old BeginFrameArgs with MISSED type will be dropped by RetroFrame handler.
> So we are sending |missed_begin_frame_args_| to renderer without much
> inspection.

@mithro's suggestion enforces monotonicity of the frame_time, which is
important, especially now that the latest patch sends a fake BeginFrame for the
first frame of dev tools.

Before the dev tools change, I think the guarantee of monotonicity from the
scheduler's begin frame source would have been good enough.

https://codereview.chromium.org/775143003/diff/140001/content/browser/composi...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/775143003/diff/140001/content/browser/composi...
content/browser/compositor/delegated_frame_host.cc:889:
OnSendBeginFrame(cc::BeginFrameArgs::Create(
We'll need some guarantee of monotonicity of the frame time of the real
BeginFrame that comes after this.

[brianderson, 2014-12-17 01:44:55]

https://codereview.chromium.org/775143003/diff/120001/ui/base/compositor/comp...
File ui/base/compositor/compositor_begin_frame_observer.h (right):

https://codereview.chromium.org/775143003/diff/120001/ui/base/compositor/comp...
ui/base/compositor/compositor_begin_frame_observer.h:3: // found in the LICENSE
file.
On 2014/12/16 15:44:11, danakj wrote:
> On 2014/12/16 10:50:58, mithro wrote:
> > Is this being added because BeginFrameObserver in
> > cc/scheduler/begin_frame_source.h can't be used in ui/base?
> > 
> > Can cc/ depend on ui/base - if so we should probably just move the file from
> > cc/scheduler into ui/base/compositor
> 
> Ui depends on cc so can't go the other way. It should stay in cc/ if its used
by
> cc and ui. If cc only needs a base class (interface) then it should move

If we want to unify the Observer classes, we'll need to keep the single observer
in cc then.

Could we:
1) Get rid of the ChildrenNeedBeginFrames paths that Simon landed in a previous
patch.
2) Have the ui::Compositor own the external BeginFrameSource.
3) Only give cc::Scheduler a raw pointer to the external BeginFrameSource,
instead of having the scheduler own it. (Enne mentioned they want to do this for
Android anyway.)
4) Make RWHVA inherit and complete BeginFrameObserverMixIn.
5) Allow the RWHVA to add itself as an observer to the external BeginFrame
source that the ui::Compositor owns.

[mithro-old, 2014-12-18 16:05:52]

On 2014/12/17 01:44:55, brianderson wrote:
>
https://codereview.chromium.org/775143003/diff/120001/ui/base/compositor/comp...
> File ui/base/compositor/compositor_begin_frame_observer.h (right):
> 
>
https://codereview.chromium.org/775143003/diff/120001/ui/base/compositor/comp...
> ui/base/compositor/compositor_begin_frame_observer.h:3: // found in the
LICENSE
> file.
> On 2014/12/16 15:44:11, danakj wrote:
> > On 2014/12/16 10:50:58, mithro wrote:
> > > Is this being added because BeginFrameObserver in
> > > cc/scheduler/begin_frame_source.h can't be used in ui/base?
> > > 
> > > Can cc/ depend on ui/base - if so we should probably just move the file
from
> > > cc/scheduler into ui/base/compositor
> > 
> > Ui depends on cc so can't go the other way. It should stay in cc/ if its
used
> by
> > cc and ui. If cc only needs a base class (interface) then it should move
> 
> If we want to unify the Observer classes, we'll need to keep the single
observer
> in cc then.
> 
> Could we:
> 1) Get rid of the ChildrenNeedBeginFrames paths that Simon landed in a
previous
> patch.
> 2) Have the ui::Compositor own the external BeginFrameSource.
> 3) Only give cc::Scheduler a raw pointer to the external BeginFrameSource,
> instead of having the scheduler own it. (Enne mentioned they want to do this
for
> Android anyway.)
> 4) Make RWHVA inherit and complete BeginFrameObserverMixIn.
> 5) Allow the RWHVA to add itself as an observer to the external BeginFrame
> source that the ui::Compositor owns.

Hi Brian,

Having the BeginFrame message "pass through" the scheduler is actually pretty
useful because the scheduler can manipulate the deadlines to take into account
the nested draw times and such. My suggestion is as follows;

1) Refactor Simon's ChildrenNeedBeginFrames code so that the scheduler
implements the BeginFrameSource interface.
2) Make RWHVA inherit and complete BeginFrameObserverMixIn.
3) Make the RWHVA obverse the scheduler as it's "BeginFrameSource".

The scheduler becomes a "modify and forward" type system. Both the
ThrottledBeginFrameSource (which made sure to have a minimum interval between
BeginFrame messages) and the ProxyBeginFrameSource classes (that didn't end up
getting landed - see
https://codereview.chromium.org/267783004/diff/160001/cc/scheduler/frame_sour...
for some very old code) are perfect example of this type of idea. The
BeginFrameSourceMultiplexer is also a more complicated version of this idea.

This has a couple of advantages;
 * We can choose to change to your system in the future without having to change
the RWHVA at all. It just stops observing the scheduler and instead starts
observing the ExternalBFS directly.
 * We could connect the RWHVA to a different BFS if we wanted such as a
BackToBackFrameSource if we wanted to do some type of unthrottled version.
 * The scheduler could be smarter in how / when it sends a BeginFrame message to
the RWHVA and everything keeps working.

This is the power of using a common interface for everything :)

Tim 'mithro' Ansell

[simonhong, 2014-12-18 16:10:38]

brian, do you mean that cc::Scheduler and each DelegatedFrameHost should be an
observer of externalBFS?

I think that model works well on Mac and Android because they provide vsync
regularly.
But, windows or linux doesn't.(they rely on syntheticBFS)

https://codereview.chromium.org/775143003/diff/140001/content/browser/composi...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/775143003/diff/140001/content/browser/composi...
content/browser/compositor/delegated_frame_host.cc:889:
OnSendBeginFrame(cc::BeginFrameArgs::Create(
On 2014/12/16 23:23:52, brianderson wrote:
> We'll need some guarantee of monotonicity of the frame time of the real
> BeginFrame that comes after this.

Removed this. I think sending BeginFrame after this is connected to compositor
is also ok.

[mithro-old, 2014-12-18 16:19:05]

https://codereview.chromium.org/775143003/diff/140001/content/browser/composi...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/775143003/diff/140001/content/browser/composi...
content/browser/compositor/delegated_frame_host.cc:892: gfx::FrameTime::Now(),
EKK! You should never be using Now() in any code related BeginFrames.

[brianderson, 2014-12-24 00:23:08]

On 2014/12/18 16:05:52, mithro (OO till Feb 16th) wrote:
> On 2014/12/17 01:44:55, brianderson wrote:
> >
>
https://codereview.chromium.org/775143003/diff/120001/ui/base/compositor/comp...
> > File ui/base/compositor/compositor_begin_frame_observer.h (right):
> > 
> >
>
https://codereview.chromium.org/775143003/diff/120001/ui/base/compositor/comp...
> > ui/base/compositor/compositor_begin_frame_observer.h:3: // found in the
> LICENSE
> > file.
> > On 2014/12/16 15:44:11, danakj wrote:
> > > On 2014/12/16 10:50:58, mithro wrote:
> > > > Is this being added because BeginFrameObserver in
> > > > cc/scheduler/begin_frame_source.h can't be used in ui/base?
> > > > 
> > > > Can cc/ depend on ui/base - if so we should probably just move the file
> from
> > > > cc/scheduler into ui/base/compositor
> > > 
> > > Ui depends on cc so can't go the other way. It should stay in cc/ if its
> used
> > by
> > > cc and ui. If cc only needs a base class (interface) then it should move
> > 
> > If we want to unify the Observer classes, we'll need to keep the single
> observer
> > in cc then.
> > 
> > Could we:
> > 1) Get rid of the ChildrenNeedBeginFrames paths that Simon landed in a
> previous
> > patch.
> > 2) Have the ui::Compositor own the external BeginFrameSource.
> > 3) Only give cc::Scheduler a raw pointer to the external BeginFrameSource,
> > instead of having the scheduler own it. (Enne mentioned they want to do this
> for
> > Android anyway.)
> > 4) Make RWHVA inherit and complete BeginFrameObserverMixIn.
> > 5) Allow the RWHVA to add itself as an observer to the external BeginFrame
> > source that the ui::Compositor owns.
> 
> Hi Brian,
> 
> Having the BeginFrame message "pass through" the scheduler is actually pretty
> useful because the scheduler can manipulate the deadlines to take into account
> the nested draw times and such. My suggestion is as follows;

You are right, Tim. I forgot the deadline needs adjustment.

> 
> 1) Refactor Simon's ChildrenNeedBeginFrames code so that the scheduler
> implements the BeginFrameSource interface.
> 2) Make RWHVA inherit and complete BeginFrameObserverMixIn.
> 3) Make the RWHVA obverse the scheduler as it's "BeginFrameSource".
> 

I'm preferring Simon's existing approach now, although it might still be nice to
have a re-usable ProxyBeginFrameSource of some sort in cc, rather than ui.
Reasoning:

I'm not sure we should make the scheduler a BeginFrameSource. It would involve
more setup boilerplate to communicate the scheduler as the source back to the
ui::Compositor and add indirection that is difficult to follow by reading the
code.

A ProxyBeginFrameSource, like you mention below, sounds like it would not have
as much boilerplate as making Scheduler a BeginFrameSource. The ui::Compositor
could create it and pass it down the same path as the external BeginFrame
source. It would also be re-usable once we have a DisplayScheduler for Surfaces
and would prevent us from overloading the scheduler. Until we have a
DisplayScheduler though, this would just add indirection compared to what Simon
has here.

Simon, feel free to keep the structure you already have in this patch (using
client->SendBeginFramesToChildren) which is very direct and easy to follow. It
would be nice to have a re-usable Observer class that we can use with the
DisplayScheduler, but we can worry about that when we get there instead of
speculating.


> The scheduler becomes a "modify and forward" type system. Both the
> ThrottledBeginFrameSource (which made sure to have a minimum interval between
> BeginFrame messages) and the ProxyBeginFrameSource classes (that didn't end up
> getting landed - see
>
https://codereview.chromium.org/267783004/diff/160001/cc/scheduler/frame_sour...
> for some very old code) are perfect example of this type of idea. The
> BeginFrameSourceMultiplexer is also a more complicated version of this idea.
> 
> This has a couple of advantages;
>  * We can choose to change to your system in the future without having to
change
> the RWHVA at all. It just stops observing the scheduler and instead starts
> observing the ExternalBFS directly.
>  * We could connect the RWHVA to a different BFS if we wanted such as a
> BackToBackFrameSource if we wanted to do some type of unthrottled version.
>  * The scheduler could be smarter in how / when it sends a BeginFrame message
to
> the RWHVA and everything keeps working.
> 
> This is the power of using a common interface for everything :)
> 
> Tim 'mithro' Ansell

[simonhong, 2015-01-19 14:55:16]

On 2014/12/24 00:23:08, brianderson wrote:
> On 2014/12/18 16:05:52, mithro (OO till Feb 16th) wrote:
> > On 2014/12/17 01:44:55, brianderson wrote:
> > >
> >
>
https://codereview.chromium.org/775143003/diff/120001/ui/base/compositor/comp...
> > > File ui/base/compositor/compositor_begin_frame_observer.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/775143003/diff/120001/ui/base/compositor/comp...
> > > ui/base/compositor/compositor_begin_frame_observer.h:3: // found in the
> > LICENSE
> > > file.
> > > On 2014/12/16 15:44:11, danakj wrote:
> > > > On 2014/12/16 10:50:58, mithro wrote:
> > > > > Is this being added because BeginFrameObserver in
> > > > > cc/scheduler/begin_frame_source.h can't be used in ui/base?
> > > > > 
> > > > > Can cc/ depend on ui/base - if so we should probably just move the
file
> > from
> > > > > cc/scheduler into ui/base/compositor
> > > > 
> > > > Ui depends on cc so can't go the other way. It should stay in cc/ if its
> > used
> > > by
> > > > cc and ui. If cc only needs a base class (interface) then it should move
> > > 
> > > If we want to unify the Observer classes, we'll need to keep the single
> > observer
> > > in cc then.
> > > 
> > > Could we:
> > > 1) Get rid of the ChildrenNeedBeginFrames paths that Simon landed in a
> > previous
> > > patch.
> > > 2) Have the ui::Compositor own the external BeginFrameSource.
> > > 3) Only give cc::Scheduler a raw pointer to the external BeginFrameSource,
> > > instead of having the scheduler own it. (Enne mentioned they want to do
this
> > for
> > > Android anyway.)
> > > 4) Make RWHVA inherit and complete BeginFrameObserverMixIn.
> > > 5) Allow the RWHVA to add itself as an observer to the external BeginFrame
> > > source that the ui::Compositor owns.
> > 
> > Hi Brian,
> > 
> > Having the BeginFrame message "pass through" the scheduler is actually
pretty
> > useful because the scheduler can manipulate the deadlines to take into
account
> > the nested draw times and such. My suggestion is as follows;
> 
> You are right, Tim. I forgot the deadline needs adjustment.
> 
> > 
> > 1) Refactor Simon's ChildrenNeedBeginFrames code so that the scheduler
> > implements the BeginFrameSource interface.
> > 2) Make RWHVA inherit and complete BeginFrameObserverMixIn.
> > 3) Make the RWHVA obverse the scheduler as it's "BeginFrameSource".
> > 
> 
> I'm preferring Simon's existing approach now, although it might still be nice
to
> have a re-usable ProxyBeginFrameSource of some sort in cc, rather than ui.
> Reasoning:
> 
> I'm not sure we should make the scheduler a BeginFrameSource. It would involve
> more setup boilerplate to communicate the scheduler as the source back to the
> ui::Compositor and add indirection that is difficult to follow by reading the
> code.
> 
> A ProxyBeginFrameSource, like you mention below, sounds like it would not have
> as much boilerplate as making Scheduler a BeginFrameSource. The ui::Compositor
> could create it and pass it down the same path as the external BeginFrame
> source. It would also be re-usable once we have a DisplayScheduler for
Surfaces
> and would prevent us from overloading the scheduler. Until we have a
> DisplayScheduler though, this would just add indirection compared to what
Simon
> has here.
> 
> Simon, feel free to keep the structure you already have in this patch (using
> client->SendBeginFramesToChildren) which is very direct and easy to follow. It
> would be nice to have a re-usable Observer class that we can use with the
> DisplayScheduler, but we can worry about that when we get there instead of
> speculating.
> 
> 
> > The scheduler becomes a "modify and forward" type system. Both the
> > ThrottledBeginFrameSource (which made sure to have a minimum interval
between
> > BeginFrame messages) and the ProxyBeginFrameSource classes (that didn't end
up
> > getting landed - see
> >
>
https://codereview.chromium.org/267783004/diff/160001/cc/scheduler/frame_sour...
> > for some very old code) are perfect example of this type of idea. The
> > BeginFrameSourceMultiplexer is also a more complicated version of this idea.
> > 
> > This has a couple of advantages;
> >  * We can choose to change to your system in the future without having to
> change
> > the RWHVA at all. It just stops observing the scheduler and instead starts
> > observing the ExternalBFS directly.
> >  * We could connect the RWHVA to a different BFS if we wanted such as a
> > BackToBackFrameSource if we wanted to do some type of unthrottled version.
> >  * The scheduler could be smarter in how / when it sends a BeginFrame
message
> to
> > the RWHVA and everything keeps working.
> > 
> > This is the power of using a common interface for everything :)
> > 
> > Tim 'mithro' Ansell

This will be rebased when ProxyBeginFrameSource is
landed.(https://codereview.chromium.org/845393002/)

[brianderson, 2015-02-14 00:03:37]

@weiliangc: It looks like https://codereview.chromium.org/781163003/ has enabled
STP+SchedulerClient for CrOS and has been in for 1 week. Do you think it will
continue to stick?

@simonhong: Can you rebase this patch? If Wei's patch sticks, we should be able
to land it soon.

[weiliangc, 2015-02-17 19:18:53]

On 2015/02/14 at 00:03:37, brianderson wrote:
> @weiliangc: It looks like https://codereview.chromium.org/781163003/ has
enabled STP+SchedulerClient for CrOS and has been in for 1 week. Do you think it
will continue to stick?
> 
Should continue to stick.

There are some windows regression bug from browser compositor scheduler patch
but shouldn't be causing revert. CrOS seems pretty stable right now.

> @simonhong: Can you rebase this patch? If Wei's patch sticks, we should be
able to land it soon.

[simonhong, 2015-03-02 15:27:20]

On 2015/02/14 00:03:37, brianderson wrote:
> @weiliangc: It looks like https://codereview.chromium.org/781163003/ has
enabled
> STP+SchedulerClient for CrOS and has been in for 1 week. Do you think it will
> continue to stick?
> 
> @simonhong: Can you rebase this patch? If Wei's patch sticks, we should be
able
> to land it soon.

@brianderson, PTAL!

[brianderson, 2015-03-02 22:38:17]

https://codereview.chromium.org/775143003/diff/200001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/775143003/diff/200001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:204: base::TimeTicks last_timebase_;
last_vsync_timebase_?

https://codereview.chromium.org/775143003/diff/200001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_unittest.cc (left):

https://codereview.chromium.org/775143003/diff/200001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_unittest.cc:535: scheduler_->SetCanDraw(true);
Why was this block removed?

https://codereview.chromium.org/775143003/diff/200001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_unittest.cc:571: scheduler_->SetCanDraw(true);
Why was this block removed?

https://codereview.chromium.org/775143003/diff/200001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/775143003/diff/200001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_unittest.cc:2413: EXPECT_EQ(authoritative_interval,
Can you also EXPECT_NE to initial_interval so it's really clear this test is
testing what it thinks it's testing.

https://codereview.chromium.org/775143003/diff/200001/content/browser/composi...
File content/browser/compositor/browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/775143003/diff/200001/content/browser/composi...
content/browser/compositor/browser_compositor_output_surface.cc:33: if
(!HasClient())
This if can be removed now.

https://codereview.chromium.org/775143003/diff/200001/content/browser/composi...
content/browser/compositor/browser_compositor_output_surface.cc:46: // Don't
want vsync notifications until there is a client.
This comment can be removed.

https://codereview.chromium.org/775143003/diff/200001/content/browser/composi...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/775143003/diff/200001/content/browser/composi...
content/browser/compositor/delegated_frame_host.cc:911:
last_sent_begin_frame_args_ =args;
Spaces needed around the =.

https://codereview.chromium.org/775143003/diff/200001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (left):

https://codereview.chromium.org/775143003/diff/200001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:443: void
RenderWidgetHostViewMac::DelegatedFrameHostUpdateVSyncParameters(
Can you have the SendBeginFrame forward the timebase and interval to
render_widget_host_->UpdateVSyncParameters here?

You have a separate patch that will get the VSync callback directly for MacOS,
but until then, we'll need to make sure to call UpdateVSyncParameters. Otherwise
the Renderer will never get the real vsync info.

https://codereview.chromium.org/775143003/diff/200001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/775143003/diff/200001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:549:
delegated_frame_host_.reset(new DelegatedFrameHost(this, false));
To forward SendBeginFrame to UpdateVSyncParameters, this will have to be "true"
after which you can just get rid of this flag since no instance will be "false".

https://codereview.chromium.org/775143003/diff/200001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/775143003/diff/200001/ui/compositor/composito...
ui/compositor/compositor.cc:97: settings.forward_begin_frames_to_children =
true;
This flag doesn't seem useful like we thought it would be. Just get rid of it?

Followup patch is fine, but please open a new issue.

https://codereview.chromium.org/775143003/diff/200001/ui/compositor/composito...
ui/compositor/compositor.cc:287: void Compositor::AddBeginFrameObserver(
Please add unit tests for this since the last_begin_frame_args_sent_to_observer
logic is hard to get right.

https://codereview.chromium.org/775143003/diff/200001/ui/compositor/composito...
ui/compositor/compositor.cc:292: if
(!last_begin_frame_args_sent_to_observer.deadline.is_null() &&
Is this check for a null deadline necessary?

https://codereview.chromium.org/775143003/diff/200001/ui/compositor/composito...
ui/compositor/compositor.cc:293: last_begin_frame_args_sent_to_observer !=
missed_begin_frame_args_) {
It is possible that last_begin_frame_args_sent_to_observer.type and 
missed_begin_frame_args_.type are not equal even though they are otherwise
equal.

Can you compare the frame_time's instead so that the type doesn't mess us up?

https://codereview.chromium.org/775143003/diff/200001/ui/compositor/composito...
ui/compositor/compositor.cc:294: missed_begin_frame_args_.type =
cc::BeginFrameArgs::MISSED;
Can you move this line to the end of Compositor::SendBeginFramesToChildren?

[simonhong, 2015-03-02 22:52:21]

https://codereview.chromium.org/775143003/diff/200001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (left):

https://codereview.chromium.org/775143003/diff/200001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:443: void
RenderWidgetHostViewMac::DelegatedFrameHostUpdateVSyncParameters(
On 2015/03/02 22:38:16, brianderson wrote:
> Can you have the SendBeginFrame forward the timebase and interval to
> render_widget_host_->UpdateVSyncParameters here?
> 
> You have a separate patch that will get the VSync callback directly for MacOS,
> but until then, we'll need to make sure to call UpdateVSyncParameters.
Otherwise
> the Renderer will never get the real vsync info.

I removed this method because MacOSX didn't receive vsync infos from
CompositorVSyncManager.
IIUC, Mac only relys on vsync info from DisplayLinkMac.

[simonhong, 2015-03-03 22:54:12]

brian, please check again except for adding unittest for
AddBeginFrameObserver().
I'm trying to find where to add that test.
Thanks for review!

https://codereview.chromium.org/775143003/diff/200001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/775143003/diff/200001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:204: base::TimeTicks last_timebase_;
On 2015/03/02 22:38:15, brianderson wrote:
> last_vsync_timebase_?

Done.

https://codereview.chromium.org/775143003/diff/200001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_unittest.cc (left):

https://codereview.chromium.org/775143003/diff/200001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_unittest.cc:535: scheduler_->SetCanDraw(true);
On 2015/03/02 22:38:15, brianderson wrote:
> Why was this block removed?

This is just a clean up.
Above three function calls are already done by SetUpScheduler(true).

https://codereview.chromium.org/775143003/diff/200001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_unittest.cc:571: scheduler_->SetCanDraw(true);
On 2015/03/02 22:38:16, brianderson wrote:
> Why was this block removed?

ditto.

https://codereview.chromium.org/775143003/diff/200001/cc/scheduler/scheduler_...
File cc/scheduler/scheduler_unittest.cc (right):

https://codereview.chromium.org/775143003/diff/200001/cc/scheduler/scheduler_...
cc/scheduler/scheduler_unittest.cc:2413: EXPECT_EQ(authoritative_interval,
On 2015/03/02 22:38:16, brianderson wrote:
> Can you also EXPECT_NE to initial_interval so it's really clear this test is
> testing what it thinks it's testing.

Done.

https://codereview.chromium.org/775143003/diff/200001/content/browser/composi...
File content/browser/compositor/browser_compositor_output_surface.cc (right):

https://codereview.chromium.org/775143003/diff/200001/content/browser/composi...
content/browser/compositor/browser_compositor_output_surface.cc:33: if
(!HasClient())
On 2015/03/02 22:38:16, brianderson wrote:
> This if can be removed now.

Done.

https://codereview.chromium.org/775143003/diff/200001/content/browser/composi...
content/browser/compositor/browser_compositor_output_surface.cc:46: // Don't
want vsync notifications until there is a client.
On 2015/03/02 22:38:16, brianderson wrote:
> This comment can be removed.

Done.

https://codereview.chromium.org/775143003/diff/200001/content/browser/composi...
File content/browser/compositor/delegated_frame_host.cc (right):

https://codereview.chromium.org/775143003/diff/200001/content/browser/composi...
content/browser/compositor/delegated_frame_host.cc:911:
last_sent_begin_frame_args_ =args;
On 2015/03/02 22:38:16, brianderson wrote:
> Spaces needed around the =.

Done.

https://codereview.chromium.org/775143003/diff/200001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://codereview.chromium.org/775143003/diff/200001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:549:
delegated_frame_host_.reset(new DelegatedFrameHost(this, false));
On 2015/03/02 22:38:16, brianderson wrote:
> To forward SendBeginFrame to UpdateVSyncParameters, this will have to be
"true"
> after which you can just get rid of this flag since no instance will be
"false".

This flag will be removed when unified BeginFrame is enabled on Mac.
Until unified BeginFrame is enabled on Mac, this flag should be set to false
because vsync info is delivered from DisplayLinkMac to RenderWidgetHost via
RWHVMac.

https://codereview.chromium.org/775143003/diff/200001/ui/compositor/composito...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/775143003/diff/200001/ui/compositor/composito...
ui/compositor/compositor.cc:97: settings.forward_begin_frames_to_children =
true;
On 2015/03/02 22:38:16, brianderson wrote:
> This flag doesn't seem useful like we thought it would be. Just get rid of it?
> 
> Followup patch is fine, but please open a new issue.

Yep, setting state_machine.SetChildrenNeedBeginFrames() is sufficient.
Removed.

https://codereview.chromium.org/775143003/diff/200001/ui/compositor/composito...
ui/compositor/compositor.cc:292: if
(!last_begin_frame_args_sent_to_observer.deadline.is_null() &&
On 2015/03/02 22:38:16, brianderson wrote:
> Is this check for a null deadline necessary?

I also can't remember why I added this a while ago. ;)
Removed.

https://codereview.chromium.org/775143003/diff/200001/ui/compositor/composito...
ui/compositor/compositor.cc:293: last_begin_frame_args_sent_to_observer !=
missed_begin_frame_args_) {
On 2015/03/02 22:38:16, brianderson wrote:
> It is possible that last_begin_frame_args_sent_to_observer.type and 
> missed_begin_frame_args_.type are not equal even though they are otherwise
> equal.
> 
> Can you compare the frame_time's instead so that the type doesn't mess us up?

Done.

https://codereview.chromium.org/775143003/diff/200001/ui/compositor/composito...
ui/compositor/compositor.cc:294: missed_begin_frame_args_.type =
cc::BeginFrameArgs::MISSED;
On 2015/03/02 22:38:17, brianderson wrote:
> Can you move this line to the end of Compositor::SendBeginFramesToChildren?

Done.

[brianderson, 2015-03-04 02:16:18]

Thanks for the updates. Looking good.

https://codereview.chromium.org/775143003/diff/200001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_mac.mm (left):

https://codereview.chromium.org/775143003/diff/200001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_mac.mm:443: void
RenderWidgetHostViewMac::DelegatedFrameHostUpdateVSyncParameters(
On 2015/03/02 22:52:21, simonhong wrote:
> On 2015/03/02 22:38:16, brianderson wrote:
> > Can you have the SendBeginFrame forward the timebase and interval to
> > render_widget_host_->UpdateVSyncParameters here?
> > 
> > You have a separate patch that will get the VSync callback directly for
MacOS,
> > but until then, we'll need to make sure to call UpdateVSyncParameters.
> Otherwise
> > the Renderer will never get the real vsync info.
> 
> I removed this method because MacOSX didn't receive vsync infos from
> CompositorVSyncManager.
> IIUC, Mac only relys on vsync info from DisplayLinkMac.

Thanks for pointing that out. You can ignore my comment then =)

[simonhong, 2015-03-06 18:34:26]

brian, I added compositor_unittest.cc
Please check again.
Thanks!

[brianderson, 2015-03-06 20:37:00]

https://codereview.chromium.org/775143003/diff/260001/ui/compositor/compositor.h
File ui/compositor/compositor.h (right):

https://codereview.chromium.org/775143003/diff/260001/ui/compositor/composito...
ui/compositor/compositor.h:305: friend class CompositorTestAPI;
Instead of adding a CompositorTestAPI as a friend, can you:
1) Make this section protected.
2) Create a TestCompositor (to replace CompositorTestAPI) that inherits from
ui::Compositor in compositor_unittest.cc.
3) Delete compositor_test_api.h and compositor_test_api.cc

It's a pattern that's used often in Chrome. It helps avoid putting test-specific
code in the production code.

https://codereview.chromium.org/775143003/diff/260001/ui/compositor/composito...
File ui/compositor/compositor_unittest.cc (right):

https://codereview.chromium.org/775143003/diff/260001/ui/compositor/composito...
ui/compositor/compositor_unittest.cc:53: TestCompositorHost::Create(host_bounds,
context_factory));
It looks like TestCompositorHost is used to test platform specific code.

Is it possible to create a ui::Compositor directly in a platform agnostic way?
For example, by using a kNullAcceleratedWidget?

Alternatively, if creating a ui::Compositor directly is too hard/invasive, maybe
pull the more complicated Observer logic out of ui::Compositor and put it into a
delegate class that you can unit test more easily?

I see why you may have gone the CompositorTestAPI approach now...

danakj: Do you have any thoughts on which approach would be best?

https://codereview.chromium.org/775143003/diff/260001/ui/compositor/composito...
ui/compositor/compositor_unittest.cc:79: TEST_F(BeginFrameWithCompositorTest,
AddBeginFrameObserver) {
The test looks good, but please add comments explaining what this test is
testing and why, both at the top and in the body.

[danakj, 2015-03-10 00:40:44]

https://codereview.chromium.org/775143003/diff/260001/ui/compositor/composito...
File ui/compositor/compositor_unittest.cc (right):

https://codereview.chromium.org/775143003/diff/260001/ui/compositor/composito...
ui/compositor/compositor_unittest.cc:51: const gfx::Rect host_bounds(10, 10,
500, 500);
kHostBounds for compile time constants

https://codereview.chromium.org/775143003/diff/260001/ui/compositor/composito...
ui/compositor/compositor_unittest.cc:53: TestCompositorHost::Create(host_bounds,
context_factory));
On 2015/03/06 20:37:00, brianderson wrote:
> It looks like TestCompositorHost is used to test platform specific code.
> 
> Is it possible to create a ui::Compositor directly in a platform agnostic way?
> For example, by using a kNullAcceleratedWidget?
> 
> Alternatively, if creating a ui::Compositor directly is too hard/invasive,
maybe
> pull the more complicated Observer logic out of ui::Compositor and put it into
a
> delegate class that you can unit test more easily?
> 
> I see why you may have gone the CompositorTestAPI approach now...
> 
> danakj: Do you have any thoughts on which approach would be best?

I think brian is right here. If you don't need pixels on the screen to debug
things, you don't need the TestCompositorHost.

Here's another unit test example that has a compositor.
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/co...

And instead of a wrapper class that is a friend you could do 2 other things:
1. subclass ui::Compositor, make it protected, expose it in the subclass with
"using"
or
2. just expose it on ui::Compositor (if it's const and simple maybe that's fine)
or
3. expose it as FooForTesting() which will not allow calls to it outside of test
targets


lastly.. if you can split this patch up that would be hugely awesome, it's quite
big right now.

[brianderson, 2015-03-10 01:36:37]

Hi Simon. I think Dana's comment to split the patch up makes sense. It'll be
easier to review and catch any mistakes.

Do you think landing as 4 separate patches in the following order makes sense?

1) Allow Aura's Browser to handle SetNeedsBeginFrames and BeginFrame messages,
but don't enable it yet.
2a) Set use_external_begin_frame to true for Aura, enabling Browser code from 1.
2b) Disable OnUpdateVSyncParameters message for Aura, since the BeginFrame
message makes it redundant.
3a) Rewire the authoritative vsync signal.
3b) Delete CompositorVSyncManager, which was needed before 2b and 3a.
4) Delete forward_begin_frames_to_children. (This doesn't have any dependencies
and can land in any order.)

Outside of that and the compositor unit test changes, this patch looks good.

[mithro-old, 2015-06-01 03:32:31]

Has everything in this patch been split up and landed now? If so we should close
this CL.