Bug 79092 – Remove stylesheet pointer from StylePropertySet

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

[Antti Koivisto]

The context should be passed as parameter for CSS parser invoking setters that actually need it.

[Antti Koivisto]

Created attachment 127945 [details]
patch

[Andreas Kling]

Comment on attachment 127945 [details]
patch

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

r=me with some glorious exposition:

> Source/WebCore/css/CSSParser.cpp:349
> +    Document* document = contextStyleSheet->findDocument();

Are we sure it's safe to always dereference 'contextStyleSheet' here? An assertion might be nice.

> Source/WebCore/css/CSSParser.cpp:473
> +    Document* document = contextStyleSheet->findDocument();

Same story.

> Source/WebCore/css/WebKitCSSMatrix.cpp:56
> -    if (CSSParser::parseValue(styleDeclaration.get(), CSSPropertyWebkitTransform, string, true, true)) {
> +    if (CSSParser::parseValue(styleDeclaration.get(), CSSPropertyWebkitTransform, string, true, true, 0)) {

Here's a call site where parseValue() would have a null contextStyleSheet.
Though it won't be a problem for CSSPropertyWebkitTransform, it sets a bad precedent.

> Source/WebCore/dom/StyledElement.h:72
> +    void applyPresentationAttributeToStyle(StylePropertySet*, int propertyID, int value);
> +    void applyPresentationAttributeToStyle(StylePropertySet*, int propertyID, double value, CSSPrimitiveValue::UnitTypes);
> +    void applyPresentationAttributeToStyle(StylePropertySet*, int propertyID, const String& value);

This name is not entirely fabulous, as the function is only concerned with a single property at a time. addPropertyToAttributeStyle() perhaps?

[WebKit Review Bot]

Comment on attachment 127945 [details]
patch

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

New failing tests:
fast/dom/css-delete-doc.html

[Antti Koivisto]

http://trac.webkit.org/changeset/108345

- Renamed to addPropertyToAttributeStyle as suggested
- Fixed handling of null context stylesheet (fixes the test crash too)

[Csaba Osztrogonác]

Reopen, because it broke the Qt build:
cc1plus: warnings being treated as errors
../../../../Source/WebCore/css/CSSParser.cpp: In static member function 'static bool WebCore::CSSParser::parseValue(WebCore::StylePropertySet*, int, const WTF::String&, bool, bool, WebCore::CSSStyleSheet*)':
../../../../Source/WebCore/css/CSSParser.cpp:472: error: 'value$m_ptr' may be used uninitialized in this function
../../../../Source/WebCore/css/CSSParser.cpp:472: note: 'value$m_ptr' was declared here

[Antti Koivisto]

(In reply to comment #5)
> Reopen, because it broke the Qt build:
> cc1plus: warnings being treated as errors
> ../../../../Source/WebCore/css/CSSParser.cpp: In static member function 'static bool WebCore::CSSParser::parseValue(WebCore::StylePropertySet*, int, const WTF::String&, bool, bool, WebCore::CSSStyleSheet*)':
> ../../../../Source/WebCore/css/CSSParser.cpp:472: error: 'value$m_ptr' may be used uninitialized in this function
> ../../../../Source/WebCore/css/CSSParser.cpp:472: note: 'value$m_ptr' was declared here

That looks like a problem with the compiler. Not sure how to work around that. Maybe just different warning flags.

[Csaba Osztrogonác]

Created attachment 127961 [details]
workaround for this build breakage

I don't think if we should disable "-Wuninitialized" to fix only this situation.
This warning hits with gcc 4.4.5 (Debian Squeeze)

[Csaba Osztrogonác]

(In reply to comment #7)
> Created an attachment (id=127961) [details]
> workaround for this build breakage
> 
> I don't think if we should disable "-Wuninitialized" to fix only this situation.
> This warning hits with gcc 4.4.5 (Debian Squeeze)

Landed in http://trac.webkit.org/changeset/108351/trunk/Source/WebCore/css/CSSParser.cpp

But it seems there are more similar bugs with gcc 4.5.2 (Ubuntu 11.04)

[Alexander Færøy]

Created attachment 127969 [details]
Patch

[Csaba Osztrogonác]

Comment on attachment 127969 [details]
Patch

r=me, I'll land it manually.

[Csaba Osztrogonác]

Comment on attachment 127969 [details]
Patch

Clearing flags on attachment: 127969

Committed r108352: <http://trac.webkit.org/changeset/108352>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.