Bug 140577 – Remove 'font' shorthand property special casing

• Component: CSS
• Status: RESOLVED FIXED

[Chris Dumez]

Remove 'font' shorthand property special casing by expanding system font values during property parsing. This way, 'font' can be treated as a regular shorthand.

[Chris Dumez]

https://src.chromium.org/viewvc/blink?view=rev&revision=184629 seems related as well.

[Chris Dumez]

Created attachment 245017 [details]
WIP Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/css/CSSParser.cpp:2896:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/WebCore/css/CSSParser.cpp:2897:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 8 files


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

[Build Bot]

Comment on attachment 245017 [details]
WIP Patch

Attachment 245017 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6584637136568320

New failing tests:
fast/text/backslash-to-yen-sign-euc.html
editing/selection/find-yensign-and-backslash.html

[Build Bot]

Created attachment 245027 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 245017 [details]
WIP Patch

Attachment 245017 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5836784546086912

New failing tests:
fast/text/backslash-to-yen-sign-euc.html
editing/selection/find-yensign-and-backslash.html

[Build Bot]

Created attachment 245051 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Chris Dumez]

Created attachment 245173 [details]
WIP Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/css/CSSPrimitiveValue.h:178:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WTF/wtf/HashFunctions.h:176:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 18 files


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

[Chris Dumez]

Created attachment 245182 [details]
WIP patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/css/CSSPrimitiveValue.h:153:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WTF/wtf/HashFunctions.h:176:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 20 files


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

[Build Bot]

Comment on attachment 245182 [details]
WIP patch

Attachment 245182 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6563632062136320

New failing tests:
fast/css/font-shorthand-line-height.html

[Build Bot]

Created attachment 245197 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Chris Dumez]

Created attachment 245201 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/css/CSSPrimitiveValue.h:153:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WTF/wtf/HashFunctions.h:176:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 3 in 32 files


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

[Build Bot]

Comment on attachment 245201 [details]
Patch

Attachment 245201 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6075983622832128

New failing tests:
fast/css/font-shorthand-line-height.html

[Build Bot]

Created attachment 245206 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 245201 [details]
Patch

Attachment 245201 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5504138792140800

New failing tests:
fast/css/font-shorthand-line-height.html

[Build Bot]

Created attachment 245207 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Chris Dumez]

Created attachment 245209 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/css/CSSPrimitiveValue.h:153:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WTF/wtf/HashFunctions.h:176:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 3 in 34 files


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

[Chris Dumez]

Created attachment 245212 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/css/CSSPrimitiveValue.h:153:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WTF/wtf/HashFunctions.h:176:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 3 in 34 files


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

[Chris Dumez]

Created attachment 245254 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/css/CSSPrimitiveValue.h:153:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
ERROR: Source/WTF/wtf/HashFunctions.h:176:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 3 in 35 files


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

[Chris Dumez]

Created attachment 245273 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
ERROR: Source/WebCore/css/CSSPrimitiveValue.h:153:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 19 files


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

[Chris Dumez]

Created attachment 245274 [details]
Patch

[Chris Dumez]

Comment on attachment 245274 [details]
Patch

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

> Source/WebCore/css/CSSValuePool.h:86
> +    typedef HashMap<std::pair<String, unsigned /* fromSystemFontID */>, RefPtr<CSSPrimitiveValue>> FontFamilyValueCache;

We should be able to use bool instead of unsigned here. I filed Bug 140848 for this as it is not strictly related.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/css/CSSPrimitiveValue.h:153:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 20 files


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

[Chris Dumez]

Created attachment 245285 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/css/CSSPrimitiveValue.h:153:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 20 files


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

[Darin Adler]

Comment on attachment 245285 [details]
Patch

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

I’m going to say r=me here, but I think this is a little more complex than it needs to be. We really don’t need to be allocating a reference counted object on the heap just to hold a string and a boolean.

> Source/WebCore/css/CSSFontFamily.h:33
> +// This is because require an extra bit to determine if the font family was resolved by the CSS
> +// CSS parser from a system font ID (caption | icon | menu | message-box | small-caption |

Double CSS here: "CSS CSS parser".

