Allow direct compositing of background images

• Component: New Bugs
• Status: RESOLVED FIXED

[Noam Rosenthal]

Allow direct compositing of background images

[Noam Rosenthal]

Created attachment 185267 [details]
Patch

[Simon Fraser (smfr)]

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.

[Noam Rosenthal]

(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.

[Simon Fraser (smfr)]

(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.

[Noam Rosenthal]

(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.

[Noam Rosenthal]

(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 :)

[Build Bot]

Comment on attachment 185267 [details]
Patch

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

[Build Bot]

Comment on attachment 185267 [details]
Patch

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

[Noam Rosenthal]

Created attachment 185443 [details]
Patch

[Noam Rosenthal]

Created attachment 185478 [details]
Patch

[Build Bot]

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

[Noam Rosenthal]

Created attachment 185505 [details]
Patch

[Build Bot]

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

[Noam Rosenthal]

Created attachment 185513 [details]
Patch

[Noam Rosenthal]

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

[WebKit Review Bot]

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

[Noam Rosenthal]

Created attachment 185990 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[WebKit Review Bot]

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

[Noam Rosenthal]

Created attachment 186221 [details]
Patch

[WebKit Review Bot]

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

[Build Bot]

Comment on attachment 186221 [details]
Patch

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

[WebKit Review Bot]

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

[Noam Rosenthal]

Created attachment 186229 [details]
Patch

[Build Bot]

Comment on attachment 186229 [details]
Patch

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

[Build Bot]

Comment on attachment 186229 [details]
Patch

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

[Noam Rosenthal]

Created attachment 186405 [details]
Patch

[Build Bot]

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

[Build Bot]

Comment on attachment 186405 [details]
Patch

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

[Noam Rosenthal]

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

[Noam Rosenthal]

Created attachment 187434 [details]
Patch

[WebKit Review Bot]

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

[Noam Rosenthal]

Created attachment 187449 [details]
Patch

[WebKit Review Bot]

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

[Noam Rosenthal]

Created attachment 187473 [details]
Patch

[WebKit Review Bot]

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

[Noam Rosenthal]

Created attachment 187543 [details]
Patch

[Noam Rosenthal]

Created attachment 187875 [details]
Patch

[Noam Rosenthal]

Ping?

[Noam Rosenthal]

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

[Simon Fraser (smfr)]

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.

[Noam Rosenthal]

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.

[Simon Fraser (smfr)]

(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.

[Noam Rosenthal]

Created attachment 193589 [details]
Patch

[Build Bot]

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

[WebKit Review Bot]

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

[Build Bot]

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

[Noam Rosenthal]

Created attachment 194501 [details]
Patch

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Noam Rosenthal]

Created attachment 194529 [details]
Patch

[Noam Rosenthal]

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.

[Simon Fraser (smfr)]

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?

[Noam Rosenthal]

Created attachment 197309 [details]
Patch

[Noam Rosenthal]

(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.

[WebKit Commit Bot]

Comment on attachment 197309 [details]
Patch

Clearing flags on attachment: 197309

Committed r148172: <http://trac.webkit.org/changeset/148172>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.