[webkit-reviews] review granted: [Bug 52983] Eliminate m_tagHistory pointer from CSSSelector : [Attachment 79876] patch for review

[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 79876: patch for review
https://bugs.webkit.org/attachment.cgi?id=79876&action=review

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

For some reason the EWS bot can’t apply this patch. You may want to find out
why, because it can be useful to know if any builds are broken.

Basic technique seems good; it’s unfortunate the details get a bit messy. More
smart pointer use may help a little even though we’ll need to move things in
and out of the smart pointers with adopt and leak.

I think that CSSParserSelector would be better if it had functions for all the
things that the grammar wants to do to a selector. There’s no reason the
grammar needs to reach through it by calling selector() each time. A few inline
functions for the operations done by the grammar wouldn’t be all that hard to
write, and generally speaking we want to keep the grammar file as simple as
possible and put as much of the complexity in code outside that file.

I’m going to say review+ because after reading all my comments none seems
mandatory, but I think there is room for improvement here, and I’d really like
to see EWS results.

> Source/WebCore/css/CSSGrammar.y:1218
> +	       CSSParserSelector* parserSelector = p->sinkFloatingSelector($4);

> +	       CSSSelector* simpleSelector = parserSelector->releaseSelector();

> +	       $$->selector()->setSimpleSelector(simpleSelector);
> +	       delete parserSelector;

The sinkFloatingSelector function should return a
PassOwnPtr<CSSParserSelector>. Similarly, the releaseSelector function should
return a PassOwnPtr<CSSSelector> and the setSimpleSelctor function should take
a PassOwnPtr. Then we would not need any local variables here. It would just
read:

   
$$->selector()->setSimpleSelector(p->sinkFloatingSelector($4)->releaseSelector(
));

And all the ownership transfer would be taken care of.

> Source/WebCore/css/CSSParser.cpp:5706
> -CSSSelector* CSSParser::sinkFloatingSelector(CSSSelector* selector)
> +CSSParserSelector* CSSParser::sinkFloatingSelector(CSSParserSelector*
selector)

This should return a PassOwnPtr as I mentioned above.

> Source/WebCore/css/CSSParserValues.cpp:80
> +    , m_tagHistory(0)

I find the name “tag history” confusing. But it’s not like it’s a new term, so
not your problem.

> Source/WebCore/css/CSSParserValues.h:98
> +    CSSSelector* releaseSelector() { return m_selector.leakPtr(); }

This should return a PassOwnPtr.

> Source/WebCore/css/CSSParserValues.h:100
> +    void setTagHistory(CSSParserSelector* selector) { m_tagHistory =
selector; }

This should take a PassOwnPtr.

> Source/WebCore/css/CSSParserValues.h:104
> +    CSSParserSelector* m_tagHistory;

This should be an OwnPtr too. You can call leakPtr on it in the destructor.

> Source/WebCore/css/CSSSelector.h:256
> +	   // AtomicString is really just an AtomicStringImpl* so the cast
below is safe.
> +	   const AtomicString& value() const { return *reinterpret_cast<const
AtomicString*>(m_hasRareData ? &m_data.m_rareData->m_value : &m_data.m_value);
}

I wonder if there’s a way to avoid this. I believe we’ve had trouble with this
kind of code in the past, with high optimization settings in the compiler.
Maybe we can have value() just return an AtomicStringImpl*?

> Source/WebCore/css/CSSSelector.h:272
> +	   void setValue(const AtomicString& value)
> +	   { 
> +	       if (m_hasRareData) {
> +		   m_data.m_rareData->m_value = value.impl();
> +		   m_data.m_rareData->m_value->ref();
> +		   return;
> +	       }
> +	       // Need to do ref counting manually in the union.
> +	       m_data.m_value = value.impl();
> +	       m_data.m_value->ref();
> +	   }

When a function gets this long I prefer to put the definition outside the
class. Maybe we should take a pass through this class and move the function
bodies out of the class. I find it makes the class way easier to read.

> Source/WebCore/css/CSSSelector.h:329
> +	       RareData(AtomicStringImpl* value)
> +		   : m_value(value)
> +		   , m_a(0)
>		   , m_b(0)
> -		   , m_tagHistory(tagHistory)
>		   , m_attribute(anyQName())
>		   , m_argument(nullAtom)
>	       {
>	       }
> +	       ~RareData()
> +	       {
> +		   if (m_value)
> +		       m_value->deref();
> +	       }

This seems hard to use correctly. The constructor doesn’t ref but the
destructor does deref. I have no doubt the code currently handles this
correctly, but I suspect it will be easy to screw this up later.

One way to do this is to make the constructor argument be a
PassRefPtr<AtomicStringImpl> and then call leakPtr on it. This makes it much
harder to use it incorrectly.

> Source/WebCore/css/CSSSelector.h:334
> +	       AtomicStringImpl* m_value;

Why not use AtomicString or RefPtr<AtomicStringImpl> here? I don’t think a raw
pointer is best. It’s easy to transfer ownership from a raw pointer, by calling
adoptRef on it. Even though you need to use a raw pointer in the union, I think
it’s better to not use a raw pointer here as well.

> Source/WebCore/css/CSSSelector.h:353
> -	       DataUnion() : m_tagHistory(0) { }
> -	       CSSSelector* m_tagHistory;
> +	       DataUnion() : m_value(0) { }
>	       RareData* m_rareData;
> +	       AtomicStringImpl* m_value;

The old union used to initialize the first value. Now you’re initializing the
second, which is a little stranger. Obviously the order doesn’t matter, but it
kind of attracts the attention a tiny bit.

> Source/WebCore/css/CSSSelectorList.cpp:50
> +    const size_t vectorSize = selectorVector.size();
> +    size_t arraySize = 0;

I don’t think it’s all that helpful to distinguish the passed-in vector from
the adopted one by calling the latter an array. I know it’s using malloc
directly, but that seems an implementation detail. I think the internal one
could be called something like “flattened” instead. Or “complete”. Or we could
call it the “list” since it’s the actual list.

> Source/WebCore/css/CSSSelectorList.cpp:60
> +	   delete selectorVector[0];

This makes it clear that selectorVector should be a
Vector<OwnPtr<CSSParserSelector> >.

Doing the deletion by hand is trickier than it needs to be.

> Source/WebCore/css/CSSSelectorList.cpp:64
> +    m_selectorArray =
reinterpret_cast<CSSSelector*>(fastMalloc(sizeof(CSSSelector) * arraySize));

This should be static_cast, not reinterpret_cast.

> Source/WebCore/css/CSSSelectorList.cpp:70
> +	       memcpy(&m_selectorArray[arrayIndex], selector,
sizeof(CSSSelector));

It’s dangerous to move a CSSSelector this way unless the class knows about it;
some classes have pointers into themselves or other such things. If we really
need this move operation, we should make a function member of the CSSSelector
class to do the work. That makes it more likely that someone changing
CSSSelector around won’t invalidate this operation. We can just name the
function move or moveIntoUninitializedMemory or something like that and make it
either a member function or static member function.

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

Same comment about keeping long functions out of the class. I also think that
the various places where we do the “+ 1” trick to find adjacent elements need
“why” comments.

> Source/WebCore/css/CSSSelectorList.h:50
> +    bool hasOneSelector() const { return m_selectorArray ?
!next(m_selectorArray) : false; }

I prefer && over ? : for cases like this, even thought the old code used ? :