Bug 46487 – Implement CSSStyleRule::setSelectorText()

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

[Alexander Pavlov (apavlov)]

CSSStyleRule::setSelectorText() is needed for the new upcoming Web Inspector CSS editing approach.

[Alexander Pavlov (apavlov)]

Created attachment 68722 [details]
[PATCH] Suggested solution

The only thing is, I'm not sure about the correct way of notifying the document about the selector change, doc->styleSelectorChanged(DeferRecalcStyle);
Should it perhaps use RecalcStyleImmediately instead?

[Sam Weinig]

Comment on attachment 68722 [details]
[PATCH] Suggested solution

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

Can you add some tests of invalid selectors?

> WebCore/css/CSSStyleRule.cpp:57
> +void CSSStyleRule::setSelectorText(const String& selectorText, ExceptionCode& /*ec*/)

We should remove the ExcpectionCode parameter (and the raises in the IDL file) since the latest spec (http://dev.w3.org/csswg/cssom/#dom-cssstylerule-selectortext) doesn't have it ever raising.

> WebCore/css/CSSStyleRule.cpp:60
> +    CSSParser p;
> +    CSSSelectorList selectorList;

There is no reason to declare these at the top of the function.

[Alexander Pavlov (apavlov)]

Created attachment 68890 [details]
[PATCH] Comments addressed

[Alexander Pavlov (apavlov)]

(In reply to comment #2)
> (From update of attachment 68722 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=68722&action=review
> 
> Can you add some tests of invalid selectors?

Done.

> > WebCore/css/CSSStyleRule.cpp:57
> > +void CSSStyleRule::setSelectorText(const String& selectorText, ExceptionCode& /*ec*/)
> 
> We should remove the ExcpectionCode parameter (and the raises in the IDL file) since the latest spec (http://dev.w3.org/csswg/cssom/#dom-cssstylerule-selectortext) doesn't have it ever raising.

Good point. Fixed "raises" in CSSStyleRule.idl and CSSPageRule.idl

> > WebCore/css/CSSStyleRule.cpp:60
> > +    CSSParser p;
> > +    CSSSelectorList selectorList;
> 
> There is no reason to declare these at the top of the function.

Right, was moving the code around and forgot to follow on these.

[Alexander Pavlov (apavlov)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
        M       LayoutTests/ChangeLog
        A       LayoutTests/fast/css/css-set-selector-text-expected.txt
        A       LayoutTests/fast/css/css-set-selector-text.html
        M       WebCore/ChangeLog
        M       WebCore/css/CSSPageRule.idl
        M       WebCore/css/CSSStyleRule.cpp
        M       WebCore/css/CSSStyleRule.h
        M       WebCore/css/CSSStyleRule.idl
Committed r68388