Bug 125213 - CSS: Add a property cascading pass to style application.
Summary: CSS: Add a property cascading pass to style application.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-12-04 02:32 PST by Andreas Kling
Modified: 2013-12-18 18:38 PST (History)
13 users (show)

See Also:


Attachments
For EWS (5.45 KB, patch)
2013-12-04 02:34 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
For EWS (13.99 KB, patch)
2013-12-04 02:34 PST, Andreas Kling
no flags Details | Formatted Diff | Diff
For EWS 2 (16.51 KB, patch)
2013-12-04 05:04 PST, Andreas Kling
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion (669.06 KB, application/zip)
2013-12-04 07:11 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (711.75 KB, application/zip)
2013-12-04 08:02 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (682.54 KB, application/zip)
2013-12-04 09:40 PST, Build Bot
no flags Details
Getting there (20.04 KB, patch)
2013-12-18 14:12 PST, Andreas Kling
eflews.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (23.32 KB, patch)
2013-12-18 16:00 PST, Andreas Kling
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2013-12-04 02:32:35 PST
CSS: Add a property cascading pass to style application.
Comment 1 Andreas Kling 2013-12-04 02:34:04 PST
Created attachment 218391 [details]
For EWS

A snack for EWS to see where this is at. Matched properties cache is broken and disabled.
Comment 2 Andreas Kling 2013-12-04 02:34:30 PST
Created attachment 218392 [details]
For EWS
Comment 3 WebKit Commit Bot 2013-12-04 02:36:55 PST
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.
Comment 4 Andreas Kling 2013-12-04 05:04:44 PST
Created attachment 218401 [details]
For EWS 2

With border-image fix and matched properties cache enabled.
Comment 5 Build Bot 2013-12-04 07:11:37 PST
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
Comment 6 Build Bot 2013-12-04 07:11:39 PST
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
Comment 7 Build Bot 2013-12-04 08:02:27 PST
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
Comment 8 Build Bot 2013-12-04 08:02:30 PST
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
Comment 9 Build Bot 2013-12-04 09:40:18 PST
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
Comment 10 Build Bot 2013-12-04 09:40:20 PST
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
Comment 11 Andreas Kling 2013-12-18 14:12:17 PST
Created attachment 219568 [details]
Getting there
Comment 12 WebKit Commit Bot 2013-12-18 14:17:53 PST
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.
Comment 13 EFL EWS Bot 2013-12-18 14:21:06 PST
Comment on attachment 219568 [details]
Getting there

Attachment 219568 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/48938545
Comment 14 Antti Koivisto 2013-12-18 14:32:13 PST
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.
Comment 15 EFL EWS Bot 2013-12-18 14:39:32 PST
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
Comment 16 Andreas Kling 2013-12-18 16:00:22 PST
Created attachment 219579 [details]
Patch
Comment 17 Antti Koivisto 2013-12-18 16:12:36 PST
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'
Comment 18 Andreas Kling 2013-12-18 16:30:25 PST
Committed r160806: <http://trac.webkit.org/changeset/160806>
Comment 20 Andreas Kling 2013-12-18 18:22:08 PST
(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.
Comment 21 Andreas Kling 2013-12-18 18:38:18 PST
(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