Fix serialization of content property to always quote

Fix serialization of content property to always quote

Fix serialization of content property to always quote
by splitting CSS_STRING into CSS_CUSTOM_IDENT (quoted
if needed) and CSS_STRING (always quoted).
Also make the CSS properties that support <string> or
<custom-ident> construct CSSPrimitiveValues with the
correct keyword(CSS_STRING/CSS_CUSTOM_IDENT), specifically
-webkit-locale, -webkit-hyphenate and -webkit-highlight
now always quote, hence a lot of tests are rebaselined.

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

[rwlbuis, 2015-03-03 02:06:19]

rob.buis@samsung.com changed reviewers:
+ timloh@chromium.org

[rwlbuis, 2015-03-03 02:06:20]

PTAL.

[Timothy Loh, 2015-03-03 02:26:18]

On 2015/03/03 02:06:20, rwlbuis wrote:
> PTAL.

We should maybe try and fix this more generally. I think this involves
- Distinguishing <custom-ident> and <string> in CSSValue.
- Always quoting CSS_STRING, never quoting CSS_CUSTOM_IDENT.

[rwlbuis, 2015-03-03 21:47:25]

On 2015/03/03 02:26:18, Timothy Loh wrote:
> On 2015/03/03 02:06:20, rwlbuis wrote:
> > PTAL.
> 
> We should maybe try and fix this more generally. I think this involves
> - Distinguishing <custom-ident> and <string> in CSSValue.
> - Always quoting CSS_STRING, never quoting CSS_CUSTOM_IDENT.

Ah, much better! The patch is now much bigger because of all the
CSS_STRING replacements, but I agree it is the way to go.
The linux_blink_rel problem is unrelated to this change.

[Timothy Loh, 2015-03-03 23:20:16]

Looking better, nice to get rid of the CSSTextFormattingFlags. We should still
keep the parser state using CSS_STRING when we have <string> types though, and
it'd be better to just fix everywhere to use the right types instead of just
content. So you might need to revert quite a few of the changes here; I assume
you can work out what's a <string> and what's a <custom-ident> :)

https://codereview.chromium.org/973623002/diff/80001/Source/core/css/CSSValue...
File Source/core/css/CSSValuePool.cpp (right):

https://codereview.chromium.org/973623002/diff/80001/Source/core/css/CSSValue...
Source/core/css/CSSValuePool.cpp:133: value =
CSSPrimitiveValue::create(familyName, CSSPrimitiveValue::CSS_CUSTOM_IDENT);
probably still CSS_STRING

https://codereview.chromium.org/973623002/diff/80001/Source/core/css/parser/C...
File Source/core/css/parser/CSSGrammar.y (right):

https://codereview.chromium.org/973623002/diff/80001/Source/core/css/parser/C...
Source/core/css/parser/CSSGrammar.y:1630: | STRING maybe_space { $$.id =
CSSValueInvalid; $$.isInt = false; $$.string = $1; $$.unit =
CSSPrimitiveValue::CSS_CUSTOM_IDENT; }
These are still <string>s though, right?

https://codereview.chromium.org/973623002/diff/80001/Source/core/css/parser/C...
File Source/core/css/parser/CSSParserValues.cpp (right):

https://codereview.chromium.org/973623002/diff/80001/Source/core/css/parser/C...
Source/core/css/parser/CSSParserValues.cpp:180: value.unit =
CSSPrimitiveValue::CSS_CUSTOM_IDENT;
These should also stay as <string>

https://codereview.chromium.org/973623002/diff/80001/Source/core/css/parser/C...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/973623002/diff/80001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:325: ASSERT(value->unit ==
CSSPrimitiveValue::CSS_CUSTOM_IDENT || value->unit ==
CSSPrimitiveValue::CSS_IDENT);
I guess this check should stay as CSS_STRING || CSS_IDENT, and we'll create
these with either CSS_STRING or CSS_CUSTOM_IDENT types.

https://codereview.chromium.org/973623002/diff/80001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.cpp:2160: parsedValue =
CSSPrimitiveValue::create(val->string, CSSPrimitiveValue::CSS_CUSTOM_IDENT);
CSS_STRING

