cc: Don't add new tilings while syncing a tiling.

cc: Don't add new tilings while syncing a tiling.

When a layer adds a tiling, it syncs to its pending/active twin and
adds the tiling there. We "cleverly" do an UpdateTilePriorities() then
immediately if possible. But now UpdateTilePriorities() does much more
than it used to, and also adds new tilings etc, which can cause a sync
back to the other twin and re-entrancy and bad things.

Instead, split everything that isn't updating tiles priorities off to
UpdateTiles() which calls UpdateTilePriorities(), and have
UpdateDrawProperties() call that instead. This way syncing only updates
priorities, and doesn't do things like adding new tilings or changing
what is considered high res.

R=enne
BUG=386076
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=278604

[danakj, 2014-06-18 22:14:47]

This depends on https://codereview.chromium.org/334953003/ but I could easily
roll it into this CL if you like.

[danakj, 2014-06-18 22:39:38]

BTW verified the telemetry pinchzoom tests pass after this CL

[tonyg, 2014-06-19 00:30:01]

https://codereview.chromium.org/343653004/diff/60001/tools/perf/benchmarks/sm...
File tools/perf/benchmarks/smoothness.py (left):

https://codereview.chromium.org/343653004/diff/60001/tools/perf/benchmarks/sm...
tools/perf/benchmarks/smoothness.py:118: @test.Disabled  # crbug.com/384730
I think this is supposed to go back to @test.Enabled('android') as I don't think
it ever worked or is intended to work on desktop.

[danakj, 2014-06-19 00:55:48]

https://codereview.chromium.org/343653004/diff/60001/tools/perf/benchmarks/sm...
File tools/perf/benchmarks/smoothness.py (left):

https://codereview.chromium.org/343653004/diff/60001/tools/perf/benchmarks/sm...
tools/perf/benchmarks/smoothness.py:118: @test.Disabled  # crbug.com/384730
On 2014/06/19 00:30:01, tonyg wrote:
> I think this is supposed to go back to @test.Enabled('android') as I don't
think
> it ever worked or is intended to work on desktop.

oh ok thanks, totally missed that.

[tonyg, 2014-06-19 00:59:30]

lgtm stamp for telemetry file

[enne (OOO), 2014-06-19 17:38:49]

https://codereview.chromium.org/343653004/diff/60001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (left):

https://codereview.chromium.org/343653004/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:744:
DCHECK(!layer_tree_impl()->needs_update_draw_properties());
Why does this go away?

[danakj, 2014-06-19 17:43:37]

https://codereview.chromium.org/343653004/diff/60001/cc/layers/picture_layer_...
File cc/layers/picture_layer_impl.cc (left):

https://codereview.chromium.org/343653004/diff/60001/cc/layers/picture_layer_...
cc/layers/picture_layer_impl.cc:744:
DCHECK(!layer_tree_impl()->needs_update_draw_properties());
On 2014/06/19 17:38:49, enne wrote:
> Why does this go away?

This is guaranteed by the callstack now, we only call into here from
UpdateDrawProps which just did CalcDrawProps.

This went away because we have a bunch of tests that just set the
draw_properties and then call this still, instead of doing a full
UpdateDrawProps(), and that caused this to trip. Removing this let me get rid of
the re-implementation of UpdateTiles in the test code as SetupAndManageTiles()
or whatnot in FakePicture*LayerImpl.

[enne (OOO), 2014-06-19 17:46:42]

Hmm, ok ok.

lgtm, thanks!

[danakj, 2014-06-19 17:53:24]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-06-19 17:55:29]

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

[danakj, 2014-06-19 19:26:41]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-06-19 19:29:08]

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

[alokp, 2014-06-19 19:32:36]

https://codereview.chromium.org/343653004/diff/100001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling_set.h (left):

https://codereview.chromium.org/343653004/diff/100001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.h:57: void UpdateTilePriorities(WhichTree
tree,
Dana: I did not understand why you had to remove this and copy the
implementation at two places - PictureLayerImpl and
picture_layer_tiling_unittest.cc.

[danakj, 2014-06-19 20:07:38]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-06-19 20:09:26]

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

[danakj, 2014-06-19 21:57:21]

https://codereview.chromium.org/343653004/diff/100001/cc/resources/picture_la...
File cc/resources/picture_layer_tiling_set.h (left):

