Bug 77204

Summary: Kill per-Attribute style declarations.
Product: WebKit Reporter: Andreas Kling <kling>
Component: DOMAssignee: Andreas Kling <kling>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, cmarcelo, darin, dglazkov, koivisto, macpherson, menard, pfeldman, rniwa, tkent, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch for EWS and brave people
webkit.review.bot: commit-queue-
Proposed patch
none
Proposed patch koivisto: review+

Description Andreas Kling 2012-01-27 04:57:40 PST
The way style-based-on-attributes works today is by creating a style declaration for each Attribute that affects element style and then adding these one by one in CSSStyleSelector::matchAllRules().
The design is a remnant of the Old Web(tm) where we people couldn't rely on CSS for styling to the extent it's possible today.

I'm proposing that we move away from this approach and instead let attributes affect a single style declaration that hangs on the element, much like a secondary inline "style" attribute. The change is transparent to the web, but frees us from having Attributes that point to style declarations.

This will mean that style declarations generated from attributes are no longer sharable in StyledElement.cpp's CSSMappedAttributeDeclaration table, but I believe it will be a net win in the end.
Comment 1 Andreas Kling 2012-01-27 05:08:04 PST
Created attachment 124293 [details]
Patch for EWS and brave people

Uploading my current WIP, which passes all layout tests, but lacks ChangeLog and a solution for supporting the old behavior of Attr::style() which is exposed as Obj-C API.
Immediate follow-up work after this patch would be removing the "preserveDecls" argument from attributeChanged() and killing CSSMappedAttributeDeclaration, moving the remaining logic into StyledElement's helpers.
Comment 2 WebKit Review Bot 2012-01-27 05:10:54 PST
Attachment 124293 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/dom/CSSMappedAttributeDeclaration.cpp:51:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/dom/CSSMappedAttributeDeclaration.cpp:58:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/dom/CSSMappedAttributeDeclaration.cpp:65:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/dom/CSSMappedAttributeDeclaration.cpp:81:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/html/HTMLHRElement.cpp:104:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 5 in 45 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 WebKit Review Bot 2012-01-27 06:13:19 PST
Comment on attachment 124293 [details]
Patch for EWS and brave people

Attachment 124293 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11265710

New failing tests:
editing/style/make-text-writing-direction-inline.html
inspector/elements/elements-panel-styles.html
inspector/styles/styles-new-API.html
inspector/styles/styles-computed-trace.html
Comment 4 Andreas Kling 2012-02-03 16:12:29 PST
CC'ing some Inspector folks since this will break their feature for displaying applied style per attribute.
Comment 5 Andreas Kling 2012-02-03 16:14:32 PST
Created attachment 125444 [details]
Proposed patch
Comment 6 Andreas Kling 2012-02-03 16:16:22 PST
Created attachment 125445 [details]
Proposed patch

Hm, seems it didn't upload the whole patch..
Comment 7 Benjamin Poulain 2012-02-03 16:52:55 PST
Comment on attachment 125445 [details]
Proposed patch

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

> Source/WebCore/dom/StyledElement.cpp:122
> -void StyledElement::removeCSSProperty(Attribute* attribute, int id)
> +void StyledElement::removeCSSProperties(int id1, int id2, int id3, int id4, int id5, int id6, int id7, int id8)

Objection!
Comment 8 Benjamin Poulain 2012-02-03 16:52:56 PST
Comment on attachment 125445 [details]
Proposed patch

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

> Source/WebCore/dom/StyledElement.cpp:122
> -void StyledElement::removeCSSProperty(Attribute* attribute, int id)
> +void StyledElement::removeCSSProperties(int id1, int id2, int id3, int id4, int id5, int id6, int id7, int id8)

Objection!
Comment 9 Andreas Kling 2012-02-03 16:55:02 PST
(In reply to comment #8)
> (From update of attachment 125445 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125445&action=review
> 
> > Source/WebCore/dom/StyledElement.cpp:122
> > -void StyledElement::removeCSSProperty(Attribute* attribute, int id)
> > +void StyledElement::removeCSSProperties(int id1, int id2, int id3, int id4, int id5, int id6, int id7, int id8)
> 
> Objection!

Yeah, fine. That's horrible. I'll expand into multiple removeCSSProperty() calls again.
Comment 10 Ryosuke Niwa 2012-02-03 16:56:31 PST
Comment on attachment 125445 [details]
Proposed patch

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

>>> Source/WebCore/dom/StyledElement.cpp:122
>>> +void StyledElement::removeCSSProperties(int id1, int id2, int id3, int id4, int id5, int id6, int id7, int id8)
>> 
>> Objection!
> 
> Objection!

I think it's okay to enumerate ids like this but you may also consider using an array here.
Comment 11 Antti Koivisto 2012-02-04 02:27:46 PST
Comment on attachment 125445 [details]
Proposed patch

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

> Source/WebCore/css/CSSStyleSelector.cpp:1006
> +                result.isCacheable = false;

Caching is useful even when properties are not shared between elements as it is common for the same element style to get recomputed.

We should be able to share identical attribute styles between elements too.

> Source/WebCore/dom/Attr.h:62
> +    // A deprecated extension to get presentational information for attributes.
> +    // We have to keep it around because it's exposed in the Obj-C DOM API.
> +    CSSStyleDeclaration* style() { return 0; }

Hope we don't have clients for this!

> Source/WebCore/dom/CSSMappedAttributeDeclaration.h:-76
>  
>      RefPtr<StylePropertySet> m_declaration;
> -    MappedAttributeEntry m_entryType;
> -    QualifiedName m_attrName;
> -    AtomicString m_attrValue;

This class has kinda lost its reason to exist.
Comment 12 Antti Koivisto 2012-02-04 02:55:01 PST
Comment on attachment 125445 [details]
Proposed patch

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

> Source/WebCore/dom/StyledElement.cpp:144
> +    if (id2 != CSSPropertyInvalid)
> +        style->removeMappedProperty(this, id2);
> +    if (id3 != CSSPropertyInvalid)
> +        style->removeMappedProperty(this, id3);
> +    if (id4 != CSSPropertyInvalid)
> +        style->removeMappedProperty(this, id4);
> +    if (id5 != CSSPropertyInvalid)
> +        style->removeMappedProperty(this, id5);
> +    if (id6 != CSSPropertyInvalid)
> +        style->removeMappedProperty(this, id6);
> +    if (id7 != CSSPropertyInvalid)
> +        style->removeMappedProperty(this, id7);
> +    if (id8 != CSSPropertyInvalid)
> +        style->removeMappedProperty(this, id8);

I think you can bail out when you see the first CSSPropertyInvalid.
Comment 13 Andreas Kling 2012-02-04 04:24:32 PST
Committed r106740: <http://trac.webkit.org/changeset/106740>
Comment 14 Andreas Kling 2012-02-04 13:32:03 PST
Committed r106741: <http://trac.webkit.org/changeset/106741>
Comment 15 Darin Adler 2012-02-06 16:59:56 PST
(In reply to comment #11)
> > Source/WebCore/dom/Attr.h:62
> > +    // A deprecated extension to get presentational information for attributes.
> > +    // We have to keep it around because it's exposed in the Obj-C DOM API.
> > +    CSSStyleDeclaration* style() { return 0; }
> 
> Hope we don't have clients for this!

If nobody is using this from Objective-C we can remove it from the Objective-C DOM API.

If we want a method that just returns nil we can set that up in Mac WebKit with a category or something like that and remove it from WebCore.

On reflection, it’s a seriously bad thing that we export Attr as part of the ObjC DOM.