[webkit-reviews] review denied: [Bug 93574] Convert CSSParser's m_reusableSelectorVector to OwnPtr and rename to m_selectorVector. : [Attachment 157381] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Aug 8 22:03:15 PDT 2012


Tony Chang <tony at chromium.org> has denied Shane Stephens
<shanestephens at google.com>'s request for review:
Bug 93574: Convert CSSParser's m_reusableSelectorVector to OwnPtr and rename to
m_selectorVector.
https://bugs.webkit.org/show_bug.cgi?id=93574

Attachment 157381: Patch
https://bugs.webkit.org/attachment.cgi?id=157381&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
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?


More information about the webkit-reviews mailing list