Make displayAsyncThen use the CompositeAndReadbackAsync path.

Make displayAsyncThen use the CompositeAndReadbackAsync path.

This avoids the use of the deprecated synchronous CompositeAndReadback
when in composited mode.

Depends on: https://codereview.chromium.org/272683003/
Depends on: https://codereview.chromium.org/262323002/

R=abarth@chromium.org, enne@chromium.org, piman@chromium.org, enne, piman
BUG=370130
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=269338

[danakj, 2014-05-05 18:35:16]

https://codereview.chromium.org/266243002/diff/40001/content/shell/renderer/t...
File content/shell/renderer/test_runner/WebTestProxy.cpp (right):

https://codereview.chromium.org/266243002/diff/40001/content/shell/renderer/t...
content/shell/renderer/test_runner/WebTestProxy.cpp:500: if
(!webWidget()->isAcceleratedCompositingActive()) {
This was the only way that compositeAndReadbackAsync could return false. I'm
going to remove the bool return value and assert instead in it in a blink CL.

https://codereview.chromium.org/266243002/diff/40001/content/shell/renderer/t...
content/shell/renderer/test_runner/WebTestProxy.cpp:638:
m_compositeAndReadbackCallback = base::Bind(
I could call CapturePixelsAsync here instead of setting the callback and calling
compositeAndReadbackAsync directly, but then we'd do yet another layout(). wdyt?

[danakj, 2014-05-05 19:30:27]

This appears to break:

virtual/deferred/inspector/timeline/timeline-paint.html
inspector/timeline/timeline-paint.html
media/track/opera/interfaces/TextTrackList/onaddtrack.html
virtual/deferred/inspector/timeline/timeline-decode-resize.html

I wonder why.

[enne (OOO), 2014-05-05 19:55:12]

Those tests are all the ones using displayAsyncThen.

Re:
https://storage.googleapis.com/chromium-layout-test-archives/win_blink_rel/62...

---
E:\b\build\slave\win_layout\build\layout-test-results\virtual/deferred/inspector/timeline/timeline-paint-expected.txt
+++
E:\b\build\slave\win_layout\build\layout-test-results\virtual/deferred/inspector/timeline/timeline-paint-actual.txt
@@ -11,6 +11,7 @@
     }
     endTime : <number>
     frameId : <string>
+    stackTrace : <object>
     startTime : <number>
     type : "Paint"
 }

pfeldman mentioned to me previously that the stackTrace only sometimes is there,
depending on when the paint call happens.  This may just be a rebaseline
(although the crashing tests are a different story).

https://codereview.chromium.org/266243002/diff/40001/content/shell/renderer/t...
File content/shell/renderer/test_runner/WebTestProxy.cpp (right):

https://codereview.chromium.org/266243002/diff/40001/content/shell/renderer/t...
content/shell/renderer/test_runner/WebTestProxy.cpp:638:
m_compositeAndReadbackCallback = base::Bind(
On 2014/05/05 18:35:17, danakj wrote:
> I could call CapturePixelsAsync here instead of setting the callback and
calling
> compositeAndReadbackAsync directly, but then we'd do yet another layout().
wdyt?

Yeah, I think that'd be cleaner.  Can you just move the layout call inside the
conditional?

A second layout shouldn't do anything wrong, I don't think.

[danakj, 2014-05-05 19:58:40]

On 2014/05/05 19:55:12, enne wrote:
> Those tests are all the ones using displayAsyncThen.
> 
> Re:
>
https://storage.googleapis.com/chromium-layout-test-archives/win_blink_rel/62...
> 
> ---
>
E:\b\build\slave\win_layout\build\layout-test-results\virtual/deferred/inspector/timeline/timeline-paint-expected.txt
> +++
>
E:\b\build\slave\win_layout\build\layout-test-results\virtual/deferred/inspector/timeline/timeline-paint-actual.txt
> @@ -11,6 +11,7 @@
>      }
>      endTime : <number>
>      frameId : <string>
> +    stackTrace : <object>
>      startTime : <number>
>      type : "Paint"
>  }
> 
> pfeldman mentioned to me previously that the stackTrace only sometimes is
there,
> depending on when the paint call happens.  This may just be a rebaseline
> (although the crashing tests are a different story).

