Organize internal properties of cc/ layer types

Organize internal properties of cc/ layer types

One useful way to classify the various properties in cc/ layer data types is:
(1) properties given by the user of the cc APIs, and (2) properties that are
computed inside of cc code that are needed to correctly draw the layers.

This patch organizes properties internally so that as we develop cc code, we
are forced to be aware of whether a property is "given" or "computed".

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171323

[shawnsingh, 2012-11-30 22:48:12]

This first patch is incomplete and preliminary.  Just wanted to give a taste of
what I had in mind to get feedback.

I propose the following changes:

(1) the new DrawProperties struct as shown in this first patch (name can be
easily changed)

(2) the corresponding accessors/settors in layer_impl would be removed, and
outside code would instead have to do the following:

layer->drawProperties().drawTransform;

or, in some useful cases:
 DrawProperties& layerDrawProps = layer->drawProperties();
 layerDrawProps.drawTransform = // etc. etc.
 layerDrawProps.screenSpaceTransform = // etc. etc.

(3) I believe all these properties are shared between layers and layer_impls, so
layer.h/.cc should also have the same API change.

Please let me know what you think =)

[danakj, 2012-12-01 03:51:40]

https://codereview.chromium.org/11280263/diff/1/cc/layer_impl.h
File cc/layer_impl.h (right):

https://codereview.chromium.org/11280263/diff/1/cc/layer_impl.h#newcode40
cc/layer_impl.h:40: struct DrawProperties {
If the struct has all the same things on both threads.. can we pull it out and
have it shared between layer and layer impl?

I'd reeeally love to have some sharing between these two.

https://codereview.chromium.org/11280263/diff/1/cc/layer_impl.h#newcode217
cc/layer_impl.h:217: float drawOpacity() const { return
m_drawProperties.drawOpacity; }
I guess the plan is for these accessors to go away?

[shawnsingh, 2012-12-01 04:49:05]

Yeah that's the plan - for both your points.   I take it this is a direction
that you like?

enne@ I'm particularly interested in your feedback too;  I don't want this to be
disruptive to the challenging work you're doing right now.

[danakj, 2012-12-01 05:09:40]

On 2012/12/01 04:49:05, shawnsingh wrote:
> Yeah that's the plan - for both your points.   I take it this is a direction
> that you like?
> 
> enne@ I'm particularly interested in your feedback too;  I don't want this to
be
> disruptive to the challenging work you're doing right now.

This is what I was thinking about having for some time, so I agree with it.

Also agree with enne@ feedbacks :)

[shawnsingh, 2012-12-02 09:11:07]

PTAL

Latest patch separates the struct into its own file, and shares the data between
layer.h and layer_impl.h

Please note that the code in layer_tree_host_common.cc is dependent on another
patch that is under review - https://codereview.chromium.org/11280271/

[shawnsingh, 2012-12-02 09:11:51]

Oh, right.   red bots are because of that dependency... silly me.

[danakj, 2012-12-03 18:54:36]

https://codereview.chromium.org/11280263/diff/3002/cc/draw_properties.h
File cc/draw_properties.h (right):

https://codereview.chromium.org/11280263/diff/3002/cc/draw_properties.h#newcode1
cc/draw_properties.h:1: // Copyright 2011 The Chromium Authors. All rights
reserved.
2012.

Use chromium style names/indents for new files.

https://codereview.chromium.org/11280263/diff/3002/cc/draw_properties.h#newco...
cc/draw_properties.h:67: }
}  // namespace cc

https://codereview.chromium.org/11280263/diff/3002/cc/layer.h
File cc/layer.h (left):

https://codereview.chromium.org/11280263/diff/3002/cc/layer.h#oldcode250
cc/layer.h:250: virtual void setContentsScale(float contentsScale) { }
setContentsScale is called from calcDrawEtc on the main thread only. How should
it relate to this struct?

https://codereview.chromium.org/11280263/diff/3002/cc/layer.h#oldcode399
cc/layer.h:399: scoped_ptr<RenderSurface> m_renderSurface;
render surface is created/assigned in calcDraw. Should it be part of the struct?

[shawnsingh, 2012-12-03 19:27:44]

Thanks for review.

By the way, could we re-name calculateDrawTransforms --> calculateDrawProeprties

https://codereview.chromium.org/11280263/diff/3002/cc/layer.h
File cc/layer.h (left):

https://codereview.chromium.org/11280263/diff/3002/cc/layer.h#oldcode250
cc/layer.h:250: virtual void setContentsScale(float contentsScale) { }
Been meaning to discuss this with you offline =)  had some questions/thoughts
about how it's computed in calcDrawTransforms.

https://codereview.chromium.org/11280263/diff/3002/cc/layer.h#oldcode399
cc/layer.h:399: scoped_ptr<RenderSurface> m_renderSurface;
On 2012/12/03 18:54:36, danakj wrote:
> render surface is created/assigned in calcDraw. Should it be part of the
struct?

Agreed, I'll move it over

[enne (OOO), 2012-12-03 21:18:14]

On 2012/12/03 19:27:44, shawnsingh wrote:

