[webkit-reviews] review granted: [Bug 38829] The HTML5 video element in Safari does not respect "visibility:hidden" CSS property : [Attachment 112757] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 38829: The HTML5 video element in Safari does not respect
"visibility:hidden" CSS property
https://bugs.webkit.org/show_bug.cgi?id=38829

Attachment 112757: Patch
https://bugs.webkit.org/attachment.cgi?id=112757&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=112757&action=review


I'd prefer it if we could manage the logic for transitioning between
contentsVisible=false -> contentsVisible=true in shared code instead of making
each GraphicsLayer implementation deal with it, since we already have API for
setContentsNeedsDisplay() and it seems that every implementation will end up
doing effectively that on that transition.  Otherwise this looks good to me.

R=me although I can't vouch for all of the GraphicsLayerCA.cpp changes.

> Source/WebCore/ChangeLog:8
> +	   Make compositiong and CSS visibility play nicely together.

compositiong -> compositing

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:968
> +    if (m_uncommittedChanges & ContentsVisibilityChanged)
> +	   updateContentsVisibility();

this is y'alls code but the order in which things are checked here is strange
to me.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1207
> +	   if (LayerMap* layerCloneMap = m_layerClones.get()) {
> +	       LayerMap::const_iterator end = layerCloneMap->end();
> +	       for (LayerMap::const_iterator it = layerCloneMap->begin(); it !=
end; ++it)
> +		   it->second->setContents(0);
> +	   }

I have to admit I have no idea what this is, and maybe someone more familiar
with GraphicsLayerCA should check it over.

> Source/WebCore/platform/graphics/ca/GraphicsLayerCA.h:342
> +	   ContentsVisibilityChanged = 1 << 24

nit: can you add a trailing comma here so the next patch doesn't cause a diff
to two lines?

> Source/WebCore/rendering/RenderLayer.cpp:4047
>      // Overflow layers are just painted by their enclosing layers, so they
don't get put in zorder lists.
> -    if ((m_hasVisibleContent || (m_hasVisibleDescendant &&
isStackingContext())) && !isNormalFlowOnly() &&
!renderer()->isRenderFlowThread()) {
> +    bool includeHiddenLayer = includeHiddenLayers || (m_hasVisibleContent ||
(m_hasVisibleDescendant && isStackingContext()));
> +    if (includeHiddenLayer && !isNormalFlowOnly() &&
!renderer()->isRenderFlowThread()) {

The comment does not seem to match the code very well any more. Could you
update it?

> LayoutTests/compositing/visibility/visibility-image-layers-dynamic.html:13
> +/*	   background-color: blue;*/

Did you mean to check this in commented out?

> LayoutTests/compositing/visibility/visibility-image-layers.html:13
> +/*	   background-color: blue;*/

same here

> LayoutTests/compositing/visibility/visibility-image-layers.html:38
> +    .composited {
> +	 -webkit-transform: translateZ(1px);

a lot of things like this are useful in pretty much every test in here, is it
worth making a stylesheet with utility classes like this to use by reference
instead of copy-paste all over the place?