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.
Created attachment 102748 [details] Patch
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.
Created attachment 102756 [details] Patch
(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.
Comment on attachment 102756 [details] Patch Clearing flags on attachment: 102756 Committed r92285: <http://trac.webkit.org/changeset/92285>
All reviewed patches have been landed. Closing bug.