Bug 89892 – Compilation failure in StyleResolver.cpp (clang)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mike West]

Currently, compilation via ninja/clang is throwing errors in StyleResolver and CSSParser like the following:

../../third_party/WebKit/Source/WebCore/css/StyleResolver.cpp:3392:13: error: enumeration value 'CSSPropertyVariable' not handled in switch [-Werror,-Wswitch]
    switch (id) {

[Mike West]

Created attachment 149317 [details]
Patch

[Alexis Menard (darktears)]

Comment on attachment 149317 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:2634
> +#endif

It is misleading to put it there as per comment above : 

// These properties should be handled before in isValidKeywordPropertyAndValue().

And CSSPropertyVariable is not handled there. So it will ASSERT which is incorrect.

You better make a case somewhere else, and return false, add a FIXME : Implement me.

> Source/WebCore/css/StyleResolver.cpp:4281
> +#endif

Ditto there. Incorrect.

[Mike West]

(In reply to comment #2)
> (From update of attachment 149317 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149317&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:2634
> > +#endif
> 
> It is misleading to put it there as per comment above : 
> 
> // These properties should be handled before in isValidKeywordPropertyAndValue().
> 
> And CSSPropertyVariable is not handled there. So it will ASSERT which is incorrect.
> 
> You better make a case somewhere else, and return false, add a FIXME : Implement me.

Thanks for the review. I figured that ASSERTing was better than just returning false (since I have no idea what the correct behavior is), but I'll add the additional case and track down the original author for implementation details.

[Mike West]

Created attachment 149324 [details]
Patch

[Mike West]

New patch; [e-mail] looks like the original author, and is on CC.

[Mike West]

Comment on attachment 149324 [details]
Patch

Ignore this, it fails.

[Mike West]

Created attachment 149325 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 149317 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=149317&action=review
> > 
> > > Source/WebCore/css/CSSParser.cpp:2634
> > > +#endif
> > 
> > It is misleading to put it there as per comment above : 
> > 
> > // These properties should be handled before in isValidKeywordPropertyAndValue().
> > 
> > And CSSPropertyVariable is not handled there. So it will ASSERT which is incorrect.
> > 
> > You better make a case somewhere else, and return false, add a FIXME : Implement me.
> 
> Thanks for the review. I figured that ASSERTing was better than just returning false (since I have no idea what the correct behavior is), but I'll add the additional case and track down the original author for implementation details.

Well ASSERTing is a bit rude. It if returns false then the CSS declaration will be dropped but at least I can continue using the browser :).

[Mike West]

Created attachment 149333 [details]
Patch

[Mike West]

(In reply to comment #9)
> Created an attachment (id=149333) [details]
> Patch

New patch moves the `case` up above the "all the rest + svg" block.

[WebKit Review Bot]

Comment on attachment 149333 [details]
Patch

Clearing flags on attachment: 149333

Committed r121192: <http://trac.webkit.org/changeset/121192>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Luke Macpherson]

Weird that these showed up in clang but not my regular variables build. In both cases this code should be unreachable. Thanks for the fixes.

[Mike West]

(In reply to comment #13)
> Weird that these showed up in clang but not my regular variables build. In both cases this code should be unreachable. Thanks for the fixes.

If it really is unreachable, can you throw up a patch to trigger the ASSERT (basically the first patch on this bug, I suppose)? I wasn't sure what the behavior should be, and Alexis suggested that special-casing it was appropriate.

If we don't need that case, we can just fold it into the big block o' case statements. :)

[Luke Macpherson]

Reopening to attach new patch.

[Luke Macpherson]

Created attachment 149426 [details]
Patch

[Luke Macpherson]

I also ran tests locally with CSS_VARIABLES enabled to make sure the code really is unreachable.

[Luke Macpherson]

Ping for r+?

[Ryosuke Niwa]

Comment on attachment 149426 [details]
Patch

ok.

[WebKit Review Bot]

Comment on attachment 149426 [details]
Patch

Clearing flags on attachment: 149426

Committed r122700: <http://trac.webkit.org/changeset/122700>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.