cc: Remove EnsureRenderSurfaceLayerList, do UpdateDrawProps when needed.

cc: Remove EnsureRenderSurfaceLayerList, do UpdateDrawProps when needed.

Hit testing is done directly on the layer tree, it doesn't use the
render surface layer list, but it does require draw properties. So,
remove EnsureRenderSurfaceLayerList and have the FindLayer* public
methods on LayerTreeImpl just do UpdateDrawProperties() if it is needed.

Since we update draw properties there, we only need to update them in
PrepareToDraw when they are dirty as well.

R=enne, vollick@chromium.org

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

[danakj, 2014-06-11 20:39:34]

Net same number of lines, but hopefully you find this an improvement?

[danakj, 2014-06-11 20:45:27]

On 2014/06/11 20:39:34, danakj wrote:
> Net same number of lines, but hopefully you find this an improvement?

There's some unit tests that break with this, hold up on review please.

[danakj, 2014-06-11 21:16:02]

Ok PTAL.

The change in UpdateDrawProperties to not clear needs_update_draw_props_ when
the update failed turned up a sketchy PictureImageLayerImpl unittest, which in
turn found a completely unused virtual method on LayerImpl outside of that test,
so killing all of that.

[enne (OOO), 2014-06-11 21:23:08]

https://codereview.chromium.org/332433003/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/332433003/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:541:
EXPECT_EQ(InputHandler::ScrollStarted,
Can you explain this change more?

[danakj, 2014-06-11 21:24:38]

https://codereview.chromium.org/332433003/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/332433003/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:541:
EXPECT_EQ(InputHandler::ScrollStarted,
On 2014/06/11 21:23:07, enne wrote:
> Can you explain this change more?

While we're unable to UpdateDrawProperties, the ScrollBegin method can still
find the InnerViewportScrollLayer(), so it starts a scroll on that. That seems
equally okay as aborting the scroll to me, but I could add some extra early out
in ScrollBegin if you don't agree.

[enne (OOO), 2014-06-11 21:28:44]

https://codereview.chromium.org/332433003/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/332433003/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:541:
EXPECT_EQ(InputHandler::ScrollStarted,
On 2014/06/11 21:24:38, danakj wrote:
> On 2014/06/11 21:23:07, enne wrote:
> > Can you explain this change more?
> 
> While we're unable to UpdateDrawProperties, the ScrollBegin method can still
> find the InnerViewportScrollLayer(), so it starts a scroll on that. That seems
> equally okay as aborting the scroll to me, but I could add some extra early
out
> in ScrollBegin if you don't agree.

Ah, ok.  That makes sense.  We no longer need a renderer to hit test.

https://codereview.chromium.org/332433003/diff/60001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/332433003/diff/60001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:1235: if (needs_update_draw_properties_ &&
!UpdateDrawProperties())
What about having UpdateDrawProperties early out with "return true" if
!needs_update_draw_properties? That'll make this condition shorter and will get
us an early out if anybody calls UpdateDrawProperties extra.

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

https://codereview.chromium.org/332433003/diff/60001/cc/layers/picture_image_...
File cc/layers/picture_image_layer_impl_unittest.cc (left):

https://codereview.chromium.org/332433003/diff/60001/cc/layers/picture_image_...
cc/layers/picture_image_layer_impl_unittest.cc:105: UpdateDrawProperties();
I did some related/unrelated blaming just now and came across this unit test.
This line was added to it in a283d31f511 where we added some DCHECKs in
PictureLayerImpl::AreVisibleResourcesReady. Since that method is gone now, this
test isn't verifying much, but that's unfortunate because I think it leaves that
CL without any kind of regression test right now.

https://codereview.chromium.org/332433003/diff/60001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/332433003/diff/60001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:1235: if (needs_update_draw_properties_ &&
!UpdateDrawProperties())
On 2014/06/11 21:28:43, enne wrote:
> What about having UpdateDrawProperties early out with "return true" if
> !needs_update_draw_properties? That'll make this condition shorter and will
get
> us an early out if anybody calls UpdateDrawProperties extra.

Ya, I didn't do that only cuz I'm scared it might make bugs I don't trust our
dirty state tracking I guess, but that's a poor reason. I'll do it.

[Ian Vollick, 2014-06-12 15:55:04]

https://codereview.chromium.org/332433003/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/332433003/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:541:
EXPECT_EQ(InputHandler::ScrollStarted,
On 2014/06/11 21:28:43, enne wrote:
> On 2014/06/11 21:24:38, danakj wrote:
> > On 2014/06/11 21:23:07, enne wrote:
> > > Can you explain this change more?
> > 
> > While we're unable to UpdateDrawProperties, the ScrollBegin method can still
> > find the InnerViewportScrollLayer(), so it starts a scroll on that. That
seems
> > equally okay as aborting the scroll to me, but I could add some extra early
> out
> > in ScrollBegin if you don't agree.
> 
> Ah, ok.  That makes sense.  We no longer need a renderer to hit test.

Can you please add a 2nd check for a ScrollBegin at a point outside the
scrollers? It'd be nice if we had tests that checked that, even with an empty
RSLL (which was the old mode of failure, right?), we could still both scroll or
not scroll.

https://codereview.chromium.org/332433003/diff/60001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/332433003/diff/60001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:444: return false;
Why don't we want to clear the RSLL when we bail early?

[danakj, 2014-06-12 16:03:58]

https://codereview.chromium.org/332433003/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/332433003/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:541:
EXPECT_EQ(InputHandler::ScrollStarted,
On 2014/06/12 15:55:03, Ian Vollick wrote:
> On 2014/06/11 21:28:43, enne wrote:
> > On 2014/06/11 21:24:38, danakj wrote:
> > > On 2014/06/11 21:23:07, enne wrote:
> > > > Can you explain this change more?
> > > 
> > > While we're unable to UpdateDrawProperties, the ScrollBegin method can
still
> > > find the InnerViewportScrollLayer(), so it starts a scroll on that. That
> seems
> > > equally okay as aborting the scroll to me, but I could add some extra
early
> > out
> > > in ScrollBegin if you don't agree.
> > 
> > Ah, ok.  That makes sense.  We no longer need a renderer to hit test.
> 
> Can you please add a 2nd check for a ScrollBegin at a point outside the
> scrollers? It'd be nice if we had tests that checked that, even with an empty
> RSLL (which was the old mode of failure, right?), we could still both scroll
or
> not scroll.

An empty RSLL only happens when there is no root layer or there is no renderer,
which this test and the one above do cover.

The ScrollMissesChild test covers scrolling outside all the layers.

https://codereview.chromium.org/332433003/diff/60001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/332433003/diff/60001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:444: return false;
On 2014/06/12 15:55:03, Ian Vollick wrote:
> Why don't we want to clear the RSLL when we bail early?

The flag is still dirty so the values are still old. I'd rather this method do
nothing if it returns false.

[Ian Vollick, 2014-06-12 16:22:20]

https://codereview.chromium.org/332433003/diff/60001/cc/trees/layer_tree_host...
File cc/trees/layer_tree_host_impl_unittest.cc (right):

https://codereview.chromium.org/332433003/diff/60001/cc/trees/layer_tree_host...
cc/trees/layer_tree_host_impl_unittest.cc:541:
EXPECT_EQ(InputHandler::ScrollStarted,
On 2014/06/12 16:03:58, danakj wrote:
> On 2014/06/12 15:55:03, Ian Vollick wrote:
> > On 2014/06/11 21:28:43, enne wrote:
> > > On 2014/06/11 21:24:38, danakj wrote:
> > > > On 2014/06/11 21:23:07, enne wrote:
> > > > > Can you explain this change more?
> > > > 
> > > > While we're unable to UpdateDrawProperties, the ScrollBegin method can
> still
> > > > find the InnerViewportScrollLayer(), so it starts a scroll on that. That
> > seems
> > > > equally okay as aborting the scroll to me, but I could add some extra
> early
> > > out
> > > > in ScrollBegin if you don't agree.
> > > 
> > > Ah, ok.  That makes sense.  We no longer need a renderer to hit test.
> > 
> > Can you please add a 2nd check for a ScrollBegin at a point outside the
> > scrollers? It'd be nice if we had tests that checked that, even with an
empty
> > RSLL (which was the old mode of failure, right?), we could still both scroll
> or
> > not scroll.
> 
> An empty RSLL only happens when there is no root layer or there is no
renderer,
> which this test and the one above do cover.
> 
> The ScrollMissesChild test covers scrolling outside all the layers.

What I was getting at was hit testing (for scrolling, etc) on layers that aren't
included in the RSLL, but that's unrelated to the test you're modifying here --
I don't think that this CL is reducing coverage, so ignore me :)

https://codereview.chromium.org/332433003/diff/60001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/332433003/diff/60001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:444: return false;
On 2014/06/12 16:03:58, danakj wrote:
> On 2014/06/12 15:55:03, Ian Vollick wrote:
> > Why don't we want to clear the RSLL when we bail early?
> 
> The flag is still dirty so the values are still old. I'd rather this method do
> nothing if it returns false.

sg

[danakj, 2014-06-12 18:26:51]

https://codereview.chromium.org/332433003/diff/60001/cc/trees/layer_tree_impl.cc
File cc/trees/layer_tree_impl.cc (right):

https://codereview.chromium.org/332433003/diff/60001/cc/trees/layer_tree_impl...
cc/trees/layer_tree_impl.cc:1235: if (needs_update_draw_properties_ &&
!UpdateDrawProperties())
On 2014/06/11 21:41:19, danakj wrote:
> On 2014/06/11 21:28:43, enne wrote:
> > What about having UpdateDrawProperties early out with "return true" if
> > !needs_update_draw_properties? That'll make this condition shorter and will
> get
> > us an early out if anybody calls UpdateDrawProperties extra.
> 
> Ya, I didn't do that only cuz I'm scared it might make bugs I don't trust our
> dirty state tracking I guess, but that's a poor reason. I'll do it.

Done.

[enne (OOO), 2014-06-12 18:30:42]

lgtm

[danakj, 2014-06-12 18:31:17]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-06-12 18:34:15]

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

[commit-bot: I haz the power, 2014-06-13 00:23:41]

Change committed as 276864