Make SingleThreadProxy a SchedulerClient

Make SingleThreadProxy a SchedulerClient

All compositors that use SingleThreadProxy are left calling composite
synchronously and now pass a flag to indicate that this is their
intention.  This patch doesn't remove synchronous composite, but now
makes it mutually exclusive with scheduling.

Only cc unittests are exercising this code at this point, although it
also unifies a lot of the code for CompositeImmediately with the
scheduled path.

BUG=329552, 287250
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291451

[enne (OOO), 2014-02-14 22:32:15]

Depends on https://codereview.chromium.org/162473003/ and
https://codereview.chromium.org/154163005/.

[danakj, 2014-02-18 23:02:49]

On 2014/02/14 22:32:15, enne wrote:
> Depends on https://codereview.chromium.org/162473003/ and
> https://codereview.chromium.org/154163005/.

Could throw this at try bots now for some valid revision on the tree, that'd be
nice to see. *begins review*

[danakj, 2014-02-19 20:42:38]

So cooool

https://codereview.chromium.org/134623005/diff/320001/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/134623005/diff/320001/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:610: // If the LTH is not visible, defer the
composite until the LTH is made
Can this go away?

https://codereview.chromium.org/134623005/diff/320001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/134623005/diff/320001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_context.cc:1514: // Single thread proxy issues
extra commits after context lost.
How come these other cases aren't fixed by this CL too? Or can we remove all
these similar if(HasImplThread()) ?

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:193: FROM_HERE,
output_surface_creation_callback_.callback());
Looks like there's no reason to use a CancelableCallback here in single thread.
Just do the bind for the post task here and drop the callback member variable?

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:212: SetNeedsCommit();
Is it a TODO to support SetNeedsUpdateLayers ie cancellable commit here?

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:217: // DCHECK(Proxy::IsMainThread());
commented out? impl and main blocked?

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:292: next_frame_is_newly_committed_frame_ =
true;
I noticed that ThreadProxy sets scheduler visibility to the LTHI's visibility
here. Should we do that here too? Or is it pointless there?

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:437: // This must happen before did lose output
surface as the scheduler
"before did lose output surface" -> "before we notify the scheduler" ?

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:438: // may try to recreate the output surface
if already in begin impl frame
"if already in begin impl frame idle"

i know all those words, but not what they mean like that

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:443:
scheduler_on_impl_thread_->DidLoseOutputSurface();
usually calling the scheduler is the last thing in the function since it can
trigger the next state in the state machine. should this be below
DidAbortSwapBuffers?

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:444: client_->DidAbortSwapBuffers();
This is going to be.. weird..  I think. The scheduler is going to call this
method when the surface is lost, from the context callback, right? Then this
will call out to ui::Compositor observers. But there was not swap in play.

And this callback seems to only exist for unit tests for ui::Compositor, which
were testing the old code you're deleting. So can we get rid of this method
entirely?

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:448: client_->DidPostSwapBuffers();
Can we get rid of this too?

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:498: return layer_tree_host_impl_->visible() &&
layer_tree_host_impl_->CanDraw();
why is this based on visible and not on Scheduler::WillDrawIfNeeded() like in
the thread proxy?

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:697: void
SingleThreadProxy::ScheduledActionUpdateVisibleTiles() {}
NOTREACHED Impl-side painting only?

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

https://codereview.chromium.org/134623005/diff/320001/ui/compositor/composito...
ui/compositor/compositor.cc:192: bool use_thread =
A nice followup would be to remove these switches so we start using the STP on
ChromeOS too.

https://codereview.chromium.org/134623005/diff/320001/ui/compositor/composito...
ui/compositor/compositor.cc:388: last_ended_frame_++;
Can we move this stuff up to the one callside of this method and delete this
method?

[enne (OOO), 2014-02-20 00:35:44]

Looks like I've got some failing tests too.  I'll take a look.  Here's an
updated version based on this review.

https://codereview.chromium.org/134623005/diff/320001/cc/test/layer_tree_test.cc
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/134623005/diff/320001/cc/test/layer_tree_test...
cc/test/layer_tree_test.cc:610: // If the LTH is not visible, defer the
composite until the LTH is made
On 2014/02/19 20:42:39, danakj wrote:
> Can this go away?

Excellent call.  Done.

https://codereview.chromium.org/134623005/diff/320001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/134623005/diff/320001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_context.cc:1514: // Single thread proxy issues
extra commits after context lost.
On 2014/02/19 20:42:39, danakj wrote:
> How come these other cases aren't fixed by this CL too? Or can we remove all
> these similar if(HasImplThread()) ?

Good catch.  Done.

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:193: FROM_HERE,
output_surface_creation_callback_.callback());
On 2014/02/19 20:42:39, danakj wrote:
> Looks like there's no reason to use a CancelableCallback here in single
thread.
> Just do the bind for the post task here and drop the callback member variable?

This is because the STP could be destroyed before the PostTask occurs.  I could
add a weak pointer member variable here instead?

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:212: SetNeedsCommit();
On 2014/02/19 20:42:39, danakj wrote:
> Is it a TODO to support SetNeedsUpdateLayers ie cancellable commit here?

The cancellable commit is really about handling offscreen invalidations and
scrolling from the impl thread.  In general, I'm ok with saying that the STP
doesn't do this optimization because places where we use the STP don't need it. 
So, not even sure if it merits a TODO.

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:217: // DCHECK(Proxy::IsMainThread());
On 2014/02/19 20:42:39, danakj wrote:
> commented out? impl and main blocked?

Uncommented.

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:292: next_frame_is_newly_committed_frame_ =
true;
On 2014/02/19 20:42:39, danakj wrote:
> I noticed that ThreadProxy sets scheduler visibility to the LTHI's visibility
> here. Should we do that here too? Or is it pointless there?

SingleThreadProxy handles it immediately, rather than on commit.

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:437: // This must happen before did lose output
surface as the scheduler
On 2014/02/19 20:42:39, danakj wrote:
> "before did lose output surface" -> "before we notify the scheduler" ?

Done.

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:438: // may try to recreate the output surface
if already in begin impl frame
On 2014/02/19 20:42:39, danakj wrote:
> "if already in begin impl frame idle"
> 
> i know all those words, but not what they mean like that

"begin impl frame idle" => BEGIN_IMPL_FRAME_STATE_IDLE

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:443:
scheduler_on_impl_thread_->DidLoseOutputSurface();
On 2014/02/19 20:42:39, danakj wrote:
> usually calling the scheduler is the last thing in the function since it can
> trigger the next state in the state machine. should this be below
> DidAbortSwapBuffers?

Seems safer.  Done.

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:444: client_->DidAbortSwapBuffers();
On 2014/02/19 20:42:39, danakj wrote:
> This is going to be.. weird..  I think. The scheduler is going to call this
> method when the surface is lost, from the context callback, right? Then this
> will call out to ui::Compositor observers. But there was not swap in play.
> 
> And this callback seems to only exist for unit tests for ui::Compositor, which
> were testing the old code you're deleting. So can we get rid of this method
> entirely?

I see this getting used in:
* render_widget_compositor.cc (for scheduling another composite, which maybe is
redundant with the scheduler)
* compositor.cc (for observers, here's real code at the end of this call:
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/window_anim...)
* compositor_impl_android.cc (which eventually gets to
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/an...)

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:448: client_->DidPostSwapBuffers();
On 2014/02/19 20:42:39, danakj wrote:
> Can we get rid of this too?

For the compositor_impl_android.cc path, the abort/swap cases route together
into the above code.  So, tentatively no.

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:498: return layer_tree_host_impl_->visible() &&
layer_tree_host_impl_->CanDraw();
On 2014/02/19 20:42:39, danakj wrote:
> why is this based on visible and not on Scheduler::WillDrawIfNeeded() like in
> the thread proxy?

Done.

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:697: void
SingleThreadProxy::ScheduledActionUpdateVisibleTiles() {}
On 2014/02/19 20:42:39, danakj wrote:
> NOTREACHED Impl-side painting only?

Done.  Did this for a few other functions too.

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

https://codereview.chromium.org/134623005/diff/320001/ui/compositor/composito...
ui/compositor/compositor.cc:192: bool use_thread =
On 2014/02/19 20:42:39, danakj wrote:
> A nice followup would be to remove these switches so we start using the STP on
> ChromeOS too.

Hmm! Who is using the single-threaded browser compositor, then? All Aura
platforms minus ChromeOS?

https://codereview.chromium.org/134623005/diff/320001/ui/compositor/composito...
ui/compositor/compositor.cc:388: last_ended_frame_++;
On 2014/02/19 20:42:39, danakj wrote:
> Can we move this stuff up to the one callside of this method and delete this
> method?

Done.

[danakj, 2014-02-20 19:30:55]

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:193: FROM_HERE,
output_surface_creation_callback_.callback());
On 2014/02/20 00:35:44, enne wrote:
> On 2014/02/19 20:42:39, danakj wrote:
> > Looks like there's no reason to use a CancelableCallback here in single
> thread.
> > Just do the bind for the post task here and drop the callback member
variable?
> 
> This is because the STP could be destroyed before the PostTask occurs.  I
could
> add a weak pointer member variable here instead?

I see, ya I prefer a weak ptr factory. That'll make this more obvious what's
going on, and we can reuse it for other post tasks if we need to in the future.

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:212: SetNeedsCommit();
On 2014/02/20 00:35:44, enne wrote:
> On 2014/02/19 20:42:39, danakj wrote:
> > Is it a TODO to support SetNeedsUpdateLayers ie cancellable commit here?
> 
> The cancellable commit is really about handling offscreen invalidations and
> scrolling from the impl thread.  In general, I'm ok with saying that the STP
> doesn't do this optimization because places where we use the STP don't need
it. 
> So, not even sure if it merits a TODO.

Or possibly occluded invalidations? That will be super common.

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:292: next_frame_is_newly_committed_frame_ =
true;
On 2014/02/20 00:35:44, enne wrote:
> On 2014/02/19 20:42:39, danakj wrote:
> > I noticed that ThreadProxy sets scheduler visibility to the LTHI's
visibility
> > here. Should we do that here too? Or is it pointless there?
> 
> SingleThreadProxy handles it immediately, rather than on commit.

ThreadProxy does also, it passes it over to SetVisibleOnImplThread(). There it
sets Visible on LTHI and Scheduler. Everywhere just sets it on scheduler, so
maybe the thread proxy one is pointless?

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:444: client_->DidAbortSwapBuffers();
On 2014/02/20 00:35:44, enne wrote:
> On 2014/02/19 20:42:39, danakj wrote:
> > This is going to be.. weird..  I think. The scheduler is going to call this
> > method when the surface is lost, from the context callback, right? Then this
> > will call out to ui::Compositor observers. But there was not swap in play.
> > 
> > And this callback seems to only exist for unit tests for ui::Compositor,
which
> > were testing the old code you're deleting. So can we get rid of this method
> > entirely?
> 
> I see this getting used in:
> * render_widget_compositor.cc (for scheduling another composite, which maybe
is
> redundant with the scheduler)
> * compositor.cc (for observers, here's real code at the end of this call:
>
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/window_anim...)
> * compositor_impl_android.cc (which eventually gets to
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/an...)

Uh, hm. Really not clear why losing the graphics context would cause animations
to stop... so weird. Can you TODO to remove this? This feels wrong like it
should not exist.

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:448: client_->DidPostSwapBuffers();
On 2014/02/20 00:35:44, enne wrote:
> On 2014/02/19 20:42:39, danakj wrote:
> > Can we get rid of this too?
> 
> For the compositor_impl_android.cc path, the abort/swap cases route together
> into the above code.  So, tentatively no.

Hm, we already have DidCommitAndDrawFrame() which they could hook off. This
seems redundant and I think it exists because we created completely different
hooks for single vs threaded mode way back. Can you put a TODO to merge this and
remove it at least?

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

https://codereview.chromium.org/134623005/diff/320001/ui/compositor/composito...
ui/compositor/compositor.cc:192: bool use_thread =
On 2014/02/20 00:35:44, enne wrote:
> On 2014/02/19 20:42:39, danakj wrote:
> > A nice followup would be to remove these switches so we start using the STP
on
> > ChromeOS too.
> 
> Hmm! Who is using the single-threaded browser compositor, then? All Aura
> platforms minus ChromeOS?

Everyone except ChromeOS ya.

[enne (OOO), 2014-02-20 20:01:39]

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:193: FROM_HERE,
output_surface_creation_callback_.callback());
On 2014/02/20 19:30:55, danakj wrote:
> On 2014/02/20 00:35:44, enne wrote:
> > On 2014/02/19 20:42:39, danakj wrote:
> > > Looks like there's no reason to use a CancelableCallback here in single
> > thread.
> > > Just do the bind for the post task here and drop the callback member
> variable?
> > 
> > This is because the STP could be destroyed before the PostTask occurs.  I
> could
> > add a weak pointer member variable here instead?
> 
> I see, ya I prefer a weak ptr factory. That'll make this more obvious what's
> going on, and we can reuse it for other post tasks if we need to in the
future.

