Bug 82040

Created attachment 133457 [details] Fixes the bug

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 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.

Created attachment 133519 [details] Rebaseliined the failling test

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 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.

Created attachment 133525 [details] Fixed typos in the change log

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 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

(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.

Committed r111888: <http://trac.webkit.org/changeset/111888>

Thanks for the review guys :D