DelegatedFrameHost should not allocate a new local surface id on frame eviction

Once we move local surface id allocation into the renderer process,
DelegatedFrameHost can no longer create a new local surface id on frame
eviction. This would normally break a few things but after 
https://codereview.chromium.org/2736053004/ we should be good.

I tried testing this CL by setting the maximum number of saved
frames to 1 so that each new tab causes the eviction of the previous tab
and compared the current behaviour vs behaviour after this CL and did
not notice any difference. Without https://codereview.chromium.org/2736053004/
if you switch tabs fast enough DCHECKs will fail in SurfaceManager for allocating
a surface using a surface id that is already in use, but that CL fixes
this issue.

TBR=jam@chromium.org
BUG=697864
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2731793002
Cr-Commit-Position: refs/heads/master@{#456180}
Committed: https://chromium.googlesource.com/chromium/src/+/c43562c581991f1d9b843cb3cec873723a009c31

[Saman Sami, 2017-03-04 00:08:43]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-04 00:09:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-04 00:35:43]

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

[commit-bot: I haz the power, 2017-03-04 00:35:44]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Saman Sami, 2017-03-06 23:58:23]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-06 23:59:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-03-07 00:00:43]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 00:01:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-03-07 01:09:54]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 01:10:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-03-07 01:10:48]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 01:11:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-03-07 01:13:47]

Description was changed from

==========
DelegatedFrameHost should not create new local surface id on eviction

BUG=697864
==========

to

==========
Currently, DelegatedFrameHost destroys its surface in EvictDelegatedFrame.
This means that after a new frame comes a new surface must be allocated.
This doesn't have to be the case. We can simply call ForceReclaimResources
in order to reclaim the resources while keeping the surface. This needs to
be done because we are moving the local surface id allocation out of 
DelegatedFrameHost into RendererCompositorFrameSink and therefore
DelegatedFrameHost must not interfere in local surface id allocation.

BUG=697864
==========

[Saman Sami, 2017-03-07 01:15:29]

Description was changed from

==========
Currently, DelegatedFrameHost destroys its surface in EvictDelegatedFrame.
This means that after a new frame comes a new surface must be allocated.
This doesn't have to be the case. We can simply call ForceReclaimResources
in order to reclaim the resources while keeping the surface. This needs to
be done because we are moving the local surface id allocation out of 
DelegatedFrameHost into RendererCompositorFrameSink and therefore
DelegatedFrameHost must not interfere in local surface id allocation.

BUG=697864
==========

to

==========
Currently, DelegatedFrameHost destroys its surface in EvictDelegatedFrame.
This means that after a new frame arrives a new surface must be created.
This doesn't have to be the case. We can simply call ForceReclaimResources
in order to reclaim the resources while keeping the surface intact.
This needs to be done because we are moving the local surface id allocation  
out of DelegatedFrameHost and into RendererCompositorFrameSink so
DelegatedFrameHost must not interfere in local surface id allocation.

BUG=697864
==========

[Saman Sami, 2017-03-07 01:20:30]

Description was changed from

==========
Currently, DelegatedFrameHost destroys its surface in EvictDelegatedFrame.
This means that after a new frame arrives a new surface must be created.
This doesn't have to be the case. We can simply call ForceReclaimResources
in order to reclaim the resources while keeping the surface intact.
This needs to be done because we are moving the local surface id allocation  
out of DelegatedFrameHost and into RendererCompositorFrameSink so
DelegatedFrameHost must not interfere in local surface id allocation.

BUG=697864
==========

to

==========
Currently, DelegatedFrameHost destroys its surface in EvictDelegatedFrame.
This means that after a new frame arrives a new surface must be created.
This doesn't have to be the case. We can simply call ForceReclaimResources
in order to reclaim the resources while keeping the surface intact.
This needs to be done because we are moving the local surface id allocation  
out of DelegatedFrameHost and into RendererCompositorFrameSink so
DelegatedFrameHost must not interfere in local surface id allocation.

I tried testing this change by setting the maximum number of saved
frames to 1 so that each new tab causes the eviction of the previous tab.
I did not notice any change in the behaviour compared to when we 
destroyed the surface.

BUG=697864
==========

[Saman Sami, 2017-03-07 01:21:10]

Description was changed from

==========
Currently, DelegatedFrameHost destroys its surface in EvictDelegatedFrame.
This means that after a new frame arrives a new surface must be created.
This doesn't have to be the case. We can simply call ForceReclaimResources
in order to reclaim the resources while keeping the surface intact.
This needs to be done because we are moving the local surface id allocation  
out of DelegatedFrameHost and into RendererCompositorFrameSink so
DelegatedFrameHost must not interfere in local surface id allocation.

I tried testing this change by setting the maximum number of saved
frames to 1 so that each new tab causes the eviction of the previous tab.
I did not notice any change in the behaviour compared to when we 
destroyed the surface.

BUG=697864
==========

to

