Bug 125213 – CSS: Add a property cascading pass to style application.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Andreas Kling]

CSS: Add a property cascading pass to style application.

[Andreas Kling]

Created attachment 218391 [details]
For EWS

A snack for EWS to see where this is at. Matched properties cache is broken and disabled.

[Andreas Kling]

Created attachment 218392 [details]
For EWS

[WebKit Commit Bot]

Attachment 218392 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/css/StyleResolver.h']" exit_code: 1
ERROR: Source/WebCore/css/StyleResolver.cpp:211:  The parameter name "matchResult" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/css/StyleResolver.cpp:231:  The parameter name "propertyWhitelistType" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/css/StyleResolver.h:362:  The parameter name "cascade" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andreas Kling]

Created attachment 218401 [details]
For EWS 2

With border-image fix and matched properties cache enabled.

[Build Bot]

Comment on attachment 218401 [details]
For EWS 2

Attachment 218401 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/43188034

New failing tests:
fast/css3-text/css3-text-decoration/text-decoration-line.html

[Build Bot]

Created attachment 218407 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 218401 [details]
For EWS 2

Attachment 218401 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/42938174

New failing tests:
fast/css3-text/css3-text-decoration/text-decoration-line.html
svg/batik/filters/filterRegions.svg

[Build Bot]

Created attachment 218411 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 218401 [details]
For EWS 2

Attachment 218401 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/39178248

New failing tests:
fast/css3-text/css3-text-decoration/text-decoration-line.html

[Build Bot]

Created attachment 218413 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Andreas Kling]

Created attachment 219568 [details]
Getting there

[WebKit Commit Bot]

Attachment 219568 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/css/StyleResolver.h', '--commit-queue']" exit_code: 1
ERROR: Source/WebCore/css/StyleResolver.h:376:  The parameter name "id" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/css/StyleResolver.h:376:  The parameter name "cssValue" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/css/StyleResolver.h:377:  The parameter name "id" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WebCore/css/StyleResolver.h:377:  The parameter name "cssValue" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 4 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EFL EWS Bot]

Comment on attachment 219568 [details]
Getting there

Attachment 219568 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/48938545

[Antti Koivisto]

Comment on attachment 219568 [details]
Getting there

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

> Source/WebCore/css/StyleResolver.cpp:1741
> +    for (auto& matchedProperties : matchResult.matchedProperties) {
> +        for (unsigned i = 0, count = matchedProperties.properties->propertyCount(); i < count; ++i) {
> +            const auto& property = matchedProperties.properties->propertyAt(i);
> +            if (!property.value()->isPrimitiveValue())
> +                continue;

Hope this crawl is not too expensive. We might want the cache the existence of these properties somewhere for faster testing.

> Source/WebCore/css/StyleResolver.cpp:4214
> +    for (unsigned i = 0, count = properties.propertyCount(); i < count; ++i) {
> +        auto current = properties.propertyAt(i);

Should that be auto& current?

StypeProperties iterator would be nice...

> Source/WebCore/css/StyleResolver.h:362
> +    class CascadedProperties {
> +    public:

Maybe you could just forward declare in the header and define in cpp? This is purely internal after all.

[EFL EWS Bot]

Comment on attachment 219568 [details]
Getting there

Attachment 219568 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/48798083

[Andreas Kling]

Created attachment 219579 [details]
Patch

[Antti Koivisto]

Comment on attachment 219579 [details]
Patch

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

very cool!

> Source/WebCore/css/StyleResolver.cpp:195
> +class StyleResolver::CascadedProperties {
> +public:

I would moved this where it is defined and used.

> Source/WebCore/css/StyleResolver.cpp:1774
> +            const auto& property = matchedProperties.properties->propertyAt(i);

...

> Source/WebCore/css/StyleResolver.cpp:4251
> +        StyleProperties::PropertyReference current = properties.propertyAt(i);

... consistency would be nicer. I guess just 'auto'

[Andreas Kling]

Committed r160806: <http://trac.webkit.org/changeset/160806>

[Simon Fraser (smfr)]

2 tests are crashing:
http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r160806%20(1196)/tables/mozilla/bugs/bug27038-3-crash-log.txt

[Andreas Kling]

(In reply to comment #19)
> 2 tests are crashing:
> http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r160806%20(1196)/tables/mozilla/bugs/bug27038-3-crash-log.txt

Aha. I think we need to bail out to slow path (without matched properties cache) sooner when encountering an explicitly inherited value.

[Andreas Kling]

(In reply to comment #20)
> (In reply to comment #19)
> > 2 tests are crashing:
> > http://build.webkit.org/results/Apple%20Mavericks%20Debug%20WK1%20(Tests)/r160806%20(1196)/tables/mozilla/bugs/bug27038-3-crash-log.txt
> 
> Aha. I think we need to bail out to slow path (without matched properties cache) sooner when encountering an explicitly inherited value.

Fix posted here: https://bugs.webkit.org/show_bug.cgi?id=125968