[webkit-reviews] review granted: [Bug 52983] Eliminate m_tagHistory pointer from CSSSelector : [Attachment 80009] better CSSParserSelector, more OwnPtrs

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 52983: Eliminate m_tagHistory pointer from CSSSelector
https://bugs.webkit.org/show_bug.cgi?id=52983

Attachment 80009: better CSSParserSelector, more OwnPtrs
https://bugs.webkit.org/attachment.cgi?id=80009&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=80009&action=review

Patch looks great. I’m going to say review+ even though I have suggestions for
further improvement.

> Source/WebCore/css/CSSParser.cpp:5940
> +    CSSParserSelector* elementNameSelector = new CSSParserSelector;
> +    elementNameSelector->setTag(tag);
>      specifiers->setTagHistory(elementNameSelector);

The local variable here should be an OwnPtr<CSSParserSelector> and the call to
setTagHistory should have a release in it. That way, the adoptPtr will be right
next to the new, which is how we want it. We want an adopt right next to new as
much as possible. We could even make CSSParserSelector::create(), which returns
a PassOwnPtr.

> Source/WebCore/css/CSSParserValues.cpp:87
> +    Vector<CSSParserSelector*, 16> toDelete;

I’d use OwnPtr here.

> Source/WebCore/css/CSSParserValues.cpp:88
> +    CSSParserSelector* selector = m_tagHistory.leakPtr();

And OwnPtr here, and release instead of leakPtr.

> Source/WebCore/css/CSSParserValues.cpp:91
> +	   toDelete.append(selector);
> +	   CSSParserSelector* next = selector->m_tagHistory.leakPtr();

And get the value of next into an OwnPtr, using release instead of leakPtr. And
use another release on the append line.

> Source/WebCore/css/CSSParserValues.cpp:94
> +	   selector = next;

And there would be a release here.

> Source/WebCore/css/CSSParserValues.cpp:96
> +    deleteAllValues(toDelete);

And then no deleteAllValues here.

>> Source/WebCore/css/CSSParserValues.h:25
>> +#include "CSSSelector.h"
> 
> Alphabetical sorting problem.  [build/include_order] [4]

The style bot is right. The includes should go in the reverse order.

>> Source/WebCore/css/CSSSelector.h:257
>> +	    const QualifiedName& tag() const { return m_tag; } ;
> 
> Extra space before last semicolon. If this should be an empty statement, use
{ } instead.  [whitespace/semicolon] [5]

We should remove that last semicolon on the line. No semicolon after a function
body.

>> Source/WebCore/css/CSSSelector.h:266
>> +	    void setValue(const AtomicString& value);
> 
> The parameter name "value" adds no information, so it should be removed. 
[readability/parameter_name] [5]

I agree with the bot.

> Source/WebCore/css/CSSSelector.h:269
>	   void setAttribute(const QualifiedName& value);
>	   void setArgument(const AtomicString& value);
> -	   void setSimpleSelector(CSSSelector* value);
> +	   void setSimpleSelector(PassOwnPtr<CSSSelector> value);

No need for these “value” argument names either.

> Source/WebCore/css/CSSSelector.h:325
> +	       AtomicStringImpl* m_value; // Plain pointer to keep things
uniform with the union.

I still think that RefPtr would make things a little safer, but with the
comment this is at least not mysterious.

>> Source/WebCore/css/CSSSelectorList.cpp:68
>> +		PassOwnPtr<CSSSelector> selector = current->releaseSelector();
> 
> Local variables should never be PassOwnPtr (see
http://webkit.org/coding/RefPtr.html).	[readability/pass_ptr] [5]

This should be OwnPtr as the stylebot says.

> Source/WebCore/css/CSSSelectorList.cpp:73
> +	       memcpy(&m_selectorArray[arrayIndex], selector.get(),
sizeof(CSSSelector));
> +	       current = current->tagHistory();
> +	       // We want to free the memory (which was allocated with
fastNew), but we
> +	       // don't want the destructor to run since it will affect the
copy we've just made.;
> +	       fastDeleteSkippingDestructor(selector.leakPtr());

I’d really like to see a single operation that combines the memcpy with the
fastDeleteSkippingDestructor, and have it be a member of the CSSSelector class.
When the idiom is tricky like this I want it to be as tightly constrained as
possible to make it hard to misuse.

> Source/WebCore/css/CSSSelectorList.cpp:82
> +    m_selectorArray[arrayIndex - 1].setLastInSelectorList();

Can the passed-in vector be empty? I see code above for size 1, but what about
size 0?

> Source/WebCore/css/CSSSelectorList.h:45
> +    static CSSSelector* next(CSSSelector* current);

I don’t think the argument name “current” adds anything here.

> Source/WebCore/css/CSSSelectorList.h:64
> +inline CSSSelector* CSSSelectorList::next(CSSSelector* current)
> +{
> +    while (!current->isLastInTagHistory())
> +	   current++;
> +    return current->isLastInSelectorList() ? 0 : current + 1;
> +}

I think this may need a comment. Skipping the tag history runs is a slightly
confusing concept.