[webkit-reviews] review denied: [Bug 53248] Refactor CSSParser's helper functions and stuctures : [Attachment 81062] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Feb 3 12:57:49 PST 2011


Darin Adler <darin at apple.com> has denied Andras Becsi <abecsi at webkit.org>'s
request for review:
Bug 53248: Refactor CSSParser's helper functions and stuctures
https://bugs.webkit.org/show_bug.cgi?id=53248

Attachment 81062: Patch
https://bugs.webkit.org/attachment.cgi?id=81062&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=81062&action=review

> Source/WebCore/css/CSSParser.h:280
> +	   PassRefPtr<CSSPrimitiveValue>
parseDeprecatedGradientPoint(CSSParserValue*, bool horizontal);
> +	   PassRefPtr<CSSPrimitiveValue>
parseGradientColorOrKeyword(CSSParserValue*);
> +	   bool parseDeprecatedGradientColorStop(CSSParserValue*,
CSSGradientColorStop&);
> +	   bool parseBackgroundClip(CSSParserValue*, RefPtr<CSSValue>&);
> +	   static bool parseColorInt(const UChar*& string, const UChar* end,
UChar terminator, int& value);
> +	   static bool parseAlphaValue(const UChar*& string, const UChar* end,
UChar terminator, int& value);

Why are these now member functions?

Making these member functions instead of standalone functions means that if you
change the function arguments or name you have to modify the header file.
That’s not as good as being able to modify them without touching the header.

Making four of these non-static member functions adds another unneeded unused
argument to the functions.

Is there some advantage that offsets these disadvantages?

> Source/WebCore/css/CSSParserHelpers.h:25
> +#ifndef CSSParserHelpers_h
> +#define CSSParserHelpers_h

It makes no sense to me to have a file called “helpers”. We normally use a file
for each class. It’s not good to have “utilities” or “helpers” files, because
there’s no good way to know what will be in such files.

> Source/WebCore/css/CSSParserHelpers.h:28
> +#define DASHBOARD_REGION_NUM_PARAMETERS	  6
> +#define DASHBOARD_REGION_SHORT_NUM_PARAMETERS  2

Things defined in a header need to go after the includes needed by the header.

We frown on use of macros for constants rather than C++ constants. And even
more for such things in headers. Does this need to be in a header?

> Source/WebCore/css/CSSParserHelpers.h:43
> +static inline bool equal(const CSSParserString& a, const char* b)

It’s not appropriate to put static on a function that’s in a header. That gives
it internal linkage which is not right for things in headers.

> Source/WebCore/css/CSSParserStructs.h:2
> +#ifndef CSSParserStructs_h
> +#define CSSParserStructs_h

This file has no header.

It is not good style to have a file named “structs” for the same reason we
don’t have files named “helpers”.


More information about the webkit-reviews mailing list