Bug 166809 – Remove PassRefPtr from more of "platform"

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Darin Adler]

Remove PassRefPtr from more of "platform"

[Darin Adler]

Created attachment 298285 [details]
Patch

[WebKit Commit Bot]

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.

[Darin Adler]

Created attachment 298287 [details]
Patch

[WebKit Commit Bot]

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.

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Darin Adler]

Created attachment 298300 [details]
Patch

[WebKit Commit Bot]

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.

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Darin Adler]

Created attachment 298337 [details]
Patch

[Darin Adler]

Created attachment 298447 [details]
Patch

[Darin Adler]

Created attachment 298547 [details]
Patch

[Darin Adler]

Created attachment 298551 [details]
Patch

[Darin Adler]

Created attachment 298554 [details]
Patch

[Alex Christensen]

930KB!?!?

[Alex Christensen]

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.

[Darin Adler]

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.

[Darin Adler]

Created attachment 298558 [details]
Patch

[Build Bot]

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.

[Build Bot]

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

[Darin Adler]

Hooray, looks like it’s building on all platforms now. Tonight I have to figure out why those tests failed.

[Darin Adler]

Test failures don’t look like they are caused by the patch; look like EWS malfunction.

[Alex Christensen]

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?

[Darin Adler]

Created attachment 298747 [details]
Patch

[Sam Weinig]

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.

[Darin Adler]

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.

[Darin Adler]

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.

[Darin Adler]

(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.

[Darin Adler]

Committed r210758: <http://trac.webkit.org/changeset/210758>

[Antti Koivisto]

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.