Bug 69796 - Support creating composited layers for fixed position elements
Summary: Support creating composited layers for fixed position elements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Vangelis Kokkevis
URL:
Keywords:
Depends on: 70067
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-10 15:55 PDT by Vangelis Kokkevis
Modified: 2011-12-19 11:13 PST (History)
10 users (show)

See Also:


Attachments
Patch (21.55 KB, patch)
2011-10-10 16:03 PDT, Vangelis Kokkevis
no flags Details | Formatted Diff | Diff
Patch (23.77 KB, patch)
2011-10-11 16:29 PDT, Vangelis Kokkevis
no flags Details | Formatted Diff | Diff
Patch (13.98 KB, patch)
2011-10-20 17:15 PDT, Vangelis Kokkevis
no flags Details | Formatted Diff | Diff
Patch (16.24 KB, patch)
2011-10-21 15:38 PDT, Vangelis Kokkevis
no flags Details | Formatted Diff | Diff
Patch (16.36 KB, patch)
2011-10-26 16:32 PDT, Vangelis Kokkevis
no flags Details | Formatted Diff | Diff
Patch (14.12 KB, patch)
2011-10-26 17:59 PDT, Vangelis Kokkevis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vangelis Kokkevis 2011-10-10 15:55:19 PDT
Add an optional setting for promoting fixed position elements to become composited if they are inside a scrolling frame.
Comment 1 Vangelis Kokkevis 2011-10-10 16:03:30 PDT
Created attachment 110427 [details]
Patch
Comment 2 Vangelis Kokkevis 2011-10-10 16:07:16 PDT
Comment on attachment 110427 [details]
Patch

Waiting on ews results to pickup missing export symbols. I will post a new patch once I have them.
Comment 3 Simon Fraser (smfr) 2011-10-10 16:15:16 PDT
You'll break z-index and clipping if you just make position:fixed do compositing.
Comment 4 Adrienne Walker 2011-10-10 16:27:40 PDT
(In reply to comment #3)
> You'll break z-index and clipping if you just make position:fixed do compositing.

Why would the path in this patch be any different than a fixed position element that became composited via some other trigger?
Comment 5 Simon Fraser (smfr) 2011-10-10 16:30:34 PDT
Because other triggers like -webkit-transform create stacking context. position:fixed does not.
Comment 6 Gustavo Noronha (kov) 2011-10-10 18:25:33 PDT
Comment on attachment 110427 [details]
Patch

Attachment 110427 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10031026
Comment 7 Vangelis Kokkevis 2011-10-11 10:13:20 PDT
(In reply to comment #3)
> You'll break z-index and clipping if you just make position:fixed do compositing.

Good catch!  Thanks, Simon. I'll update the patch and only make fixed position elements composited if they create a stacking context (e.g. have a z-index).  That still covers a number of useful cases.
Comment 8 Simon Fraser (smfr) 2011-10-11 12:03:33 PDT
Or you can have position:fixed create stacking context, although that does break the web slightly.

I have a patch already for position:fixed and compositing. I'll put it up here.
Comment 9 Simon Fraser (smfr) 2011-10-11 12:20:43 PDT
Actually I'll just wait until you've made another patch and make comments. I don't have any additional code to add.
Comment 10 James Robinson 2011-10-11 12:33:32 PDT
I think there are 3 interesting cases:

1.) position:fixed element is a stacking context for other reasons
2.) position:fixed element is not a stacking context, and treating is as a stacking context would change the rendering
3.) position:fixed element is not a stacking context but the page would render identically is if were a stacking context.

I think (3) is actually the most common on the web - how hard would it be to detect?
Comment 11 Simon Fraser (smfr) 2011-10-11 12:40:14 PDT
(In reply to comment #10)
> I think there are 3 interesting cases:
> 
> 1.) position:fixed element is a stacking context for other reasons
> 2.) position:fixed element is not a stacking context, and treating is as a stacking context would change the rendering
> 3.) position:fixed element is not a stacking context but the page would render identically is if were a stacking context.
> 
> I think (3) is actually the most common on the web - how hard would it be to detect?

