Bug 108203

Created attachment 185267 [details] Patch

Comment on attachment 185267 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185267&action=review > Source/WebCore/platform/graphics/GraphicsLayer.h:330 > + // contentsTileRect is a single tile of contents in layer coordinates, if contents is to be repeated. > + virtual void setContentsTileRect(const IntRect& r) { m_contentsTileRect = r; } > + IntRect contentsTileRect() const { return m_contentsTileRect; } Are you only supporting tiled background images? What about tiling just in x or y, or untiled? > Source/WebCore/rendering/RenderLayerBacking.cpp:1373 > + if (renderer()->isImage()) > + return; This seems a bit out of place here. I would not expect to get into this function for an image.

(In reply to comment #2) > (From update of attachment 185267 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=185267&action=review > > > Source/WebCore/platform/graphics/GraphicsLayer.h:330 > > + // contentsTileRect is a single tile of contents in layer coordinates, if contents is to be repeated. > > + virtual void setContentsTileRect(const IntRect& r) { m_contentsTileRect = r; } > > + IntRect contentsTileRect() const { return m_contentsTileRect; } > > Are you only supporting tiled background images? What about tiling just in x or y, or untiled? > Supporting x, y and untiled as well, but those don't require a contentsTileRect. > > Source/WebCore/rendering/RenderLayerBacking.cpp:1373 > > + if (renderer()->isImage()) > > + return; > > This seems a bit out of place here. I would not expect to get into this function for an image. Right.

(In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 185267 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=185267&action=review > > > > > Source/WebCore/platform/graphics/GraphicsLayer.h:330 > > > + // contentsTileRect is a single tile of contents in layer coordinates, if contents is to be repeated. > > > + virtual void setContentsTileRect(const IntRect& r) { m_contentsTileRect = r; } > > > + IntRect contentsTileRect() const { return m_contentsTileRect; } > > > > Are you only supporting tiled background images? What about tiling just in x or y, or untiled? > > > Supporting x, y and untiled as well, but those don't require a contentsTileRect. I didn't see functions on GraphicsLayer to specify the tiling behavior.

(In reply to comment #4) > (In reply to comment #3) > > (In reply to comment #2) > > > (From update of attachment 185267 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=185267&action=review > > > > > > > Source/WebCore/platform/graphics/GraphicsLayer.h:330 > > > > + // contentsTileRect is a single tile of contents in layer coordinates, if contents is to be repeated. > > > > + virtual void setContentsTileRect(const IntRect& r) { m_contentsTileRect = r; } > > > > + IntRect contentsTileRect() const { return m_contentsTileRect; } > > > > > > Are you only supporting tiled background images? What about tiling just in x or y, or untiled? > > > > > Supporting x, y and untiled as well, but those don't require a contentsTileRect. > > I didn't see functions on GraphicsLayer to specify the tiling behavior. It's not necessary. RenderBoxObjectModel::getGeometryForBackgroundImage takes care of that. For example if the background is tiled only horizontally, the contentsRect would be cropped to include only the tiled row.

(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > (In reply to comment #2) > > > > (From update of attachment 185267 [details] [details] [details] [details]) > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=185267&action=review > > > > > > > > > Source/WebCore/platform/graphics/GraphicsLayer.h:330 > > > > > + // contentsTileRect is a single tile of contents in layer coordinates, if contents is to be repeated. > > > > > + virtual void setContentsTileRect(const IntRect& r) { m_contentsTileRect = r; } > > > > > + IntRect contentsTileRect() const { return m_contentsTileRect; } > > > > > > > > Are you only supporting tiled background images? What about tiling just in x or y, or untiled? > > > > > > > Supporting x, y and untiled as well, but those don't require a contentsTileRect. > > > > I didn't see functions on GraphicsLayer to specify the tiling behavior. > It's not necessary. RenderBoxObjectModel::getGeometryForBackgroundImage takes care of that. > For example if the background is tiled only horizontally, the contentsRect would be cropped to include only the tiled row. I should probably add that to the Changelog :)

Comment on attachment 185267 [details] Patch Attachment 185267 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16218108

Comment on attachment 185267 [details] Patch Attachment 185267 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16195270

Created attachment 185443 [details] Patch

Created attachment 185478 [details] Patch

Comment on attachment 185478 [details] Patch Attachment 185478 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16195694 New failing tests: compositing/patterns/direct-pattern-compositing-rotation.html

Created attachment 185505 [details] Patch

Comment on attachment 185505 [details] Patch Attachment 185505 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16218609 New failing tests: compositing/patterns/direct-pattern-compositing-rotation.html

Created attachment 185513 [details] Patch

Removed the rotation test, it doesn't make sense as a ref test.

Comment on attachment 185513 [details] Patch Attachment 185513 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16272096 New failing tests: platform/chromium/virtual/gpu/compositedscrolling/overflow/scroll-ancestor-update.html platform/chromium/virtual/gpu/fast/canvas/fillrect_gradient.html platform/chromium/virtual/softwarecompositing/overflow/scroll-ancestor-update.html platform/chromium/virtual/gpu/fast/canvas/patternfill-repeat.html platform/chromium/virtual/gpu/fast/canvas/canvas-composite.html platform/chromium/virtual/softwarecompositing/geometry/video-fixed-scrolling.html compositing/patterns/direct-pattern-compositing-padding.html compositing/overflow/overflow-compositing-descendant.html compositing/layers-inside-overflow-scroll.html platform/chromium/virtual/softwarecompositing/overflow/overflow-compositing-descendant.html fast/loader/text-document-wrapping.html platform/chromium/virtual/softwarecompositing/tile-cache-must-flatten.html compositing/self-painting-layers.html compositing/overflow/scroll-ancestor-update.html platform/chromium/virtual/softwarecompositing/self-painting-layers.html platform/chromium/virtual/gpu/fast/canvas/canvas-text-alignment.html compositing/visibility/visibility-simple-video-layer.html platform/chromium/virtual/gpu/fast/canvas/canvas-text-baseline.html compositing/geometry/video-opacity-overlay.html platform/chromium/virtual/softwarecompositing/repaint/clipped-layer-size-change.html compositing/geometry/video-fixed-scrolling.html platform/chromium/virtual/gpu/compositedscrolling/overflow/overflow-compositing-descendant.html platform/chromium/virtual/softwarecompositing/geometry/video-opacity-overlay.html platform/chromium/virtual/softwarecompositing/layers-inside-overflow-scroll.html platform/chromium/virtual/softwarecompositing/patterns/direct-pattern-compositing-padding.html compositing/webgl/webgl-no-alpha.html compositing/webgl/webgl-reflection.html fast/loader/javascript-url-in-object.html

Created attachment 185990 [details] Patch

Comment on attachment 185990 [details] Patch Attachment 185990 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16293679 New failing tests: compositing/background-color/background-color-change-to-text.html css3/filters/huge-region-composited.html compositing/background-color/background-color-padding-change.html

Comment on attachment 185990 [details] Patch Attachment 185990 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16306732 New failing tests: compositing/background-color/background-color-change-to-text.html css3/filters/huge-region-composited.html compositing/background-color/background-color-padding-change.html

Comment on attachment 185990 [details] Patch Attachment 185990 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16308768 New failing tests: fast/css/sticky/sticky-as-positioning-container.html compositing/background-color/background-color-change-to-text.html css3/filters/huge-region-composited.html compositing/background-color/background-color-padding-change.html compositing/patterns/direct-pattern-compositing-add-text.html

Comment on attachment 185990 [details] Patch Attachment 185990 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16331211 New failing tests: compositing/patterns/direct-pattern-compositing-padding.html css3/filters/huge-region-composited.html platform/chromium/virtual/softwarecompositing/patterns/direct-pattern-compositing-padding.html

Created attachment 186221 [details] Patch

Comment on attachment 186221 [details] Patch Attachment 186221 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16343677 New failing tests: compositing/patterns/direct-pattern-compositing-padding.html platform/chromium/virtual/softwarecompositing/patterns/direct-pattern-compositing-padding.html

Comment on attachment 186221 [details] Patch Attachment 186221 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16337768

Comment on attachment 186221 [details] Patch Attachment 186221 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16342710 New failing tests: compositing/patterns/direct-pattern-compositing-padding.html platform/chromium/virtual/softwarecompositing/patterns/direct-pattern-compositing-padding.html

Created attachment 186229 [details] Patch

Comment on attachment 186229 [details] Patch Attachment 186229 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16335922

Comment on attachment 186229 [details] Patch Attachment 186229 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16346026

Created attachment 186405 [details] Patch

Comment on attachment 186405 [details] Patch Attachment 186405 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16374289 New failing tests: compositing/patterns/direct-pattern-compositing-change.html

Comment on attachment 186405 [details] Patch Attachment 186405 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/16371408

Those tests work perfectly on my machine; Is there some flakiness with running ref-tests with CA?

Created attachment 187434 [details] Patch

Comment on attachment 187434 [details] Patch Attachment 187434 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16467620 New failing tests: platform/chromium/virtual/softwarecompositing/patterns/direct-pattern-compositing-add-text.html compositing/patterns/direct-pattern-compositing-padding.html platform/chromium/virtual/softwarecompositing/patterns/direct-pattern-compositing-position.html platform/chromium/virtual/softwarecompositing/patterns/direct-pattern-compositing-cover.html compositing/patterns/direct-pattern-compositing.html compositing/patterns/direct-pattern-compositing-cover.html compositing/patterns/direct-pattern-compositing-position.html platform/chromium/virtual/softwarecompositing/patterns/direct-pattern-compositing.html compositing/patterns/direct-pattern-compositing-contain.html compositing/patterns/direct-pattern-compositing-change.html platform/chromium/virtual/softwarecompositing/patterns/direct-pattern-compositing-change.html platform/chromium/virtual/softwarecompositing/patterns/direct-pattern-compositing-contain.html compositing/patterns/direct-pattern-compositing-size.html platform/chromium/virtual/softwarecompositing/patterns/direct-pattern-compositing-size.html compositing/patterns/direct-pattern-compositing-add-text.html platform/chromium/virtual/softwarecompositing/patterns/direct-pattern-compositing-padding.html

Created attachment 187449 [details] Patch

Comment on attachment 187449 [details] Patch Attachment 187449 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16473699 New failing tests: compositing/patterns/direct-pattern-compositing-change.html platform/chromium/virtual/softwarecompositing/patterns/direct-pattern-compositing-change.html

Created attachment 187473 [details] Patch

Comment on attachment 187473 [details] Patch Attachment 187473 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16479703 New failing tests: platform/chromium/virtual/softwarecompositing/patterns/direct-pattern-compositing-add-text.html compositing/patterns/direct-pattern-compositing-change.html platform/chromium/virtual/softwarecompositing/patterns/direct-pattern-compositing-change.html compositing/patterns/direct-pattern-compositing-add-text.html

Created attachment 187543 [details] Patch

Created attachment 187875 [details] Patch

Ping?

(In reply to comment #41) > Ping? Simon, any chance for review? This has been sitting here for a while.

Comment on attachment 187875 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187875&action=review > Source/WebCore/platform/graphics/GraphicsLayer.h:328 > + // contentsTileRect is a single tile of contents in layer coordinates, if contents is to be repeated. This comment and name are a bit ambiguous. For repeated contents, does this represent just a single tile (in order to specify the offset and size), not the area covered by tiles? Might it be clearer to use phase and tileSize instead? > Source/WebCore/rendering/RenderLayerBacking.cpp:1344 > + Color color = style->visitedDependentColor(CSSPropertyBackgroundColor); > + if (color.isValid() && color.alpha()) > + return false; Why does an alpha color bail here? Can't you still set the layer's background color and also do tiling? > Source/WebCore/rendering/RenderLayerBacking.cpp:1353 > + if (!styleImage->isLoaded()) > + return false; Does the image loading cause us to re-evaluate compositing? > Source/WebCore/rendering/RenderLayerBacking.cpp:1363 > + enum { MaxSizeForPatten = 1024 }; > + if (image->width() > MaxSizeForPatten || image->height() > MaxSizeForPatten) > + return false; This looks pretty platform-specific. > Source/WebCore/rendering/RenderLayerBacking.cpp:1377 > + if (!isSimpleContainer || !style->hasBackgroundImage()) { > + m_graphicsLayer->setContentsToImage(0); > + return; It's not clear from the function name that you'll never get here for composited <img>, so setting contents to image unconditionally looks a bit weird. > LayoutTests/compositing/patterns/direct-pattern-compositing-add-text-expected.html:2 > +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" > + "http://www.w3.org/TR/html4/loose.dtd"> Yuck. > LayoutTests/compositing/patterns/direct-pattern-compositing-add-text.html:2 > +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" > + "http://www.w3.org/TR/html4/loose.dtd"> Ditto.

Comment on attachment 187875 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=187875&action=review >> Source/WebCore/platform/graphics/GraphicsLayer.h:328 >> + // contentsTileRect is a single tile of contents in layer coordinates, if contents is to be repeated. > > This comment and name are a bit ambiguous. For repeated contents, does this represent just a single tile (in order to specify the offset and size), not the area covered by tiles? > > Might it be clearer to use phase and tileSize instead? I was contemplating this. Yes, maybe setContentsTilePhase and setContentsTileSize would work better. >> Source/WebCore/rendering/RenderLayerBacking.cpp:1344 >> + return false; > > Why does an alpha color bail here? Can't you still set the layer's background color and also do tiling? It's currently not supported. We can support this in the future, but need to take care of background-compositing and other issues. Trying to err on the careful side for this patch. also, the more elements added to the layer, the less it benefits from direct compositing. >> Source/WebCore/rendering/RenderLayerBacking.cpp:1353 >> + return false; > > Does the image loading cause us to re-evaluate compositing? Actually, no. I'll investigate what's the right thing to do with contentChanged. >> Source/WebCore/rendering/RenderLayerBacking.cpp:1363 >> + return false; > > This looks pretty platform-specific. OK, I'll move to GraphicsLayer::shouldDirectlyCompositeTiledImage >> Source/WebCore/rendering/RenderLayerBacking.cpp:1377 >> + return; > > It's not clear from the function name that you'll never get here for composited <img>, so setting contents to image unconditionally looks a bit weird. Would a comment suffice for this? or maybe change the function name to updateDirectlyCompositedBackgroundImage? >> LayoutTests/compositing/patterns/direct-pattern-compositing-add-text-expected.html:2 >> + "http://www.w3.org/TR/html4/loose.dtd"> > > Yuck. Oops, copy paste from somewhere.

(In reply to comment #44) > (From update of attachment 187875 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=187875&action=review > > Why does an alpha color bail here? Can't you still set the layer's background color and also do tiling? > > It's currently not supported. We can support this in the future, but need to take care of background-compositing and other issues. > Trying to err on the careful side for this patch. also, the more elements added to the layer, the less it benefits from direct compositing. OK. > > It's not clear from the function name that you'll never get here for composited <img>, so setting contents to image unconditionally looks a bit weird. > > Would a comment suffice for this? or maybe change the function name to updateDirectlyCompositedBackgroundImage? That name is better.

Created attachment 193589 [details] Patch

Comment on attachment 193589 [details] Patch Attachment 193589 [details] did not pass mac-ews (mac): Output: http://webkit-commit-queue.appspot.com/results/17187500 New failing tests: css3/filters/custom/effect-custom-transform-parameters.html compositing/background-color/background-color-alpha.html fast/repaint/region-painting-in-composited-view.html compositing/background-color/background-color-text-change.html compositing/background-color/background-color-composite.html compositing/background-color/background-color-text-clip.html compositing/background-color/background-color-content-clip.html compositing/geometry/fixed-position-composited-page-scale-smaller-than-viewport.html compositing/background-color/background-color-padding-clip.html fast/layers/no-clipping-overflow-hidden-added-after-transition.html compositing/background-color/background-color-container.html compositing/background-color/background-color-simple.html css3/filters/huge-region-composited.html compositing/overlap-blending/children-opacity-no-overlap.html

Comment on attachment 193589 [details] Patch Attachment 193589 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17132280 New failing tests: css3/filters/effect-blur-hw.html compositing/webgl/webgl-nonpremultiplied-blend.html compositing/nested-direct-image-compositing.html compositing/direct-image-compositing.html http/tests/cache/subresource-failover-to-network.html fast/css/font-family-pictograph.html css3/filters/huge-region-composited.html compositing/visibility/visibility-simple-canvas2d-layer.html fast/canvas/webgl/webgl-layer-update.html compositing/webgl/webgl-repaint.html compositing/images/content-image.html compositing/images/content-image-change.html fast/canvas/webgl/webgl-composite-modes-repaint.html compositing/visibility/visibility-simple-webgl-layer.html compositing/patterns/direct-pattern-compositing.html compositing/culling/tile-occlusion-boundaries.html compositing/overflow/overflow-hidden-canvas-layer.html compositing/visibility/visibility-image-layers.html compositing/webgl/webgl-no-alpha.html css3/filters/effect-reference-hw.html compositing/webgl/webgl-reflection.html compositing/visibility/visibility-image-layers-dynamic.html compositing/backface-visibility/backface-visibility-image.html compositing/masks/direct-image-mask.html fast/canvas/webgl/webgl-composite-modes.html fast/loader/javascript-url-in-object.html compositing/color-matching/image-color-matching.html compositing/overflow/overflow-compositing-descendant.html compositing/backface-visibility/backface-visibility-webgl.html compositing/reflections/simple-composited-reflections.html

Comment on attachment 193589 [details] Patch Attachment 193589 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-commit-queue.appspot.com/results/17121799 New failing tests: fast/css/sticky/sticky-margins.html fast/css/sticky/sticky-top-margins.html compositing/background-color/background-color-composite.html fast/css/sticky/sticky-left-percentage.html fast/css/sticky/sticky-stacking-context.html fast/css/sticky/sticky-writing-mode-vertical-rl.html fast/css/sticky/sticky-both-sides.html css3/filters/huge-region-composited.html fast/css/sticky/sticky-side-margins.html fast/css/sticky/sticky-left.html compositing/background-color/background-color-text-change.html compositing/background-color/background-color-simple.html css3/filters/custom/effect-custom-transform-parameters.html fast/css/sticky/sticky-top.html fast/layers/no-clipping-overflow-hidden-added-after-transition.html fast/css/sticky/inflow-sticky.html compositing/background-color/background-color-content-clip.html editing/pasteboard/smart-paste-001.html compositing/overlap-blending/children-opacity-no-overlap.html fast/forms/number/number-spinbutton-in-multi-column.html compositing/geometry/fixed-position-composited-page-scale-smaller-than-viewport.html compositing/background-color/background-color-padding-clip.html compositing/background-color/background-color-container.html fast/css/sticky/sticky-writing-mode-vertical-lr.html fast/css/sticky/sticky-writing-mode-horizontal-bt.html http/tests/security/cross-origin-appcache-allowed.html canvas/philip/tests/2d.text.draw.fontface.notinpage.html compositing/background-color/background-color-text-clip.html fast/css/sticky/sticky-overflowing.html compositing/background-color/background-color-alpha.html

Created attachment 194501 [details] Patch

Comment on attachment 194501 [details] Patch Attachment 194501 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17140618 New failing tests: platform/chromium/virtual/softwarecompositing/patterns/direct-pattern-compositing-load.html compositing/patterns/direct-pattern-compositing-load.html

Created attachment 194517 [details] Archive of layout-test-results from gce-cr-linux-05 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-05 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

Comment on attachment 194501 [details] Patch Attachment 194501 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17247356 New failing tests: platform/chromium/virtual/softwarecompositing/patterns/direct-pattern-compositing-load.html compositing/patterns/direct-pattern-compositing-load.html

Created attachment 194522 [details] Archive of layout-test-results from gce-cr-linux-06 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-06 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

Created attachment 194529 [details] Patch

This should now be cleaned up and ready for review. I've added a test for the case where the background image is loaded after the layer is already composited.

Comment on attachment 194529 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=194529&action=review > Source/WebCore/platform/graphics/GraphicsLayer.h:330 > + virtual void setContentsTilePhase(const IntPoint& p) { m_contentsTilePhase = p; } > + IntPoint contentsTilePhase() const { return m_contentsTilePhase; } Would be nice to add a comment saying what this represents. What is it relative to; the GrpahicsLayer bounds? The content layer bounds? The offsetFromRenderer?

Created attachment 197309 [details] Patch

(In reply to comment #57) > (From update of attachment 194529 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=194529&action=review > > > Source/WebCore/platform/graphics/GraphicsLayer.h:330 > > + virtual void setContentsTilePhase(const IntPoint& p) { m_contentsTilePhase = p; } > > + IntPoint contentsTilePhase() const { return m_contentsTilePhase; } > > Would be nice to add a comment saying what this represents. What is it relative to; the GrpahicsLayer bounds? The content layer bounds? The offsetFromRenderer? Thanks for the review Simon, I've added a comment and will land after it gets another EWS run.

Comment on attachment 197309 [details] Patch Clearing flags on attachment: 197309 Committed r148172: <http://trac.webkit.org/changeset/148172>

All reviewed patches have been landed. Closing bug.