[webkit-reviews] review denied: [Bug 134301] Implement parsing for CSS scroll snap points : [Attachment 235582] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has denied Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 134301: Implement parsing for CSS scroll snap points
https://bugs.webkit.org/show_bug.cgi?id=134301

Attachment 235582: Patch
https://bugs.webkit.org/attachment.cgi?id=235582&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=235582&action=review


Let's do one final round.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1088
> +static PassRef<CSSValueList> getScrollSnapDestination(RenderStyle* style,
Length x, Length y)

Drop the "get", so just scrollSnapDestination()

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1104
> +static PassRef<CSSValueList> getScrollSnapPoints(RenderStyle* style, const
Vector<Length>& points, Length repeatPoint, bool hasRepeat)

scrollSnapPoints()

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1114
> +static PassRef<CSSValueList> getScrollSnapCoordinates(RenderStyle* style,
const Vector<SnapCoordinate>& coordinates)

scrollSnapCoordinates()

> Source/WebCore/css/CSSParser.cpp:3186
> +bool CSSParser::parseScrollSnapPointsWithoutElements(CSSPropertyID propId,
bool important)

This name is odd. Once you add elements will that be handled here or in another
function?

> Source/WebCore/css/CSSParser.cpp:3205
> +	       if (validUnit(parserVal, FLength | FPercent)) {
> +		  
values->append(cssValuePool().createValue(Repeat::create(createPrimitiveNumeric
Value(parserVal))));
> +		   m_valueList->next();
> +		   if (m_valueList->current())
> +		       return false;
> +		   break;
> +	       }

Does this need to do anything to handle calc()?

> Source/WebCore/css/CSSParser.cpp:3221
> +    RefPtr<CSSValueList> position = CSSValueList::createSpaceSeparated();
> +    RefPtr<CSSValue> cssValueX, cssValueY;

Declare on first use.

> Source/WebCore/css/CSSPrimitiveValue.h:112
> +	   CSS_REPEAT = 34,

Please call this CSS_LENGTH_REPEAT

> Source/WebCore/css/CSSPrimitiveValue.h:192
> +    bool isRepeat() const { return m_primitiveUnitType == CSS_REPEAT; }

isLengthRepeat()

> Source/WebCore/css/CSSPrimitiveValue.h:318
> +    Repeat* getRepeatValue(ExceptionCode&) const;
> +    Repeat* getRepeatValue() const { return m_primitiveUnitType !=
CSS_REPEAT ? 0 : m_value.repeat; }

getLengthRepeat etc.

> Source/WebCore/css/Repeat.h:2
> +/*
> + * Copyright (C) 2014 Apple Inc. All rights reserved.

Please rename the file to LengthRepeat.h

> Source/WebCore/css/Repeat.h:18
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Library General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Library General Public License for more details.
> + *
> + * You should have received a copy of the GNU Library General Public License

> + * along with this library; see the file COPYING.LIB.  If not, write to
> + * the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
> + * Boston, MA 02110-1301, USA.
> + */

Wrong license. This should have the 2-clause Apple license (e.g. from
ScrollingTreeIOS.h).

> Source/WebCore/css/Repeat.h:31
> +class Repeat : public RefCounted<Repeat> {

LengthRepeat

> Source/WebCore/css/Repeat.h:32
> +

No blank line here.

> Source/WebCore/css/Repeat.h:34
> +

No blank line here.

> Source/WebCore/css/Repeat.h:54
> +

Remove.

> Source/WebCore/css/Repeat.h:60
> +    RefPtr<CSSPrimitiveValue> m_interval;

Isn't this just a Length value?

> Source/WebCore/css/Repeat.h:61
> +

Remove

> Source/WebCore/css/StyleResolver.cpp:2913
> +	   Vector<SnapCoordinate> coordinates;

You could reserveCapacity() on this vector since you know the final size.

> Source/WebCore/rendering/style/StyleScrollSnapPoints.cpp:18
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
met:
> + * 
> + * 1. Redistributions of source code must retain the above copyright notice,
this
> + * list of conditions and the following disclaimer.
> + * 
> + * 2. Redistributions in binary form must reproduce the above copyright
notice,
> + * this list of conditions and the following disclaimer in the documentation
and/
> + * or other materials provided with the distribution.
> + * 
> + * 3. Neither the name of the copyright holder nor the names of its
contributors
> + * may be used to endorse or promote products derived from this software
without
> + * specific prior written permission.
> + * 

Wrong license. This should have the 2-clause Apple license (e.g. from
ScrollingTreeIOS.h).

> Source/WebCore/rendering/style/StyleScrollSnapPoints.h:18
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
met:
> + * 
> + * 1. Redistributions of source code must retain the above copyright notice,
this
> + * list of conditions and the following disclaimer.
> + * 
> + * 2. Redistributions in binary form must reproduce the above copyright
notice,
> + * this list of conditions and the following disclaimer in the documentation
and/
> + * or other materials provided with the distribution.
> + * 
> + * 3. Neither the name of the copyright holder nor the names of its
contributors
> + * may be used to endorse or promote products derived from this software
without
> + * specific prior written permission.
> + * 

Ditto.