I would like to do 3) for other things too (like overflow), but haven't figured out how hard it would be yet. It should be doable by analyzing the structure of the z-order tree.
Comment 12 James Robinson 2011-10-11 12:51:23 PDT
I think (and double-checked with Tab) just checking for descendants of the position:fixed element that have a z-index set should be good enough to start - if there are no descendants with a z-index set, it's definitely fine to treat the element as if it established a stacking context.  If descendants do have a z-index set, it's still fine so long as the z-index values are higher than anything else in the page. We also don't have to worry about descendants of elements that establish stacking contexts of their own that are descendants of the position:fixed element.

How about we just keep a bit on RenderLayer tracking whether descendants inside this stacking context have z-index set? Should catch most cases.
Comment 13 Simon Fraser (smfr) 2011-10-11 13:00:47 PDT
(In reply to comment #12)
> I think (and double-checked with Tab) just checking for descendants of the position:fixed element that have a z-index set should be good enough to start - if there are no descendants with a z-index set, it's definitely fine to treat the element as if it established a stacking context.  If descendants do have a z-index set, it's still fine so long as the z-index values are higher than anything else in the page. We also don't have to worry about descendants of elements that establish stacking contexts of their own that are descendants of the position:fixed element.

Don't forget that opacity and transform also create stacking context, so I think it's more complex than this.

> How about we just keep a bit on RenderLayer tracking whether descendants inside this stacking context have z-index set? Should catch most cases.

You're really asking whether it 'renders atomically' i think. This is hard to know, since positioned descendent are hoisted up into the z-order lists of the enclosing stacking context. You have to figure out whether the fixed thing, and its RenderLayer descendants are all adjacent siblings in that z-order list.
Comment 14 Vangelis Kokkevis 2011-10-11 16:29:09 PDT
Created attachment 110600 [details]
Patch
Comment 15 Vangelis Kokkevis 2011-10-11 16:32:30 PDT
Updated patch creates compositing layers only if the fixed position element has its own stacking context. From a quick survey of common pages, it appears that position:fixed is usually accompanied by a z-index which does force the creation of a new stacking context.
Comment 16 Simon Fraser (smfr) 2011-10-11 16:36:27 PDT
Comment on attachment 110600 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110600&action=review

> Source/WebCore/ChangeLog:14
> +        This CL adds the necessary WebCore setting and modifies the RLC logic to take into

"CL"?

> Source/WebCore/platform/graphics/GraphicsLayer.h:275
> +    bool fixedPositioned() const { return m_fixedPositioned; }
> +    virtual void setFixedPositioned(bool b) { m_fixedPositioned = b; }
> +
> +    // Set to true when the layer corresponds to the root layer of a Frame.
> +    bool isFrameRoot() const { return m_isFrameRoot; }
> +    virtual void setIsFrameRoot(bool b) { m_isFrameRoot = b; }

I don't think these belong on GraphicsLayer. It shouldn't really know anything about position or frames.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1499
> +bool RenderLayerCompositor::requiresCompositingForFixedPosition(RenderObject* renderer) const

I'd call this requiresCompositingForPosition()

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1502
> +    if (!(renderer->isPositioned() && renderer->style()->position() == FixedPosition && renderer->enclosingLayer()->isStackingContext()))
> +        return false;

The enclosingLayer()->isStackingContext() test is too mysterious without a detailed comment. I'm not sure it's right. Also, the tests should exercise more combinations of fixed with stacking, as discussed in the bug.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1515
> +    // Fixed position elements get composited only when inside a frame that scrolls.
> +    // If we can't reliably know the size of the enclosing frame, don't change compositing state.
> +    m_compositingDependsOnGeometry = true;
> +    if (m_renderView->needsLayout())
> +        return m_renderView->hasLayer() && m_renderView->layer()->isComposited();
> +
> +    ScrollView* scrollView = m_renderView->frameView();
> +    return scrollView->verticalScrollbar() || scrollView->horizontalScrollbar();

I think there's too much Chrome-specific behavior baked in here. For example, similar behavior might not make any sense on mobile platforms.
Comment 17 Vangelis Kokkevis 2011-10-11 18:12:32 PDT
Thanks for the speedy review!

(In reply to comment #16)
> (From update of attachment 110600 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110600&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        This CL adds the necessary WebCore setting and modifies the RLC logic to take into
> 
> "CL"?

CL = changelist  ... I'll remove :)
> 
> > Source/WebCore/platform/graphics/GraphicsLayer.h:275
> > +    bool fixedPositioned() const { return m_fixedPositioned; }
> > +    virtual void setFixedPositioned(bool b) { m_fixedPositioned = b; }
> > +
> > +    // Set to true when the layer corresponds to the root layer of a Frame.
> > +    bool isFrameRoot() const { return m_isFrameRoot; }
> > +    virtual void setIsFrameRoot(bool b) { m_isFrameRoot = b; }
> 
> I don't think these belong on GraphicsLayer. It shouldn't really know anything about position or frames.
> 

To give some context about this change, the end goal of this effort is to give the compositor the ability to scroll a page/iframe without having to rely on webkit to adjust fixed position elements.  This in similar vain to being able to run animations independent of the main webkit thread.

To do that, we need some meta-information about how a layer positions itself to be stored in the GraphicsLayer. 

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1499
> > +bool RenderLayerCompositor::requiresCompositingForFixedPosition(RenderObject* renderer) const
> 
> I'd call this requiresCompositingForPosition()
> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1502
> > +    if (!(renderer->isPositioned() && renderer->style()->position() == FixedPosition && renderer->enclosingLayer()->isStackingContext()))
> > +        return false;
> 
> The enclosingLayer()->isStackingContext() test is too mysterious without a detailed comment. I'm not sure it's right. 

RenderLayer::isStackingContext() returns true for layers with an explicit z-index. In CSSStyleSelector.cpp the z-index is set explicitly to 0 if the element has a transform property, mask, reflection, or opacity < 1.  It looks like it covers all the bases.
 

> Also, the tests should exercise more combinations of fixed with stacking, as discussed in the bug.

Sure, will do.

> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1515
> > +    // Fixed position elements get composited only when inside a frame that scrolls.
> > +    // If we can't reliably know the size of the enclosing frame, don't change compositing state.
> > +    m_compositingDependsOnGeometry = true;
> > +    if (m_renderView->needsLayout())
> > +        return m_renderView->hasLayer() && m_renderView->layer()->isComposited();
> > +
> > +    ScrollView* scrollView = m_renderView->frameView();
> > +    return scrollView->verticalScrollbar() || scrollView->horizontalScrollbar();
> 
> I think there's too much Chrome-specific behavior baked in here. For example, similar behavior might not make any sense on mobile platforms.

Is the issue that I'm using the presence of scroll bars in the ScrollView to determine whether the layer is scrolling?  I'll change it to pick it up from the RenderLayer directly.
Comment 18 Simon Fraser (smfr) 2011-10-11 18:24:51 PDT
(In reply to comment #17)
> Thanks for the speedy review!
> 
> (In reply to comment #16)
> > (From update of attachment 110600 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=110600&action=review
> > 
> > > Source/WebCore/ChangeLog:14
> > > +        This CL adds the necessary WebCore setting and modifies the RLC logic to take into
> > 
> > "CL"?
> 
> CL = changelist  ... I'll remove :)
> > 
> > > Source/WebCore/platform/graphics/GraphicsLayer.h:275
> > > +    bool fixedPositioned() const { return m_fixedPositioned; }
> > > +    virtual void setFixedPositioned(bool b) { m_fixedPositioned = b; }
> > > +
> > > +    // Set to true when the layer corresponds to the root layer of a Frame.
> > > +    bool isFrameRoot() const { return m_isFrameRoot; }
> > > +    virtual void setIsFrameRoot(bool b) { m_isFrameRoot = b; }
> > 
> > I don't think these belong on GraphicsLayer. It shouldn't really know anything about position or frames.
> > 
> 
> To give some context about this change, the end goal of this effort is to give the compositor the ability to scroll a page/iframe without having to rely on webkit to adjust fixed position elements.  This in similar vain to being able to run animations independent of the main webkit thread.
> 
> To do that, we need some meta-information about how a layer positions itself to be stored in the GraphicsLayer. 

I understand, but I think you should store this information outside of GraphicsLayer, or phrase it in such a way that it's layout-agnostic.

iOS5 now supports compositing for position:fixed, but it doesn't need to touch GraphicsLayer at all for this.

> > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1499
> > > +bool RenderLayerCompositor::requiresCompositingForFixedPosition(RenderObject* renderer) const
> > 
> > I'd call this requiresCompositingForPosition()
> > 
> > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1502
> > > +    if (!(renderer->isPositioned() && renderer->style()->position() == FixedPosition && renderer->enclosingLayer()->isStackingContext()))
> > > +        return false;
> > 
> > The enclosingLayer()->isStackingContext() test is too mysterious without a detailed comment. I'm not sure it's right. 
> 
> RenderLayer::isStackingContext() returns true for layers with an explicit z-index. In CSSStyleSelector.cpp the z-index is set explicitly to 0 if the element has a transform property, mask, reflection, or opacity < 1.  It looks like it covers all the bases.

The enclosingLayer is confusing. You want renderer->layer() (which it will have, since it's positioned).

So I guess you'll only do fast scrolling if all the fixed things on the page happen to also be stacking contexts? My guess is that won't cover the majority of pages with fixed.

> > Also, the tests should exercise more combinations of fixed with stacking, as discussed in the bug.
> 
> Sure, will do.
> 
> > 
> > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1515
> > > +    // Fixed position elements get composited only when inside a frame that scrolls.
> > > +    // If we can't reliably know the size of the enclosing frame, don't change compositing state.
> > > +    m_compositingDependsOnGeometry = true;
> > > +    if (m_renderView->needsLayout())
> > > +        return m_renderView->hasLayer() && m_renderView->layer()->isComposited();
> > > +
> > > +    ScrollView* scrollView = m_renderView->frameView();
> > > +    return scrollView->verticalScrollbar() || scrollView->horizontalScrollbar();
> > 
> > I think there's too much Chrome-specific behavior baked in here. For example, similar behavior might not make any sense on mobile platforms.
> 
> Is the issue that I'm using the presence of scroll bars in the ScrollView to determine whether the layer is scrolling?  I'll change it to pick it up from the RenderLayer directly.

I think the decision about whether fixedpos causes compositing needs to be pushed into client code somehow.
Comment 19 Vangelis Kokkevis 2011-10-11 23:43:01 PDT
> > > I don't think these belong on GraphicsLayer. It shouldn't really know anything about position or frames.
> > > 
> > 
> > To give some context about this change, the end goal of this effort is to give the compositor the ability to scroll a page/iframe without having to rely on webkit to adjust fixed position elements.  This in similar vain to being able to run animations independent of the main webkit thread.
> > 
> > To do that, we need some meta-information about how a layer positions itself to be stored in the GraphicsLayer. 
> 
> I understand, but I think you should store this information outside of GraphicsLayer, or phrase it in such a way that it's layout-agnostic.
> 
> iOS5 now supports compositing for position:fixed, but it doesn't need to touch GraphicsLayer at all for this.
>

Any chance that code could be shared? :)

I'll see if I can tie this somehow to the existing Animation hooks of GraphicsLayer.

 
> > > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1499
> > > > +bool RenderLayerCompositor::requiresCompositingForFixedPosition(RenderObject* renderer) const
> > > 
> > > I'd call this requiresCompositingForPosition()
> > > 
> > > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1502
> > > > +    if (!(renderer->isPositioned() && renderer->style()->position() == FixedPosition && renderer->enclosingLayer()->isStackingContext()))
> > > > +        return false;
> > > 
> > > The enclosingLayer()->isStackingContext() test is too mysterious without a detailed comment. I'm not sure it's right. 
> > 
> > RenderLayer::isStackingContext() returns true for layers with an explicit z-index. In CSSStyleSelector.cpp the z-index is set explicitly to 0 if the element has a transform property, mask, reflection, or opacity < 1.  It looks like it covers all the bases.
> 
> The enclosingLayer is confusing. You want renderer->layer() (which it will have, since it's positioned).
> 

Ah, got it!

> So I guess you'll only do fast scrolling if all the fixed things on the page happen to also be stacking contexts? My guess is that won't cover the majority of pages with fixed.
> 

That's true, at least until we figure out how to lift the stacking context restriction.

> > > Also, the tests should exercise more combinations of fixed with stacking, as discussed in the bug.
> > 
> > Sure, will do.
> > 
> > > 
> > > > Source/WebCore/rendering/RenderLayerCompositor.cpp:1515
> > > > +    // Fixed position elements get composited only when inside a frame that scrolls.
> > > > +    // If we can't reliably know the size of the enclosing frame, don't change compositing state.
> > > > +    m_compositingDependsOnGeometry = true;
> > > > +    if (m_renderView->needsLayout())
> > > > +        return m_renderView->hasLayer() && m_renderView->layer()->isComposited();
> > > > +
> > > > +    ScrollView* scrollView = m_renderView->frameView();
> > > > +    return scrollView->verticalScrollbar() || scrollView->horizontalScrollbar();
> > > 
> > > I think there's too much Chrome-specific behavior baked in here. For example, similar behavior might not make any sense on mobile platforms.
> > 
> > Is the issue that I'm using the presence of scroll bars in the ScrollView to determine whether the layer is scrolling?  I'll change it to pick it up from the RenderLayer directly.
> 
> I think the decision about whether fixedpos causes compositing needs to be pushed into client code somehow.

Is it because the iOS implementation for the same effect is different and somehow conflicting with this one?  I did not intend to make this code Chrome-specific.
Comment 20 Vangelis Kokkevis 2011-10-13 16:46:06 PDT
I decided to split this into a few patches to separate out the issues raised by Simon's review:

Patch #1: https://bugs.webkit.org/show_bug.cgi?id=70067 : Add the necessary hooks for client code to decide whether to create a compositing layer.
Patch #2: Once #1 lands, I will move the fixed position layer creation logic into Chrome's implementation of ChromeClient.  This should only touch Chrome-specific code.
Patch #3: Deal with any necessary additions to the GraphicsLayer interface.
Comment 21 Vangelis Kokkevis 2011-10-19 18:01:51 PDT
Is there a plan for the recently released iOS5 WebKit code that deals with fixed position elements to be merged into trunk? It would be nice to avoid having each platform doing its own implementation.
Comment 22 Simon Fraser (smfr) 2011-10-19 19:08:27 PDT
Sorry for my lack of input here. Other stuff getting in the way.

There is very little iOS code that is sharable; mostly just requiresCompositiongForPosition(), and some ChromeClient callbacks used to register/unregister fixed position layers with platform code.

I'm not sure where the code should live that looks to see if the view is scrollable. Maybe it can be in RLC, and platforms which don't want it can #ifdef it out.

There is another wrinkle this patch doesn't deal with, which is that position:fixed inside a transform doesn't behave fixed.
Comment 23 Vangelis Kokkevis 2011-10-20 17:15:46 PDT
Created attachment 111876 [details]
Patch
Comment 24 Vangelis Kokkevis 2011-10-20 17:19:14 PDT
New patch uploaded. This one only deals with creating compositing layers for position:fixed elements. It doesn't modify GraphicsLayer or require elements to be in a scrolling frame to become composited.
Comment 25 Simon Fraser (smfr) 2011-10-20 17:27:12 PDT
Comment on attachment 111876 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111876&action=review

You need to check that the

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1504
> +    // position:fixed elements that create their own stacking context (e.g. have an explicit z-index,
> +    // opacity, transform) can get their own composited layer.
> +    if (!(renderer->isPositioned() && renderer->style()->position() == FixedPosition && layer->isStackingContext()))
> +        return false;

This will incorrectly return true for a position:fixed element inside a transformed one. You should also consider nested position:fixed too. Both need test cases.

You want to check that the container() of the renderer is the RenderView, but beware; this method may be called before the renderers have been hooked up.
Comment 26 Vangelis Kokkevis 2011-10-20 18:32:27 PDT
(In reply to comment #25)
> (From update of attachment 111876 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111876&action=review
> 
> You need to check that the
> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1504
> > +    // position:fixed elements that create their own stacking context (e.g. have an explicit z-index,
> > +    // opacity, transform) can get their own composited layer.
> > +    if (!(renderer->isPositioned() && renderer->style()->position() == FixedPosition && layer->isStackingContext()))
> > +        return false;
> 
> This will incorrectly return true for a position:fixed element inside a transformed one. You should also consider nested position:fixed too. Both need test cases.
> 

Do we need to worry about it here? It seems that we can safely create a layer even if we're below a transformed element.  It's the logic that handles positioning the layer during scrolls that will need to make sure it stops at the closest transformed ancestor instead of going all the way up to the frame.  


> You want to check that the container() of the renderer is the RenderView, but beware; this method may be called before the renderers have been hooked up.
Comment 27 Vangelis Kokkevis 2011-10-21 15:38:09 PDT
Created attachment 112035 [details]
Patch
Comment 28 Vangelis Kokkevis 2011-10-21 15:41:40 PDT
Modified the patch to check that the container is indeed our RenderView.  I had to add a flag to bypass the early out that happens when we try to update the composited layers and we think nothing changed. Simon, please have another look?
Comment 29 Vangelis Kokkevis 2011-10-25 16:40:27 PDT
(In reply to comment #28)
> Modified the patch to check that the container is indeed our RenderView.  I had to add a flag to bypass the early out that happens when we try to update the composited layers and we think nothing changed. Simon, please have another look?

Simon, any thoughts on this last patch?
Comment 30 Vangelis Kokkevis 2011-10-26 14:15:42 PDT
(In reply to comment #29)
> (In reply to comment #28)
> > Modified the patch to check that the container is indeed our RenderView.  I had to add a flag to bypass the early out that happens when we try to update the composited layers and we think nothing changed. Simon, please have another look?
> 
> Simon, any thoughts on this last patch?

Looking more closely at the iOS code I'm not sure I find it to be an improvement over what I posted in my first patch.  Essentially the iOS code ends up calling into the ChromeClient every time a layer is touched to notify it whether it is fixed position or not.  It will do that for every layer in the hierarchy.

I find it much cleaner and more efficient to set the fixed position property directly on the GraphicsLayer at the same time the rest of the GL properties are being updated.  I realize that GraphicsLayer's currently don't have any concept or API's related to layout properties, but fixed positioning seems to be universally useful.

Simon, how can get this moving along?
Comment 31 Simon Fraser (smfr) 2011-10-26 14:49:38 PDT
Comment on attachment 112035 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=112035&action=review

r+ but I'd prefer m_compositingNeedsUpdate and m_compositingDependsOnGeometry be combined.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1505
> +    // position:fixed elements that create their own stacking context (e.g. have an explicit z-index,
> +    // opacity, transform) can get their own composited layer.

Would be nice to explain here why you need the stacking context check.

> Source/WebCore/rendering/RenderLayerCompositor.h:303
> +    mutable bool m_compositingNeedsUpdate;

This has a similar role to m_compositingDependsOnGeometry; can they be combined into one "composting must be updated after layout"?
Comment 32 Vangelis Kokkevis 2011-10-26 15:18:12 PDT
(In reply to comment #31)
> (From update of attachment 112035 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=112035&action=review
> 
> r+ but I'd prefer m_compositingNeedsUpdate and m_compositingDependsOnGeometry be combined.
> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1505
> > +    // position:fixed elements that create their own stacking context (e.g. have an explicit z-index,
> > +    // opacity, transform) can get their own composited layer.
> 
> Would be nice to explain here why you need the stacking context check.
> 
> > Source/WebCore/rendering/RenderLayerCompositor.h:303
> > +    mutable bool m_compositingNeedsUpdate;
> 
> This has a similar role to m_compositingDependsOnGeometry; can they be combined into one "composting must be updated after layout"?

I agree, it would be nice if they could be merged.  However, m_compositingDependsOnGeometry is unfortunately a sticky bit whereas m_compositingNeedsUpdate can be cleared after layout has occurred.
Comment 33 Vangelis Kokkevis 2011-10-26 16:32:23 PDT
Created attachment 112616 [details]
Patch
Comment 34 Vangelis Kokkevis 2011-10-26 16:33:28 PDT
(In reply to comment #33)
> Created an attachment (id=112616) [details]
> Patch

One more attempt to see if the win-ews bot passes this time. No review requested.
Comment 35 Vangelis Kokkevis 2011-10-26 17:59:17 PDT
Created attachment 112628 [details]
Patch
Comment 36 Vangelis Kokkevis 2011-10-26 18:00:23 PDT
(In reply to comment #35)
> Created an attachment (id=112628) [details]
> Patch

This time without the WebKit2 deps files.  No review requested.  Still trying to get green bots.
Comment 37 Vangelis Kokkevis 2011-10-27 13:28:59 PDT
Committed r98627: <http://trac.webkit.org/changeset/98627>
Comment 38 Eric Seidel (no email) 2011-12-19 11:13:59 PST
Comment on attachment 112628 [details]
Patch

Cleared review? from attachment 112628 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).