[Webkit-unassigned] [Bug 143248] [CSS MultiColumn] Parse "columns: auto <length>" shorthand property value properly

[bugzilla-daemon at webkit.org]

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

--- Comment #18 from Joonghun Park <jh718.park at samsung.com> ---
(In reply to comment #17)
> Comment on attachment 249890 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249890&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:3618
> > +RefPtr<CSSValue> CSSParser::parseColumnWidth()
> > +{
> > +    ValueWithCalculation valueWithCalculation(*m_valueList->current());
> > +    CSSValueID id = valueWithCalculation.value().id;
> > +    // Always parse this property in strict mode, since it would be ambiguous otherwise when used in the 'columns' shorthand property.
> > +    if (id == CSSValueAuto || (validateUnit(valueWithCalculation, FLength | FNonNeg, CSSStrictMode) && parsedDouble(valueWithCalculation))) {
> > +        RefPtr<CSSValue> parsedValue = parseValidPrimitive(id, valueWithCalculation);
> > +        m_valueList->next();
> > +        return parsedValue;
> > +    }
> > +
> > +    return nullptr;
> > +}
> 
> I think these functions would read better with early return (check for error
> and return nullptr) rather than nesting the normal code path inside an if.
> Yes, I'd apply it.
> > Source/WebCore/css/CSSParser.cpp:3661
> > +    ASSERT(columnCount || coulmnWidth);
> 
> This will fail to compile because of the typo: coulmnWidth.
> After applying your feedback, I found this assert statment is not safisfied when we are in "columns: auto" case. So I removed it.
> > LayoutTests/fast/css/getPropertyValue-columns.html:38
> > +      shouldBeEqualToString('columnsValue("columns4")', 'auto auto');
> 
> Is this serialization of "auto" as "auto auto" correct behavior, or a bug?
Hmm, it seems that spec doesn't say anything about it. In my opinion, 'auto auto' looks more consistent, but I choose to maintain current state.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150402/fbc00cdf/attachment.html>