Bug 187182 – RectEdges constructor: Use correct template parameter in std::forward<>

• Component: Layout and Rendering
• Status: NEW

[Alicia Boya García]

The RectEdges constructor is defined with template<typename U>, but then U is unused while std::forward is receiving the template parameter of the class (which will not be a reference type).

If my knowledge of this templates is not wrong (it may well be), this is causing a copy even when a move would be acceptable.

[Alicia Boya García]

Created attachment 343920 [details]
Patch

[EWS Watchlist]

Comment on attachment 343920 [details]
Patch

Attachment 343920 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/8385318

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

[EWS Watchlist]

Created attachment 343928 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Michael Catanzaro]

I think your change is right, but notice:

/Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/WebCore.framework/PrivateHeaders/RectEdges.h:39:45: error: non-constant-expression cannot be narrowed from type 'double' to 'std::__1::array<float, 4>::value_type' (aka 'float') in initializer list [-Wc++11-narrowing]
        : m_sides({ { std::forward<U>(top), std::forward<U>(right), std::forward<U>(bottom), std::forward<U>(left) } })
                                            ^~~~~~~~~~~~~~~~~~~~~~
/Volumes/Data/EWS/WebKit/WebKitBuild/Release-iphonesimulator/WebCore.framework/PrivateHeaders/RectEdges.h:39:45: note: insert an explicit cast to silence this issue
        : m_sides({ { std::forward<U>(top), std::forward<U>(right), std::forward<U>(bottom), std::forward<U>(left) } })
                                            ^~~~~~~~~~~~~~~~~~~~~~
                                            static_cast<value_type>( )

The initializer list constructor prevents narrowing conversions. Perhaps that's a clue that this is not an appropriate place to use std::forward at all?

[Darin Adler]

The std::forward is valuable for Length, and not valuable for float or double. The problem is arising when it’s float or double. Might require some thinking about what to do.

I think we would like to preserve the optimization of not copying a Length, because that has branches and reference count churn.

The real reason we are getting the warning is that the old version had argument-passing-style conversion but in the new version we only have the initializer list, which is stricter about types. It’s possible we can quiet the warning by using parentheses "()" instead of braces "{}". Or we can specifically address the "narrow double to float" case with an explicit trick like an overloaded function.

[Darin Adler]

Or we can change the call sites to use the correct types, explicitly narrowing to float before constructing a FloatBoxExtent. I don’t see a reason we have to support implicit narrowing from double to float. It’s even possible that there is only one call site relying on this, the one in WebView.mm.