StylePropertySerializer should expand all property if needed.

StylePropertySerializer should expand all property if needed.

For example, if we have "div { all: initial; color: red; }", the all property
should be expanded to all properties with initial value, e.g. display: initial,
except color. So the expected result is "div { display: initial; ...
color: red; }".

Spec of all property: http://dev.w3.org/csswg/css-cascade/#all-shorthand

BUG=172051
TEST=fast/css/all-shorthand-css-text.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=184252

[tasak, 2014-06-19 08:12:01]

Would you review this CL?

The differences between my old patch
(https://codereview.chromium.org/216803002/) and this patch is:

- create a new class: StylePropertySetForSerializer instead of adding new
methods to StylePropertySerializer.

The StylePropertySetForSerializer has the same methods, which are used in
StylePropertySerializer, as StylePropertySet.
e.g. propertyAt, propertyIsImportant, and so on.

The StylePropertyForSerializer uses BitArray instead of Vector, because
inserting expanded properties with their values is expensive.

The BitArray answeres whether a given property should be serialized or not.

e.g. A given StylePropertySet is created from "div { color: red; all: initial;
background-color: green; }".

- The color is initial because of "all". So the BitArray[CSSPropertyColor] is 0.
We find that we don't need to serialize the color property.

- The background-color is green, not initial. So the
BitArray[CSSPropertyBackgroundColor] is 1.

StylePropertySetForSerializer has m_needToExpandAll. If a given StylePropertySet
has only "all", we don't need to expand the "all" shorthand property. In this
case, m_needToExpandAll is false.
(e.g. "div { color: red; all: initial; }", "div { all: initial; }", "div { all:
initial !important; color: red; }" and so on)

[tasak, 2014-07-01 08:44:22]

Would you review this CL?

[tasak, 2014-09-01 07:24:40]

On 2014/07/01 08:44:22, tasak wrote:
> Would you review this CL?

ping.

[esprehn, 2014-09-02 19:41:57]

Why do we want to expand all: unset into hundreds of properties instead of just
serializing it into all: unset?

https://codereview.chromium.org/341033003/diff/70001/Source/core/css/StylePro...
File Source/core/css/StylePropertySerializer.h (right):

