Issue 2141233002: cc: Clean up RecordingSource API

Menglin

Description was changed from ========== cc: Clean up RecordingSource API The RecordingSource isn't directly used ...

4 years, 5 months ago (2016-07-12 21:29:48 UTC) #1

Menglin

mlliu@chromium.org changed reviewers: + khushalsagar@chromium.org

4 years, 5 months ago (2016-07-12 21:31:47 UTC) #2

Khushal

https://codereview.chromium.org/2141233002/diff/1/cc/playback/recording_source.h File cc/playback/recording_source.h (right): https://codereview.chromium.org/2141233002/diff/1/cc/playback/recording_source.h#newcode91 cc/playback/recording_source.h:91: Inputs(); You can inline the initialization and remove the ...

4 years, 5 months ago (2016-07-12 23:28:19 UTC) #4

Menglin

https://codereview.chromium.org/2141233002/diff/1/cc/playback/recording_source.h File cc/playback/recording_source.h (right): https://codereview.chromium.org/2141233002/diff/1/cc/playback/recording_source.h#newcode91 cc/playback/recording_source.h:91: Inputs(); On 2016/07/12 23:28:19, Khushal wrote: > You can ...

4 years, 5 months ago (2016-07-12 23:36:47 UTC) #5

danakj

https://codereview.chromium.org/2141233002/diff/1/cc/playback/recording_source.h File cc/playback/recording_source.h (right): https://codereview.chromium.org/2141233002/diff/1/cc/playback/recording_source.h#newcode91 cc/playback/recording_source.h:91: Inputs(); On 2016/07/12 23:36:47, Menglin wrote: > On 2016/07/12 ...

4 years, 5 months ago (2016-07-12 23:39:43 UTC) #6

Menglin

mlliu@chromium.org changed reviewers: + danakj@chromium.org

4 years, 5 months ago (2016-07-13 00:16:47 UTC) #7

danakj

On 2016/07/13 00:16:48, Menglin wrote: > mailto:danakj@chromium.org: Please review changes. thanks! I can look when ...

4 years, 5 months ago (2016-07-13 00:32:16 UTC) #9

Khushal

On 2016/07/13 00:32:16, danakj wrote: > On 2016/07/13 00:16:48, Menglin wrote: > > mailto:danakj@chromium.org: Please ...

4 years, 5 months ago (2016-07-13 00:46:44 UTC) #10

danakj

https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_source.cc File cc/playback/recording_source.cc (right): https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_source.cc#newcode139 cc/playback/recording_source.cc:139: inputs_.recorded_viewport = new_recorded_viewport; I don't understand the reason behind ...

4 years, 5 months ago (2016-07-13 20:05:34 UTC) #11

Khushal

https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_source.cc File cc/playback/recording_source.cc (right): https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_source.cc#newcode139 cc/playback/recording_source.cc:139: inputs_.recorded_viewport = new_recorded_viewport; On 2016/07/13 20:05:34, danakj wrote: > ...

4 years, 5 months ago (2016-07-13 20:16:51 UTC) #12

danakj

https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_source.cc File cc/playback/recording_source.cc (right): https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_source.cc#newcode139 cc/playback/recording_source.cc:139: inputs_.recorded_viewport = new_recorded_viewport; On 2016/07/13 20:16:50, Khushal wrote: > ...

4 years, 5 months ago (2016-07-13 20:19:34 UTC) #13

Khushal

https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_source.cc File cc/playback/recording_source.cc (right): https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_source.cc#newcode139 cc/playback/recording_source.cc:139: inputs_.recorded_viewport = new_recorded_viewport; On 2016/07/13 20:19:33, danakj wrote: > ...

4 years, 5 months ago (2016-07-13 20:55:16 UTC) #14

danakj

https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_source.cc File cc/playback/recording_source.cc (right): https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_source.cc#newcode139 cc/playback/recording_source.cc:139: inputs_.recorded_viewport = new_recorded_viewport; On 2016/07/13 20:55:16, Khushal wrote: > ...

4 years, 5 months ago (2016-07-14 20:15:17 UTC) #15

https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_s...
File cc/playback/recording_source.cc (right):

https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_s...
cc/playback/recording_source.cc:139: inputs_.recorded_viewport =
new_recorded_viewport;
On 2016/07/13 20:55:16, Khushal wrote:
> On 2016/07/13 20:19:33, danakj wrote:
> > On 2016/07/13 20:16:50, Khushal wrote:
> > > On 2016/07/13 20:05:34, danakj wrote:
> > > > I don't understand the reason behind this CL, can you explain why you're
> > > putting
> > > > things into a struct in this class?
> > > > 
> > > > For eg recorded_viewport is moving to the struct, but size_ isn't? They
> are
> > > both
> > > > set in this method.
> > > > 
> > > > This class isn't even exposed to blink, it goes through picture layer
> right?
> > > 
> > > We want to separate the data PictureLayer gets from ContentLayerClient,
> which
> > it
> > > pulls by going through the recording source. The size comes from the
> > > PictureLayer itself.
> > 
> > Why? They both come from blink.
> 
> On the client side we will mostly be replicating the interaction that cc had
> with blink on the service side. So we will implement a ContentLayerClient and
> when the recording source asks it to paint, we will return the same display
list
> and recorded viewport.
> 
> The size of the layer here comes from the layer during the update layers step
> after it has been received from blink. And we will set all the layer
properties
> when LTH tells its client to UpdateLayerTreeHost.

I find it really strange that this function is setting things in inputs_ and
things not in inputs_ regardless.

> > 
> > > Is there a better way to create that distinction?

I think we can change this a bit more fundamentally, and discussed with vmpstr
about it. We suggest that the inputs_ here should be members on PictureLayerImpl
instead, and move this UpdateAndExpandInvalidation over there.

Then just create the RecordingSource at the time of commit, instead of having it
as a member of PictureLayer. vmpstr believes we could/should work toward just
eliminate the RecordingSource as a class entirely as followup, and I could
believe that would be best.

danakj

danakj@chromium.org changed reviewers: + vmpstr@chromium.org

4 years, 5 months ago (2016-07-14 20:15:26 UTC) #16

vmpstr

On 2016/07/14 20:15:17, danakj wrote: > https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_source.cc > File cc/playback/recording_source.cc (right): > > https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_source.cc#newcode139 > ...

4 years, 5 months ago (2016-07-14 20:21:45 UTC) #18

On 2016/07/14 20:15:17, danakj wrote:
>
https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_s...
> File cc/playback/recording_source.cc (right):
> 
>
https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_s...
> cc/playback/recording_source.cc:139: inputs_.recorded_viewport =
> new_recorded_viewport;
> On 2016/07/13 20:55:16, Khushal wrote:
> > On 2016/07/13 20:19:33, danakj wrote:
> > > On 2016/07/13 20:16:50, Khushal wrote:
> > > > On 2016/07/13 20:05:34, danakj wrote:
> > > > > I don't understand the reason behind this CL, can you explain why
you're
> > > > putting
> > > > > things into a struct in this class?
> > > > > 
> > > > > For eg recorded_viewport is moving to the struct, but size_ isn't?
They
> > are
> > > > both
> > > > > set in this method.
> > > > > 
> > > > > This class isn't even exposed to blink, it goes through picture layer
> > right?
> > > > 
> > > > We want to separate the data PictureLayer gets from ContentLayerClient,
> > which
> > > it
> > > > pulls by going through the recording source. The size comes from the
> > > > PictureLayer itself.
> > > 
> > > Why? They both come from blink.
> > 
> > On the client side we will mostly be replicating the interaction that cc had
> > with blink on the service side. So we will implement a ContentLayerClient
and
> > when the recording source asks it to paint, we will return the same display
> list
> > and recorded viewport.
> > 
> > The size of the layer here comes from the layer during the update layers
step
> > after it has been received from blink. And we will set all the layer
> properties
> > when LTH tells its client to UpdateLayerTreeHost.
> 
> I find it really strange that this function is setting things in inputs_ and
> things not in inputs_ regardless.
> 
> > > 
> > > > Is there a better way to create that distinction?
> 
> I think we can change this a bit more fundamentally, and discussed with vmpstr
> about it. We suggest that the inputs_ here should be members on
PictureLayerImpl
> instead, and move this UpdateAndExpandInvalidation over there.
> 
> Then just create the RecordingSource at the time of commit, instead of having
it
> as a member of PictureLayer. vmpstr believes we could/should work toward just
> eliminate the RecordingSource as a class entirely as followup, and I could
> believe that would be best.

I think that would be awesome if we can eliminate the recording source somehow
:P Alternatively, as a first step, we can simply remove the painter concept from
the recording source. As far as I can tell, we only call functions on it that
don't depend on any state other than the state that the picture layer passes in.
So, picture layer itself can just call the same functions on the client and just
pass whatever results are needed to the recording source. (Again, I think at
that point the recording source would become a fairly thin wrapper around some
data out of which we create a raster source, which in turn means we might be
able to just remove it).

danakj

On Thu, Jul 14, 2016 at 1:21 PM, <vmpstr@chromium.org> wrote: > On 2016/07/14 20:15:17, danakj ...

4 years, 5 months ago (2016-07-14 21:22:00 UTC) #19

On Thu, Jul 14, 2016 at 1:21 PM, <vmpstr@chromium.org> wrote:

