[webkit-reviews] review granted: [Bug 57082] CSS calc parsing stage : [Attachment 124270] Patch

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has granted Mike Lawther
<mikelawther at chromium.org>'s request for review:
Bug 57082: CSS calc parsing stage
https://bugs.webkit.org/show_bug.cgi?id=57082

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

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=124270&action=review


r=me, with a few renaming suggestions. Not going to cq+ as a result of that.

> Source/WebCore/css/CSSCalculationValue.cpp:87
> +    static PassRefPtr<CSSCalcPrimitiveValue> create(CSSPrimitiveValue*
value, bool isInt)

isInteger would read better.

> Source/WebCore/css/CSSCalculationValue.cpp:99
> +    explicit CSSCalcPrimitiveValue(CSSPrimitiveValue* value, bool isInt)

Ditto.

> Source/WebCore/css/CSSCalculationValue.cpp:113
> +    {CalcNumber,	   CalcOther,	      CalcPercentNumber,
CalcPercentNumber, CalcOther},
> +    {CalcOther,	   CalcLength,	      CalcPercentLength, CalcOther,    
    CalcPercentLength},
> +    {CalcPercentNumber, CalcPercentLength, CalcPercent,	
CalcPercentNumber, CalcPercentLength},
> +    {CalcPercentNumber, CalcOther,	      CalcPercentNumber,
CalcPercentNumber, CalcOther},
> +    {CalcOther,	   CalcPercentLength, CalcPercentLength, CalcOther,    
    CalcPercentLength},

Minor style nit: I think you should have spaces before and after the { } on
each line, e.g.,:

{ CalcNumber,	     CalcOther, 	CalcPercentNumber, CalcPercentNumber,
CalcOther },

> Source/WebCore/css/CSSCalculationValue.h:64
> +    bool isInt() const { return m_isInt; }

I think isInteger would read better.

> Source/WebCore/css/CSSCalculationValue.h:75
> +    bool m_isInt;

Ditto.

> Source/WebCore/css/CSSParser.cpp:5213
> +    CSSParser* m_parent;

I don't like using the terminology "parent" here. Why not just m_parser?

> Source/WebCore/css/CSSParser.cpp:5602
> +    : m_parent(parent)

Same here. Would prefer m_parser.

> Source/WebCore/css/CSSParser.cpp:5720
> +    : m_parent(parent)

Same here.