[webkit-reviews] review granted: [Bug 22938] [Transforms] WebView background is not painted correctly if the document has a transform : [Attachment 26158] Fixed patch - restore the !view() check

Fri Dec 19 14:31:48 PST 2008

Darin Adler <darin at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 22938: [Transforms] WebView background is not painted correctly if the
document has a transform
https://bugs.webkit.org/show_bug.cgi?id=22938

Attachment 26158: Fixed patch - restore the !view() check
https://bugs.webkit.org/attachment.cgi?id=26158&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    // If painting will entirely fill the view, no need to fill the
background

Comments in sentence style should end in a period.

> +    if (elt ||
> +	   (firstChild() && (firstChild()->style()->visibility() == VISIBLE &&
!firstChild()->style()->hasTransform())) ||
> +	   !view())
>	   return;

You have an extra set of parentheses in there that I don't think add clarity.
You don't need the ones after "firstChild() &&".

Our typical format for long if statements like this is to start lines with
operators, and also (maybe not a good format) to indent an extra level so the
expression doesn't line up with the return statement.

	if (elt
		|| (firstChild() && firstChild()->style()->visibility() ==
VISIBLE && !firstChild()->style()->hasTransform())
		|| !view())
	    return;

But I think the best way to clean this up is with a helper function.

    static inline bool isVisibleWithNoTransform(RenderObject* object)
    {
	return object && object->style()->visibility() == VISIBLE && ! object
>style()->hasTransform();
    }

    [...]
	if (elt || isVisibleWithNoTransform(firstChild()) || !view())
	    return;
    [...]

I think it's worth it to keep this code readable.

> -    // This code typically only executes if the root element's visibility
has been set to hidden.
> +    // This code typically only executes if the root element's visibility
has been set to hidden,
> +    // or there is a transform on the <html>

Comments in sentence style should end in a period.

r=me -- please consider my refinement suggestions.