[Webkit-unassigned] [Bug 134301] Implement parsing for CSS scroll snap points

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=134301





--- Comment #29 from Wenson Hsieh <wenson_hsieh at apple.com>  2014-06-30 11:11:59 PST ---
(From update of attachment 234048)
View in context: https://bugs.webkit.org/attachment.cgi?id=234048&action=review

Thanks for the review! I fixed most of the smaller issues, and I'll put up a new version of the patch as soon as I add support for the different types of <length>s.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1105
>> +        repeatStrBuilder.append("repeat(", 7);
> 
> This should use StringBuilder::appendLiteral(...)

Got it -- fixed!

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1106
>> +        repeatStrBuilder.append(String::numberToStringFixedWidth((double) repeatPoint.value, 0));
> 
> If the cast to double is necessary, you should use a static_cast.

Removed double cast, good catch.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1108
>> +            repeatStrBuilder.append("%%", 1);
> 
> This should use StringBuilder::appendLiteral(...)

Fixed.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1110
>> +            repeatStrBuilder.append("px", 2);
> 
> This should use StringBuilder::appendLiteral(...)

Fixed.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1111
>> +        repeatStrBuilder.append(")", 1);
> 
> This should use StringBuilder::appendLiteral(...)

Fixed.

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1118
>> +static PassRef<CSSValueList> getScrollSnapCoordinates(const Vector<SnapPoint>& coords)
> 
> Can we pass this in / store these as a Vector<std::pair<SnapPoint, SnapPoint>>.

Good point -- this makes a lot more sense than storing them in a big list. Fixed.

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2320
>> +        UNUSED_PARAM(styleResolver);
> 
> There is no reason to use UNUSED_PARAM() here. Just don't declare the parameter name in the signature.

Fixed.

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2337
>> +            renderStyle->setScrollSnapUsesElementsX(false);
> 
> You should assert that the CSSValue is a CSSValueList here.

Added ASSERT check.

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2339
>> +            Vector<SnapPoint> points = Vector<SnapPoint>();
> 
> This can be written as Vector<SnapPoint> points;

Fixed -- thanks!

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2341
>> +            for (size_t i = 0; i < valueList.length(); i++) {
> 
> This should probably use a CSSValueListIterator

Fixed -- thanks!

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2370
>> +        UNUSED_PARAM(styleResolver);
> 
> There is no reason to use UNUSED_PARAM() here. Just don't declare the parameter name in the signature.

Fixed -- thanks!

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2420
>> +        UNUSED_PARAM(styleResolver);
> 
> There is no reason to use UNUSED_PARAM() here. Just don't declare the parameter name in the signature.

Fixed -- thanks!

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2452
>> +        UNUSED_PARAM(styleResolver);
> 
> There is no reason to use UNUSED_PARAM() here. Just don't declare the parameter name in the signature.

Fixed -- thanks!

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2457
>> +        UNUSED_PARAM(styleResolver);
> 
> There is no reason to use UNUSED_PARAM() here. Just don't declare the parameter name in the signature.

Fixed -- thanks!

>> Source/WebCore/css/DeprecatedStyleBuilder.cpp:2467
>> +            return;
> 
> This seems like an overly complicated way to say % 2.

Good catch -- changed to % 2.

>> Source/WebCore/rendering/style/RenderStyle.h:1080
>> +    Vector<SnapPoint> scrollSnapPointsY() const { return rareNonInheritedData->m_scrollSnapPoints->pointsY; }
> 
> Do we really want to copy the Vector here?  If not, this should return a const Vector<SnapPoint>&.

Fixed -- thanks!

>> Source/WebCore/rendering/style/RenderStyle.h:1087
>> +    Vector<SnapPoint> scrollSnapCoordinates() const { return rareNonInheritedData->m_scrollSnapPoints->coords; }
> 
> Do we really want to copy the Vector here?  If not, this should return a const Vector<SnapPoint>&.

Fixed -- thanks!

>> Source/WebCore/rendering/style/RenderStyleConstants.h:557
>> +enum EScrollSnapType { ScrollSnapTypeNone = 0, ScrollSnapTypeProximity = 1, ScrollSnapTypeMandatory = 2};
> 
> The E prefix is unnecessary and this should be an enum class.

Changed to use enum class

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:19
>> + */
> 
> Please use the BSD license.

Fixed with BSD license from http://opensource.org/licenses/BSD-3-Clause (hopefully this is the right one?)

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:26
>> +#include <wtf/text/WTFString.h>
> 
> This is already included in the header.

Removed -- thanks!

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:41
>> +    , coords(Vector<SnapPoint>())
> 
> These don't require explicit initialization.

Fixed -- thanks!

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:19
>> + */
> 
> Please use the BSD license.

Fixed with BSD license from http://opensource.org/licenses/BSD-3-Clause (hopefully this is the right one?)

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:36
>> +};
> 
> This should be an enum class.

Fixed -- thanks!

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:38
>> +struct SnapPoint {
> 
> I'm not too familiar with the terminology, but we usually think of a point as something with at least an X and Y.  Is this something the spec names?  If not, perhaps we can rename this to something more suitable (SnapValue? SnapLength?).

The spec doesn't explicitly say that a snap point is defined by both an x and a y, although in CSS having snapping behavior in one direction only requires scroll-snap-points-x or scroll-snap-points-y -- I suppose the other point is defined implicitly as repeat(100%). I agree, though; in either case, SnapLength is probably a better name.

>> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:44
>> +    SnapPoint(SnapPointConstant snapConstant)
> 
> This should be an explicit constructor.

Fixed.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.