https://codereview.chromium.org/343653004/diff/100001/cc/resources/picture_la...
cc/resources/picture_layer_tiling_set.h:57: void UpdateTilePriorities(WhichTree
tree,
On 2014/06/19 19:32:35, Alok Priyadarshi wrote:
> Dana: I did not understand why you had to remove this and copy the
> implementation at two places - PictureLayerImpl and
> picture_layer_tiling_unittest.cc.

All it is now is a for loop, it used to compute a new visible rect so it had
more purpose. Just simpler to call UTP on the tilings directly, I think esp
since there's only one call site.

[alokp, 2014-06-19 23:06:53]

On 2014/06/19 21:57:21, danakj wrote:
>
https://codereview.chromium.org/343653004/diff/100001/cc/resources/picture_la...
> File cc/resources/picture_layer_tiling_set.h (left):
> 
>
https://codereview.chromium.org/343653004/diff/100001/cc/resources/picture_la...
> cc/resources/picture_layer_tiling_set.h:57: void
UpdateTilePriorities(WhichTree
> tree,
> On 2014/06/19 19:32:35, Alok Priyadarshi wrote:
> > Dana: I did not understand why you had to remove this and copy the
> > implementation at two places - PictureLayerImpl and
> > picture_layer_tiling_unittest.cc.
> 
> All it is now is a for loop, it used to compute a new visible rect so it had
> more purpose. Just simpler to call UTP on the tilings directly, I think esp
> since there's only one call site.

OK. Although there are many such wrapper functions on PictureLayerTilingSet that
just iterate over the tilings.

[commit-bot: I haz the power, 2014-06-20 05:03:28]

Change committed as 278604

[Nico, 2015-12-02 17:29:28]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-12-02 17:29:29]

https://codereview.chromium.org/343653004/diff/120001/cc/layers/picture_image...
File cc/layers/picture_image_layer_impl_unittest.cc (right):

https://codereview.chromium.org/343653004/diff/120001/cc/layers/picture_image...
cc/layers/picture_image_layer_impl_unittest.cc:131:
static_cast<TestablePictureImageLayerImpl*>(
ubsan points out that this cast is not valid:

[ RUN      ] PictureImageLayerImplTest.IgnoreIdealContentScale
../../cc/layers/picture_image_layer_impl_unittest.cc:140:7: runtime error:
downcast of address 0x2c023a6b3d80 which does not point to an object of type
'cc::(anonymous namespace)::TestablePictureImageLayerImpl'
0x2c023a6b3d80: note: object is of type 'cc::PictureImageLayerImpl'
 00 00 00 00  48 a0 d2 05 00 00 00 00  e0 a1 d2 05 00 00 00 00  08 a2 d2 05 00
00 00 00  00 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'cc::PictureImageLayerImpl'

I don't know if that's a recent regression though.

(
https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinuxUBSanVptr%20t...
)

[danakj, 2015-12-02 23:14:15]

https://codereview.chromium.org/343653004/diff/120001/cc/layers/picture_image...
File cc/layers/picture_image_layer_impl_unittest.cc (right):

https://codereview.chromium.org/343653004/diff/120001/cc/layers/picture_image...
cc/layers/picture_image_layer_impl_unittest.cc:131:
static_cast<TestablePictureImageLayerImpl*>(
On 2015/12/02 17:29:29, Nico wrote:
> ubsan points out that this cast is not valid:
> 
> [ RUN      ] PictureImageLayerImplTest.IgnoreIdealContentScale
> ../../cc/layers/picture_image_layer_impl_unittest.cc:140:7: runtime error:
> downcast of address 0x2c023a6b3d80 which does not point to an object of type
> 'cc::(anonymous namespace)::TestablePictureImageLayerImpl'
> 0x2c023a6b3d80: note: object is of type 'cc::PictureImageLayerImpl'
>  00 00 00 00  48 a0 d2 05 00 00 00 00  e0 a1 d2 05 00 00 00 00  08 a2 d2 05 00
> 00 00 00  00 00 00 00
>               ^~~~~~~~~~~~~~~~~~~~~~~
>               vptr for 'cc::PictureImageLayerImpl'
> 
> I don't know if that's a recent regression though.
> 
> (
>
https://build.chromium.org/p/chromium.fyi/builders/ClangToTLinuxUBSanVptr%20t...
> )

Thanks. Looks like TestablePictureImageLayerImpl does not override
CreateLayerImpl, so it would be an incorrect (tho currently pretty harmless i
guess..) cast. I can fix.