> Source/WebCore/css/CSSFontFamily.h:39
> +class CSSFontFamily : public RefCounted<CSSFontFamily> {

Does this really need to be a heap allocated, reference counted object? Can’t this just be a struct?

> Source/WebCore/css/CSSParser.cpp:6290
> +    RefPtr<CSSValueList> fontFamilyList = CSSValueList::createCommaSeparated();

Should be Ref, not RefPtr.

> Source/WebCore/css/CSSParser.cpp:6291
> +    fontFamilyList->append(cssValuePool().createFontFamilyValue(fontDescription.familyAt(0), true /* fromSystemFontID */));

This "true with comment" stinks. We should find a way to do better.

> Source/WebCore/css/StyleBuilderCustom.h:865
> +            // If the family name was resolved by the CSS parser from a system
> +            // font ID, then it is generic.

Single line for this comment please. No good reason to break it.

> Source/WebCore/css/makeprop.pl:1020
> +        return Vector<StylePropertyShorthand>();

We should instead just write:

    return { };

Unless that’s inefficient or something. It’s annoying to have to say the type name just to return nothing.

[Chris Dumez]

Comment on attachment 245285 [details]
Patch

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

>> Source/WebCore/css/CSSFontFamily.h:39
>> +class CSSFontFamily : public RefCounted<CSSFontFamily> {
> 
> Does this really need to be a heap allocated, reference counted object? Can’t this just be a struct?

It probably doesn't need to be RefCounted, I'll check. All other internal representations of CSSPrimitiveValue were so I just followed the pattern. That said, I think we may still want to heap allocate it. CSSPrimitiveValue is using a union containing mostly pointers (size <= 1 word). If I add a CSSFontFamily instead of a CSSFontFamily* to this union, I will increase the size of all CSSPrimitiveValues by 1 word.

>> Source/WebCore/css/CSSParser.cpp:6291
>> +    fontFamilyList->append(cssValuePool().createFontFamilyValue(fontDescription.familyAt(0), true /* fromSystemFontID */));
> 
> This "true with comment" stinks. We should find a way to do better.

Will switch to using an enum like we often do.

[Chris Dumez]

Created attachment 245290 [details]
Patch

[Chris Dumez]

Comment on attachment 245285 [details]
Patch

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

>> Source/WebCore/css/CSSFontFamily.h:33
>> +// CSS parser from a system font ID (caption | icon | menu | message-box | small-caption |
> 
> Double CSS here: "CSS CSS parser".

Done.

>>> Source/WebCore/css/CSSFontFamily.h:39
>>> +class CSSFontFamily : public RefCounted<CSSFontFamily> {
>> 
>> Does this really need to be a heap allocated, reference counted object? Can’t this just be a struct?
> 
> It probably doesn't need to be RefCounted, I'll check. All other internal representations of CSSPrimitiveValue were so I just followed the pattern. That said, I think we may still want to heap allocate it. CSSPrimitiveValue is using a union containing mostly pointers (size <= 1 word). If I add a CSSFontFamily instead of a CSSFontFamily* to this union, I will increase the size of all CSSPrimitiveValues by 1 word.

No longer ref-counted.

>> Source/WebCore/css/CSSParser.cpp:6290
>> +    RefPtr<CSSValueList> fontFamilyList = CSSValueList::createCommaSeparated();
> 
> Should be Ref, not RefPtr.

Done.

>>> Source/WebCore/css/CSSParser.cpp:6291
>>> +    fontFamilyList->append(cssValuePool().createFontFamilyValue(fontDescription.familyAt(0), true /* fromSystemFontID */));
>> 
>> This "true with comment" stinks. We should find a way to do better.
> 
> Will switch to using an enum like we often do.

Done.

>> Source/WebCore/css/StyleBuilderCustom.h:865
>> +            // font ID, then it is generic.
> 
> Single line for this comment please. No good reason to break it.

Done.

>> Source/WebCore/css/makeprop.pl:1020
>> +        return Vector<StylePropertyShorthand>();
> 
> We should instead just write:
> 
>     return { };
> 
> Unless that’s inefficient or something. It’s annoying to have to say the type name just to return nothing.

Done.

[WebKit Commit Bot]

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


ERROR: Source/WebCore/css/CSSPrimitiveValue.h:153:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 20 files


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

[WebKit Commit Bot]

Comment on attachment 245290 [details]
Patch

Rejecting attachment 245290 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 245290, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/6674250018586624

[Darin Adler]

Comment on attachment 245290 [details]
Patch

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

This is fine, and an improvement over the first patch. I still have some thoughts about this; we are overdoing the use of pointers and the heap a bit. We are following the pattern of some older code that isn’t all that well thought out.

> Source/WebCore/css/CSSFontFamily.h:42
> +// sign" hack.
> +enum FromSystemFontIDOrNot { NotFromSystemFontID, FromSystemFontID };
> +class CSSFontFamily {

This part of the source file looks really crowded. I’d add a couple blank lines to make it more readable.

Since this class is just two data members with no real encapsulation, why not use a struct instead? We could still std::make_unique with a struct. If we had a struct and an == function we’d be better off. The reason I suggest a struct rather than just using std::pair is that the struct lets us name the two data members.

> Source/WebCore/css/CSSParser.cpp:2896
> +        if (num == 1 && id >= CSSValueCaption && id <= CSSValueStatusBar) {

The "num == 1" check here is a good bug fix. Do we have test coverage for it?

> Source/WebCore/css/CSSPrimitiveValue.cpp:505
> +        m_value.fontFamily = 0;

nullptr

> Source/WebCore/css/CSSPrimitiveValue.cpp:1232
> +        result = CSSPrimitiveValue::create(std::make_unique<CSSFontFamily>(m_value.fontFamily->family(), m_value.fontFamily->fromSystemFontID()));

No reason to copy this indirect way. More direct way:

    std::make_unique<CSSFontFamily>(*m_value.fontFamily);

But also, I think the make_unique should be inside CSSPrimitiveValue. See my other comments above.

> Source/WebCore/css/CSSPrimitiveValue.h:348
> +    CSSFontFamily* getFontFamily() const { return m_primitiveUnitType != CSS_FONT_FAMILY ? nullptr : m_value.fontFamily; }

The pattern here isn’t great. In reality, both callers of getFontFamily already know the type is font family since they always call isFontFamily first. So the use of “get” in the name and the use of a null value are both unnecessary. It would be better to just have:

    const CSSFontFamily& fontFamily() const { ASSERT(isFontFamily()); return *m_value.fontFamily; }

> Source/WebCore/css/CSSPrimitiveValue.h:423
> +    void init(std::unique_ptr<CSSFontFamily>&&);

Seems to me this could just take a const CSSFontFamily& and do the heap allocation inside the init function. I see no reason the caller needs to have the CSSFontFamily already allocated on the heap. It’s only CSSPrimitiveValue that wants to put it on the heap (to avoid making the CSSPrimitiveValue larger). We’d have to make sure the forwarding from the constructor to init worked, of course.

> Source/WebCore/css/CSSValuePool.cpp:123
> +    RefPtr<CSSPrimitiveValue>& value = m_fontFamilyValueCache.add(std::make_pair(familyName, fromSystemFontID), nullptr).iterator->value;

I think that we can use { } syntax instead of make_pair:

    m_fontFamilyValueCache.add({ familyName, fromSystemFontID }, nullptr)

> Source/WebCore/css/CSSValuePool.h:87
> +    typedef HashMap<std::pair<String, bool>, RefPtr<CSSPrimitiveValue>> FontFamilyValueCache;

I’d be tempted to make the key be a CSSFontFamily rather than a pair. But then I suppose we’d have to implement the hash function for a CSSFontFamily.

[Chris Dumez]

Created attachment 245314 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/css/CSSPrimitiveValue.h:153:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 23 files


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

[Chris Dumez]

Comment on attachment 245290 [details]
Patch

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

>> Source/WebCore/css/CSSFontFamily.h:42
>> +class CSSFontFamily {
> 
> This part of the source file looks really crowded. I’d add a couple blank lines to make it more readable.
> 
> Since this class is just two data members with no real encapsulation, why not use a struct instead? We could still std::make_unique with a struct. If we had a struct and an == function we’d be better off. The reason I suggest a struct rather than just using std::pair is that the struct lets us name the two data members.

Done. Now a struct with the 2 data members, a constructor and an operator==.

>> Source/WebCore/css/CSSParser.cpp:2896
>> +        if (num == 1 && id >= CSSValueCaption && id <= CSSValueStatusBar) {
> 
> The "num == 1" check here is a good bug fix. Do we have test coverage for it?

Done, I added a layout test to cover this case in the latest iteration.

>> Source/WebCore/css/CSSPrimitiveValue.cpp:505
>> +        m_value.fontFamily = 0;
> 
> nullptr

old habits.. Done.

>> Source/WebCore/css/CSSPrimitiveValue.cpp:1232
>> +        result = CSSPrimitiveValue::create(std::make_unique<CSSFontFamily>(m_value.fontFamily->family(), m_value.fontFamily->fromSystemFontID()));
> 
> No reason to copy this indirect way. More direct way:
> 
>     std::make_unique<CSSFontFamily>(*m_value.fontFamily);
> 
> But also, I think the make_unique should be inside CSSPrimitiveValue. See my other comments above.

Done. No make_unique anymore even.

>> Source/WebCore/css/CSSPrimitiveValue.h:348
>> +    CSSFontFamily* getFontFamily() const { return m_primitiveUnitType != CSS_FONT_FAMILY ? nullptr : m_value.fontFamily; }
> 
> The pattern here isn’t great. In reality, both callers of getFontFamily already know the type is font family since they always call isFontFamily first. So the use of “get” in the name and the use of a null value are both unnecessary. It would be better to just have:
> 
>     const CSSFontFamily& fontFamily() const { ASSERT(isFontFamily()); return *m_value.fontFamily; }

Right, done.

>> Source/WebCore/css/CSSPrimitiveValue.h:423
>> +    void init(std::unique_ptr<CSSFontFamily>&&);
> 
> Seems to me this could just take a const CSSFontFamily& and do the heap allocation inside the init function. I see no reason the caller needs to have the CSSFontFamily already allocated on the heap. It’s only CSSPrimitiveValue that wants to put it on the heap (to avoid making the CSSPrimitiveValue larger). We’d have to make sure the forwarding from the constructor to init worked, of course.

Done.

>> Source/WebCore/css/CSSValuePool.cpp:123
>> +    RefPtr<CSSPrimitiveValue>& value = m_fontFamilyValueCache.add(std::make_pair(familyName, fromSystemFontID), nullptr).iterator->value;
> 
> I think that we can use { } syntax instead of make_pair:
> 
>     m_fontFamilyValueCache.add({ familyName, fromSystemFontID }, nullptr)

Done.

>> Source/WebCore/css/CSSValuePool.h:87
>> +    typedef HashMap<std::pair<String, bool>, RefPtr<CSSPrimitiveValue>> FontFamilyValueCache;
> 
> I’d be tempted to make the key be a CSSFontFamily rather than a pair. But then I suppose we’d have to implement the hash function for a CSSFontFamily.

Doing so adds more code / constraints (We needs to define the right HashTraits for CSSFontFamily and CSSFontFamily needs to have a default constructor). I give this a try then went back to an std::pair for the key because I don't think it is worth the extra code.

[Darin Adler]

Comment on attachment 245314 [details]
Patch

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

I have six small comments; I was tempted to set commit-queue+ anyway, but I didn’t.

> Source/WebCore/ChangeLog:58
> +        Support constructing a CSSPrimitiveValue from a Ref<CSSFontFamily>.

Comment no longer accurate.

> Source/WebCore/css/CSSFontFamily.h:49
> +    CSSFontFamily(const String& familyName, FromSystemFontIDOrNot fromSystemFontID)
> +        : familyName(familyName)
> +        , fromSystemFontID(fromSystemFontID)
> +    {
> +    }

We don’t need this, since we can use the { } syntax to initialize a struct.

> Source/WebCore/css/CSSFontFamily.h:54
> +    bool operator==(const CSSFontFamily& other) const
> +    {
> +        return familyName == other.familyName && fromSystemFontID == other.fromSystemFontID;
> +    }

I think it would be more elegant to have this as a free function rather than a member function.

> Source/WebCore/css/CSSPrimitiveValue.h:348
> +    CSSFontFamily& fontFamily() const { ASSERT(m_primitiveUnitType == CSS_FONT_FAMILY); return *m_value.fontFamily; }

Return type should be const reference, not non-const reference.

> Source/WebCore/css/CSSPrimitiveValueMappings.h:4926
> +    m_value.fontFamily = new CSSFontFamily(fontFamily);

Might save one reference count churn on a string by using WTF::move(fontFamily) here.

> Source/WebCore/css/CSSValuePool.cpp:126
> +        value = CSSPrimitiveValue::create(CSSFontFamily(familyName, fromSystemFontID));

If we take out the unneeded CSSFontFamily constructor, we would just use CSSFontFamily{familyName, fromSystemFontID} here.

[Darin Adler]

Comment on attachment 245290 [details]
Patch

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

>>> Source/WebCore/css/CSSValuePool.h:87
>>> +    typedef HashMap<std::pair<String, bool>, RefPtr<CSSPrimitiveValue>> FontFamilyValueCache;
>> 
>> I’d be tempted to make the key be a CSSFontFamily rather than a pair. But then I suppose we’d have to implement the hash function for a CSSFontFamily.
> 
> Doing so adds more code / constraints (We needs to define the right HashTraits for CSSFontFamily and CSSFontFamily needs to have a default constructor). I give this a try then went back to an std::pair for the key because I don't think it is worth the extra code.

I think you made the right call.

But a quick comment on the default constructor part of this. The right way to do this in this case is:

1) Don’t explicitly define any constructor for this struct; we don’t need one because of aggregate initialization syntax.
2) Give initial values to all scalars in the struct. It ends up just looking like this:

    FromSystemFontIDOrNot fromSystemFontID { NotFromSystemFontID };

Then we would not need to explicitly define a default constructor.

[Chris Dumez]

Created attachment 245315 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/css/CSSValuePool.cpp:126:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/css/CSSPrimitiveValue.h:153:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 23 files


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

[Chris Dumez]

Comment on attachment 245314 [details]
Patch

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

>> Source/WebCore/ChangeLog:58
>> +        Support constructing a CSSPrimitiveValue from a Ref<CSSFontFamily>.
> 
> Comment no longer accurate.

Removed.

>> Source/WebCore/css/CSSFontFamily.h:49
>> +    }
> 
> We don’t need this, since we can use the { } syntax to initialize a struct.

Removed.

>> Source/WebCore/css/CSSFontFamily.h:54
>> +    }
> 
> I think it would be more elegant to have this as a free function rather than a member function.

Done.

>> Source/WebCore/css/CSSPrimitiveValue.h:348
>> +    CSSFontFamily& fontFamily() const { ASSERT(m_primitiveUnitType == CSS_FONT_FAMILY); return *m_value.fontFamily; }
> 
> Return type should be const reference, not non-const reference.

Done.

>> Source/WebCore/css/CSSPrimitiveValueMappings.h:4926
>> +    m_value.fontFamily = new CSSFontFamily(fontFamily);
> 
> Might save one reference count churn on a string by using WTF::move(fontFamily) here.

Done.

>> Source/WebCore/css/CSSValuePool.cpp:126
>> +        value = CSSPrimitiveValue::create(CSSFontFamily(familyName, fromSystemFontID));
> 
> If we take out the unneeded CSSFontFamily constructor, we would just use CSSFontFamily{familyName, fromSystemFontID} here.

Done.

[Chris Dumez]

Created attachment 245316 [details]
Patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/css/CSSValuePool.cpp:126:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/WebCore/css/CSSPrimitiveValue.h:153:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 23 files


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

[WebKit Commit Bot]

Comment on attachment 245316 [details]
Patch

Clearing flags on attachment: 245316

Committed r179100: <http://trac.webkit.org/changeset/179100>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This broke many tests: https://build.webkit.org/results/Apple%20Yosemite%20Release%20WK1%20(Tests)/r179100%20(2282)/results.html

[Darin Adler]

I’m trying to find Chris to figure out why those tests are broken.

Apparently they all passed on the EWS server, so maybe it was a last minute change that broke things?

[Chris Dumez]

The patch landed through the CQ and all EWS was green. Not sure what happened, I am taking a look now.

[Chris Dumez]

Comment on attachment 245316 [details]
Patch

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

> Source/WebCore/css/CSSValuePool.cpp:124
> +    RefPtr<CSSPrimitiveValue>& value = m_fontFamilyValueCache.add({familyName, fromSystemFontID}, nullptr).iterator->value;

The issue seems to be with this cache. There is no issue if I run the tests individually or if I disable this cache.

[Alexey Proskuryakov]

Any update? I'd prefer to not have so many tests broken in the morning, when people come to work and check out trunk locally.

[Chris Dumez]

(In reply to comment #56)
> Any update? I'd prefer to not have so many tests broken in the morning, when
> people come to work and check out trunk locally.

I am still working on the fix. If I don't have it fixed tonight, I'll roll out.
I have a fix but I am trying to understand why it works.

[Chris Dumez]

Reopening to attach new patch.

[Chris Dumez]

Created attachment 245328 [details]
Patch

[Chris Dumez]

Comment on attachment 245328 [details]
Patch

Clearing flags on attachment: 245328

Committed r179105: <http://trac.webkit.org/changeset/179105>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

Ok, looks like the bots are green again:
https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20%28Tests%29

Sorry about the delay.

[Chris Dumez]

The bots seems to show a 5.8% progression on DoYouEvenBench with this patch:
Mac: https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22mac-mavericks%22%2C%22DoYouEvenBench%2FFull%3ATime%3ATotal%22%5D%5D (~5.8%)
GTK: https://perf.webkit.org/#mode=charts&chartList=%5B%5B%22gtk%22%2C%22DoYouEvenBench%2FFull%3ATime%3ATotal%22%5D%5D (~4%)