Bug 19415 – CSSParser needs a bath

• Component: CSS
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

I was trying to hunt down a crasher in CSSParser earlier this week.  The ugliness of some of this old code was driving me batty.  So I decided to finally clean a little of it.  I don't have much time ATM, but I'll post the first (of eventually many) cleanup patches.

[Eric Seidel (no email)]

Created attachment 21529 [details]
Rename CSSParser::styleElement to m_styleElement

 WebCore/ChangeLog         |   21 ++++++++++++++++++
 WebCore/css/CSSGrammar.y  |   32 +++++++++++++-------------
 WebCore/css/CSSParser.cpp |   52 ++++++++++++++++++++++----------------------
 WebCore/css/CSSParser.h   |    2 +-
 4 files changed, 64 insertions(+), 43 deletions(-)

[Eric Seidel (no email)]

The real problem with CSSParser (in terms of variable naming) is that there is all sorts of argument shadowing of member variables!  "id" "important" are common offenders.  I've not tried to fix that yet (as it will take some careful surgery.  But this patch is one step in that direction.  More when I (or Sam, or other cleanup artists) have time.

Comment on attachment 21529 [details]
Rename CSSParser::styleElement to m_styleElement

r=me

[Eric Seidel (no email)]

Created attachment 21530 [details]
Rename CSSParser::parsedProperties to m_parsedProperties

 WebCore/ChangeLog            |   23 +++++++++++++++++++++++
 WebCore/css/CSSParser.cpp    |   38 +++++++++++++++++++-------------------
 WebCore/css/CSSParser.h      |    2 +-
 WebCore/css/SVGCSSParser.cpp |    2 +-
 4 files changed, 44 insertions(+), 21 deletions(-)

Comment on attachment 21530 [details]
Rename CSSParser::parsedProperties to m_parsedProperties

r=me

[Darin Adler]

The thing about styleElement is that it's not really an arbitrary style element, even though the code seems to think it is. It's *always* a stylesheet. I have a patch that renames styleElement to m_styleSheet and changes its type as well, but it's not currently attached to any bug.

[Eric Seidel (no email)]

Created attachment 21532 [details]
Rename CSSParser::numParsedProperties and maxNumParsedProperites to m_*

 WebCore/ChangeLog            |   23 +++++++++++++++++++
 WebCore/css/CSSGrammar.y     |    8 +++---
 WebCore/css/CSSParser.cpp    |   51 +++++++++++++++++++----------------------
 WebCore/css/CSSParser.h      |    6 ++--
 WebCore/css/SVGCSSParser.cpp |    2 +-
 5 files changed, 55 insertions(+), 35 deletions(-)

Comment on attachment 21532 [details]
Rename CSSParser::numParsedProperties and maxNumParsedProperites to m_*

ditto

[Eric Seidel (no email)]

Created attachment 21548 [details]
More cleanup to CSSParser

 WebCore/ChangeLog         |   17 +++++++++++++
 WebCore/css/CSSParser.cpp |   59 ++++++++++++++------------------------------
 WebCore/css/CSSParser.h   |    2 +-
 3 files changed, 37 insertions(+), 41 deletions(-)

[Eric Seidel (no email)]

Created attachment 21549 [details]
Rename CSSParser::valueList to m_valueList

 WebCore/ChangeLog            |   47 ++++++++
 WebCore/css/CSSGrammar.y     |   12 +-
 WebCore/css/CSSParser.cpp    |  240 +++++++++++++++++++++---------------------
 WebCore/css/CSSParser.h      |    2 +-
 WebCore/css/SVGCSSParser.cpp |   32 +++---
 5 files changed, 190 insertions(+), 143 deletions(-)

[Alexey Proskuryakov]

Comment on attachment 21548 [details]
More cleanup to CSSParser

I do not think it's such a great idea to wrap CSSParser::currentParser manipulation in a function, when it can be just removed (see how it's done in JSC, YYLEX_PARAM and YYPARSE_PARAM).

r- for you to consider doing it this way.

rs=me on m_mediaQuery renaming.

[Alexey Proskuryakov]

Comment on attachment 21549 [details]
Rename CSSParser::valueList to m_valueList

rs=me

[Eric Seidel (no email)]

Created attachment 21550 [details]
Rename CSSParser::rule to m_rule

 WebCore/ChangeLog         |   15 +++++++++++++++
 WebCore/css/CSSGrammar.y  |    2 +-
 WebCore/css/CSSParser.cpp |   10 +++++-----
 WebCore/css/CSSParser.h   |    2 +-
 4 files changed, 22 insertions(+), 7 deletions(-)

[Alexey Proskuryakov]

Comment on attachment 21550 [details]
Rename CSSParser::rule to m_rule

rs=me

[Eric Seidel (no email)]

