m_implicitShorthand is switched to true then false manually across the code, which is highly error-prone. Most probably this switching should either go into ShorthandScope or a standalone RAII implementation.
Created attachment 92251 [details] [PATCH] Suggested fix
Comment on attachment 92251 [details] [PATCH] Suggested fix View in context: https://bugs.webkit.org/attachment.cgi?id=92251&action=review > Source/WebCore/css/CSSParser.h:397 > + bool m_oldImplicitShorthand; is m_oldImplicitShorthand used?
(In reply to comment #2) > (From update of attachment 92251 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=92251&action=review > > > Source/WebCore/css/CSSParser.h:397 > > + bool m_oldImplicitShorthand; > > is m_oldImplicitShorthand used? Yikes, it was intended to be, but then I decided to retain the original behavior. Attaching a fixed patch now.
Created attachment 92272 [details] [PATCH] Removed unused field
Comment on attachment 92272 [details] [PATCH] Removed unused field View in context: https://bugs.webkit.org/attachment.cgi?id=92272&action=review > Source/WebCore/css/CSSParser.cpp:2411 > + ImplicitScope implicitScope(this, true); Can we move this before the switch statement and remove duplication? > Source/WebCore/css/CSSParser.h:386 > + ImplicitScope(CSSParser* parser, bool isImplicit = true) : m_parser(parser) Please make it explicit or it looks we don't need the default value for the second parameter. Also, We don't need to put the header file. This can be local to CSSparser.cpp.
Created attachment 93633 [details] [PATCH] Comments addressed
Comment on attachment 92272 [details] [PATCH] Removed unused field View in context: https://bugs.webkit.org/attachment.cgi?id=92272&action=review >> Source/WebCore/css/CSSParser.cpp:2411 >> + ImplicitScope implicitScope(this, true); > > Can we move this before the switch statement and remove duplication? No, the rule-specified "longhand" properties in a 4-value shorthand (e.g. top right bottom left) are not implicit. >> Source/WebCore/css/CSSParser.h:386 >> + ImplicitScope(CSSParser* parser, bool isImplicit = true) : m_parser(parser) > > Please make it explicit or it looks we don't need the default value for the second parameter. > Also, We don't need to put the header file. This can be local to CSSparser.cpp. Done.
Comment on attachment 93633 [details] [PATCH] Comments addressed View in context: https://bugs.webkit.org/attachment.cgi?id=93633&action=review > Source/WebCore/css/CSSParser.cpp:98 > + WTF_MAKE_FAST_ALLOCATED; Why are you adding this here? What about non-copyable? > Source/WebCore/css/CSSParser.cpp:100 > + ImplicitScope(WebCore::CSSParser* parser, bool isImplicit) : m_parser(parser) Put initialization for m_parser on a new line. Why pass in isImplicit when it is always true? > Source/WebCore/css/CSSParser.cpp:102 > + m_parser->m_implicitShorthand = isImplicit; It looks like you have a 3 space indent instead of 4 spaces. > Source/WebCore/css/CSSParser.cpp:103 > + } add blank line here.
(In reply to comment #8) > (From update of attachment 93633 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93633&action=review > > > Source/WebCore/css/CSSParser.cpp:98 > > + WTF_MAKE_FAST_ALLOCATED; > > Why are you adding this here? This class is a replica of ShorthandScope which has this macro. Removed. > What about non-copyable? Added. I'm not sure about the guidelines for both of these macros (are they articulated anywhere?) > > Source/WebCore/css/CSSParser.cpp:100 > > + ImplicitScope(WebCore::CSSParser* parser, bool isImplicit) : m_parser(parser) > > Put initialization for m_parser on a new line. Done. > Why pass in isImplicit when it is always true? I have a followup change that may win a little something with this in place. > > Source/WebCore/css/CSSParser.cpp:102 > > + m_parser->m_implicitShorthand = isImplicit; > > It looks like you have a 3 space indent instead of 4 spaces. Done. > > Source/WebCore/css/CSSParser.cpp:103 > > + } > > add blank line here. Done.
Created attachment 93751 [details] [PATCH] levin's comments addressed
@reviewers: ping?
Reviewers: ping
Comment on attachment 93751 [details] [PATCH] levin's comments addressed View in context: https://bugs.webkit.org/attachment.cgi?id=93751&action=review > Source/WebCore/css/CSSParser.cpp:100 > + ImplicitScope(WebCore::CSSParser* parser, bool isImplicit) bool isImplicit should be an enum (instead of a bool) because one can't tell at the calling site what it means. For example, when one sees ImplicitScope implicitScope(this, true); It isn't clear what "true" means.
(In reply to comment #9) > (In reply to comment #8) > > (From update of attachment 93633 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=93633&action=review > > What about non-copyable? > > Added. I'm not sure about the guidelines for both of these macros (are they articulated anywhere?) No idea about WTF_MAKE_FAST_ALLOCATE so I never include it. I leave it up to the people who put it in but i'm pretty sure it is meaningless in this case since it has to do with how the class is allocated (when using new) and this class is only used on the stack. For non-copyable, always add it whenever you can. (Think of it like the equivalent in the Google style guide.)
Created attachment 94630 [details] [PATCH] Enum instead of bool
Comment on attachment 94630 [details] [PATCH] Enum instead of bool View in context: https://bugs.webkit.org/attachment.cgi?id=94630&action=review > Source/WebCore/css/CSSParser.cpp:100 > + enum PropertyType { Making this enum a class member makes the call sites ugly with extra qualification needed. How about just making this a namespace-level enum instead? > Source/WebCore/css/CSSParser.cpp:101 > + PropertyExplicit = 0, Do we really need the "= 0" here? Please omit it. > Source/WebCore/css/CSSParser.cpp:105 > + ImplicitScope(WebCore::CSSParser* parser, PropertyType propertyType) I don’t think we need an argument at all. This class is already named ImplicitScope. I suggest removing the argument completely and just having this set to true. > Source/WebCore/css/CSSParser.cpp:108 > + m_parser->m_implicitShorthand = static_cast<bool>(propertyType); I don’t think the static_cast here is good style. Is it required by some compiler? Actually, I would suggest writing this: m_parser->m_implicitShorthand = propertyType == PropertyImplicit; I think that’s clearer.
Thanks for the review, Darin (In reply to comment #16) > (From update of attachment 94630 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=94630&action=review > > > Source/WebCore/css/CSSParser.cpp:100 > > + enum PropertyType { > > Making this enum a class member makes the call sites ugly with extra qualification needed. How about just making this a namespace-level enum instead? OK > > Source/WebCore/css/CSSParser.cpp:101 > > + PropertyExplicit = 0, > > Do we really need the "= 0" here? Please omit it. This was necessary to clarify the static_cast validity below. > > Source/WebCore/css/CSSParser.cpp:105 > > + ImplicitScope(WebCore::CSSParser* parser, PropertyType propertyType) > > I don’t think we need an argument at all. This class is already named ImplicitScope. I suggest removing the argument completely and just having this set to true. As I mentioned in the #9 comment, I've got a pending patch (https://bugs.webkit.org/attachment.cgi?id=92972) for bug 15598 that will make a good use of this argument (now it uses an ugly solution instead). > > Source/WebCore/css/CSSParser.cpp:108 > > + m_parser->m_implicitShorthand = static_cast<bool>(propertyType); > > I don’t think the static_cast here is good style. Is it required by some compiler? Actually, I would suggest writing this: > > m_parser->m_implicitShorthand = propertyType == PropertyImplicit; > > I think that’s clearer. Makes sense. The sole reason I wrote it this way is that I seem to have seen a similar cast once in the WebKit code (but not sure).
Created attachment 94635 [details] [PATCH] darin@'s comments addressed (parameter retained for reuse by a subsequent patch) The class name "ImplicitScope" denotes the fact that it affects the state of the "implicit" flag used by CSSParser when constructing property instances. Once a property has been determined to be implicit or explicit, the corresponding value can be passed into the ImplicitScope constructor (this is handy for constructing the "font" property longhands).
Committed r87281: <http://trac.webkit.org/changeset/87281>