If a compositing update is declared as needed, scheduleAnimation should be triggered.

If a compositing update is declared as needed, scheduleAnimation should be triggered.

This fixes some issues where deferred compositing on aura was
causing things to not be rendered because frames were not
being scheduled correctly.

BUG=336676
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=166969

[shawnsingh, 2014-02-04 07:23:54]

Dear reviewers, PTAL this is time-sensitive m33 release blocker.  If people
still insist that we need a better test for this, then we need to create a
separate bug for it, because it will be a bigger task (explanation follows). 
But this fix cannot wait any longer.

testing woes:

I tried many things all day long to get a more ideal rigorous test to ensure
that scheduleAnimation() gets called from setNeedsCompositingUpdate().

After several dead ends, I've concluded that there is no ideal way to get
scheduleAnimation() called without actually also initiating the full compositor
update itself.  Because of this, there's no point in plumbing
"animate_requested" logic from the chromium side, because that boolean will
become true and reset to false all before javascript has a chance to even check
the value.  The same issue also applies to the blink side - there's no way to
"reset" the boolean in an intuitive way without making it impossible to test
from javascript.  Thus, the patch I have has the quirky "reset, notify, and
access" paradigm - not ideal, but I think it's the only reasonable option
without ridiculously heavy refactors.

And then to make matters more moot - there is no known repro that requires ONLY
the scheduleAnimation calls from setNeedsCompositingUpdate() anymore.   I
believe that abarth's cleanup to remove the styleRecalcTimer also fixed the
repros for this bugfix by issuing scheduleAnimations from style recalc at the
right time.  But still, we all agree that scheduleAnimation is correct in
setNeedsCompositingUpdate.

So yeah.

[jamesr, 2014-02-05 01:37:12]

The code change in RenderLayerCompositor.cpp / RenderImage.cpp lgtm, but the
test changes are IMHO more harmful than helpful.  I think a successful test
would not involve any of the Page/Chrome/etc tests and instead do the following:

1.) Set up a page in an idle state i.e. by loading up, waiting for a RAF
callback, then setting a timeout to run after the RAF
2.) Make the change to the DOM that should trigger a compositing update
3.) Call a window.internals function that queries if we *actually* have a frame
scheduled by querying the real compositor, not querying a fake value stored in
blink.
4.) If we need to repeat, proceed to an idle state by waiting for a RAF
callback/etc until we're back in an idle state.

Trying to store a fake animation pending state and resetting it is wasteful in
production code and doesn't provide good test coverage of the actual system
since it does not proceed through the same state machine that the real
compositing update system goes through.  Going through the real state machine
really isn't all that hard - it's what the code does all the time.

I believe you'll have to add a new trivial blink API entry point to implement
the window.internals call in step 3.  Please do that and do not land the rest of
this code and, if you need to, land the bugfix separately so it can be merged.

[shawnsingh, 2014-02-05 03:49:24]

On 2014/02/05 01:37:12, jamesr wrote:
> 3.) Call a window.internals function that queries if we *actually* have a
frame
> scheduled by querying the real compositor, not querying a fake value stored in
> blink.

Sorry my first message was not clear.

I did implement this originally - your step #3 is exactly what I'm saying is not
possible without major changes.  scheduleAnimation() triggers the actual
compositor frame so immediately that javascript doesn't re-enter before the
frame totally finishes.  By the time javascript can check the flag, it is
already be reset to false again.  The only way around this is to add a new
boolean whose semantics are to latch to true on scheduleAnimation(), but remain
true until we explicitly reset it... which is what this patch does.  And I agree
its not very satisfactory way to test this.

As a side note, I also tried this with the virtual/threaded/ tests to use the
multithreaded proxy, and saw the same issue.  Worse, I imagine the multithreaded
proxy would be flaky about this, too.  If you know of a way to control impl-side
tasks from blink layout tests, that might actually be a very elegant way to do
this test properly.  But it would certainly warrant a separate patch for that
infrastructure.

Do you have a preference how to proceed?

[jamesr, 2014-02-05 20:34:39]

On 2014/02/05 03:49:24, shawnsingh wrote:
> On 2014/02/05 01:37:12, jamesr wrote:
> > 3.) Call a window.internals function that queries if we *actually* have a
> frame
> > scheduled by querying the real compositor, not querying a fake value stored
in
> > blink.
> 
> Sorry my first message was not clear.
> 
> I did implement this originally - your step #3 is exactly what I'm saying is
not
> possible without major changes.  scheduleAnimation() triggers the actual
> compositor frame so immediately that javascript doesn't re-enter before the
> frame totally finishes.

