Bug 65598 – [V8] V8CSSRuleCustom.cpp: not all enum values are listed in switch statement

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Roland Steiner]

The (only) switch statement in V8CSSRuleCustom.cpp misses an entry for UNKNOWN_RULE. Although it's fine that UNKNOWN_RULE is handled by the fallback code, this causes a build warning -> error when the type() function used in the switch() expression is changed to return an enum (currently unsigned int). This blocks https://bugs.webkit.org/show_bug.cgi?id=65506.

[Roland Steiner]

Created attachment 102748 [details]
Patch

[Dominic Cooney]

Comment on attachment 102748 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Added UNKNOWN_RULE to the switch() statement.

Drop the parens?

> Source/WebCore/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).

Doesn’t this go before the previous paragraph.

> Source/WebCore/bindings/v8/custom/V8CSSRuleCustom.cpp:49
>      switch (impl->type()) {

If the switch is exhausted, isn’t the code after the switch dead? You should remove it.

> Source/WebCore/bindings/v8/custom/V8CSSRuleCustom.cpp:51
> +        // CSSUnknownRule.idl is explicitly excluded as it doesn't add anything

Shorten this? You don’t need to talk about GYP files or IDL files… just say you’re wrapping at the base type.

[Roland Steiner]

Created attachment 102756 [details]
Patch

[Roland Steiner]

(In reply to comment #2)
> (From update of attachment 102748 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=102748&action=review
> 
> > Source/WebCore/ChangeLog:6
> > +        Added UNKNOWN_RULE to the switch() statement.
> 
> Drop the parens?

Sure.

> > Source/WebCore/ChangeLog:8
> > +        Reviewed by NOBODY (OOPS!).
> 
> Doesn’t this go before the previous paragraph.

Oops, that's right - corrected.

> > Source/WebCore/bindings/v8/custom/V8CSSRuleCustom.cpp:49
> >      switch (impl->type()) {
> 
> If the switch is exhausted, isn’t the code after the switch dead? You should remove it.

type() still returns unsigned int, so the compiler would scream at me. Can only remove with the patch to bug 65506.

> > Source/WebCore/bindings/v8/custom/V8CSSRuleCustom.cpp:51
> > +        // CSSUnknownRule.idl is explicitly excluded as it doesn't add anything
> 
> Shorten this? You don’t need to talk about GYP files or IDL files… just say you’re wrapping at the base type.

Originally I had 

    return toV8(static_cast<CSSUnknownRule*>(impl));

but this doesn't link, even though V8CSSUnknownRule.cpp exists. It took me quite a while to figure out why, so I believe this comment has merit.

[WebKit Review Bot]

Comment on attachment 102756 [details]
Patch

Clearing flags on attachment: 102756

Committed r92285: <http://trac.webkit.org/changeset/92285>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.