31454 2009-11-12 20:28:01 -0800 WebKit Coding Style Guidelines should say not to use "using namespace" statements in header files 2009-11-14 11:13:35 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) All All RESOLVED FIXED https://lists.webkit.org/pipermail/webkit-dev/2009-November/010434.html P4 Trivial --- 0 cjerdonek webkit-unassigned commit-queue darin oldest_to_newest 163190 0 43130 cjerdonek 2009-11-12 20:28:01 -0800 Created attachment 43130 Patch submitted On the webkit-dev e-mail list, Darin Adler said that "using namespace" statements are not permitted in header files (see this report's URL for the e-mail). He said we should clarify that -- hence this report. A patch to the Coding Style Guidelines is attached. Thanks. 163210 1 43130 darin 2009-11-12 21:19:47 -0800 Comment on attachment 43130 Patch submitted Need a </li>. 163229 2 43136 cjerdonek 2009-11-12 23:06:23 -0800 Created attachment 43136 New patch: added some closing </li>'s The surrounding sections did not have closing </li>'s, which is what I was going by. (The entire document is inconsistent in this regard.) I am attaching a new patch with closing </li>'s for the three list items in the "using namespace" section. Thanks. 163231 3 43136 cjerdonek 2009-11-12 23:08:22 -0800 Comment on attachment 43136 New patch: added some closing </li>'s Marking previous attachment as a patch 163319 4 43136 darin 2009-11-13 08:40:02 -0800 Comment on attachment 43136 New patch: added some closing </li>'s Someone should run the file through an HTML validator and fix issues they find. r=me on this change 163447 5 43130 eric 2009-11-13 12:32:58 -0800 Comment on attachment 43130 Patch submitted Be sure to mark your old patches as obsolete when uploading replacement patches. Some tools, like bugzilla-tool post-diff or bugzilla-tool post-commits will do this automatically for you. 163493 6 43136 commit-queue 2009-11-13 13:59:49 -0800 Comment on attachment 43136 New patch: added some closing </li>'s Rejecting patch 43136 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 11621 test cases. fast/workers/worker-terminate.html -> crashed Exiting early after 1 failures. 8405 tests run. 361.78s total testing time 8404 test cases (99%) succeeded 1 test case (<1%) crashed 3 test cases (<1%) had stderr output 163494 7 43136 eric 2009-11-13 14:01:18 -0800 Comment on attachment 43136 New patch: added some closing </li>'s Not your fault. :( Bug 31461 again. 163513 8 43136 commit-queue 2009-11-13 14:12:42 -0800 Comment on attachment 43136 New patch: added some closing </li>'s Rejecting patch 43136 from commit-queue. Failed to run "WebKitTools/Scripts/build-webkit" exit_code: 1 Last 500 characters of output: ld/WebCore.build/Release/WebCore.build/Objects-normal/i386/JSCompositionEvent.o /Users/eseidel/Projects/CommitQueue/WebKitBuild/Release/DerivedSources/WebCore/JSCompositionEvent.cpp normal i386 c++ com.apple.compilers.gcc.4_2 Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/i386/JSEventCustom.o /Users/eseidel/Projects/CommitQueue/WebCore/bindings/js/JSEventCustom.cpp normal i386 c++ com.apple.compilers.gcc.4_2 (2 failures) 163516 9 43136 eric 2009-11-13 14:14:16 -0800 Comment on attachment 43136 New patch: added some closing </li>'s I'm in the process of moving the commit-queue to a new machine and it's had hiccups. Sorry. :( 163582 10 43136 commit-queue 2009-11-13 16:25:43 -0800 Comment on attachment 43136 New patch: added some closing </li>'s Clearing flags on attachment: 43136 Committed r50978: <http://trac.webkit.org/changeset/50978> 163583 11 commit-queue 2009-11-13 16:25:47 -0800 All reviewed patches have been landed. Closing bug. 163612 12 cjerdonek 2009-11-13 17:51:25 -0800 (In reply to comment #4) > (From update of attachment 43136 [details]) > Someone should run the file through an HTML validator and fix issues they find. > > r=me on this change I did this while preparing the patch, and there was only one issue. The <style> tag specifying a style unique to this page (the style for code excerpts) occurs in the <body> rather than the <head>. This is partly a consequence of how the site is structured in source control with the <body> of each page being included, the <head> lying in another file, etc. So I didn't try addressing it in this patch. By the way, the document type for the page is HTML 4.01 strict, which doesn't really require closing <li>'s. If we want to switch to XHTML for the site, perhaps that should be made as a decision. It would be like deciding on a style guideline for the HTML in addition to the code. 163619 13 cjerdonek 2009-11-13 18:00:20 -0800 (In reply to comment #5) > (From update of attachment 43130 [details]) > Be sure to mark your old patches as obsolete when uploading replacement > patches. Some tools, like bugzilla-tool post-diff or bugzilla-tool > post-commits will do this automatically for you. I wanted to give the reviewer the option of selecting either patch, given the extra information I supplied with the second patch. Is there a better way for me to do that? Also, I apologize in advance if I shouldn't be adding comments to a closed bug report. Let me know if there is a better way to handle that as well. Thanks for your help as I learn the ropes. 163673 14 darin 2009-11-14 11:13:35 -0800 (In reply to comment #12) > By the way, the document type for the page is HTML 4.01 strict, which doesn't > really require closing <li>'s. I was surprised to learn this. > If we want to switch to XHTML for the site, perhaps that should be made as a decision. Absolutely not! My desire to close an <li> element was not driven at all by a desire to use XHTML. I just have a habit of closing elements that are not self-closing. My ignorance -- sorry. 43130 2009-11-12 20:28:01 -0800 2009-11-13 12:32:58 -0800 Patch submitted patch.diff text/plain 1063 cjerdonek SW5kZXg6IFdlYktpdFNpdGUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYktpdFNpdGUvQ2hh bmdlTG9nCShyZXZpc2lvbiA1MDkyNCkKKysrIFdlYktpdFNpdGUvQ2hhbmdlTG9nCSh3b3JraW5n IGNvcHkpCkBAIC0xLDMgKzEsMTIgQEAKKzIwMDktMTEtMTIgIENocmlzIEplcmRvbmVrICA8Y2hy aXMuamVyZG9uZWtAZ21haWwuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIEFkZGVkIHRvIHRoZSBXZWJLaXQgQ29kaW5nIFN0eWxlIEd1aWRlbGlu ZXMgbm90IHRvIHVzZSAKKyAgICAgICAgInVzaW5nIG5hbWVzcGFjZSIgc3RhdGVtZW50cyBpbiBo ZWFkZXIgZmlsZXMuCisKKyAgICAgICAgKiBjb2RpbmcvY29kaW5nLXN0eWxlLmh0bWw6CisKIDIw MDktMTEtMTAgIE9saXZlciBIdW50ICA8b2xpdmVyQGFwcGxlLmNvbT4KIAogICAgICAgICBSZXZp ZXdlZCBieSBNYWNpZWogU3RhY2hvd2lhay4KSW5kZXg6IFdlYktpdFNpdGUvY29kaW5nL2NvZGlu Zy1zdHlsZS5odG1sCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYktpdFNpdGUvY29kaW5nL2NvZGluZy1zdHls ZS5odG1sCShyZXZpc2lvbiA1MDkyNCkKKysrIFdlYktpdFNpdGUvY29kaW5nL2NvZGluZy1zdHls ZS5odG1sCSh3b3JraW5nIGNvcHkpCkBAIC03MDUsNiArNzA1LDggQEAgRG9uJ3QgYm90aGVyIHRv IG9yZ2FuaXplIHRoZW0gaW4gYSBsb2dpYwogCiA8b2w+CiAKKzxsaT5EbyBub3QgdXNlICJ1c2lu ZyBuYW1lc3BhY2UiIHN0YXRlbWVudHMgaW4gaGVhZGVyIGZpbGVzLgorCiA8bGk+QW55ICJ1c2lu ZyBuYW1lc3BhY2UiIHN0YXRlbWVudHMgZm9yIGEgbmVzdGVkIG5hbWVzcGFjZSB3aG9zZSBwYXJl bnQgbmFtZXNwYWNlIAogaXMgYWxzbyBkZWZpbmVkIGluIGEgZmlsZSBtdXN0IGJlIGRlY2xhcmVk IHdpdGhpbiB0aGF0IG5hbWVzcGFjZSBkZWZpbml0aW9uLgogCg== 43136 2009-11-12 23:06:23 -0800 2009-11-13 16:25:42 -0800 New patch: added some closing </li>'s patch2.diff text/plain 1285 cjerdonek SW5kZXg6IFdlYktpdFNpdGUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYktpdFNpdGUvQ2hh bmdlTG9nCShyZXZpc2lvbiA1MDkyNCkKKysrIFdlYktpdFNpdGUvQ2hhbmdlTG9nCSh3b3JraW5n IGNvcHkpCkBAIC0xLDMgKzEsMTIgQEAKKzIwMDktMTEtMTIgIENocmlzIEplcmRvbmVrICA8Y2hy aXMuamVyZG9uZWtAZ21haWwuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIEFkZGVkIHRvIHRoZSBXZWJLaXQgQ29kaW5nIFN0eWxlIEd1aWRlbGlu ZXMgbm90IHRvIHVzZSAKKyAgICAgICAgInVzaW5nIG5hbWVzcGFjZSIgc3RhdGVtZW50cyBpbiBo ZWFkZXIgZmlsZXMuCisKKyAgICAgICAgKiBjb2RpbmcvY29kaW5nLXN0eWxlLmh0bWw6CisKIDIw MDktMTEtMTAgIE9saXZlciBIdW50ICA8b2xpdmVyQGFwcGxlLmNvbT4KIAogICAgICAgICBSZXZp ZXdlZCBieSBNYWNpZWogU3RhY2hvd2lhay4KSW5kZXg6IFdlYktpdFNpdGUvY29kaW5nL2NvZGlu Zy1zdHlsZS5odG1sCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYktpdFNpdGUvY29kaW5nL2NvZGluZy1zdHls ZS5odG1sCShyZXZpc2lvbiA1MDkyNCkKKysrIFdlYktpdFNpdGUvY29kaW5nL2NvZGluZy1zdHls ZS5odG1sCSh3b3JraW5nIGNvcHkpCkBAIC03MDUsNiArNzA1LDkgQEAKIAogPG9sPgogCis8bGk+ RG8gbm90IHVzZSAidXNpbmcgbmFtZXNwYWNlIiBzdGF0ZW1lbnRzIGluIGhlYWRlciBmaWxlcy4K KzwvbGk+CisKIDxsaT5BbnkgInVzaW5nIG5hbWVzcGFjZSIgc3RhdGVtZW50cyBmb3IgYSBuZXN0 ZWQgbmFtZXNwYWNlIHdob3NlIHBhcmVudCBuYW1lc3BhY2UgCiBpcyBhbHNvIGRlZmluZWQgaW4g YSBmaWxlIG11c3QgYmUgZGVjbGFyZWQgd2l0aGluIHRoYXQgbmFtZXNwYWNlIGRlZmluaXRpb24u CiAKQEAgLTcyOSw2ICs3MzIsNyBAQAogCiB9IC8vIG5hbWVzcGFjZSBXZWJDb3JlCiA8L3ByZT4K KzwvbGk+CiAKIDxsaT5Bbnkgb3RoZXIgInVzaW5nIG5hbWVzcGFjZSIgc3RhdGVtZW50cyBtdXN0 IGJlIGRlY2xhcmVkIGJlZm9yZSB0aGUgZmlyc3QgbmFtZXNwYWNlIAogZGVmaW5pdGlvbiBpbiB0 aGUgZmlsZS4KQEAgLTc1Myw2ICs3NTcsOCBAQAogCiB9IC8vIG5hbWVzcGFjZSBXZWJDb3JlCiA8 L3ByZT4KKzwvbGk+CisKIDwvb2w+CiAKIDw/cGhwCg==