Bug 140577 - Remove 'font' shorthand property special casing
Summary: Remove 'font' shorthand property special casing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL: https://src.chromium.org/viewvc/blink...
Keywords: BlinkMergeCandidate
Depends on: 140801 140810 140829 140840 140848 140891
Blocks: 140817
  Show dependency treegraph
 
Reported: 2015-01-16 16:54 PST by Chris Dumez
Modified: 2015-02-02 13:19 PST (History)
7 users (show)

See Also:


Attachments
WIP Patch (20.31 KB, patch)
2015-01-20 14:18 PST, Chris Dumez
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (1.20 MB, application/zip)
2015-01-20 15:25 PST, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-mavericks (859.54 KB, application/zip)
2015-01-20 23:21 PST, Build Bot
no flags Details
WIP Patch (34.44 KB, patch)
2015-01-22 15:25 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP patch (40.89 KB, patch)
2015-01-22 16:30 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-mavericks (549.35 KB, application/zip)
2015-01-22 19:44 PST, Build Bot
no flags Details
Patch (74.07 KB, patch)
2015-01-22 20:41 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-mavericks (516.56 KB, application/zip)
2015-01-22 21:55 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-mavericks-wk2 (639.29 KB, application/zip)
2015-01-22 22:02 PST, Build Bot
no flags Details
Patch (75.50 KB, patch)
2015-01-22 22:10 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (76.01 KB, patch)
2015-01-22 23:09 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (78.80 KB, patch)
2015-01-23 15:32 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (41.40 KB, patch)
2015-01-23 20:17 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (46.06 KB, patch)
2015-01-23 21:10 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (46.08 KB, patch)
2015-01-24 09:50 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (46.57 KB, patch)
2015-01-24 19:27 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (48.48 KB, patch)
2015-01-25 14:46 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (48.21 KB, patch)
2015-01-25 16:38 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (48.32 KB, patch)
2015-01-25 16:57 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (1.44 KB, patch)
2015-01-25 22:56 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2015-01-16 16:54:53 PST
Remove 'font' shorthand property special casing by expanding system font values during property parsing. This way, 'font' can be treated as a regular shorthand.
Comment 1 Chris Dumez 2015-01-16 17:09:47 PST
https://src.chromium.org/viewvc/blink?view=rev&revision=184629 seems related as well.
Comment 2 Chris Dumez 2015-01-20 14:18:38 PST
Created attachment 245017 [details]
WIP Patch
Comment 3 WebKit Commit Bot 2015-01-20 14:20:48 PST
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.
Comment 4 Build Bot 2015-01-20 15:25:21 PST
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
Comment 5 Build Bot 2015-01-20 15:25:24 PST
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
Comment 6 Build Bot 2015-01-20 23:21:34 PST
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
Comment 7 Build Bot 2015-01-20 23:21:40 PST
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
Comment 8 Chris Dumez 2015-01-22 15:25:32 PST
Created attachment 245173 [details]
WIP Patch
Comment 9 WebKit Commit Bot 2015-01-22 15:29:10 PST
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.
Comment 10 Chris Dumez 2015-01-22 16:30:26 PST
Created attachment 245182 [details]
WIP patch
Comment 11 WebKit Commit Bot 2015-01-22 16:33:41 PST
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.
Comment 12 Build Bot 2015-01-22 19:44:25 PST
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
Comment 13 Build Bot 2015-01-22 19:44:27 PST
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
Comment 14 Chris Dumez 2015-01-22 20:41:43 PST
Created attachment 245201 [details]
Patch
Comment 15 WebKit Commit Bot 2015-01-22 20:44:03 PST
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.
Comment 16 Build Bot 2015-01-22 21:55:18 PST
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
Comment 17 Build Bot 2015-01-22 21:55:21 PST
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
Comment 18 Build Bot 2015-01-22 22:02:13 PST
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
Comment 19 Build Bot 2015-01-22 22:02:21 PST
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
Comment 20 Chris Dumez 2015-01-22 22:10:25 PST
Created attachment 245209 [details]
Patch
Comment 21 WebKit Commit Bot 2015-01-22 22:12:39 PST
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.
Comment 22 Chris Dumez 2015-01-22 23:09:02 PST
Created attachment 245212 [details]
Patch
Comment 23 WebKit Commit Bot 2015-01-22 23:11:41 PST
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.
Comment 24 Chris Dumez 2015-01-23 15:32:04 PST
Created attachment 245254 [details]
Patch
Comment 25 WebKit Commit Bot 2015-01-23 15:34:34 PST
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.
Comment 26 Chris Dumez 2015-01-23 20:17:54 PST
Created attachment 245273 [details]
Patch
Comment 27 WebKit Commit Bot 2015-01-23 20:19:17 PST
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.
Comment 28 Chris Dumez 2015-01-23 21:10:53 PST
Created attachment 245274 [details]
Patch
Comment 29 Chris Dumez 2015-01-23 21:12:50 PST
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.
Comment 30 WebKit Commit Bot 2015-01-23 21:13:18 PST
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.
Comment 31 Chris Dumez 2015-01-24 09:50:04 PST
Created attachment 245285 [details]
Patch
Comment 32 WebKit Commit Bot 2015-01-24 09:53:16 PST
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.
Comment 33 Darin Adler 2015-01-24 15:23:32 PST
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.
Comment 34 Chris Dumez 2015-01-24 17:24:55 PST
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.
Comment 35 Chris Dumez 2015-01-24 19:27:56 PST
Created attachment 245290 [details]
Patch
Comment 36 Chris Dumez 2015-01-24 19:29:23 PST
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.
Comment 37 WebKit Commit Bot 2015-01-24 19:30:08 PST
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.
Comment 38 WebKit Commit Bot 2015-01-25 11:54:18 PST
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
Comment 39 Darin Adler 2015-01-25 12:37:49 PST
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.
Comment 40 Chris Dumez 2015-01-25 14:46:54 PST
Created attachment 245314 [details]
Patch
Comment 41 WebKit Commit Bot 2015-01-25 14:49:07 PST
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.
Comment 42 Chris Dumez 2015-01-25 14:50:57 PST
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.
Comment 43 Darin Adler 2015-01-25 14:55:20 PST
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.
Comment 44 Darin Adler 2015-01-25 14:59:07 PST
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.
Comment 45 Chris Dumez 2015-01-25 16:38:39 PST
Created attachment 245315 [details]
Patch
Comment 46 WebKit Commit Bot 2015-01-25 16:40:04 PST
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.
Comment 47 Chris Dumez 2015-01-25 16:42:27 PST
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.
Comment 48 Chris Dumez 2015-01-25 16:57:52 PST
Created attachment 245316 [details]
Patch
Comment 49 WebKit Commit Bot 2015-01-25 16:59:28 PST
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.
Comment 50 WebKit Commit Bot 2015-01-25 18:11:18 PST
Comment on attachment 245316 [details]
Patch