Done.

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:212: SetNeedsCommit();
On 2014/02/20 19:30:55, danakj wrote:
> On 2014/02/20 00:35:44, enne wrote:
> > On 2014/02/19 20:42:39, danakj wrote:
> > > Is it a TODO to support SetNeedsUpdateLayers ie cancellable commit here?
> > 
> > The cancellable commit is really about handling offscreen invalidations and
> > scrolling from the impl thread.  In general, I'm ok with saying that the STP
> > doesn't do this optimization because places where we use the STP don't need
> it. 
> > So, not even sure if it merits a TODO.
> 
> Or possibly occluded invalidations? That will be super common.

These don't happen with impl-side painting, so...

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:292: next_frame_is_newly_committed_frame_ =
true;
On 2014/02/20 19:30:55, danakj wrote:
> On 2014/02/20 00:35:44, enne wrote:
> > On 2014/02/19 20:42:39, danakj wrote:
> > > I noticed that ThreadProxy sets scheduler visibility to the LTHI's
> visibility
> > > here. Should we do that here too? Or is it pointless there?
> > 
> > SingleThreadProxy handles it immediately, rather than on commit.
> 
> ThreadProxy does also, it passes it over to SetVisibleOnImplThread(). There it
> sets Visible on LTHI and Scheduler. Everywhere just sets it on scheduler, so
> maybe the thread proxy one is pointless?