https://codereview.chromium.org/973623002/diff/80001/Source/core/css/parser/C...
File Source/core/css/parser/CSSPropertyParser.h (right):

https://codereview.chromium.org/973623002/diff/80001/Source/core/css/parser/C...
Source/core/css/parser/CSSPropertyParser.h:275:
PassRefPtrWillBeRawPtr<CSSPrimitiveValue>
createPrimitiveStringValue(CSSParserValue*, CSSPrimitiveValue::UnitType =
CSSPrimitiveValue::CSS_CUSTOM_IDENT);
Maybe slightly better to not have a default argument.

[rwlbuis, 2015-03-04 22:50:59]

On 2015/03/03 23:20:16, Timothy Loh wrote:
> Looking better, nice to get rid of the CSSTextFormattingFlags. We should still
> keep the parser state using CSS_STRING when we have <string> types though, and
> it'd be better to just fix everywhere to use the right types instead of just
> content. So you might need to revert quite a few of the changes here; I assume
> you can work out what's a <string> and what's a <custom-ident> :)

I agree with most of this except below two things.

>
https://codereview.chromium.org/973623002/diff/80001/Source/core/css/CSSValue...
> File Source/core/css/CSSValuePool.cpp (right):
> 
>
https://codereview.chromium.org/973623002/diff/80001/Source/core/css/CSSValue...
> Source/core/css/CSSValuePool.cpp:133: value =
> CSSPrimitiveValue::create(familyName, CSSPrimitiveValue::CSS_CUSTOM_IDENT);
> probably still CSS_STRING

That would actually be a big change, in the sense that a) a lot of tests would
need to be rebased and
b) FireFox does not even quote the family names. So I left it out of patchset
#7.

>
https://codereview.chromium.org/973623002/diff/80001/Source/core/css/parser/C...
> Source/core/css/parser/CSSPropertyParser.h:275:
> PassRefPtrWillBeRawPtr<CSSPrimitiveValue>
> createPrimitiveStringValue(CSSParserValue*, CSSPrimitiveValue::UnitType =
> CSSPrimitiveValue::CSS_CUSTOM_IDENT);
> Maybe slightly better to not have a default argument.

I prefer the default enum value, otherwise I need to update 6 callers and
the CSSPrimitiveValue::FOO stuff is a bit verbose.

Patch set #7 is starting to show up green so I think it is ready for review :)

[Timothy Loh, 2015-03-04 23:21:45]

I think the main reason why I'd rather have everything correctly set to
string/custom-ident is that otherwise this patch is basically adding support for
this, but incorrectly/inconsistently in a few places.

OK to leave <family-name> as is for now (it's either a string or list of idents,
so this is fine), but I think we should change the other properties to be
consistent (property parser should output same types as
LayoutStyleToCSSValueMapping).

>
https://codereview.chromium.org/973623002/diff/80001/Source/core/css/parser/C...
> > Source/core/css/parser/CSSPropertyParser.h:275:
> > PassRefPtrWillBeRawPtr<CSSPrimitiveValue>
> > createPrimitiveStringValue(CSSParserValue*, CSSPrimitiveValue::UnitType =
> > CSSPrimitiveValue::CSS_CUSTOM_IDENT);
> > Maybe slightly better to not have a default argument.
> 
> I prefer the default enum value, otherwise I need to update 6 callers and
> the CSSPrimitiveValue::FOO stuff is a bit verbose.

The function is called createPrimitiveStringValue though, so it's weird to
default to not actually creating a CSS_STRING type. Maybe it makes more sense to
split it up into createPrimitiveString and createPrimitiveCustomIdent?

https://codereview.chromium.org/973623002/diff/120001/Source/core/css/LayoutS...
File Source/core/css/LayoutStyleCSSValueMapping.cpp (right):

https://codereview.chromium.org/973623002/diff/120001/Source/core/css/LayoutS...
Source/core/css/LayoutStyleCSSValueMapping.cpp:1222: filterValue =
CSSFunctionValue::create(CSSValueUrl);
Maybe we should just make this use a CSS_URI here.

https://codereview.chromium.org/973623002/diff/120001/Source/core/css/LayoutS...
Source/core/css/LayoutStyleCSSValueMapping.cpp:1662: return
cssValuePool().createValue(style.highlight(),
CSSPrimitiveValue::CSS_CUSTOM_IDENT);
string

