Bug 41201

Summary: CSS3 background: Number of layers should be determined by background-image element count
Product: WebKit Reporter: Andreas Kling <kling>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: bdakin, benjamin, commit-queue, hyatt, koivisto, mitz, simon.fraser, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 42173, 43592    
Bug Blocks:    
Attachments:
Description Flags
Proposed patch
none
Proposed patch v2
none
Proposed patch v3 none

Comment 1 Andreas Kling 2010-06-25 02:27:58 PDT
Created attachment 59745 [details]
Proposed patch

Slightly altered culling logic in FillLayer to discard all layers after (and including) the first one with no image set.

Would like some suggestions on how to test this.
Comment 2 WebKit Review Bot 2010-06-25 02:32:00 PDT
Attachment 59745 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/rendering/style/FillLayer.cpp:253:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/style/FillLayer.cpp:254:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Andreas Kling 2010-07-12 15:56:56 PDT
Created attachment 61280 [details]
Proposed patch v2

I spoke with Antti about this on IRC, he suggested I make the logic a bit more readable.
I've also included a manual test.
Comment 4 WebKit Commit Bot 2010-07-13 04:05:06 PDT
Comment on attachment 61280 [details]
Proposed patch v2

Clearing flags on attachment: 61280

Committed r63192: <http://trac.webkit.org/changeset/63192>
Comment 5 WebKit Commit Bot 2010-07-13 04:05:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Antonio Gomes 2010-07-13 12:56:52 PDT
Should this be re-opened since bug 42173 reverted it.
Comment 7 Andreas Kling 2010-07-13 13:28:46 PDT
Yeah, reopening.
Comment 8 Andreas Kling 2010-07-16 05:50:26 PDT
We need to fillUnsetProperties() before we cullEmptyLayers()

Consider the following scenario:

#foo {
    background-image: url('Layer1'), ('Layer2');
    background-repeat: no-repeat;
}

The previous version of the patch would discard Layer2 since it had no corresponding properties. If we instead fillUnsetProperties() first, we get the correct result:

#foo {
    background-image: url('Layer1'), ('Layer2');
    background-repeat: no-repeat, no-repeat;
}

Now for the reverse scenario:

#foo {
    background-image: url('Layer1');
    background-repeat: no-repeat, no-repeat;
}

cullEmptyLayers() would previously remove the second 'no-repeat' before fillUnsetProperties() had a chance to repeat Layer1. If we swap the order of fillUnsetProperties() and cullEmptyLayers() we need to alter fillUnsetProperties() a bit - it shouldn't repeat image properties.
Comment 9 Andreas Kling 2010-07-16 06:09:53 PDT
Created attachment 61796 [details]
Proposed patch v3
Comment 10 WebKit Commit Bot 2010-07-19 07:06:15 PDT
Comment on attachment 61796 [details]
Proposed patch v3

Clearing flags on attachment: 61796

Committed r63653: <http://trac.webkit.org/changeset/63653>
Comment 11 WebKit Commit Bot 2010-07-19 07:06:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Simon Fraser (smfr) 2010-08-05 15:51:11 PDT
This broke multiple background image: bug 43581.
Comment 13 Simon Fraser (smfr) 2010-08-05 16:59:46 PDT
Rolling this out via bug 43592.
Comment 14 Simon Fraser (smfr) 2010-09-26 08:38:06 PDT
Reopening, since this was rolled out.
Comment 15 Andreas Kling 2011-03-11 12:17:53 PST

*** This bug has been marked as a duplicate of bug 47159 ***