aura: Remove old GL paths from RenderWidgetHostViewAura.

aura: Remove old GL paths from RenderWidgetHostViewAura.

Frames from the renderer are either legacy software,
software compositing, or ubercomp. Remove the old GL composited
paths that are no longer in use.

For layout tests, put them into composited mailbox mode, and
instead of sending the frame to the browser at all, just put the
frame in a post task to ack back directly to the renderer.

R=piman
BUG=332998
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249226

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258742

[danakj, 2014-01-09 22:07:03]

This has https://codereview.chromium.org/132473004/ inside it for the trybots.
Only RWHVA changes are this CL alone.

[danakj, 2014-01-09 22:52:25]

aboutflag

[danakj, 2014-01-09 22:52:51]

Removed the about:flags entry for delegated rendering on aura as well.

[piman, 2014-01-09 23:16:41]

On 2014/01/09 22:52:51, danakj wrote:
> Removed the about:flags entry for delegated rendering on aura as well.

We should also remove FCM and thread, since if they're disabled we don't do
delegated rendering.

[piman, 2014-01-09 23:24:18]

https://codereview.chromium.org/131443007/diff/280001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/131443007/diff/280001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1225:
NOTREACHED();
nit: Technically, a malicious renderer could swap on the "view" context which
would end up reaching here.

https://codereview.chromium.org/131443007/diff/280001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1491:
DCHECK(frame->software_frame_data);
Not DCHECK since a malicious renderer can still send a GLFrameData. We can just
ignore the message then.

(isn't this hit by the unit test?)

https://codereview.chromium.org/131443007/diff/280001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:1516:
NOTREACHED();
nit: could be reached too.

[danakj, 2014-01-10 00:34:14]

OK I removed yet more code and stopped using NOTREACHED/DCHECK as requested.
PTAL

https://codereview.chromium.org/131443007/diff/360001/chrome/browser/about_fl...
File chrome/browser/about_flags.cc (right):

https://codereview.chromium.org/131443007/diff/360001/chrome/browser/about_fl...
chrome/browser/about_flags.cc:422: kOsLinux |
Removed Thread and FCM from linux-aura. It wasn't available for other auras
already.

[piman, 2014-01-10 01:45:08]

lgtm

https://codereview.chromium.org/131443007/diff/280001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/131443007/diff/280001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2367:
DCHECK(old_mailbox.IsSharedMemory());
nit: I think that code was still fine, since this is the mailbox we gave to the
layer, it's always a shared memory.

[danakj, 2014-01-10 17:51:41]

https://codereview.chromium.org/131443007/diff/280001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/131443007/diff/280001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2367:
DCHECK(old_mailbox.IsSharedMemory());
On 2014/01/10 01:45:09, piman wrote:
> nit: I think that code was still fine, since this is the mailbox we gave to
the
> layer, it's always a shared memory.

You mean the if (IsSharedMemory)? I was thinking to just have a DCHECK that we
never get here with a texture backed TextureMailbox - which we shouldn't of
course.

[piman, 2014-01-10 19:47:32]

On Fri, Jan 10, 2014 at 9:51 AM, <danakj@chromium.org> wrote:

>
> https://codereview.chromium.org/131443007/diff/280001/
> content/browser/renderer_host/render_widget_host_view_aura.cc
> File content/browser/renderer_host/render_widget_host_view_aura.cc
> (right):
>
> https://codereview.chromium.org/131443007/diff/280001/
> content/browser/renderer_host/render_widget_host_view_aura.cc#newcode2367
> content/browser/renderer_host/render_widget_host_view_aura.cc:2367:
> DCHECK(old_mailbox.IsSharedMemory());
> On 2014/01/10 01:45:09, piman wrote:
>
>> nit: I think that code was still fine, since this is the mailbox we
>>
> gave to the
>
>> layer, it's always a shared memory.
>>
>
> You mean the if (IsSharedMemory)? I was thinking to just have a DCHECK
> that we never get here with a texture backed TextureMailbox - which we
> shouldn't of course.
>

Sure, either way.


>
> https://codereview.chromium.org/131443007/
>

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

[danakj, 2014-02-03 20:37:30]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-02-03 20:38:48]

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

[danakj, 2014-02-03 20:39:14]

The CQ bit was unchecked by danakj@chromium.org

[commit-bot: I haz the power, 2014-02-03 20:39:19]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-03 20:39:22]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-03 20:40:39]

CQ bit was unchecked on CL. Ignoring.

[danakj, 2014-02-03 21:53:39]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-02-03 21:57:15]

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

[danakj, 2014-02-03 23:06:17]

The CQ bit was checked by danakj@chromium.org

[danakj, 2014-02-03 23:09:19]

The CQ bit was unchecked by danakj@chromium.org

