cc: Remove max tiles and skewport constants from tiling client.

cc: Remove max tiles and skewport constants from tiling client.

This patch removes some more client functionality from picture layer
tiling. These values don't typically change (aside from going from
gpu to cpu rasterization), but are frequently accessed. By eliminating
these calls makes the code more direct (ie it's obvious where these
settings are set).

Also, it potentially makes the code quicker since it eliminates several
virtual calls, although this is only called once per frame.

BUG=433048
R=danakj, enne

Committed: https://crrev.com/806cff5e6757e655cdfa769832f1b5699116867b
Cr-Commit-Position: refs/heads/master@{#307963}

[vmpstr, 2014-12-02 01:00:51]

PTAL. danakj@, this clashes a bit with your no-swap patch, so I'll rebase when
it lands, but please point out things that I should change if it doesn't align
with that patch.

[enne (OOO), 2014-12-02 21:25:58]

lgtm to remove more things from the tiling client

https://codereview.chromium.org/775483002/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/775483002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:607: // TODO(vmpstr): Should this funciton set
tilings_ to nullptr instead?
I'm not sure.  I think previously PictureLayerImpl has just tried to always
assume tilings_ was not null, but maybe if it was more explicit about when it
could be null and when it got lazily created then this could be clearer? I'm not
100% what all the details of how this would look.

[danakj, 2014-12-02 21:58:37]

https://codereview.chromium.org/775483002/diff/1/cc/layers/picture_layer_impl.cc
File cc/layers/picture_layer_impl.cc (right):

https://codereview.chromium.org/775483002/diff/1/cc/layers/picture_layer_impl...
cc/layers/picture_layer_impl.cc:607: // TODO(vmpstr): Should this funciton set
tilings_ to nullptr instead?
On 2014/12/02 21:25:58, enne wrote:
> I'm not sure.  I think previously PictureLayerImpl has just tried to always
> assume tilings_ was not null, but maybe if it was more explicit about when it
> could be null and when it got lazily created then this could be clearer? I'm
not
> 100% what all the details of how this would look.

I'd like to not deal with nullptrs. With my CL it's always non-null and that
seems nice. It could even change to a non-pointer member perhaps.

I'm a bit surprised this function doesn't do tilings_ = PLTS::Create(... all
these new values ...), then you don't need setters. Given that it's a scoped_ptr
right now. How come it doesn't just recreate tilings_?

Maybe this class could have a method (or static file method) that creates a PLTS
and takes a LTI* as a parameter to look up stuff. And PLI could use that method
to reset the PLTS here and in the other creation location?

https://codereview.chromium.org/775483002/diff/1/cc/resources/picture_layer_t...
File cc/resources/picture_layer_tiling_perftest.cc (right):

https://codereview.chromium.org/775483002/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling_perftest.cc:26: const size_t
kMaxTilesForInterestArea = 250;
Maybe these should be defaults in LayerTreeSettings and this could just use them
from there? It's marginally funny to have constants in the perf test that dont
change when we'd change them in prod?

https://codereview.chromium.org/775483002/diff/1/cc/resources/picture_layer_t...
File cc/resources/picture_layer_tiling_set_unittest.cc (right):

https://codereview.chromium.org/775483002/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling_set_unittest.cc:21: const size_t
kMaxTilesForInterestArea = 128;
kinda similar comment here.. these are somewhat meaningless for these tests i'm
guessing? maybe we can grab them from LayerTreeSettings tho? idk. Given they are
in LTSettings means the client can change them tho anyways.. so the defaults
don't necessarily matter either. /ponders

[vmpstr, 2014-12-10 20:15:03]

ptal. I think most comments were addressed in the latest patch.

[danakj, 2014-12-10 22:23:25]

https://codereview.chromium.org/775483002/diff/40001/cc/debug/rasterize_and_r...
File cc/debug/rasterize_and_record_benchmark_impl.cc (right):

https://codereview.chromium.org/775483002/diff/40001/cc/debug/rasterize_and_r...
cc/debug/rasterize_and_record_benchmark_impl.cc:211:
settings.skewport_target_time_in_seconds,
how come this doesn't check for gpu raster and pass 0?

https://codereview.chromium.org/775483002/diff/40001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling_set.h (right):

https://codereview.chromium.org/775483002/diff/40001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling_set.h:172: size_t
max_tiles_for_interest_area_;
const?

https://codereview.chromium.org/775483002/diff/40001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/775483002/diff/40001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling_unittest.cc:71: return Create(contents_scale,
layer_bounds, client, 10000,
did you mean to use 10000 here?

https://codereview.chromium.org/775483002/diff/40001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling_unittest.cc:110: tiling_ =
TestablePictureLayerTiling::Create(
or else this can just call CreateDefault?

[vmpstr, 2014-12-11 00:14:17]

ptal

https://codereview.chromium.org/775483002/diff/40001/cc/debug/rasterize_and_r...
File cc/debug/rasterize_and_record_benchmark_impl.cc (right):

https://codereview.chromium.org/775483002/diff/40001/cc/debug/rasterize_and_r...
cc/debug/rasterize_and_record_benchmark_impl.cc:211:
settings.skewport_target_time_in_seconds,
On 2014/12/10 22:23:24, danakj wrote:
> how come this doesn't check for gpu raster and pass 0?

It doesn't really matter what the value is in this case. This will just create
all tiles anyway and rasterize the viewport. I added a comment to that effect.

https://codereview.chromium.org/775483002/diff/40001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling_set.h (right):

https://codereview.chromium.org/775483002/diff/40001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling_set.h:172: size_t
max_tiles_for_interest_area_;
On 2014/12/10 22:23:25, danakj wrote:
> const?

Yes! thanks.

https://codereview.chromium.org/775483002/diff/40001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling_unittest.cc (right):

https://codereview.chromium.org/775483002/diff/40001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling_unittest.cc:71: return Create(contents_scale,
layer_bounds, client, 10000,
On 2014/12/10 22:23:25, danakj wrote:
> did you mean to use 10000 here?

I changed this a bit. I removed CreateDefault, and added LayerTreeSettings as a
parameter to Create. Each of the tests that creates this now passes the settings
either unmodified from default, or modifies the settings and passes it. I think
that's the most sane approach.

https://codereview.chromium.org/775483002/diff/40001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling_unittest.cc:110: tiling_ =
TestablePictureLayerTiling::Create(
On 2014/12/10 22:23:25, danakj wrote:
> or else this can just call CreateDefault?

Done.

[danakj, 2014-12-11 00:15:59]

LGTM

[vmpstr, 2014-12-11 19:06:33]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2014-12-11 19:07:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/775483002/100001

[commit-bot: I haz the power, 2014-12-11 20:30:17]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-12-11 20:31:10]

Patchset 6 (id:??) landed as
https://crrev.com/806cff5e6757e655cdfa769832f1b5699116867b
Cr-Commit-Position: refs/heads/master@{#307963}