Summary: | Style in WebCore/bridge/jni/jsc/JavaClassJSC needs fixing | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Steve Block <steveblock> | ||||||
Component: | WebCore Misc. | Assignee: | Steve Block <steveblock> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, android-webkit-unforking, darin, steveblock | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | All | ||||||||
OS: | All | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 33561 | ||||||||
Attachments: |
|
Description
Steve Block
2010-01-18 17:22:30 PST
Created attachment 46871 [details] Patch 1 for Bug 33819 Comment on attachment 46871 [details] Patch 1 for Bug 33819 > Index: WebCore/bridge/jni/jsc/JavaClassJSC.cpp > =================================================================== > --- WebCore/bridge/jni/jsc/JavaClassJSC.cpp (revision 53443) > +++ WebCore/bridge/jni/jsc/JavaClassJSC.cpp (working copy) > @@ -1,5 +1,5 @@ > /* > - * Copyright (C) 2003 Apple Computer, Inc. All rights reserved. > + * Copyright (C) 2010 Apple Computer, Inc. All rights reserved. Should be "Copyright (C) 2003, 2010 Apple Inc. All rights reserved." and should also list any other years the code here had significant changes that were published (checked in to the open source repository). Would take a little research to determine that. > + free((void*)m_name); Since you're fixing style, this should be const_cast<char*>(m_name) instead of (void*)m_name. > - * Copyright (C) 2003 Apple Computer, Inc. All rights reserved. > + * Copyright (C) 2010 Apple Computer, Inc. All rights reserved. Same comment as above. Please don't erase earlier publication years. > + JavaClass(jobject anInstance); The argument name here does not make it any clearer what the argument is. If we can't do any better, then we should omit the argument name. > + virtual MethodList methodsNamed(const Identifier&, Instance* instance) const; > + virtual Field* fieldNamed(const Identifier&, Instance* instance) const; The argument name "instance" should be omitted here. review- because the copyright date should not be changed in this manner Created attachment 46924 [details] Patch 2 for Bug 33819 Have addressed all comments. Comment on attachment 46924 [details] Patch 2 for Bug 33819 > Index: WebCore/bridge/jni/jsc/JavaClassJSC.cpp > =================================================================== > --- WebCore/bridge/jni/jsc/JavaClassJSC.cpp (revision 53450) > +++ WebCore/bridge/jni/jsc/JavaClassJSC.cpp (working copy) > @@ -1,5 +1,5 @@ > /* > - * Copyright (C) 2003 Apple Computer, Inc. All rights reserved. > + * Copyright (C) 2003-2010 Apple Computer, Inc. All rights reserved. Folks prefer the years to be listed explicitly. Feel free to do 2003, 2010 > + return (!strcmp(m_name, "java.lang.Byte") > + || !strcmp(m_name, "java.lang.Short") Ideally this would line up with the parenthesis that is enclosing it like this: return (!strcmp(m_name, "java.lang.Byte") || !strcmp(m_name, "java.lang.Short") > + || !strcmp(m_name, "java.lang.Integer") > + || !strcmp(m_name, "java.lang.Long") > + || !strcmp(m_name, "java.lang.Float") > + || !strcmp(m_name, "java.lang.Double")); > Index: WebCore/bridge/jni/jsc/JavaClassJSC.h > - * Copyright (C) 2003 Apple Computer, Inc. All rights reserved. > + * Copyright (C) 2003-2005, 2007, 2009, 2010 Apple Computer, Inc. All rights reserved. Same comment about year ranges. (In reply to comment #4) > > - * Copyright (C) 2003 Apple Computer, Inc. All rights reserved. > > + * Copyright (C) 2003-2010 Apple Computer, Inc. All rights reserved. > > Folks prefer the years to be listed explicitly. Feel free to do 2003, 2010 Actually: 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010 The company’s name is now Apple Inc. We use one space after the period. > > + return (!strcmp(m_name, "java.lang.Byte") > > + || !strcmp(m_name, "java.lang.Short") > > Ideally this would line up with the parenthesis that is enclosing it like this: > > return (!strcmp(m_name, "java.lang.Byte") > || !strcmp(m_name, "java.lang.Short") I think we don’t do that. The guidelines even say not to. Fixed patch according to Darin's comments Landed manually as http://trac.webkit.org/changeset/53489 Closing bug as resolved |