Bug 22938 - [Transforms] WebView background is not painted correctly if the document has a transform
Summary: [Transforms] WebView background is not painted correctly if the document has ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-12-19 13:09 PST by Simon Fraser (smfr)
Modified: 2008-12-19 14:44 PST (History)
1 user (show)

See Also:


Attachments
Testcase (1.10 KB, text/html)
2008-12-19 13:10 PST, Simon Fraser (smfr)
no flags Details
Patch, testcase, changelog (5.10 KB, patch)
2008-12-19 13:49 PST, Simon Fraser (smfr)
darin: review-
Details | Formatted Diff | Diff
Fixed patch - restore the !view() check (5.09 KB, patch)
2008-12-19 14:22 PST, Simon Fraser (smfr)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2008-12-19 13:09:19 PST
If the document has a transform, via something like:

    html {
      -webkit-transform: translateY(100px) rotate(20deg);
    }

then the exposed area of the viewport that is not covered by the document is unpainted, and full of pixel crap.
Comment 1 Simon Fraser (smfr) 2008-12-19 13:10:08 PST
Created attachment 26152 [details]
Testcase
Comment 2 Simon Fraser (smfr) 2008-12-19 13:49:48 PST
Created attachment 26156 [details]
Patch, testcase, changelog
Comment 3 Darin Adler 2008-12-19 14:03:34 PST
Comment on attachment 26156 [details]
Patch, testcase, changelog

Where did the null check of view() go? Removing that could lead to crashes. If it was redundant, then it's OK to remove it, but the change log should explain that.
Comment 4 Simon Fraser (smfr) 2008-12-19 14:16:26 PST
Oops that was a mistake. New patch coming.
Comment 5 Simon Fraser (smfr) 2008-12-19 14:22:02 PST
Created attachment 26158 [details]
Fixed patch - restore the !view() check
Comment 6 Darin Adler 2008-12-19 14:31:48 PST
Comment on attachment 26158 [details]
Fixed patch - restore the !view() check

> +    // 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.
Comment 7 Simon Fraser (smfr) 2008-12-19 14:44:27 PST
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	A	LayoutTests/fast/transforms/transformed-document-element.html
	A	LayoutTests/platform/mac/fast/transforms/transformed-document-element-expected.checksum
	A	LayoutTests/platform/mac/fast/transforms/transformed-document-element-expected.png
	A	LayoutTests/platform/mac/fast/transforms/transformed-document-element-expected.txt
	M	WebCore/ChangeLog
	M	WebCore/rendering/RenderView.cpp
Committed r39413