> On 2016/07/14 20:15:17, danakj wrote:
> >
>
>
https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_s...
> > File cc/playback/recording_source.cc (right):
> >
> >
>
>
https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_s...
> > cc/playback/recording_source.cc:139: inputs_.recorded_viewport =
> > new_recorded_viewport;
> > On 2016/07/13 20:55:16, Khushal wrote:
> > > On 2016/07/13 20:19:33, danakj wrote:
> > > > On 2016/07/13 20:16:50, Khushal wrote:
> > > > > On 2016/07/13 20:05:34, danakj wrote:
> > > > > > I don't understand the reason behind this CL, can you explain why
> you're
> > > > > putting
> > > > > > things into a struct in this class?
> > > > > >
> > > > > > For eg recorded_viewport is moving to the struct, but size_
> isn't?
> They
> > > are
> > > > > both
> > > > > > set in this method.
> > > > > >
> > > > > > This class isn't even exposed to blink, it goes through picture
> layer
> > > right?
> > > > >
> > > > > We want to separate the data PictureLayer gets from
> ContentLayerClient,
> > > which
> > > > it
> > > > > pulls by going through the recording source. The size comes from
> the
> > > > > PictureLayer itself.
> > > >
> > > > Why? They both come from blink.
> > >
> > > On the client side we will mostly be replicating the interaction that
> cc had
> > > with blink on the service side. So we will implement a
> ContentLayerClient
> and
> > > when the recording source asks it to paint, we will return the same
> display
> > list
> > > and recorded viewport.
> > >
> > > The size of the layer here comes from the layer during the update
> layers
> step
> > > after it has been received from blink. And we will set all the layer
> > properties
> > > when LTH tells its client to UpdateLayerTreeHost.
> >
> > I find it really strange that this function is setting things in inputs_
> and
> > things not in inputs_ regardless.
> >
> > > >
> > > > > Is there a better way to create that distinction?
> >
> > I think we can change this a bit more fundamentally, and discussed with
> vmpstr
> > about it. We suggest that the inputs_ here should be members on
> PictureLayerImpl
> > instead, and move this UpdateAndExpandInvalidation over there.
> >
> > Then just create the RecordingSource at the time of commit, instead of
> having
> it
> > as a member of PictureLayer. vmpstr believes we could/should work toward
> just
> > eliminate the RecordingSource as a class entirely as followup, and I
> could
> > believe that would be best.
>
> I think that would be awesome if we can eliminate the recording source
> somehow
> :P Alternatively, as a first step, we can simply remove the painter
> concept from
> the recording source. As far as I can tell, we only call functions on it
> that
> don't depend on any state other than the state that the picture layer
> passes in.
> So, picture layer itself can just call the same functions on the client
> and just
> pass whatever results are needed to the recording source. (Again, I think
> at
> that point the recording source would become a fairly thin wrapper around
> some
> data out of which we create a raster source, which in turn means we might
> be
> able to just remove it).
>

SGTM

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

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Menglin

On 2016/07/14 20:15:17, danakj wrote: > https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_source.cc > File cc/playback/recording_source.cc (right): > > https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_source.cc#newcode139 > ...

4 years, 5 months ago (2016-07-14 23:33:55 UTC) #20

On 2016/07/14 20:15:17, danakj wrote:
>
https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_s...
> File cc/playback/recording_source.cc (right):
> 
>
https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_s...
> cc/playback/recording_source.cc:139: inputs_.recorded_viewport =
> new_recorded_viewport;
> On 2016/07/13 20:55:16, Khushal wrote:
> > On 2016/07/13 20:19:33, danakj wrote:
> > > On 2016/07/13 20:16:50, Khushal wrote:
> > > > On 2016/07/13 20:05:34, danakj wrote:
> > > > > I don't understand the reason behind this CL, can you explain why
you're
> > > > putting
> > > > > things into a struct in this class?
> > > > > 
> > > > > For eg recorded_viewport is moving to the struct, but size_ isn't?
They
> > are
> > > > both
> > > > > set in this method.
> > > > > 
> > > > > This class isn't even exposed to blink, it goes through picture layer
> > right?
> > > > 
> > > > We want to separate the data PictureLayer gets from ContentLayerClient,
> > which
> > > it
> > > > pulls by going through the recording source. The size comes from the
> > > > PictureLayer itself.
> > > 
> > > Why? They both come from blink.
> > 
> > On the client side we will mostly be replicating the interaction that cc had
> > with blink on the service side. So we will implement a ContentLayerClient
and
> > when the recording source asks it to paint, we will return the same display
> list
> > and recorded viewport.
> > 
> > The size of the layer here comes from the layer during the update layers
step
> > after it has been received from blink. And we will set all the layer
> properties
> > when LTH tells its client to UpdateLayerTreeHost.
> 
> I find it really strange that this function is setting things in inputs_ and
> things not in inputs_ regardless.
> 
> > > 
> > > > Is there a better way to create that distinction?
> 
> I think we can change this a bit more fundamentally, and discussed with vmpstr
> about it. We suggest that the inputs_ here should be members on
PictureLayerImpl
> instead, and move this UpdateAndExpandInvalidation over there.
> 
> Then just create the RecordingSource at the time of commit, instead of having
it
> as a member of PictureLayer. vmpstr believes we could/should work toward just
> eliminate the RecordingSource as a class entirely as followup, and I could
> believe that would be best.

OK. I'll change my next CL according to your suggestion. Thanks!

Menglin

On 2016/07/14 23:33:55, Menglin wrote: > On 2016/07/14 20:15:17, danakj wrote: > > > https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_source.cc ...

4 years, 5 months ago (2016-07-14 23:34:14 UTC) #21

On 2016/07/14 23:33:55, Menglin wrote:
> On 2016/07/14 20:15:17, danakj wrote:
> >
>
https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_s...
> > File cc/playback/recording_source.cc (right):
> > 
> >
>
https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_s...
> > cc/playback/recording_source.cc:139: inputs_.recorded_viewport =
> > new_recorded_viewport;
> > On 2016/07/13 20:55:16, Khushal wrote:
> > > On 2016/07/13 20:19:33, danakj wrote:
> > > > On 2016/07/13 20:16:50, Khushal wrote:
> > > > > On 2016/07/13 20:05:34, danakj wrote:
> > > > > > I don't understand the reason behind this CL, can you explain why
> you're
> > > > > putting
> > > > > > things into a struct in this class?
> > > > > > 
> > > > > > For eg recorded_viewport is moving to the struct, but size_ isn't?
> They
> > > are
> > > > > both
> > > > > > set in this method.
> > > > > > 
> > > > > > This class isn't even exposed to blink, it goes through picture
layer
> > > right?
> > > > > 
> > > > > We want to separate the data PictureLayer gets from
ContentLayerClient,
> > > which
> > > > it
> > > > > pulls by going through the recording source. The size comes from the
> > > > > PictureLayer itself.
> > > > 
> > > > Why? They both come from blink.
> > > 
> > > On the client side we will mostly be replicating the interaction that cc
had
> > > with blink on the service side. So we will implement a ContentLayerClient
> and
> > > when the recording source asks it to paint, we will return the same
display
> > list
> > > and recorded viewport.
> > > 
> > > The size of the layer here comes from the layer during the update layers
> step
> > > after it has been received from blink. And we will set all the layer
> > properties
> > > when LTH tells its client to UpdateLayerTreeHost.
> > 
> > I find it really strange that this function is setting things in inputs_ and
> > things not in inputs_ regardless.
> > 
> > > > 
> > > > > Is there a better way to create that distinction?
> > 
> > I think we can change this a bit more fundamentally, and discussed with
vmpstr
> > about it. We suggest that the inputs_ here should be members on
> PictureLayerImpl
> > instead, and move this UpdateAndExpandInvalidation over there.
> > 
> > Then just create the RecordingSource at the time of commit, instead of
having
> it
> > as a member of PictureLayer. vmpstr believes we could/should work toward
just
> > eliminate the RecordingSource as a class entirely as followup, and I could
> > believe that would be best.
> 
> OK. I'll change my next CL according to your suggestion. Thanks!

I meant next patch

Khushal

On 2016/07/14 20:21:45, vmpstr wrote: > On 2016/07/14 20:15:17, danakj wrote: > > > https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_source.cc ...

4 years, 5 months ago (2016-07-14 23:49:51 UTC) #22

