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.
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(-)
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
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
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.
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
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(-)
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(-)
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.
Comment on attachment 21549 [details] Rename CSSParser::valueList to m_valueList rs=me
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(-)
Comment on attachment 21550 [details] Rename CSSParser::rule to m_rule rs=me
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(-)
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(-)
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(-)
Comment on attachment 21551 [details] rename CSSParser::important to m_important rs=me
Comment on attachment 21552 [details] Rename CSSParser::defaultNamespace to m_defaultNamespace rs=me
Comment on attachment 21553 [details] Rename CSSParser::strict to m_strict rs=me
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(-)
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 ;)
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(-)
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
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(-)
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
Ok, I think this bug has had enough lovin. CSSParser still needs more work, but that can be done as part of other bugs.