Bug 82040

Summary: CSSParser doesn't set border-*-width/style/color to initial by border shorthand property
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: CSSAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: dbates, dglazkov, kling, koivisto, macpherson, menard, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 81737    
Attachments:
Description Flags
Fixes the bug
none
Rebaseliined the failling test
none
Fixed typos in the change log koivisto: review+

Description Ryosuke Niwa 2012-03-23 03:13:34 PDT
​<div id="test" style="border:solid;">hello</div>​​​​​​​​​​​​​​​​​​​​​​​​​​​​​​
alert(​document.getElementById('test').style.borderTopColor);

should alert "initial".
​
Comment 1 Ryosuke Niwa 2012-03-23 04:14:22 PDT
Created attachment 133457 [details]
Fixes the bug
Comment 2 WebKit Review Bot 2012-03-23 04:45:36 PDT
Comment on attachment 133457 [details]
Fixes the bug

Attachment 133457 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12120603

New failing tests:
inspector/styles/styles-new-API.html
Comment 3 Andy Estes 2012-03-23 11:20:27 PDT
Comment on attachment 133457 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=133457&action=review

I just have a few drive-by comments about ChangeLog typos.

> Source/WebCore/ChangeLog:9
> +        While CSSParser::parseValue can process these shorthand properties property and set the longhand properties

"shorthand properties *properly*"

> Source/WebCore/ChangeLog:20
> +        This allows us to initialize mutliple properties (e.g. border-*-color) for a signle property missing in the set.

s/mutliple/multiple

> Source/WebCore/ChangeLog:26
> +        (CSSPropertyLonghand): Added the version that takes longhands instances for initialization purposes.

longhand instances, not longhands instances.
Comment 4 Ryosuke Niwa 2012-03-23 11:28:49 PDT
Created attachment 133519 [details]
Rebaseliined the failling test
Comment 5 Simon Fraser (smfr) 2012-03-23 11:32:28 PDT
Comment on attachment 133519 [details]
Rebaseliined the failling test

View in context: https://bugs.webkit.org/attachment.cgi?id=133519&action=review

> Source/WebCore/ChangeLog:9
> +        While CSSParser::parseValue can process these shorthand properties property and set the longhand properties

properties property? Did you mean "properties properly"?
Comment 6 Ryosuke Niwa 2012-03-23 11:35:27 PDT
Comment on attachment 133519 [details]
Rebaseliined the failling test

View in context: https://bugs.webkit.org/attachment.cgi?id=133519&action=review

>> Source/WebCore/ChangeLog:9
>> +        While CSSParser::parseValue can process these shorthand properties property and set the longhand properties
> 
> properties property? Did you mean "properties properly"?

Oops, yes. properly.
Comment 7 Ryosuke Niwa 2012-03-23 11:50:23 PDT
Created attachment 133525 [details]
Fixed typos in the change log
Comment 8 Simon Fraser (smfr) 2012-03-23 12:00:48 PDT
Comment on attachment 133525 [details]
Fixed typos in the change log

View in context: https://bugs.webkit.org/attachment.cgi?id=133525&action=review

> Source/WebCore/ChangeLog:11
> +        The border shorthand property sets values for border-width, border-style, and border-color shorthand properties.
> +        While CSSParser::parseValue can process these shorthand properties properly and set the longhand properties
> +        such as border-top-width, border-right-width, ... border-left-color, CSSParser::addProperty can't and the
> +        initialization in parseShorthand fails for the border property.

Please state why you think this won't break the web.

> Source/WebCore/css/CSSParser.cpp:2805
> +    bool propertyFound[6]= { false, false, false, false, false, false }; // 6 is enough size.

The comment is terribly worded.

> Source/WebCore/css/CSSParser.cpp:2811
> +                    propertyFound[propIndex] = found = true;

We shy away from multiple assignments like this in general. Please split onto 2 lines.

> Source/WebCore/css/CSSParser.cpp:2827
> +    const CSSPropertyLonghand* const* const longhandsForInitialization = longhand.longhandsForInitialization();

That's a lot of consts.

> Source/WebCore/css/CSSParser.cpp:2833
> +            const CSSPropertyLonghand& initLonghand = *(longhandsForInitialization[i]);

I don't really see the benefit of using a reference rather than a pointer here.

> Source/WebCore/css/CSSPropertyLonghand.h:35
> +    CSSPropertyLonghand(const int* properties, unsigned numProperties)

I wish we used CSSPropertyID and not int.

> Source/WebCore/css/CSSPropertyLonghand.h:42
> +    CSSPropertyLonghand(const int* properties, const CSSPropertyLonghand** longhandsForInitialization, unsigned numProperties)

Would const CSSPropertyLonghand*[] be clearer?
Comment 9 Daniel Bates 2012-03-23 12:03:39 PDT
Comment on attachment 133525 [details]
Fixed typos in the change log

View in context: https://bugs.webkit.org/attachment.cgi?id=133525&action=review

The majority of my remarks were already raised by Simon Fraser. I had some very minor nits.

> Source/WebCore/ChangeLog:9
> +        While CSSParser::parseValue can process these shorthand properties properly and set the longhand properties

Nit: CSSParser::parseValue => CSSParser::parseValue()

Since you are referring to the function here as opposed to the name of the function.

> Source/WebCore/ChangeLog:10
> +        such as border-top-width, border-right-width, ... border-left-color, CSSParser::addProperty can't and the

Similarly, "CSSParser::addProperty" should be "CSSParser::addProperty()".

>> Source/WebCore/ChangeLog:11
>> +        initialization in parseShorthand fails for the border property.
> 
> Please state why you think this won't break the web.

Similarly, "parseShorthand" should be "parseShorthand()".

> Source/WebCore/ChangeLog:20
> +        This allows us to initialize mutliple properties (e.g. border-*-color) for a single property missing in the set.

Nit: mutliple => multiple
Comment 10 Ryosuke Niwa 2012-03-23 12:23:38 PDT
(In reply to comment #9)
> (From update of attachment 133525 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=133525&action=review
> 
> The majority of my remarks were already raised by Simon Fraser. I had some very minor nits.
> 
> > Source/WebCore/ChangeLog:9
> > +        While CSSParser::parseValue can process these shorthand properties properly and set the longhand properties
> 
> Nit: CSSParser::parseValue => CSSParser::parseValue()
> 
> Since you are referring to the function here as opposed to the name of the function.

I'm not a big fun of this particular notation since it doesn't state the full signature. In fact, CSSParser::parseValue() is strictly different from CSSParser::parseValue(StylePropertySet*, int, const String&, bool, bool, CSSStyleSheet*). While CSSParser::parseValue can refer to any symbols or functions of the same name.
Comment 11 Ryosuke Niwa 2012-03-23 12:37:45 PDT
Committed r111888: <http://trac.webkit.org/changeset/111888>
Comment 12 Ryosuke Niwa 2012-03-23 12:59:05 PDT
Thanks for the review guys :D