==========
Currently, DelegatedFrameHost destroys its surface in EvictDelegatedFrame.
This means that after a new frame arrives a new surface must be created.
This doesn't have to be the case. We can simply call ForceReclaimResources
in order to reclaim the resources while keeping the surface intact.
This needs to be done because we are moving the local surface id allocation  
out of DelegatedFrameHost and into RendererCompositorFrameSink so
DelegatedFrameHost must not interfere in local surface id allocation.

I tried testing this change by setting the maximum number of saved
frames to 1 so that each new tab causes the eviction of the previous tab
and compared the current behaviour vs behaviour after this CL and did
not notice any difference.

BUG=697864
==========

[Saman Sami, 2017-03-07 01:22:13]

samans@chromium.org changed reviewers:
+ fsamuel@chromium.org, jbauman@chromium.org

[Saman Sami, 2017-03-07 01:22:14]

PTAL

[jbauman, 2017-03-07 01:25:40]

On 2017/03/07 01:22:14, Saman Sami wrote:
> PTAL

I'm surprised this doesn't cause a black flicker on tab switching if the
previous frame's contents are destroyed before the browser stops using them.

[Saman Sami, 2017-03-07 01:29:28]

On 2017/03/07 01:25:40, jbauman wrote:
> On 2017/03/07 01:22:14, Saman Sami wrote:
> > PTAL
> 
> I'm surprised this doesn't cause a black flicker on tab switching if the
> previous frame's contents are destroyed before the browser stops using them.

The page goes white for a few frames, but that happens even without this change.
Sorry I just realized resize seems to be broken even though tab eviction looks
good to me. I'll try to fix it.

[commit-bot: I haz the power, 2017-03-07 01:43:34]

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

[commit-bot: I haz the power, 2017-03-07 01:43:35]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Saman Sami, 2017-03-07 01:53:51]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 01:55:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-03-07 02:04:31]

On 2017/03/07 01:29:28, Saman Sami wrote:
> On 2017/03/07 01:25:40, jbauman wrote:
> > On 2017/03/07 01:22:14, Saman Sami wrote:
> > > PTAL
> > 
> > I'm surprised this doesn't cause a black flicker on tab switching if the
> > previous frame's contents are destroyed before the browser stops using them.
> 
> The page goes white for a few frames, but that happens even without this
change.
> Sorry I just realized resize seems to be broken even though tab eviction looks
> good to me. I'll try to fix it.

I fixed resize.

I think the behaviour is not different because it seems like we first switch to
the new tab and then evict the old tab so the surface that we evict is not being
shown and is immediately deleted.

[commit-bot: I haz the power, 2017-03-07 05:00:32]

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

[commit-bot: I haz the power, 2017-03-07 05:00:33]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Saman Sami, 2017-03-07 05:34:58]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 05:35:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 08:03:56]

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

[commit-bot: I haz the power, 2017-03-07 08:03:57]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[saman, 2017-03-07 13:55:25]

The CQ bit was checked by samans@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 13:55:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 15:07:15]

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

[commit-bot: I haz the power, 2017-03-07 15:07:17]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2017-03-07 15:16:55]

samans@chromium.org changed reviewers:
- samans@google.com

[Saman Sami, 2017-03-07 15:17:12]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-07 15:17:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-07 15:52:20]

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

[commit-bot: I haz the power, 2017-03-07 15:52:21]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[danakj, 2017-03-07 16:18:55]

On 2017/03/07 02:04:31, Saman Sami wrote:
> On 2017/03/07 01:29:28, Saman Sami wrote:
> > On 2017/03/07 01:25:40, jbauman wrote:
> > > On 2017/03/07 01:22:14, Saman Sami wrote:
> > > > PTAL
> > > 
> > > I'm surprised this doesn't cause a black flicker on tab switching if the
> > > previous frame's contents are destroyed before the browser stops using
them.
> > 
> > The page goes white for a few frames, but that happens even without this
> change.
> > Sorry I just realized resize seems to be broken even though tab eviction
looks
> > good to me. I'll try to fix it.
> 
> I fixed resize.

Did any tests catch what you broke? A quick look at bot logs makes me think not.
If not can you add one?

> I think the behaviour is not different because it seems like we first switch
to
> the new tab and then evict the old tab so the surface that we evict is not
being
> shown and is immediately deleted.

[Saman Sami, 2017-03-07 17:14:16]

On 2017/03/07 16:18:55, danakj wrote:
> On 2017/03/07 02:04:31, Saman Sami wrote:
> > On 2017/03/07 01:29:28, Saman Sami wrote:
> > > On 2017/03/07 01:25:40, jbauman wrote:
> > > > On 2017/03/07 01:22:14, Saman Sami wrote:
> > > > > PTAL
> > > > 
> > > > I'm surprised this doesn't cause a black flicker on tab switching if the
> > > > previous frame's contents are destroyed before the browser stops using
> them.
> > > 
> > > The page goes white for a few frames, but that happens even without this
> > change.
> > > Sorry I just realized resize seems to be broken even though tab eviction
> looks
> > > good to me. I'll try to fix it.
> > 
> > I fixed resize.
> 
> Did any tests catch what you broke? A quick look at bot logs makes me think
not.
> If not can you add one?

