Bug 79939

Summary: [WIP] Implement CSS Hierarchies
Product: WebKit Reporter: Shane Stephens <shanestephens>
Component: CSSAssignee: Shane Stephens <shanestephens>
Status: RESOLVED LATER    
Severity: Normal CC: cmarcelo, dglazkov, divya, donggwan.kim, eoconnor, gyuyoung.kim, kling, koivisto, macpherson, menard, mike, ojan, paulirish, peter, rakuco, sam, syoichi, tony, vestbo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 92433, 93574, 93664, 94099    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch webkit.review.bot: commit-queue-

Description Shane Stephens 2012-02-29 13:28:48 PST
[WIP] Implement CSS Hierarchies
Comment 1 Shane Stephens 2012-02-29 13:41:09 PST
Created attachment 129509 [details]
Patch
Comment 2 Shane Stephens 2012-03-08 03:20:14 PST
This CSS feature is proposed in the following Editor's Draft: http://dev.w3.org/csswg/css3-hierarchies/, and has been discussed by the CSSWG (the group is largely favorable to the idea). It's currently a WIP. I'd appreciate feedback on whether this is the correct approach for a feature like this.
Comment 3 Luke Macpherson 2012-03-19 19:36:45 PDT
Comment on attachment 129509 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:8833
> +        rule->styleRule()->adoptSelectorVector(*selectors, m_selectorVectorStack);

perhaps m_nestedSelectorStack would be a good alternative name.

> Source/WebCore/css/CSSParser.h:268
> +    Vector<OwnPtr<CSSParserSelector> >* reusableSelectorVector() { return m_reusableSelectorVector; }

resusableSelectorVector -> selectorVector (no longer re-usable).

> Source/WebCore/css/CSSParser.h:270
> +    void pushSelectorVector(bool trailingSpace)

pushNestedSelector()

> Source/WebCore/css/CSSParser.h:283
> +    void popSelectorVector()

popNestedSelector()

> Source/WebCore/css/CSSParser.h:455
> +    Vector<OwnPtr<CSSParserSelector> > *m_reusableSelectorVector;

OwnPtr<Vector<OwnPtr<CSSParserSelector> > > here.

> Source/WebCore/css/CSSParserValues.cpp:109
> +void CSSParserSelector::adoptSelectorVector(Vector<OwnPtr<CSSParserSelector> >& selectorVector, Vector<Vector<OwnPtr<CSSParserSelector> >* >& stackedSelectors)

typedef-ing Vector<OwnPtr<CSSParserSelector> > CSSParserSelectorList might make some of this code more readable.