> By the way, could we re-name calculateDrawTransforms -->
calculateDrawProperties

sgtm

https://codereview.chromium.org/11280263/diff/3002/cc/layer.h
File cc/layer.h (left):

https://codereview.chromium.org/11280263/diff/3002/cc/layer.h#oldcode250
cc/layer.h:250: virtual void setContentsScale(float contentsScale) { }
On 2012/12/03 19:27:44, shawnsingh wrote:
> Been meaning to discuss this with you offline =)  had some questions/thoughts
> about how it's computed in calcDrawTransforms.

Given that contentsScale isn't stored on Layer in any case, I don't think it can
be part of the struct.  I'm fine with punting.  If you wanted something in the
struct, it'd have to be "ideal contents scale" and layers could choose to ignore
it if they wanted.

[danakj, 2012-12-03 21:29:45]

https://codereview.chromium.org/11280263/diff/3002/cc/layer.h
File cc/layer.h (left):

https://codereview.chromium.org/11280263/diff/3002/cc/layer.h#oldcode250
cc/layer.h:250: virtual void setContentsScale(float contentsScale) { }
On 2012/12/03 21:18:14, enne wrote:
> On 2012/12/03 19:27:44, shawnsingh wrote:
> > Been meaning to discuss this with you offline =)  had some
questions/thoughts
> > about how it's computed in calcDrawTransforms.
> 
> Given that contentsScale isn't stored on Layer in any case, I don't think it
can
> be part of the struct.  I'm fine with punting.  If you wanted something in the
> struct, it'd have to be "ideal contents scale" and layers could choose to
ignore
> it if they wanted.

Oh, good point! Putting it in the struct is a bit awkward. We'd really want a
virtual method or something and then C++ would expload.

[shawnsingh, 2012-12-03 21:44:39]

/me punts the contentsScale

I'll get a new version after the transform tighten lands.  ETA 2-3 days.

[jamesr, 2012-12-04 06:44:32]

I'm a fan!

https://codereview.chromium.org/11280263/diff/3002/cc/draw_properties.h
File cc/draw_properties.h (right):

https://codereview.chromium.org/11280263/diff/3002/cc/draw_properties.h#newco...
cc/draw_properties.h:29: gfx::Transform drawTransform;
drawProperties().drawTransform is a bit redundant. maybe just
drawProperties().transform ?

https://codereview.chromium.org/11280263/diff/3002/cc/draw_properties.h#newco...
cc/draw_properties.h:36: float drawOpacity;
same here - drawProperties().opacity ?

https://codereview.chromium.org/11280263/diff/3002/cc/draw_properties.h#newco...
cc/draw_properties.h:43: bool drawTransformIsAnimating;
more possibly unnecessary draw prefixes

https://codereview.chromium.org/11280263/diff/3002/cc/layer_impl.cc
File cc/layer_impl.cc (right):

https://codereview.chromium.org/11280263/diff/3002/cc/layer_impl.cc#newcode23
cc/layer_impl.cc:23: 
extra newline

[danakj, 2012-12-04 08:15:46]

On Mon, Dec 3, 2012 at 2:27 PM,  <shawnsingh@chromium.org> wrote:
> Thanks for review.
>
> By the way, could we re-name calculateDrawTransforms -->
> calculateDrawProeprties

I'm okay with it if enne@ is. Tho, I prefer Properties ;)

>
> https://codereview.chromium.org/11280263/diff/3002/cc/layer.h
> File cc/layer.h (left):
>
> https://codereview.chromium.org/11280263/diff/3002/cc/layer.h#oldcode250
> cc/layer.h:250: virtual void setContentsScale(float contentsScale) { }
> Been meaning to discuss this with you offline =)  had some
> questions/thoughts about how it's computed in calcDrawTransforms.
>
>
> https://codereview.chromium.org/11280263/diff/3002/cc/layer.h#oldcode399
> cc/layer.h:399: scoped_ptr<RenderSurface> m_renderSurface;
> On 2012/12/03 18:54:36, danakj wrote:
>>
>> render surface is created/assigned in calcDraw. Should it be part of
>
> the struct?
>
> Agreed, I'll move it over
>
> https://codereview.chromium.org/11280263/
>
> --
>
>

[shawnsingh, 2012-12-05 03:57:23]

OK this is ready for review again.

I also found another round of math-tightening we can do after this patch.  I
tried it already, and I have good hopes it will improve both performance and
elegance of this code.

[enne (OOO), 2012-12-05 18:34:04]

lgtm

[danakj, 2012-12-05 18:35:00]

https://codereview.chromium.org/11280263/diff/13001/cc/draw_properties.h
File cc/draw_properties.h (right):

https://codereview.chromium.org/11280263/diff/13001/cc/draw_properties.h#newc...
cc/draw_properties.h:72: } // namespace cc
nit: 2 spaces before //

https://codereview.chromium.org/11280263/diff/13001/cc/draw_properties.h#newc...
cc/draw_properties.h:74: #endif // CC_DRAW_PROPERTIES_H_
nit: 2 spaces