CSS: Implement 'font' property in CSSComputedStyle.

• Component: CSS
• Status: RESOLVED FIXED

[Andreas Kling]

This property is implemented by Opera, but not Gecko (haven't checked IE.)

[Andreas Kling]

Created attachment 104698 [details]
Proposed patch

Patch for comments. I'm not a big fan of the verbosity, so if someone knows a better way to safely express "RefPtr<BaseClass> = PassRefPtr<SubClass>", I'm all ears.

[Simon Fraser (smfr)]

Comment on attachment 104698 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=104698&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1110
> +            CSSValue* fontStyle = getPropertyCSSValue(CSSPropertyFontStyle, DoNotUpdateLayout).leakRef();
> +            ASSERT(fontStyle->isPrimitiveValue());
> +            computedFont->style = adoptRef(static_cast<CSSPrimitiveValue*>(fontStyle));

The leakRef() / adoptRef() pattern is weird. Why not just use RefPtrs?

(In reply to comment #1)
> Created an attachment (id=104698) [details]
> Proposed patch
> 
> Patch for comments. I'm not a big fan of the verbosity, so if someone knows a better way to safely express "RefPtr<BaseClass> = PassRefPtr<SubClass>", I'm all ears.

static_pointer_cast?

[Andreas Kling]

(In reply to comment #3)
> (In reply to comment #1)
> > Created an attachment (id=104698) [details] [details]
> > Proposed patch
> > 
> > Patch for comments. I'm not a big fan of the verbosity, so if someone knows a better way to safely express "RefPtr<BaseClass> = PassRefPtr<SubClass>", I'm all ears.
> 
> static_pointer_cast?

Magical! Thanks, I'll upload something slightly less ugly in a jiffy :)

[Andreas Kling]

Created attachment 104702 [details]
Proposed patch v2

Same patch using static_pointer_cast instead of making a mess.
I left out the assertions for isPrimitiveValue() and isValueList(), if you feel they did actually belong, let me know.

[Darin Adler]

Comment on attachment 104702 [details]
Proposed patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=104702&action=review

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1112
> +            computedFont->style = static_pointer_cast<CSSPrimitiveValue>(getPropertyCSSValue(CSSPropertyFontStyle, DoNotUpdateLayout));
> +            computedFont->variant = static_pointer_cast<CSSPrimitiveValue>(getPropertyCSSValue(CSSPropertyFontVariant, DoNotUpdateLayout));
> +            computedFont->weight = static_pointer_cast<CSSPrimitiveValue>(getPropertyCSSValue(CSSPropertyFontWeight, DoNotUpdateLayout));
> +            computedFont->size = static_pointer_cast<CSSPrimitiveValue>(getPropertyCSSValue(CSSPropertyFontSize, DoNotUpdateLayout));
> +            computedFont->lineHeight = static_pointer_cast<CSSPrimitiveValue>(getPropertyCSSValue(CSSPropertyLineHeight, DoNotUpdateLayout));
> +            computedFont->family = static_pointer_cast<CSSValueList>(getPropertyCSSValue(CSSPropertyFontFamily, DoNotUpdateLayout));

You mentioned that you left out assertions, which seems OK. But what guarantees the pointers will be to objects of the right type? Is there some guarantee?

[Andreas Kling]

(In reply to comment #6)
> (From update of attachment 104702 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=104702&action=review
> 
> > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1112
> > +            computedFont->style = static_pointer_cast<CSSPrimitiveValue>(getPropertyCSSValue(CSSPropertyFontStyle, DoNotUpdateLayout));
> > +            computedFont->variant = static_pointer_cast<CSSPrimitiveValue>(getPropertyCSSValue(CSSPropertyFontVariant, DoNotUpdateLayout));
> > +            computedFont->weight = static_pointer_cast<CSSPrimitiveValue>(getPropertyCSSValue(CSSPropertyFontWeight, DoNotUpdateLayout));
> > +            computedFont->size = static_pointer_cast<CSSPrimitiveValue>(getPropertyCSSValue(CSSPropertyFontSize, DoNotUpdateLayout));
> > +            computedFont->lineHeight = static_pointer_cast<CSSPrimitiveValue>(getPropertyCSSValue(CSSPropertyLineHeight, DoNotUpdateLayout));
> > +            computedFont->family = static_pointer_cast<CSSValueList>(getPropertyCSSValue(CSSPropertyFontFamily, DoNotUpdateLayout));
> 
> You mentioned that you left out assertions, which seems OK. But what guarantees the pointers will be to objects of the right type? Is there some guarantee?

The current implementation of the included properties in getPropertyCSSValue() cannot return any other types.

This is a valid guarantee right now, but I wouldn't exactly call it future-proof. I don't believe the types are checked anywhere else. I'll gladly put the asserts back in, or add them elsewhere (something like CSSValue::asPrimitiveValue() and CSSValue::asValueList() perhaps?)

[Darin Adler]

(In reply to comment #7)
> add them elsewhere (something like CSSValue::asPrimitiveValue() and CSSValue::asValueList() perhaps?)

I think that’s one thing we are going to want in the future.

But an even better idea for this case here would be to factor out private functions in CSSComputedStyleDeclaration for each of these properties that return these computed pieces and then call them both from the big switch statement and from here.

[Andreas Kling]

Comment on attachment 104702 [details]
Proposed patch v2

(In reply to comment #8)
> But an even better idea for this case here would be to factor out private functions in CSSComputedStyleDeclaration for each of these properties that return these computed pieces and then call them both from the big switch statement and from here.

Yep, that's a great idea. It also exposes the case where CSSPropertyFontFamily would be requested with !style->fontDescription().family().next() yielding a CSSPrimitiveValue return value, rather than a CSSValueList.

Clearing r? until I prepare a new patch.

[Andreas Kling]

Created attachment 104738 [details]
Proposed patch v3

Factored the CSSPropertyFont* code out of getPropertyCSSValue() to guarantee consistent return types.
CSSPropertyFontFamily will now always yield a CSSValueList, where it would previously return a CSSPrimitiveValue when there was only a single family.
They serialize just the same, and the storage overhead for a Vector<RefPtr<CSSValue>> (and a bool) seems negligible.

[WebKit Review Bot]

Attachment 104738 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/css/CSSComputedStyleDeclaration.cpp:851:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

(In reply to comment #10)
> CSSPropertyFontFamily will now always yield a CSSValueList, where it would previously return a CSSPrimitiveValue when there was only a single family.
> They serialize just the same, and the storage overhead for a Vector<RefPtr<CSSValue>> (and a bool) seems negligible.

This is an observable change from JavaScript, not just a performance-affecting one. Can we test and see what other browsers do? Unfortunately, the computed value in CSS simply says “as specified” and so does not make it clear what the CSS DOM object type should be.

[WebKit Review Bot]

Comment on attachment 104738 [details]
Proposed patch v3

Attachment 104738 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9468869

New failing tests:
svg/css/getComputedStyle-basic.xhtml
fast/dom/setPrimitiveValue-exceptions.html

[Andreas Kling]

(In reply to comment #12)
> (In reply to comment #10)
> > CSSPropertyFontFamily will now always yield a CSSValueList, where it would previously return a CSSPrimitiveValue when there was only a single family.
> > They serialize just the same, and the storage overhead for a Vector<RefPtr<CSSValue>> (and a bool) seems negligible.
> 
> This is an observable change from JavaScript, not just a performance-affecting one. Can we test and see what other browsers do? Unfortunately, the computed value in CSS simply says “as specified” and so does not make it clear what the CSS DOM object type should be.

Righto, I did some testing.

Gecko returns a CSSPrimitiveValue in both cases, e.g "sans-serif" for the single-family case, and "sans-serif, sans-serif, etc" for the multi-family case.

Opera (11.50, Linux) throws indecipherable exceptions when I call getPropertyCSSValue() with any arguments.

IE does not support getPropertyCSSValue() at all.

I personally feel that always returning CSSValueList makes the most sense, but it might be better to match Gecko. Thoughts?

[Darin Adler]

(In reply to comment #14)
> I personally feel that always returning CSSValueList makes the most sense, but it might be better to match Gecko.

Matching both Gecko and older versions of WebKit seems like the smart approach for compatibility. I don’t know the importance, though.

[Andreas Kling]

Comment on attachment 104738 [details]
Proposed patch v3

Clearing r? while I prepare something more backwards-compatible.

[Andreas Kling]

Created attachment 108164 [details]
Proposed patch v4

Keep the old (WebKit) behavior of getPropertyCSSValue('font-family'), i.e returning a CSSPrimitiveValue for single-family values and a CSSValueList for multiple families.
Added a comment about this in the CSSPropertyFontFamily block of getPropertyCSSValue().
Also extended the test to exercise getPropertyCSSValue() as well as getPropertyValue().

[WebKit Review Bot]

Attachment 108164 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/css/CSSComputedStyleDeclaration.cpp:970:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Comment on attachment 108164 [details]
Proposed patch v4

r=me with the style fix.

[Andreas Kling]

Created attachment 108315 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 108315 [details]
Patch for landing

Clearing flags on attachment: 108315

Committed r95714: <http://trac.webkit.org/changeset/95714>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.