[webkit-reviews] review granted: [Bug 100397] Eliminate setRenderStyle, you really want setNonRendererRenderStyle : [Attachment 170697] Patch

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has granted Elliott Sprehn
<esprehn at chromium.org>'s request for review:
Bug 100397: Eliminate setRenderStyle, you really want setNonRendererRenderStyle
https://bugs.webkit.org/show_bug.cgi?id=100397

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

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=170697&action=review


Nice cleanup!

> Source/WebCore/ChangeLog:8
> +	   Introduce a new method setNonRendererRenderStyle that more
explicitly does

Bikeshed nit: setNonRendererStyle. It's less verbose and consistent with
setStyleInternal and setAnimatableStyle.

> Source/WebCore/ChangeLog:14
> +	   also hides the if statement protecting against null rendererers
meaning we end up

typo: rendererers

> Source/WebCore/ChangeLog:21
> +

You need to update the list below. Specifically, it's missing the removal of
setRenderStyle. I think that deserves a comment explaining why it's being
removed.

> Source/WebCore/ChangeLog:26
> +	   (WebCore::Element::pseudoStyleCacheIsInvalid):

Might be worth nothing that the only place this is called is in
Element::recalcStyle.

> Source/WebCore/dom/Element.cpp:1168
> +		   renderer->setAnimatableStyle(newStyle.get());

Technically, this is a one-line control clause and should not have curlies. May
as well fix it while you're in this code.