> Source/WebCore/css/CSSSelector.h:34
> +    class CSSSelectorListWrapper :public RefCounted<CSSSelectorListWrapper> {

space before public - also rename to RefcountedCSSSelectorList or similar.

> Source/WebCore/css/CSSSelector.h:50
> +            CSSSelectorListWrapper(CSSSelectorList* impl)

one-liner.

> Source/WebCore/css/CSSSelector.h:253
> +        CSSSelectorList* selectorList() const { return m_hasRareData && m_data.m_rareData->m_selectorList.get() ? m_data.m_rareData->m_selectorList->get() : 0; }

Remove the first .get to make this code clearer. Second get could probably be renamed too.
Comment 4 Shane Stephens 2012-03-21 19:35:40 PDT
Created attachment 133171 [details]
Patch
Comment 5 Shane Stephens 2012-03-21 19:38:34 PDT
Comment on attachment 129509 [details]
Patch

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

>> Source/WebCore/css/CSSParser.h:455
>> +    Vector<OwnPtr<CSSParserSelector> > *m_reusableSelectorVector;
> 
> OwnPtr<Vector<OwnPtr<CSSParserSelector> > > here.

Done.

>> Source/WebCore/css/CSSParserValues.cpp:109
>> +void CSSParserSelector::adoptSelectorVector(Vector<OwnPtr<CSSParserSelector> >& selectorVector, Vector<Vector<OwnPtr<CSSParserSelector> >* >& stackedSelectors)
> 
> typedef-ing Vector<OwnPtr<CSSParserSelector> > CSSParserSelectorList might make some of this code more readable.

Done.

>> Source/WebCore/css/CSSSelector.h:34
>> +    class CSSSelectorListWrapper :public RefCounted<CSSSelectorListWrapper> {
> 
> space before public - also rename to RefcountedCSSSelectorList or similar.

Done.

>> Source/WebCore/css/CSSSelector.h:50
>> +            CSSSelectorListWrapper(CSSSelectorList* impl)
> 
> one-liner.

Done.

>> Source/WebCore/css/CSSSelector.h:253
>> +        CSSSelectorList* selectorList() const { return m_hasRareData && m_data.m_rareData->m_selectorList.get() ? m_data.m_rareData->m_selectorList->get() : 0; }
> 
> Remove the first .get to make this code clearer. Second get could probably be renamed too.

Done. Decided not to rename the second 'get'.
Comment 6 WebKit Review Bot 2012-03-21 19:39:39 PDT
Attachment 133171 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
Source/WebCore/css/CSSSelectorList.cpp:115:  Semicolon defining empty statement for this loop. Use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 Shane Stephens 2012-03-21 19:40:32 PDT
I think this patch might be ready for a reviewer to look at it. Style failure is due to using a 

do {
..
} while(..);

construct.
Comment 8 Build Bot 2012-03-21 19:55:55 PDT
Comment on attachment 133171 [details]
Patch

Attachment 133171 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/12086752
Comment 9 Shane Stephens 2012-03-21 22:02:57 PDT
Created attachment 133186 [details]
Patch
Comment 10 WebKit Review Bot 2012-03-21 22:06:37 PDT
Attachment 133186 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
Source/WebCore/css/CSSSelectorList.cpp:115:  Semicolon defining empty statement for this loop. Use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Sam Weinig 2012-04-01 19:40:45 PDT
This absolutely needs to be reviewed by Antti before it goes in.
Comment 12 Shane Stephens 2012-04-02 15:13:04 PDT
It'd be great if Antti could do a review!
Comment 13 Andreas Kling 2012-04-03 13:16:25 PDT
Yeah, this is definitely for Antti.
Comment 14 Alexis Menard (darktears) 2012-04-03 13:21:59 PDT
Comment on attachment 133186 [details]
Patch

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

> Source/WebCore/css/CSSParser.h:311
> +    OwnPtr<Vector<CSSProperty, 256> > m_parsedProperties;

I'm wondering is this change was really needed right now. It make the diff noisy.
Comment 15 Shane Stephens 2012-04-03 14:56:04 PDT
Comment on attachment 133186 [details]
Patch

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

>> Source/WebCore/css/CSSParser.h:311
>> +    OwnPtr<Vector<CSSProperty, 256> > m_parsedProperties;
> 
> I'm wondering is this change was really needed right now. It make the diff noisy.

I needed to convert m_parsedProperties from an in-instance-data value to a pointer to a value, because it needs to be pushed onto and popped off m_stackedParsedProperties (see pushNestedSelector and popNestedSelector above). It seemed sensible to make it an OwnPtr<Vector<CSSProperty, 256> > instead of a naked Vector<CSSProperty, 256>*.
Comment 16 Divya Manian 2012-04-19 20:10:22 PDT
+1 for Antti's review.
Comment 17 Ojan Vafai 2012-07-18 10:25:04 PDT
Comment on attachment 133186 [details]
Patch

Have you considered instead creating a concept of a NestedCSSStyleRule that has a pointer to a parent rule and then having matchSelector know about parent style rules? Then you don't need to do the expanding. I think that approach would make it a ton easier to make the CSSOM bits work eventually.
Comment 18 Shane Stephens 2012-07-18 17:54:44 PDT
Yes, I have considered this approach. I also tried to implement it, and eventually convinced myself and Dimitri Glaskov that it was not appropriate.

The issue is that different combinators or pseudo-classes have different expansion rules - e.g. 
.a .b is re-ordered to select on .b, then check for .a; whereas .a:hover selects on .a, then checks for :hover. These expansions can exist across nesting boundaries (e.g. .a { & .b { ... } } and .a { &:hover { ... { } }). Because nesting can be arbitrarily deep, the expansion can cause quite complex re-ordering. Furthermore, multiple re-orderings can be required for a single set of nested rules (e.g. .a { & .b, &:hover { & .c, &:active {...} } }.

Have a look at the expansion code. Essentially this logic would need to be implemented recursively in the selector matching code if we were to use a NestedCSSStyleRule + pointer approach.
Comment 19 Shane Stephens 2012-07-25 22:34:15 PDT
Created attachment 154542 [details]
Patch
Comment 20 WebKit Review Bot 2012-07-25 22:38:01 PDT
Attachment 154542 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
Source/WebCore/css/CSSSelectorList.cpp:136:  Semicolon defining empty statement for this loop. Use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Tony Chang 2012-07-26 10:26:46 PDT
Comment on attachment 154542 [details]
Patch

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

I mostly just skimmed the style in this patch.

I think getting matchSelector to work would be better in the long run, although harder to implement.  For example, you could probably avoid the exponential memory use, although it would still be slow.

> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:42
> +ENABLE_CSS_HIERARCHIES = ;

I would make a separate patch that adds the compiler flag and updates all the build systems.

> Source/WebCore/css/CSSSelectorList.cpp:68
> +    if (stackedSelectors.size() > 0)

!stackedSelectors.isEmpty()

> Source/WebCore/css/CSSSelectorList.cpp:81
> +    if (flattenedSize == 1 && (!stackedSelectors.size())) {

Nit: stackedSelectors.isEmpty()

> Source/WebCore/css/CSSSelectorList.cpp:90
> +    // Flatten the selector hierarchy into explodedSelectors. Each explodedSelectors vector is a single
> +    // element from the cartesian product of the sets of selectors at each level in the hierarchy.

It seems like you could easily overflow the heap and explodedResultsSize (e.g., 64 nested hierarchies each with 2 rules).  Should we limit this somehow?

> Source/WebCore/css/CSSSelectorList.cpp:93
> +    for (size_t i = 0; i < stackedSelectors.size(); i++)

Nit: ++i

> Source/WebCore/css/CSSSelectorList.cpp:96
> +    Vector<OwnPtr<CSSSelector> >* explodedSelectors = new Vector<OwnPtr<CSSSelector> >[explodedResultsSize];

Can we use an OwnArrayPtr rather than manually calling delete []?

> Source/WebCore/css/CSSSelectorList.cpp:97
> +    size_t* insertLocations = new size_t[explodedResultsSize];

Can we use a Vector<size_t> here?

> Source/WebCore/css/CSSSelectorList.cpp:98
> +    size_t considered = 1;

Please move this down to where it is assigned (line 109).

> Source/WebCore/css/CSSSelectorList.cpp:102
> +    for (size_t j = 0; j < explodedResultsSize; j++) {
> +        size_t i = j % selectorVector.size();

Please use more descriptive variable names than i and j.  E.g., explodedIndex and selectorIndex,.

> Source/WebCore/css/CSSSelectorList.cpp:121
> +    for (int i = stackedSelectors.size() - 1; i >= 0; i--) {
> +        CSSSelectorVector& currentInStack = *(stackedSelectors[i]);
> +        for (size_t j = 0; j < explodedResultsSize; j++) {
> +            size_t use = (j / considered) % currentInStack.size();

Please use more descriptive variable names than i, j and use.

> Source/WebCore/css/CSSSelectorList.cpp:132
> +                int k = 0;

Please use a better variable name than k.

> Source/WebCore/css/CSSSelectorList.cpp:142
> +    considered *= currentInStack.size();

Nit: Bad indent.

> LayoutTests/css3/simple-hierarchies.html:97
> +if (layoutTestController) {
> +  layoutTestController.dumpAsText();

Nit: layoutTestController -> testRunner.

Why does the expected result have a render tree dump if this is a dumpAsText test?  I would probably make this a JS test and use getComputedStyle to verify that the right style is being applied.
Comment 22 Shane Stephens 2012-07-26 17:43:21 PDT
Comment on attachment 154542 [details]
Patch

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

>> Source/JavaScriptCore/Configurations/FeatureDefines.xcconfig:42
>> +ENABLE_CSS_HIERARCHIES = ;
> 
> I would make a separate patch that adds the compiler flag and updates all the build systems.

Done (92433)

>> Source/WebCore/css/CSSSelectorList.cpp:68
>> +    if (stackedSelectors.size() > 0)
> 
> !stackedSelectors.isEmpty()

done

>> Source/WebCore/css/CSSSelectorList.cpp:81
>> +    if (flattenedSize == 1 && (!stackedSelectors.size())) {
> 
> Nit: stackedSelectors.isEmpty()

done

>> Source/WebCore/css/CSSSelectorList.cpp:90
>> +    // element from the cartesian product of the sets of selectors at each level in the hierarchy.
> 
> It seems like you could easily overflow the heap and explodedResultsSize (e.g., 64 nested hierarchies each with 2 rules).  Should we limit this somehow?

I've added a check against static const value so that we don't accumulate more than 2^16 exploded results.

>> Source/WebCore/css/CSSSelectorList.cpp:93
>> +    for (size_t i = 0; i < stackedSelectors.size(); i++)
> 
> Nit: ++i

Done.

>> Source/WebCore/css/CSSSelectorList.cpp:96
>> +    Vector<OwnPtr<CSSSelector> >* explodedSelectors = new Vector<OwnPtr<CSSSelector> >[explodedResultsSize];
> 
> Can we use an OwnArrayPtr rather than manually calling delete []?

Done.

>> Source/WebCore/css/CSSSelectorList.cpp:97
>> +    size_t* insertLocations = new size_t[explodedResultsSize];
> 
> Can we use a Vector<size_t> here?

I ended up using an OwnArrayPtr here too, as we know the size up-front.

>> Source/WebCore/css/CSSSelectorList.cpp:98
>> +    size_t considered = 1;
> 
> Please move this down to where it is assigned (line 109).

Done.

>> Source/WebCore/css/CSSSelectorList.cpp:102
>> +        size_t i = j % selectorVector.size();
> 
> Please use more descriptive variable names than i and j.  E.g., explodedIndex and selectorIndex,.

Done.

>> Source/WebCore/css/CSSSelectorList.cpp:121
>> +            size_t use = (j / considered) % currentInStack.size();
> 
> Please use more descriptive variable names than i, j and use.

Done.

>> Source/WebCore/css/CSSSelectorList.cpp:132
>> +                int k = 0;
> 
> Please use a better variable name than k.

Done.

>> Source/WebCore/css/CSSSelectorList.cpp:136
>> +                while (relation == CSSSelector::SubSelector);
> 
> Semicolon defining empty statement for this loop. Use { } instead.  [whitespace/semicolon] [5]

What should I do about this? It's neater to use a do .. while loop, but this style error is triggered.

>> Source/WebCore/css/CSSSelectorList.cpp:142
>> +    considered *= currentInStack.size();
> 
> Nit: Bad indent.

Done.

>> LayoutTests/css3/simple-hierarchies.html:97
>> +  layoutTestController.dumpAsText();
> 
> Nit: layoutTestController -> testRunner.
> 
> Why does the expected result have a render tree dump if this is a dumpAsText test?  I would probably make this a JS test and use getComputedStyle to verify that the right style is being applied.

Removed. It looks like this wasn't running because there wasn't a </script>. Yuck.

WRT JS test vs how this is now - isn't this a bit cleaner as we don't rely on any JS to do the comparisons? color and bgcolor are both included in the render tree dump.
Comment 23 Shane Stephens 2012-07-26 18:08:47 PDT
Created attachment 154799 [details]
Patch
Comment 24 WebKit Review Bot 2012-07-26 18:14:06 PDT
Attachment 154799 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
Source/WebCore/css/CSSSelectorList.cpp:141:  Semicolon defining empty statement for this loop. Use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 25 Tony Chang 2012-07-27 10:27:53 PDT
Comment on attachment 154799 [details]
Patch

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

Some of the code changes (like typedefs for CSSSelectorVector and CSSSelectorVectorList and converting m_parsedProperties to an OwnPtr) make this diff harder to review.  You may want to move them into a separate change or revert them.

> Source/WebCore/css/CSSParser.h:245
> +    PassOwnPtr<CSSSelectorVector > sinkFloatingSelectorVector(CSSSelectorVector*);

Nit: Extra space before >

> Source/WebCore/css/CSSSelectorList.cpp:99
> +        if (explodedResultsSize > maxExplodedResults)
> +            return;

Can you add a test case that hits this?

>> Source/WebCore/css/CSSSelectorList.cpp:141
>> +                while (relation == CSSSelector::SubSelector);
> 
> Semicolon defining empty statement for this loop. Use { } instead.  [whitespace/semicolon] [5]

"If any of these errors are false positives, please file a bug against check-webkit-style."  I would say that this is a false positive.

> LayoutTests/css3/simple-hierarchies-expected.txt:2
> +layer at (0,0) size 800x600
> +  RenderView at (0,0) size 800x600

> WRT JS test vs how this is now - isn't this a bit cleaner as we don't rely on any JS to do the comparisons? color and bgcolor are both included in the render tree dump.

You'll have to check in a separate rendertree output for every port and platform because text sizes aren't exactly the same.  You also will need pixel dumps for each platform.  It's harder to quickly inspect that the test is correct rather than a series of PASS/FAIL being displayed.

I think it would also be fine to have a ref-test to verify that the correct colors are being painted, but it seems like if getComputedStyle and what is painted are different, that's a bug in some other part of the code, not something you introduced.
Comment 26 Shane Stephens 2012-08-07 00:01:10 PDT
Created attachment 156881 [details]
Patch
Comment 27 WebKit Review Bot 2012-08-07 00:03:08 PDT
Attachment 156881 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
Source/WebCore/css/CSSSelectorList.cpp:140:  Semicolon defining empty statement for this loop. Use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 28 Shane Stephens 2012-08-07 00:04:43 PDT
Comment on attachment 154799 [details]
Patch

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

>> Source/WebCore/css/CSSParser.h:245
>> +    PassOwnPtr<CSSSelectorVector > sinkFloatingSelectorVector(CSSSelectorVector*);
> 
> Nit: Extra space before >

Done

>> Source/WebCore/css/CSSSelectorList.cpp:99
>> +            return;
> 
> Can you add a test case that hits this?

Done

>>> Source/WebCore/css/CSSSelectorList.cpp:141
>>> +                while (relation == CSSSelector::SubSelector);
>> 
>> Semicolon defining empty statement for this loop. Use { } instead.  [whitespace/semicolon] [5]
> 
> "If any of these errors are false positives, please file a bug against check-webkit-style."  I would say that this is a false positive.

Done
Comment 29 Shane Stephens 2012-08-07 00:05:40 PDT
I've also fixed the layout test to use dumpAsText and check the computedStyle.
Comment 30 Gyuyoung Kim 2012-08-07 00:20:59 PDT
Comment on attachment 156881 [details]
Patch

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

> Source/WebCore/css/CSSPageRule.h:-48
> -

IMHO, this line removal is unrelated to this patch.

> Source/WebCore/css/CSSParser.h:315
> +        StackedParsedProperties *stackHead = m_stackedParsedProperties.last();

I think this is wrong '*' place.

> Source/WebCore/css/CSSParser.h:358
> +    Vector<StackedParsedProperties *> m_stackedParsedProperties;

According to my quick search, WebKit coding style didn't use a space between Vector<...>

> Source/WebCore/css/CSSSelector.h:49
> +            CSSSelectorList *m_impl;

This look wrong '*' place.

> Source/WebCore/css/CSSSelector.h:61
> +        CSSSelector(CSSSelector* copy);

It looks *explicit* is missing.

> Source/WebCore/css/CSSSelectorList.cpp:27
> +

IMHO, this is unneeded line.

> LayoutTests/platform/qt/TestExpectations:87
> +BUGWK79939 SKIP : css3/simple-hierarchies.html = PASS

Could you add this skip to LayoutTests/platform/efl/TestExpectations as well ?
Comment 31 Shane Stephens 2012-08-07 16:43:57 PDT
Comment on attachment 156881 [details]
Patch

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

>> Source/WebCore/css/CSSPageRule.h:-48
>> -
> 
> IMHO, this line removal is unrelated to this patch.

removed.

>> Source/WebCore/css/CSSParser.h:315
>> +        StackedParsedProperties *stackHead = m_stackedParsedProperties.last();
> 
> I think this is wrong '*' place.

Fixed.

>> Source/WebCore/css/CSSParser.h:358
>> +    Vector<StackedParsedProperties *> m_stackedParsedProperties;
> 
> According to my quick search, WebKit coding style didn't use a space between Vector<...>

Do you mean that this should be Vector<StackedParsedProperties*>? I've fixed it, if so.

>> Source/WebCore/css/CSSSelector.h:49
>> +            CSSSelectorList *m_impl;
> 
> This look wrong '*' place.

Fixed.

>> Source/WebCore/css/CSSSelector.h:61
>> +        CSSSelector(CSSSelector* copy);
> 
> It looks *explicit* is missing.

Added.

>> Source/WebCore/css/CSSSelectorList.cpp:27
>> +
> 
> IMHO, this is unneeded line.

Fixed.

>> LayoutTests/platform/qt/TestExpectations:87
>> +BUGWK79939 SKIP : css3/simple-hierarchies.html = PASS
> 
> Could you add this skip to LayoutTests/platform/efl/TestExpectations as well ?

Added.
Comment 32 Shane Stephens 2012-08-07 17:05:32 PDT
Created attachment 157048 [details]
Patch
Comment 33 WebKit Review Bot 2012-08-07 17:08:14 PDT
Attachment 157048 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/css3..." exit_code: 1
Source/WebCore/css/CSSSelectorList.cpp:139:  Semicolon defining empty statement for this loop. Use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 34 Tony Chang 2012-08-08 21:33:32 PDT
Comment on attachment 157048 [details]
Patch

For those watching this bug:
Shane has been emailing with Antti and Antti's main concern is that the changes to the CSS grammar, parser and data structures are significant and will make understanding this code harder for everyone.

However, he doesn't feel strongly enough that this should block the feature completely, just that we consider it.

The plan going forward is to break this into multiple patches to make it easier to review and to try to make the changes less invasive.
Comment 35 WebKit Review Bot 2012-08-09 17:26:49 PDT
Comment on attachment 157048 [details]
Patch

Attachment 157048 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13470410
Comment 36 Antti Koivisto 2012-08-10 05:22:35 PDT
(In reply to comment #34)
> (From update of attachment 157048 [details])
> For those watching this bug:
> Shane has been emailing with Antti and Antti's main concern is that the changes to the CSS grammar, parser and data structures are significant and will make understanding this code harder for everyone.

My main concern was really that this feature is in very early stages in the CSSWG doesn't seem that great in general. It would be far less disruptive to experiment in a branch somewhere rather than in mainline WebKit.
Comment 37 Antti Koivisto 2012-08-10 05:23:37 PDT
*and doesn't seem
Comment 38 Antti Koivisto 2012-10-29 09:14:46 PDT
CSS WG conducted a prioritization poll. Hierarchies was placed 47th out of 58 features with 4 very strong/strong interest votes vs. 16 low/no interest votes.

http://disruptive-innovations.com/zoo/customers/CSSWG/Priorities.html

Based on this I don't think this feature should go into WebKit at this time.
Comment 39 Antti Koivisto 2012-10-29 09:16:44 PDT
s/16/12/
Comment 40 Shane Stephens 2012-10-29 15:58:31 PDT
That list is intended as an indication of where to allocate effort when time grows short at CSSWG face-to-face meetings, and should not be used as a prioritization of implementation effort for feature work. A salient example: "sticky positioning" (http://lists.w3.org/Archives/Public/www-style/2012Jun/0627.html, https://bugs.webkit.org/show_bug.cgi?id=90046) is not even included on a specification in that list yet, but is undergoing active development, behind a flag.

Some of the discussion at the WG has been around rolling hierarchies in as a part of Selectors level 4 or 5, which would remove the need for a separate specification. Perhaps this is part of the reason that the _specification_ has been given low priority? Certainly discussions of the _feature_ have engendered a general level of acceptance at the WG.
Comment 41 Tony Chang 2012-11-14 15:45:20 PST
(In reply to comment #38)
> CSS WG conducted a prioritization poll. Hierarchies was placed 47th out of 58 features with 4 very strong/strong interest votes vs. 16 low/no interest votes.
> 
> http://disruptive-innovations.com/zoo/customers/CSSWG/Priorities.html
> 
> Based on this I don't think this feature should go into WebKit at this time.

I talked to Shane off list and we've agreed to halt development of this feature at this time.  I'd like to see buy-in from at least one other browser vendor.

I'll post some patches to revert the incomplete work.

Thanks!