[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