https://codereview.chromium.org/973623002/diff/120001/Source/core/css/LayoutS...
Source/core/css/LayoutStyleCSSValueMapping.cpp:1666: return
cssValuePool().createValue(style.hyphenationString(),
CSSPrimitiveValue::CSS_CUSTOM_IDENT);
string

https://codereview.chromium.org/973623002/diff/120001/Source/core/css/LayoutS...
Source/core/css/LayoutStyleCSSValueMapping.cpp:1698: return
cssValuePool().createValue(style.locale(), CSSPrimitiveValue::CSS_CUSTOM_IDENT);
string

https://codereview.chromium.org/973623002/diff/120001/Source/core/css/LayoutS...
Source/core/css/LayoutStyleCSSValueMapping.cpp:1866: return
cssValuePool().createValue(style.textEmphasisCustomMark(),
CSSPrimitiveValue::CSS_CUSTOM_IDENT);
string I think?

[rwlbuis, 2015-03-05 22:27:16]

On 2015/03/04 23:21:45, Timothy Loh wrote:
> I think the main reason why I'd rather have everything correctly set to
> string/custom-ident is that otherwise this patch is basically adding support
for
> this, but incorrectly/inconsistently in a few places.
> 
> OK to leave <family-name> as is for now (it's either a string or list of
idents,
> so this is fine), but I think we should change the other properties to be
> consistent (property parser should output same types as
> LayoutStyleToCSSValueMapping).

My latest patch set has all these comments addressed. Most of the rebaselines
are for the locale and content properties. Starting to turn up green!

[Timothy Loh, 2015-03-05 22:41:54]

On 2015/03/05 22:27:16, rwlbuis wrote:
> On 2015/03/04 23:21:45, Timothy Loh wrote:
> > I think the main reason why I'd rather have everything correctly set to
> > string/custom-ident is that otherwise this patch is basically adding support
> for
> > this, but incorrectly/inconsistently in a few places.
> > 
> > OK to leave <family-name> as is for now (it's either a string or list of
> idents,
> > so this is fine), but I think we should change the other properties to be
> > consistent (property parser should output same types as
> > LayoutStyleToCSSValueMapping).
> 
> My latest patch set has all these comments addressed. Most of the rebaselines
> are for the locale and content properties. Starting to turn up green!

Thanks for making these changes, lgtm!

[Timothy Loh, 2015-03-05 22:43:35]

https://codereview.chromium.org/973623002/diff/200001/Source/core/css/parser/...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/973623002/diff/200001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:395: return
createPrimitiveCustomIdentValue(value);
What hit's this case now?

[rwlbuis, 2015-03-05 22:56:17]

On 2015/03/05 22:43:35, Timothy Loh wrote:
>
https://codereview.chromium.org/973623002/diff/200001/Source/core/css/parser/...
> File Source/core/css/parser/CSSPropertyParser.cpp (right):
> 
>
https://codereview.chromium.org/973623002/diff/200001/Source/core/css/parser/...
> Source/core/css/parser/CSSPropertyParser.cpp:395: return
> createPrimitiveCustomIdentValue(value);
> What hit's this case now?
 
I added an ASSERT(false) there and it does not seem to get hit. So I'll remove
it
for the "patch for landing", can always revert to patchset #11 in case the
buildbot
indicates a problem.

[rwlbuis, 2015-03-05 23:02:36]

On 2015/03/05 22:56:17, rwlbuis wrote:
> On 2015/03/05 22:43:35, Timothy Loh wrote:
> >
>
https://codereview.chromium.org/973623002/diff/200001/Source/core/css/parser/...
> > File Source/core/css/parser/CSSPropertyParser.cpp (right):
> > 
> >
>
https://codereview.chromium.org/973623002/diff/200001/Source/core/css/parser/...
> > Source/core/css/parser/CSSPropertyParser.cpp:395: return
> > createPrimitiveCustomIdentValue(value);
> > What hit's this case now?
>  
> I added an ASSERT(false) there and it does not seem to get hit. So I'll remove
> it
> for the "patch for landing", can always revert to patchset #11 in case the
> buildbot
> indicates a problem.