I don't follow.  In script, you want to do something like:

  container.appendChild(canvasElement);
  if (window.internals.compositorFrameIsPending())
    // PASS!
  else
    // FAIL!

why would you need to run additional script to run?  Either the appendChild
causes us to recognize that we need a compositor update or it doesn't.  That's
the bug we're trying to test, right?

>  By the time javascript can check the flag, it is
> already be reset to false again.  The only way around this is to add a new
> boolean whose semantics are to latch to true on scheduleAnimation(), but
remain
> true until we explicitly reset it... which is what this patch does.  And I
agree
> its not very satisfactory way to test this.

No, it isn't - but that isn't what you want to do.  You want to query if we
*actually* have a compositor update scheduled, not to have an extra flag.  The
extra flag you're adding is the problem.  Just ask the compositor if it is
really going to call you back or not.

[shawnsingh, 2014-02-10 18:24:17]

On 2014/02/05 20:34:39, jamesr wrote:
> On 2014/02/05 03:49:24, shawnsingh wrote:
> > On 2014/02/05 01:37:12, jamesr wrote:
> > > 3.) Call a window.internals function that queries if we *actually* have a
> > frame
> > > scheduled by querying the real compositor, not querying a fake value
stored
> in
> > > blink.
> > 
> > Sorry my first message was not clear.
> > 
> > I did implement this originally - your step #3 is exactly what I'm saying is
> not
> > possible without major changes.  scheduleAnimation() triggers the actual
> > compositor frame so immediately that javascript doesn't re-enter before the
> > frame totally finishes.
> 
> I don't follow.  In script, you want to do something like:
> 
>   container.appendChild(canvasElement);
>   if (window.internals.compositorFrameIsPending())
>     // PASS!
>   else
>     // FAIL!
> 

Yeah I understood this already.  The existing boolean flag you have in mind is
something like LTH::BeginMainFrameRequested().  After further debugging it
turned out that SingleThreadProxy and threaded proxy have different semantics
for their implementation of LayerTreeHost::BeginMainFrameRequested().  The
single thread proxy always returns false and doesn't really have a boolean
variable with the semantics we want.  So I will create it as part of the
plumbing for testing.

[jamesr, 2014-02-10 18:44:34]

The single thread proxy doesn't do scheduling, so it wouldn't have a bit like
this.  content::RenderWidget does the scheduling in single threaded mode, so it
would answer this question.

[shawnsingh, 2014-02-11 01:40:12]

On 2014/02/10 18:44:34, jamesr wrote:
> The single thread proxy doesn't do scheduling, so it wouldn't have a bit like
> this.  content::RenderWidget does the scheduling in single threaded mode, so
it
> would answer this question.

New patch uploaded - PTAL at the plumbing.  
https://codereview.chromium.org/159493002/ should land first, then this can land
seamlessly.  When merging, I'll actually just merge the two scheduleAnimation()
lines and nothing else.

[jamesr, 2014-02-11 01:50:13]

lgtm

https://codereview.chromium.org/139103009/diff/220001/Source/core/rendering/R...
File Source/core/rendering/RenderLayerCompositor.cpp (right):

https://codereview.chromium.org/139103009/diff/220001/Source/core/rendering/R...
Source/core/rendering/RenderLayerCompositor.cpp:299: // FIXME: this code needs
to be carefully merged with setNeedsCompositingUpdate.
I don't really understand what this comment is trying to convey.  Is there a bug
on this issue? Could you just have a reference to that bug here?

[shawnsingh, 2014-02-11 02:01:30]

On 2014/02/11 01:50:13, jamesr wrote:
> lgtm
> 
>
https://codereview.chromium.org/139103009/diff/220001/Source/core/rendering/R...
> File Source/core/rendering/RenderLayerCompositor.cpp (right):
> 
>
https://codereview.chromium.org/139103009/diff/220001/Source/core/rendering/R...
> Source/core/rendering/RenderLayerCompositor.cpp:299: // FIXME: this code needs
> to be carefully merged with setNeedsCompositingUpdate.
> I don't really understand what this comment is trying to convey.  Is there a
bug
> on this issue? Could you just have a reference to that bug here?

Done - and thanks for your patience reviewing this.

[shawnsingh, 2014-02-12 00:26:02]

The CQ bit was checked by shawnsingh@chromium.org

[commit-bot: I haz the power, 2014-02-12 01:58:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shawnsingh@chromium.org/139103009/270001

[commit-bot: I haz the power, 2014-02-12 07:33:25]

Change committed as 166969