You can see in patch set 5 that a lot of tests failed actually.

[danakj, 2017-03-07 17:15:25]

On 2017/03/07 17:14:16, Saman Sami wrote:
> On 2017/03/07 16:18:55, danakj wrote:
> > On 2017/03/07 02:04:31, Saman Sami wrote:
> > > On 2017/03/07 01:29:28, Saman Sami wrote:
> > > > On 2017/03/07 01:25:40, jbauman wrote:
> > > > > On 2017/03/07 01:22:14, Saman Sami wrote:
> > > > > > PTAL
> > > > > 
> > > > > I'm surprised this doesn't cause a black flicker on tab switching if
the
> > > > > previous frame's contents are destroyed before the browser stops using
> > them.
> > > > 
> > > > The page goes white for a few frames, but that happens even without this
> > > change.
> > > > Sorry I just realized resize seems to be broken even though tab eviction
> > looks
> > > > good to me. I'll try to fix it.
> > > 
> > > I fixed resize.
> > 
> > Did any tests catch what you broke? A quick look at bot logs makes me think
> not.
> > If not can you add one?
> 
> You can see in patch set 5 that a lot of tests failed actually.

Yeh, they didn't look resize related but wasn't sure. Sounds like they were,
thanks!

[Saman Sami, 2017-03-08 00:36:25]

Patchset #7 (id:120001) has been deleted

[Saman Sami, 2017-03-08 01:08:25]

On 2017/03/07 17:15:25, danakj wrote:
> On 2017/03/07 17:14:16, Saman Sami wrote:
> > On 2017/03/07 16:18:55, danakj wrote:
> > > On 2017/03/07 02:04:31, Saman Sami wrote:
> > > > On 2017/03/07 01:29:28, Saman Sami wrote:
> > > > > On 2017/03/07 01:25:40, jbauman wrote:
> > > > > > On 2017/03/07 01:22:14, Saman Sami wrote:
> > > > > > > PTAL
> > > > > > 
> > > > > > I'm surprised this doesn't cause a black flicker on tab switching if
> the
> > > > > > previous frame's contents are destroyed before the browser stops
using
> > > them.
> > > > > 
> > > > > The page goes white for a few frames, but that happens even without
this
> > > > change.
> > > > > Sorry I just realized resize seems to be broken even though tab
eviction
> > > looks
> > > > > good to me. I'll try to fix it.
> > > > 
> > > > I fixed resize.
> > > 
> > > Did any tests catch what you broke? A quick look at bot logs makes me
think
> > not.
> > > If not can you add one?
> > 
> > You can see in patch set 5 that a lot of tests failed actually.
> 
> Yeh, they didn't look resize related but wasn't sure. Sounds like they were,
> thanks!

danakj@: Yes, the tests don't seem to be directly related to resize, but when
resize breaks so many things go wrong and some tests are gonna fail.

jbauman@: I was talking to Fady offline and he had a very good point. We call
SetShowSolidColorContent in EvictDelegatedFrame, which means the page will go
blank no matter what we do afterwards (force reclaim resources or evict
surface). The surface will also get destroyed immediately because there is no
one referencing it.

[jbauman, 2017-03-08 01:18:05]

On 2017/03/08 01:08:25, Saman Sami wrote:
> On 2017/03/07 17:15:25, danakj wrote:
> > On 2017/03/07 17:14:16, Saman Sami wrote:
> > > On 2017/03/07 16:18:55, danakj wrote:
> > > > On 2017/03/07 02:04:31, Saman Sami wrote:
> > > > > On 2017/03/07 01:29:28, Saman Sami wrote:
> > > > > > On 2017/03/07 01:25:40, jbauman wrote:
> > > > > > > On 2017/03/07 01:22:14, Saman Sami wrote:
> > > > > > > > PTAL
> > > > > > > 
> > > > > > > I'm surprised this doesn't cause a black flicker on tab switching
if
> > the
> > > > > > > previous frame's contents are destroyed before the browser stops
> using
> > > > them.
> > > > > > 
> > > > > > The page goes white for a few frames, but that happens even without
> this
> > > > > change.
> > > > > > Sorry I just realized resize seems to be broken even though tab
> eviction
> > > > looks
> > > > > > good to me. I'll try to fix it.
> > > > > 
> > > > > I fixed resize.
> > > > 
> > > > Did any tests catch what you broke? A quick look at bot logs makes me
> think
> > > not.
> > > > If not can you add one?
> > > 
> > > You can see in patch set 5 that a lot of tests failed actually.
> > 
> > Yeh, they didn't look resize related but wasn't sure. Sounds like they were,
> > thanks!
> 
> danakj@: Yes, the tests don't seem to be directly related to resize, but when
> resize breaks so many things go wrong and some tests are gonna fail.
> 
> jbauman@: I was talking to Fady offline and he had a very good point. We call
> SetShowSolidColorContent in EvictDelegatedFrame, which means the page will go
> blank no matter what we do afterwards (force reclaim resources or evict
> surface). The surface will also get destroyed immediately because there is no
> one referencing it.