[commit-bot: I haz the power, 2014-02-03 23:09:21]

CQ bit was unchecked on CL. Ignoring.

[commit-bot: I haz the power, 2014-02-03 23:09:23]

CQ bit was unchecked on CL. Ignoring.

[danakj, 2014-02-03 23:09:38]

fsamual: please review chrome/browser/apps/web_view_browsertest.cc

[commit-bot: I haz the power, 2014-02-03 23:11:10]

CQ bit was unchecked on CL. Ignoring.

[Fady Samuel, 2014-02-03 23:11:34]

chrome/browser/apps/web_view_browsertest.cc LGTM

[danakj, 2014-02-03 23:22:07]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-02-03 23:26:11]

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

[commit-bot: I haz the power, 2014-02-04 04:16:25]

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

[commit-bot: I haz the power, 2014-02-04 04:16:26]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[piman, 2014-02-04 04:53:20]

You can get rid of AutoSizeSW. We really don't want it.


On Mon, Feb 3, 2014 at 8:16 PM, <commit-bot@chromium.org> wrote:

> Retried try job too often on linux_aura for step(s) browser_tests
> http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_
> aura&number=117668
>
> https://codereview.chromium.org/131443007/
>

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

[danakj, 2014-02-04 18:18:21]

Done, disabled on aura.

[danakj, 2014-02-04 18:18:26]

The CQ bit was checked by danakj@chromium.org

[danakj, 2014-02-04 18:19:08]

The CQ bit was checked by danakj@chromium.org

[danakj, 2014-02-04 18:19:40]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-02-04 18:25:19]

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

[commit-bot: I haz the power, 2014-02-04 19:36:38]

Retried try job too often on win_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-02-04 19:39:37]

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

[commit-bot: I haz the power, 2014-02-05 00:40:19]

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

[Paweł Hajdan Jr., 2014-02-05 18:51:12]

The CQ bit was unchecked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2014-02-05 19:16:03]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-02-05 19:22:47]

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

[danakj, 2014-02-05 21:58:35]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-02-05 23:27:03]

Retried try job too often on ios_rel_device_ninja for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[commit-bot: I haz the power, 2014-02-05 23:27:23]

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

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

Change committed as 249226

[pdr., 2014-02-06 08:03:05]

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

The reason for reverting is: This patch is causing hundreds of layouttest
timouts in Blink (see: http://goo.gl/6tX16t). My investigation is at
crbug.com/341344. I'm going to roll this out for now to try to green the Blink
tree.

BUG=341344.

[danakj, 2014-02-06 23:48:36]

In my "/o\ look at all the ImageTransportSurface code" state, I uploaded a CL
that just drops/acks frames on the old BuffersSwapped path. Maybe it's worth it
to remove all this code..

I'm not sure how else to tackle this without diverting into replacing all the
repaint tests first.

Maybe James has some other ideas.. or.. what do you think of this?

[jamesr, 2014-02-07 05:30:11]

the 18->17 diff appears to have some unrelated changes in it - was there a
rebase?

Sending acks back in this path seems ok, but do we understand what change in
behavior this is causing in the renderer? maybe we could be more targetted. We
should at least guard the layout-test-only codepaths by the layout test command
line flag (kDumpRenderTree iirc)

[danakj, 2014-02-07 17:08:55]

On Fri, Feb 7, 2014 at 12:30 AM, <jamesr@chromium.org> wrote:

> the 18->17 diff appears to have some unrelated changes in it - was there a
> rebase?
>

ya.


>
> Sending acks back in this path seems ok, but do we understand what change
> in
> behavior this is causing in the renderer? maybe we could be more
> targetted. We
> should at least guard the layout-test-only codepaths by the layout test
> command
> line flag (kDumpRenderTree iirc)
>

oh thanks good to know about that flag. piman@ suggested making the
renderer use composite-to-mailbox and then skipping the browser entirely
and just acking the swaps in the renderer. I'm trying to make that work,
inside a flag (will use that one).


>
> https://codereview.chromium.org/131443007/
>

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

[danakj, 2014-02-07 20:00:55]

I made the compositing layout tests use composite-to-mailbox, and I made
CompositorOutputSurface not even swap to the browser and just post-task a swap
ack when in layout tests.

[jamesr, 2014-02-07 20:43:48]

The ps19->18 diff lgtm

https://codereview.chromium.org/131443007/diff/2390001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/131443007/diff/2390001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:1227: // Oldschool
composited mode is no longer supported.
should we NOTREACHED() this? why is this function still needed?

https://codereview.chromium.org/131443007/diff/2390001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:1517: // Oldschool
composited mode is no longer supported.
ditto

https://codereview.chromium.org/131443007/diff/2390001/content/renderer/rende...
File content/renderer/render_widget.cc (left):

https://codereview.chromium.org/131443007/diff/2390001/content/renderer/rende...
content/renderer/render_widget.cc:940: DCHECK(is_threaded_compositing_enabled_);
can you modify this to DCHECK that we're either in threaded compositing mode or
layout test mode with a link to the bug for layout tests?

[danakj, 2014-02-07 20:48:05]

https://codereview.chromium.org/131443007/diff/2390001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/131443007/diff/2390001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:1227: // Oldschool
composited mode is no longer supported.
On 2014/02/07 20:43:49, jamesr wrote:
> should we NOTREACHED() this? why is this function still needed?

It's a pure virtual method on RWHVPort, so we need to override it.

piman@ has requested we don't NOTREACHED() in code that a compromised renderer
could get to legitimately as it interferes with unit testing.

https://codereview.chromium.org/131443007/diff/2390001/content/renderer/rende...
File content/renderer/render_widget.cc (left):

https://codereview.chromium.org/131443007/diff/2390001/content/renderer/rende...
content/renderer/render_widget.cc:940: DCHECK(is_threaded_compositing_enabled_);
On 2014/02/07 20:43:49, jamesr wrote:
> can you modify this to DCHECK that we're either in threaded compositing mode
or
> layout test mode with a link to the bug for layout tests?

Done.

[danakj, 2014-02-07 20:49:02]

https://codereview.chromium.org/131443007/diff/2390001/content/browser/render...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/131443007/diff/2390001/content/browser/render...
content/browser/renderer_host/render_widget_host_view_aura.cc:1227: // Oldschool
composited mode is no longer supported.
On 2014/02/07 20:48:06, danakj wrote:
> On 2014/02/07 20:43:49, jamesr wrote:
> > should we NOTREACHED() this? why is this function still needed?
> 
> It's a pure virtual method on RWHVPort, so we need to override it.

I think we can remove this when GTK linux goes away and we remove all the
BuffersSwapped paths in RWHImpl etc..

[danakj, 2014-02-07 21:12:07]

Green linux_layout_rel, yay. Going to CQ.

[danakj, 2014-02-07 21:12:15]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-02-07 21:15:19]

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

