|
Description
Darin Adler
2017-01-07 09:27:10 PST
Created attachment 298285 [details]
Patch
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.
Created attachment 298287 [details]
Patch
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.
Comment on attachment 298287 [details] Patch Attachment 298287 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2851138 Number of test failures exceeded the failure limit. 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
Comment on attachment 298287 [details] Patch Attachment 298287 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2851147 Number of test failures exceeded the failure limit. 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
Comment on attachment 298287 [details] Patch Attachment 298287 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2851139 Number of test failures exceeded the failure limit. 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
Comment on attachment 298287 [details] Patch Attachment 298287 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2851160 Number of test failures exceeded the failure limit. 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
Created attachment 298300 [details]
Patch
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.
Comment on attachment 298300 [details] Patch Attachment 298300 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/2853037 Number of test failures exceeded the failure limit. 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
Comment on attachment 298300 [details] Patch Attachment 298300 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2853036 Number of test failures exceeded the failure limit. 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
Comment on attachment 298300 [details] Patch Attachment 298300 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/2853027 Number of test failures exceeded the failure limit. 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
Comment on attachment 298300 [details] Patch Attachment 298300 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/2853047 Number of test failures exceeded the failure limit. 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
Created attachment 298337 [details]
Patch
Created attachment 298447 [details]
Patch
Created attachment 298547 [details]
Patch
Created attachment 298551 [details]
Patch
Created attachment 298554 [details]
Patch
930KB!?!? 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 298558 [details]
Patch
Comment on attachment 298558 [details] Patch Attachment 298558 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/2868890 Number of test failures exceeded the failure limit. 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
Hooray, looks like it’s building on all platforms now. Tonight I have to figure out why those tests failed. Test failures don’t look like they are caused by the patch; look like EWS malfunction. 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? Created attachment 298747 [details]
Patch
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. Committed r210758: <http://trac.webkit.org/changeset/210758> 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. |