Bug 92492

Summary: [Texmap] Remove the backing store after 'style.visibility' for an element sets 'hidden'.
Product: WebKit Reporter: Dongseong Hwang <dongseong.hwang>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: noam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description Dongseong Hwang 2012-07-27 04:54:02 PDT
This patch's purpose is to save vram memory.
When visibility of the element sets hidden, we do not need to draw the element,
so we do not need to keep a texture of the backing store.
Currently, Texmap does not draw the element with visibility:hidden because
RenderLayerBacking::paintIntoLayer does not draw anything.
This patch just removes unused textures.
Comment 1 Dongseong Hwang 2012-07-27 05:03:38 PDT
Created attachment 154909 [details]
Patch
Comment 2 Dongseong Hwang 2012-07-27 05:04:53 PDT
(In reply to comment #1)
> Created an attachment (id=154909) [details]
> Patch
Comment 3 Dongseong Hwang 2012-07-27 05:08:10 PDT
Currently, chromium port handles m_contentsVisible like m_drawsContent.

void GraphicsLayerChromium::updateLayerIsDrawable()
{
    // For the rest of the accelerated compositor code, there is no reason to make a
    // distinction between drawsContent and contentsVisible. So, for m_layer, these two
    // flags are combined here. m_contentsLayer shouldn't receive the drawsContent flag
    // so it is only given contentsVisible.

    m_layer.setDrawsContent(m_drawsContent && m_contentsVisible);

    if (!m_contentsLayer.isNull())
        m_contentsLayer.setDrawsContent(m_contentsVisible);

    if (m_drawsContent)
        m_layer.invalidate();

    updateDebugIndicators();
}

Some web developers think visibility:hidden causes to save the texture memory.
Comment 4 Noam Rosenthal 2012-07-27 07:09:58 PDT
Comment on attachment 154909 [details]
Patch

Actually, something is missing here. We should clear the backing stores not just for the layer that becomes invisible, but also for all of its descendants.
Comment 5 Dongseong Hwang 2012-07-30 19:19:24 PDT
(In reply to comment #4)
> (From update of attachment 154909 [details])
> Actually, something is missing here. We should clear the backing stores not just for the layer that becomes invisible, but also for all of its descendants.

Thank you for your review.

I was concerned about what you said.

http://www.w3.org/TR/CSS21/visufx.html#propdef-visibility

The spec said
"hidden
The generated box is invisible (fully transparent, nothing is drawn), but still affects layout. Furthermore, descendants of the element will be visible if they have 'visibility: visible'."

If some descendants have 'visibility: visible', they need the backing stores.

RenderLayerBacking set contents visible of the graphics layer to false even if a graphics layer has visible compositing descendant layers.

void RenderLayerBacking::updateGraphicsLayerGeometry()
{
    ...
    // m_graphicsLayer is the corresponding GraphicsLayer for this RenderLayer and its non-compositing
    // descendants. So, the visibility flag for m_graphicsLayer should be true if there are any
    // non-compositing visible layers.
    m_graphicsLayer->setContentsVisible(m_owningLayer->hasVisibleContent() || hasVisibleNonCompositingDescendantLayers());
    ...
}

So, I removed the backing stores for only the layer that becomes invisible.

I think I may misunderstand, so I need your feedback.
Comment 6 Noam Rosenthal 2012-07-30 20:29:53 PDT
Comment on attachment 154909 [details]
Patch

Previous r- was based on a misunderstanding of the spec :)
Comment 7 Dongseong Hwang 2012-07-30 20:48:14 PDT
Comment on attachment 154909 [details]
Patch

I forgot to set commit-queue to ?. Could you change +? :)
Comment 8 WebKit Review Bot 2012-07-30 23:56:12 PDT
Comment on attachment 154909 [details]
Patch

Clearing flags on attachment: 154909

Committed r124178: <http://trac.webkit.org/changeset/124178>
Comment 9 WebKit Review Bot 2012-07-30 23:56:16 PDT
All reviewed patches have been landed.  Closing bug.