Bug 33819 - Style in WebCore/bridge/jni/jsc/JavaClassJSC needs fixing
Summary: Style in WebCore/bridge/jni/jsc/JavaClassJSC needs fixing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Steve Block
URL:
Keywords:
Depends on:
Blocks: 33561
  Show dependency treegraph
 
Reported: 2010-01-18 17:22 PST by Steve Block
Modified: 2010-01-19 13:53 PST (History)
4 users (show)

See Also:


Attachments
Patch 1 for Bug 33819 (8.46 KB, patch)
2010-01-18 17:52 PST, Steve Block
darin: review-
Details | Formatted Diff | Diff
Patch 2 for Bug 33819 (8.57 KB, patch)
2010-01-19 10:24 PST, Steve Block
levin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steve Block 2010-01-18 17:22:30 PST
Style in WebCore/bridge/jni/jsc/JavaClassJSC needs fixing
Comment 1 Steve Block 2010-01-18 17:52:17 PST
Created attachment 46871 [details]
Patch 1 for Bug 33819
Comment 2 Darin Adler 2010-01-19 09:09:08 PST
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
Comment 3 Steve Block 2010-01-19 10:24:39 PST
Created attachment 46924 [details]
Patch 2 for Bug 33819

Have addressed all comments.
Comment 4 David Levin 2010-01-19 10:57:44 PST
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.
Comment 5 Darin Adler 2010-01-19 12:10:45 PST
(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.
Comment 6 Steve Block 2010-01-19 13:53:35 PST
Fixed patch according to Darin's comments

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

Closing bug as resolved