RESOLVED FIXED 140577
Remove 'font' shorthand property special casing
https://bugs.webkit.org/show_bug.cgi?id=140577
Summary Remove 'font' shorthand property special casing
Chris Dumez
Reported 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.
Attachments
WIP Patch (20.31 KB, patch)
2015-01-20 14:18 PST, Chris Dumez
buildbot: commit-queue-
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
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
WIP Patch (34.44 KB, patch)
2015-01-22 15:25 PST, Chris Dumez
no flags
WIP patch (40.89 KB, patch)
2015-01-22 16:30 PST, Chris Dumez
no flags
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
Patch (74.07 KB, patch)
2015-01-22 20:41 PST, Chris Dumez
no flags
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
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
Patch (75.50 KB, patch)
2015-01-22 22:10 PST, Chris Dumez
no flags
Patch (76.01 KB, patch)
2015-01-22 23:09 PST, Chris Dumez
no flags
Patch (78.80 KB, patch)
2015-01-23 15:32 PST, Chris Dumez
no flags
Patch (41.40 KB, patch)
2015-01-23 20:17 PST, Chris Dumez
no flags
Patch (46.06 KB, patch)
2015-01-23 21:10 PST, Chris Dumez
no flags
Patch (46.08 KB, patch)
2015-01-24 09:50 PST, Chris Dumez
no flags
Patch (46.57 KB, patch)
2015-01-24 19:27 PST, Chris Dumez
no flags
Patch (48.48 KB, patch)
2015-01-25 14:46 PST, Chris Dumez
no flags
Patch (48.21 KB, patch)
2015-01-25 16:38 PST, Chris Dumez
no flags
Patch (48.32 KB, patch)
2015-01-25 16:57 PST, Chris Dumez
no flags
Patch (1.44 KB, patch)
2015-01-25 22:56 PST, Chris Dumez
no flags
Chris Dumez
Comment 1 2015-01-16 17:09:47 PST
Chris Dumez
Comment 2 2015-01-20 14:18:38 PST
Created attachment 245017 [details] WIP Patch
WebKit Commit Bot
Comment 3 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.
Build Bot
Comment 4 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
Build Bot
Comment 5 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
Build Bot
Comment 6 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
Build Bot
Comment 7 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
Chris Dumez
Comment 8 2015-01-22 15:25:32 PST
Created attachment 245173 [details] WIP Patch
WebKit Commit Bot
Comment 9 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.
Chris Dumez
Comment 10 2015-01-22 16:30:26 PST
Created attachment 245182 [details] WIP patch
WebKit Commit Bot
Comment 11 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.
Build Bot
Comment 12 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
Build Bot
Comment 13 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
Chris Dumez
Comment 14 2015-01-22 20:41:43 PST
WebKit Commit Bot
Comment 15 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.
Build Bot
Comment 16 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
Build Bot
Comment 17 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
Build Bot
Comment 18 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
Build Bot
Comment 19 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
Chris Dumez
Comment 20 2015-01-22 22:10:25 PST
WebKit Commit Bot
Comment 21 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.
Chris Dumez
Comment 22 2015-01-22 23:09:02 PST
WebKit Commit Bot
Comment 23 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.
Chris Dumez
Comment 24 2015-01-23 15:32:04 PST
WebKit Commit Bot
Comment 25 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.
Chris Dumez
Comment 26 2015-01-23 20:17:54 PST
WebKit Commit Bot
Comment 27 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.
Chris Dumez
Comment 28 2015-01-23 21:10:53 PST
Chris Dumez
Comment 29 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.
WebKit Commit Bot
Comment 30 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.
Chris Dumez
Comment 31 2015-01-24 09:50:04 PST
WebKit Commit Bot
Comment 32 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.
Darin Adler
Comment 33 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.
Chris Dumez
Comment 34 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.
Chris Dumez
Comment 35 2015-01-24 19:27:56 PST
Chris Dumez
Comment 36 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.
WebKit Commit Bot
Comment 37 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.
WebKit Commit Bot
Comment 38 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
Darin Adler
Comment 39 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.
Chris Dumez
Comment 40 2015-01-25 14:46:54 PST
WebKit Commit Bot
Comment 41 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.
Chris Dumez
Comment 42 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.
Darin Adler
Comment 43 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.
Darin Adler
Comment 44 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.
Chris Dumez
Comment 45 2015-01-25 16:38:39 PST
WebKit Commit Bot
Comment 46 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.
Chris Dumez
Comment 47 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.
Chris Dumez
Comment 48 2015-01-25 16:57:52 PST
WebKit Commit Bot
Comment 49 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.
WebKit Commit Bot
Comment 50 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>
WebKit Commit Bot
Comment 51 2015-01-25 18:11:26 PST
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 53 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?
Chris Dumez
Comment 54 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.
Chris Dumez
Comment 55 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.
Alexey Proskuryakov
Comment 56 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.
Chris Dumez
Comment 57 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.
Chris Dumez
Comment 58 2015-01-25 22:56:00 PST
Reopening to attach new patch.
Chris Dumez
Comment 59 2015-01-25 22:56:02 PST
Chris Dumez
Comment 60 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>
Chris Dumez
Comment 61 2015-01-25 23:10:41 PST
All reviewed patches have been landed. Closing bug.
Chris Dumez
Comment 62 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.
Note You need to log in before you can comment on or make changes to this bug.