On 2016/07/14 20:21:45, vmpstr wrote:
> On 2016/07/14 20:15:17, danakj wrote:
> >
>
https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_s...
> > File cc/playback/recording_source.cc (right):
> > 
> >
>
https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_s...
> > cc/playback/recording_source.cc:139: inputs_.recorded_viewport =
> > new_recorded_viewport;
> > On 2016/07/13 20:55:16, Khushal wrote:
> > > On 2016/07/13 20:19:33, danakj wrote:
> > > > On 2016/07/13 20:16:50, Khushal wrote:
> > > > > On 2016/07/13 20:05:34, danakj wrote:
> > > > > > I don't understand the reason behind this CL, can you explain why
> you're
> > > > > putting
> > > > > > things into a struct in this class?
> > > > > > 
> > > > > > For eg recorded_viewport is moving to the struct, but size_ isn't?
> They
> > > are
> > > > > both
> > > > > > set in this method.
> > > > > > 
> > > > > > This class isn't even exposed to blink, it goes through picture
layer
> > > right?
> > > > > 
> > > > > We want to separate the data PictureLayer gets from
ContentLayerClient,
> > > which
> > > > it
> > > > > pulls by going through the recording source. The size comes from the
> > > > > PictureLayer itself.
> > > > 
> > > > Why? They both come from blink.
> > > 
> > > On the client side we will mostly be replicating the interaction that cc
had
> > > with blink on the service side. So we will implement a ContentLayerClient
> and
> > > when the recording source asks it to paint, we will return the same
display
> > list
> > > and recorded viewport.
> > > 
> > > The size of the layer here comes from the layer during the update layers
> step
> > > after it has been received from blink. And we will set all the layer
> > properties
> > > when LTH tells its client to UpdateLayerTreeHost.
> > 
> > I find it really strange that this function is setting things in inputs_ and
> > things not in inputs_ regardless.
> > 
> > > > 
> > > > > Is there a better way to create that distinction?
> > 
> > I think we can change this a bit more fundamentally, and discussed with
vmpstr
> > about it. We suggest that the inputs_ here should be members on
> PictureLayerImpl
> > instead, and move this UpdateAndExpandInvalidation over there.
> > 
> > Then just create the RecordingSource at the time of commit, instead of
having
> it
> > as a member of PictureLayer. vmpstr believes we could/should work toward
just
> > eliminate the RecordingSource as a class entirely as followup, and I could
> > believe that would be best.
> 
> I think that would be awesome if we can eliminate the recording source somehow
> :P Alternatively, as a first step, we can simply remove the painter concept
from
> the recording source. As far as I can tell, we only call functions on it that
> don't depend on any state other than the state that the picture layer passes
in.
> So, picture layer itself can just call the same functions on the client and
just
> pass whatever results are needed to the recording source. (Again, I think at
> that point the recording source would become a fairly thin wrapper around some
> data out of which we create a raster source, which in turn means we might be
> able to just remove it).

I like the idea of isolating data bits that come from the
embedder(ContentLayerClient) into PictureLayer. After that you can keep
recording source around as an internal helper class  for PictureLayer to isolate
the data that needs to be computed for the raster source. As long as its just an
internal cc class, whatever is cleaner is good. :)

vmpstr

On 2016/07/14 23:49:51, Khushal wrote: > On 2016/07/14 20:21:45, vmpstr wrote: > > On 2016/07/14 ...

4 years, 5 months ago (2016-07-15 00:26:44 UTC) #23

On 2016/07/14 23:49:51, Khushal wrote:
> On 2016/07/14 20:21:45, vmpstr wrote:
> > On 2016/07/14 20:15:17, danakj wrote:
> > >
> >
>
https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_s...
> > > File cc/playback/recording_source.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2141233002/diff/20001/cc/playback/recording_s...
> > > cc/playback/recording_source.cc:139: inputs_.recorded_viewport =
> > > new_recorded_viewport;
> > > On 2016/07/13 20:55:16, Khushal wrote:
> > > > On 2016/07/13 20:19:33, danakj wrote:
> > > > > On 2016/07/13 20:16:50, Khushal wrote:
> > > > > > On 2016/07/13 20:05:34, danakj wrote:
> > > > > > > I don't understand the reason behind this CL, can you explain why
> > you're
> > > > > > putting
> > > > > > > things into a struct in this class?
> > > > > > > 
> > > > > > > For eg recorded_viewport is moving to the struct, but size_ isn't?
> > They
> > > > are
> > > > > > both
> > > > > > > set in this method.
> > > > > > > 
> > > > > > > This class isn't even exposed to blink, it goes through picture
> layer
> > > > right?
> > > > > > 
> > > > > > We want to separate the data PictureLayer gets from
> ContentLayerClient,
> > > > which
> > > > > it
> > > > > > pulls by going through the recording source. The size comes from the
> > > > > > PictureLayer itself.
> > > > > 
> > > > > Why? They both come from blink.
> > > > 
> > > > On the client side we will mostly be replicating the interaction that cc
> had
> > > > with blink on the service side. So we will implement a
ContentLayerClient
> > and
> > > > when the recording source asks it to paint, we will return the same
> display
> > > list
> > > > and recorded viewport.
> > > > 
> > > > The size of the layer here comes from the layer during the update layers
> > step
> > > > after it has been received from blink. And we will set all the layer
> > > properties
> > > > when LTH tells its client to UpdateLayerTreeHost.
> > > 
> > > I find it really strange that this function is setting things in inputs_
and
> > > things not in inputs_ regardless.
> > > 
> > > > > 
> > > > > > Is there a better way to create that distinction?
> > > 
> > > I think we can change this a bit more fundamentally, and discussed with
> vmpstr
> > > about it. We suggest that the inputs_ here should be members on
> > PictureLayerImpl
> > > instead, and move this UpdateAndExpandInvalidation over there.
> > > 
> > > Then just create the RecordingSource at the time of commit, instead of
> having
> > it
> > > as a member of PictureLayer. vmpstr believes we could/should work toward
> just
> > > eliminate the RecordingSource as a class entirely as followup, and I could
> > > believe that would be best.
> > 
> > I think that would be awesome if we can eliminate the recording source
somehow
> > :P Alternatively, as a first step, we can simply remove the painter concept
> from
> > the recording source. As far as I can tell, we only call functions on it
that
> > don't depend on any state other than the state that the picture layer passes
> in.
> > So, picture layer itself can just call the same functions on the client and
> just
> > pass whatever results are needed to the recording source. (Again, I think at
> > that point the recording source would become a fairly thin wrapper around
some
> > data out of which we create a raster source, which in turn means we might be
> > able to just remove it).
> 
> I like the idea of isolating data bits that come from the
> embedder(ContentLayerClient) into PictureLayer. After that you can keep
> recording source around as an internal helper class  for PictureLayer to
isolate
> the data that needs to be computed for the raster source. As long as its just
an
> internal cc class, whatever is cleaner is good. :)

Yeah, I agree. I think the bulk of the work will be updating various tests that
rely on a concept of a recording source in order to produce a raster source. I
do think it's work worth doing though.

Menglin

Description was changed from ========== cc: Clean up RecordingSource API The RecordingSource isn't directly used ...

4 years, 5 months ago (2016-07-19 22:43:39 UTC) #24

Menglin

Hi all, I update another patch. This patch only has the dev code change, no ...

4 years, 5 months ago (2016-07-19 22:48:47 UTC) #25

Menglin

Oh and you don't need to look at picture_layer_impl_unittest.cc, picture_layer_unittest.cc, discardable_image_map_unittest.cc, fake_recording_source.cc, and fake_recording_source.h, because ...

4 years, 5 months ago (2016-07-19 22:56:37 UTC) #26

vmpstr

I think this is a really good direction. RecordingSource can probably be eliminated as well, ...

4 years, 5 months ago (2016-07-19 23:43:22 UTC) #27

Menglin

Hi vmpstr@, I replied to three of your comments on your questions. For the rest, ...

4 years, 5 months ago (2016-07-20 00:18:14 UTC) #28

Hi vmpstr@,

I replied to three of your comments on your questions. For the rest, I totally
agree and will do the change in my next patch.

Thanks!

Menglin

