Bug 33819 – Style in WebCore/bridge/jni/jsc/JavaClassJSC needs fixing

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Steve Block]

Style in WebCore/bridge/jni/jsc/JavaClassJSC needs fixing

[Steve Block]

Created attachment 46871 [details]
Patch 1 for Bug 33819

[Darin Adler]

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

[Steve Block]

Created attachment 46924 [details]
Patch 2 for Bug 33819

Have addressed all comments.

[David Levin]

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.

[Darin Adler]

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

[Steve Block]

Fixed patch according to Darin's comments

Landed manually as http://trac.webkit.org/changeset/53489

Closing bug as resolved