[webkit-reviews] review denied: [Bug 40857] Altering the CSS class of an attached SVG element causes WebKit to crash : [Attachment 60542] Updated Patch -- The previous one was the wrong file
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Jul 5 09:38:44 PDT 2010
Darin Adler <darin at apple.com> has denied review:
Bug 40857: Altering the CSS class of an attached SVG element causes WebKit to
crash
https://bugs.webkit.org/show_bug.cgi?id=40857
Attachment 60542: Updated Patch -- The previous one was the wrong file
https://bugs.webkit.org/attachment.cgi?id=60542&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
You did not put this patch up for review, which is done by setting the review
flag to "?", but I assume you want it reviewed.
> + if (!namedAttrMap)
> + attributes(false);
> + if (hasClass)
> + mappedAttributes()->setClass(newClassString);
> + else
> + mappedAttributes()->clearClass();
The attributes function is not meant to be called just for its side effect. If
you have determined it needs to be called, then you should use its result and
call setClass and clearClass on that. There is no need to use the
mappedAttributes function.
Also, there is no need to change the clearClass code path. In that path,
checking namedAttrMap for 0 seems the right way to go. It's only in the
setClass path that we'd want to call attributes to force the map to be created.
Something like this:
if (hasClass)
attributes()->setClass(newClassString);
else {
if (namedAttrMap)
namedAttrMap->clearClass();
}
That would work well, I think.
More information about the webkit-reviews
mailing list