That will only happen after the browser compositor does its commit and draw,
whereas ForceReclaimResources will cause the contents to disappear before the
browser compositor has a chance to draw a frame that doesn't use the surface.

[Fady Samuel, 2017-03-08 01:46:03]

On 2017/03/08 01:18:05, jbauman wrote:
> On 2017/03/08 01:08:25, Saman Sami wrote:
> > On 2017/03/07 17:15:25, danakj wrote:
> > > On 2017/03/07 17:14:16, Saman Sami wrote:
> > > > On 2017/03/07 16:18:55, danakj wrote:
> > > > > On 2017/03/07 02:04:31, Saman Sami wrote:
> > > > > > On 2017/03/07 01:29:28, Saman Sami wrote:
> > > > > > > On 2017/03/07 01:25:40, jbauman wrote:
> > > > > > > > On 2017/03/07 01:22:14, Saman Sami wrote:
> > > > > > > > > PTAL
> > > > > > > > 
> > > > > > > > I'm surprised this doesn't cause a black flicker on tab
switching
> if
> > > the
> > > > > > > > previous frame's contents are destroyed before the browser stops
> > using
> > > > > them.
> > > > > > > 
> > > > > > > The page goes white for a few frames, but that happens even
without
> > this
> > > > > > change.
> > > > > > > Sorry I just realized resize seems to be broken even though tab
> > eviction
> > > > > looks
> > > > > > > good to me. I'll try to fix it.
> > > > > > 
> > > > > > I fixed resize.
> > > > > 
> > > > > Did any tests catch what you broke? A quick look at bot logs makes me
> > think
> > > > not.
> > > > > If not can you add one?
> > > > 
> > > > You can see in patch set 5 that a lot of tests failed actually.
> > > 
> > > Yeh, they didn't look resize related but wasn't sure. Sounds like they
were,
> > > thanks!
> > 
> > danakj@: Yes, the tests don't seem to be directly related to resize, but
when
> > resize breaks so many things go wrong and some tests are gonna fail.
> > 
> > jbauman@: I was talking to Fady offline and he had a very good point. We
call
> > SetShowSolidColorContent in EvictDelegatedFrame, which means the page will
go
> > blank no matter what we do afterwards (force reclaim resources or evict
> > surface). The surface will also get destroyed immediately because there is
no
> > one referencing it.
> 
> That will only happen after the browser compositor does its commit and draw,
> whereas ForceReclaimResources will cause the contents to disappear before the
> browser compositor has a chance to draw a frame that doesn't use the surface.

So what if we request that the frame be evicted, and then later refer to the
same surface ID again?

The surface has been marked as destroyed...can we UNMARK it again? Or does that
add too much complexity?

I guess I don't necessarily want children to be allocating new surface IDs on
frame eviction. I guess the control flow is really the parent allocates the
surface ID and passes it to the child.

[jbauman, 2017-03-08 01:52:41]

On 2017/03/08 01:18:05, jbauman wrote:
> On 2017/03/08 01:08:25, Saman Sami wrote:
> > On 2017/03/07 17:15:25, danakj wrote:
> > > On 2017/03/07 17:14:16, Saman Sami wrote:
> > > > On 2017/03/07 16:18:55, danakj wrote:
> > > > > On 2017/03/07 02:04:31, Saman Sami wrote:
> > > > > > On 2017/03/07 01:29:28, Saman Sami wrote:
> > > > > > > On 2017/03/07 01:25:40, jbauman wrote:
> > > > > > > > On 2017/03/07 01:22:14, Saman Sami wrote:
> > > > > > > > > PTAL
> > > > > > > > 
> > > > > > > > I'm surprised this doesn't cause a black flicker on tab
switching
> if
> > > the
> > > > > > > > previous frame's contents are destroyed before the browser stops
> > using
> > > > > them.
> > > > > > > 
> > > > > > > The page goes white for a few frames, but that happens even
without
> > this
> > > > > > change.
> > > > > > > Sorry I just realized resize seems to be broken even though tab
> > eviction
> > > > > looks
> > > > > > > good to me. I'll try to fix it.
> > > > > > 
> > > > > > I fixed resize.
> > > > > 
> > > > > Did any tests catch what you broke? A quick look at bot logs makes me
> > think
> > > > not.
> > > > > If not can you add one?
> > > > 
> > > > You can see in patch set 5 that a lot of tests failed actually.
> > > 
> > > Yeh, they didn't look resize related but wasn't sure. Sounds like they
were,
> > > thanks!
> > 
> > danakj@: Yes, the tests don't seem to be directly related to resize, but
when
> > resize breaks so many things go wrong and some tests are gonna fail.
> > 
> > jbauman@: I was talking to Fady offline and he had a very good point. We
call
> > SetShowSolidColorContent in EvictDelegatedFrame, which means the page will
go
> > blank no matter what we do afterwards (force reclaim resources or evict
> > surface). The surface will also get destroyed immediately because there is
no
> > one referencing it.
> 
> That will only happen after the browser compositor does its commit and draw,
> whereas ForceReclaimResources will cause the contents to disappear before the
> browser compositor has a chance to draw a frame that doesn't use the surface.