https://codereview.chromium.org/341033003/diff/70001/Source/core/css/StylePro...
Source/core/css/StylePropertySerializer.h:34: class PropertyValueForSerializer {
Put this in its own file.

https://codereview.chromium.org/341033003/diff/70001/Source/core/css/StylePro...
Source/core/css/StylePropertySerializer.h:36:
PropertyValueForSerializer(StylePropertySet::PropertyReference property)
explicit

https://codereview.chromium.org/341033003/diff/70001/Source/core/css/StylePro...
Source/core/css/StylePropertySerializer.h:65: class
StylePropertySetForSerializer {
ditto

https://codereview.chromium.org/341033003/diff/70001/Source/core/css/StylePro...
Source/core/css/StylePropertySerializer.h:67:
StylePropertySetForSerializer(const StylePropertySet&);
explicit

[rune, 2014-09-02 20:13:17]

https://codereview.chromium.org/341033003/diff/70001/Source/core/css/StylePro...
File Source/core/css/StylePropertySerializer.cpp (right):

https://codereview.chromium.org/341033003/diff/70001/Source/core/css/StylePro...
Source/core/css/StylePropertySerializer.cpp:57: if ((unsigned)m_allIndex >= i)
Use static_cast instead of C-style cast.

https://codereview.chromium.org/341033003/diff/70001/Source/core/css/StylePro...
Source/core/css/StylePropertySerializer.cpp:62: ++numAffectedLonghands;
Can't you just set m_needToExpandAll here and drop numAffectedLonghands
altogether?

https://codereview.chromium.org/341033003/diff/70001/Source/core/css/StylePro...
File Source/core/css/StylePropertySerializer.h (right):

https://codereview.chromium.org/341033003/diff/70001/Source/core/css/StylePro...
Source/core/css/StylePropertySerializer.h:58: CSSPropertyID m_id;
I think I would've put this member below m_value as the class probably uses less
memory on a 64-bit system that way. Not that the memory is an issue here, so
feel free to ignore this.

[tasak, 2014-09-04 06:08:38]

Thank you for reviewing.

> Why do we want to expand all: unset into hundreds of properties instead of
just
serializing it into all: unset?

If we have only { all: unset; }, the code doesn't expand all: unset into many
properties.
However, if we have { all: unset; color: blue; }, I think, we need to generate
"... font: unset; animation-play-state: unset; ... color: blue; }".

I checked how firefox works. I saw "... font: unset; animation-play-state:
unset; ... color: blue; }".

https://codereview.chromium.org/341033003/diff/70001/Source/core/css/StylePro...
File Source/core/css/StylePropertySerializer.cpp (right):

https://codereview.chromium.org/341033003/diff/70001/Source/core/css/StylePro...
Source/core/css/StylePropertySerializer.cpp:57: if ((unsigned)m_allIndex >= i)
On 2014/09/02 20:13:07, rune wrote:
> Use static_cast instead of C-style cast.

Done.

https://codereview.chromium.org/341033003/diff/70001/Source/core/css/StylePro...
Source/core/css/StylePropertySerializer.cpp:62: ++numAffectedLonghands;
On 2014/09/02 20:13:07, rune wrote:
> Can't you just set m_needToExpandAll here and drop numAffectedLonghands
> altogether?

Done.

https://codereview.chromium.org/341033003/diff/70001/Source/core/css/StylePro...
File Source/core/css/StylePropertySerializer.h (right):

https://codereview.chromium.org/341033003/diff/70001/Source/core/css/StylePro...
Source/core/css/StylePropertySerializer.h:34: class PropertyValueForSerializer {
On 2014/09/02 19:41:57, esprehn wrote:
> Put this in its own file.

I put this into class StylePropertySerializer, because only
StylePropertySerializer uses this class.

https://codereview.chromium.org/341033003/diff/70001/Source/core/css/StylePro...
Source/core/css/StylePropertySerializer.h:36:
PropertyValueForSerializer(StylePropertySet::PropertyReference property)
On 2014/09/02 19:41:56, esprehn wrote:
> explicit

Done.

https://codereview.chromium.org/341033003/diff/70001/Source/core/css/StylePro...
Source/core/css/StylePropertySerializer.h:58: CSSPropertyID m_id;
On 2014/09/02 20:13:17, rune wrote:
> I think I would've put this member below m_value as the class probably uses
less
> memory on a 64-bit system that way. Not that the memory is an issue here, so
> feel free to ignore this.

Done.

https://codereview.chromium.org/341033003/diff/70001/Source/core/css/StylePro...
Source/core/css/StylePropertySerializer.h:65: class
StylePropertySetForSerializer {
On 2014/09/02 19:41:57, esprehn wrote:
> ditto

I put this into class StylePropertySerializer too.

https://codereview.chromium.org/341033003/diff/70001/Source/core/css/StylePro...
Source/core/css/StylePropertySerializer.h:67:
StylePropertySetForSerializer(const StylePropertySet&);
On 2014/09/02 19:41:57, esprehn wrote:
> explicit

Done.

[tasak, 2014-09-18 11:24:08]

ping.

[tasak, 2014-10-03 09:40:53]

On 2014/09/18 11:24:08, tasak wrote:
> ping.

ping.

[rune, 2014-10-03 09:49:31]

On 2014/10/03 09:40:53, tasak wrote:
> On 2014/09/18 11:24:08, tasak wrote:
> > ping.
> 
> ping.

My issues addressed, so this is for esprehn@.

[dglazkov, 2014-10-22 15:42:52]

On 2014/10/03 at 09:49:31, rune wrote:
> On 2014/10/03 09:40:53, tasak wrote:
> > On 2014/09/18 11:24:08, tasak wrote:
> > > ping.
> > 
> > ping.
> 
> My issues addressed, so this is for esprehn@.

lgtm.

[tasak, 2014-10-23 10:19:29]

rune, dglazkov, thank you.

[tasak, 2014-10-23 10:19:36]

The CQ bit was checked by tasak@google.com

[commit-bot: I haz the power, 2014-10-23 10:20:32]

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

[commit-bot: I haz the power, 2014-10-23 10:24:22]

Committed patchset #4 (id:110001) as 184252

[Timothy Loh, 2014-10-23 12:19:00]

Shouldn't regular StylePropertySets have the behaviour of the
StylePropertySetForSerializer? The 'all' property is just a regular shorthand,
it shouldn't be expanded only in cssText. If foo.style.all is set, then we
should be able to read the value from foo.style.padding or
foo.style.transitionProperty, as all other shorthands behave this way.