[Webkit-unassigned] [Bug 52983] Eliminate m_tagHistory pointer from CSSSelector

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=52983


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #80009|review?                     |review+
               Flag|                            |




--- Comment #7 from Darin Adler <darin at apple.com>  2011-01-25 08:57:47 PST ---
(From update of attachment 80009)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.