c:: Give the raster source to the PictureLayerTilings.

c:: Give the raster source to the PictureLayerTilings.

We were passing around just the layer_bounds, and passing the raster
source through the tilings to the tiles. Instead, let the tiling save
the raster source and then it can skip creating unrasterable tiles
itself making the PictureLayerImpl::CreateTile no longer able to return
a nullptr.

R=enne, vmpstr
BUG=387116
Committed: https://crrev.com/16f38e8107f49d00d6bfe694e33ea61d394aaa7b
Cr-Commit-Position: refs/heads/master@{#309251}

[danakj, 2014-12-19 17:12:14]

My original motivation for this was to replace PLTClient::CreateTile with
PLTClient::GetTileManager. But there's a bunch of other stuff that PLI passes
along to CreateTile, like the flags, layer id and source frame number. So then
I'd need to return those along with, or in another virtual from, the tile
manager.

So.. I didn't do that yet.. but the one (two?) benefit(s) of this change are:
- PLT::CreateTile can skip tiles making the contract for PLTClient::CreateTile
simpler - it always returns a tile.
- No longer passing redundant raster source and layer bounds around.

WDYT?

[tfarina, 2014-12-19 17:18:11]

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

https://codereview.chromium.org/820703002/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling_set_unittest.cc:37: auto pile =
I know auto is allowed now that we are using C++11, but I fear it makes code
harder to read. Without looking at CreateEmptyPileWithDefaultTileSize(), I have
no idea what it returns. A pointer, a reference, a value? To what type?

That is what I don't like in languages like Python and Javascript, the like of
type safety.

[tfarina, 2014-12-19 17:19:49]

On Fri, Dec 19, 2014 at 3:18 PM, <tfarina@chromium.org> wrote:
>
>
> https://codereview.chromium.org/820703002/diff/1/cc/
> resources/picture_layer_tiling_set_unittest.cc
> File cc/resources/picture_layer_tiling_set_unittest.cc (right):
>
> https://codereview.chromium.org/820703002/diff/1/cc/
> resources/picture_layer_tiling_set_unittest.cc#newcode37
> cc/resources/picture_layer_tiling_set_unittest.cc:37: auto pile =
> I know auto is allowed now that we are using C++11, but I fear it makes
> code harder to read. Without looking at
> CreateEmptyPileWithDefaultTileSize(), I have no idea what it returns. A
> pointer, a reference, a value? To what type?
>
> That is what I don't like in languages like Python and Javascript, the
> like of type safety.
>
> s/like/lack


> https://codereview.chromium.org/820703002/
>


-- 
Thiago Farina

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

[danakj, 2014-12-19 17:22:07]

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

https://codereview.chromium.org/820703002/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling_set_unittest.cc:37: auto pile =
On 2014/12/19 17:18:11, tfarina wrote:
> I know auto is allowed now that we are using C++11, but I fear it makes code
> harder to read. Without looking at CreateEmptyPileWithDefaultTileSize(), I
have
> no idea what it returns. A pointer, a reference, a value? To what type?

FWIW Foo::Create() methods always return smart pointers in cc/ at least.

> That is what I don't like in languages like Python and Javascript, the lack of
> type safety.

This doesn't change type safety, auto will pick a fixed type.

[tfarina, 2014-12-19 17:27:30]

On Fri, Dec 19, 2014 at 3:22 PM, <danakj@chromium.org> wrote:
>
>
> https://codereview.chromium.org/820703002/diff/1/cc/
> resources/picture_layer_tiling_set_unittest.cc
> File cc/resources/picture_layer_tiling_set_unittest.cc (right):
>
> https://codereview.chromium.org/820703002/diff/1/cc/
> resources/picture_layer_tiling_set_unittest.cc#newcode37
> cc/resources/picture_layer_tiling_set_unittest.cc:37: auto pile =
> On 2014/12/19 17:18:11, tfarina wrote:
>
>> I know auto is allowed now that we are using C++11, but I fear it
>>
> makes code
>
>> harder to read. Without looking at
>>
> CreateEmptyPileWithDefaultTileSize(), I have
>
>> no idea what it returns. A pointer, a reference, a value? To what
>>
> type?
>
> FWIW Foo::Create() methods always return smart pointers in cc/ at least.
>
>  That is what I don't like in languages like Python and Javascript, the
>>
> lack of
>
>> type safety.
>>
>
> This doesn't change type safety, auto will pick a fixed type.
>
> I know the type won't be something random. That is not what I mean. I
mean, without explicit declaring the type, one has to waste time to either
git grep for that function to know what it retuns, or use an IDE (which is
not feasible on Linux) or go to cs.chromium.org.

Just wanted to point this for consideration. It is just my claim for not
making code harder to read, it is already complex (and big) enough.

https://codereview.chromium.org/820703002/
>


-- 
Thiago Farina

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

[danakj, 2014-12-19 17:52:02]

On 2014/12/19 17:27:30, tfarina wrote:
> On Fri, Dec 19, 2014 at 3:22 PM, <mailto:danakj@chromium.org> wrote:
> >
> >
> > https://codereview.chromium.org/820703002/diff/1/cc/
> > resources/picture_layer_tiling_set_unittest.cc
> > File cc/resources/picture_layer_tiling_set_unittest.cc (right):
> >
> > https://codereview.chromium.org/820703002/diff/1/cc/
> > resources/picture_layer_tiling_set_unittest.cc#newcode37
> > cc/resources/picture_layer_tiling_set_unittest.cc:37: auto pile =
> > On 2014/12/19 17:18:11, tfarina wrote:
> >
> >> I know auto is allowed now that we are using C++11, but I fear it
> >>
> > makes code
> >
> >> harder to read. Without looking at
> >>
> > CreateEmptyPileWithDefaultTileSize(), I have
> >
> >> no idea what it returns. A pointer, a reference, a value? To what
> >>
> > type?
> >
> > FWIW Foo::Create() methods always return smart pointers in cc/ at least.
> >
> >  That is what I don't like in languages like Python and Javascript, the
> >>
> > lack of
> >
> >> type safety.
> >>
> >
> > This doesn't change type safety, auto will pick a fixed type.
> >
> > I know the type won't be something random. That is not what I mean. I
> mean, without explicit declaring the type, one has to waste time to either
> git grep for that function to know what it retuns, or use an IDE (which is
> not feasible on Linux) or go to http://cs.chromium.org.

What can Foo::Create() return other than a Foo? It might be a scoped_ptr, and it
might be a scoped_refptr, that's true. But I don't think we care at all in these
tests which it is, we just create it and pass it along. Do you think it matters
when reading the code?

> Just wanted to point this for consideration. It is just my claim for not
> making code harder to read, it is already complex (and big) enough.

I sympathize, I want to optimize for reading, not for writing the code. I'm not
100% sure auto is the right thing to do here, but I'm not sure it isn't either.

  auto set = CreateTilingSet(&client);

I think this one is more ambiguous. There is no TilingSet type, this requires a
bit more knowledge about the codebase to understand it'd be a
PictureLayerTilingSet.

[vmpstr, 2014-12-19 17:59:18]

On 2014/12/19 17:52:02, danakj wrote:
> On 2014/12/19 17:27:30, tfarina wrote:
> > On Fri, Dec 19, 2014 at 3:22 PM, <mailto:danakj@chromium.org> wrote:
> > >
> > >
> > > https://codereview.chromium.org/820703002/diff/1/cc/
> > > resources/picture_layer_tiling_set_unittest.cc
> > > File cc/resources/picture_layer_tiling_set_unittest.cc (right):
> > >
> > > https://codereview.chromium.org/820703002/diff/1/cc/
> > > resources/picture_layer_tiling_set_unittest.cc#newcode37
> > > cc/resources/picture_layer_tiling_set_unittest.cc:37: auto pile =
> > > On 2014/12/19 17:18:11, tfarina wrote:
> > >
> > >> I know auto is allowed now that we are using C++11, but I fear it
> > >>
> > > makes code
> > >
> > >> harder to read. Without looking at
> > >>
> > > CreateEmptyPileWithDefaultTileSize(), I have
> > >
> > >> no idea what it returns. A pointer, a reference, a value? To what
> > >>
> > > type?
> > >
> > > FWIW Foo::Create() methods always return smart pointers in cc/ at least.
> > >
> > >  That is what I don't like in languages like Python and Javascript, the
> > >>
> > > lack of
> > >
> > >> type safety.
> > >>
> > >
> > > This doesn't change type safety, auto will pick a fixed type.
> > >
> > > I know the type won't be something random. That is not what I mean. I
> > mean, without explicit declaring the type, one has to waste time to either
> > git grep for that function to know what it retuns, or use an IDE (which is
> > not feasible on Linux) or go to http://cs.chromium.org.
> 
> What can Foo::Create() return other than a Foo? It might be a scoped_ptr, and
it
> might be a scoped_refptr, that's true. But I don't think we care at all in
these
> tests which it is, we just create it and pass it along. Do you think it
matters
> when reading the code?
> 
> > Just wanted to point this for consideration. It is just my claim for not
> > making code harder to read, it is already complex (and big) enough.
> 
> I sympathize, I want to optimize for reading, not for writing the code. I'm
not
> 100% sure auto is the right thing to do here, but I'm not sure it isn't
either.
> 
>   auto set = CreateTilingSet(&client);
> 
> I think this one is more ambiguous. There is no TilingSet type, this requires
a
> bit more knowledge about the codebase to understand it'd be a
> PictureLayerTilingSet.

Although you're in PictureLayerTilingSetTest, so it's not a stretch to assume it
there. I don't think auto really solves any particular problem, except the
problem of unreadable code. If we're in a PictureLayerTilingSetTest and we are
creating a "set" by calling CreateTilingSet, it's far more readable for me to
have auto there instead of PictureLayerTilingSet, possibly scoped_ptr or
scoped_refptr (I don't even know which one it is, and more importantly it
doesn't matter).

[enne (OOO), 2014-12-19 18:02:55]

lgtm

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

https://codereview.chromium.org/820703002/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling_set_unittest.cc:37: auto pile =
+1 to auto

[tfarina, 2014-12-19 18:11:34]

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

https://codereview.chromium.org/820703002/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling_set_unittest.cc:37: auto pile =
On 2014/12/19 18:02:55, enne wrote:
> +1 to auto
Ouch :(

[danakj, 2014-12-19 18:26:11]

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

https://codereview.chromium.org/820703002/diff/1/cc/resources/picture_layer_t...
cc/resources/picture_layer_tiling_set_unittest.cc:37: auto pile =
On 2014/12/19 18:11:34, tfarina wrote:
> On 2014/12/19 18:02:55, enne wrote:
> > +1 to auto
> Ouch :(

Sorry :/ Thank you for bringing it up, we need to be careful to not go overboard
on auto. I think here the return type is in the function call since it's a
Create() method. A general GetFooBar() seems much less so, it could be a raw
pointer or not, for example.

[danakj, 2014-12-19 18:26:18]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-12-19 18:27:20]

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

[danakj, 2014-12-19 18:35:44]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2014-12-19 19:12:23]

removed autos

[danakj, 2014-12-19 19:12:39]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-12-19 19:13:54]

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

[commit-bot: I haz the power, 2014-12-19 19:23:08]

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

[commit-bot: I haz the power, 2014-12-19 19:23:09]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[danakj, 2014-12-19 19:26:17]

The CQ bit was checked by danakj@chromium.org

[commit-bot: I haz the power, 2014-12-19 19:27:58]

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

[commit-bot: I haz the power, 2014-12-19 20:50:56]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-12-19 20:53:06]

Patchset 5 (id:??) landed as
https://crrev.com/16f38e8107f49d00d6bfe694e33ea61d394aaa7b
Cr-Commit-Position: refs/heads/master@{#309251}