[webkit-reviews] review granted: [Bug 59301] Opacity transition doesn't work while content property is set. : [Attachment 96312] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Jun 7 16:52:29 PDT 2011


Darin Adler <darin at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 59301: Opacity transition doesn't work while content property is set.
https://bugs.webkit.org/show_bug.cgi?id=59301

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

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

r=me as is, but I see some opportunity to improve

> Source/WebCore/rendering/style/ContentData.cpp:53
> +    switch (type()) {

Since we already have subclasses for each type of ContentData, it would be
better style to use virtual function rather than a switch on type() for the
type-specific part of this function. That would eliminate all the calls to
static_cast.

> Source/WebCore/rendering/style/ContentData.cpp:57
> +	   RefPtr<StyleImage> image = const_cast<StyleImage*>(static_cast<const
ImageContentData*>(this)->image());

There must be some way to avoid this const_cast. Ugh.

> Source/WebCore/rendering/style/ContentData.cpp:75
> +    if (result && m_next)
> +	   result->setNext(m_next->clone());

It should be straightforward to write this in a loop instead of making it
recursive. It’s slightly better to not use stack space proportional to the
complexity of the content.

> Source/WebCore/rendering/style/CounterDirectives.cpp:45
> +    CounterDirectiveMap::const_iterator end = counterDirectives.end();
> +    for (CounterDirectiveMap::const_iterator it = counterDirectives.begin();
it != end; ++it)
> +	   result->add(it->first, it->second);

You should just be able to say *result = counterDirectives here. Did you try
that?

> Source/WebCore/rendering/style/StyleRareNonInheritedData.cpp:162
> +    if ((!m_counterDirectives && o.m_counterDirectives) ||
(m_counterDirectives && !o.m_counterDirectives))
> +	   return false;
> +    if (m_counterDirectives && o.m_counterDirectives &&
(*m_counterDirectives != *o.m_counterDirectives))
> +	   return false;
> +    return true;
> +

Your version is OK. My version is the same speed or faster and maybe a little
more obvious:

    if (m_counterDirectives == o.m_counterDirectives)
	return true;
    if (m_counterDirectives && o.m_counterDirectives && *m_counterDirectives ==
*o.m_counterDirectives)
	return false;
    return false;


More information about the webkit-reviews mailing list