Bug 79092

Created attachment 127945 [details] patch

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?

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

http://trac.webkit.org/changeset/108345 - Renamed to addPropertyToAttributeStyle as suggested - Fixed handling of null context stylesheet (fixes the test crash too)

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

(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.

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)

(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)

Created attachment 127969 [details] Patch

Comment on attachment 127969 [details] Patch r=me, I'll land it manually.

Comment on attachment 127969 [details] Patch Clearing flags on attachment: 127969 Committed r108352: <http://trac.webkit.org/changeset/108352>

All reviewed patches have been landed. Closing bug.