For inspector/timeline/timeline-paint.html I had removed your post-task for
software mode (oops).

But for the virtual/deferred/ FCM tests, I guess this is a rebase.. it means
we're getting to the paint via js now instead of getting there directly from
WebTestProxyBase?

Which crashes did you see?

> 
>
https://codereview.chromium.org/266243002/diff/40001/content/shell/renderer/t...
> File content/shell/renderer/test_runner/WebTestProxy.cpp (right):
> 
>
https://codereview.chromium.org/266243002/diff/40001/content/shell/renderer/t...
> content/shell/renderer/test_runner/WebTestProxy.cpp:638:
> m_compositeAndReadbackCallback = base::Bind(
> On 2014/05/05 18:35:17, danakj wrote:
> > I could call CapturePixelsAsync here instead of setting the callback and
> calling
> > compositeAndReadbackAsync directly, but then we'd do yet another layout().
> wdyt?
> 
> Yeah, I think that'd be cleaner.  Can you just move the layout call inside the
> conditional?
> 
> A second layout shouldn't do anything wrong, I don't think.

The conditional can change based on the layout above it :/ That's the problem
there.

[danakj, 2014-05-05 20:01:26]

Oh virtual/deferred/inspector/timeline/timeline-decode-resize.html is crashing,
I'll take a look.

[danakj, 2014-05-05 21:43:06]

PTAL. The only failures should be rebaselines now.

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

lgtm, not that I'm an owner

[danakj, 2014-05-05 22:04:58]

thanks!

piman: OWNERS please

[abarth-chromium, 2014-05-05 22:37:42]

https://codereview.chromium.org/266243002/diff/70001/content/shell/renderer/t...
File content/shell/renderer/test_runner/WebTestProxy.cpp (right):

https://codereview.chromium.org/266243002/diff/70001/content/shell/renderer/t...
content/shell/renderer/test_runner/WebTestProxy.cpp:629: webWidget()->layout();
Will this change what we're testing by introducing another call to layout?  What
if the compositing mode changes while the callback is in flight?

[danakj, 2014-05-05 22:46:55]

https://codereview.chromium.org/266243002/diff/70001/content/shell/renderer/t...
File content/shell/renderer/test_runner/WebTestProxy.cpp (right):

https://codereview.chromium.org/266243002/diff/70001/content/shell/renderer/t...
content/shell/renderer/test_runner/WebTestProxy.cpp:629: webWidget()->layout();
On 2014/05/05 22:37:43, abarth wrote:
> Will this change what we're testing by introducing another call to layout?

This does add one additional layout.

> What if the compositing mode changes while the callback is in flight?

If it leaves during the c&RAsync, and destroys the compositor, the callback will
return here, and we'll run the |callback| passed in here. I could have it run
the software display() in that case instead?

[danakj, 2014-05-05 22:51:31]

On 2014/05/05 22:46:55, danakj wrote:
>
https://codereview.chromium.org/266243002/diff/70001/content/shell/renderer/t...
> File content/shell/renderer/test_runner/WebTestProxy.cpp (right):
> 
>
https://codereview.chromium.org/266243002/diff/70001/content/shell/renderer/t...
> content/shell/renderer/test_runner/WebTestProxy.cpp:629:
webWidget()->layout();
> On 2014/05/05 22:37:43, abarth wrote:
> > Will this change what we're testing by introducing another call to layout?
> 
> This does add one additional layout.

FWIW it should still paint only once, though. I can't really comment on if this
is okay or not. If not, it'll have to wait for FCM.

> > What if the compositing mode changes while the callback is in flight?
> 
> If it leaves during the c&RAsync, and destroys the compositor, the callback
will
> return here, and we'll run the |callback| passed in here. I could have it run
> the software display() in that case instead?

I've uploaded a patch with this change, what to you think?

[abarth-chromium, 2014-05-05 22:53:04]

On 2014/05/05 22:46:55, danakj wrote:
>
https://codereview.chromium.org/266243002/diff/70001/content/shell/renderer/t...
> File content/shell/renderer/test_runner/WebTestProxy.cpp (right):
> 
>
https://codereview.chromium.org/266243002/diff/70001/content/shell/renderer/t...
> content/shell/renderer/test_runner/WebTestProxy.cpp:629:
webWidget()->layout();
> On 2014/05/05 22:37:43, abarth wrote:
> > Will this change what we're testing by introducing another call to layout?
> 
> This does add one additional layout.

Could that mask bugs that otherwise would have been caught by a test?

> > What if the compositing mode changes while the callback is in flight?
> 
> If it leaves during the c&RAsync, and destroys the compositor, the callback
will
> return here, and we'll run the |callback| passed in here. I could have it run
> the software display() in that case instead?

I haven't studied the details, so I'm not sure what to recommend.  It's possible
what you've got is already the best thing.

[danakj, 2014-05-05 22:59:35]

On Mon, May 5, 2014 at 6:53 PM, <abarth@chromium.org> wrote:

> On 2014/05/05 22:46:55, danakj wrote:
>
> https://codereview.chromium.org/266243002/diff/70001/
> content/shell/renderer/test_runner/WebTestProxy.cpp
>
>> File content/shell/renderer/test_runner/WebTestProxy.cpp (right):
>>
>
>
> https://codereview.chromium.org/266243002/diff/70001/
> content/shell/renderer/test_runner/WebTestProxy.cpp#newcode629
>
>> content/shell/renderer/test_runner/WebTestProxy.cpp:629:
>>
> webWidget()->layout();
>
>> On 2014/05/05 22:37:43, abarth wrote:
>> > Will this change what we're testing by introducing another call to
>> layout?
>>
>
>  This does add one additional layout.
>>
>
> Could that mask bugs that otherwise would have been caught by a test?


In theory I think perhaps it could, though the extra layout will go away
once we have FCM and then I'll re-expose them. ^^;


>
>
>  > What if the compositing mode changes while the callback is in flight?
>>
>
>  If it leaves during the c&RAsync, and destroys the compositor, the
>> callback
>>
> will
>
>> return here, and we'll run the |callback| passed in here. I could have it
>> run
>> the software display() in that case instead?
>>
>
> I haven't studied the details, so I'm not sure what to recommend.  It's
> possible
> what you've got is already the best thing.


A test that does displayAsyncThen(foo); changeStyleToLeaveCompositing();
sounds really weird to me, so I'm not sure if we need to worry about it.
This also goes away with FCM.

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

[danakj, 2014-05-06 18:59:00]

Since this may not be the best course of action I've pulled out the stuff to
make compositeAndReadbackAsync return void over here:
https://codereview.chromium.org/270463003

And I'm currently investigating to make tests unable to leave compositing mode
once entering, which would allow us to remove the extra layout() and the
fallback paths in here.

[abarth-chromium, 2014-05-07 05:30:27]

On 2014/05/06 18:59:00, danakj wrote:
> And I'm currently investigating to make tests unable to leave compositing mode
> once entering, which would allow us to remove the extra layout() and the
> fallback paths in here.

Oh, that's a good idea.

[danakj, 2014-05-07 22:55:48]

On 2014/05/07 05:30:27, abarth wrote:
> On 2014/05/06 18:59:00, danakj wrote:
> > And I'm currently investigating to make tests unable to leave compositing
mode
> > once entering, which would allow us to remove the extra layout() and the
> > fallback paths in here.
> 
> Oh, that's a good idea.

Solved this with https://codereview.chromium.org/262323002/ and rebaselined on
top of it.

PTAL Adam

[abarth-chromium, 2014-05-08 12:58:15]

Much nicer.  Thanks!

LGTM

[piman, 2014-05-08 18:08:32]

LGTM - sorry, I thought I did that one already.

[piman, 2014-05-08 18:08:43]

The CQ bit was checked by piman@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/danakj@chromium.org/266243002/130001

[danakj, 2014-05-08 18:14:25]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2014-05-08 18:14:40]

On 2014/05/08 18:08:32, piman wrote:
> LGTM - sorry, I thought I did that one already.

No worries, I'm waiting on https://codereview.chromium.org/272683003/ to land
before this.

[danakj, 2014-05-09 16:37:29]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-05-09 16:43:06]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/danakj@chromium.org/266243002/130001

[danakj, 2014-05-09 16:52:33]

Committed patchset #9 manually as r269338 (presubmit successful).