Actually I spoke too soon ; CSSPropertyTextAlign is one case that
hits this, I see it in fast/css/first-letter-anonymous-block-crash.html

[rwlbuis, 2015-03-05 23:02:45]

The CQ bit was checked by rob.buis@samsung.com

[rwlbuis, 2015-03-05 23:03:30]

The CQ bit was unchecked by rob.buis@samsung.com

[Timothy Loh, 2015-03-05 23:15:31]

On 2015/03/05 23:02:36, rwlbuis wrote:
> On 2015/03/05 22:56:17, rwlbuis wrote:
> > On 2015/03/05 22:43:35, Timothy Loh wrote:
> > >
> >
>
https://codereview.chromium.org/973623002/diff/200001/Source/core/css/parser/...
> > > File Source/core/css/parser/CSSPropertyParser.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/973623002/diff/200001/Source/core/css/parser/...
> > > Source/core/css/parser/CSSPropertyParser.cpp:395: return
> > > createPrimitiveCustomIdentValue(value);
> > > What hit's this case now?
> >  
> > I added an ASSERT(false) there and it does not seem to get hit. So I'll
remove
> > it
> > for the "patch for landing", can always revert to patchset #11 in case the
> > buildbot
> > indicates a problem.
> 
> Actually I spoke too soon ; CSSPropertyTextAlign is one case that
> hits this, I see it in fast/css/first-letter-anonymous-block-crash.html

We don't even support <string>s on text-align more than parsing it. Maybe we
should just disallow it in the parser / stylebuilder? Otherwise feature
detection will be slightly off here.

[rwlbuis, 2015-03-05 23:31:31]

On 2015/03/05 23:15:31, Timothy Loh wrote:
> On 2015/03/05 23:02:36, rwlbuis wrote:
> > On 2015/03/05 22:56:17, rwlbuis wrote:
> > > On 2015/03/05 22:43:35, Timothy Loh wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/973623002/diff/200001/Source/core/css/parser/...
> > > > File Source/core/css/parser/CSSPropertyParser.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/973623002/diff/200001/Source/core/css/parser/...
> > > > Source/core/css/parser/CSSPropertyParser.cpp:395: return
> > > > createPrimitiveCustomIdentValue(value);
> > > > What hit's this case now?
> > >  
> > > I added an ASSERT(false) there and it does not seem to get hit. So I'll
> remove
> > > it
> > > for the "patch for landing", can always revert to patchset #11 in case the
> > > buildbot
> > > indicates a problem.
> > 
> > Actually I spoke too soon ; CSSPropertyTextAlign is one case that
> > hits this, I see it in fast/css/first-letter-anonymous-block-crash.html
> 
> We don't even support <string>s on text-align more than parsing it. Maybe we
> should just disallow it in the parser / stylebuilder? Otherwise feature
> detection will be slightly off here.

Can you maybe make a bug for this? Happy to work on it and I see some
opportunity
for further cleanup, but I think this patch is big enough as-is.

I changed the commit log which was getting a bit outdated.

[Timothy Loh, 2015-03-05 23:33:47]

On 2015/03/05 23:31:31, rwlbuis wrote:
> On 2015/03/05 23:15:31, Timothy Loh wrote:
> > We don't even support <string>s on text-align more than parsing it. Maybe we
> > should just disallow it in the parser / stylebuilder? Otherwise feature
> > detection will be slightly off here.
> 
> Can you maybe make a bug for this? Happy to work on it and I see some
> opportunity
> for further cleanup, but I think this patch is big enough as-is.
> 
> I changed the commit log which was getting a bit outdated.

Happy if you just land this as is and remove <string> from text-align later.
It's simple enough to not really need a bug for it...

[rwlbuis, 2015-03-05 23:44:06]

The CQ bit was checked by rob.buis@samsung.com

[rwlbuis, 2015-03-05 23:44:25]

The CQ bit was unchecked by rob.buis@samsung.com

[rwlbuis, 2015-03-06 00:56:03]

The CQ bit was checked by rob.buis@samsung.com

[commit-bot: I haz the power, 2015-03-06 00:56:28]

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

[commit-bot: I haz the power, 2015-03-06 01:21:27]

Committed patchset #11 (id:200001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191396