Created attachment 21551 [details]
rename CSSParser::important to m_important

 WebCore/ChangeLog         |   14 ++++++++++++++
 WebCore/css/CSSGrammar.y  |    2 +-
 WebCore/css/CSSParser.cpp |    8 ++++----
 WebCore/css/CSSParser.h   |    2 +-
 4 files changed, 20 insertions(+), 6 deletions(-)

[Eric Seidel (no email)]

Created attachment 21552 [details]
Rename CSSParser::defaultNamespace to m_defaultNamespace

 WebCore/ChangeLog             |   14 ++++++++++++++
 WebCore/css/CSSGrammar.y      |   12 ++++++------
 WebCore/css/CSSParser.cpp     |    4 ++--
 WebCore/css/CSSParser.h       |    2 +-
 WebCore/css/CSSStyleSheet.cpp |    2 +-
 5 files changed, 24 insertions(+), 10 deletions(-)

[Eric Seidel (no email)]

Created attachment 21553 [details]
Rename CSSParser::strict to m_strict

 WebCore/ChangeLog         |   26 +++++++++++
 WebCore/css/CSSGrammar.y  |    6 +-
 WebCore/css/CSSParser.cpp |  106 ++++++++++++++++++++++----------------------
 WebCore/css/CSSParser.h   |    2 +-
 4 files changed, 83 insertions(+), 57 deletions(-)

[Alexey Proskuryakov]

Comment on attachment 21551 [details]
rename CSSParser::important to m_important

rs=me

[Alexey Proskuryakov]

Comment on attachment 21552 [details]
Rename CSSParser::defaultNamespace to m_defaultNamespace

rs=me

[Alexey Proskuryakov]

Comment on attachment 21553 [details]
Rename CSSParser::strict to m_strict

rs=me

[Eric Seidel (no email)]

Created attachment 21555 [details]
still more CSSParser cleanup

 WebCore/ChangeLog         |   17 +++++++++++++++++
 WebCore/css/CSSGrammar.y  |    2 +-
 WebCore/css/CSSParser.cpp |   34 +++++++++++++++++-----------------
 WebCore/css/CSSParser.h   |    4 ++--
 4 files changed, 37 insertions(+), 20 deletions(-)

[Alexey Proskuryakov]

Comment on attachment 21555 [details]
still more CSSParser cleanup

rs=me 

+    if (m_data)
+        fastFree(m_data);

You can remove the check, if git allows for that ;)

[Eric Seidel (no email)]

Created attachment 21557 [details]
even more cssparser cleanup

 WebCore/ChangeLog            |   27 ++++++++++++
 WebCore/css/CSSGrammar.y     |   40 +++++++++---------
 WebCore/css/CSSParser.cpp    |   91 +++++++++++++++++++++---------------------
 WebCore/css/CSSParser.h      |    4 +-
 WebCore/css/SVGCSSParser.cpp |    3 +-
 5 files changed, 95 insertions(+), 70 deletions(-)

[Alexey Proskuryakov]

Comment on attachment 21557 [details]
even more cssparser cleanup

+            parsedValue = new CSSFontFaceSrcValue(KURL(m_styleSheet->baseURL(), value).string(), false);

StyleBase::baseURL() returns KURL, you can avoid copying here (repeated several times).

+    if (!m_styleSheet->isCSSStyleSheet())
         return 0;

This looks like it should be omitted (also, more than once).

r=me

[Eric Seidel (no email)]

Created attachment 21558 [details]
        Reviewed by ap.


        More cleanup to CSSParser, rename mediaQuery to m_mediaQuery
        Remove CSSParser::current and CSSParser::currentParser and use
        the magic of YYLEX_PARAM instead.

        * css/CSSParser.cpp:
        (WebCore::enterGeneratedParser):
        (WebCore::CSSParser::parseSheet):
        (WebCore::CSSParser::parseRule):
        (WebCore::CSSParser::parseValue):
        (WebCore::CSSParser::parseColor):
        (WebCore::CSSParser::parseDeclaration):
        (WebCore::CSSParser::parseMediaQuery):
        * css/CSSParser.h:
---
 WebCore/ChangeLog         |   18 ++++++++++++++++++
 WebCore/css/CSSGrammar.y  |   14 +++++++++++---
 WebCore/css/CSSParser.cpp |   41 +++++------------------------------------
 WebCore/css/CSSParser.h   |    6 +-----
 4 files changed, 35 insertions(+), 44 deletions(-)

[Alexey Proskuryakov]

Comment on attachment 21558 [details]
        Reviewed by ap.

"you can avoid copying here" - bzzz, false!

+    if (string.isEmpty() || string.isNull())

This can be just isEmpty().

r=me

[Eric Seidel (no email)]

Ok, I think this bug has had enough lovin.  CSSParser still needs more work, but that can be done as part of other bugs.