[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 #11 from Joonghun Park <jh718.park at samsung.com> ---
(In reply to comment #9)
> Comment on attachment 249815 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=249815&action=review
> 
> Where is the test case that covers “implicit” vs. “explicit”?
> 
> > Source/WebCore/css/CSSParser.cpp:3641
> > +            return false; // Too many vlaues for this shorthand. Invalid declaration.
> 
> Typo "vlaues"
> 
> > Source/WebCore/css/CSSParser.cpp:3665
> > +        // Time to assign the previously skipped 'auto' values to a property.
> > +        // If both properties are unassigned at this point (i.e. 'columns:auto'),
> > +        // it doesn't matter that much which one we set.
> > +        // The one we don't set here will get an implicit 'auto' value further down.
> 
> If the implicit vs. explicit distinction matters, I don’t understand why it
> doesn’t matter in this case and an implicit auto is sufficient. To put it
> another way, if the implicit vs. explicit distinction did not matter, then
> we wouldn’t need hasPendingExplicitAuto at all!
> 
> > Source/WebCore/css/CSSParser.cpp:3680
> > +        addProperty(CSSPropertyColumnWidth, cssValuePool().createIdentifierValue(CSSValueAuto), important, true /* implicit */);
> 
> I think this code should just pass !hasPendingExplicitAuto instead of
> passing "true" for implicit. Then we could get rid of the entire if
> statement above.
> 
> > Source/WebCore/css/CSSParser.cpp:3685
> > +        addProperty(CSSPropertyColumnCount, cssValuePool().createIdentifierValue(CSSValueAuto), important, true /* implicit */);
> 
> Ditto.
> 
> > Source/WebCore/css/CSSParser.h:196
> > +    PassRefPtr<CSSValue> parseColumnWidth();
> > +    PassRefPtr<CSSValue> parseColumnCount();
> 
> For new functions, return type should just be RefPtr. There is no reason to
> use PassRefPtr for return values any more.

I applied your valuable comments in a new patch, so would you please review this patch again?

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