I've put an example of the black flicker this patch causes at
https://drive.google.com/a/google.com/file/d/0B-5b6Bd64rGhNHhOYmRzWXBqZk0/vie...
. The flicker at the top left is caused by reducing max saved frames to 1,
causing a 1 second delay in RenderWidget::WasShown (though that's probably not
necessary), using --disable-gpu-vsync, having 2 windows open, causing the one
window's renderer to redraw, then switching tabs in the other window. I suspect
that not all those steps are necessary, but they make the timing work out more
often.

[jbauman, 2017-03-08 01:55:37]

On 2017/03/08 01:46:03, Fady Samuel wrote:
> On 2017/03/08 01:18:05, jbauman wrote:
> > On 2017/03/08 01:08:25, Saman Sami wrote:
> > > On 2017/03/07 17:15:25, danakj wrote:
> > > > On 2017/03/07 17:14:16, Saman Sami wrote:
> > > > > On 2017/03/07 16:18:55, danakj wrote:
> > > > > > On 2017/03/07 02:04:31, Saman Sami wrote:
> > > > > > > On 2017/03/07 01:29:28, Saman Sami wrote:
> > > > > > > > On 2017/03/07 01:25:40, jbauman wrote:
> > > > > > > > > On 2017/03/07 01:22:14, Saman Sami wrote:
> > > > > > > > > > PTAL
> > > > > > > > > 
> > > > > > > > > I'm surprised this doesn't cause a black flicker on tab
> switching
> > if
> > > > the
> > > > > > > > > previous frame's contents are destroyed before the browser
stops
> > > using
> > > > > > them.
> > > > > > > > 
> > > > > > > > The page goes white for a few frames, but that happens even
> without
> > > this
> > > > > > > change.
> > > > > > > > Sorry I just realized resize seems to be broken even though tab
> > > eviction
> > > > > > looks
> > > > > > > > good to me. I'll try to fix it.
> > > > > > > 
> > > > > > > I fixed resize.
> > > > > > 
> > > > > > Did any tests catch what you broke? A quick look at bot logs makes
me
> > > think
> > > > > not.
> > > > > > If not can you add one?
> > > > > 
> > > > > You can see in patch set 5 that a lot of tests failed actually.
> > > > 
> > > > Yeh, they didn't look resize related but wasn't sure. Sounds like they
> were,
> > > > thanks!
> > > 
> > > danakj@: Yes, the tests don't seem to be directly related to resize, but
> when
> > > resize breaks so many things go wrong and some tests are gonna fail.
> > > 
> > > jbauman@: I was talking to Fady offline and he had a very good point. We
> call
> > > SetShowSolidColorContent in EvictDelegatedFrame, which means the page will
> go
> > > blank no matter what we do afterwards (force reclaim resources or evict
> > > surface). The surface will also get destroyed immediately because there is
> no
> > > one referencing it.
> > 
> > That will only happen after the browser compositor does its commit and draw,
> > whereas ForceReclaimResources will cause the contents to disappear before
the
> > browser compositor has a chance to draw a frame that doesn't use the
surface.
> 
> So what if we request that the frame be evicted, and then later refer to the
> same surface ID again?

This seems like it would be a good solution.
> 
> The surface has been marked as destroyed...can we UNMARK it again? Or does
that
> add too much complexity?

I think the SurfaceManager would probably break if you tried to do this now, but
it probably wouldn't be too hard to fix. 
> 
> I guess I don't necessarily want children to be allocating new surface IDs on
> frame eviction. I guess the control flow is really the parent allocates the
> surface ID and passes it to the child.

I guess it's more that the child would have to allocate a new surface ID on
WasShown or the equivalent.

[Saman Sami, 2017-03-08 16:32:00]

Description was changed from

==========
Currently, DelegatedFrameHost destroys its surface in EvictDelegatedFrame.
This means that after a new frame arrives a new surface must be created.
This doesn't have to be the case. We can simply call ForceReclaimResources
in order to reclaim the resources while keeping the surface intact.
This needs to be done because we are moving the local surface id allocation  
out of DelegatedFrameHost and into RendererCompositorFrameSink so
DelegatedFrameHost must not interfere in local surface id allocation.