That's possible.  Maybe the scheduler needs to be kicked but the SetVisible is a
no-op.  *makes a note to look into it*

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:444: client_->DidAbortSwapBuffers();
On 2014/02/20 19:30:55, danakj wrote:
> On 2014/02/20 00:35:44, enne wrote:
> > On 2014/02/19 20:42:39, danakj wrote:
> > > This is going to be.. weird..  I think. The scheduler is going to call
this
> > > method when the surface is lost, from the context callback, right? Then
this
> > > will call out to ui::Compositor observers. But there was not swap in play.
> > > 
> > > And this callback seems to only exist for unit tests for ui::Compositor,
> which
> > > were testing the old code you're deleting. So can we get rid of this
method
> > > entirely?
> > 
> > I see this getting used in:
> > * render_widget_compositor.cc (for scheduling another composite, which maybe
> is
> > redundant with the scheduler)
> > * compositor.cc (for observers, here's real code at the end of this call:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/ash/wm/window_anim...)
> > * compositor_impl_android.cc (which eventually gets to
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/an...)
> 
> Uh, hm. Really not clear why losing the graphics context would cause
animations
> to stop... so weird. Can you TODO to remove this? This feels wrong like it
> should not exist.

Done.

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:448: client_->DidPostSwapBuffers();
On 2014/02/20 19:30:55, danakj wrote:
> On 2014/02/20 00:35:44, enne wrote:
> > On 2014/02/19 20:42:39, danakj wrote:
> > > Can we get rid of this too?
> > 
> > For the compositor_impl_android.cc path, the abort/swap cases route together
> > into the above code.  So, tentatively no.
> 
> Hm, we already have DidCommitAndDrawFrame() which they could hook off. This
> seems redundant and I think it exists because we created completely different
> hooks for single vs threaded mode way back. Can you put a TODO to merge this
and
> remove it at least?

Done.

[danakj, 2014-02-20 20:06:33]

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:212: SetNeedsCommit();
 2014/02/20 20:01:39, enne wrote:
> On 2014/02/20 19:30:55, danakj wrote:
> > On 2014/02/20 00:35:44, enne wrote:
> > > On 2014/02/19 20:42:39, danakj wrote:
> > > > Is it a TODO to support SetNeedsUpdateLayers ie cancellable commit here?
> > > 
> > > The cancellable commit is really about handling offscreen invalidations
and
> > > scrolling from the impl thread.  In general, I'm ok with saying that the
STP
> > > doesn't do this optimization because places where we use the STP don't
need
> > it. 
> > > So, not even sure if it merits a TODO.
> > 
> > Or possibly occluded invalidations? That will be super common.
> 
> These don't happen with impl-side painting, so...

But this patch has nothing to do with impl-side painting, we'd be using STP
without impl-side painting here. So, we'll be invalidating tab load spinners
that are not visible, not uploading any textures cuz they are occluded, then
committing and drawing for no reason?  And we need to add occlusion something to
the browser compositor anyway. Idk, I thought this seemed more clear, am I way
off?

[danakj, 2014-02-20 20:08:13]

On 2014/02/20 20:06:33, danakj wrote:
>
https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
> File cc/trees/single_thread_proxy.cc (right):
> 
>
https://codereview.chromium.org/134623005/diff/320001/cc/trees/single_thread_...
> cc/trees/single_thread_proxy.cc:212: SetNeedsCommit();
>  2014/02/20 20:01:39, enne wrote:
> > On 2014/02/20 19:30:55, danakj wrote:
> > > On 2014/02/20 00:35:44, enne wrote:
> > > > On 2014/02/19 20:42:39, danakj wrote:
> > > > > Is it a TODO to support SetNeedsUpdateLayers ie cancellable commit
here?
> > > > 
> > > > The cancellable commit is really about handling offscreen invalidations
> and
> > > > scrolling from the impl thread.  In general, I'm ok with saying that the
> STP
> > > > doesn't do this optimization because places where we use the STP don't
> need
> > > it. 
> > > > So, not even sure if it merits a TODO.
> > > 
> > > Or possibly occluded invalidations? That will be super common.
> > 
> > These don't happen with impl-side painting, so...
> 
> But this patch has nothing to do with impl-side painting, we'd be using STP
> without impl-side painting here. So, we'll be invalidating tab load spinners
> that are not visible, not uploading any textures cuz they are occluded, then
> committing and drawing for no reason?  And we need to add occlusion something
to
> the browser compositor anyway. Idk, I thought this seemed more clear, am I way
> off?

I guess the argument is that with impl-side we'd record anyway, so we have to
commit the recording?

[enne (OOO), 2014-02-20 20:12:35]

You're suggesting an optimization that wasn't there before, and I'm suggesting
that we punt because it won't be relevant in the future.  With impl-side
painting, we don't have occlusion at record time and don't not record things
because of occlusion.

[danakj, 2014-02-20 20:14:51]

On Thu, Feb 20, 2014 at 3:12 PM, <enne@chromium.org> wrote:

> You're suggesting an optimization that wasn't there before, and I'm
> suggesting
> that we punt because it won't be relevant in the future.  With impl-side
> painting, we don't have occlusion at record time and don't not record
> things
> because of occlusion.
>

OK ya I see what you're saying, sg.

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

[danakj, 2014-02-20 20:56:39]

https://codereview.chromium.org/134623005/diff/560001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/560001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:190: weak_ptr_ = weak_factory_.GetWeakPtr();
I don't think you need a member variable for this. Just call GetWeakPtr()
directly in Bind().

https://codereview.chromium.org/134623005/diff/560001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:671: OffscreenContextProvider();
Hm ok one more question.

Is it going to be confusing that this is happening during Draw instead of during
commit here? It's definitely moving the ordering for this around in the
single-thread case too.

In the thread case, we do this during commit, since it comes from the main
thread. (Though maybe we don't need it at all in thread proxy anymore since
renderers don't have direct renderers, so idk). WDYT?

https://codereview.chromium.org/134623005/diff/560001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.h (right):

https://codereview.chromium.org/134623005/diff/560001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.h:149: base::WeakPtrFactory<SingleThreadProxy>
weak_factory_;
This should be the last member of the class.

[enne (OOO), 2014-02-20 21:07:12]

https://codereview.chromium.org/134623005/diff/560001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/560001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:190: weak_ptr_ = weak_factory_.GetWeakPtr();
On 2014/02/20 20:56:40, danakj wrote:
> I don't think you need a member variable for this. Just call GetWeakPtr()
> directly in Bind().

It seemed silly to create a new weak pointer every time this function was
called? Also, didn't you just suggest to use a factory in case there were
multiple users of this weak pointer?

https://codereview.chromium.org/134623005/diff/560001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:671: OffscreenContextProvider();
On 2014/02/20 20:56:40, danakj wrote:
> Hm ok one more question.
> 
> Is it going to be confusing that this is happening during Draw instead of
during
> commit here? It's definitely moving the ordering for this around in the
> single-thread case too.
> 
> In the thread case, we do this during commit, since it comes from the main
> thread. (Though maybe we don't need it at all in thread proxy anymore since
> renderers don't have direct renderers, so idk). WDYT?

Is there an ordering issue that you're concerned about here, or is this just an
inconsistency issue?

Looking at the original code, I don't see any reason this needed to be called
prior to DoCommit.

https://codereview.chromium.org/134623005/diff/560001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.h (right):

https://codereview.chromium.org/134623005/diff/560001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.h:149: base::WeakPtrFactory<SingleThreadProxy>
weak_factory_;
On 2014/02/20 20:56:40, danakj wrote:
> This should be the last member of the class.

Done.

[danakj, 2014-02-20 21:09:46]

https://codereview.chromium.org/134623005/diff/560001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/560001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:190: weak_ptr_ = weak_factory_.GetWeakPtr();
On 2014/02/20 21:07:13, enne wrote:
> On 2014/02/20 20:56:40, danakj wrote:
> > I don't think you need a member variable for this. Just call GetWeakPtr()
> > directly in Bind().
> 
> It seemed silly to create a new weak pointer every time this function was
> called? Also, didn't you just suggest to use a factory in case there were
> multiple users of this weak pointer?

This is called rarely, and I think it's not a typical pattern to store the
weakptr unless you're going to use it a lot on critical paths? ThreadProxy is
the only place I can think of that I've seen that. I suggested the factory
because that's the right way to get weakptrs. The other way is to subclass
SupportsWeakPtr but that's deprecated.

https://codereview.chromium.org/134623005/diff/560001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:671: OffscreenContextProvider();
On 2014/02/20 21:07:13, enne wrote:
> On 2014/02/20 20:56:40, danakj wrote:
> > Hm ok one more question.
> > 
> > Is it going to be confusing that this is happening during Draw instead of
> during
> > commit here? It's definitely moving the ordering for this around in the
> > single-thread case too.
> > 
> > In the thread case, we do this during commit, since it comes from the main
> > thread. (Though maybe we don't need it at all in thread proxy anymore since
> > renderers don't have direct renderers, so idk). WDYT?
> 
> Is there an ordering issue that you're concerned about here, or is this just
an
> inconsistency issue?
> 
> Looking at the original code, I don't see any reason this needed to be called
> prior to DoCommit.

Just consistency. It comes to mind because when writing unit tests I've had to
do if (threaded) {} else {} many times because we called things at different
times. I've tried to eliminate those differences over time.

[enne (OOO), 2014-02-21 19:34:09]

This should be ready to go at this point.

https://codereview.chromium.org/134623005/diff/560001/cc/trees/single_thread_...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/560001/cc/trees/single_thread_...
cc/trees/single_thread_proxy.cc:190: weak_ptr_ = weak_factory_.GetWeakPtr();
On 2014/02/20 21:09:47, danakj wrote:
> On 2014/02/20 21:07:13, enne wrote:
> > On 2014/02/20 20:56:40, danakj wrote:
> > > I don't think you need a member variable for this. Just call GetWeakPtr()
> > > directly in Bind().
> > 
> > It seemed silly to create a new weak pointer every time this function was
> > called? Also, didn't you just suggest to use a factory in case there were
> > multiple users of this weak pointer?
> 
> This is called rarely, and I think it's not a typical pattern to store the
> weakptr unless you're going to use it a lot on critical paths? ThreadProxy is
> the only place I can think of that I've seen that. I suggested the factory
> because that's the right way to get weakptrs. The other way is to subclass
> SupportsWeakPtr but that's deprecated.

Removed weak pointer.

[danakj, 2014-02-21 19:36:20]

LGTM

=> jamesr for content/renderer/

https://codereview.chromium.org/134623005/diff/750001/content/browser/composi...
File
content/browser/compositor/software_browser_compositor_output_surface_unittest.cc
(right):

https://codereview.chromium.org/134623005/diff/750001/content/browser/composi...
content/browser/compositor/software_browser_compositor_output_surface_unittest.cc:78:
scoped_ptr<ui::TestCompositorHost> window_;
s/window_/compositor_host_/

[danakj, 2014-02-21 19:36:34]

jamesr@chromium.org: Please review changes in content/renderer

[enne (OOO), 2014-02-21 19:41:31]

https://codereview.chromium.org/134623005/diff/750001/content/browser/composi...
File
content/browser/compositor/software_browser_compositor_output_surface_unittest.cc
(right):

https://codereview.chromium.org/134623005/diff/750001/content/browser/composi...
content/browser/compositor/software_browser_compositor_output_surface_unittest.cc:78:
scoped_ptr<ui::TestCompositorHost> window_;
On 2014/02/21 19:36:21, danakj wrote:
> s/window_/compositor_host_/

Done.

PS
https://code.google.com/p/chromium/codesearch#chromium/src/ui/compositor/laye...

[danakj, 2014-02-21 19:49:27]

On Fri, Feb 21, 2014 at 2:41 PM, <enne@chromium.org> wrote:

>
> https://codereview.chromium.org/134623005/diff/750001/
> content/browser/compositor/software_browser_compositor_
> output_surface_unittest.cc
> File
> content/browser/compositor/software_browser_compositor_
> output_surface_unittest.cc
> (right):
>
> https://codereview.chromium.org/134623005/diff/750001/
> content/browser/compositor/software_browser_compositor_
> output_surface_unittest.cc#newcode78
> content/browser/compositor/software_browser_compositor_
> output_surface_unittest.cc:78:
> scoped_ptr<ui::TestCompositorHost> window_;
> On 2014/02/21 19:36:21, danakj wrote:
>
>> s/window_/compositor_host_/
>>
>
> Done.
>
> PS
> https://code.google.com/p/chromium/codesearch#chromium/
> src/ui/compositor/layer_unittest.cc&l=206


Oh snap. I thought piman told me to make that say compositor_host and I
changed it. Hm.

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

[jamesr, 2014-02-21 22:39:07]

I think for RenderWidget we'd be better off moving all of the scheduling
responsibilities over to the STP's scheduler instead of letting both mechanisms
(compositor and RenderWidget::InvalidationCallback) run concurrently.  To
accomplish this, you'd route scheduleComposite() calls to the compositor like
the threaded path does and make sure that we do all the RenderWidget work like
UpdateTextInputState() etc in response to compositor callbacks.  Then
::DoDeferredUpdate() would become legacy software path only.

One thing that's tricky is that single threaded on-demand compositing requires
being able to enter / exit compositing mode in the middle of the pipeline. 
RenderWidget::DoDeferredUpdate() can handle this by re-checking state in the
middle a few times, but the compositor has never had to deal with this.  Maybe
it'd work, but it's a bit complicated.

[enne (OOO), 2014-02-24 22:46:15]

Oh, RenderWidget.  Oh, Linux GTK.  :(

I'm going to put this patch back in the freezer until Linux Aura is a thing and
Linux GTK is no more.  I'm sure there's some intermediate solution where
RenderWidget could be FCM all the time or none of the time, but I think the
amount of work required to keep this patch rebaselined is much much less than
the amount of work required to fix RenderWidget regressions if Linux Aura
doesn't make m35.

[danakj, 2014-04-14 18:52:11]

From sievers on another CL:

> Also see LayerTreeHostClientNotReadyDoesNotCreateOutputSurface, which is
> however only run as MULTI_THREAD_TEST_F(). If possible, we should run it
> singlethreaded w/scheduler.

[enne (OOO), 2014-06-12 20:45:00]

PTAL

https://codereview.chromium.org/134623005/diff/1180001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/134623005/diff/1180001/cc/scheduler/scheduler...
cc/scheduler/scheduler.cc:275: if (!impl_task_runner_)
This seemed like the only place that the scheduler actually needed to avoid
posting a task for the null message loop case.  So, I'll take one conditional
over a ManuallyCompositedProxy.

https://codereview.chromium.org/134623005/diff/1200001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest_context.cc (right):

https://codereview.chromium.org/134623005/diff/1200001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest_context.cc:319:
SINGLE_AND_MULTI_THREAD_TEST_F(
On 2014/04/14 at 18:52:11, danakj wrote:
> From sievers on another CL:
> 
> > Also see LayerTreeHostClientNotReadyDoesNotCreateOutputSurface, which is
> > however only run as MULTI_THREAD_TEST_F(). If possible, we should run it
> > singlethreaded w/scheduler.

Done.

[brianderson, 2014-06-12 23:00:36]

https://codereview.chromium.org/134623005/diff/1180001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/134623005/diff/1180001/cc/scheduler/scheduler...
cc/scheduler/scheduler.cc:275: if (!impl_task_runner_)
On 2014/06/12 20:44:59, enne wrote:
> This seemed like the only place that the scheduler actually needed to avoid
> posting a task for the null message loop case.  So, I'll take one conditional
> over a ManuallyCompositedProxy.

What code path doesn't set the impl_task_runner_ in this patch?

Also impl_task_runner_ should probably be renamed to something else now.

[enne (OOO), 2014-06-12 23:09:11]

https://codereview.chromium.org/134623005/diff/1180001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/134623005/diff/1180001/cc/scheduler/scheduler...
cc/scheduler/scheduler.cc:275: if (!impl_task_runner_)
On 2014/06/12 23:00:36, brianderson wrote:
> On 2014/06/12 20:44:59, enne wrote:
> > This seemed like the only place that the scheduler actually needed to avoid
> > posting a task for the null message loop case.  So, I'll take one
conditional
> > over a ManuallyCompositedProxy.
> 
> What code path doesn't set the impl_task_runner_ in this patch?

https://code.google.com/p/chromium/codesearch#chromium/src/cc/trees/layer_tre...

> Also impl_task_runner_ should probably be renamed to something else now.

Changed to task_runner_.

[simonhong, 2014-06-13 11:55:12]

Hi enne,
I did some review and hopes my comment is helpful.

https://codereview.chromium.org/134623005/diff/1220001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/134623005/diff/1220001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:716: SetLayerTreeHostClientReady();
Is there a case that Composite() is called before SetLayerTreeHostClientReady()
is called?

https://codereview.chromium.org/134623005/diff/1220001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/1220001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:413: TRACE_EVENT0("cc",
"ThreadProxy::BeginFrame");
nit: "SingleThreadProxy::BeginFrame"

https://codereview.chromium.org/134623005/diff/1220001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:543: surface->SetNeedsBeginFrame(enable);
How about layer_tree_host_impl_->SetNeedsBeginFrame(enable)?

https://codereview.chromium.org/134623005/diff/1220001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:575: DrawResult
SingleThreadProxy::ScheduledActionDrawAndSwapForced() {
Scheduler should never call this because it only wants forced draw when there
are consecutive checkerboard animation.
Or, does SingleThreadProxy also need to handle the return value of
PrepareToDraw() at line 489?

[enne (OOO), 2014-06-13 18:06:37]

https://codereview.chromium.org/134623005/diff/1220001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host.cc (right):

https://codereview.chromium.org/134623005/diff/1220001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host.cc:716: SetLayerTreeHostClientReady();
On 2014/06/13 11:55:12, simonhong wrote:
> Is there a case that Composite() is called before
SetLayerTreeHostClientReady()
> is called?

Yes, this happens quite a bit in tests.  An alternative to this is to move this
call out to a dozen tests.

https://codereview.chromium.org/134623005/diff/1220001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/1220001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:413: TRACE_EVENT0("cc",
"ThreadProxy::BeginFrame");
On 2014/06/13 11:55:12, simonhong wrote:
> nit: "SingleThreadProxy::BeginFrame"

Done.

https://codereview.chromium.org/134623005/diff/1220001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:543: surface->SetNeedsBeginFrame(enable);
On 2014/06/13 11:55:12, simonhong wrote:
> How about layer_tree_host_impl_->SetNeedsBeginFrame(enable)?

Done.

https://codereview.chromium.org/134623005/diff/1220001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:575: DrawResult
SingleThreadProxy::ScheduledActionDrawAndSwapForced() {
On 2014/06/13 11:55:12, simonhong wrote:
> Scheduler should never call this because it only wants forced draw when there
> are consecutive checkerboard animation.
> Or, does SingleThreadProxy also need to handle the return value of
> PrepareToDraw() at line 489?

It should never call this, but there's no reason not to handle it. 
SingleThreadProxy won't skip draws for animations so every draw is a forced
draw.

[danakj, 2014-06-13 18:09:33]

https://codereview.chromium.org/134623005/diff/1220001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/1220001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:575: DrawResult
SingleThreadProxy::ScheduledActionDrawAndSwapForced() {
On 2014/06/13 18:06:37, enne wrote:
> On 2014/06/13 11:55:12, simonhong wrote:
> > Scheduler should never call this because it only wants forced draw when
there
> > are consecutive checkerboard animation.
> > Or, does SingleThreadProxy also need to handle the return value of
> > PrepareToDraw() at line 489?
> 
> It should never call this, but there's no reason not to handle it. 
> SingleThreadProxy won't skip draws for animations so every draw is a forced
> draw.

You could NOTREACHED() in here though, we'll never come here in practice. I
think that'd be preferable.

[enne (OOO), 2014-06-13 18:32:22]

https://codereview.chromium.org/134623005/diff/1220001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/1220001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:575: DrawResult
SingleThreadProxy::ScheduledActionDrawAndSwapForced() {
On 2014/06/13 18:09:32, danakj wrote:
> On 2014/06/13 18:06:37, enne wrote:
> > On 2014/06/13 11:55:12, simonhong wrote:
> > > Scheduler should never call this because it only wants forced draw when
> there
> > > are consecutive checkerboard animation.
> > > Or, does SingleThreadProxy also need to handle the return value of
> > > PrepareToDraw() at line 489?
> > 
> > It should never call this, but there's no reason not to handle it. 
> > SingleThreadProxy won't skip draws for animations so every draw is a forced
> > draw.
> 
> You could NOTREACHED() in here though, we'll never come here in practice. I
> think that'd be preferable. 

Sure, done.

[danakj, 2014-06-13 18:34:10]

https://codereview.chromium.org/134623005/diff/1240001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/134623005/diff/1240001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:314: virtual void ScheduleComposite() OVERRIDE {
Are there any callers of this left? Can this (and ScheduleAnimation) go away?

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:1629: #ifndef NDEBUG
why not #if DCHECK_IS_ON?

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:92:
scheduler_on_impl_thread_->SetVisible(layer_tree_host_impl_->visible());
can this be the last thing in the method? scheduler kicks off new tasks so it's
kinda weird to do things after calling it (and has been a source of bugs)

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:123: Proxy::MainThreadTaskRunner()->PostTask(
Do you think posting vs just calling it inline here is better? Just want us to
think about it rather than copying it blindly.

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:229:
scheduler_on_impl_thread_->SetNeedsCommit();
DCHECK(IsMainThread) + DebugScopedImplThreadThing?

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:555: return;
Hm! ThreadProxy saves the beginframe and runs it later when undeferred. Do you
think we can make it stop doing that since this doesn't?

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:566: DidCommitAndDrawFrame();
we did?

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:567: return DrawResult::DRAW_SUCCESS;
why not DRAW_ABORTED_CONTEXT_LOST?

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:601: CreateAndInitializeOutputSurface();
If you make this a post-task does it save a lot of the hoop-dancing the patch
does to get around different call stacks?

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.h (right):

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/single_thread...
cc/trees/single_thread_proxy.h:128: scoped_refptr<ContextProvider>
OffscreenContextProvider();
this doesn't exist anymore right?

https://codereview.chromium.org/134623005/diff/1240001/ui/compositor/test/tes...
File ui/compositor/test/test_compositor_host_x11.cc (right):

https://codereview.chromium.org/134623005/diff/1240001/ui/compositor/test/tes...
ui/compositor/test/test_compositor_host_x11.cc:17: #include
"ui/compositor/test/draw_waiter_for_test.h"
needed?

[enne (OOO), 2014-06-13 20:10:42]

https://codereview.chromium.org/134623005/diff/1240001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/134623005/diff/1240001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:314: virtual void ScheduleComposite() OVERRIDE {
On 2014/06/13 18:34:10, danakj wrote:
> Are there any callers of this left? Can this (and ScheduleAnimation) go away?

There was only one caller in a test, so this resulted in a lot of code going
away.  A lot of this code still looks it does something, particularly in RWC and
Android's CompositorImpl.  I made RWC SetSuppressScheduleComposite and
CompositorImpl both change to use deferred commits, since WebViewImpl is still
using that code, but it doesn't look like it actually does anything anymore. 
So, the intended suppression of commits now actually should do so.

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_impl.cc (right):

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_impl.cc:1629: #ifndef NDEBUG
On 2014/06/13 18:34:10, danakj wrote:
> why not #if DCHECK_IS_ON?

Sorry, poor merge across three months.

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:92:
scheduler_on_impl_thread_->SetVisible(layer_tree_host_impl_->visible());
On 2014/06/13 18:34:10, danakj wrote:
> can this be the last thing in the method? scheduler kicks off new tasks so
it's
> kinda weird to do things after calling it (and has been a source of bugs)

Done.

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:123: Proxy::MainThreadTaskRunner()->PostTask(
On 2014/06/13 18:34:10, danakj wrote:
> Do you think posting vs just calling it inline here is better? Just want us to
> think about it rather than copying it blindly.

If it's not posted, then I need to do some extra scheduler logic to advance
frames, etc.  Otherwise it won't create the surface because it's already tried
to this frame.

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:229:
scheduler_on_impl_thread_->SetNeedsCommit();
On 2014/06/13 18:34:10, danakj wrote:
> DCHECK(IsMainThread) + DebugScopedImplThreadThing?

Sure sure!

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:555: return;
On 2014/06/13 18:34:10, danakj wrote:
> Hm! ThreadProxy saves the beginframe and runs it later when undeferred. Do you
> think we can make it stop doing that since this doesn't?

ThreadProxy's BeginMainFrameAndCommitState info comes from the compositor
thread.  So, it receives it and holds onto it until it decides to undefer that
commit.  SingleThreadProxy is all one one thread so it doesn't need to pass
along scroll deltas (since no compositor thread scrolling) or the current frame
time or the memory limits (which can just be applied immediately).

So, I think this makes this cleaner, but I don't see how to make ThreadProxy any
different.  They seem to have different needs.

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:566: DidCommitAndDrawFrame();
On 2014/06/13 18:34:10, danakj wrote:
> we did?

So "context lost" is not a valid return from scheduled action draw.  ThreadProxy
also says DRAW_SUCCESS and DidCommitAndDrawFrame if the context is lost after
draw but before swap.  (It also doesn't check beforehand.)

Option 1 (this patch): a lost context is still a valid draw attempt and we'll
redraw after the context is restored so no use handling it specially.

Option 2: update scheduler to not explode when context lost is here but also
don't do anything different.  Update ThreadProxy to issue aborted draw return
values as well.  Update tests to not expect DidCommitAndDrawFrame (maybe they
need to wait for the single thread aborted swap callback?), although this adds
some weirded single thread vs threaded divergence.

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:567: return DrawResult::DRAW_SUCCESS;
On 2014/06/13 18:34:10, danakj wrote:
> why not DRAW_ABORTED_CONTEXT_LOST?

See comment above.

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:601: CreateAndInitializeOutputSurface();
On 2014/06/13 18:34:10, danakj wrote:
> If you make this a post-task does it save a lot of the hoop-dancing the patch
> does to get around different call stacks?

So a post task here to avoid a post task in CreateAndInitializeOutputSurface? 
What does that clean up?

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.h (right):

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/single_thread...
cc/trees/single_thread_proxy.h:128: scoped_refptr<ContextProvider>
OffscreenContextProvider();
On 2014/06/13 18:34:10, danakj wrote:
> this doesn't exist anymore right?

OOPS

https://codereview.chromium.org/134623005/diff/1240001/ui/compositor/test/tes...
File ui/compositor/test/test_compositor_host_x11.cc (right):

https://codereview.chromium.org/134623005/diff/1240001/ui/compositor/test/tes...
ui/compositor/test/test_compositor_host_x11.cc:17: #include
"ui/compositor/test/draw_waiter_for_test.h"
On 2014/06/13 18:34:10, danakj wrote:
> needed?

OOPSOOPS.  Sorry for the sloppy patch.  :(

[danakj, 2014-06-13 20:16:39]

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:601: CreateAndInitializeOutputSurface();
On 2014/06/13 20:10:42, enne wrote:
> On 2014/06/13 18:34:10, danakj wrote:
> > If you make this a post-task does it save a lot of the hoop-dancing the
patch
> > does to get around different call stacks?
> 
> So a post task here to avoid a post task in CreateAndInitializeOutputSurface? 
> What does that clean up?

I was thinking of the various "hold onto this pointer and clear it cuz it will
do the recreate synchronously type stuff that occurs with comments explaining it
in 2 or 3 places in this CL.

[enne (OOO), 2014-06-13 20:32:29]

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/1240001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:601: CreateAndInitializeOutputSurface();
On 2014/06/13 20:16:38, danakj wrote:
> On 2014/06/13 20:10:42, enne wrote:
> > On 2014/06/13 18:34:10, danakj wrote:
> > > If you make this a post-task does it save a lot of the hoop-dancing the
> patch
> > > does to get around different call stacks?
> > 
> > So a post task here to avoid a post task in
CreateAndInitializeOutputSurface? 
> > What does that clean up?
> 
> I was thinking of the various "hold onto this pointer and clear it cuz it will
> do the recreate synchronously type stuff that occurs with comments explaining
it
> in 2 or 3 places in this CL.

Done in the last patch set.  There's an awkward comment block here now, but that
should bite fewer people than the synchronous output surface recreation, I
think.

[no sievers, 2014-06-13 21:25:43]

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

https://codereview.chromium.org/134623005/diff/1340002/content/browser/render...
content/browser/renderer_host/compositor_impl_android.cc:312:
host_->SetDeferCommits(true);
I'm slightly worried that during client_->Layout() below, where the tab stack
syncs its internal representation of the UI with the cc layer tree, we somehow
end up redundantly touching some layer property which keeps us compositing
continuously.

Is it also possible to ignore 'set needs commit'? Otherwise maybe we can come up
with some sort of DCHECK() in the app that if no properties changed in the
original representation we don't end up with a 'needs commit' from the call to
Layout().

https://codereview.chromium.org/134623005/diff/1340002/content/browser/render...
content/browser/renderer_host/compositor_impl_android.cc:458:
PostComposite(COMPOSITE_IMMEDIATELY);
Does this need to become host_->SetNeedsCommit()?

Does PostComposite() need to go away entirely? What happens if we directly call
Composite?

[danakj, 2014-06-13 21:41:54]

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

https://codereview.chromium.org/134623005/diff/1340002/content/browser/render...
content/browser/renderer_host/compositor_impl_android.cc:312:
host_->SetDeferCommits(true);
On 2014/06/13 21:25:43, sievers wrote:
> I'm slightly worried that during client_->Layout() below, where the tab stack
> syncs its internal representation of the UI with the cc layer tree, we somehow
> end up redundantly touching some layer property which keeps us compositing
> continuously.
> 
> Is it also possible to ignore 'set needs commit'? Otherwise maybe we can come
up
> with some sort of DCHECK() in the app that if no properties changed in the
> original representation we don't end up with a 'needs commit' from the call to
> Layout().

FWIW You could run with the --strict-layer-property-change-checking flag that
would cause dchecks when this happens.

[enne (OOO), 2014-06-17 17:15:23]

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

https://codereview.chromium.org/134623005/diff/1340002/content/browser/render...
content/browser/renderer_host/compositor_impl_android.cc:458:
PostComposite(COMPOSITE_IMMEDIATELY);
On 2014/06/13 21:25:43, sievers wrote:
> Does this need to become host_->SetNeedsCommit()?
> 
> Does PostComposite() need to go away entirely? What happens if we directly
call
> Composite?

Hmm, I was trying to minimally change this code, but it looks like maybe I need
to convert the browser compositor to not call composite directly and just be
scheduled by the scheduler as well.

[enne (OOO), 2014-06-18 21:03:30]

PTAL

In order to make this patch more landable, I've left CompositorImpl doing the
same things it used to do.  ScheduleComposite/Animation had to stay because of
that, but CompositorImpl is now the only implementer of that function. 
CompositorImpl also passes a flag to not create the scheduler, which I will rip
back out again once CompositorImpl uses the scheduler.

[enne (OOO), 2014-06-18 21:03:42]

https://codereview.chromium.org/134623005/diff/1390001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_single_thread_client.h (right):

https://codereview.chromium.org/134623005/diff/1390001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_single_thread_client.h:13: virtual void
ScheduleComposite() {}
I had to bring these back to make Android browser compositor work.

https://codereview.chromium.org/134623005/diff/1390001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest.cc (right):

https://codereview.chromium.org/134623005/diff/1390001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:4635: // MULTI_THREAD_TEST_F(
This is just a presubmit error introduced by another change.

https://codereview.chromium.org/134623005/diff/1390001/cc/trees/layer_tree_se...
File cc/trees/layer_tree_settings.h (right):

https://codereview.chromium.org/134623005/diff/1390001/cc/trees/layer_tree_se...
cc/trees/layer_tree_settings.h:24: bool single_thread_proxy_scheduler;
It seemed like the safest way to make the Android browser compositor continue to
behave the same (although I'll come back and fix it next) was to just not create
a scheduler.  The easiest way to pipe that through is a LayerTreeSetting.

[danakj, 2014-06-19 17:20:37]

https://codereview.chromium.org/134623005/diff/1390001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/134623005/diff/1390001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:600: if (!layer_tree_host_)
instead of early out now just:
if (lth)
  lth->SetVisible()

https://codereview.chromium.org/134623005/diff/1390001/cc/test/test_web_graph...
File cc/test/test_web_graphics_context_3d.cc (right):

https://codereview.chromium.org/134623005/diff/1390001/cc/test/test_web_graph...
cc/test/test_web_graphics_context_3d.cc:447: 
extra whitespace

https://codereview.chromium.org/134623005/diff/1390001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/1390001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:98: UpdateBackgroundAnimateTicking();
I probably asked you to put the scheduler call last here, but ThreadProxy does
not do it last I noticed :/ Can we make them match (change here or there)

https://codereview.chromium.org/134623005/diff/1390001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:128: } else if (Proxy::MainThreadTaskRunner()) {
If there's no scheduler we should SNC()? Is this related to the
ScheduleComposite() added in DidAbortSwapBuffers?

https://codereview.chromium.org/134623005/diff/1390001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:145: client_->ScheduleAnimation();
This will ScheduleAnimation() and then ScheduleComposite(), remove this and just
SNC()? (Do we need to differentiate Animate vs Composite?)

https://codereview.chromium.org/134623005/diff/1390001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:152: client_->ScheduleComposite();
This will ScheduleComposite() twice now, remove this and just SNC()?

https://codereview.chromium.org/134623005/diff/1390001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:210: UpdateBackgroundAnimateTicking();
This happens after LTHI::CommitComplete() on the thread, can it move there for
here as well?

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

https://codereview.chromium.org/134623005/diff/1390001/content/browser/render...
content/browser/renderer_host/compositor_impl_android.cc:580:
ScheduleComposite();
How come this is needed now?

https://codereview.chromium.org/134623005/diff/1390001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/134623005/diff/1390001/ui/compositor/composit...
ui/compositor/compositor.cc:72: last_started_frame_(0),
this var can go with its accessor method?

https://codereview.chromium.org/134623005/diff/1390001/ui/compositor/composit...
ui/compositor/compositor.cc:73: last_ended_frame_(0),
if this is needed, why isn't it important for us to update the
last_started_frame too? can this go away?

https://codereview.chromium.org/134623005/diff/1390001/ui/compositor/composit...
ui/compositor/compositor.cc:77: schedule_draw_factory_(this) {
this var can go

[enne (OOO), 2014-06-19 20:32:28]

https://codereview.chromium.org/134623005/diff/1390001/cc/test/layer_tree_tes...
File cc/test/layer_tree_test.cc (right):

https://codereview.chromium.org/134623005/diff/1390001/cc/test/layer_tree_tes...
cc/test/layer_tree_test.cc:600: if (!layer_tree_host_)
On 2014/06/19 17:20:36, danakj wrote:
> instead of early out now just:
> if (lth)
>   lth->SetVisible()

Done.

https://codereview.chromium.org/134623005/diff/1390001/cc/test/test_web_graph...
File cc/test/test_web_graphics_context_3d.cc (right):

https://codereview.chromium.org/134623005/diff/1390001/cc/test/test_web_graph...
cc/test/test_web_graphics_context_3d.cc:447: 
On 2014/06/19 17:20:36, danakj wrote:
> extra whitespace

Done.

https://codereview.chromium.org/134623005/diff/1390001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/1390001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:98: UpdateBackgroundAnimateTicking();
On 2014/06/19 17:20:36, danakj wrote:
> I probably asked you to put the scheduler call last here, but ThreadProxy does
> not do it last I noticed :/ Can we make them match (change here or there)

Done.  I don't think the order matters here, but I have moved
UpdateBackgroundAnimateTicking last in both to be consistent.

https://codereview.chromium.org/134623005/diff/1390001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:145: client_->ScheduleAnimation();
On 2014/06/19 17:20:36, danakj wrote:
> This will ScheduleAnimation() and then ScheduleComposite(), remove this and
just
> SNC()? (Do we need to differentiate Animate vs Composite?)

Yeah, for now it calls root_window_->Animate in CompositorImpl::Composite if
this has been called, so it's not a no-op.

https://codereview.chromium.org/134623005/diff/1390001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:152: client_->ScheduleComposite();
On 2014/06/19 17:20:36, danakj wrote:
> This will ScheduleComposite() twice now, remove this and just SNC()?

Done.

https://codereview.chromium.org/134623005/diff/1390001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:210: UpdateBackgroundAnimateTicking();
On 2014/06/19 17:20:36, danakj wrote:
> This happens after LTHI::CommitComplete() on the thread, can it move there for
> here as well?

Done.

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

https://codereview.chromium.org/134623005/diff/1390001/content/browser/render...
content/browser/renderer_host/compositor_impl_android.cc:580:
ScheduleComposite();
On 2014/06/19 17:20:36, danakj wrote:
> How come this is needed now?

This is the equivalent of the removed lines in void
SingleThreadProxy::CreateAndInitializeOutputSurface():

    // Force another recreation attempt to happen by requesting another commit.
    SetNeedsCommit();

I felt like it made SingleThreadProxy cleaner.  Only this manually scheduled one
needs the scheduled composite.

https://codereview.chromium.org/134623005/diff/1390001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/134623005/diff/1390001/ui/compositor/composit...
ui/compositor/compositor.cc:73: last_ended_frame_(0),
On 2014/06/19 17:20:36, danakj wrote:
> if this is needed, why isn't it important for us to update the
> last_started_frame too? can this go away?

I'll just remove it.  This is a good catch too because I removed an async begin
but not the async end using last_started_frame.

https://codereview.chromium.org/134623005/diff/1390001/ui/compositor/composit...
ui/compositor/compositor.cc:77: schedule_draw_factory_(this) {
On 2014/06/19 17:20:36, danakj wrote:
> this var can go

Done.

[danakj, 2014-06-19 22:04:02]

Last round of comments I think.. thanks for all the fixings

https://codereview.chromium.org/134623005/diff/1430001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/1430001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:268: if (!defer_commits_ &&
finish_commit_deferred_) {
nit: finish_deferred_commit_? that's what it's saying that it will do right?

https://codereview.chromium.org/134623005/diff/1430001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:277: return finish_commit_deferred_;
Can you convince me why this shouldn't be true after SNC() until a
BeginMainFrame happens?

Wouldn't this trigger if you scrolled something?
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/layer.cc...

https://codereview.chromium.org/134623005/diff/1430001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:282: return finish_commit_deferred_;
Can we NOTREACHED() with a comment about why here? This is only used by
RenderWidget for input events.

https://codereview.chromium.org/134623005/diff/1430001/ui/compositor/composit...
File ui/compositor/compositor.cc (left):

https://codereview.chromium.org/134623005/diff/1430001/ui/compositor/composit...
ui/compositor/compositor.cc:66: namespace {
lol..

https://codereview.chromium.org/134623005/diff/1430001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/134623005/diff/1430001/ui/compositor/composit...
ui/compositor/compositor.cc:158: host_->SetNeedsCommit();
I think this should be SetNeedsUpdate, though we don't have aborted commits
here.. so maybe it doesn't matter. Do you think it does?

[enne (OOO), 2014-06-19 22:38:06]

https://codereview.chromium.org/134623005/diff/1430001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/1430001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:268: if (!defer_commits_ &&
finish_commit_deferred_) {
On 2014/06/19 22:04:02, danakj wrote:
> nit: finish_deferred_commit_? that's what it's saying that it will do right?

Months ago, this actually was deferring a function called FinishCommit.  I'll
rename it to commit_was_deferred_.

https://codereview.chromium.org/134623005/diff/1430001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:277: return finish_commit_deferred_;
On 2014/06/19 22:04:02, danakj wrote:
> Can you convince me why this shouldn't be true after SNC() until a
> BeginMainFrame happens?
> 
> Wouldn't this trigger if you scrolled something?
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/layers/layer.cc...

Ok, fixed up to call set it to true in SNC and false after Layout to correspond
with ThreadProxy.

I think my previous thought had been that BeginMainFrame and commit are always
synchronous so only deferred commits would indicate a commit was in flight, but
SetNeedsCommit and commit may not be synchronous with each other.

https://codereview.chromium.org/134623005/diff/1430001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:282: return finish_commit_deferred_;
On 2014/06/19 22:04:02, danakj wrote:
> Can we NOTREACHED() with a comment about why here? This is only used by
> RenderWidget for input events.

Why put an extra landmine? I'd rather just have this work since it's no extra
complexity.  I don't like needless "no callers use this so make it explode"
code.

https://codereview.chromium.org/134623005/diff/1430001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/134623005/diff/1430001/ui/compositor/composit...
ui/compositor/compositor.cc:158: host_->SetNeedsCommit();
On 2014/06/19 22:04:02, danakj wrote:
> I think this should be SetNeedsUpdate, though we don't have aborted commits
> here.. so maybe it doesn't matter. Do you think it does?

I don't think it matters.  Also, ui::Compositor uses it internally in set
background color to mean commit as well.

[danakj, 2014-06-19 22:41:21]

https://codereview.chromium.org/134623005/diff/1430001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/1430001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:282: return finish_commit_deferred_;
On 2014/06/19 22:38:06, enne wrote:
> On 2014/06/19 22:04:02, danakj wrote:
> > Can we NOTREACHED() with a comment about why here? This is only used by
> > RenderWidget for input events.
> 
> Why put an extra landmine? I'd rather just have this work since it's no extra
> complexity.  I don't like needless "no callers use this so make it explode"
> code.

I was thinking that we could remove it and make it some thread-only thing, and
NOTREACHED()s in here would clearly show what can be removed.

[danakj, 2014-06-19 22:42:02]

LGTM

[enne (OOO), 2014-06-20 17:54:04]

brianderson mentioned that he wanted a chance to look at this too, so holding
off on landing until then.

[brianderson, 2014-06-20 23:08:13]

https://codereview.chromium.org/134623005/diff/1450001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest.cc (left):

https://codereview.chromium.org/134623005/diff/1450001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:95: EXPECT_GE(1, num_draws_);
Why did these change from EXPECT_GE to EXPECT_LE?

https://codereview.chromium.org/134623005/diff/1450001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/1450001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:132:
base::Bind(&SingleThreadProxy::CreateAndInitializeOutputSurface,
Is this a tight polling mechanism? What happens if this method always fails?

I remember there was logic before that would count the number of output surface
creation failures - has that been removed?

https://codereview.chromium.org/134623005/diff/1450001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:531: BlockingTaskRunner::CapturePostTasks
blocked;
Is this only needed for the PostSwapBuffersComplete in
OutputSurface::SwapBuffers? I wonder if we can just call DidSwapBuffersComplete
directly instead of posting the task there.

https://codereview.chromium.org/134623005/diff/1450001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:588: return DoComposite(gfx::FrameTime::Now(),
&frame);
Instead of Now(), can you use layer_tree_host_impl_->CurrentFrameTimeTicks() ?

https://codereview.chromium.org/134623005/diff/1450001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:598: DoCommit(gfx::FrameTime::Now());
Instead of Now(), can you use layer_tree_host_impl_->CurrentFrameTimeTicks() ?

https://codereview.chromium.org/134623005/diff/1450001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/1450001/cc/trees/thread_proxy....
cc/trees/thread_proxy.cc:457: if (main().defer_commits == defer_commits)
Why is this not a DCHECK anymore?

[danakj, 2014-06-20 23:15:31]

I'll let enne@ answer the rest but here's some feedback

https://codereview.chromium.org/134623005/diff/1450001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/1450001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:132:
base::Bind(&SingleThreadProxy::CreateAndInitializeOutputSurface,
On 2014/06/20 23:08:12, brianderson wrote:
> Is this a tight polling mechanism? What happens if this method always fails?
> 
> I remember there was logic before that would count the number of output
surface
> creation failures - has that been removed?

That's inside
layer_tree_host_->OnCreateAndInitializeOutputSurfaceAttempted(success);

https://codereview.chromium.org/134623005/diff/1450001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:531: BlockingTaskRunner::CapturePostTasks
blocked;
On 2014/06/20 23:08:12, brianderson wrote:
> Is this only needed for the PostSwapBuffersComplete in
> OutputSurface::SwapBuffers? I wonder if we can just call
DidSwapBuffersComplete
> directly instead of posting the task there.

This is for releasing resources from ResourceProvider back to the main thread.

https://codereview.chromium.org/134623005/diff/1450001/cc/trees/thread_proxy.cc
File cc/trees/thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/1450001/cc/trees/thread_proxy....
cc/trees/thread_proxy.cc:457: if (main().defer_commits == defer_commits)
On 2014/06/20 23:08:13, brianderson wrote:
> Why is this not a DCHECK anymore?

RenderWidgetCompositor no longer guards this by mirroring the deferred state to
prevent us from setting it twice. It had an early out, now it just calls
through.

[enne (OOO), 2014-06-23 17:08:45]

https://codereview.chromium.org/134623005/diff/1450001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest.cc (left):

https://codereview.chromium.org/134623005/diff/1450001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:95: EXPECT_GE(1, num_draws_);
On 2014/06/20 23:08:12, brianderson wrote:
> Why did these change from EXPECT_GE to EXPECT_LE?

The comment on line 72 says "at least one commit and at least one draw".  I
think I needed this earlier in this patch sequence but it isn't needed now.  I
can put it in another patch if you'd like.

https://codereview.chromium.org/134623005/diff/1450001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/1450001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:588: return DoComposite(gfx::FrameTime::Now(),
&frame);
On 2014/06/20 23:08:12, brianderson wrote:
> Instead of Now(), can you use layer_tree_host_impl_->CurrentFrameTimeTicks() ?

Done.

https://codereview.chromium.org/134623005/diff/1450001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:598: DoCommit(gfx::FrameTime::Now());
On 2014/06/20 23:08:12, brianderson wrote:
> Instead of Now(), can you use layer_tree_host_impl_->CurrentFrameTimeTicks() ?

Done.

[brianderson, 2014-06-23 20:20:05]

Thanks for answering my questions. lgtm2.

https://codereview.chromium.org/134623005/diff/1450001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest.cc (left):

https://codereview.chromium.org/134623005/diff/1450001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:95: EXPECT_GE(1, num_draws_);
Don't worry about moving it to a separate patch.

[enne (OOO), 2014-06-23 20:22:24]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-06-23 20:24:04]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/134623005/1510001

[commit-bot: I haz the power, 2014-06-24 02:07:27]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[commit-bot: I haz the power, 2014-06-24 02:11:09]

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

[commit-bot: I haz the power, 2014-06-24 02:11:11]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[enne (OOO), 2014-06-24 06:58:14]

boliu: android_webview/ OWNERS
piman: content/ OWNERS

[boliu, 2014-06-24 16:33:11]

The DCHECK in HardwareRenderer::DidBeginMainFrame is failing. Previously it was
assumed to be only called from Composite. Now it looks like AddChild can cause a
commit. I'm not sure if this is expected? Will Composite cause another commit
(which gets aborted)?

DidBeginMainFrame is only used to set the draw contraints on the OutputSurface,
and DidBeginMainFrame was the cleanest ways to do it. The issue was
OutputSurface is created the first time Composite is called. We could do the
safer but slightly less clean way if DidBeginMainFrame and other callbacks don't
work.

[piman, 2014-06-24 18:29:06]

LGTM for content/

[enne (OOO), 2014-06-24 18:54:04]

On 2014/06/24 16:33:11, boliu wrote:
> The DCHECK in HardwareRenderer::DidBeginMainFrame is failing. Previously it
was
> assumed to be only called from Composite. Now it looks like AddChild can cause
a
> commit. I'm not sure if this is expected? Will Composite cause another commit
> (which gets aborted)?

Thanks for helping me get this sorted out.  HardwareRenderer now also sets the
"single_thread_proxy_scheduler = false" flag like CompositorImpl.

[boliu, 2014-06-24 18:54:56]

lgtm

[enne (OOO), 2014-06-24 18:56:42]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-06-24 18:58:35]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/134623005/1530001

[commit-bot: I haz the power, 2014-06-24 22:11:21]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[enne (OOO), 2014-06-24 22:34:58]

The CQ bit was unchecked by enne@chromium.org

[enne (OOO), 2014-06-24 22:40:15]

The win8 aura issues seem legit.  It looks like the copy output results posted
to the main thread aren't getting run.

[enne (OOO), 2014-06-25 22:49:04]

Ok, I see what's happening.  I think this has potentially always been flaky.  

These two things always happen in order on one thread.
(1) OnCompositingStarted (swap posted to gpu)
(2) Post relay output request callback (async readback finished, post original
callback back to main thread task runner)

At some point asynchronously, the gpu finishes the swap and posts a task to run
DidCompleteSwapBuffers back on the same thread that #1 and #2 are running on. 
When run, this task calls OnCompositingEnded, exiting the nested
DrawWaiterForTest message loop.

There's a race in task ordering between the output request and
DidCompleteSwapBuffers.  If DidCompleteSwapBuffers gets run first, then the loop
in layer_unittest.cc's ReadPixels routine finishes and exits the message loop
prior to the output request callback running.  If it takes longer and runs
second (which is usually does), then the output request callback goes first.

A simple fix here is just to loop one more time to catch this case. 
Alternatively, I could also change DrawWaiter to wait for OnCompositingStarted
instead, but I'd still need another loop.

[enne (OOO), 2014-06-26 00:06:35]

Ok, after a conversation with danakj, I went ahead and made DrawWaiter
differentiate between OnCompositingStarted and OnCompositingEnded.  This pointed
out a bug where sometimes in ui::Compositor OnCompositingStarted might not occur
because commits and draws are now separable (thanks, scheduler!).

To fix this, OnCompositingStarted gets moved to OnPostSwapBuffers rather than
DidCommitAndDrawFrame, as not every draw comes from a commit.  However,
OnPostSwapBuffers is a SingleThreadProxy only feature.  Rather than piping
through a message from the compositor thread in ThreadProxy that nobody else
cares about, ui::Compositor synthesizes OnCompositingStarted messages in the
case that threaded mode is being used and OnCompositingEnded occurs.

DrawWaiterForTest now differentiates between what it's waiting on, and so a
bunch of code in layer_unittest.cc got adjusted to either wait for draws (aka
OnCompositingStarted) or swaps (OnCompositingEnded), depending on what it's
actually looking for.

Most importantly, ReadPixels now just waits for OnCompositingStarted, which
doesn't race with DidCompleteSwapBuffers and so now reliably takes exactly three
loops to finish, which I think is more understandable than working around
raciness.

[danakj, 2014-06-26 00:19:06]

LGTM thanks!!

[enne (OOO), 2014-06-26 00:21:58]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-06-26 00:24:02]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/134623005/1570001

[enne (OOO), 2014-06-26 01:37:00]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2014-06-26 01:37:50]

The CQ bit was unchecked by enne@chromium.org

[enne (OOO), 2014-06-26 01:38:26]

ben: ui/views and ui/snapshot OWNERS

[enne (OOO), 2014-06-26 17:26:12]

TBRing ben for ui/ since it's just renaming a function from ui/compositor/.

[enne (OOO), 2014-06-26 17:26:43]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-06-26 17:27:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/134623005/1590001

[commit-bot: I haz the power, 2014-06-26 21:58:10]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[Ben Goodger (Google), 2014-06-26 22:35:54]

lgtm

[commit-bot: I haz the power, 2014-06-26 23:10:34]

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

[commit-bot: I haz the power, 2014-06-26 23:10:36]

Try jobs failed on following builders:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[enne (OOO), 2014-06-27 00:57:42]

Hmm, these all look like legit failures in browser tests that are now trying to
create output surfaces but don't have a RenderThreadImpl.  This got suppressed
previously because RenderWidget specially ignores scheduleComposite in this case
and the output surface was only created during the first composite.  I'll see
what I can do here.

[enne (OOO), 2014-06-27 19:16:51]

https://codereview.chromium.org/134623005/diff/1610001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/1610001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:262: if (!scheduler_on_impl_thread_)
This has to not be a DCHECK anymore because the scheduler is created in
SetLayerTreeHostClientReady which may be skipped in tests.

https://codereview.chromium.org/134623005/diff/1610001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/134623005/diff/1610001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:439: if
(RenderThreadImpl::current())
This is the fix for browser tests.  By not setting layer tree host client ready,
it prevents output surface creation, as happened previously.

[enne (OOO), 2014-06-27 19:17:02]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-06-27 19:18:20]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/134623005/1610001

[commit-bot: I haz the power, 2014-06-27 22:21:38]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-06-27 22:29:37]

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

[commit-bot: I haz the power, 2014-06-27 22:29:39]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)

[enne (OOO), 2014-06-28 00:11:04]

Trybots turned up another failure where
WebContentsViewAuraTest.OverscrollNavigationWithTouchHandler timed out.  It
turns out this was because the test was waiting for animations to occur, which
were no longer happening with the changes to ui::Compositor.

Patch set #42, you are my only hope!

https://codereview.chromium.org/134623005/diff/1630001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest_animation.cc (right):

https://codereview.chromium.org/134623005/diff/1630001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest_animation.cc:769: EXPECT_TRUE(have_animated_);
This would fail in the single thread case if the call to UpdateClientAnimations
isn't made.

https://codereview.chromium.org/134623005/diff/1630001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/1630001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:161:
layer_tree_host_->UpdateClientAnimations(frame_begin_time);
This used to be done in ui::Compositor::Draw, but now that ui::Compositor isn't
doing its own drawing, SingleThreadProxy needs to make this call.  (On the other
hand, ThreadProxy makes this call, so this is some cleanup of previous
asymmetry.)

[enne (OOO), 2014-06-28 00:11:25]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-06-28 00:12:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/134623005/1630001

[enne (OOO), 2014-06-28 00:37:53]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-06-28 00:38:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/134623005/1650001

[commit-bot: I haz the power, 2014-06-28 05:35:58]

Change committed as 280493

[sof, 2014-06-28 13:53:46]

Causing a number of crashes on the blink bots, e.g.,
  http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel

[Nikita (slow), 2014-06-28 17:48:19]

I wonder if this was the root cause for test failure, see
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20Tests%20%...

I'm thinking of reverting this CL to verify that.

[Nikita (slow), 2014-06-28 17:50:12]

A revert of this CL has been created in
https://codereview.chromium.org/359103003/ by nkostylev@chromium.org.

The reason for reverting is: Check if this CL breaks
TouchActionBrowserTest.TouchActionNone test http://goo.gl/pwdNwL

Also it was reported that this CL causing a number of crashes on the blink bots,
e.g.,
  http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel
.

[Nikita (slow), 2014-06-28 17:51:56]

A revert of this CL has been created in
https://codereview.chromium.org/342413005/ by nkostylev@chromium.org.

The reason for reverting is: Check if this CL breaks
TouchActionBrowserTest.TouchActionNone test http://goo.gl/pwdNwL 

Also it was reported that this CL causing a number of crashes on the blink bots,
e.g., 
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel .

[Nikita (slow), 2014-06-28 17:55:10]

Ok, these reverts CLs are not properly tagged so I'm using drover now.

[Nikita (slow), 2014-06-28 17:57:56]

On 2014/06/28 17:55:10, Nikita Kostylev wrote:
> Ok, these reverts CLs are not properly tagged so I'm using drover now.

https://codereview.chromium.org/359103004/

[Nikita (slow), 2014-06-28 18:30:42]

On 2014/06/28 13:53:46, sof wrote:
> Causing a number of crashes on the blink bots, e.g.,
>   http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel

Somehow blink bot got green on 280515 which was just version update
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...

Next build was with my revert and all builds are green after that.
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...

[Nikita (slow), 2014-06-28 18:49:49]

On 2014/06/28 17:48:19, Nikita Kostylev wrote:
> I wonder if this was the root cause for test failure, see
>
http://build.chromium.org/p/chromium.memory/builders/Linux%20ASan%20Tests%20%...
> 
> I'm thinking of reverting this CL to verify that.

Bot is green now:
http://build.chromium.org/p/chromium.memory/buildstatus?builder=Linux%20ASan%...

[Nikita (slow), 2014-06-28 18:54:28]

On 2014/06/28 13:53:46, sof wrote:
> Causing a number of crashes on the blink bots, e.g.,
>   http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel

For the reference crashes:

http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...

started with
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...
(@280513)

previous build was green
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...
(@280484)

[enne (OOO), 2014-06-30 22:22:08]

https://codereview.chromium.org/134623005/diff/1670001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/134623005/diff/1670001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:580:
layer_tree_host_->Composite(gfx::FrameTime::Now());
This call was breaking in layout tests as it would stack up more pending swaps
than actual swaps.  Calling Composite() manually is now mutually exclusive with
using the scheduler, and so I've added a DCHECK to the LTH::Composite function
too.

[mithro-old, 2014-07-02 06:05:47]

Hi,

What is the status of this patch, it looks like it was landed and then reverted?
Do you think you can land it again easily?

Thanks!

Tim

[enne (OOO), 2014-07-02 17:23:52]

On 2014/07/02 at 06:05:47, mithro wrote:

> What is the status of this patch, it looks like it was landed and then
reverted? Do you think you can land it again easily?

Landing it turned up a bunch of bugs in Blink layout tests, which I hadn't
caught before.  I am hoping to work through all the bugs by early next week.

There was an obvious crasher (compositeAndReadbackAsync needed to not call
composite directly unless the scheduler wasn't being used or swaps piled up and
the scheduler exploded).  There was an animation bug (WebViewImpl::layout must
be called prior to WebViewImpl::Animate().  There were some crashes with
shutdown races between creating output surfaces (which are now async) and an
async close message to the browser which destroyed the gpu surface.  I've almost
got those fixed, but I need to land a patch in Blink for them.  There's also
some animation issues, which I'm hoping to solve by recording the time at which
the test finishes and then animate the frame at that time later.  There may be
some other issues, but I need to rerun layout tests and see what else turns up.

[enne (OOO), 2014-07-02 21:19:43]

I'm going to wait on https://codereview.chromium.org/362403003/ to land and then
see what the Blink trybots have to say.

https://codereview.chromium.org/134623005/diff/1710001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/1710001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:162: layer_tree_host_->Layout();
This ordering is how ThreadProxy does it too.  Without it, animations are off by
a frame.

https://codereview.chromium.org/134623005/diff/1710001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/134623005/diff/1710001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:676:
widget_->scheduleAnimation();
Unfortunately, I had to bring this back to get WebTestProxy's fake raf to run
instantly to get the precision needed in tests to get Javascript animation
inspection to pass.  I think this can move into the scheduler, but am punting in
this patch.

https://codereview.chromium.org/134623005/diff/1710001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/134623005/diff/1710001/content/renderer/rende...
content/renderer/render_widget.cc:1306: DestroyLayerTreeView();
This is the Chromium side of preventing the race that's described in
https://codereview.chromium.org/362403003/.

[enne (OOO), 2014-07-02 21:19:46]

I'm going to wait on https://codereview.chromium.org/362403003/ to land and then
see what the Blink trybots have to say.

[danakj, 2014-07-02 21:31:42]

LGTM

https://codereview.chromium.org/134623005/diff/1710001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/134623005/diff/1710001/content/renderer/rende...
content/renderer/render_widget.cc:1171: if (host_closing_)
Do you think this can/should be a DCHECK one day? If you do can you TODO that it
should be?

[enne (OOO), 2014-07-10 20:37:43]

I'm going to throw this at the blink bots and see what comes out.

Locally on Linux there's about ~10 tests that are rebaselined for the better. 
About ~4 that just need to be rebaselined for unknown reasons.  There's about ~4
tests (with video) that are now flaky and I can't figure out what to do about
them, and I'm just going to file a bug and move on.

https://codereview.chromium.org/134623005/diff/1710001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/134623005/diff/1710001/content/renderer/rende...
content/renderer/render_widget.cc:1171: if (host_closing_)
On 2014/07/02 21:31:42, danakj wrote:
> Do you think this can/should be a DCHECK one day? If you do can you TODO that
it
> should be?

Done.

https://codereview.chromium.org/134623005/diff/1730001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/1730001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:391: void
SingleThreadProxy::DidActivateSyncTree() {
This gets called now even for non impl-side painting due to alokp's recent work.
 Changing it from NOTREACHED to no-op here.

https://codereview.chromium.org/134623005/diff/1730001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/134623005/diff/1730001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:423: void
RenderWidgetCompositor::SetSuppressScheduleComposite(bool suppress) {
This no longer makes any sense in an FCM world, and deferring commits was just
wrong.

I will remove it in a follow-up.

https://codereview.chromium.org/134623005/diff/1750001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/134623005/diff/1750001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:677:
layer_tree_host_->CompositeNoSwap(gfx::FrameTime::Now());
This is the most unfortunate part of this patch.  In my epic failure of a
struggle against layout test flakiness, I was unable to wrangle tests to not be
(as) flaky without synchronously doing the commit and composite as a part of the
notifyDone callstack that.

I tried a bunch of other approaches (saving animation times, synchronous commit
but deferred composite), but either my implementation was bogus or there was
some other form of flakiness (e.g. accelerated video).

There may be some follow-up cleanup that would make this less awful, but for now
this grossness needs to live on.

The reason it doesn't swap is because the scheduler is really not expecting a
swap at this point and will explode if it piles up too many due to backpressure.
 This is just a readback, so it doesn't actually need to make it to the screen. 
Therefore, CompositeNoSwap.

https://codereview.chromium.org/134623005/diff/1750001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/134623005/diff/1750001/content/renderer/rende...
content/renderer/render_widget.cc:1186: // Always send this notification to
prevent new layer tree views from
This fixed a race where the layer tree view would try to be created for the
first time while shutting down.

[jamesr, 2014-07-10 21:55:40]

https://codereview.chromium.org/134623005/diff/1730001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/134623005/diff/1730001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:423: void
RenderWidgetCompositor::SetSuppressScheduleComposite(bool suppress) {
On 2014/07/10 20:37:42, enne wrote:
> This no longer makes any sense in an FCM world, and deferring commits was just
> wrong.
> 
> I will remove it in a follow-up.

Which part doesn't make sense?  Suppressing composition during part of the page
load is very important for performance and FOUC prevention.  How this should
actually be accomplished is tricky, but the behavior is important.

[danakj, 2014-07-10 21:57:14]

https://codereview.chromium.org/134623005/diff/1730001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/134623005/diff/1730001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:423: void
RenderWidgetCompositor::SetSuppressScheduleComposite(bool suppress) {
On 2014/07/10 21:55:40, jamesr wrote:
> On 2014/07/10 20:37:42, enne wrote:
> > This no longer makes any sense in an FCM world, and deferring commits was
just
> > wrong.
> > 
> > I will remove it in a follow-up.
> 
> Which part doesn't make sense?  Suppressing composition during part of the
page
> load is very important for performance and FOUC prevention.  How this should
> actually be accomplished is tricky, but the behavior is important.

This is done by SetDeferCommits in WebViewImpl now, right?

[enne (OOO), 2014-07-10 22:03:49]

https://codereview.chromium.org/134623005/diff/1730001/content/renderer/gpu/r...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/134623005/diff/1730001/content/renderer/gpu/r...
content/renderer/gpu/render_widget_compositor.cc:423: void
RenderWidgetCompositor::SetSuppressScheduleComposite(bool suppress) {
On 2014/07/10 21:55:40, jamesr wrote:
> On 2014/07/10 20:37:42, enne wrote:
> > This no longer makes any sense in an FCM world, and deferring commits was
just
> > wrong.
> > 
> > I will remove it in a follow-up.
> 
> Which part doesn't make sense?  Suppressing composition during part of the
page
> load is very important for performance and FOUC prevention.  How this should
> actually be accomplished is tricky, but the behavior is important.

SetSuppressScheduleComposite != SetDeferCommits.

ScheduleComposite is the non-cc::Scheduler way for cc to tell its embedder to
schedule a composite.  Currently, WebViewImpl sets and unsets this during the
course of setRootGraphicslayer to prevent this call from going out.  The
intersection of the sets (code still using ScheduleComposite) and (code calling
SetSuppressScheduleComposite) is empty.

Preventing commits during page load is done by SetDeferCommits and is set during
setIsAcceleratedCompositingActive(false) and unset later on in a few places
where it seems likely that the page has loaded.  This needs to stay, for sure.

[jamesr, 2014-07-10 22:09:10]

Gotcha, makes sense.

[enne (OOO), 2014-07-11 17:32:24]

Hmm, looks like ccameron landed a patch this week that calls
ui::Compositor::Draw which got removed here.  That needs to get sorted out
before this can land.

[enne (OOO), 2014-07-13 05:22:43]

On 2014/07/11 at 17:32:24, enne wrote:
> Hmm, looks like ccameron landed a patch this week that calls
ui::Compositor::Draw which got removed here.  That needs to get sorted out
before this can land.

After some discussion it sounds like we'll try to figure out a different way to
make this work, so this patch can land.

Most of the layout tests look ok or better (see
https://storage.googleapis.com/chromium-layout-test-archives/win_blink_rel/16...),
but there are some repaint rect changes that I worry are flaky and a bunch of
inspector/tracing assertions that I just don't have time to look into.

Maybe it's me being conservative, but I'm reluctant to land a patch that might
make layout tests flaky when I'm out next week and don't have the time to
shepherd this through this weekend.  :(

[enne (OOO), 2014-07-23 20:45:53]

Punted on fixing flaky layout tests by adding a content switch to disable this
SingleThreadProxy scheduler and just do what it's always done.  This should make
this patch a lot less risky to land.

[enne (OOO), 2014-07-23 20:46:18]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-07-23 20:48:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/134623005/1870001

[enne (OOO), 2014-07-23 21:56:23]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-07-23 21:57:03]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/134623005/1890001

[commit-bot: I haz the power, 2014-07-24 03:04:09]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)

[commit-bot: I haz the power, 2014-07-24 03:46:12]

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

[commit-bot: I haz the power, 2014-07-24 03:46:15]

Try jobs failed on following builders:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

[enne (OOO), 2014-07-24 17:03:16]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-07-24 17:05:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/134623005/1890001

[Ken Russell (switch to Gerrit), 2014-07-24 18:00:36]

On 2014/07/24 03:46:15, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
>
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

These failures look real. Please contact me by email if they aren't.

[commit-bot: I haz the power, 2014-07-24 19:55:59]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)

[commit-bot: I haz the power, 2014-07-24 20:33:44]

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

[commit-bot: I haz the power, 2014-07-24 20:33:47]

Try jobs failed on following builders:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

[enne (OOO), 2014-07-25 21:20:31]

https://codereview.chromium.org/134623005/diff/1930001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/134623005/diff/1930001/ui/compositor/composit...
ui/compositor/compositor.cc:184: void Compositor::ScheduleFullRedraw() {
content/browser/compositor/browser_compositor_view_private_mac.mm calls this
function and expects it to schedule a commit (and animation) whereas
ScheduleRedraw in cc land means schedule a draw (and no commit).  I think
ScheduleDraw/ScheduleRedraw(Rect) maybe need some renaming to differentiate
them.

[enne (OOO), 2014-07-25 21:20:49]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-07-25 21:22:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/134623005/1930001

[piman, 2014-07-25 21:44:23]

https://codereview.chromium.org/134623005/diff/1930001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/134623005/diff/1930001/ui/compositor/composit...
ui/compositor/compositor.cc:190: host_->SetNeedsCommit();
Could we fix mac instead? The other callers don't need a commit.
(possibly as a follow up).

[enne (OOO), 2014-07-25 21:46:19]

https://codereview.chromium.org/134623005/diff/1930001/ui/compositor/composit...
File ui/compositor/compositor.cc (right):

https://codereview.chromium.org/134623005/diff/1930001/ui/compositor/composit...
ui/compositor/compositor.cc:190: host_->SetNeedsCommit();
On 2014/07/25 21:44:23, piman (slow to review) wrote:
> Could we fix mac instead? The other callers don't need a commit.
> (possibly as a follow up).

I'm putting up a follow up patch.  Just tired of accumulating more changes in
this neverending one.

[commit-bot: I haz the power, 2014-07-26 05:02:22]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)

[commit-bot: I haz the power, 2014-07-26 05:26:24]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg/bui...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-07-26 05:28:29]

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

[commit-bot: I haz the power, 2014-07-26 05:28:31]

Try jobs failed on following builders:
  mac_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...)

[enne (OOO), 2014-07-29 00:31:46]

Hmm, ok.  It looks like although this patch fixes a number of failures in gpu
tests, there is still some flakiness that I need to look into on Mac before this
can land.  There are two tests that something like: "TimeoutException: Timed out
while waiting 5s for IsSnapshotComplete."

http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...
http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...

[ccameron, 2014-07-29 00:33:20]

On 2014/07/29 00:31:46, enne wrote:
> Hmm, ok.  It looks like although this patch fixes a number of failures in gpu
> tests, there is still some flakiness that I need to look into on Mac before
this
> can land.  There are two tests that something like: "TimeoutException: Timed
out
> while waiting 5s for IsSnapshotComplete."
> 
>
http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...
>
http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_triggered...

That guy's almost certainly my fault ... the snapshot mechanism on Mac is ...
creative ... at best.

I'll patch in locally and see if I can reproduce it.

[enne (OOO), 2014-07-30 22:26:59]

https://codereview.chromium.org/134623005/diff/1980001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/1980001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:575: // Although this proxy is single-threaded,
it's problematic to synchronously
This is the fix for the flakiness around window screenshot callbacks timing out
on Mac.  This was Mac-only because other browser compositors are threaded.

The details are that DelegatedFrameHost would set the new frame information and
then in the same function set the latency info (containing the swap promise
callbacks for the screenshot).  Because SingleThreadProxy is mostly synchronous,
setting the frame would call SetNeedsCommit which would poke the scheduler to
ProcessScheduledActions which would then ScheduledActionBeginMainFrame.  This
function prior to this patch would synchronously start the commit.  This meant
that DelegatedFrameHost caused a commit between setting two properties, causing
one to be committed in one frame (the frame info, with the damage) and the other
to be committed in the next (the swap promise).  Because the second commit had
no damage, the draw/swap would early out and abort, cancelling the promise, and
leaving the snapshot callback hanging.  This was flaky, because the commit would
only happen if the timing was in the right part of the frame deadline.  In some
cases, no commit would occur and the frame info and swap promise would be put
together in the same frame, successfully completing the test.

The fix for this is to just make SendBeginMainFrame asynchronously call
BeginMainFrame (like ThreadProxy does).  This prevents weird synchronous commits
in the middle of a series of property changes.

[enne (OOO), 2014-07-30 22:27:18]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-07-30 22:28:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/134623005/1980001

[piman, 2014-07-30 22:31:00]

https://codereview.chromium.org/134623005/diff/1980001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/1980001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:575: // Although this proxy is single-threaded,
it's problematic to synchronously
On 2014/07/30 22:26:59, enne wrote:
> This is the fix for the flakiness around window screenshot callbacks timing
out
> on Mac.  This was Mac-only because other browser compositors are threaded.

Desktop Windows/Linux aren't threaded. That said I'm willing to believe Mac
quirks exposed it only there.

> 
> The details are that DelegatedFrameHost would set the new frame information
and
> then in the same function set the latency info (containing the swap promise
> callbacks for the screenshot).  Because SingleThreadProxy is mostly
synchronous,
> setting the frame would call SetNeedsCommit which would poke the scheduler to
> ProcessScheduledActions which would then ScheduledActionBeginMainFrame.  This
> function prior to this patch would synchronously start the commit.  This meant
> that DelegatedFrameHost caused a commit between setting two properties,
causing
> one to be committed in one frame (the frame info, with the damage) and the
other
> to be committed in the next (the swap promise).  Because the second commit had
> no damage, the draw/swap would early out and abort, cancelling the promise,
and
> leaving the snapshot callback hanging.  This was flaky, because the commit
would
> only happen if the timing was in the right part of the frame deadline.  In
some
> cases, no commit would occur and the frame info and swap promise would be put
> together in the same frame, successfully completing the test.
> 
> The fix for this is to just make SendBeginMainFrame asynchronously call
> BeginMainFrame (like ThreadProxy does).  This prevents weird synchronous
commits
> in the middle of a series of property changes.

LGTM on that. Re-entrancy would cause problems.

https://codereview.chromium.org/134623005/diff/1980001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:582: Proxy::MainThreadTaskRunner()->PostTask(
nit: no need for Proxy::

[enne (OOO), 2014-07-30 22:36:41]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-07-30 22:38:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/134623005/2000001

[enne (OOO), 2014-07-31 00:30:55]

The CQ bit was unchecked by enne@chromium.org

[enne (OOO), 2014-07-31 00:31:42]

Hmm, looks like the linux_chromium_rel content_browsertests
"[6:6:0730/161002:171199482597:FATAL:render_widget.cc(648)] Check failed:
resize_ack != SEND_RESIZE_ACK || !next_paint_is_resize_ack()" failure is real
and introduced by this patch.  More flaky tests!

[mlamouri (slow - plz ping), 2014-07-31 16:33:59]

On 2014/07/31 00:31:42, enne wrote:
> Hmm, looks like the linux_chromium_rel content_browsertests
> "[6:6:0730/161002:171199482597:FATAL:render_widget.cc(648)] Check failed:
> resize_ack != SEND_RESIZE_ACK || !next_paint_is_resize_ack()" failure is real
> and introduced by this patch.  More flaky tests!

It should have been fixed by https://codereview.chromium.org/436543002/

[enne (OOO), 2014-07-31 17:08:00]

On 2014/07/31 at 16:33:59, mlamouri wrote:
> On 2014/07/31 00:31:42, enne wrote:
> > Hmm, looks like the linux_chromium_rel content_browsertests
> > "[6:6:0730/161002:171199482597:FATAL:render_widget.cc(648)] Check failed:
> > resize_ack != SEND_RESIZE_ACK || !next_paint_is_resize_ack()" failure is
real
> > and introduced by this patch.  More flaky tests!
> 
> It should have been fixed by https://codereview.chromium.org/436543002/

Thanks so much for fixing that.  It seems fixed locally to me too.

[enne (OOO), 2014-07-31 17:08:15]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-07-31 17:09:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/134623005/2020001

[commit-bot: I haz the power, 2014-07-31 20:23:12]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[enne (OOO), 2014-07-31 22:30:03]

The CQ bit was unchecked by enne@chromium.org

[enne (OOO), 2014-07-31 22:30:30]

Hmm.  That ScreenOrientation test seems fixed on linux (no more crashing) but is
now timing out on windows.

[enne (OOO), 2014-08-04 20:43:53]

https://codereview.chromium.org/441713002/ appears to fix the Windows timeouts
(at least in one win32 and win64 tryjob each).  Once that lands, I'll try to
commit this again.

[danakj, 2014-08-05 13:41:00]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-08-05 13:41:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/134623005/2060001

[danakj, 2014-08-05 13:47:49]

On Mon, Aug 4, 2014 at 4:43 PM, <enne@chromium.org> wrote:

> https://codereview.chromium.org/441713002/ appears to fix the Windows
> timeouts
> (at least in one win32 and win64 tryjob each).  Once that lands, I'll try
> to
> commit this again.
>

Cool, it landed, so I'll CQ.

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

[commit-bot: I haz the power, 2014-08-05 17:47:13]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-05 17:51:13]

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

[commit-bot: I haz the power, 2014-08-05 17:51:18]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)

[danakj, 2014-08-05 17:52:43]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-08-05 17:54:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/134623005/2060001

[commit-bot: I haz the power, 2014-08-05 18:05:58]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-05 18:15:45]

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

[commit-bot: I haz the power, 2014-08-05 18:15:49]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)

[enne (OOO), 2014-08-05 18:16:59]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-08-05 18:20:50]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/134623005/2080001

[commit-bot: I haz the power, 2014-08-05 22:25:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)

[commit-bot: I haz the power, 2014-08-06 00:29:30]

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

[commit-bot: I haz the power, 2014-08-06 00:29:33]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)

[danakj, 2014-08-06 02:39:02]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-08-06 02:40:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/134623005/2080001

[commit-bot: I haz the power, 2014-08-06 07:21:57]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)

[commit-bot: I haz the power, 2014-08-06 10:16:53]

Change committed as 287747

[Sami, 2014-08-06 15:02:11]

A revert of this CL has been created in
https://codereview.chromium.org/448713002/ by skyostil@chromium.org.

The reason for reverting is: Looks like this made cc_perftests hang on all
platforms :(

http://build.chromium.org/p/chromium.perf/builders/Linux%20Perf%20%282%29/bui...

http://build.chromium.org/p/chromium.perf/builders/Mac%2010.9%20Perf%20%282%2...

http://build.chromium.org/p/chromium.perf/builders/Win%207%20Perf%20%282%29/b....

[danakj, 2014-08-06 20:52:12]

LLLLLLLLLLLLLLLLLLGTM

https://codereview.chromium.org/134623005/diff/2120001/chrome/browser/ui/view...
File chrome/browser/ui/views/menu_view_drag_and_drop_test.cc (right):

https://codereview.chromium.org/134623005/diff/2120001/chrome/browser/ui/view...
chrome/browser/ui/views/menu_view_drag_and_drop_test.cc:22: #if
defined(OS_LINUX) || defined(USE_AURA)
OS_LINUX || USE_AURA == USE_AURA

[enne (OOO), 2014-08-06 20:54:42]

Rereopened, rerelanding.

https://codereview.chromium.org/134623005/diff/2120001/chrome/browser/ui/view...
File chrome/browser/ui/views/menu_view_drag_and_drop_test.cc (right):

https://codereview.chromium.org/134623005/diff/2120001/chrome/browser/ui/view...
chrome/browser/ui/views/menu_view_drag_and_drop_test.cc:22: #if
defined(OS_LINUX) || defined(USE_AURA)
On 2014/08/06 20:52:12, danakj wrote:
> OS_LINUX || USE_AURA == USE_AURA

Done.

[enne (OOO), 2014-08-06 20:55:25]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-08-06 20:57:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/134623005/2140001

[commit-bot: I haz the power, 2014-08-07 01:20:28]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)

[commit-bot: I haz the power, 2014-08-07 03:15:52]

Change committed as 287950

[tkent, 2014-08-07 05:26:48]

On 2014/08/07 03:15:52, I haz the power (commit-bot) wrote:
> Change committed as 287950

Some layout tests started crashing since this.  Reverting.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=fas...

[tkent, 2014-08-07 05:27:29]

A revert of this CL has been created in
https://codereview.chromium.org/447133002/ by tkent@chromium.org.

The reason for reverting is: Broke some layout tests.

[danakj, 2014-08-07 13:05:52]

Not the greatest stacktrace from the linux debug bot:


22:35:50.328 984 worker/6 fast/dom/Window/new-window-opener.html
crashed, (stderr lines):
22:35:50.328 984   ASSERTION FAILED: !active || m_layerTreeView
22:35:50.328 984
../../third_party/WebKit/Source/web/WebViewImpl.cpp(3994) : void
blink::WebViewImpl::setIsAcceleratedCompositingActive(bool)
22:35:50.328 984   1   0x7f9ff09752a7
22:35:50.328 984   2   0x7f9ff097adef
22:35:50.328 984   3   0x7f9ff0864bc1
22:35:50.328 984   4   0x7f9ff2c4756c
22:35:50.328 984   5   0x7f9ff2c4432a
22:35:50.328 984   6   0x7f9ff2c43da6
22:35:50.328 984   7   0x7f9ff2c4495c
22:35:50.329 984   8   0x7f9ff2c4576d
22:35:50.329 984   9   0x7f9ff2756cc3
22:35:50.329 984   10  0x7f9ff275b3d3
22:35:50.329 984   11  0x7f9ffc6d218a
blink::ScrollView::adjustScrollbarExistence(blink::ScrollView::ComputeScrollbarExistenceOption)
22:35:50.329 984   12  0x7f9ffc6cff89
blink::ScrollView::updateScrollbars(blink::IntSize const&)
22:35:50.329 984   13  0x7f9ffc6d084a
blink::ScrollView::setContentsSize(blink::IntSize const&)




On Thu, Aug 7, 2014 at 1:27 AM, <tkent@chromium.org> wrote:

> A revert of this CL has been created in
> https://codereview.chromium.org/447133002/ by tkent@chromium.org.
>
> The reason for reverting is: Broke some layout tests.
>
> https://codereview.chromium.org/134623005/
>

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

[danakj, 2014-08-07 15:01:43]

Hm, this seemed to cause a commit to happen while the OS was still lost form the
POV of the LTH:

	00caef20 6a3a1e3a chrome_68850000!cc::LayerTreeHost::UpdateLayers+0x57
	00caf1ac 6a3a2b66 chrome_68850000!cc::SingleThreadProxy::DoCommit+0x16a
	00caf1d4 6a3dc188
chrome_68850000!cc::SingleThreadProxy::ScheduledActionCommit+0x36
	00caf2e0 6a3dbc34 chrome_68850000!cc::Scheduler::ProcessScheduledActions+0x158
	00caf314 6a3a14be chrome_68850000!cc::Scheduler::NotifyReadyToCommit+0xa4
	00caf3d8 69c5a5c6 chrome_68850000!cc::SingleThreadProxy::BeginMainFrame+0xce

*stares at code*

[danakj, 2014-08-07 15:10:37]

ScheduledActionSendBeginMainFrame posts the BeginMainFrame. Maybe the
LostContext callback was previously posted and handled in between? Then we
should have told the scheduler it was lost. We went through
ProcessScheduledActions() so it should see that the surface is lost.

But it does a commit, so is that a scheduler bug? (I kinda doubt it but hmmm) Or
maybe NotifyReadyToCommit is used differently?

[enne (OOO), 2014-08-07 20:39:10]

LayoutTests should be fixed with https://codereview.chromium.org/453523002.  It
was hitting an assert that had changed only recently.

Still looking into the scheduler / UpdateLayers issue.

[enne (OOO), 2014-08-09 00:04:43]

danakj: r? cq?

https://codereview.chromium.org/134623005/diff/2180001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest.cc (left):

https://codereview.chromium.org/134623005/diff/2180001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest.cc:697: class
LayerTreeHostTestAbortFrameWhenInvisible : public LayerTreeHostTest {
This test is redundant with
LayerTreeHostTestBreakSwapPromiseForVisibilityAbortedCommit, which doesn't use
EndTestAfterDelay(1) and a flaky check and instead uses swap promises to verify
commits are explicitly aborted.

https://codereview.chromium.org/134623005/diff/2180001/cc/trees/single_thread...
File cc/trees/single_thread_proxy.cc (right):

https://codereview.chromium.org/134623005/diff/2180001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:282: BeginMainFrame();
This had to change to have the same swap promise behavior as non-deferred
commits.

https://codereview.chromium.org/134623005/diff/2180001/cc/trees/single_thread...
cc/trees/single_thread_proxy.cc:595: if
(layer_tree_host_->output_surface_lost()) {
Early-outing on the lost output surface here prevents the UpdateLayers crash
asserting on !output_surface_lost.

[danakj, 2014-08-10 12:36:09]

https://codereview.chromium.org/134623005/diff/2180001/cc/BUILD.gn
File cc/BUILD.gn (right):

https://codereview.chromium.org/134623005/diff/2180001/cc/BUILD.gn#newcode454
cc/BUILD.gn:454: "trees/scoped_swap_promise_checker.h",
I don't see this file.

[danakj, 2014-08-10 12:44:42]

On 2014/08/10 12:36:09, danakj wrote:
> https://codereview.chromium.org/134623005/diff/2180001/cc/BUILD.gn
> File cc/BUILD.gn (right):
> 
> https://codereview.chromium.org/134623005/diff/2180001/cc/BUILD.gn#newcode454
> cc/BUILD.gn:454: "trees/scoped_swap_promise_checker.h",
> I don't see this file.

R+ LGTM Once you add the file. Since this class is becoming a first class
citizen consider a name that captures more of the comment above its use in
ThreadProxy. ScopedAbortRemainingSwapPromises?

[enne (OOO), 2014-08-11 17:12:53]

On 2014/08/10 at 12:44:42, danakj wrote:

> R+ LGTM Once you add the file. Since this class is becoming a first class
citizen consider a name that captures more of the comment above its use in
ThreadProxy. ScopedAbortRemainingSwapPromises?

Done.

[enne (OOO), 2014-08-11 17:13:17]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-08-11 17:15:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/134623005/2210001

[commit-bot: I haz the power, 2014-08-11 20:02:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-11 20:08:38]

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

[commit-bot: I haz the power, 2014-08-11 20:08:42]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[enne (OOO), 2014-08-11 20:55:49]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-08-11 20:57:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/134623005/2210001

[commit-bot: I haz the power, 2014-08-11 21:05:02]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-11 21:12:42]

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

[commit-bot: I haz the power, 2014-08-11 21:12:45]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[enne (OOO), 2014-08-11 22:05:02]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-08-11 22:06:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/134623005/2230001

[commit-bot: I haz the power, 2014-08-12 01:43:24]

Change committed as 288866

[danakj, 2014-08-13 18:30:20]

https://codereview.chromium.org/134623005/diff/2230001/content/renderer/rende...
File content/renderer/render_widget.cc (right):

https://codereview.chromium.org/134623005/diff/2230001/content/renderer/rende...
content/renderer/render_widget.cc:396: host_closing_(false),
This var could exist #if DCHECK_IS_ON

https://codereview.chromium.org/134623005/diff/2230001/content/renderer/rende...
content/renderer/render_widget.cc:1217: webwidget_->willCloseLayerTreeView();
Ok so my super deep (ha ha haahha) understanding of this is that JS can request
a WebWidget to close by calling closeWidgetSoon on it's client (here). Then
we'll post a DoDeferredClose which will call this method.

But at the same time the user might close the window from the UI, shutting down
the RenderViewHostImpl, which will end up in RVHI::Shutdown->RWHI::Shutdown
which sends Close to the RenderWidget. Then DestroyLayerTreeView can't call
willCloseLayerTreeView cuz webwidget_ has been closed and destroyed already. So
you need to check this variable for null.

[enne (OOO), 2014-08-22 00:48:54]

Ok, round 6 of this patch.  For sanity reasons, I've removed all of the changes
to ui::Compositor and have only modified SingleThreadProxy and most unit tests
to use the scheduler.  Once I've fixed the five or eight bugs that are caused by
ui::Compositor using the scheduler, I'll land that change separately.

This conflicts with https://codereview.chromium.org/478703002/, so that needs to
be sorted out too.

[danakj, 2014-08-22 00:56:39]

WOO! LGTM!

[dneto, 2014-08-22 13:44:34]

On 2014/08/22 00:56:39, danakj wrote:
> WOO! LGTM!

Please go ahead and don't worry about the conflict with
https://codereview.chromium.org/478703002/ 
I'll sort out the conflicts after yours lands.  Besides, it has a bot failure I
still have to resolve.

And congratulations. :-)

[enne (OOO), 2014-08-22 17:03:29]

The CQ bit was checked by enne@chromium.org

[commit-bot: I haz the power, 2014-08-22 17:04:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/enne@chromium.org/134623005/2290001

[commit-bot: I haz the power, 2014-08-22 18:06:55]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[commit-bot: I haz the power, 2014-08-22 18:16:38]

Committed patchset #76 (2290001) as 291451