Bug 116140

Summary: Direct pattern compositing breaks when no-repeat is set on a large layer
Product: WebKit Reporter: Noam Rosenthal <noam>
Component: New BugsAssignee: Noam Rosenthal <noam>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, esprehn+autocc, glenn, simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Noam Rosenthal 2013-05-15 00:36:28 PDT
Direct pattern compositing breaks when no-repeat is set on a large layer
Comment 1 Noam Rosenthal 2013-05-15 00:40:00 PDT
Created attachment 201797 [details]
Patch
Comment 2 Csaba Osztrogonác 2013-05-15 00:47:26 PDT
Comment on attachment 201797 [details]
Patch

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

> LayoutTests/compositing/patterns/direct-pattern-compositing-contain-no-repeat-expected.html:8
> +      background-image: url(http://achicu.github.io/css-presentation/images/webkit-icon.png);

Do we really want to depend on an external image?

> LayoutTests/compositing/patterns/direct-pattern-compositing-contain-no-repeat.html:8
> +      background-image: url(http://achicu.github.io/css-presentation/images/webkit-icon.png);

ditto
Comment 3 Noam Rosenthal 2013-05-15 00:49:11 PDT
Created attachment 201798 [details]
Patch
Comment 4 Noam Rosenthal 2013-05-15 00:49:37 PDT
(In reply to comment #2)
> (From update of attachment 201797 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=201797&action=review
> 
> > LayoutTests/compositing/patterns/direct-pattern-compositing-contain-no-repeat-expected.html:8
> > +      background-image: url(http://achicu.github.io/css-presentation/images/webkit-icon.png);
> 
> Do we really want to depend on an external image?
> 
> > LayoutTests/compositing/patterns/direct-pattern-compositing-contain-no-repeat.html:8
> > +      background-image: url(http://achicu.github.io/css-presentation/images/webkit-icon.png);
> 
> ditto

Oops, of course not :)
Comment 5 Benjamin Poulain 2013-05-15 13:53:06 PDT
Comment on attachment 201798 [details]
Patch

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

> LayoutTests/compositing/patterns/direct-pattern-compositing-contain-no-repeat-expected.html:4
> +    .composited { -webkit-transform: matrix3d(1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 0, 0, 3, 1); }

I'd remove this from the reference.
Comment 6 Simon Fraser (smfr) 2013-05-15 13:57:38 PDT
Comment on attachment 201798 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Making sure that the solid color logic only overrides the cotnentsRect if the background

Typo cotnentsRect

> Source/WebCore/rendering/RenderLayerBacking.cpp:894
>      updateDirectlyCompositedBackgroundImage(isSimpleContainer, didUpdateContentsRect);
> +    if (didUpdateContentsRect)
> +        return;
>      updateDirectlyCompositedBackgroundColor(isSimpleContainer, didUpdateContentsRect);

It's not clear why you should bail if didUpdateContentsRect is true. Does that mean you chose to use the background image path? Why does didUpdateContentsRect indicate that, rather than the return value?

>> LayoutTests/compositing/patterns/direct-pattern-compositing-contain-no-repeat-expected.html:4
>> +    .composited { -webkit-transform: matrix3d(1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 0, 0, 3, 1); }
> 
> I'd remove this from the reference.

Composited vs. non-composted refs break on Mac, alas.

But this should use translateZ() or something less mysterious.
Comment 7 Noam Rosenthal 2013-05-15 14:06:12 PDT
> It's not clear why you should bail if didUpdateContentsRect is true. Does that mean you chose to use the background image path? Why does didUpdateContentsRect indicate that, rather than the return value?
> 
Right, I'll change it to be a return value.

> >> LayoutTests/compositing/patterns/direct-pattern-compositing-contain-no-repeat
> 
> Composited vs. non-composted refs break on Mac, alas.
Is that going to be fixed as we discussed at the summit, or should I make this into a regular pixel test? Ref-tests seem like the right choice here given that the platform supports them.

> But this should use translateZ() or something less mysterious.
Sure :)
Comment 8 Noam Rosenthal 2013-05-17 05:47:32 PDT
Created attachment 202070 [details]
Patch
Comment 9 Noam Rosenthal 2013-05-24 09:21:48 PDT
Created attachment 202829 [details]
Patch
Comment 10 Antti Koivisto 2013-05-24 09:24:32 PDT
Comment on attachment 202829 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerBacking.cpp:879
> -    updateDirectlyCompositedBackgroundImage(isSimpleContainer, didUpdateContentsRect);
>      updateDirectlyCompositedBackgroundColor(isSimpleContainer, didUpdateContentsRect);
> +    updateDirectlyCompositedBackgroundImage(isSimpleContainer, didUpdateContentsRect);

Would be good to add a comment mentioning why the order matters.
Comment 11 Noam Rosenthal 2013-05-25 01:28:50 PDT
Created attachment 202879 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2013-05-25 02:51:08 PDT
Comment on attachment 202879 [details]
Patch for landing

Clearing flags on attachment: 202879

Committed r150685: <http://trac.webkit.org/changeset/150685>
Comment 13 WebKit Commit Bot 2013-05-25 02:51:11 PDT
All reviewed patches have been landed.  Closing bug.