cc: Remove GetRasterSource from PictureLayerTilingClient.

cc: Remove GetRasterSource from PictureLayerTilingClient.

This patch removes GetRasterSource from
PictureLayerTilingClient and adds it to PLTS.

Also changes PLTS creation from using a constructor to
using a ::Create function.

BUG=433048
R=danakj

Committed: https://crrev.com/5879bdc6865f534d4d8e78ef9389d0d75b3dc42f
Cr-Commit-Position: refs/heads/master@{#304457}

[vmpstr, 2014-11-13 21:56:56]

Hi,

Please take a look. I'd like to try and do this work over several smaller
patches, so this just removes one thing from the client.

[enne (OOO), 2014-11-14 21:27:02]

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

[enne (OOO), 2014-11-14 21:27:03]

lgtm

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

https://codereview.chromium.org/716283003/diff/40001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling.h:147: void
UpdateTilesToCurrentRasterSource(RasterSource* raster_source,
How nice that UpdateTilesToCurrentRasterResource includes the raster_source!

[danakj, 2014-11-14 21:39:51]

https://codereview.chromium.org/716283003/diff/60001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling_set.cc (right):

https://codereview.chromium.org/716283003/diff/60001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling_set.cc:52: bool
PictureLayerTilingSet::SyncTilings(const PictureLayerTilingSet& other,
how about passing it in here instead of adding a member variable to PLTS?

[vmpstr, 2014-11-14 23:49:02]

https://codereview.chromium.org/716283003/diff/60001/cc/resources/picture_lay...
File cc/resources/picture_layer_tiling_set.cc (right):

https://codereview.chromium.org/716283003/diff/60001/cc/resources/picture_lay...
cc/resources/picture_layer_tiling_set.cc:52: bool
PictureLayerTilingSet::SyncTilings(const PictureLayerTilingSet& other,
On 2014/11/14 21:39:51, danakj wrote:
> how about passing it in here instead of adding a member variable to PLTS?

I kind of like it as a PLTS member... My reason is that PLTS itself is a thing
that should know about the current raster source, since if we move towards it
being more opaque to the layer, it makes sense that a cache for rasterized
content is aware of the source of said rasterization (... if that makes any
sense ...)

I do see the flip side that it's more difficult to maintain if we have to set it
independently. However, I think it's worth the effort. I think the fact that we
UpdateTilesToCurrentRasterSource is called in SyncTilings is pretty arbitrary.
That is, I can picture a system where SyncTilings grabs tilings and invalidates
stuff, and SetRasterSource is the thing that UpdatesToCurrentRasterSource.

Maybe? I think this is more of a design choice than anything here, so if you
feel strongly about it, I can change it. Wdyt?

[danakj, 2014-11-15 00:05:19]

On Nov 14, 2014 6:49 PM, <vmpstr@chromium.org> wrote:
>
>
>
https://codereview.chromium.org/716283003/diff/60001/cc/resources/picture_lay...
> File cc/resources/picture_layer_tiling_set.cc (right):
>
>
https://codereview.chromium.org/716283003/diff/60001/cc/resources/picture_lay...
> cc/resources/picture_layer_tiling_set.cc:52: bool
> PictureLayerTilingSet::SyncTilings(const PictureLayerTilingSet& other,
> On 2014/11/14 21:39:51, danakj wrote:
>>
>> how about passing it in here instead of adding a member variable to
>
> PLTS?
>
> I kind of like it as a PLTS member... My reason is that PLTS itself is a
> thing that should know about the current raster source, since if we move
> towards it being more opaque to the layer, it makes sense that a cache
> for rasterized content is aware of the source of said rasterization (...
> if that makes any sense ...)
>
> I do see the flip side that it's more difficult to maintain if we have
> to set it independently. However, I think it's worth the effort. I think
> the fact that we UpdateTilesToCurrentRasterSource is called in
> SyncTilings is pretty arbitrary. That is, I can picture a system where
> SyncTilings grabs tilings and invalidates stuff, and SetRasterSource is
> the thing that UpdatesToCurrentRasterSource.
>
> Maybe? I think this is more of a design choice than anything here, so if
> you feel strongly about it, I can change it. Wdyt?

I understand ur motivation and I'm happy about where you're going. But I
prefer not to add a member variable to this class just for it to pass it
along later. Its much less clear which raster source it'll use since now u
need to know when set was last called. If it makes sense in the future to
make it a member then that's cool but ATM I don't think it does.

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

[vmpstr, 2014-11-15 00:27:19]

On 2014/11/15 00:05:19, danakj wrote:
> On Nov 14, 2014 6:49 PM, <mailto:vmpstr@chromium.org> wrote:
> >
> >
> >
>
https://codereview.chromium.org/716283003/diff/60001/cc/resources/picture_lay...
> > File cc/resources/picture_layer_tiling_set.cc (right):
> >
> >
>
https://codereview.chromium.org/716283003/diff/60001/cc/resources/picture_lay...
> > cc/resources/picture_layer_tiling_set.cc:52: bool
> > PictureLayerTilingSet::SyncTilings(const PictureLayerTilingSet& other,
> > On 2014/11/14 21:39:51, danakj wrote:
> >>
> >> how about passing it in here instead of adding a member variable to
> >
> > PLTS?
> >
> > I kind of like it as a PLTS member... My reason is that PLTS itself is a
> > thing that should know about the current raster source, since if we move
> > towards it being more opaque to the layer, it makes sense that a cache
> > for rasterized content is aware of the source of said rasterization (...
> > if that makes any sense ...)
> >
> > I do see the flip side that it's more difficult to maintain if we have
> > to set it independently. However, I think it's worth the effort. I think
> > the fact that we UpdateTilesToCurrentRasterSource is called in
> > SyncTilings is pretty arbitrary. That is, I can picture a system where
> > SyncTilings grabs tilings and invalidates stuff, and SetRasterSource is
> > the thing that UpdatesToCurrentRasterSource.
> >
> > Maybe? I think this is more of a design choice than anything here, so if
> > you feel strongly about it, I can change it. Wdyt?
> 
> I understand ur motivation and I'm happy about where you're going. But I
> prefer not to add a member variable to this class just for it to pass it
> along later. Its much less clear which raster source it'll use since now u
> need to know when set was last called. If it makes sense in the future to
> make it a member then that's cool but ATM I don't think it does.
> 

Ok, I can agree with that.


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

[vmpstr, 2014-11-15 00:44:17]

PTAL. Any objections to making this a ::Create type of class instead of a
on-the-stack-or-new type of class? I don't really have much of a preference, but
in my mind this is going to become more of a "heavy" class, and less of a thin
wrapper over tilings...

[danakj, 2014-11-17 16:24:24]

LGTM

[danakj, 2014-11-17 16:25:39]

On 2014/11/15 00:44:17, vmpstr wrote:
> PTAL. Any objections to making this a ::Create type of class instead of a
> on-the-stack-or-new type of class? I don't really have much of a preference,
but
> in my mind this is going to become more of a "heavy" class, and less of a thin
> wrapper over tilings...

No objection. I think we could just go and remove all the Create() methods since
in the future we'll want to make_unique() them instead. But we do it a lot
currently, so it's more consistent to repeat the pattern for things put into
scoped_ptrs.

[vmpstr, 2014-11-17 18:37:18]

The CQ bit was checked by vmpstr@chromium.org

[commit-bot: I haz the power, 2014-11-17 18:38:27]

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

[commit-bot: I haz the power, 2014-11-17 19:51:40]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-11-17 19:52:49]

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