[webkit-reviews] review denied: [Bug 26623] [Chromium] Upstream V8Proxy : [Attachment 31673] patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jun 22 21:48:12 PDT 2009


David Levin <levin at chromium.org> has denied Nate Chapin
<nate.chapin at gmail.com>'s request for review:
Bug 26623: [Chromium] Upstream V8Proxy
https://bugs.webkit.org/show_bug.cgi?id=26623

Attachment 31673: patch
https://bugs.webkit.org/attachment.cgi?id=31673&action=review

------- Additional Comments from David Levin <levin at chromium.org>
Thanks Nate for taking this on.  I know it is huge.  However, I think this
needs to be scrubbed a bit more before an in depth review.  

Here's some steps to do to get this in better shape:

1. s/NULL/0/
2. Find all "!= 0" and "== 0" and get rid of them, replacing with appropriate
logic.
3. Find variables_like_this and replace with variablesLikeThis.
4. getRidOfAbbrInVarNmes
5. Get rid of USE_VAR and replace with UNUSED_PARAM(whateverVariable); (from
<wtf/UnusedParam.h>).
6. Change static locals in functions to use DEFINE_STATIC_LOCAL from
wtf/StdLibExtras.h
7. Variable names should be descriptive words and "a" doesn't count :)	(Loop
variables and iterators are the rare exception.)
8. End of line comments should only have one space before them.
9. Fix indentation in V8Proxy::SVGObjectWithContextToV8Object (actually there
are several other places with 2 space indenting, scan the file and they will
pop out at you).
10. Remove comments like "// static"
11. Get rid of variable names in function definitions when they add no
information.  For example: "void AddToPage(Page* page) const;" but there are
others.
12. Change MethodNames to be methodNames.
13. Spaces around |
14. Replace ASSERT(false); with ASSERT_NOT_REACHED() (from wtf/Assertions.h).
15. At about line 3128, the function bracing is at the end of line.  For
example "void V8Proxy::CreateUtilityContext() {"
16. TODO(*): -> FIXME:


More information about the webkit-reviews mailing list