Summary: | CSSParser needs a bath | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||||||||||||||||||||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | sam | ||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||||||||||||||
Attachments: |
|
Description
Eric Seidel (no email)
2008-06-06 14:45:49 PDT
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. |