[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


Simon Fraser (smfr) <simon.fraser at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #235582|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #48 from Simon Fraser (smfr) <simon.fraser at apple.com>  2014-08-01 16:53:43 PST ---
(From update of attachment 235582)
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(createPrimitiveNumericValue(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.

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