Clearing flags on attachment: 245316

Committed r179100: <http://trac.webkit.org/changeset/179100>
Comment 51 WebKit Commit Bot 2015-01-25 18:11:26 PST
All reviewed patches have been landed.  Closing bug.
Comment 53 Darin Adler 2015-01-25 19:50:09 PST
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?
Comment 54 Chris Dumez 2015-01-25 20:02:47 PST
The patch landed through the CQ and all EWS was green. Not sure what happened, I am taking a look now.
Comment 55 Chris Dumez 2015-01-25 20:53:10 PST
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.
Comment 56 Alexey Proskuryakov 2015-01-25 22:41:32 PST
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.
Comment 57 Chris Dumez 2015-01-25 22:43:29 PST
(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.
Comment 58 Chris Dumez 2015-01-25 22:56:00 PST
Reopening to attach new patch.
Comment 59 Chris Dumez 2015-01-25 22:56:02 PST
Created attachment 245328 [details]
Patch
Comment 60 Chris Dumez 2015-01-25 23:10:32 PST
Comment on attachment 245328 [details]
Patch

Clearing flags on attachment: 245328

Committed r179105: <http://trac.webkit.org/changeset/179105>
Comment 61 Chris Dumez 2015-01-25 23:10:41 PST
All reviewed patches have been landed.  Closing bug.
Comment 62 Chris Dumez 2015-01-26 00:08:39 PST
Ok, looks like the bots are green again:
https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20%28Tests%29

Sorry about the delay.