I tried testing this change by setting the maximum number of saved
frames to 1 so that each new tab causes the eviction of the previous tab
and compared the current behaviour vs behaviour after this CL and did
not notice any difference.

BUG=697864
==========

to

==========
Currently, DelegatedFrameHost destroys its surface in EvictDelegatedFrame.
This means that after a new frame arrives a new surface must be created.
This doesn't have to be the case. We can simply call ForceReclaimResources
in order to reclaim the resources while keeping the surface intact.
This needs to be done because we are moving the local surface id allocation  
out of DelegatedFrameHost and into RendererCompositorFrameSink so
DelegatedFrameHost must not interfere in local surface id allocation.

I tried testing this change by setting the maximum number of saved
frames to 1 so that each new tab causes the eviction of the previous tab
and compared the current behaviour vs behaviour after this CL and did
not notice any difference.

BUG=697864
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2017-03-08 16:32:11]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 16:32:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-03-08 16:33:23]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[Saman Sami, 2017-03-08 16:33:27]

Patchset #7 (id:140001) has been deleted

[commit-bot: I haz the power, 2017-03-08 16:33:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Saman Sami, 2017-03-08 16:38:43]

On 2017/03/08 01:55:37, jbauman wrote:
> On 2017/03/08 01:46:03, Fady Samuel wrote:
> > On 2017/03/08 01:18:05, jbauman wrote:
> > > On 2017/03/08 01:08:25, Saman Sami wrote:
> > > > On 2017/03/07 17:15:25, danakj wrote:
> > > > > On 2017/03/07 17:14:16, Saman Sami wrote:
> > > > > > On 2017/03/07 16:18:55, danakj wrote:
> > > > > > > On 2017/03/07 02:04:31, Saman Sami wrote:
> > > > > > > > On 2017/03/07 01:29:28, Saman Sami wrote:
> > > > > > > > > On 2017/03/07 01:25:40, jbauman wrote:
> > > > > > > > > > On 2017/03/07 01:22:14, Saman Sami wrote:
> > > > > > > > > > > PTAL
> > > > > > > > > > 
> > > > > > > > > > I'm surprised this doesn't cause a black flicker on tab
> > switching
> > > if
> > > > > the
> > > > > > > > > > previous frame's contents are destroyed before the browser
> stops
> > > > using
> > > > > > > them.
> > > > > > > > > 
> > > > > > > > > The page goes white for a few frames, but that happens even
> > without
> > > > this
> > > > > > > > change.
> > > > > > > > > Sorry I just realized resize seems to be broken even though
tab
> > > > eviction
> > > > > > > looks
> > > > > > > > > good to me. I'll try to fix it.
> > > > > > > > 
> > > > > > > > I fixed resize.
> > > > > > > 
> > > > > > > Did any tests catch what you broke? A quick look at bot logs makes
> me
> > > > think
> > > > > > not.
> > > > > > > If not can you add one?
> > > > > > 
> > > > > > You can see in patch set 5 that a lot of tests failed actually.
> > > > > 
> > > > > Yeh, they didn't look resize related but wasn't sure. Sounds like they
> > were,
> > > > > thanks!
> > > > 
> > > > danakj@: Yes, the tests don't seem to be directly related to resize, but
> > when
> > > > resize breaks so many things go wrong and some tests are gonna fail.
> > > > 
> > > > jbauman@: I was talking to Fady offline and he had a very good point. We
> > call
> > > > SetShowSolidColorContent in EvictDelegatedFrame, which means the page
will
> > go
> > > > blank no matter what we do afterwards (force reclaim resources or evict
> > > > surface). The surface will also get destroyed immediately because there
is
> > no
> > > > one referencing it.
> > > 
> > > That will only happen after the browser compositor does its commit and
draw,
> > > whereas ForceReclaimResources will cause the contents to disappear before
> the
> > > browser compositor has a chance to draw a frame that doesn't use the
> surface.
> > 
> > So what if we request that the frame be evicted, and then later refer to the
> > same surface ID again?
> 
> This seems like it would be a good solution.
> > 
> > The surface has been marked as destroyed...can we UNMARK it again? Or does
> that
> > add too much complexity?
> 
> I think the SurfaceManager would probably break if you tried to do this now,
but
> it probably wouldn't be too hard to fix. 
> > 
> > I guess I don't necessarily want children to be allocating new surface IDs
on
> > frame eviction. I guess the control flow is really the parent allocates the
> > surface ID and passes it to the child.
> 
> I guess it's more that the child would have to allocate a new surface ID on
> WasShown or the equivalent.

Thank you John for the video. I'm going to keep EvictFrame and try to fix
SurfaceManager so the old surface id can be used again.

[Saman Sami, 2017-03-08 16:39:37]

