Bug 31454

Summary:

WebKit Coding Style Guidelines should say not to use "using namespace" statements in header files

Product:

WebKit

Reporter:

Chris Jerdonek <cjerdonek>

Component:

New Bugs

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Trivial

CC:

commit-queue, darin

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

URL:

https://lists.webkit.org/pipermail/webkit-dev/2009-November/010434.html

Attachments:

Description	Flags
Patch submitted	none
New patch: added some closing </li>'s	none

Chris Jerdonek

Reported 2009-11-12 20:28:01 PST

Created attachment 43130 [details] 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.

Attachments
Patch submitted (1.04 KB, patch) 2009-11-12 20:28 PST, Chris Jerdonek	no flags	Details Formatted Diff Diff
New patch: added some closing </li>'s (1.25 KB, patch) 2009-11-12 23:06 PST, Chris Jerdonek	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Darin Adler

Comment 1 2009-11-12 21:19:47 PST

Comment on attachment 43130 [details] Patch submitted Need a </li>.

Chris Jerdonek

Comment 2 2009-11-12 23:06:23 PST

Created attachment 43136 [details] 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.

Chris Jerdonek

Comment 3 2009-11-12 23:08:22 PST

Comment on attachment 43136 [details] New patch: added some closing </li>'s Marking previous attachment as a patch

Darin Adler

Comment 4 2009-11-13 08:40:02 PST

Comment on attachment 43136 [details] 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

Eric Seidel (no email)

Comment 5 2009-11-13 12:32:58 PST

Comment on attachment 43130 [details] 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.

WebKit Commit Bot

Comment 6 2009-11-13 13:59:49 PST

Comment on attachment 43136 [details] 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

Eric Seidel (no email)

Comment 7 2009-11-13 14:01:18 PST

Comment on attachment 43136 [details] New patch: added some closing </li>'s Not your fault. :( Bug 31461 again.

WebKit Commit Bot

Comment 8 2009-11-13 14:12:42 PST

Comment on attachment 43136 [details] 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)

Eric Seidel (no email)

Comment 9 2009-11-13 14:14:16 PST

Comment on attachment 43136 [details] 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. :(

WebKit Commit Bot

Comment 10 2009-11-13 16:25:43 PST

Comment on attachment 43136 [details] New patch: added some closing </li>'s Clearing flags on attachment: 43136 Committed r50978: <http://trac.webkit.org/changeset/50978>

WebKit Commit Bot

Comment 11 2009-11-13 16:25:47 PST

All reviewed patches have been landed. Closing bug.

Chris Jerdonek

Comment 12 2009-11-13 17:51:25 PST

(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.

Chris Jerdonek

Comment 13 2009-11-13 18:00:20 PST

(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.

Darin Adler

Comment 14 2009-11-14 11:13:35 PST

(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.

Note You need to log in before you can comment on or make changes to this bug.