65598 2011-08-03 00:42:09 -0700 [V8] V8CSSRuleCustom.cpp: not all enum values are listed in switch statement 2011-08-03 09:15:40 -0700 1 1 1 Unclassified WebKit Platform 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P3 Minor --- 65506 1 rolandsteiner rolandsteiner dominicc webkit.review.bot oldest_to_newest 445745 0 rolandsteiner 2011-08-03 00:42:09 -0700 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. 445749 1 102748 rolandsteiner 2011-08-03 00:45:47 -0700 Created attachment 102748 Patch 445767 2 102748 dominicc 2011-08-03 02:06:12 -0700 Comment on attachment 102748 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. 445772 3 102756 rolandsteiner 2011-08-03 02:24:48 -0700 Created attachment 102756 Patch 445773 4 rolandsteiner 2011-08-03 02:27:37 -0700 (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. 445899 5 102756 webkit.review.bot 2011-08-03 09:15:35 -0700 Comment on attachment 102756 Patch Clearing flags on attachment: 102756 Committed r92285: <http://trac.webkit.org/changeset/92285> 445900 6 webkit.review.bot 2011-08-03 09:15:40 -0700 All reviewed patches have been landed. Closing bug. 102748 2011-08-03 00:45:47 -0700 2011-08-03 02:24:42 -0700 Patch bug-65598-20110803164545.patch text/plain 1669 rolandsteiner U3VidmVyc2lvbiBSZXZpc2lvbjogOTIyNDkKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBiNzk3YjIzOWM3OTdlM2Ux OTM4N2I1NWIwMzJjNmZjNjRjZDBmNTkzLi5mZWVkZTYzZDQ1ZTBkMDhmYWI2ZTgxNDFmZDZkNzMx Njk4Yjg2YTM2IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTEtMDgtMDMgIFJvbGFu ZCBTdGVpbmVyICA8cm9sYW5kc3RlaW5lckBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgW1Y4XSBW OENTU1J1bGVDdXN0b20uY3BwOiBub3QgYWxsIGVudW0gdmFsdWVzIGFyZSBsaXN0ZWQgaW4gc3dp dGNoIHN0YXRlbWVudAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9NjU1OTgKKworICAgICAgICBBZGRlZCBVTktOT1dOX1JVTEUgdG8gdGhlIHN3aXRjaCgp IHN0YXRlbWVudC4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICBObyBuZXcgdGVzdHMuIChObyBzZW1hbnRpYyBjaGFuZ2UpCisKKyAgICAgICAgKiBiaW5k aW5ncy92OC9jdXN0b20vVjhDU1NSdWxlQ3VzdG9tLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OnRv VjgpOgorCiAyMDExLTA4LTAyICBKZWZmcmV5IFBmYXUgIDxqcGZhdUBhcHBsZS5jb20+CiAKICAg ICAgICAgQWRkIG5hbWVzcGFjZSBwcmVmaXggc3VwcG9ydCBpbiBYTUwgdG9rZW5pemVyCmRpZmYg LS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy92OC9jdXN0b20vVjhDU1NSdWxlQ3VzdG9t LmNwcCBiL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL3Y4L2N1c3RvbS9WOENTU1J1bGVDdXN0b20u Y3BwCmluZGV4IDBkZmQ5NTQ0M2M3MTkyMjYxN2I4MTQyNDJlYmVmMGYxMTc0Mjc5M2UuLjE5ZmVi NzUyYzhhOTcxMzU2YTBiNDY4NDRmMjUzYzY1OWY0OTM3YzUgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9X ZWJDb3JlL2JpbmRpbmdzL3Y4L2N1c3RvbS9WOENTU1J1bGVDdXN0b20uY3BwCisrKyBiL1NvdXJj ZS9XZWJDb3JlL2JpbmRpbmdzL3Y4L2N1c3RvbS9WOENTU1J1bGVDdXN0b20uY3BwCkBAIC00Nyw2 ICs0NywxMSBAQCB2ODo6SGFuZGxlPHY4OjpWYWx1ZT4gdG9WOChDU1NSdWxlKiBpbXBsKQogICAg IGlmICghaW1wbCkKICAgICAgICAgcmV0dXJuIHY4OjpOdWxsKCk7CiAgICAgc3dpdGNoIChpbXBs LT50eXBlKCkpIHsKKyAgICBjYXNlIENTU1J1bGU6OlVOS05PV05fUlVMRToKKyAgICAgICAgLy8g Q1NTVW5rbm93blJ1bGUuaWRsIGlzIGV4cGxpY2l0bHkgZXhjbHVkZWQgYXMgaXQgZG9lc24ndCBh ZGQgYW55dGhpbmcKKyAgICAgICAgLy8gb3ZlciBDU1NSdWxlLmlkbCAoc2VlIFdlYkNvcmUuZ3lw L1dlYkNvcmUuZ3lwOiAnYmluZGluZ3NfaWRsX2ZpbGVzJykuCisgICAgICAgIC8vIC0+IFVzZSB0 aGUgYmFzZSBjbGFzcyB3cmFwcGVyIGhlcmUuCisgICAgICAgIHJldHVybiBWOENTU1J1bGU6Ondy YXAoaW1wbCk7CiAgICAgY2FzZSBDU1NSdWxlOjpTVFlMRV9SVUxFOgogICAgICAgICByZXR1cm4g dG9WOChzdGF0aWNfY2FzdDxDU1NTdHlsZVJ1bGUqPihpbXBsKSk7CiAgICAgY2FzZSBDU1NSdWxl OjpDSEFSU0VUX1JVTEU6Cg== 102756 2011-08-03 02:24:48 -0700 2011-08-03 09:15:35 -0700 Patch bug-65598-20110803182446.patch text/plain 1667 rolandsteiner U3VidmVyc2lvbiBSZXZpc2lvbjogOTIyNDkKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBiNzk3YjIzOWM3OTdlM2Ux OTM4N2I1NWIwMzJjNmZjNjRjZDBmNTkzLi43MzFlOGU4NjkxODlkNzgwOWY1MjAzYjg1MmE1MDdj ZTYzMTM0OWJlIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIwMTEtMDgtMDMgIFJvbGFu ZCBTdGVpbmVyICA8cm9sYW5kc3RlaW5lckBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgW1Y4XSBW OENTU1J1bGVDdXN0b20uY3BwOiBub3QgYWxsIGVudW0gdmFsdWVzIGFyZSBsaXN0ZWQgaW4gc3dp dGNoIHN0YXRlbWVudAorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9NjU1OTgKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICBBZGRlZCBVTktOT1dOX1JVTEUgdG8gdGhlIHN3aXRjaCBzdGF0ZW1lbnQuCisKKyAgICAg ICAgTm8gbmV3IHRlc3RzLiAoTm8gc2VtYW50aWMgY2hhbmdlKQorCisgICAgICAgICogYmluZGlu Z3MvdjgvY3VzdG9tL1Y4Q1NTUnVsZUN1c3RvbS5jcHA6CisgICAgICAgIChXZWJDb3JlOjp0b1Y4 KToKKwogMjAxMS0wOC0wMiAgSmVmZnJleSBQZmF1ICA8anBmYXVAYXBwbGUuY29tPgogCiAgICAg ICAgIEFkZCBuYW1lc3BhY2UgcHJlZml4IHN1cHBvcnQgaW4gWE1MIHRva2VuaXplcgpkaWZmIC0t Z2l0IGEvU291cmNlL1dlYkNvcmUvYmluZGluZ3MvdjgvY3VzdG9tL1Y4Q1NTUnVsZUN1c3RvbS5j cHAgYi9Tb3VyY2UvV2ViQ29yZS9iaW5kaW5ncy92OC9jdXN0b20vVjhDU1NSdWxlQ3VzdG9tLmNw cAppbmRleCAwZGZkOTU0NDNjNzE5MjI2MTdiODE0MjQyZWJlZjBmMTE3NDI3OTNlLi4xOWZlYjc1 MmM4YTk3MTM1NmEwYjQ2ODQ0ZjI1M2M2NTlmNDkzN2M1IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2Vi Q29yZS9iaW5kaW5ncy92OC9jdXN0b20vVjhDU1NSdWxlQ3VzdG9tLmNwcAorKysgYi9Tb3VyY2Uv V2ViQ29yZS9iaW5kaW5ncy92OC9jdXN0b20vVjhDU1NSdWxlQ3VzdG9tLmNwcApAQCAtNDcsNiAr NDcsMTEgQEAgdjg6OkhhbmRsZTx2ODo6VmFsdWU+IHRvVjgoQ1NTUnVsZSogaW1wbCkKICAgICBp ZiAoIWltcGwpCiAgICAgICAgIHJldHVybiB2ODo6TnVsbCgpOwogICAgIHN3aXRjaCAoaW1wbC0+ dHlwZSgpKSB7CisgICAgY2FzZSBDU1NSdWxlOjpVTktOT1dOX1JVTEU6CisgICAgICAgIC8vIENT U1Vua25vd25SdWxlLmlkbCBpcyBleHBsaWNpdGx5IGV4Y2x1ZGVkIGFzIGl0IGRvZXNuJ3QgYWRk IGFueXRoaW5nCisgICAgICAgIC8vIG92ZXIgQ1NTUnVsZS5pZGwgKHNlZSBXZWJDb3JlLmd5cC9X ZWJDb3JlLmd5cDogJ2JpbmRpbmdzX2lkbF9maWxlcycpLgorICAgICAgICAvLyAtPiBVc2UgdGhl IGJhc2UgY2xhc3Mgd3JhcHBlciBoZXJlLgorICAgICAgICByZXR1cm4gVjhDU1NSdWxlOjp3cmFw KGltcGwpOwogICAgIGNhc2UgQ1NTUnVsZTo6U1RZTEVfUlVMRToKICAgICAgICAgcmV0dXJuIHRv Vjgoc3RhdGljX2Nhc3Q8Q1NTU3R5bGVSdWxlKj4oaW1wbCkpOwogICAgIGNhc2UgQ1NTUnVsZTo6 Q0hBUlNFVF9SVUxFOgo=