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) {
Created attachment 149317 [details] Patch
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.
(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.
Created attachment 149324 [details] Patch
New patch; macpherson@chromium.org looks like the original author, and is on CC.
Comment on attachment 149324 [details] Patch Ignore this, it fails.
Created attachment 149325 [details] Patch
(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 :).
Created attachment 149333 [details] Patch
(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.
Comment on attachment 149333 [details] Patch Clearing flags on attachment: 149333 Committed r121192: <http://trac.webkit.org/changeset/121192>
All reviewed patches have been landed. Closing bug.
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.
(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. :)
Reopening to attach new patch.
Created attachment 149426 [details] Patch
I also ran tests locally with CSS_VARIABLES enabled to make sure the code really is unreachable.
Ping for r+?
Comment on attachment 149426 [details] Patch ok.
Comment on attachment 149426 [details] Patch Clearing flags on attachment: 149426 Committed r122700: <http://trac.webkit.org/changeset/122700>