63564 2011-06-28 14:36:08 -0700 coding-style: Add note about cases where enums are preferred over bools 2011-06-29 10:57:18 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 leandro leandro darin webkit.review.bot oldest_to_newest 429151 0 leandro 2011-06-28 14:36:08 -0700 coding-style: Add note about cases where enums are preferred over bools 429183 1 98979 leandro 2011-06-28 15:23:19 -0700 Created attachment 98979 Patch 429290 2 98979 darin 2011-06-28 17:15:32 -0700 Comment on attachment 98979 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=98979&action=review > Websites/webkit.org/coding/coding-style.html:606 > +<li>On functions with more than one parameter, prefer enums to bools if callers are likely to be passing constants. This rule applies to functions with one parameter too. The number of parameters is not the key. Maybe the reason you though it depended on the number of parameters is that it does not apply to a setXXX function where the name of the function makes clear what the boolean is. I also think the guideline should explain the reason. The code below does allude to the reason this matters, but it’s worth saying that the named constants are much easier to read at the call site. 429291 3 98979 darin 2011-06-28 17:16:08 -0700 Comment on attachment 98979 Patch review- because I’d like to remove the mistaken “more than one parameter” 429575 4 99083 leandro 2011-06-29 07:01:34 -0700 Created attachment 99083 Patch 429614 5 99083 darin 2011-06-29 08:19:50 -0700 Comment on attachment 99083 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99083&action=review Thanks. Getting better. > Websites/webkit.org/coding/coding-style.html:607 > +passing constants, since named constants are easier to read on the call site. Should be "at the call site" rather than "on the call site". > Websites/webkit.org/coding/coding-style.html:608 > +The only exception would be on setters, where the name of the function Instead of "The only exception would be on setters," I would say, "An exception to this rule is a setter function," > Websites/webkit.org/coding/coding-style.html:613 > +doSomething(something, AllowFooBar); > +paintTextWithShadows(context, ..., textStrokeWidth > 0, isHorizontal()); The right example should include a setter that takes a boolean. > Websites/webkit.org/coding/coding-style.html:618 > +doSomething(something, false); The wrong example should include a setter that takes an enum. 429685 6 99107 leandro 2011-06-29 09:52:52 -0700 Created attachment 99107 Patch 429687 7 leandro 2011-06-29 09:54:19 -0700 (In reply to comment #5) > > Should be "at the call site" rather than "on the call site". > Instead of "The only exception would be on setters," I would say, "An exception to this rule is a setter function," > The right example should include a setter that takes a boolean. > The wrong example should include a setter that takes an enum. > Fixed everything. Thanks. 429722 8 webkit.review.bot 2011-06-29 10:55:42 -0700 The commit-queue encountered the following flaky tests while processing attachment 99107: http/tests/media/reload-after-dialog.html bug 59901 (authors: annacc@chromium.org, eric.carlson@apple.com, jamesr@chromium.org, and vrk@chromium.org) The commit-queue is continuing to process your patch. 429725 9 99107 webkit.review.bot 2011-06-29 10:57:13 -0700 Comment on attachment 99107 Patch Clearing flags on attachment: 99107 Committed r90027: <http://trac.webkit.org/changeset/90027> 429726 10 webkit.review.bot 2011-06-29 10:57:18 -0700 All reviewed patches have been landed. Closing bug. 98979 2011-06-28 15:23:19 -0700 2011-06-29 07:01:28 -0700 Patch bug-63564-20110628192317.patch text/plain 1657 leandro SW5kZXg6IFdlYnNpdGVzL3dlYmtpdC5vcmcvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYnNp dGVzL3dlYmtpdC5vcmcvQ2hhbmdlTG9nCShyZXZpc2lvbiA4OTk2MSkKKysrIFdlYnNpdGVzL3dl YmtpdC5vcmcvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTEt MDYtMjggIExlYW5kcm8gUGVyZWlyYSAgPGxlYW5kcm9AcHJvZnVzaW9uLm1vYmk+CisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQWRkIG5vdGUgYWJvdXQg Y2FzZXMgd2hlcmUgZW51bXMgYXJlIHByZWZlcnJlZCBvdmVyIGJvb2xzIG9uIGZ1bmN0aW9uIHBh cmFtZXRlcnMuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD02MzU2NAorCisgICAgICAgIFRoaXMgbWF0Y2hlcyB0aGUgb3V0Y29tZSBvZiB0aGlzIHdlYmtp dC1kZXYgdGhyZWFkOgorICAgICAgICBodHRwczovL2xpc3RzLndlYmtpdC5vcmcvcGlwZXJtYWls L3dlYmtpdC1kZXYvMjAxMC1EZWNlbWJlci8wMTUxOTIuaHRtbAorCisgICAgICAgICogY29kaW5n L2NvZGluZy1zdHlsZS5odG1sOgorCiAyMDExLTA1LTMxICBDaHJpcyBFdmFucyAgPGNldmFuc0Bj aHJvbWl1bS5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgRHJldyBZYW8uCkluZGV4OiBXZWJz aXRlcy93ZWJraXQub3JnL2NvZGluZy9jb2Rpbmctc3R5bGUuaHRtbAo9PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBX ZWJzaXRlcy93ZWJraXQub3JnL2NvZGluZy9jb2Rpbmctc3R5bGUuaHRtbAkocmV2aXNpb24gODk5 MzkpCisrKyBXZWJzaXRlcy93ZWJraXQub3JnL2NvZGluZy9jb2Rpbmctc3R5bGUuaHRtbAkod29y a2luZyBjb3B5KQpAQCAtNjAzLDYgKzYwMywyMCBAQCB2b2lkIGRvU29tZXRoaW5nKFNjcmlwdEV4 ZWN1dGlvbkNvbnRleHQqCiA8L3ByZT4KIDwvbGk+CiAKKzxsaT5PbiBmdW5jdGlvbnMgd2l0aCBt b3JlIHRoYW4gb25lIHBhcmFtZXRlciwgcHJlZmVyIGVudW1zIHRvIGJvb2xzIGlmIGNhbGxlcnMg YXJlIGxpa2VseSB0byBiZSBwYXNzaW5nIGNvbnN0YW50cy4KKworPGg0IGNsYXNzPSJyaWdodCI+ UmlnaHQ6PC9oND4KKzxwcmUgY2xhc3M9ImNvZGUiPgorZG9Tb21ldGhpbmcoc29tZXRoaW5nLCBB bGxvd0Zvb0Jhcik7CitwYWludFRleHRXaXRoU2hhZG93cyhjb250ZXh0LCAuLi4sIHRleHRTdHJv a2VXaWR0aCA+IDAsIGlzSG9yaXpvbnRhbCgpKTsKKzwvcHJlPgorCis8aDQgY2xhc3M9Indyb25n Ij5Xcm9uZzo8L2g0PgorPHByZSBjbGFzcz0iY29kZSI+Citkb1NvbWV0aGluZyhzb21ldGhpbmcs IGZhbHNlKTsKKzwvcHJlPgorPC9saT4KKwogPGxpPk9iamVjdGl2ZS1DIG1ldGhvZCBuYW1lcyBz aG91bGQgZm9sbG93IHRoZSBDb2NvYSBuYW1pbmcgZ3VpZGVsaW5lcyAmbWRhc2g7CiB0aGV5IHNo b3VsZCByZWFkIGxpa2UgYSBwaHJhc2UgYW5kIGVhY2ggcGllY2Ugb2YgdGhlIHNlbGVjdG9yIHNo b3VsZCAKIHN0YXJ0IHdpdGggYSBsb3dlcmNhc2UgbGV0dGVyIGFuZCB1c2UgaW50ZXJjYXBzLjwv bGk+Cg== 99083 2011-06-29 07:01:34 -0700 2011-06-29 09:52:46 -0700 Patch bug-63564-20110629110132.patch text/plain 1810 leandro SW5kZXg6IFdlYnNpdGVzL3dlYmtpdC5vcmcvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYnNp dGVzL3dlYmtpdC5vcmcvQ2hhbmdlTG9nCShyZXZpc2lvbiA4OTk2MSkKKysrIFdlYnNpdGVzL3dl YmtpdC5vcmcvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTEt MDYtMjggIExlYW5kcm8gUGVyZWlyYSAgPGxlYW5kcm9AcHJvZnVzaW9uLm1vYmk+CisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQWRkIG5vdGUgYWJvdXQg Y2FzZXMgd2hlcmUgZW51bXMgYXJlIHByZWZlcnJlZCBvdmVyIGJvb2xzIG9uIGZ1bmN0aW9uIHBh cmFtZXRlcnMuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD02MzU2NAorCisgICAgICAgIFRoaXMgbWF0Y2hlcyB0aGUgb3V0Y29tZSBvZiB0aGlzIHdlYmtp dC1kZXYgdGhyZWFkOgorICAgICAgICBodHRwczovL2xpc3RzLndlYmtpdC5vcmcvcGlwZXJtYWls L3dlYmtpdC1kZXYvMjAxMC1EZWNlbWJlci8wMTUxOTIuaHRtbAorCisgICAgICAgICogY29kaW5n L2NvZGluZy1zdHlsZS5odG1sOgorCiAyMDExLTA1LTMxICBDaHJpcyBFdmFucyAgPGNldmFuc0Bj aHJvbWl1bS5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgRHJldyBZYW8uCkluZGV4OiBXZWJz aXRlcy93ZWJraXQub3JnL2NvZGluZy9jb2Rpbmctc3R5bGUuaHRtbAo9PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBX ZWJzaXRlcy93ZWJraXQub3JnL2NvZGluZy9jb2Rpbmctc3R5bGUuaHRtbAkocmV2aXNpb24gODk5 MzkpCisrKyBXZWJzaXRlcy93ZWJraXQub3JnL2NvZGluZy9jb2Rpbmctc3R5bGUuaHRtbAkod29y a2luZyBjb3B5KQpAQCAtNjAzLDYgKzYwMywyMiBAQCB2b2lkIGRvU29tZXRoaW5nKFNjcmlwdEV4 ZWN1dGlvbkNvbnRleHQqCiA8L3ByZT4KIDwvbGk+CiAKKzxsaT5QcmVmZXIgZW51bXMgdG8gYm9v bHMgb24gZnVuY3Rpb24gcGFyYW1ldGVycyBpZiBjYWxsZXJzIGFyZSBsaWtlbHkgdG8gYmUKK3Bh c3NpbmcgY29uc3RhbnRzLCBzaW5jZSBuYW1lZCBjb25zdGFudHMgYXJlIGVhc2llciB0byByZWFk IG9uIHRoZSBjYWxsIHNpdGUuIAorVGhlIG9ubHkgZXhjZXB0aW9uIHdvdWxkIGJlIG9uIHNldHRl cnMsIHdoZXJlIHRoZSBuYW1lIG9mIHRoZSBmdW5jdGlvbgorYWxyZWFkeSBtYWtlcyBjbGVhciB3 aGF0IHRoZSBib29sZWFuIGlzLgorPGg0IGNsYXNzPSJyaWdodCI+UmlnaHQ6PC9oND4KKzxwcmUg Y2xhc3M9ImNvZGUiPgorZG9Tb21ldGhpbmcoc29tZXRoaW5nLCBBbGxvd0Zvb0Jhcik7CitwYWlu dFRleHRXaXRoU2hhZG93cyhjb250ZXh0LCAuLi4sIHRleHRTdHJva2VXaWR0aCA+IDAsIGlzSG9y aXpvbnRhbCgpKTsKKzwvcHJlPgorCis8aDQgY2xhc3M9Indyb25nIj5Xcm9uZzo8L2g0PgorPHBy ZSBjbGFzcz0iY29kZSI+Citkb1NvbWV0aGluZyhzb21ldGhpbmcsIGZhbHNlKTsKKzwvcHJlPgor PC9saT4KKwogPGxpPk9iamVjdGl2ZS1DIG1ldGhvZCBuYW1lcyBzaG91bGQgZm9sbG93IHRoZSBD b2NvYSBuYW1pbmcgZ3VpZGVsaW5lcyAmbWRhc2g7CiB0aGV5IHNob3VsZCByZWFkIGxpa2UgYSBw aHJhc2UgYW5kIGVhY2ggcGllY2Ugb2YgdGhlIHNlbGVjdG9yIHNob3VsZCAKIHN0YXJ0IHdpdGgg YSBsb3dlcmNhc2UgbGV0dGVyIGFuZCB1c2UgaW50ZXJjYXBzLjwvbGk+Cg== 99107 2011-06-29 09:52:52 -0700 2011-06-29 10:57:13 -0700 Patch bug-63564-20110629135250.patch text/plain 1869 leandro SW5kZXg6IFdlYnNpdGVzL3dlYmtpdC5vcmcvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYnNp dGVzL3dlYmtpdC5vcmcvQ2hhbmdlTG9nCShyZXZpc2lvbiA4OTk2MSkKKysrIFdlYnNpdGVzL3dl YmtpdC5vcmcvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTEt MDYtMjggIExlYW5kcm8gUGVyZWlyYSAgPGxlYW5kcm9AcHJvZnVzaW9uLm1vYmk+CisKKyAgICAg ICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQWRkIG5vdGUgYWJvdXQg Y2FzZXMgd2hlcmUgZW51bXMgYXJlIHByZWZlcnJlZCBvdmVyIGJvb2xzIG9uIGZ1bmN0aW9uIHBh cmFtZXRlcnMuCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9p ZD02MzU2NAorCisgICAgICAgIFRoaXMgbWF0Y2hlcyB0aGUgb3V0Y29tZSBvZiB0aGlzIHdlYmtp dC1kZXYgdGhyZWFkOgorICAgICAgICBodHRwczovL2xpc3RzLndlYmtpdC5vcmcvcGlwZXJtYWls L3dlYmtpdC1kZXYvMjAxMC1EZWNlbWJlci8wMTUxOTIuaHRtbAorCisgICAgICAgICogY29kaW5n L2NvZGluZy1zdHlsZS5odG1sOgorCiAyMDExLTA1LTMxICBDaHJpcyBFdmFucyAgPGNldmFuc0Bj aHJvbWl1bS5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgRHJldyBZYW8uCkluZGV4OiBXZWJz aXRlcy93ZWJraXQub3JnL2NvZGluZy9jb2Rpbmctc3R5bGUuaHRtbAo9PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBX ZWJzaXRlcy93ZWJraXQub3JnL2NvZGluZy9jb2Rpbmctc3R5bGUuaHRtbAkocmV2aXNpb24gODk5 MzkpCisrKyBXZWJzaXRlcy93ZWJraXQub3JnL2NvZGluZy9jb2Rpbmctc3R5bGUuaHRtbAkod29y a2luZyBjb3B5KQpAQCAtNjAzLDYgKzYwMywyNCBAQCB2b2lkIGRvU29tZXRoaW5nKFNjcmlwdEV4 ZWN1dGlvbkNvbnRleHQqCiA8L3ByZT4KIDwvbGk+CiAKKzxsaT5QcmVmZXIgZW51bXMgdG8gYm9v bHMgb24gZnVuY3Rpb24gcGFyYW1ldGVycyBpZiBjYWxsZXJzIGFyZSBsaWtlbHkgdG8gYmUKK3Bh c3NpbmcgY29uc3RhbnRzLCBzaW5jZSBuYW1lZCBjb25zdGFudHMgYXJlIGVhc2llciB0byByZWFk IGF0IHRoZSBjYWxsCitzaXRlLiAgQW4gZXhjZXB0aW9uIHRvIHRoaXMgcnVsZSBpcyBhIHNldHRl ciBmdW5jdGlvbiwgd2hlcmUgdGhlIG5hbWUgb2YgdGhlCitmdW5jdGlvbiBhbHJlYWR5IG1ha2Vz IGNsZWFyIHdoYXQgdGhlIGJvb2xlYW4gaXMuCis8aDQgY2xhc3M9InJpZ2h0Ij5SaWdodDo8L2g0 PgorPHByZSBjbGFzcz0iY29kZSI+Citkb1NvbWV0aGluZyhzb21ldGhpbmcsIEFsbG93Rm9vQmFy KTsKK3BhaW50VGV4dFdpdGhTaGFkb3dzKGNvbnRleHQsIC4uLiwgdGV4dFN0cm9rZVdpZHRoID4g MCwgaXNIb3Jpem9udGFsKCkpOworc2V0UmVzaXphYmxlKGZhbHNlKTsKKzwvcHJlPgorCis8aDQg Y2xhc3M9Indyb25nIj5Xcm9uZzo8L2g0PgorPHByZSBjbGFzcz0iY29kZSI+Citkb1NvbWV0aGlu Zyhzb21ldGhpbmcsIGZhbHNlKTsKK3NldFJlc2l6YWJsZShOb3RSZXNpemFibGUpOworPC9wcmU+ Cis8L2xpPgorCiA8bGk+T2JqZWN0aXZlLUMgbWV0aG9kIG5hbWVzIHNob3VsZCBmb2xsb3cgdGhl IENvY29hIG5hbWluZyBndWlkZWxpbmVzICZtZGFzaDsKIHRoZXkgc2hvdWxkIHJlYWQgbGlrZSBh IHBocmFzZSBhbmQgZWFjaCBwaWVjZSBvZiB0aGUgc2VsZWN0b3Igc2hvdWxkIAogc3RhcnQgd2l0 aCBhIGxvd2VyY2FzZSBsZXR0ZXIgYW5kIHVzZSBpbnRlcmNhcHMuPC9saT4K