Bug 53248 – Refactor CSSParser's helper functions and stuctures

• Status: RESOLVED
• Resolution: WONTFIX
• Component: CSS

[Andras Becsi]

Move the definitions of CSS helper functions, and structures to their own headers to make CSSParser.cpp less crowded. Make the structures fast allocated.

[Andras Becsi]

Created attachment 80345 [details]
proposed patch

[WebKit Review Bot]

Attachment 80345 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/css/CSSParserHelpers.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/css/CSSParserHelpers.h:37:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/css/CSSParserHelpers.h:112:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/WebCore/css/CSSParserHelpers.h:142:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/css/CSSParserHelpers.h:332:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/css/CSSParserHelpers.h:432:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/WebCore/css/CSSParserHelpers.h:460:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Source/WebCore/css/CSSParser.h:275:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/css/CSSParser.h:276:  The parameter name "value" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/css/CSSParser.h:277:  The parameter name "a" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/css/CSSParser.h:277:  The parameter name "stop" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/css/CSSParserStructs.h:165:  Local variables should never be PassRefPtr (see http://webkit.org/coding/RefPtr.html).  [readability/pass_ptr] [5]
Source/WebCore/css/CSSParserStructs.h:304:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/css/CSSParserStructs.h:305:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/css/CSSParser.cpp:4179:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Source/WebCore/css/CSSParser.cpp:4206:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 16 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andras Becsi]

Created attachment 80445 [details]
proposed patch

[Andras Becsi]

Created attachment 80447 [details]
proposed patch

Deduplicated the changelog entry.

[Eric Seidel (no email)]

So to be clear:

1. You're only moving code, no functional changes, right?
2.  It's unclear if we should have performance concerns about this change.  Did you do any perf testing?

[Andras Becsi]

(In reply to comment #5)
> So to be clear:
> 
> 1. You're only moving code, no functional changes, right?
> 2.  It's unclear if we should have performance concerns about this change.  Did you do any perf testing?

Sorry for the late response:

1. Yes, I only move the functions and structures to separate headers and add missing allocation macros to structures.
However I just realized that in the uploaded patch git tries to be smart and this is a bit confusing. I'll work around this in an updated patch.

2. My main goal was not to introduce performance penalties when making CSSParser.cpp a bit less crowded. That's why I kept the inlining of the functions and did not introduce another build unit. I used our page loading test to measure performance, and it did not show any performance change on the Qt port.

I'll update the patch to ToT.

[Andras Becsi]

Created attachment 80912 [details]
updated patch

[WebKit Review Bot]

Attachment 80912 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 2

perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LANG = "en_US.US-ASCII"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
Updating OpenSource
git.webkit.org[0: 17.254.20.231]: errno=Connection timed out
fatal: unable to connect a socket (Connection timed out)
Died at Tools/Scripts/update-webkit line 131.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andras Becsi]

Created attachment 81062 [details]
Patch

[Darin Adler]

Comment on attachment 81062 [details]
Patch

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

[Andras Becsi]

Closing this as wontfix. There is no achievable performance or memory benefit.