https://codereview.chromium.org/2141233002/diff/40001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/2141233002/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:61: std::unique_ptr<RecordingSource>
recording_source(
On 2016/07/19 23:43:22, vmpstr wrote:
> Just create this on the stack if you need it. Do you need it? Can we have a
> CreateRasterSource function that bypasses a recording source? Or is there some
> non trivial work that happens there?

Currently the raster source is constructed based on a recording source.
RasterSource(const RecordingSource* other, bool can_use_lcd_text). I can add
something like RasterSource(const PictureLayer* other, bool can_use_lcd_text) so
that it can be created from a PictureLayer, bypassing a recording source, and we
don't need the recording source at all. It's not too much work to do that

https://codereview.chromium.org/2141233002/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:77: pl_data_.reset();
On 2016/07/19 23:43:22, vmpstr wrote:
> Where is PictureLayerData defined? I can't find it anywhere. 
> 
Now it's in recording_source.h. I had it there because the RecordingSource
constructor needs to know those types. If we remove the whole recording source,
it'll be moved in picture_layer.h

> From the uses around here, it doesn't appear to be a pointer, so it shouldn't
> have a lower case reset function. Maybe just pl_data_ = PictureLayerData();?
if
> you need to clear it?
It's a struct. and yeah i can just use the constructor to clear it. Thanks for
letting me know the code style

https://codereview.chromium.org/2141233002/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:355: void
PictureLayer::SetSlowdownRasterScaleFactor(int factor) {
On 2016/07/19 23:43:22, vmpstr wrote:
> As far as I can tell there's not a lot of places that call these setters. Can
we
> just set the variable directly without introducing these functions?

OK. I will check each one of them, and do the clean up.

vmpstr

https://codereview.chromium.org/2141233002/diff/40001/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/2141233002/diff/40001/cc/layers/picture_layer.cc#newcode61 cc/layers/picture_layer.cc:61: std::unique_ptr<RecordingSource> recording_source( On 2016/07/20 00:18:13, Menglin wrote: > On ...

4 years, 5 months ago (2016-07-20 00:27:06 UTC) #29

Menglin

On 2016/07/20 00:27:06, vmpstr wrote: > https://codereview.chromium.org/2141233002/diff/40001/cc/layers/picture_layer.cc > File cc/layers/picture_layer.cc (right): > > https://codereview.chromium.org/2141233002/diff/40001/cc/layers/picture_layer.cc#newcode61 > ...

4 years, 5 months ago (2016-07-20 00:28:32 UTC) #30

Menglin

https://codereview.chromium.org/2141233002/diff/40001/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/2141233002/diff/40001/cc/layers/picture_layer.cc#newcode37 cc/layers/picture_layer.cc:37: pl_data_ = pl_data; On 2016/07/19 23:43:22, vmpstr wrote: > ...

4 years, 5 months ago (2016-07-20 00:28:42 UTC) #31

Menglin

Hello, Before I start to touch the test code, I want to upload this new ...

4 years, 5 months ago (2016-07-20 21:25:22 UTC) #32

Hello,

Before I start to touch the test code, I want to upload this new patch on the
dev code, in which the RecordingSource is totally eliminated.

As for the test code, I'm planning to remove FakeRecordingSource, and move
whatever necessary to FakePictureLayer. Tons of code change coming up!

Thanks

Menglin

https://codereview.chromium.org/2141233002/diff/40001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/2141233002/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:37: pl_data_ = pl_data;
On 2016/07/20 00:28:42, Menglin wrote:
> On 2016/07/19 23:43:22, vmpstr wrote:
> > set these in the initialization list just above
> 
> Done.

Actually, if i move the initialization in the list, it'll have this compile
error "an initializer for a delegating constructor must appear alone", because i
guess when I delegate the member initialization to another constructor, there is
an assumption that the other constructor initializes the object completely,
including all members. I can't therefore initialize any of the members again.

https://codereview.chromium.org/2141233002/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:61: std::unique_ptr<RecordingSource>
recording_source(
On 2016/07/20 00:27:06, vmpstr wrote:
> On 2016/07/20 00:18:13, Menglin wrote:
> > On 2016/07/19 23:43:22, vmpstr wrote:
> > > Just create this on the stack if you need it. Do you need it? Can we have
a
> > > CreateRasterSource function that bypasses a recording source? Or is there
> some
> > > non trivial work that happens there?
> > 
> > Currently the raster source is constructed based on a recording source.
> > RasterSource(const RecordingSource* other, bool can_use_lcd_text). I can add
> > something like RasterSource(const PictureLayer* other, bool
can_use_lcd_text)
> so
> > that it can be created from a PictureLayer, bypassing a recording source,
and
> we
> > don't need the recording source at all. It's not too much work to do that
> 
> I think we can just have RasterSource(display_list, background_color,
> requires_clear, is_solid_color, ...) and whatever else we need. All the raster
> source's current ctor is doing is unpacking the recording source into those
> variables. We might as well just pass the variables. 

I change it to RasterSource(const PictureLayerData& layer_data,
               const ContentLayerClientData& client_data,
               bool can_use_lcd_text);
because putting each single member as an argument makes the function declaration
look very bulky..

https://codereview.chromium.org/2141233002/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:142: InvalidationRegion invalidation_local;
On 2016/07/19 23:43:22, vmpstr wrote:
> I don't think you need the word "local".. lack of training underscore implies
> that it's local. 

Done.

https://codereview.chromium.org/2141233002/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:148: std::unique_ptr<RecordingSource>
recording_source(
On 2016/07/19 23:43:22, vmpstr wrote:
> Same comment as above: Either create it on the stack or just don't create it
:)

Done.

https://codereview.chromium.org/2141233002/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:355: void
PictureLayer::SetSlowdownRasterScaleFactor(int factor) {
On 2016/07/20 00:18:13, Menglin wrote:
> On 2016/07/19 23:43:22, vmpstr wrote:
> > As far as I can tell there's not a lot of places that call these setters.
Can
> we
> > just set the variable directly without introducing these functions?
> 
> OK. I will check each one of them, and do the clean up.

For now I only removed SetSlowdownRasterScaleFactor, because the others are used
in the test code. I'll deal with the rest when I change the test code in the
next patch

https://codereview.chromium.org/2141233002/diff/40001/cc/layers/picture_layer.h
File cc/layers/picture_layer.h (right):

https://codereview.chromium.org/2141233002/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer.h:47: PictureLayerData* pl_data,
On 2016/07/19 23:43:22, vmpstr wrote:
> layer_data or picture_layer_data (here and everywhere)

Done. I'm very bad at names...

https://codereview.chromium.org/2141233002/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer.h:48: ContentLayerClientData* clc_data,
On 2016/07/19 23:43:22, vmpstr wrote:
> client_data (here and everywhere)

Done.

https://codereview.chromium.org/2141233002/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer.h:51: void SetEmptyBounds();
On 2016/07/19 23:43:22, vmpstr wrote:
> This is confusing since cc::Layer has a SetBounds function. It seems like
> SetEmptyBounds would be SetBounds(gfx::Size()), but it isn't.

change it to ResetLayerAndClientData

https://codereview.chromium.org/2141233002/diff/40001/cc/layers/picture_layer...
cc/layers/picture_layer.h:57: gfx::Size GetSize() const;
On 2016/07/19 23:43:22, vmpstr wrote:
> Some of these concept become ambiguous/weird when talking about a Layer
instead
> of a RasterSource... like "Clear" and "GetSize"... I'm not sure what they mean
> when applied to a layer. Can you either eliminate some of these calls (as I
> commented later), or rename the function and put a comment explaining what it
> does?

removed Clear. change GetSize to GetSizeInLayerData

vmpstr

https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer.cc#newcode85 cc/layers/picture_layer.cc:85: layer_data_ = PictureLayerData(); What's the difference between this and ...

4 years, 5 months ago (2016-07-20 22:18:10 UTC) #33

Menglin

Hi vmpstr@, thanks for your comments i replied to them. https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer.cc#newcode85 ...

4 years, 5 months ago (2016-07-20 22:39:31 UTC) #34

Hi vmpstr@, thanks for your comments i replied to them.

https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:85: layer_data_ = PictureLayerData();
On 2016/07/20 22:18:10, vmpstr wrote:
> What's the difference between this and ResetLayerAndClientData()?
(Specifically,
> why doesn't reset clear everything like this is doing)

ResetLayerAndClientData() is essentially 
  layer_data_.size = gfx::Size();
  client_data_.recorded_viewport = gfx::Rect();
  client_data_.display_list = nullptr;
  client_data_.painter_reported_memory_usage = 0;
  layer_data_.is_solid_color = false;

it resets some members in both structs so i give it that name, need wisdom on a
better name

https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:97: SetNeedsDisplayRectSimple(layer_rect);
On 2016/07/20 22:18:10, vmpstr wrote:
> Can this function go away as well and just be replaced by the code? (I don't
> really like SetNeedsDisplayRectSimple as the name...)

we probably want to keep it since the test code uses it. but i'll think about a
better name

https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:123: &last_updated_invalidation_, layer_size,
PictureLayer::RECORD_NORMALLY,
On 2016/07/20 22:18:10, vmpstr wrote:
> Do we ever pass something other than "RECORD_NORMALLY"? Sorry I haven't
checked
> all the call sites. 

I checked, it always pass RECORD_NORMALLY

https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:124: &layer_data_, &client_data_, &invalidation_);
On 2016/07/20 22:18:10, vmpstr wrote:
> In general, if we can eliminate some of these parameters and just use the
member
> variables, it would be preferable (I'm not sure if it's possible or not). 

but that doesn't work in GetPicture, where it receives the local variables
instead of member variables

https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:150: InvalidationRegion invalidation_regision;
On 2016/07/20 22:18:10, vmpstr wrote:
> s/regision/region/

ok

https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:155: &client_data, &invalidation_regision);
Here it needs the local variables

https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer.h
File cc/layers/picture_layer.h (right):

https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer.h:14: namespace {
On 2016/07/20 22:18:10, vmpstr wrote:
> I don't think anonymous namespace in a header file is useful. This variable
> would still be visible by any file that includes this header. Can we move this
> to the .cc file, even if it means that the ctor will actually have to
initialize
> this variable?

OK.

https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer.h:31: struct PictureLayerData {
On 2016/07/20 22:18:10, vmpstr wrote:
> These should probably move to separate files. 

Some header file just for these two structs?

https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer.h:35: gfx::Size size;
On 2016/07/20 22:18:10, vmpstr wrote:
> Can you rename this to something more meaningful?
> 
> PictureLayerData::size makes me think this is layer size? Is it? Or is this
the
> recorded region size?
> 
> Or maybe we can rename PictureLayerData to something... hmm Maybe
> PictureLayerRasterData? I'm not too sure... danakj@, do you have an opinion
> here?

Looking at code here, I think it's the recorded region size
https://cs.chromium.org/chromium/src/cc/playback/recording_source.cc?dr=CSs&s...

Maybe I can leave the member name as it is, but only change the struct name, if
we can get a proper struct name

vmpstr

https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer.cc#newcode85 cc/layers/picture_layer.cc:85: layer_data_ = PictureLayerData(); On 2016/07/20 22:39:31, Menglin wrote: > ...

4 years, 5 months ago (2016-07-20 23:55:55 UTC) #35

https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer.cc
File cc/layers/picture_layer.cc (right):

https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:85: layer_data_ = PictureLayerData();
On 2016/07/20 22:39:31, Menglin wrote:
> On 2016/07/20 22:18:10, vmpstr wrote:
> > What's the difference between this and ResetLayerAndClientData()?
> (Specifically,
> > why doesn't reset clear everything like this is doing)
> 
> ResetLayerAndClientData() is essentially 
>   layer_data_.size = gfx::Size();
>   client_data_.recorded_viewport = gfx::Rect();
>   client_data_.display_list = nullptr;
>   client_data_.painter_reported_memory_usage = 0;
>   layer_data_.is_solid_color = false;
> 
> it resets some members in both structs so i give it that name, need wisdom on
a
> better name

Hmm... It certainly isn't reset if it's only resetting partial stuff /o\

https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:97: SetNeedsDisplayRectSimple(layer_rect);
On 2016/07/20 22:39:31, Menglin wrote:
> On 2016/07/20 22:18:10, vmpstr wrote:
> > Can this function go away as well and just be replaced by the code? (I don't
> > really like SetNeedsDisplayRectSimple as the name...)
> 
> we probably want to keep it since the test code uses it. but i'll think about
a
> better name

Thanks! FWIW, we sometimes have FooInternal when the function is called Foo, but
those should be strictly private. If the test calls it, then this needs
something else

https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:124: &layer_data_, &client_data_, &invalidation_);
On 2016/07/20 22:39:31, Menglin wrote:
> On 2016/07/20 22:18:10, vmpstr wrote:
> > In general, if we can eliminate some of these parameters and just use the
> member
> > variables, it would be preferable (I'm not sure if it's possible or not). 
> 
> but that doesn't work in GetPicture, where it receives the local variables
> instead of member variables 

Oh yeah, good point.

https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer.cc:155: &client_data, &invalidation_regision);
On 2016/07/20 22:39:31, Menglin wrote:
> Here it needs the local variables

Acknowledged.

https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer.h
File cc/layers/picture_layer.h (right):

https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer.h:31: struct PictureLayerData {
On 2016/07/20 22:39:31, Menglin wrote:
> On 2016/07/20 22:18:10, vmpstr wrote:
> > These should probably move to separate files. 
> 
> Some header file just for these two structs?

Or even a header per... The rule is usually one class per file.. See here for
example
https://cs.chromium.org/chromium/src/cc/playback/draw_image.h?q=draw_image&sq...

draw_image is also very small, but it has it's own .h/.cc

https://codereview.chromium.org/2141233002/diff/60001/cc/layers/picture_layer...
cc/layers/picture_layer.h:35: gfx::Size size;
On 2016/07/20 22:39:31, Menglin wrote:
> On 2016/07/20 22:18:10, vmpstr wrote:
> > Can you rename this to something more meaningful?
> > 
> > PictureLayerData::size makes me think this is layer size? Is it? Or is this
> the
> > recorded region size?
> > 
> > Or maybe we can rename PictureLayerData to something... hmm Maybe
> > PictureLayerRasterData? I'm not too sure... danakj@, do you have an opinion
> > here?
> 
> Looking at code here, I think it's the recorded region size
>
https://cs.chromium.org/chromium/src/cc/playback/recording_source.cc?dr=CSs&s...
> 
> Maybe I can leave the member name as it is, but only change the struct name,
if
> we can get a proper struct name

Yeah I think that's a good idea.

Khushal

I'm wondering if its really advantageous to remove the recording source and stash all of ...

4 years, 5 months ago (2016-07-21 00:42:41 UTC) #36

vmpstr

On 2016/07/21 00:42:41, Khushal wrote: > I'm wondering if its really advantageous to remove the ...

4 years, 5 months ago (2016-07-21 18:22:21 UTC) #37

Menglin

On 2016/07/21 18:22:21, vmpstr wrote: > On 2016/07/21 00:42:41, Khushal wrote: > > I'm wondering ...

4 years, 5 months ago (2016-07-21 18:36:05 UTC) #38

On 2016/07/21 18:22:21, vmpstr wrote:
> On 2016/07/21 00:42:41, Khushal wrote:
> > I'm wondering if its really advantageous to remove the recording source and
> > stash all of it in PictureLayer and crowd that class more.
> > 
> > Here is another suggestion. Could we have the ContentLayerClientData struct
> live
> > in PictureLayer and just have the recording source hold a reference to it
from
> > PictureLayer? You can have it as an inner class of PictureLayer as well if
> that
> > is better.
> > 
> > Also, is there an advantage of removing RecordingSource from RasterSource?
Its
> > just copying the data over?
> 
> We can keep the raster source if that makes the code changes smaller or the
code
I think you meant "recording source" instead of "raster source" here?

> simpler. I'm not really against that. Can we at least pass any of the
> ContentLayerClientData into UpdateAndExpandInvalidation instead? (and while
> we're at it, maybe we can rename that function, since it does more than just
> expand an invalidation: it actually updates the display list as well).
> Basically, if we're going to keep RecordingSource, I'd like to just hold on to
> data that is relevant to being a RecordingSource. ContentLayerClientData
doesn't
> seem to belong there, since it's not data that recording source needs, it's
data
> that the recording source uses to extract the data it needs... right?

I agree ContentLayerClientData doesn't belong to RecordingSource, though in
UpdateAndExpandInvalidation, ContentLayerClientData gets updated (e.g., display
list, recorded viewport are updated). I think Khushal's comment is a feasible
way to achieve the goal of making sure RecordingSource doesn't hold onto
ContentLayerClientData.

Basically, (1) we have RecordingSource as an inner class of PictureLayer, (2)
ContentLayerClientData owned by picture layer, (3) RecordingSource holds a
reference (maybe a pointer) to ContentLayerClientData. It can do this because
now it's the inner class, (4) when ContentLayerClientData should be updated in
UpdateAndExpandInvalidation, it'll use the pointer to refer
ContentLayerClientData.

I agree it's better to change UpdateAndExpandInvalidation to a different name

What do you think?

vmpstr

On 2016/07/21 18:36:05, Menglin wrote: > On 2016/07/21 18:22:21, vmpstr wrote: > > On 2016/07/21 ...

4 years, 5 months ago (2016-07-21 18:53:08 UTC) #39

On 2016/07/21 18:36:05, Menglin wrote:
> On 2016/07/21 18:22:21, vmpstr wrote:
> > On 2016/07/21 00:42:41, Khushal wrote:
> > > I'm wondering if its really advantageous to remove the recording source
and
> > > stash all of it in PictureLayer and crowd that class more.
> > > 
> > > Here is another suggestion. Could we have the ContentLayerClientData
struct
> > live
> > > in PictureLayer and just have the recording source hold a reference to it
> from
> > > PictureLayer? You can have it as an inner class of PictureLayer as well if
> > that
> > > is better.
> > > 
> > > Also, is there an advantage of removing RecordingSource from RasterSource?
> Its
> > > just copying the data over?
> > 
> > We can keep the raster source if that makes the code changes smaller or the
> code
> I think you meant "recording source" instead of "raster source" here?

Err, yeah recording source. 

> 
> > simpler. I'm not really against that. Can we at least pass any of the
> > ContentLayerClientData into UpdateAndExpandInvalidation instead? (and while
> > we're at it, maybe we can rename that function, since it does more than just
> > expand an invalidation: it actually updates the display list as well).
> > Basically, if we're going to keep RecordingSource, I'd like to just hold on
to
> > data that is relevant to being a RecordingSource. ContentLayerClientData
> doesn't
> > seem to belong there, since it's not data that recording source needs, it's
> data
> > that the recording source uses to extract the data it needs... right?
> 
> I agree ContentLayerClientData doesn't belong to RecordingSource, though in
> UpdateAndExpandInvalidation, ContentLayerClientData gets updated (e.g.,
display
> list, recorded viewport are updated). I think Khushal's comment is a feasible
> way to achieve the goal of making sure RecordingSource doesn't hold onto
> ContentLayerClientData.

What I was suggesting here is that display list and recorded viewport don't
depend on any state within the recording source; so can they be passed in
instead?

> 
> Basically, (1) we have RecordingSource as an inner class of PictureLayer, (2)
> ContentLayerClientData owned by picture layer, (3) RecordingSource holds a
> reference (maybe a pointer) to ContentLayerClientData. It can do this because
> now it's the inner class, (4) when ContentLayerClientData should be updated in
> UpdateAndExpandInvalidation, it'll use the pointer to refer
> ContentLayerClientData.

I'm not sure I follow your reasoning about why we can hold
ContentLayerClientData in RecordingSource? I guess this kinda goes back to the
root question of why do we need ContentLayerClientData? Like what is so special
about this data that it needs to be wrapped in an extra layer of structs?

The reason I don't like having a pointer to the data because then it looks like
the raster source is getting its contents purely from the recording source, but
in reality it's also reaching into the layer via a pointer. It's a bit subtle. 

> 
> I agree it's better to change UpdateAndExpandInvalidation to a different name
> 
> What do you think?

danakj reminded me that "Update" in this case refers to the raster source not
the invalidation, but kind of looks like it refers to the invalidation. danakj
also proposes that we change the word "Update" to be "Paint", which I think is a
good idea.

Menglin

I'm answering with the best of my understanding of this change since i just started ...

4 years, 5 months ago (2016-07-21 19:42:40 UTC) #40

I'm answering with the best of my understanding of this change since i just
started to pick up. please excuse me if anything i say sounds obviously wrong to
you.

On 2016/07/21 18:53:08, vmpstr wrote:
> On 2016/07/21 18:36:05, Menglin wrote:
> > On 2016/07/21 18:22:21, vmpstr wrote:
> > > On 2016/07/21 00:42:41, Khushal wrote:
> > > > I'm wondering if its really advantageous to remove the recording source
> and
> > > > stash all of it in PictureLayer and crowd that class more.
> > > > 
> > > > Here is another suggestion. Could we have the ContentLayerClientData
> struct
> > > live
> > > > in PictureLayer and just have the recording source hold a reference to
it
> > from
> > > > PictureLayer? You can have it as an inner class of PictureLayer as well
if
> > > that
> > > > is better.
> > > > 
> > > > Also, is there an advantage of removing RecordingSource from
RasterSource?
> > Its
> > > > just copying the data over?
> > > 
> > > We can keep the raster source if that makes the code changes smaller or
the
> > code
> > I think you meant "recording source" instead of "raster source" here?
> 
> Err, yeah recording source. 
> 
> > 
> > > simpler. I'm not really against that. Can we at least pass any of the
> > > ContentLayerClientData into UpdateAndExpandInvalidation instead? (and
while
> > > we're at it, maybe we can rename that function, since it does more than
just
> > > expand an invalidation: it actually updates the display list as well).
> > > Basically, if we're going to keep RecordingSource, I'd like to just hold
on
> to
> > > data that is relevant to being a RecordingSource. ContentLayerClientData
> > doesn't
> > > seem to belong there, since it's not data that recording source needs,
it's
> > data
> > > that the recording source uses to extract the data it needs... right?
> > 
> > I agree ContentLayerClientData doesn't belong to RecordingSource, though in
> > UpdateAndExpandInvalidation, ContentLayerClientData gets updated (e.g.,
> display
> > list, recorded viewport are updated). I think Khushal's comment is a
feasible
> > way to achieve the goal of making sure RecordingSource doesn't hold onto
> > ContentLayerClientData.
> 
> What I was suggesting here is that display list and recorded viewport don't
> depend on any state within the recording source; so can they be passed in
> instead?

I'd agree that. I think the benefit of holding a reference of
ContentLayerClientData in RecordingSource is we almost don't have to change any
methods in RecordingSource. The methods I have in minds are like
GetDisplayItemList() and IsSuitableForGpuRasterization(). But you're right it's
a bit subtle when it appears RasterSource is copying data from RecordingSource,
it's really fetching the data using the pointer. If we pass
ContentLayerClientData to UpdateAndExpandInvalidation instead, then should we
add a constructor RasterSource(PictureLayer*, RecordingSource*, bool) ?
> 
> > 
> > Basically, (1) we have RecordingSource as an inner class of PictureLayer,
(2)
> > ContentLayerClientData owned by picture layer, (3) RecordingSource holds a
> > reference (maybe a pointer) to ContentLayerClientData. It can do this
because
> > now it's the inner class, (4) when ContentLayerClientData should be updated
in
> > UpdateAndExpandInvalidation, it'll use the pointer to refer
> > ContentLayerClientData.
> 
> I'm not sure I follow your reasoning about why we can hold
> ContentLayerClientData in RecordingSource? I guess this kinda goes back to the
> root question of why do we need ContentLayerClientData? Like what is so
special
> about this data that it needs to be wrapped in an extra layer of structs?

We want to separate the data PictureLayer gets from ContentLayerClient, which it
pulls by going through the recording source. The size comes from the
PictureLayer itself.

On the client side we will mostly be replicating the interaction that cc had
with blink on the service side. So we will implement a ContentLayerClient and
when the recording source asks it to paint, we will return the same display list
and recorded viewport.

The size of the layer here comes from the layer during the update layers step
after it has been received from blink. And we will set all the layer properties
when LTH tells its client to UpdateLayerTreeHost.



> 
> The reason I don't like having a pointer to the data because then it looks
like
> the raster source is getting its contents purely from the recording source,
but
> in reality it's also reaching into the layer via a pointer. It's a bit subtle.

> 
> > 
> > I agree it's better to change UpdateAndExpandInvalidation to a different
name
> > 
> > What do you think?
> 
> danakj reminded me that "Update" in this case refers to the raster source not
> the invalidation, but kind of looks like it refers to the invalidation. danakj
> also proposes that we change the word "Update" to be "Paint", which I think is
a
> good idea.

sg

vmpstr

On 2016/07/21 19:42:40, Menglin wrote: > I'm answering with the best of my understanding of ...

4 years, 5 months ago (2016-07-21 22:35:15 UTC) #41

On 2016/07/21 19:42:40, Menglin wrote:
> I'm answering with the best of my understanding of this change since i just
> started to pick up. please excuse me if anything i say sounds obviously wrong
to
> you.
> 
> On 2016/07/21 18:53:08, vmpstr wrote:
> > On 2016/07/21 18:36:05, Menglin wrote:
> > > On 2016/07/21 18:22:21, vmpstr wrote:
> > > > On 2016/07/21 00:42:41, Khushal wrote:
> > > > > I'm wondering if its really advantageous to remove the recording
source
> > and
> > > > > stash all of it in PictureLayer and crowd that class more.
> > > > > 
> > > > > Here is another suggestion. Could we have the ContentLayerClientData
> > struct
> > > > live
> > > > > in PictureLayer and just have the recording source hold a reference to
> it
> > > from
> > > > > PictureLayer? You can have it as an inner class of PictureLayer as
well
> if
> > > > that
> > > > > is better.
> > > > > 
> > > > > Also, is there an advantage of removing RecordingSource from
> RasterSource?
> > > Its
> > > > > just copying the data over?
> > > > 
> > > > We can keep the raster source if that makes the code changes smaller or
> the
> > > code
> > > I think you meant "recording source" instead of "raster source" here?
> > 
> > Err, yeah recording source. 
> > 
> > > 
> > > > simpler. I'm not really against that. Can we at least pass any of the
> > > > ContentLayerClientData into UpdateAndExpandInvalidation instead? (and
> while
> > > > we're at it, maybe we can rename that function, since it does more than
> just
> > > > expand an invalidation: it actually updates the display list as well).
> > > > Basically, if we're going to keep RecordingSource, I'd like to just hold
> on
> > to
> > > > data that is relevant to being a RecordingSource. ContentLayerClientData
> > > doesn't
> > > > seem to belong there, since it's not data that recording source needs,
> it's
> > > data
> > > > that the recording source uses to extract the data it needs... right?
> > > 
> > > I agree ContentLayerClientData doesn't belong to RecordingSource, though
in
> > > UpdateAndExpandInvalidation, ContentLayerClientData gets updated (e.g.,
> > display
> > > list, recorded viewport are updated). I think Khushal's comment is a
> feasible
> > > way to achieve the goal of making sure RecordingSource doesn't hold onto
> > > ContentLayerClientData.
> > 
> > What I was suggesting here is that display list and recorded viewport don't
> > depend on any state within the recording source; so can they be passed in
> > instead?
> 
> I'd agree that. I think the benefit of holding a reference of
> ContentLayerClientData in RecordingSource is we almost don't have to change
any
> methods in RecordingSource. The methods I have in minds are like
> GetDisplayItemList() and IsSuitableForGpuRasterization(). But you're right
it's
> a bit subtle when it appears RasterSource is copying data from
RecordingSource,
> it's really fetching the data using the pointer. If we pass
> ContentLayerClientData to UpdateAndExpandInvalidation instead, then should we
> add a constructor RasterSource(PictureLayer*, RecordingSource*, bool) ?

> > 
> > > 
> > > Basically, (1) we have RecordingSource as an inner class of PictureLayer,
> (2)
> > > ContentLayerClientData owned by picture layer, (3) RecordingSource holds a
> > > reference (maybe a pointer) to ContentLayerClientData. It can do this
> because
> > > now it's the inner class, (4) when ContentLayerClientData should be
updated
> in
> > > UpdateAndExpandInvalidation, it'll use the pointer to refer
> > > ContentLayerClientData.
> > 
> > I'm not sure I follow your reasoning about why we can hold
> > ContentLayerClientData in RecordingSource? I guess this kinda goes back to
the
> > root question of why do we need ContentLayerClientData? Like what is so
> special
> > about this data that it needs to be wrapped in an extra layer of structs?
> 
> We want to separate the data PictureLayer gets from ContentLayerClient, which
it
> pulls by going through the recording source. The size comes from the
> PictureLayer itself.

So I think, we can still have ContentLayerClientData, but set that on the
layer.. Basically, here's what I'm proposing:

class PictureLayer {
 ...
 RecordingSource recording_source_;
 ContentLayerClientData client_data_;
};

PictureLayer::Update() {
 ... // figure out |painting_control|
 client_data_.recorded_viewport = inputs_.client->PaintableRegion();
 client_data_.display_list =
inputs_client->PaintContentsToDisplayList(painting_control);
 recording_source_.PaintAndUpdateInvalidation(client_data_.recorded_viewport,
client_data_.display_list, ...);
}

but then at that point, my question would be "why have client_data_ instead of
just local variables"


> 
> On the client side we will mostly be replicating the interaction that cc had
> with blink on the service side. So we will implement a ContentLayerClient and
> when the recording source asks it to paint, we will return the same display
list
> and recorded viewport.
> 
> The size of the layer here comes from the layer during the update layers step
> after it has been received from blink. And we will set all the layer
properties
> when LTH tells its client to UpdateLayerTreeHost.
> 
> 
> 
> > 
> > The reason I don't like having a pointer to the data because then it looks
> like
> > the raster source is getting its contents purely from the recording source,
> but
> > in reality it's also reaching into the layer via a pointer. It's a bit
subtle.
> 
> > 
> > > 
> > > I agree it's better to change UpdateAndExpandInvalidation to a different
> name
> > > 
> > > What do you think?
> > 
> > danakj reminded me that "Update" in this case refers to the raster source
not
> > the invalidation, but kind of looks like it refers to the invalidation.
danakj
> > also proposes that we change the word "Update" to be "Paint", which I think
is
> a
> > good idea.
> 
> sg

Khushal

No, you're right. No need for the separate struct, I said that because I thought ...

4 years, 5 months ago (2016-07-22 17:01:04 UTC) #42

Khushal

Also, SetNeedsDisplayRect can also go away from RecordingSource into PictureLayer. So then recording source doesn't ...

4 years, 5 months ago (2016-07-22 17:11:35 UTC) #43

Menglin

On 2016/07/22 17:01:04, Khushal wrote: > No, you're right. No need for the separate struct, ...

4 years, 5 months ago (2016-07-22 18:02:50 UTC) #44

Menglin

Hi all, In patch 5, recorded_viewport, display_list and painter_reported_memory_usage are moved to PictureLayer. UpdateAndExpandInvalidation is ...

4 years, 4 months ago (2016-07-25 18:38:52 UTC) #45

vmpstr

Sorry to keep going in circles, but I think if we make a recording source ...

4 years, 4 months ago (2016-07-25 20:29:14 UTC) #46

Menglin

https://codereview.chromium.org/2141233002/diff/80001/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/2141233002/diff/80001/cc/layers/picture_layer.cc#newcode130 cc/layers/picture_layer.cc:130: old_recorded_viewport, inputs_.recorded_viewport, inputs_.display_list); On 2016/07/25 20:29:14, vmpstr wrote: > ...

4 years, 4 months ago (2016-07-25 20:41:08 UTC) #47

vmpstr

On 2016/07/25 20:41:08, Menglin wrote: > https://codereview.chromium.org/2141233002/diff/80001/cc/layers/picture_layer.cc > File cc/layers/picture_layer.cc (right): > > https://codereview.chromium.org/2141233002/diff/80001/cc/layers/picture_layer.cc#newcode130 > ...

4 years, 4 months ago (2016-07-25 20:48:31 UTC) #48

Menglin

On 2016/07/25 20:48:31, vmpstr wrote: > On 2016/07/25 20:41:08, Menglin wrote: > > > https://codereview.chromium.org/2141233002/diff/80001/cc/layers/picture_layer.cc ...

4 years, 4 months ago (2016-07-25 20:50:39 UTC) #49

Menglin

Hi vmpstr, ptal of patch 7 before I change more test code. Thanks! https://codereview.chromium.org/2141233002/diff/80001/cc/layers/picture_layer.cc File ...

4 years, 4 months ago (2016-07-25 21:58:52 UTC) #50

vmpstr

I just looked at picture_layer/recording_source and I think these changes look good! https://codereview.chromium.org/2141233002/diff/120001/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc ...

4 years, 4 months ago (2016-07-25 22:03:26 UTC) #51

Menglin

Hi Vlad, Patch 8 is the whole refactor. The code pass all unittests. But I ...

4 years, 4 months ago (2016-07-26 17:49:42 UTC) #52

vmpstr

This is looking good overall, just some questions/comments https://codereview.chromium.org/2141233002/diff/140001/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/2141233002/diff/140001/cc/layers/picture_layer.cc#newcode64 cc/layers/picture_layer.cc:64: recording_source_->CreateRasterSource(can_use_lcd_text); ...

4 years, 4 months ago (2016-07-27 18:18:37 UTC) #53

Menglin

https://codereview.chromium.org/2141233002/diff/140001/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/2141233002/diff/140001/cc/layers/picture_layer.cc#newcode64 cc/layers/picture_layer.cc:64: recording_source_->CreateRasterSource(can_use_lcd_text); On 2016/07/27 18:18:37, vmpstr wrote: > On 2016/07/26 ...

4 years, 4 months ago (2016-07-27 19:17:48 UTC) #54

Menglin

mlliu@chromium.org changed reviewers: + wkorman@chromium.org

4 years, 4 months ago (2016-07-27 19:19:02 UTC) #55

Menglin

wkorman, could you comment on this? https://codereview.chromium.org/2141233002/diff/140001/cc/playback/recording_source.cc#oldcode153

4 years, 4 months ago (2016-07-27 19:19:03 UTC) #56

wkorman

https://codereview.chromium.org/2141233002/diff/140001/cc/playback/recording_source.cc File cc/playback/recording_source.cc (left): https://codereview.chromium.org/2141233002/diff/140001/cc/playback/recording_source.cc#oldcode153 cc/playback/recording_source.cc:153: case RECORD_WITH_PAINTING_DISABLED: On 2016/07/27 at 19:17:48, Menglin wrote: > ...

4 years, 4 months ago (2016-07-27 20:12:41 UTC) #57

vmpstr

https://codereview.chromium.org/2141233002/diff/140001/cc/playback/recording_source.cc File cc/playback/recording_source.cc (left): https://codereview.chromium.org/2141233002/diff/140001/cc/playback/recording_source.cc#oldcode153 cc/playback/recording_source.cc:153: case RECORD_WITH_PAINTING_DISABLED: On 2016/07/27 20:12:40, wkorman wrote: > On ...

4 years, 4 months ago (2016-07-28 18:19:58 UTC) #58

https://codereview.chromium.org/2141233002/diff/140001/cc/playback/recording_...
File cc/playback/recording_source.cc (left):

https://codereview.chromium.org/2141233002/diff/140001/cc/playback/recording_...
cc/playback/recording_source.cc:153: case RECORD_WITH_PAINTING_DISABLED:
On 2016/07/27 20:12:40, wkorman wrote:
> On 2016/07/27 at 19:17:48, Menglin wrote:
> > On 2016/07/27 18:18:37, vmpstr wrote:
> > > Are these really not used anywhere? O_O I thought rasterize and record
> > > benchmarks did different metrics here to measure stuff. +wkorman to
verify. 
> > > 
> > > If these are really not used, can we remove the enum?
> > 
> > Right rasterize_and_record_benchmark has this
>
https://cs.chromium.org/chromium/src/cc/debug/rasterize_and_record_benchmark....
> > 
> > Just thought whatever calls UpdateAndExpandInvalidation only passes
NORMALLY.
> wkorman@ could you verify?
> 
> Your assessment looks correct to me. We still want the enum for use in the
> benchmark AFAIK, but it doesn't seem needed here in
UpdateAndExpandInvalidation.

OK. In that case, I think we can move the enum to the benchmark so that it
doesn't confuse non-test code. This can be a follow-up.

https://codereview.chromium.org/2141233002/diff/140001/cc/test/fake_picture_l...
File cc/test/fake_picture_layer.h (right):

https://codereview.chromium.org/2141233002/diff/140001/cc/test/fake_picture_l...
cc/test/fake_picture_layer.h:45: void SetRecordedViewport(const gfx::Rect&
recorded_viewport) {
On 2016/07/27 19:17:48, Menglin wrote:
> On 2016/07/27 18:18:37, vmpstr wrote:
> > can you make these hacker_case instead of CamelCase? (most functions
specific
> to
> > fakes do this, I think). 
> 
> you mean SetRecordedViewport and SetForceUnsuitableForGpuRasterization  ?

I mean if the function is inlined and it's a fake only function, then it should
be

set_recorded_viewport and
set_force_unsuitable_for_gpu_rasterization

It's just a convention nit...

https://codereview.chromium.org/2141233002/diff/140001/cc/test/fake_recording...
File cc/test/fake_recording_source.h (right):

https://codereview.chromium.org/2141233002/diff/140001/cc/test/fake_recording...
cc/test/fake_recording_source.h:32: const gfx::Rect& recorded_viewport,
On 2016/07/27 19:17:48, Menglin wrote:
> On 2016/07/27 18:18:37, vmpstr wrote:
> > On 2016/07/26 17:49:42, Menglin wrote:
> > > Do I still need to care about the recorded_viewport here? I feel it
doesn't
> > > matter so I kept it. 
> > 
> > Where do we call SetRecordedViewport aside from tests?
> 
> the only place it is called besides in this file is in fake_raster_source
>
https://cs.chromium.org/chromium/src/cc/test/fake_raster_source.cc?q=SetRecor...
> i feel it's just using recorded_viewport between recording source and raster
> source

If that's only called in test, then maybe we can just remove the function? I'm
not sure if any test relies on that to set up some sort of a special recording
source

Menglin

i'm working on updated patch. https://codereview.chromium.org/2141233002/diff/140001/cc/playback/recording_source.cc File cc/playback/recording_source.cc (left): https://codereview.chromium.org/2141233002/diff/140001/cc/playback/recording_source.cc#oldcode153 cc/playback/recording_source.cc:153: case RECORD_WITH_PAINTING_DISABLED: On 2016/07/28 ...

4 years, 4 months ago (2016-07-29 21:32:56 UTC) #59

i'm working on updated patch.

https://codereview.chromium.org/2141233002/diff/140001/cc/playback/recording_...
File cc/playback/recording_source.cc (left):

https://codereview.chromium.org/2141233002/diff/140001/cc/playback/recording_...
cc/playback/recording_source.cc:153: case RECORD_WITH_PAINTING_DISABLED:
On 2016/07/28 18:19:57, vmpstr wrote:
> On 2016/07/27 20:12:40, wkorman wrote:
> > On 2016/07/27 at 19:17:48, Menglin wrote:
> > > On 2016/07/27 18:18:37, vmpstr wrote:
> > > > Are these really not used anywhere? O_O I thought rasterize and record
> > > > benchmarks did different metrics here to measure stuff. +wkorman to
> verify. 
> > > > 
> > > > If these are really not used, can we remove the enum?
> > > 
> > > Right rasterize_and_record_benchmark has this
> >
>
https://cs.chromium.org/chromium/src/cc/debug/rasterize_and_record_benchmark....
> > > 
> > > Just thought whatever calls UpdateAndExpandInvalidation only passes
> NORMALLY.
> > wkorman@ could you verify?
> > 
> > Your assessment looks correct to me. We still want the enum for use in the
> > benchmark AFAIK, but it doesn't seem needed here in
> UpdateAndExpandInvalidation.
> 
> OK. In that case, I think we can move the enum to the benchmark so that it
> doesn't confuse non-test code. This can be a follow-up. 

OK. I will move the enum to rasterize_and_record_benchmark.h as a follow up CL

https://codereview.chromium.org/2141233002/diff/140001/cc/test/fake_recording...
File cc/test/fake_recording_source.h (right):

https://codereview.chromium.org/2141233002/diff/140001/cc/test/fake_recording...
cc/test/fake_recording_source.h:32: const gfx::Rect& recorded_viewport,
On 2016/07/28 18:19:58, vmpstr wrote:
> On 2016/07/27 19:17:48, Menglin wrote:
> > On 2016/07/27 18:18:37, vmpstr wrote:
> > > On 2016/07/26 17:49:42, Menglin wrote:
> > > > Do I still need to care about the recorded_viewport here? I feel it
> doesn't
> > > > matter so I kept it. 
> > > 
> > > Where do we call SetRecordedViewport aside from tests?
> > 
> > the only place it is called besides in this file is in fake_raster_source
> >
>
https://cs.chromium.org/chromium/src/cc/test/fake_raster_source.cc?q=SetRecor...
> > i feel it's just using recorded_viewport between recording source and raster
> > source
> 
> If that's only called in test, then maybe we can just remove the function? I'm
> not sure if any test relies on that to set up some sort of a special recording
> source

These two functions are called by FakeRasterSource to set up a special raster
source I think. For example, here
https://cs.chromium.org/chromium/src/cc/test/fake_raster_source.cc?sq=package...
And there are many similar places. So I think I should keep them.

Menglin

I uploaded a new patch. ptal. Thanks! Menglin https://codereview.chromium.org/2141233002/diff/140001/cc/layers/picture_layer.cc File cc/layers/picture_layer.cc (right): https://codereview.chromium.org/2141233002/diff/140001/cc/layers/picture_layer.cc#newcode212 cc/layers/picture_layer.cc:212: for ...

4 years, 4 months ago (2016-07-29 23:50:15 UTC) #60

wkorman

FYI http://crrev.com/1484163002 will likely conflict with this change and require some manual merigng. I am ...

4 years, 4 months ago (2016-07-29 23:59:27 UTC) #61

Menglin

On 2016/07/29 23:59:27, wkorman wrote: > FYI http://crrev.com/1484163002 will likely conflict with this change and ...

4 years, 4 months ago (2016-07-30 00:21:11 UTC) #62

wkorman

On 2016/07/30 at 00:21:11, mlliu wrote: > On 2016/07/29 23:59:27, wkorman wrote: > > FYI ...

4 years, 4 months ago (2016-07-30 00:32:17 UTC) #63

vmpstr

lgtm thanks https://codereview.chromium.org/2141233002/diff/160001/cc/playback/recording_source.cc File cc/playback/recording_source.cc (right): https://codereview.chromium.org/2141233002/diff/160001/cc/playback/recording_source.cc#newcode71 cc/playback/recording_source.cc:71: if (display_list_) { nit: braces optional

4 years, 4 months ago (2016-08-01 20:29:53 UTC) #64

Menglin

wkorman, i'm submitting this CL now! Thanks for everybody's comments on this refactor Menglin https://codereview.chromium.org/2141233002/diff/160001/cc/playback/recording_source.cc ...

4 years, 4 months ago (2016-08-02 02:58:43 UTC) #65

Menglin

The patchset sent to the CQ was uploaded after l-g-t-m from khushalsagar@chromium.org, vmpstr@chromium.org Link to ...

4 years, 4 months ago (2016-08-02 02:58:58 UTC) #67

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2141233002/180001

4 years, 4 months ago (2016-08-02 02:59:12 UTC) #68

commit-bot: I haz the power

Your CL relies on deprecated CQ feature(s): * Specifying master names in CQ_INCLUDE_TRYBOTS part of ...

4 years, 4 months ago (2016-08-02 02:59:15 UTC) #69

commit-bot: I haz the power

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

4 years, 4 months ago (2016-08-02 03:00:42 UTC) #70

commit-bot: I haz the power

Try jobs failed on following builders: linux_blink_rel on tryserver.blink (JOB_FAILED, no build URL)

4 years, 4 months ago (2016-08-02 03:00:44 UTC) #71

Menglin

Description was changed from ========== cc: Clean up RecordingSource API Currently, the data held in ...

4 years, 4 months ago (2016-08-02 18:54:30 UTC) #72

Khushal

https://codereview.chromium.org/2141233002/diff/200001/cc/playback/recording_source.cc File cc/playback/recording_source.cc (right): https://codereview.chromium.org/2141233002/diff/200001/cc/playback/recording_source.cc#newcode105 cc/playback/recording_source.cc:105: ContentLayerClient* painter, Not used here anymore?

4 years, 4 months ago (2016-08-02 20:20:36 UTC) #73

wkorman

On 2016/08/02 at 02:58:43, mlliu wrote: > wkorman, i'm submitting this CL now! Ah, I ...

4 years, 4 months ago (2016-08-02 20:32:16 UTC) #74

Menglin

On 2016/08/02 20:32:16, wkorman wrote: > On 2016/08/02 at 02:58:43, mlliu wrote: > > wkorman, ...

4 years, 4 months ago (2016-08-02 20:40:37 UTC) #75

Menglin

trybots passed. submitting again https://codereview.chromium.org/2141233002/diff/200001/cc/playback/recording_source.cc File cc/playback/recording_source.cc (right): https://codereview.chromium.org/2141233002/diff/200001/cc/playback/recording_source.cc#newcode105 cc/playback/recording_source.cc:105: ContentLayerClient* painter, On 2016/08/02 20:20:36, ...

4 years, 4 months ago (2016-08-02 21:04:26 UTC) #76

Menglin

The patchset sent to the CQ was uploaded after l-g-t-m from khushalsagar@chromium.org, vmpstr@chromium.org Link to ...

4 years, 4 months ago (2016-08-02 21:04:38 UTC) #78

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2141233002/220001

4 years, 4 months ago (2016-08-02 21:05:54 UTC) #79

commit-bot: I haz the power

Description was changed from ========== cc: Clean up RecordingSource API Currently, the data held in ...

4 years, 4 months ago (2016-08-02 22:18:39 UTC) #80

commit-bot: I haz the power

Description was changed from ========== cc: Clean up RecordingSource API Currently, the data held in ...

4 years, 4 months ago (2016-08-02 22:20:43 UTC) #82

Message was sent while issue was closed.

Description was changed from

==========
cc: Clean up RecordingSource API

Currently, the data held in RecordingSource is either received from
ContentLayerClient or PictureLayer. Separate them into two structs:
ContentLayerClientData and PictureLayerClientData, and move them to
PictureLayer. Also move most of the methods in RecordingSource to PictureLayer,
except CreateRasterSource.

UpdateAndExpandInvalidation is moved to PictureLayer. It will take pointers to
the two structs, and update the members of the structs, instead of updating the
internal state. So that when UpdateAndExpandInvalidation is called in
PushPropertiesTo, PictureLayer's internal structs will be passed in. And when
UpdateAndExpandInvalidation is called in GetPicture, local temporal structs will
be passed in.

BUG=625290
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
cc: Clean up RecordingSource API

Currently, the data held in RecordingSource is either received from
ContentLayerClient or PictureLayer. Separate them into two structs:
ContentLayerClientData and PictureLayerClientData, and move them to
PictureLayer. Also move most of the methods in RecordingSource to PictureLayer,
except CreateRasterSource.

UpdateAndExpandInvalidation is moved to PictureLayer. It will take pointers to
the two structs, and update the members of the structs, instead of updating the
internal state. So that when UpdateAndExpandInvalidation is called in
PushPropertiesTo, PictureLayer's internal structs will be passed in. And when
UpdateAndExpandInvalidation is called in GetPicture, local temporal structs will
be passed in.

BUG=625290
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/51126b7275df6aa885015cf4693901003358049c
Cr-Commit-Position: refs/heads/master@{#409348}
==========

commit-bot: I haz the power

4 years, 4 months ago (2016-08-02 22:20:44 UTC) #83

Message was sent while issue was closed.

Patchset 12 (id:??) landed as
https://crrev.com/51126b7275df6aa885015cf4693901003358049c
Cr-Commit-Position: refs/heads/master@{#409348}

Issue 2141233002: cc: Clean up RecordingSource API (Closed)

Description

Patch Set 1 #

Patch Set 2 : inline the initialization, constructor/destructor = default in .cc #

Patch Set 3 : remove recoding_source_ from PictureLayer, and move all its internal state to PictureLayer #

Patch Set 4 : Address comments #

Patch Set 5 : keep RecordingSource, move three members to PictureLayer #

Patch Set 6 : RecordingSource keeps copies of content client layer related data #

Patch Set 7 : move types to the beginning of the access block #

Patch Set 8 : Test code refactor #

Patch Set 9 : address vmpstr's comments #

Patch Set 10 : nit addressed and sync to head #

Patch Set 11 : fix failed compositor_unittests #

Patch Set 12 : Remove ContentLayerClient* painter from UpdateAndExpandInvalidation and sync to head #

Messages