Attachment 298285[details] did not pass style-queue:
ERROR: Source/WebCore/platform/animation/Animation.h:73: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5]
ERROR: Source/WebCore/rendering/style/DataRef.h:33: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4]
Total errors found: 2 in 86 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Attachment 298287[details] did not pass style-queue:
ERROR: Source/WebCore/platform/animation/Animation.h:73: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5]
ERROR: Source/WebCore/rendering/style/DataRef.h:33: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4]
Total errors found: 2 in 87 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 298291[details]
Archive of layout-test-results from ews113 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 298292[details]
Archive of layout-test-results from ews101 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 298293[details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 298295[details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Attachment 298300[details] did not pass style-queue:
ERROR: Source/WebCore/platform/animation/Animation.h:73: Line contains only semicolon. If this should be an empty statement, use { } instead. [whitespace/semicolon] [5]
ERROR: Source/WebCore/rendering/style/DataRef.h:33: Should be indented on a separate line, with the colon or comma first on that line. [whitespace/indent] [4]
Total errors found: 2 in 87 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 298301[details]
Archive of layout-test-results from ews100 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 298302[details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 298303[details]
Archive of layout-test-results from ews113 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 298304[details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 298554[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=298554&action=review> Source/WTF/ChangeLog:9
> + * wtf/Ref.h: Changed the template so that a const Ref<T> does not prohibit non-const
> + use of T. We can still use const Ref<const T> to express that. The earlier design
Might I suggest doing just this in one patch and removing the const_casts? Then the rest of this patch would go through EWS much faster.
I can break up the patch in that way, and many other ways too.
But that approach won’t get me through EWS faster, unless it’s actually landed. I can’t upload the rest of the patch until after it is.
Created attachment 298566[details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 298558[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=298558&action=review
partial review. This patch is huge.
> Source/WebCore/ChangeLog:573
> + * rendering/style/StyleBackgroundData.cpp: Update data member names.
> + * rendering/style/StyleBackgroundData.h: Ditto; make them public and remove the
> + getter functions.
Don't we usually make such classes into structs?
> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:-2899
> - // NOTE: Gecko always returns this as a comma-separated CSSPrimitiveValue string.
Would it be useful to keep this comment around? Is it a problem that we behave differently than Gecko? Is there a spec?
> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3608
> + if (auto* animList = style->transitions()) {
If we're dropping the type in favor of auto, we should probably change the variable name to animationList.
> Source/WebCore/css/StyleBuilderCustom.h:64
> +inline Length forwardInheritedValue(const Length& value) { auto copy = value; return copy; }
> +inline LengthSize forwardInheritedValue(const LengthSize& value) { auto copy = value; return copy; }
> +inline LengthBox forwardInheritedValue(const LengthBox& value) { auto copy = value; return copy; }
Are we sure we only want to copy these three types? Could we make these a template, too?
Comment on attachment 298747[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=298747&action=review
Very nice. My biggest takeaway from reading this is that our list-like classes need iterators so they can participate in modern-for-loops more cleanly.
> Source/WebCore/page/PrintContext.cpp:311
> + return String::number(style->pageSize().width.value()) + ' ' + String::number(style->pageSize().height.value());
Would be nice to have a makeString overload that that allowed passing numbers if we don't have one already.
Comment on attachment 298558[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=298558&action=review>> Source/WebCore/ChangeLog:573
>> + getter functions.
>
> Don't we usually make such classes into structs?
Sure; I could do that with all of these. The downside is that all of these style data classes have some private members as well as the public members, so they feel like a bit of a hybrid. But I think they should be structs, eventually if not immediately.
>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:-2899
>> - // NOTE: Gecko always returns this as a comma-separated CSSPrimitiveValue string.
>
> Would it be useful to keep this comment around? Is it a problem that we behave differently than Gecko? Is there a spec?
No, it would not. There is a specification, CSS 2.1, that says this should be "as specified" and I am pretty sure that this is both tested and behaves as it should. This comment is from a very old period in this project; it’s likely that Firefox no longer behaves this way, in fact, but not really worth researching.
>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3608
>> + if (auto* animList = style->transitions()) {
>
> If we're dropping the type in favor of auto, we should probably change the variable name to animationList.
Good point, I agree that would be better.
>> Source/WebCore/css/StyleBuilderCustom.h:64
>> +inline LengthBox forwardInheritedValue(const LengthBox& value) { auto copy = value; return copy; }
>
> Are we sure we only want to copy these three types? Could we make these a template, too?
Yes, I am sure about this; we only want to copy types where it’s necessary to make a copy to compile. Moving is OK for all other cases.
No, there is no simple way to make these three a template.
Comment on attachment 298747[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=298747&action=review>> Source/WebCore/page/PrintContext.cpp:311
>> + return String::number(style->pageSize().width.value()) + ' ' + String::number(style->pageSize().height.value());
>
> Would be nice to have a makeString overload that that allowed passing numbers if we don't have one already.
I agree. Would even be pretty simple. We don’t have one yet. I added this for StringBuilder a while back, but not for makeString.
(In reply to comment #38)
> My biggest takeaway from reading this is that our list-like
> classes need iterators so they can participate in modern-for-loops more
> cleanly.
I definitely noticed that, too. Another fix for this in many cases is to just use Vector directly instead of defining a class.
Comment on attachment 298747[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=298747&action=review> Source/WTF/ChangeLog:13
> + * wtf/Ref.h: Changed the template so that a const Ref<T> does not prohibit non-const
> + use of T. We can still use const Ref<const T> to express that. The earlier design
> + was intentional, but was not consistent with either actual references or with
> + other smart pointer classes like RefPtr. One way to see how much better this is,
> + is to see all the many, many cases where we have const_cast just to work around
> + this. I searched for those and included fixes for many in this patch.
Still not a fan of stealth const_casting over doing it explicitly but I suppose consistency with std::unique_ptr is a decent argument for it.
2017-01-07 14:07 PST, Darin Adler
2017-01-07 15:38 PST, Darin Adler
2017-01-07 16:44 PST, Build Bot
2017-01-07 16:45 PST, Build Bot
2017-01-07 16:45 PST, Build Bot
2017-01-07 17:00 PST, Build Bot
2017-01-08 01:48 PST, Darin Adler
2017-01-08 02:55 PST, Build Bot
2017-01-08 02:56 PST, Build Bot
2017-01-08 03:00 PST, Build Bot
2017-01-08 03:14 PST, Build Bot
2017-01-09 02:39 PST, Darin Adler
2017-01-09 23:46 PST, Darin Adler
2017-01-10 19:29 PST, Darin Adler
2017-01-10 20:46 PST, Darin Adler
2017-01-10 22:05 PST, Darin Adler
2017-01-11 00:06 PST, Darin Adler
2017-01-11 03:10 PST, Build Bot
2017-01-12 18:53 PST, Darin Adler