Bug 40380 – [Qt] When a layer is really opaque (ie has a background color that has 255 alpha) we should fill the layer with that color to tell Qt that the pixemap has no alpha.

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: WebKit Qt

[Sam Magnuson]

Blt'ing a non-alpha pixmap is going to be much faster and can avoid a lot of tests for alpha. In my specific case my hardware doesn't do accelerated alpha scales which means scaling animations frame-rate drops considerably without being able to tell QGraphicsLayer not to have an alpha.

[Sam Magnuson]

Created attachment 58276 [details]
Adding support for setBackgroundColor/clearBackground color and using it in QGraphicsLayer to pre-fill the pixmap cache.

[Noam Rosenthal]

This also means that we have to recache when the background-color changes; I think that's missing from the patch :)

[Sam Magnuson]

I agree with you in principal - I can certainly toss out the cache if you'd like. This sort of goes back to our discussion that I get setNeedsDisplay later anyway (just as we do for setContentsToImage, where we don't toss out the cache and we just get a setNeedsDisplay later). 

Do you want me to go ahead and do it for this case, or should I look into that bug as a whole class of bugs later?

As I mentioned to you before I'd like to fix the setNeedsDisplay that we get, since when some things change (like zorder, opacity, etc) we don't need a setNeedsDisplay from WebKit as it causes us to throw away a perfectly valid cached pixmap.

[Sam Magnuson]

Created attachment 58558 [details]
Proposed patch with proper coding style

[Kenneth Rohde Christiansen]

Comment on attachment 58558 [details]
Proposed patch with proper coding style

Can't we get a test case for this?

r- due to missing test case and because you should do the style clean up in a separate patch.

[Sam Magnuson]

Created attachment 59434 [details]
Rediff against trunk

[Kenneth Rohde Christiansen]

So did you remove the style update as I requested? and put it into another patch?

[Sam Magnuson]

(In reply to comment #7)
> So did you remove the style update as I requested? and put it into another patch?

I had to redo the patches since I had done them against the qtwebkit2.0 branch accidentally. In so doing many of the style issues had since been fixed on trunk - in this case the style issues I had fixed were gone. In another couple of patches I remove all extraneous style fixes and I will submit a separate bug against those.

[WebKit Commit Bot]

Comment on attachment 59434 [details]
Rediff against trunk

Rejecting patch 59434 from commit-queue.

Failed to run "[u'git', u'svn', u'dcommit']" exit_code: 1
Last 500 characters of output:
runk ...
	M	WebCore/ChangeLog
	M	WebCore/platform/graphics/qt/GraphicsLayerQt.cpp
	M	WebCore/rendering/RenderLayerBacking.cpp
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following ChangeLog files contain OOPS:

        trunk/WebCore/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.
 at /usr/local/git/libexec/git-core/git-svn line 570


Full output: http://webkit-commit-queue.appspot.com/results/3277789

[Sam Magnuson]

Created attachment 59777 [details]
Patch

[Adam Barth]

Comment on attachment 59777 [details]
Patch

Forwarding previous R+.

[WebKit Commit Bot]

Comment on attachment 59777 [details]
Patch

Rejecting patch 59777 from commit-queue.

Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Adam Barth', u'--force']" exit_code: 1
Parsed 3 diffs from patch file(s).
patching file WebCore/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file WebCore/platform/graphics/qt/GraphicsLayerQt.cpp
Hunk #1 FAILED at 345.
Hunk #2 FAILED at 356.
Hunk #3 FAILED at 364.
Hunk #4 succeeded at 566 (offset 80 lines).
Hunk #5 succeeded at 578 (offset 81 lines).
3 out of 5 hunks FAILED -- saving rejects to file WebCore/platform/graphics/qt/GraphicsLayerQt.cpp.rej
patching file WebCore/rendering/RenderLayerBacking.cpp

Full output: http://webkit-commit-queue.appspot.com/results/3333773

[Sam Magnuson]

Created attachment 59932 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 59932 [details]
Patch

WebCore/ChangeLog:7
 +          that the pixemap has no alpha.
pixmap :-)

Looks fine to me. No'am?

[Sam Magnuson]

Created attachment 59933 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 59933 [details]
Patch

> +    if (rendererHasBackground()) {
> +        const Color &bgColor = rendererBackgroundColor();
> +        m_graphicsLayer->setBackgroundColor(bgColor);
> +    } else
> +        m_graphicsLayer->clearBackgroundColor();

You can't do this. GraphicsLayers are not always the same size as their elements; in many cases, they are larger (e.g. try one with a box-shadow, or a abs. pos. child).

[Simon Fraser (smfr)]

I think you're really trying to fix bug 40629.

[Noam Rosenthal]

*** This bug has been marked as a duplicate of bug 40629 ***