Bug 50418

I don't understand the comment; opacity doesn't affect visual overflow, but it does affect whether RenderLayers get created.

dhyatt: visual overflow doesn't cross layers dhyatt: so if layers come and go you have to recompute it dhyatt: opacity layers are self-painting dhyatt: so if they go away, their visual overflow now has to be part of some parent layer

dhyatt: a more general way of looking at it is just "in the absence of overflow, would this property require a relayout" dhyatt: smfr: at the time i did this patch dhyatt: smfr: you could probably do a simplified layout dhyatt: smfr: in these cases dhyatt: but you'd need a new hint dhyatt: StyleDifferenceSimplifiedLayout smfr: dhyatt: is this the same as positioned objects layout, or another kind of simplified layout? dhyatt: you can do normal flow simplified layout too

Created attachment 194363 [details] Proposed patch This patch adds an option to check if a scheduled scripted animation is ongoing, and if so, we avoid forcing a layout when opacity has toggled between opaque and transparent. This solves the issue on extra repaints of compositing layers.

Comment on attachment 194363 [details] Proposed patch Attachment 194363 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17294048

Comment on attachment 194363 [details] Proposed patch Attachment 194363 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17112654

Created attachment 194536 [details] Proposed patch Fixed a typo (catch by mac* EWS bots).

Comment on attachment 194536 [details] Proposed patch Removing review flag, I've noticed that Document::isScriptedAnimationScheduled() always returns false when called from RenderObject (as we're handling the computed style generated from the previously-scheduled animation step).

Created attachment 194614 [details] Proposed patch Now checking if there are ongoing scripted animations by looking for fired callbacks.

Comment on attachment 194614 [details] Proposed patch Why would needing a layout or not depend on if we're in a scripted animation callback?

