33771 2010-01-17 09:22:19 -0800 style-check script reports loads of errors on gtk2drawing.c 2010-01-18 01:55:58 -0800 1 1 1 Unclassified WebKit Tools / Tests 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 32833 1 gustavo webkit-unassigned abarth cjerdonek commit-queue hamaji mrobinson oldest_to_newest 181605 0 gustavo 2010-01-17 09:22:19 -0800 Files WebCore/platform/gtk/gtk2drawing.c and WebCore/platform/gtk/gtk2drawing.h are special - they come from the Mozilla source tree, and we keep style modifications to a minimum on them - I would recommend simply ignoring them. 181627 1 46762 abarth 2010-01-17 15:34:28 -0800 Created attachment 46762 Patch 181635 2 abarth 2010-01-17 15:48:05 -0800 *** Bug 33609 has been marked as a duplicate of this bug. *** 181682 3 46762 darin 2010-01-17 18:09:47 -0800 Comment on attachment 46762 Patch Looks fine, rs=me 181689 4 46762 commit-queue 2010-01-17 18:39:20 -0800 Comment on attachment 46762 Patch Clearing flags on attachment: 46762 Committed r53383: <http://trac.webkit.org/changeset/53383> 181690 5 commit-queue 2010-01-17 18:39:24 -0800 All reviewed patches have been landed. Closing bug. 181701 6 cjerdonek 2010-01-17 19:22:45 -0800 With this fix, will the style bot still log a single error? sys.stderr.write('Ignoring %s; This file is exempt from the style guide.\n' % filename) With files to exempt, I just wanted to clarify whether they should be silently ignored, or if they should be skipped after logging a single line. (Again, I'm not sure if the style bot is smart enough to distinguish between different types of errors logged to sys.stderr.write.) 181776 7 abarth 2010-01-18 01:18:13 -0800 (In reply to comment #6) > With this fix, will the style bot still log a single error? The style bot is smart enough to know whether there were any errors. It looks at the exit code to figure out if the check passed. > With files to exempt, I just wanted to clarify whether they should be silently > ignored, or if they should be skipped after logging a single line. I put the logging in to make sure that folks using the tool know that they aren't getting coverage on those files. It will show up in bugzilla if there are other errors caught by the bot, but it won't generate a bug post on its own. 181779 8 cjerdonek 2010-01-18 01:35:24 -0800 (In reply to comment #7) > (In reply to comment #6) > > With this fix, will the style bot still log a single error? > > The style bot is smart enough to know whether there were any errors. It looks > at the exit code to figure out if the check passed. > > > With files to exempt, I just wanted to clarify whether they should be silently > > ignored, or if they should be skipped after logging a single line. > > I put the logging in to make sure that folks using the tool know that they > aren't getting coverage on those files. It will show up in bugzilla if there > are other errors caught by the bot, but it won't generate a bug post on its > own. Okay, thanks, Adam. Just to clarify, then, it seems like there are two classes of file exemptions: (1) exemptions-with-warnings, e.g. files named "gtk2drawing.c" and files in the folder "WebKit/qt/Api/", and (2) exemptions-without-warnings, e.g. all files in the "LayoutTests" folder. If that's correct, I will make an explicit note of this in the code in my next patch. Can you provide a rough rule of thumb as to which type new exemptions should belong to? For example, which type should the "JavaScriptGlue" folder belong to? https://lists.webkit.org/pipermail/webkit-dev/2009-December/011047.html (e-mail referencing JavaScriptGlue) 181790 9 abarth 2010-01-18 01:55:58 -0800 > Okay, thanks, Adam. Just to clarify, then, it seems like there are two classes > of file exemptions: > > (1) exemptions-with-warnings, e.g. files named "gtk2drawing.c" > and files in the folder "WebKit/qt/Api/", and > (2) exemptions-without-warnings, e.g. all files in the "LayoutTests" > folder. Yes. Although the code calls (2) can_handle. > If that's correct, I will make an explicit note of this in the code in my next > patch. Can you provide a rough rule of thumb as to which type new exemptions > should belong to? It's some optimization problem between the principle of least surprise and not spamming useless information. For example, I don't think anyone would be surprised that the LayoutTests aren't covered and if we included a warning that warning would be spammed on every patch. On the other hand, it's kind of surprising that don't have style coverage for gtk2drawing.c and that file isn't modified that often. That makes printing the error a better trade-off. > For example, which type should the "JavaScriptGlue" folder > belong to? I'd print the warning. I've never seen that folder changed so the spam problem is quite low. 46762 2010-01-17 15:34:28 -0800 2010-01-17 18:39:19 -0800 Patch bug-33771-20100117153426.patch text/plain 3346 abarth SW5kZXg6IFdlYktpdFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBXZWJLaXRUb29scy9D aGFuZ2VMb2cJKHJldmlzaW9uIDUzMzcyKQorKysgV2ViS2l0VG9vbHMvQ2hhbmdlTG9nCSh3b3Jr aW5nIGNvcHkpCkBAIC0xLDMgKzEsMTYgQEAKKzIwMTAtMDEtMTcgIEFkYW0gQmFydGggIDxhYmFy dGhAd2Via2l0Lm9yZz4KKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKwor ICAgICAgICBzdHlsZS1jaGVjayBzY3JpcHQgcmVwb3J0cyBsb2FkcyBvZiBlcnJvcnMgb24gZ3Rr MmRyYXdpbmcuYworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9MzM3NzEKKworICAgICAgICBFeGVtcHQgV2ViQ29yZS9wbGF0Zm9ybS9ndGsvZ3RrMmRyYXdp bmcuYyBhbmQKKyAgICAgICAgV2ViQ29yZS9wbGF0Zm9ybS9ndGsvZ3RrMmRyYXdpbmcuaCBmcm9t IHN0eWxlIGNoZWNrcy4KKworICAgICAgICAqIFNjcmlwdHMvd2Via2l0cHkvc3R5bGUvY3BwX3N0 eWxlLnB5OgorICAgICAgICAqIFNjcmlwdHMvd2Via2l0cHkvc3R5bGUvY3BwX3N0eWxlX3VuaXR0 ZXN0LnB5OgorCiAyMDEwLTAxLTE1ICBKb24gSG9uZXljdXR0ICA8amhvbmV5Y3V0dEBhcHBsZS5j b20+CiAKICAgICAgICAgZ2V0X2FjY1BhcmVudCBzaG91bGQgdHJ5IHRvIHJldHJpZXZlIHBhcmVu dCBBY2Nlc3NpYmlsaXR5T2JqZWN0LCBiZWZvcmUKSW5kZXg6IFdlYktpdFRvb2xzL1NjcmlwdHMv d2Via2l0cHkvc3R5bGUvY3BwX3N0eWxlLnB5Cj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYktpdFRvb2xzL1Nj cmlwdHMvd2Via2l0cHkvc3R5bGUvY3BwX3N0eWxlLnB5CShyZXZpc2lvbiA1MzM3MikKKysrIFdl YktpdFRvb2xzL1NjcmlwdHMvd2Via2l0cHkvc3R5bGUvY3BwX3N0eWxlLnB5CSh3b3JraW5nIGNv cHkpCkBAIC0yOTIxLDYgKzI5MjEsOCBAQCBkZWYgcHJvY2Vzc19maWxlKGZpbGVuYW1lLCBlcnJv cik6CiAgICAgIyBzaG91bGQgcmVseSBvbiB0aGUgZXh0ZW5zaW9uLgogICAgIGlmIChmaWxlbmFt ZSAhPSAnLScgYW5kIG5vdCBjYW5faGFuZGxlKGZpbGVuYW1lKSk6CiAgICAgICAgIHN5cy5zdGRl cnIud3JpdGUoJ0lnbm9yaW5nICVzOyBub3QgYSAuY3BwLCAuYyBvciAuaCBmaWxlXG4nICUgZmls ZW5hbWUpCisgICAgZWxpZiBpc19leGVtcHQoZmlsZW5hbWUpOgorICAgICAgICBzeXMuc3RkZXJy LndyaXRlKCdJZ25vcmluZyAlczsgVGhpcyBmaWxlIGlzIGV4ZW1wdCBmcm9tIHRoZSBzdHlsZSBn dWlkZS5cbicgJSBmaWxlbmFtZSkKICAgICBlbHNlOgogICAgICAgICBwcm9jZXNzX2ZpbGVfZGF0 YShmaWxlbmFtZSwgZmlsZV9leHRlbnNpb24sIGxpbmVzLCBlcnJvcikKICAgICAgICAgaWYgY2Fy cmlhZ2VfcmV0dXJuX2ZvdW5kIGFuZCBvcy5saW5lc2VwICE9ICdcclxuJzoKQEAgLTI5MzgsMyAr Mjk0MCwxNyBAQCBkZWYgY2FuX2hhbmRsZShmaWxlbmFtZSk6CiAgICAgICBmaWxlbmFtZTogQSBm aWxlbmFtZS4gSXQgbWF5IGNvbnRhaW4gZGlyZWN0b3J5IG5hbWVzLgogICAgICAiIiIKICAgICBy ZXR1cm4gb3MucGF0aC5zcGxpdGV4dChmaWxlbmFtZSlbMV0gaW4gKCcuaCcsICcuY3BwJywgJy5j JykKKworCitkZWYgaXNfZXhlbXB0KGZpbGVuYW1lKToKKyAgICAiIiJDaGVja3MgaWYgdGhlIGdp dmVuIGZpbGUgaXMgZXhlbXB0IGZyb20gdGhlIHN0eWxlIGd1aWRlLiAgRm9yIGV4YW1wbGUsCisg ICAgc29tZSBmaWxlcyBhcmUgcHVycG9zZWZ1bGx5IG1hbnRhaW5lZCBpbiBNb3ppbGxhIHN0eWxl IHRvIGVhc2UgZnV0dXJlCisgICAgbWVyZ2VzLgorCisgICAgQXJnczoKKyAgICAgIGZpbGVuYW1l OiBBIGZpbGVuYW1lLiBJdCBtYXkgY29udGFpbiBkaXJlY3RvcnkgbmFtZXMuCisgICAgICIiIgor ICAgIHJldHVybiBvcy5wYXRoLmJhc2VuYW1lKGZpbGVuYW1lKSBpbiAoCisgICAgICAgICdndGsy ZHJhd2luZy5jJywKKyAgICAgICAgJ2d0azJkcmF3aW5nLmgnLAorICAgICkKSW5kZXg6IFdlYktp dFRvb2xzL1NjcmlwdHMvd2Via2l0cHkvc3R5bGUvY3BwX3N0eWxlX3VuaXR0ZXN0LnB5Cj09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT0KLS0tIFdlYktpdFRvb2xzL1NjcmlwdHMvd2Via2l0cHkvc3R5bGUvY3BwX3N0eWxlX3Vu aXR0ZXN0LnB5CShyZXZpc2lvbiA1MzM3MikKKysrIFdlYktpdFRvb2xzL1NjcmlwdHMvd2Via2l0 cHkvc3R5bGUvY3BwX3N0eWxlX3VuaXR0ZXN0LnB5CSh3b3JraW5nIGNvcHkpCkBAIC0zNTc4LDYg KzM1NzgsMTggQEAgY2xhc3MgV2ViS2l0U3R5bGVUZXN0KENwcFN0eWxlVGVzdEJhc2UpOgogICAg ICAgICBzZWxmLmFzc2VydF8obm90IGNwcF9zdHlsZS5jYW5faGFuZGxlKCdmb28ubW0nKSkKICAg ICAgICAgc2VsZi5hc3NlcnRfKG5vdCBjcHBfc3R5bGUuY2FuX2hhbmRsZSgnLScpKQogCisgICAg ZGVmIHRlc3RfaXNfZXhlbXB0KHNlbGYpOgorICAgICAgICAiIiJUZXN0cyBmb3IgY3BwX3N0eWxl LmlzX2V4ZW1wdCgpLiIiIgorICAgICAgICBzZWxmLmFzc2VydF8obm90IGNwcF9zdHlsZS5pc19l eGVtcHQoJycpKQorICAgICAgICBzZWxmLmFzc2VydF8obm90IGNwcF9zdHlsZS5pc19leGVtcHQo J2Zvby5oJykpCisgICAgICAgIHNlbGYuYXNzZXJ0Xyhub3QgY3BwX3N0eWxlLmlzX2V4ZW1wdCgn Zm9vLmhwcCcpKQorICAgICAgICBzZWxmLmFzc2VydF8obm90IGNwcF9zdHlsZS5pc19leGVtcHQo J2Zvby5jJykpCisgICAgICAgIHNlbGYuYXNzZXJ0Xyhub3QgY3BwX3N0eWxlLmlzX2V4ZW1wdCgn Zm9vLmNwcCcpKQorICAgICAgICBzZWxmLmFzc2VydF8obm90IGNwcF9zdHlsZS5pc19leGVtcHQo Jy0nKSkKKyAgICAgICAgc2VsZi5hc3NlcnRfKGNwcF9zdHlsZS5pc19leGVtcHQoJ2d0azJkcmF3 aW5nLmgnKSkKKyAgICAgICAgc2VsZi5hc3NlcnRfKGNwcF9zdHlsZS5pc19leGVtcHQoJ1dlYkNv cmUvcGxhdGZvcm0vZ3RrL2d0azJkcmF3aW5nLmgnKSkKKyAgICAgICAgc2VsZi5hc3NlcnRfKGNw cF9zdHlsZS5pc19leGVtcHQoJ2d0azJkcmF3aW5nLmMnKSkKKyAgICAgICAgc2VsZi5hc3NlcnRf KGNwcF9zdHlsZS5pc19leGVtcHQoJ1dlYkNvcmUvcGxhdGZvcm0vZ3RrL2d0azJkcmF3aW5nLmMn KSkKIAogZGVmIHRlYXJEb3duKCk6CiAgICAgIiIiQSBnbG9iYWwgY2hlY2sgdG8gbWFrZSBzdXJl IGFsbCBlcnJvci1jYXRlZ29yaWVzIGhhdmUgYmVlbiB0ZXN0ZWQuCg==