Bug 101080 – Color-profile property triggers assert

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Florin Malita]

Created attachment 172097 [details]
Asserts on debug builds.

Looks like we're not handling CSSPropertyColorProfile in StyleResolver::applySVGProperty():

STDERR: ASSERTION FAILED: unimplemented propertyID: 1357
STDERR: 0
STDERR: ../../third_party/WebKit/Source/WebCore/css/SVGCSSStyleSelector.cpp(596) : void WebCore::StyleResolver::applySVGProperty(WebCore::CSSPropertyID, WebCore::CSSValue*)


Chromium issue: http://code.google.com/p/chromium/issues/detail?id=157492

[Florin Malita]

Created attachment 172101 [details]
Patch

[WebKit Review Bot]

Attachment 172101 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/rendering/style/SVGRenderStyle.h:409:  _colorProfile is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
Total errors found: 1 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Florin Malita]

(In reply to comment #2)
> Source/WebCore/rendering/style/SVGRenderStyle.h:409:  _colorProfile is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]

Just following the local style there.

[Dirk Schulze]

Comment on attachment 172101 [details]
Patch

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

Since we don't handle it at all, why not do the same like for background-enable:

        case CSSPropertyEnableBackground:
            // Silently ignoring this property for now

Some might use at as feature detection. I would rather not enable it in CSSOM, when we don't support it at all.

>> Source/WebCore/rendering/style/SVGRenderStyle.h:409
>> +        unsigned _colorProfile : 1; // EColorProfile
> 
> _colorProfile is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]

Well, it should still be fixed IMHO. Maybe in a preparation patch?

[Florin Malita]

(In reply to comment #4)
> (From update of attachment 172101 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172101&action=review
> 
> Since we don't handle it at all, why not do the same like for background-enable:
> 
>         case CSSPropertyEnableBackground:
>             // Silently ignoring this property for now

Since I thought we'll eventually add color-profile support, I figured it's best to fix the crash by building the required plumbing.


> Some might use at as feature detection. I would rather not enable it in CSSOM, when we don't support it at all.

Ah, that makes sense. I'll update the patch.

 
> >> Source/WebCore/rendering/style/SVGRenderStyle.h:409
> >> +        unsigned _colorProfile : 1; // EColorProfile
> > 
> > _colorProfile is incorrectly named. Don't use underscores in your identifier names.  [readability/naming/underscores] [4]
> 
> Well, it should still be fixed IMHO. Maybe in a preparation patch?

Sure, that can be fixed. But we'll be ignoring CSSPropertyColorProfile for now, so it's completely unrelated.

[Florin Malita]

Created attachment 172223 [details]
Patch

[WebKit Review Bot]

Comment on attachment 172223 [details]
Patch

Clearing flags on attachment: 172223

Committed r133420: <http://trac.webkit.org/changeset/133420>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.