Bug 93574

Created attachment 157381 [details] Patch

Comment on attachment 157381 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157381&action=review r- for the ChangeLog. After you upload a new patch, let's wait a day so people in other times zones can look and provide feedback. > Source/WebCore/ChangeLog:22 > + * css/CSSGrammar.y: > + * css/CSSParser.cpp: > + (WebCore::CSSParser::CSSParser): > + (WebCore::CSSParser::parseValue): > + (WebCore::CSSParser::parseColor): Please annotate some of the major changes here. For example, renaming reusableSelectorVector to selectorVector or where the typedef is added. > Source/WebCore/css/CSSParser.cpp:1159 > + declaration->addParsedProperties(*m_parsedProperties.get()); Nit: I don't think you need the .get(). > Source/WebCore/css/CSSParser.cpp:1250 > - declaration->addParsedProperties(m_parsedProperties); > + declaration->addParsedProperties(*m_parsedProperties.get()); Nit: I don't think you need the .get(). > Source/WebCore/css/CSSParserValues.h:109 > +typedef Vector<OwnPtr<CSSParserSelector> > CSSSelectorVector; If we moved this typedef into CSSParserSelector, would we have to say CSSParserSelector::CSSSelectorVector in many places?

Comment on attachment 157381 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=157381&action=review >> Source/WebCore/ChangeLog:22 >> + (WebCore::CSSParser::parseColor): > > Please annotate some of the major changes here. For example, renaming reusableSelectorVector to selectorVector or where the typedef is added. Done. Most changes are pretty minor though. >> Source/WebCore/css/CSSParser.cpp:1159 >> + declaration->addParsedProperties(*m_parsedProperties.get()); > > Nit: I don't think you need the .get(). Done. >> Source/WebCore/css/CSSParser.cpp:1250 >> + declaration->addParsedProperties(*m_parsedProperties.get()); > > Nit: I don't think you need the .get(). Done. >> Source/WebCore/css/CSSParserValues.h:109 >> +typedef Vector<OwnPtr<CSSParserSelector> > CSSSelectorVector; > > If we moved this typedef into CSSParserSelector, would we have to say CSSParserSelector::CSSSelectorVector in many places? A rough count: 1 in CSSGrammar.y 7 in CSSParser.cpp, 9 in CSSParser.h 1 in CSSParserValues.cpp, 2 in CSSParserValues.h 1 in CSSSelectorList.cpp, 1 in CSSSelectorList.h 1 in StyleRule.cpp, 4 in StyleRule.h For a grand total of 27 places. Probably not worth moving?

Created attachment 157389 [details] Patch

(In reply to comment #3) > For a grand total of 27 places. Probably not worth moving? Yeah, not worth moving.

Created attachment 157398 [details] Patch

Comment on attachment 157398 [details] Patch Rejecting attachment 157398 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: orationProperty(WebCore::CSSPropertyID, WTF::PassRefPtr<WebCore::CSSValue>, bool)': Source/WebCore/css/CSSParser.cpp:7921: error: 'class WTF::OwnPtr<WTF::Vector<WebCore::CSSProperty, 256ul> >' has no member named 'size' Source/WebCore/css/CSSParser.cpp:7922: error: no match for 'operator[]' in '((WebCore::CSSParser*)this)->WebCore::CSSParser::m_parsedProperties[i]' make: *** [out/Debug/obj.target/webcore_remaining/Source/WebCore/css/CSSParser.o] Error 1 make: *** Waiting for unfinished jobs.... Full output: http://queues.webkit.org/results/13471322

Created attachment 157615 [details] Patch for landing

Comment on attachment 157615 [details] Patch for landing Clearing flags on attachment: 157615 Committed r125252: <http://trac.webkit.org/changeset/125252>

All reviewed patches have been landed. Closing bug.

This patch added two heap allocations to the CSSParser constructor as "prep work" for CSS hierarchies (bug 79939) which hasn't progressed in over a month AFAICT. I don't think this is acceptable overhead for an experimental feature. Can we put this behind ENABLE(CSS_HIERARCHIES) somehow?

(In reply to comment #11) > This patch added two heap allocations to the CSSParser constructor as "prep work" for CSS hierarchies (bug 79939) which hasn't progressed in over a month AFAICT. > > I don't think this is acceptable overhead for an experimental feature. Can we put this behind ENABLE(CSS_HIERARCHIES) somehow? Shane is on paternity leave, which is why he's been AFK. If this is showing up on a profile or page cycler, I think it would be fine to roll out. Wouldn't putting this behind an ENABLE(CSS_HIERARCHIES) be worse? It touches a lot of lines of code.