On 2017/03/08 16:38:43, Saman Sami wrote:
> On 2017/03/08 01:55:37, jbauman wrote:
> > On 2017/03/08 01:46:03, Fady Samuel wrote:
> > > On 2017/03/08 01:18:05, jbauman wrote:
> > > > On 2017/03/08 01:08:25, Saman Sami wrote:
> > > > > On 2017/03/07 17:15:25, danakj wrote:
> > > > > > On 2017/03/07 17:14:16, Saman Sami wrote:
> > > > > > > On 2017/03/07 16:18:55, danakj wrote:
> > > > > > > > On 2017/03/07 02:04:31, Saman Sami wrote:
> > > > > > > > > On 2017/03/07 01:29:28, Saman Sami wrote:
> > > > > > > > > > On 2017/03/07 01:25:40, jbauman wrote:
> > > > > > > > > > > On 2017/03/07 01:22:14, Saman Sami wrote:
> > > > > > > > > > > > PTAL
> > > > > > > > > > > 
> > > > > > > > > > > I'm surprised this doesn't cause a black flicker on tab
> > > switching
> > > > if
> > > > > > the
> > > > > > > > > > > previous frame's contents are destroyed before the browser
> > stops
> > > > > using
> > > > > > > > them.
> > > > > > > > > > 
> > > > > > > > > > The page goes white for a few frames, but that happens even
> > > without
> > > > > this
> > > > > > > > > change.
> > > > > > > > > > Sorry I just realized resize seems to be broken even though
> tab
> > > > > eviction
> > > > > > > > looks
> > > > > > > > > > good to me. I'll try to fix it.
> > > > > > > > > 
> > > > > > > > > I fixed resize.
> > > > > > > > 
> > > > > > > > Did any tests catch what you broke? A quick look at bot logs
makes
> > me
> > > > > think
> > > > > > > not.
> > > > > > > > If not can you add one?
> > > > > > > 
> > > > > > > You can see in patch set 5 that a lot of tests failed actually.
> > > > > > 
> > > > > > Yeh, they didn't look resize related but wasn't sure. Sounds like
they
> > > were,
> > > > > > thanks!
> > > > > 
> > > > > danakj@: Yes, the tests don't seem to be directly related to resize,
but
> > > when
> > > > > resize breaks so many things go wrong and some tests are gonna fail.
> > > > > 
> > > > > jbauman@: I was talking to Fady offline and he had a very good point.
We
> > > call
> > > > > SetShowSolidColorContent in EvictDelegatedFrame, which means the page
> will
> > > go
> > > > > blank no matter what we do afterwards (force reclaim resources or
evict
> > > > > surface). The surface will also get destroyed immediately because
there
> is
> > > no
> > > > > one referencing it.
> > > > 
> > > > That will only happen after the browser compositor does its commit and
> draw,
> > > > whereas ForceReclaimResources will cause the contents to disappear
before
> > the
> > > > browser compositor has a chance to draw a frame that doesn't use the
> > surface.
> > > 
> > > So what if we request that the frame be evicted, and then later refer to
the
> > > same surface ID again?
> > 
> > This seems like it would be a good solution.
> > > 
> > > The surface has been marked as destroyed...can we UNMARK it again? Or does
> > that
> > > add too much complexity?
> > 
> > I think the SurfaceManager would probably break if you tried to do this now,
> but
> > it probably wouldn't be too hard to fix. 
> > > 
> > > I guess I don't necessarily want children to be allocating new surface IDs
> on
> > > frame eviction. I guess the control flow is really the parent allocates
the
> > > surface ID and passes it to the child.
> > 
> > I guess it's more that the child would have to allocate a new surface ID on
> > WasShown or the equivalent.
> 
> Thank you John for the video. I'm going to keep EvictFrame and try to fix
> SurfaceManager so the old surface id can be used again.

The changes in SurfaceManager will be in another CL that I will ask you to
review later.

[commit-bot: I haz the power, 2017-03-08 17:24:54]

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

[commit-bot: I haz the power, 2017-03-08 17:24:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Saman Sami, 2017-03-08 18:13:05]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-08 18:13:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-08 18:36:03]

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

[commit-bot: I haz the power, 2017-03-08 18:36:04]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2017-03-09 16:20:58]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 16:21:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 17:36:11]

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

[commit-bot: I haz the power, 2017-03-09 17:36:12]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2017-03-09 21:49:46]

Description was changed from

==========
Currently, DelegatedFrameHost destroys its surface in EvictDelegatedFrame.
This means that after a new frame arrives a new surface must be created.
This doesn't have to be the case. We can simply call ForceReclaimResources
in order to reclaim the resources while keeping the surface intact.
This needs to be done because we are moving the local surface id allocation  
out of DelegatedFrameHost and into RendererCompositorFrameSink so
DelegatedFrameHost must not interfere in local surface id allocation.

I tried testing this change by setting the maximum number of saved
frames to 1 so that each new tab causes the eviction of the previous tab
and compared the current behaviour vs behaviour after this CL and did
not notice any difference.

BUG=697864
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Once we move local surface id allocation into the renderer process,
DelegatedFrameHost can no longer create a new local surface id on frame
eviction. This would normally break a few things but after 
https://codereview.chromium.org/2736053004/ we should be good.

I tried testing this CL by setting the maximum number of saved
frames to 1 so that each new tab causes the eviction of the previous tab
and compared the current behaviour vs behaviour after this CL and did
not notice any difference. Without https://codereview.chromium.org/2736053004/
if you switch tabs fast enough DCHECKs will fail in SurfaceManager for
allocating
a surface using a surface id that is already in use, but that CL fixes
this issue.

BUG=697864
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2017-03-09 22:06:55]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-09 22:07:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-09 22:35:36]

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

[commit-bot: I haz the power, 2017-03-09 22:35:37]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Saman Sami, 2017-03-10 01:08:03]

The CQ bit was checked by samans@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-10 01:08:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Fady Samuel, 2017-03-10 02:10:01]

lgtm

[commit-bot: I haz the power, 2017-03-10 02:24:00]

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

[commit-bot: I haz the power, 2017-03-10 02:24:01]

Dry run: This issue passed the CQ dry run.

[Saman Sami, 2017-03-10 02:24:15]

jbauman@: After I landed my fix in SurfaceManager all the bots are green. Can
you please take another look? Notice that we are sticking with EvictFrame and
not ForceReclaimResources.

[jbauman, 2017-03-10 21:53:03]

lgtm

[Saman Sami, 2017-03-10 21:53:59]

Description was changed from

==========
Once we move local surface id allocation into the renderer process,
DelegatedFrameHost can no longer create a new local surface id on frame
eviction. This would normally break a few things but after 
https://codereview.chromium.org/2736053004/ we should be good.

I tried testing this CL by setting the maximum number of saved
frames to 1 so that each new tab causes the eviction of the previous tab
and compared the current behaviour vs behaviour after this CL and did
not notice any difference. Without https://codereview.chromium.org/2736053004/
if you switch tabs fast enough DCHECKs will fail in SurfaceManager for
allocating
a surface using a surface id that is already in use, but that CL fixes
this issue.

BUG=697864
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Once we move local surface id allocation into the renderer process,
DelegatedFrameHost can no longer create a new local surface id on frame
eviction. This would normally break a few things but after 
https://codereview.chromium.org/2736053004/ we should be good.

I tried testing this CL by setting the maximum number of saved
frames to 1 so that each new tab causes the eviction of the previous tab
and compared the current behaviour vs behaviour after this CL and did
not notice any difference. Without https://codereview.chromium.org/2736053004/
if you switch tabs fast enough DCHECKs will fail in SurfaceManager for
allocating
a surface using a surface id that is already in use, but that CL fixes
this issue.

TBR=jam@chromium.org
BUG=697864
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Saman Sami, 2017-03-10 21:53:59]

samans@chromium.org changed reviewers:
+ jam@chromium.org

[Saman Sami, 2017-03-10 21:54:55]

The CQ bit was checked by samans@chromium.org

[commit-bot: I haz the power, 2017-03-10 21:55:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-10 22:00:36]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1489182895626300,
"parent_rev": "77414f1023d40d82ec1b636f67a99316b26210be", "commit_rev":
"c43562c581991f1d9b843cb3cec873723a009c31"}

[commit-bot: I haz the power, 2017-03-10 22:01:22]

Description was changed from

==========
Once we move local surface id allocation into the renderer process,
DelegatedFrameHost can no longer create a new local surface id on frame
eviction. This would normally break a few things but after 
https://codereview.chromium.org/2736053004/ we should be good.

I tried testing this CL by setting the maximum number of saved
frames to 1 so that each new tab causes the eviction of the previous tab
and compared the current behaviour vs behaviour after this CL and did
not notice any difference. Without https://codereview.chromium.org/2736053004/
if you switch tabs fast enough DCHECKs will fail in SurfaceManager for
allocating
a surface using a surface id that is already in use, but that CL fixes
this issue.

TBR=jam@chromium.org
BUG=697864
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Once we move local surface id allocation into the renderer process,
DelegatedFrameHost can no longer create a new local surface id on frame
eviction. This would normally break a few things but after 
https://codereview.chromium.org/2736053004/ we should be good.

I tried testing this CL by setting the maximum number of saved
frames to 1 so that each new tab causes the eviction of the previous tab
and compared the current behaviour vs behaviour after this CL and did
not notice any difference. Without https://codereview.chromium.org/2736053004/
if you switch tabs fast enough DCHECKs will fail in SurfaceManager for
allocating
a surface using a surface id that is already in use, but that CL fixes
this issue.

TBR=jam@chromium.org
BUG=697864
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2731793002
Cr-Commit-Position: refs/heads/master@{#456180}
Committed:
https://chromium.googlesource.com/chromium/src/+/c43562c581991f1d9b843cb3cec8...
==========

[commit-bot: I haz the power, 2017-03-10 22:01:24]

Committed patchset #9 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/c43562c581991f1d9b843cb3cec8...