Summary: | Remove 'font' shorthand property special casing | ||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||||||||||||||||||||||||||||||
Component: | CSS | Assignee: | Chris Dumez <cdumez> | ||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | ap, buildbot, commit-queue, darin, kling, koivisto, rniwa | ||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | BlinkMergeCandidate | ||||||||||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||
URL: | https://src.chromium.org/viewvc/blink?view=rev&revision=184449 | ||||||||||||||||||||||||||||||||||||||||||||
Bug Depends on: | 140801, 140810, 140829, 140840, 140848, 140891 | ||||||||||||||||||||||||||||||||||||||||||||
Bug Blocks: | 140817 | ||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2015-01-16 16:54:53 PST
https://src.chromium.org/viewvc/blink?view=rev&revision=184629 seems related as well. Created attachment 245017 [details]
WIP Patch
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 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 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 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 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
Created attachment 245173 [details]
WIP Patch
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.
Created attachment 245182 [details]
WIP patch
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 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 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
Created attachment 245201 [details]
Patch
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 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 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 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 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
Created attachment 245209 [details]
Patch
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.
Created attachment 245212 [details]
Patch
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.
Created attachment 245254 [details]
Patch
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.
Created attachment 245273 [details]
Patch
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.
Created attachment 245274 [details]
Patch
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. 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.
Created attachment 245285 [details]
Patch
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 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 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. Created attachment 245290 [details]
Patch
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. 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 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 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. Created attachment 245314 [details]
Patch
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 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 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 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. Created attachment 245315 [details]
Patch
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 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. Created attachment 245316 [details]
Patch
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 on attachment 245316 [details] Patch Clearing flags on attachment: 245316 Committed r179100: <http://trac.webkit.org/changeset/179100> All reviewed patches have been landed. Closing bug. This broke many tests: https://build.webkit.org/results/Apple%20Yosemite%20Release%20WK1%20(Tests)/r179100%20(2282)/results.html 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? The patch landed through the CQ and all EWS was green. Not sure what happened, I am taking a look now. 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. 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. (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. Reopening to attach new patch. Created attachment 245328 [details]
Patch
Comment on attachment 245328 [details] Patch Clearing flags on attachment: 245328 Committed r179105: <http://trac.webkit.org/changeset/179105> All reviewed patches have been landed. Closing bug. Ok, looks like the bots are green again: https://build.webkit.org/builders/Apple%20Yosemite%20Release%20WK2%20%28Tests%29 Sorry about the delay. 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%) |