[webkit-reviews] review denied: [Bug 94131] Avoid repeated calls to decorationColor on RenderObject::getTextDecorationColors : [Attachment 158625] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has denied Bruno Abinader - out until
02.12.2012 <bruno.abinader at basyskom.com>'s request for review:
Bug 94131: Avoid repeated calls to decorationColor on
RenderObject::getTextDecorationColors
https://bugs.webkit.org/show_bug.cgi?id=94131

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

------- Additional Comments from Brent Fulgham <bfulgham at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=158625&action=review


While I like the goal of your change, I think the current form of your patch
introduces a potential crash.

I think you should stay more like the original code in layout, but incorporate
the constant lifting you correctly identified.

I'm curious if you did any profiling that identified this code as a bottleneck,
or if you ran across this from code inspection?

> Source/WebCore/rendering/RenderObject.cpp:2616
> +    Color resultColor = decorationColor(styleToUse);

I think you should just declare these variables here, and assign in the top of
the do loop, as was done previously. This avoids a potential null dereference
later.

> Source/WebCore/rendering/RenderObject.cpp:-2617
> -	   int currDecs = styleToUse->textDecoration();

Move assignments here (but keep the change from int to ETextDecoration)

> Source/WebCore/rendering/RenderObject.cpp:-2636
> -	       curr = toRenderBlock(curr)->continuation();

I think you should keep this stanza as-is, rather than the new block below.

> Source/WebCore/rendering/RenderObject.cpp:2637
> +	       styleToUse = curr->style(firstlineStyle);

You might have just reassigned curr from the call to continuation in the line
above. You should null-check again to avoid faulting here.

I think a better flow would be to follow the original logic as outlined in the
earlier comments I made.