Bug 40629 - GraphicsLayer: Allow more layers to be opaque when applicable
Summary: GraphicsLayer: Allow more layers to be opaque when applicable
Status: RESOLVED DUPLICATE of bug 70634
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P3 Normal
Assignee: Nobody
Keywords: InRadar
: 40380 (view as bug list)
Depends on:
Blocks: 38744
  Show dependency treegraph
Reported: 2010-06-15 10:02 PDT by Noam Rosenthal
Modified: 2011-12-01 12:09 PST (History)
6 users (show)

See Also:

Patch (6.13 KB, patch)
2010-06-30 23:02 PDT, Sam Magnuson
no flags Details | Formatted Diff | Diff
Patch (6.02 KB, patch)
2010-06-30 23:15 PDT, Sam Magnuson
simon.fraser: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Noam Rosenthal 2010-06-15 10:02:50 PDT
This would include layers with a 100% solid background color.
Comment 1 Simon Fraser (smfr) 2010-06-15 10:31:12 PDT
Comment 2 Noam Rosenthal 2010-06-28 14:26:55 PDT
*** Bug 40380 has been marked as a duplicate of this bug. ***
Comment 3 Sam Magnuson 2010-06-30 23:02:54 PDT
Created attachment 60200 [details]
Comment 4 Early Warning System Bot 2010-06-30 23:09:12 PDT
Attachment 60200 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/3370100
Comment 5 Sam Magnuson 2010-06-30 23:15:59 PDT
Created attachment 60201 [details]
Comment 6 Noam Rosenthal 2010-07-01 08:39:01 PDT
The Qt-specific content LGTM!
Comment 7 Noam Rosenthal 2010-07-01 09:14:00 PDT
question - if we use contentsRect() for the texture instead of size(), can't we set the contents to opaque if the bgcolor is opaque, regardless of the layer size? 
I'm not really sure how contentsRect is supposed to work, but if it corresponds to the element size, this should be feasible and would also save us some texture-memory.
Comment 8 Noam Rosenthal 2010-07-04 20:50:33 PDT
ignore my last comment; contentsRect is only for directly composited image/media.
Comment 9 Simon Fraser (smfr) 2010-07-06 13:08:30 PDT
Comment on attachment 60201 [details]

> index d531bb5d0a0e29abdfe930ec7bf2166d3b55b422..fcee1962310652d679e6977f1b587b9581242dff 100644
> +        No new tests: this is an optimization.        

You can and should add tests that dump the layer tree (layoutTestController.layerTreeAsText()). You should also add a test or two to make sure that you don't hit this optimization when you don't intend to.

>  void GraphicsLayerQtImpl::paint(QPainter* painter, const QStyleOptionGraphicsItem* option, QWidget* widget)
>  {
> -    if (m_currentContent.backgroundColor.isValid())
> -        painter->fillRect(option->rect, QColor(m_currentContent.backgroundColor));
> +//     if (m_currentContent.backgroundColor.isValid())
> +//         painter->fillRect(option->rect, QColor(m_currentContent.backgroundColor));

Don't check in commented-out code.

> diff --git a/WebCore/rendering/RenderLayerBacking.cpp b/WebCore/rendering/RenderLayerBacking.cpp

> +    bool contentsOpaque = false;
> +    if (rendererHasBackground()) {
> +        const Color &bgColor = rendererBackgroundColor();

Color is sizeof(int), so no point in using a reference there.

> +        if (bgColor.alpha() == 255) {

Use !bgColor.hasAlpha().

> +            const IntRect elementBounds = m_owningLayer->localBoundingBox(),
> +                            layerBounds = compositedBounds();

We don't use multiple declarations like this. Also no need for the IntRects to be 'const'. Also elementBounds is a bad name; that's really layer bounds. So use layerBounds and compositedBounds or something.

> +            if (elementBounds.width() >= layerBounds.width() && elementBounds.height() >= layerBounds.height())
> +                contentsOpaque = true;

You should just test for rect equality. The composited bounds will never be smaller than m_owningLayer->localBoundingBox().

You should file a follow-up bug for other obvious enhancements (e.g. non-alpha background image).

r- for lack of tests.
Comment 10 Noam Rosenthal 2010-08-14 08:13:25 PDT
This one is still applicable even though it's with GraphicsLayerQt, though it doesn't give us a huge performance benefit.
Comment 11 Jesus Sanchez-Palencia 2011-12-01 12:08:28 PST
Noam, Igor, is this a duplicate of https://bugs.webkit.org/show_bug.cgi?id=70634 ?

Comment 12 Simon Fraser (smfr) 2011-12-01 12:09:28 PST

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