(In reply to comment #10) > (From update of attachment 194614 [details]) > Why would needing a layout or not depend on if we're in a scripted animation callback? Rationale is that when animating, we can assume that the layout was previously computed before the animation has begun, thus causing no harm. Pure CSS animations (using keyframes) goes under another code path, which also does not cause a full layout from toggling between opaque and transparent.

Comment on attachment 194614 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=194614&action=review Some comments, though it definitively needs smfr or hyatt's review. > Source/WebCore/ChangeLog:13 > + Patch covered by existing tests. It is important to list the tests. Is that a test that gets fixed by this? Not possible to add a test? > Source/WebCore/dom/Document.cpp:4963 > +bool Document::isScriptedAnimationActive() const > Source/WebCore/dom/ScriptedAnimationController.cpp:85 > +bool ScriptedAnimationController::isActive() const > Source/WebCore/rendering/RenderObject.cpp:1732 > + // FIXME: Force a layout if not on a scheduled scripted animation (see > + // RenderStyle::diff() for details. I do not like this "see RenderStyle::diff for details" part here. It is already hard to keep comments up to date. Keep comments referring other parts of the code it even fragiler, as the other part can change and it won't get updated here.

Comment on attachment 194614 [details] Proposed patch I don't think looking for running animations is appropriate.

(In reply to comment #13) > (From update of attachment 194614 [details]) > I don't think looking for running animations is appropriate. I understand the "intrusiveness" level this patch proposes is quite high, however I'm pretty sure those extra texture uploads are not necessary. Do you have another approach suggestion in mind? I thought about instead of having this check when selecting which style diff to use, we could avoid the texture upload by not marking the backing store tiles dirty depending if we know they already have their own RenderLayer.

If you care about layout or style updates can't you query the layout/style systems for the information you need?

Created attachment 195426 [details] Proposed patch Fixed implementation by checking if the RenderObject already has its own layer and a layout test to verify that we're not uploading redundant textures. Includes added window.internals* helper functions to obtain repaint count data. I'll wait for EWS results to request review flag.

Comment on attachment 195426 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=195426&action=review > Source/WebCore/page/Frame.h:139 > + String layerTreeRepaintCountAsText() const; You should not add a new function for this. Just add a new flag to LayerTreeFlags, and optionally dump repaint counts as part of the layer tree dump. > Source/WebCore/rendering/RenderObject.cpp:1733 > + if (contextSensitiveProperties & ContextSensitivePropertyOpacityToggle && !hasLayer()) > + diff = StyleDifferenceLayout; > + else if (!isText() && (!hasLayer() || !toRenderLayerModelObject(this)->layer()->isComposited())) This is wrong. Opacity may have toggled on a renderer with a layer (maybe it has overflow), but we still have to do a layout because opacity affects stacking context. > LayoutTests/ChangeLog:9 > + Added layout test to check if we are avoiding redundant texture uploads > + in composite layers when toggling opacity. Please don't call these "texture uploads"; that's very specific to your point of view. They are just paints.

(In reply to comment #17) > (From update of attachment 195426 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=195426&action=review > > > Source/WebCore/page/Frame.h:139 > > + String layerTreeRepaintCountAsText() const; > > You should not add a new function for this. Just add a new flag to LayerTreeFlags, and optionally dump repaint counts as part of the layer tree dump. I remembered that we can already dump repaint rects in composting layers, which should be enough to test your changes here. No new testing infrastructure required.

(In reply to comment #18) > (In reply to comment #17) > > (From update of attachment 195426 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=195426&action=review > > > > > Source/WebCore/page/Frame.h:139 > > > + String layerTreeRepaintCountAsText() const; > > > > You should not add a new function for this. Just add a new flag to LayerTreeFlags, and optionally dump repaint counts as part of the layer tree dump. > > I remembered that we can already dump repaint rects in composting layers, which should be enough to test your changes here. No new testing infrastructure required. You are right, Simon. I erroneously assumed that repaint rects were collected for another purpose. Using repaint rects fits the purpose of the layout test. I am about to upload a new version of this patch, w/o changes to window.internals, after patch from bug 113732 lands.

Created attachment 197160 [details] Proposed patch Fix from bug 113732 only affects visual output of the layout test, but generated render layer is the same.

Comment on attachment 197160 [details] Proposed patch Attachment 197160 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17649066 New failing tests: media/video-zoom.html compositing/repaint/avoid-repaint-when-changing-opacity.html

Created attachment 197190 [details] Archive of layout-test-results from webkit-ews-07 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-07 Port: mac-mountainlion Platform: Mac OS X 10.8.2

Comment on attachment 197160 [details] Proposed patch Attachment 197160 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17722124 New failing tests: compositing/repaint/avoid-repaint-when-changing-opacity.html

Created attachment 197246 [details] Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-15 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.2

Created attachment 197252 [details] Proposed patch Added mac-specific test result / invalidate_rect.html fail is unrelated and handled in r148066.

Created attachment 197256 [details] Proposed patch Mac results were actually correct for all platforms, after changes from r148048 which fixed window.internals.repaintRectAsText() behavior.

Comment on attachment 197256 [details] Proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=197256&action=review > Source/WebCore/rendering/RenderObject.cpp:1735 > + // if we are not composited or have no children neither overflow. Do you mean "no children that overflow" or "no children, nor overflow"? > Source/WebCore/rendering/RenderObject.cpp:1739 > + if ((contextSensitiveProperties & ContextSensitivePropertyOpacityToggle) && (!isCompositedLayer || (!isEmpty() && !hasOverflowClip()))) > + diff = StyleDifferenceLayout; I don't think this (!isEmpty() && !hasOverflowClip()) test makes sense. You need to do layout when opacity toggles to update z-order lists, visual overflow etc.

Created attachment 198193 [details] Patch Now when opacity toggles, full layout behavior is preserved, however full repaint is avoided in favor of recomposite when layer is composited.

*** Bug 116047 has been marked as a duplicate of this bug. ***

I am running into a full relayout and repaint of a composited layer which gets some opacity (it goes from fully opaque to translucent). Would adding a StyleDifferentLayoutIfUncomposited flag or passing whether the styled element is already composited to RenderStyle::diff work? I think it'd be OK so long as the element isn't composited merely because it has opacity (which might be the case on iOS). RenderStyle doesn't know anything about what it's applied to currently, so adding the knowledge of whether something is composited is ugly. Adding a new StyleDifference flag that's almost the same as Layout is lame too, since there are several places that check for StyleDifferenceLayout. Simon, what do you think about: 1. Handling this just for "already composited" elements? 2. Adding a StyleDifferentLayoutIfUncomposited flag versus passing a ElementIsComposited flag to RenderStyle::diff?

Comment on attachment 198193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198193&action=review > Source/WebCore/rendering/RenderLayerModelObject.cpp:126 > || oldStyle->transform() != newStyle->transform() The negative condition on the result of the || operation makes it harder to read. Maybe "... && oldStyle->opacity() != 1 && newStyle->opacity() != 1)" would be clearer? > Source/WebCore/rendering/RenderObject.cpp:1811 > + I think it would be better to avoid the if/else sequence and just use the condition as a parameter to the setter.

Comment on attachment 198193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=198193&action=review > Source/WebCore/rendering/RenderObject.h:1105 > + ADD_BOOLEAN_BITFIELD(needsLayoutRecompositeOnly, NeedsLayoutRecompositeOnly); I don't think it's right to use a RenderObject bit for this. First, it doesn't apply to all RenderObjects (only RenderElements). Second, it's state that only persists between style changes and not for the life of the object.