97563 2012-09-25 06:28:34 -0700 GIFImageReader: fix tautological compare 2012-09-25 10:11:48 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 hans hans abarth thakis webkit.review.bot oldest_to_newest 727575 0 hans 2012-09-25 06:28:34 -0700 GIFImageReader: fix tautological compare 727578 1 165601 hans 2012-09-25 06:33:09 -0700 Created attachment 165601 Patch 727579 2 hans 2012-09-25 06:35:08 -0700 Adam: you had the pleasure of touching this file in Bug 52363, perhaps you can take a look? 727580 3 webkit.review.bot 2012-09-25 06:35:31 -0700 Attachment 165601 did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:659: disposal_method is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style. 727583 4 hans 2012-09-25 06:38:11 -0700 (In reply to comment #3) > Attachment 165601 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 > Source/WebCore/platform/image-decoders/gif/GIFImageReader.cpp:659: disposal_method is incorrectly named. Don't use underscores in your identifier names. [readability/naming/underscores] [4] > Total errors found: 1 in 2 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. The style is consistent with the rest of the file. 727705 5 165601 abarth 2012-09-25 09:26:47 -0700 Comment on attachment 165601 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165601&action=review > Source/WebCore/ChangeLog:15 > + This patch fixes the code by doing the comparison before storing the > + value in the enum. Is there any observable consequence of this change? If so, should we write a test? 727706 6 abarth 2012-09-25 09:27:19 -0700 That file is also massively out of style, but I agree that point-fixing this one issue isn't the right thing to do in this patch. 727738 7 hans 2012-09-25 09:44:00 -0700 (In reply to comment #5) > (From update of attachment 165601 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=165601&action=review > > > Source/WebCore/ChangeLog:15 > > + This patch fixes the code by doing the comparison before storing the > > + value in the enum. > > Is there any observable consequence of this change? If so, should we write a test? There's no observable difference. The code currently relied on unspecified behavior (i.e. that the enum can represent more values than it strictly has to), and my change just fixes that. 727743 8 165601 abarth 2012-09-25 09:47:17 -0700 Comment on attachment 165601 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165601&action=review >>> Source/WebCore/ChangeLog:15 >>> + value in the enum. >> >> Is there any observable consequence of this change? If so, should we write a test? > > There's no observable difference. > > The code currently relied on unspecified behavior (i.e. that the enum can represent more values than it strictly has to), and my change just fixes that. Ah, I see. We have storage for all the bits, they're just outside the enum range. Makes sense. 727757 9 165601 webkit.review.bot 2012-09-25 10:11:45 -0700 Comment on attachment 165601 Patch Clearing flags on attachment: 165601 Committed r129523: <http://trac.webkit.org/changeset/129523> 727758 10 webkit.review.bot 2012-09-25 10:11:48 -0700 All reviewed patches have been landed. Closing bug. 165601 2012-09-25 06:33:09 -0700 2012-09-25 10:11:44 -0700 Patch bug-97563-20120925143228.patch text/plain 2380 hans U3VidmVyc2lvbiBSZXZpc2lvbjogMTI5NDgzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMzlhZWNmNTAyNWIxZTBl ZmVlNTMyNWM0OTA3ODliODMzMzMyZTk4YS4uY2M4NWQxYjBiNzZlM2VjNmJjZmEzMzRlYTFiZDIy ZjU2NjZhNjQ4YyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI0IEBACisyMDEyLTA5LTI1ICBIYW5z IFdlbm5ib3JnICA8aGFuc0BjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgR0lGSW1hZ2VSZWFkZXI6 IGZpeCB0YXV0b2xvZ2ljYWwgY29tcGFyZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9OTc1NjMKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICBUaGUgZnJhbWVfcmVhZGVyLT5kaXNwb3NhbF9tZXRob2QgZmllbGQg aXMgb2YgdHlwZQorICAgICAgICBJbWFnZUZyYW1lOjpGcmFtZURpc3Bvc2FsTWV0aG9kLCB3aGlj aCBoYXMgYSByYW5nZSBvZiAwLS0zLiBUaGVyZWZvcmUsCisgICAgICAgIHJlY2VudCB2ZXJzaW9u cyBvZiBDbGFuZyBjb25jbHVkZXMgdGhhdCB0aGUgZXhwcmVzc2lvbgorICAgICAgICBmcmFtZV9y ZWFkZXItPmRpc3Bvc2FsX21ldGhvZCA9PSA0CisgICAgICAgIHdpbGwgbmV2ZXIgYmUgdHJ1ZSwg YW5kIHdhcm5zIGFib3V0IGl0LgorCisgICAgICAgIFRoaXMgcGF0Y2ggZml4ZXMgdGhlIGNvZGUg YnkgZG9pbmcgdGhlIGNvbXBhcmlzb24gYmVmb3JlIHN0b3JpbmcgdGhlCisgICAgICAgIHZhbHVl IGluIHRoZSBlbnVtLgorCisgICAgICAgIE5vIG5ldyBmdW5jdGlvbmFsaXR5LCBubyBuZXcgdGVz dHMuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9pbWFnZS1kZWNvZGVycy9naWYvR0lGSW1hZ2VSZWFk ZXIuY3BwOgorICAgICAgICAoR0lGSW1hZ2VSZWFkZXI6OnJlYWQpOgorCiAyMDEyLTA5LTI1ICBL ZWlzaGkgSGF0dG9yaSAgPGtlaXNoaUB3ZWJraXQub3JnPgogCiAgICAgICAgIFJFR1JFU1NJT04o cjEyOTQ0OCk6IG11bHRpcGxlIGZpZWxkcyB0aW1lIGlucHV0IFVJIGRvZXNuJ3QgdXNlIHN5c3Rl bSB0aW1lIGZvcm1hdCBzZXR0aW5ncyBvbiBDaHJvbWl1bS1NYWMKZGlmZiAtLWdpdCBhL1NvdXJj ZS9XZWJDb3JlL3BsYXRmb3JtL2ltYWdlLWRlY29kZXJzL2dpZi9HSUZJbWFnZVJlYWRlci5jcHAg Yi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9pbWFnZS1kZWNvZGVycy9naWYvR0lGSW1hZ2VSZWFk ZXIuY3BwCmluZGV4IDBjYTk2ZTMwZjM3NDViNjE3ODRlY2M1ZTZmZmYzMjQ2NzU2MmZlYWUuLjc0 MTRlOTgxODU5MDdjMGY2ODdiMzdjOTM1MDUzMTkyN2ZmYjg4M2IgMTAwNjQ0Ci0tLSBhL1NvdXJj ZS9XZWJDb3JlL3BsYXRmb3JtL2ltYWdlLWRlY29kZXJzL2dpZi9HSUZJbWFnZVJlYWRlci5jcHAK KysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vaW1hZ2UtZGVjb2RlcnMvZ2lmL0dJRkltYWdl UmVhZGVyLmNwcApAQCAtNjU2LDEwICs2NTYsMTEgQEAgYm9vbCBHSUZJbWFnZVJlYWRlcjo6cmVh ZChjb25zdCB1bnNpZ25lZCBjaGFyICpidWYsIHVuc2lnbmVkIGxlbiwKICAgICAgICAgfQogICAg ICAgICAvLyBOT1RFOiBUaGlzIHJlbGllcyBvbiB0aGUgdmFsdWVzIGluIHRoZSBGcmFtZURpc3Bv c2FsTWV0aG9kIGVudW0KICAgICAgICAgLy8gbWF0Y2hpbmcgdGhvc2UgaW4gdGhlIEdJRiBzcGVj IQotICAgICAgICBmcmFtZV9yZWFkZXItPmRpc3Bvc2FsX21ldGhvZCA9IChXZWJDb3JlOjpJbWFn ZUZyYW1lOjpGcmFtZURpc3Bvc2FsTWV0aG9kKSgoKCpxKSA+PiAyKSAmIDB4Nyk7CisgICAgICAg IGludCBkaXNwb3NhbF9tZXRob2QgPSAoKCpxKSA+PiAyKSAmIDB4NzsKKyAgICAgICAgZnJhbWVf cmVhZGVyLT5kaXNwb3NhbF9tZXRob2QgPSAoV2ViQ29yZTo6SW1hZ2VGcmFtZTo6RnJhbWVEaXNw b3NhbE1ldGhvZClkaXNwb3NhbF9tZXRob2Q7CiAgICAgICAgIC8vIFNvbWUgc3BlY3Mgc2F5IDNy ZCBiaXQgKHZhbHVlIDQpLCBvdGhlciBzcGVjcyBzYXkgdmFsdWUgMwogICAgICAgICAvLyBMZXQn cyBjaG9vc2UgMyAodGhlIG1vcmUgcG9wdWxhcikKLSAgICAgICAgaWYgKGZyYW1lX3JlYWRlci0+ ZGlzcG9zYWxfbWV0aG9kID09IDQpCisgICAgICAgIGlmIChkaXNwb3NhbF9tZXRob2QgPT0gNCkK ICAgICAgICAgICBmcmFtZV9yZWFkZXItPmRpc3Bvc2FsX21ldGhvZCA9IFdlYkNvcmU6OkltYWdl RnJhbWU6OkRpc3Bvc2VPdmVyd3JpdGVQcmV2aW91czsKICAgICAgICAgZnJhbWVfcmVhZGVyLT5k ZWxheV90aW1lID0gR0VUSU5UMTYocSArIDEpICogMTA7CiAgICAgICB9Cg==