Bug 93574 – Convert CSSParser's m_reusableSelectorVector to OwnPtr and rename to m_selectorVector.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Shane Stephens]

Convert CSSParser::m_reusableSelectorVector to OwnPtr and rename to m_selectorVector.

[Shane Stephens]

Created attachment 157381 [details]
Patch

[Tony Chang]

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?

[Shane Stephens]

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?

[Shane Stephens]

Created attachment 157389 [details]
Patch

[Tony Chang]

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

Yeah, not worth moving.

[Shane Stephens]

Created attachment 157398 [details]
Patch

[WebKit Review Bot]

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

[Shane Stephens]

Created attachment 157615 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 157615 [details]
Patch for landing

Clearing flags on attachment: 157615

Committed r125252: <http://trac.webkit.org/changeset/125252>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Andreas Kling]

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?

[Tony Chang]

(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.