Bug 40380

Summary: [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.
Product: WebKit Reporter: Sam Magnuson <smagnuso>
Component: WebKit QtAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: commit-queue, kenneth, noam, simon.fraser
Priority: P2 Keywords: Qt, QtTriaged
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 38744    
Description Flags
Adding support for setBackgroundColor/clearBackground color and using it in QGraphicsLayer to pre-fill the pixmap cache.
Proposed patch with proper coding style
kenneth: review-
Rediff against trunk
Patch simon.fraser: review-

Description Sam Magnuson 2010-06-09 11:47:49 PDT
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.
Comment 1 Sam Magnuson 2010-06-09 12:38:05 PDT
Created attachment 58276 [details]
Adding support for setBackgroundColor/clearBackground color and using it in QGraphicsLayer to pre-fill the pixmap cache.
Comment 2 Noam Rosenthal 2010-06-10 17:07:11 PDT
This also means that we have to recache when the background-color changes; I think that's missing from the patch :)
Comment 3 Sam Magnuson 2010-06-11 13:58:16 PDT
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.
Comment 4 Sam Magnuson 2010-06-12 10:23:22 PDT
Created attachment 58558 [details]
Proposed patch with proper coding style
Comment 5 Kenneth Rohde Christiansen 2010-06-13 07:40:25 PDT
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.
Comment 6 Sam Magnuson 2010-06-22 16:10:29 PDT
Created attachment 59434 [details]
Rediff against trunk
Comment 7 Kenneth Rohde Christiansen 2010-06-22 18:11:31 PDT
So did you remove the style update as I requested? and put it into another patch?
Comment 8 Sam Magnuson 2010-06-22 23:33:48 PDT
(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.
Comment 9 WebKit Commit Bot 2010-06-25 10:01:27 PDT
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:


    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
Comment 10 Sam Magnuson 2010-06-25 10:11:54 PDT
Created attachment 59777 [details]
Comment 11 Adam Barth 2010-06-25 11:15:53 PDT
Comment on attachment 59777 [details]

Forwarding previous R+.
Comment 12 WebKit Commit Bot 2010-06-25 23:14:18 PDT
Comment on attachment 59777 [details]

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
Comment 13 Sam Magnuson 2010-06-28 13:33:41 PDT
Created attachment 59932 [details]
Comment 14 Kenneth Rohde Christiansen 2010-06-28 13:38:18 PDT
Comment on attachment 59932 [details]

 +          that the pixemap has no alpha.
pixmap :-)

Looks fine to me. No'am?
Comment 15 Sam Magnuson 2010-06-28 13:41:52 PDT
Created attachment 59933 [details]
Comment 16 Simon Fraser (smfr) 2010-06-28 13:49:12 PDT
Comment on attachment 59933 [details]

> +    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).
Comment 17 Simon Fraser (smfr) 2010-06-28 14:15:26 PDT
I think you're really trying to fix bug 40629.
Comment 18 Noam Rosenthal 2010-06-28 14:26:55 PDT

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