[piman, 2014-02-07 21:51:23]

drive-by...

https://codereview.chromium.org/131443007/diff/2590002/content/renderer/gpu/c...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/131443007/diff/2590002/content/renderer/gpu/c...
content/renderer/gpu/compositor_output_surface.cc:145: base::Unretained(this),
Is this Unretained safe?

https://codereview.chromium.org/131443007/diff/2590002/content/renderer/gpu/c...
content/renderer/gpu/compositor_output_surface.cc:146: output_surface_id_,
nit: You're not using this AFAICT

[danakj, 2014-02-07 21:57:24]

The CQ bit was unchecked by danakj@chromium.org

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

https://codereview.chromium.org/131443007/diff/2590002/content/renderer/gpu/c...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/131443007/diff/2590002/content/renderer/gpu/c...
content/renderer/gpu/compositor_output_surface.cc:145: base::Unretained(this),
On 2014/02/07 21:51:24, piman wrote:
> Is this Unretained safe?

I *think* so, cuz I'm not seeing crashes on layout tests.. do you think I should
WeakPtr? I wasn't really sure.

https://codereview.chromium.org/131443007/diff/2590002/content/renderer/gpu/c...
content/renderer/gpu/compositor_output_surface.cc:146: output_surface_id_,
On 2014/02/07 21:51:24, piman wrote:
> nit: You're not using this AFAICT

Oh, ya. I was passing this to the OnSwapAck() but not doing that now.

[piman, 2014-02-07 23:07:16]

https://codereview.chromium.org/131443007/diff/2590002/content/renderer/gpu/c...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/131443007/diff/2590002/content/renderer/gpu/c...
content/renderer/gpu/compositor_output_surface.cc:145: base::Unretained(this),
On 2014/02/07 21:58:44, danakj wrote:
> On 2014/02/07 21:51:24, piman wrote:
> > Is this Unretained safe?
> 
> I *think* so, cuz I'm not seeing crashes on layout tests.. do you think I
should
> WeakPtr? I wasn't really sure.

