WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
19415
CSSParser needs a bath
https://bugs.webkit.org/show_bug.cgi?id=19415
Summary
CSSParser needs a bath
Eric Seidel (no email)
Reported
2008-06-06 14:45:49 PDT
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.
Attachments
Rename CSSParser::styleElement to m_styleElement
(15.07 KB, patch)
2008-06-06 14:46 PDT
,
Eric Seidel (no email)
mitz: review+
Details
Formatted Diff
Diff
Rename CSSParser::parsedProperties to m_parsedProperties
(9.90 KB, patch)
2008-06-06 14:57 PDT
,
Eric Seidel (no email)
mitz: review+
Details
Formatted Diff
Diff
Rename CSSParser::numParsedProperties and maxNumParsedProperites to m_*
(11.62 KB, patch)
2008-06-06 15:05 PDT
,
Eric Seidel (no email)
mitz: review+
Details
Formatted Diff
Diff
More cleanup to CSSParser
(4.58 KB, patch)
2008-06-07 00:34 PDT
,
Eric Seidel (no email)
ap
: review-
Details
Formatted Diff
Diff
Rename CSSParser::valueList to m_valueList
(47.11 KB, patch)
2008-06-07 00:52 PDT
,
Eric Seidel (no email)
ap
: review+
Details
Formatted Diff
Diff
Rename CSSParser::rule to m_rule
(3.21 KB, patch)
2008-06-07 01:04 PDT
,
Eric Seidel (no email)
ap
: review+
Details
Formatted Diff
Diff
rename CSSParser::important to m_important
(2.89 KB, patch)
2008-06-07 01:14 PDT
,
Eric Seidel (no email)
ap
: review+
Details
Formatted Diff
Diff
Rename CSSParser::defaultNamespace to m_defaultNamespace
(4.67 KB, patch)
2008-06-07 01:21 PDT
,
Eric Seidel (no email)
ap
: review+
Details
Formatted Diff
Diff
Rename CSSParser::strict to m_strict
(25.63 KB, patch)
2008-06-07 01:39 PDT
,
Eric Seidel (no email)
ap
: review+
Details
Formatted Diff
Diff
still more CSSParser cleanup
(5.35 KB, patch)
2008-06-07 02:02 PDT
,
Eric Seidel (no email)
ap
: review+
Details
Formatted Diff
Diff
even more cssparser cleanup
(19.10 KB, patch)
2008-06-07 02:48 PDT
,
Eric Seidel (no email)
ap
: review+
Details
Formatted Diff
Diff
Reviewed by ap.
(5.88 KB, patch)
2008-06-07 03:07 PDT
,
Eric Seidel (no email)
ap
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Eric Seidel (no email)
Comment 1
2008-06-06 14:46:38 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(-)
Eric Seidel (no email)
Comment 2
2008-06-06 14:47:58 PDT
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.
mitz
Comment 3
2008-06-06 14:48:00 PDT
Comment on
attachment 21529
[details]
Rename CSSParser::styleElement to m_styleElement r=me
Eric Seidel (no email)
Comment 4
2008-06-06 14:57:30 PDT
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(-)
mitz
Comment 5
2008-06-06 14:58:11 PDT
Comment on
attachment 21530
[details]
Rename CSSParser::parsedProperties to m_parsedProperties r=me
Darin Adler
Comment 6
2008-06-06 15:05:12 PDT
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)
Comment 7
2008-06-06 15:05:39 PDT
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(-)
mitz
Comment 8
2008-06-06 15:07:41 PDT
Comment on
attachment 21532
[details]
Rename CSSParser::numParsedProperties and maxNumParsedProperites to m_* ditto
Eric Seidel (no email)
Comment 9
2008-06-07 00:34:25 PDT
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)
Comment 10
2008-06-07 00:52:33 PDT
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 11
2008-06-07 01:00:07 PDT
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 12
2008-06-07 01:01:33 PDT
Comment on
attachment 21549
[details]
Rename CSSParser::valueList to m_valueList rs=me
Eric Seidel (no email)
Comment 13
2008-06-07 01:04:04 PDT
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 14
2008-06-07 01:11:07 PDT
Comment on
attachment 21550
[details]
Rename CSSParser::rule to m_rule rs=me
Eric Seidel (no email)
Comment 15
2008-06-07 01:14:09 PDT
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)
Comment 16
2008-06-07 01:21:34 PDT
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)
Comment 17
2008-06-07 01:39:30 PDT
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 18
2008-06-07 01:48:00 PDT
Comment on
attachment 21551
[details]
rename CSSParser::important to m_important rs=me
Alexey Proskuryakov
Comment 19
2008-06-07 01:48:18 PDT
Comment on
attachment 21552
[details]
Rename CSSParser::defaultNamespace to m_defaultNamespace rs=me
Alexey Proskuryakov
Comment 20
2008-06-07 01:48:45 PDT
Comment on
attachment 21553
[details]
Rename CSSParser::strict to m_strict rs=me
Eric Seidel (no email)
Comment 21
2008-06-07 02:02:31 PDT
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 22
2008-06-07 02:09:51 PDT
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)
Comment 23
2008-06-07 02:48:42 PDT
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 24
2008-06-07 02:59:34 PDT
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)
Comment 25
2008-06-07 03:07:35 PDT
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 26
2008-06-07 03:11:05 PDT
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)
Comment 27
2008-06-07 03:34:44 PDT
Ok, I think this bug has had enough lovin. CSSParser still needs more work, but that can be done as part of other bugs.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug