Bug 51586 – CSSParser: m_implicitShorthand should probably be RAII

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Alexander Pavlov (apavlov)]

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.

[Alexander Pavlov (apavlov)]

Created attachment 92251 [details]
[PATCH] Suggested fix

[Luiz Agostini]

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?

[Alexander Pavlov (apavlov)]

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

[Alexander Pavlov (apavlov)]

Created attachment 92272 [details]
[PATCH] Removed unused field

[Hajime Morrita]

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.

[Alexander Pavlov (apavlov)]

Created attachment 93633 [details]
[PATCH] Comments addressed

[Alexander Pavlov (apavlov)]

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.

[David Levin]

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.

[Alexander Pavlov (apavlov)]

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

[Alexander Pavlov (apavlov)]

Created attachment 93751 [details]
[PATCH] levin's comments addressed

[Alexander Pavlov (apavlov)]

@reviewers: ping?

[Alexander Pavlov (apavlov)]

Reviewers: ping

[David Levin]

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.

[David Levin]

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

[Alexander Pavlov (apavlov)]

Created attachment 94630 [details]
[PATCH] Enum instead of bool

[Darin Adler]

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.

[Alexander Pavlov (apavlov)]

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

[Alexander Pavlov (apavlov)]

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

[Alexander Pavlov (apavlov)]

Committed r87281: <http://trac.webkit.org/changeset/87281>