I don't see anything that would guarantee it.
On shutdown, I don't see code that waits for all the swap acks before destroying
the LTHI which is what would destroy the OS (besides, in non-threaded mode, we
can't).

I think a WeakPtrFactory would be safer.

[danakj, 2014-02-08 00:55:31]

There's still a timeout happening on mac, and flaky timeouts on win/mac/linux. I
need to debug some more...

https://codereview.chromium.org/131443007/diff/2590002/content/renderer/gpu/c...
File content/renderer/gpu/compositor_output_surface.cc (right):

https://codereview.chromium.org/131443007/diff/2590002/content/renderer/gpu/c...
content/renderer/gpu/compositor_output_surface.cc:145: base::Unretained(this),
On 2014/02/07 23:07:17, piman wrote:
> On 2014/02/07 21:58:44, danakj wrote:
> > On 2014/02/07 21:51:24, piman wrote:
> > > Is this Unretained safe?
> > 
> > I *think* so, cuz I'm not seeing crashes on layout tests.. do you think I
> should
> > WeakPtr? I wasn't really sure.
> 
> I don't see anything that would guarantee it.
> On shutdown, I don't see code that waits for all the swap acks before
destroying
> the LTHI which is what would destroy the OS (besides, in non-threaded mode, we
> can't).
> 
> I think a WeakPtrFactory would be safer.

ok! done.

[piman, 2014-02-08 01:26:07]

lgtm

[danakj, 2014-03-21 22:46:54]

Looks like this is now passing layout tests! I blame all the things enne/jamesr
have been doing, hooray!

[piman, 2014-03-21 22:48:24]

On 2014/03/21 22:46:54, danakj wrote:
> Looks like this is now passing layout tests! I blame all the things
enne/jamesr
> have been doing, hooray!

Dooooo ittttttt

[danakj, 2014-03-21 22:48:53]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-03-21 22:49:31]

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

[commit-bot: I haz the power, 2014-03-22 02:51:09]

Change committed as 258742

[vsevik, 2014-03-24 08:41:59]

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

The reason for reverting is: This is causing many virtual/threaded/transitions
and virtual/threaded/animations tests to time out.
Some of them are timing out flakily, some are consistent.

Here is what I’ve seen so far:

virtual/threaded/transitions/move-after-transition.html - consistent time outs.

virtual/threaded/transitions/cubic-bezier-overflow-svg-length.html - flaky time
outs
virtual/threaded/transitions/change-duration-during-transition.html - flaky time
outs
virtual/threaded/animations/cross-fade-background-image.html - flaky
ImageOnlyFailure, looks like it is related..

[vsevik, 2014-03-24 09:03:04]

virtual/threaded/animations/compositor-start-event-timing.html is also timing
out on win debug bots now.

[vsevik, 2014-03-24 09:56:31]

and virtual/threaded/animations/display-change-does-not-terminate-animation.html

[vsevik, 2014-03-24 09:58:15]

and virtual/threaded/transitions/transition-on-element-with-content.html
and maybe virtual/threaded/transitions/transition-end-event-destroy-iframe.html

[danakj, 2014-03-24 15:59:14]

Argh. But they pass locally and on the trybots. Enne@ suggested maybe just
disabling these tests. Idk how else to land this right now.
On Mar 24, 2014 4:42 AM, <vsevik@chromium.org> wrote:

> A revert of this CL has been created in
> https://codereview.chromium.org/209903002/ by vsevik@chromium.org.
>
> The reason for reverting is: This is causing many
> virtual/threaded/transitions
> and virtual/threaded/animations tests to time out.
> Some of them are timing out flakily, some are consistent.
>
> Here is what I've seen so far:
>
> virtual/threaded/transitions/move-after-transition.html - consistent time
> outs.
>
> virtual/threaded/transitions/cubic-bezier-overflow-svg-length.html -
> flaky time
> outs
> virtual/threaded/transitions/change-duration-during-transition.html -
> flaky time
> outs
> virtual/threaded/animations/cross-fade-background-image.html - flaky
> ImageOnlyFailure, looks like it is related..
>
> https://codereview.chromium.org/131443007/
>

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

[piman, 2014-04-02 04:48:49]

On 2014/03/24 15:59:14, danakj wrote:
> Argh. But they pass locally and on the trybots. Enne@ suggested maybe just
> disabling these tests. Idk how else to land this right now.
> On Mar 24, 2014 4:42 AM, <mailto:vsevik@chromium.org> wrote:
> 
> > A revert of this CL has been created in
> > https://codereview.chromium.org/209903002/ by mailto:vsevik@chromium.org.
> >
> > The reason for reverting is: This is causing many
> > virtual/threaded/transitions
> > and virtual/threaded/animations tests to time out.
> > Some of them are timing out flakily, some are consistent.
> >
> > Here is what I've seen so far:
> >
> > virtual/threaded/transitions/move-after-transition.html - consistent time
> > outs.
> >
> > virtual/threaded/transitions/cubic-bezier-overflow-svg-length.html -
> > flaky time
> > outs
> > virtual/threaded/transitions/change-duration-during-transition.html -
> > flaky time
> > outs
> > virtual/threaded/animations/cross-fade-background-image.html - flaky
> > ImageOnlyFailure, looks like it is related..
> >
> > https://codereview.chromium.org/131443007/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

I think I can repro those failures on linux. I will try to take a look tomorrow.

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

Moved to https://codereview.chromium.org/225403008/ closing this.