Ensure that we don't allow 'normal' alongside other values in content property

Ensure that we don't allow 'normal' alongside other values in content property

Currently Chrome accepts a list of values that include "normal" along
with other values as an acceptable input for CSS property content. This
breaks from the spec stated here:
https://drafts.csswg.org/css2/generate.html#content
where "normal" is only accepted as a valid input if it is the
only value assigned to content. This patch fixes that behaviour.
Other browsers (FF, IE and Edge are consistent with spec here.)

Committed: https://crrev.com/ae95f37d78cfa0a3d0cc28e89531d68050c1c707
Cr-Commit-Position: refs/heads/master@{#383690}

[nainar, 2016-03-23 06:31:21]

nainar@chromium.org changed reviewers:
+ timloh@chromium.org

[nainar, 2016-03-23 06:31:22]

@timloh,

PTAL?

Thanks!

[Timothy Loh, 2016-03-23 06:45:48]

Surely this will hit an crash/assert in applyValueCSSPropertyCustom? :\

Patch title should give more detail, e.g. "Don't allow 'normal' alongside other
values in content property" or something like that. Patch description is a bit
weird, prefer to talk about web-exposed concepts instead of implementation
details (shouldn't have to mention the CSSValueNormal enum)

Please also add tests for normal alongside other values (e.g. urls).

[nainar, 2016-03-23 23:27:43]

Description was changed from

==========
Enforce acceptable values for content

Currently Chrome accepts a list of CSSValueNormal as an acceptable input
for CSS property content. This breaks from the spec stated here:
https://drafts.csswg.org/css2/generate.html#content
where only a single ident of CSSValueNormal is accepted as a valid input.
This patch fixes that behaviour.

BUG=313670
==========

to

==========
Ensure that we don't allow 'normal' alongside other values in content property

Currently Chrome accepts a list of values that include "normal" along 
with other values as an acceptable input for CSS property content. This 
breaks from the spec stated here:
https://drafts.csswg.org/css2/generate.html#content
where "normal" is only accepted as a valid input if it is the 
only value assigned to content. This patch fixes that behaviour.

A dependant patch will also add the ability to accept "none" as a 
valid input for content when on its own and not paired with other values.
This behaviour is listed in the spec but not supported by Chromium.

BUG=313670
==========

[nainar, 2016-03-24 04:31:44]

Tim, 

I've edited the patch description as well as added more cases to the layout
tests. PTAL?

Thanks!

[Timothy Loh, 2016-03-24 04:38:41]

The linked bug isn't relevant. In fact I think for the other patch you're better
off making a new bug for it since the bug reported in the original report has
been fixed. Also as usual, please mention what other browsers do in the patch
description.

https://codereview.chromium.org/1824283002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/css-parser/content-parsing.html (right):

https://codereview.chromium.org/1824283002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/css-parser/content-parsing.html:12:
assert_invalid_value("content", 'normal "a"');
should at least have one test case with normal as the second value

https://codereview.chromium.org/1824283002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/css-parser/content-parsing.html:14:
assert_invalid_value("content", "normal 'open-quote;");
not sure what this case is trying to test with mismatched quotes??

https://codereview.chromium.org/1824283002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/1824283002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:661: if
(toCSSPrimitiveValue(*value).getValueID() == CSSValueNormal) {
should be an assert instead since if we get here this will always be true

[nainar, 2016-03-24 04:57:16]

Description was changed from

==========
Ensure that we don't allow 'normal' alongside other values in content property

Currently Chrome accepts a list of values that include "normal" along 
with other values as an acceptable input for CSS property content. This 
breaks from the spec stated here:
https://drafts.csswg.org/css2/generate.html#content
where "normal" is only accepted as a valid input if it is the 
only value assigned to content. This patch fixes that behaviour.

A dependant patch will also add the ability to accept "none" as a 
valid input for content when on its own and not paired with other values.
This behaviour is listed in the spec but not supported by Chromium.

BUG=313670
==========

to

==========
Ensure that we don't allow 'normal' alongside other values in content property

Currently Chrome accepts a list of values that include "normal" along 
with other values as an acceptable input for CSS property content. This 
breaks from the spec stated here:
https://drafts.csswg.org/css2/generate.html#content
where "normal" is only accepted as a valid input if it is the 
only value assigned to content. This patch fixes that behaviour.

A dependant patch will also add the ability to accept "none" as a 
valid input for content when on its own and not paired with other values.
This behaviour is listed in the spec but not supported by Chromium.
==========

[nainar, 2016-03-29 00:56:14]

Description was changed from

==========
Ensure that we don't allow 'normal' alongside other values in content property

Currently Chrome accepts a list of values that include "normal" along 
with other values as an acceptable input for CSS property content. This 
breaks from the spec stated here:
https://drafts.csswg.org/css2/generate.html#content
where "normal" is only accepted as a valid input if it is the 
only value assigned to content. This patch fixes that behaviour.

A dependant patch will also add the ability to accept "none" as a 
valid input for content when on its own and not paired with other values.
This behaviour is listed in the spec but not supported by Chromium.
==========

to

==========
Ensure that we don't allow 'normal' alongside other values in content property

Currently Chrome accepts a list of values that include "normal" along 
with other values as an acceptable input for CSS property content. This 
breaks from the spec stated here:
https://drafts.csswg.org/css2/generate.html#content
where "normal" is only accepted as a valid input if it is the 
only value assigned to content. This patch fixes that behaviour. 
Other browsers (FF, IE and Edge are consistent with spec here.)

A dependant patch will also add the ability to accept "none" as a 
valid input for content when on its own and not paired with other values.
This behaviour is listed in the spec but not supported by Chromium.
==========

[nainar, 2016-03-29 00:56:39]

@timloh,

PTAL?

Thanks!

https://codereview.chromium.org/1824283002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/css-parser/content-parsing.html (right):

https://codereview.chromium.org/1824283002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/css-parser/content-parsing.html:12:
assert_invalid_value("content", 'normal "a"');
Done.

https://codereview.chromium.org/1824283002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/1824283002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:661: if
(toCSSPrimitiveValue(*value).getValueID() == CSSValueNormal) {
Done.

[Timothy Loh, 2016-03-29 03:10:54]

ok lgtm

https://codereview.chromium.org/1824283002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/1824283002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:733: //
normal and none do not have any effect.
ASSERT_NOT_REACHED() here

https://codereview.chromium.org/1824283002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:738: if
(!didSet)
ASSERT(didSet)

[nainar, 2016-03-29 06:37:49]

Made the changes, committing.

Thanks for the review!

https://codereview.chromium.org/1824283002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/1824283002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:733: //
normal and none do not have any effect.
Done.

https://codereview.chromium.org/1824283002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:738: if
(!didSet)
Done.

[nainar, 2016-03-29 06:37:51]

The CQ bit was checked by nainar@chromium.org

[nainar, 2016-03-29 06:37:51]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1824283002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2016-03-29 06:38:06]

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

[Timothy Loh, 2016-03-29 06:51:38]

https://codereview.chromium.org/1824283002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/1824283002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:739:
state.style()->clearContent();
...this is wrong, this just makes us never have content any more

[Timothy Loh, 2016-03-29 06:52:40]

https://codereview.chromium.org/1824283002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp (right):

https://codereview.chromium.org/1824283002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/resolver/StyleBuilderCustom.cpp:733: //
normal and none do not have any effect.
also remove the comment because it doesn't make sense any more

[nainar, 2016-03-29 06:52:44]

The CQ bit was unchecked by nainar@chromium.org

[nainar, 2016-03-29 07:00:10]

The CQ bit was unchecked by nainar@chromium.org

[nainar, 2016-03-29 07:28:52]

changed the comment to remove reference to normal. Not adding the asserts as per
discussed.

[nainar, 2016-03-29 07:29:06]

The CQ bit was checked by nainar@chromium.org

[nainar, 2016-03-29 07:29:06]

The patchset sent to the CQ was uploaded after l-g-t-m from timloh@chromium.org
Link to the patchset: https://codereview.chromium.org/1824283002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2016-03-29 07:29:15]

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

[nainar, 2016-03-29 07:29:29]

The CQ bit was unchecked by nainar@chromium.org

[nainar, 2016-03-29 07:30:10]

Description was changed from

==========
Ensure that we don't allow 'normal' alongside other values in content property

Currently Chrome accepts a list of values that include "normal" along 
with other values as an acceptable input for CSS property content. This 
breaks from the spec stated here:
https://drafts.csswg.org/css2/generate.html#content
where "normal" is only accepted as a valid input if it is the 
only value assigned to content. This patch fixes that behaviour. 
Other browsers (FF, IE and Edge are consistent with spec here.)

A dependant patch will also add the ability to accept "none" as a 
valid input for content when on its own and not paired with other values.
This behaviour is listed in the spec but not supported by Chromium.
==========

to

==========
Ensure that we don't allow 'normal' alongside other values in content property

Currently Chrome accepts a list of values that include "normal" along 
with other values as an acceptable input for CSS property content. This 
breaks from the spec stated here:
https://drafts.csswg.org/css2/generate.html#content
where "normal" is only accepted as a valid input if it is the 
only value assigned to content. This patch fixes that behaviour. 
Other browsers (FF, IE and Edge are consistent with spec here.)
==========

[nainar, 2016-03-29 07:30:15]

The CQ bit was checked by nainar@chromium.org

[commit-bot: I haz the power, 2016-03-29 07:30:25]

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

[commit-bot: I haz the power, 2016-03-29 08:44:24]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-29 08:45:28]

Description was changed from

==========
Ensure that we don't allow 'normal' alongside other values in content property

Currently Chrome accepts a list of values that include "normal" along 
with other values as an acceptable input for CSS property content. This 
breaks from the spec stated here:
https://drafts.csswg.org/css2/generate.html#content
where "normal" is only accepted as a valid input if it is the 
only value assigned to content. This patch fixes that behaviour. 
Other browsers (FF, IE and Edge are consistent with spec here.)
==========

to

==========
Ensure that we don't allow 'normal' alongside other values in content property

Currently Chrome accepts a list of values that include "normal" along
with other values as an acceptable input for CSS property content. This
breaks from the spec stated here:
https://drafts.csswg.org/css2/generate.html#content
where "normal" is only accepted as a valid input if it is the
only value assigned to content. This patch fixes that behaviour.
Other browsers (FF, IE and Edge are consistent with spec here.)

Committed: https://crrev.com/ae95f37d78cfa0a3d0cc28e89531d68050c1c707
Cr-Commit-Position: refs/heads/master@{#383690}
==========

[commit-bot: I haz the power, 2016-03-29 08:45:29]

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