Bug 33914 - Style in WebCore/bridge/jni/JNIBridge needs fixing
Summary: Style in WebCore/bridge/jni/JNIBridge 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: Nobody
URL:
Keywords:
Depends on:
Blocks: 33899
  Show dependency treegraph
 
Reported: 2010-01-20 11:14 PST by Steve Block
Modified: 2010-01-21 02:26 PST (History)
2 users (show)

See Also:


Attachments
Patch (34.30 KB, patch)
2010-01-20 11:22 PST, Steve Block
no flags Details | Formatted Diff | Diff
Patch (34.32 KB, patch)
2010-01-20 11:31 PST, Steve Block
levin: review+
levin: commit-queue-
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-20 11:14:23 PST
Style in WebCore/bridge/jni/JNIBridge needs fixing.

See Bug 33899, which this bug blocks.
Comment 1 Steve Block 2010-01-20 11:22:08 PST
Created attachment 47049 [details]
Patch
Comment 2 Steve Block 2010-01-20 11:31:17 PST
Created attachment 47050 [details]
Patch
Comment 3 David Levin 2010-01-20 15:20:10 PST
Comment on attachment 47050 [details]
Patch

Looks fine overall. I've noted a few nits below that would be nice to fix on landing.


Consider adding the copyright for Google to files you are changing since you are changing more than 10 lines.


> Index: WebCore/bridge/jni/JNIBridge.cpp

> +JavaMethod::JavaMethod(JNIEnv* env, jobject aMethod)
> +    jstring returnTypeName = static_cast<jstring>(callJNIMethod<jobject>(returnType, "getName", "()Ljava/lang/String;"));
> +    m_returnType =JavaString(env, returnTypeName);

Please add a space after the =.


> +    m_JNIReturnType = JNITypeFromClassName(m_returnType.UTF8String());
> +    env->DeleteLocalRef(returnType);
> +    env->DeleteLocalRef(returnTypeName);
>  
>      // Get method name
> -    jstring methodName = (jstring)callJNIMethod<jobject>(aMethod, "getName", "()Ljava/lang/String;");
> -    _name = JavaString (env, methodName);
> -    env->DeleteLocalRef (methodName);
> +    jstring methodName = static_cast<jstring>(callJNIMethod<jobject>(aMethod, "getName", "()Ljava/lang/String;"));
> +    m_name = JavaString(env, methodName);
> +    env->DeleteLocalRef(methodName);
>  
>      // Get parameters
> -    jarray jparameters = (jarray)callJNIMethod<jobject>(aMethod, "getParameterTypes", "()[Ljava/lang/Class;");
> -    _numParameters = env->GetArrayLength (jparameters);
> -    _parameters = new JavaParameter[_numParameters];
> -    
> +    jarray jparameters = static_cast<jarray>(callJNIMethod<jobject>(aMethod, "getParameterTypes", "()[Ljava/lang/Class;"));
> +    m_numParameters = env->GetArrayLength(jparameters);
> +    m_parameters = new JavaParameter[m_numParameters];
> +
>      int i;

It would be nice to move this in the loop (e.g. "for (int i = 0;....")


> +    for (i = 0; i < m_numParameters; i++) {
> +        jobject aParameter = env->GetObjectArrayElement((jobjectArray)jparameters, i);

It looks like you missed converting a cast here to the *_cast style.


> +JavaMethod::~JavaMethod()
>  {
> -    if (_signature)
> -        free(_signature);
> -    delete [] _parameters;
> +    if (m_signature)
> +        free(m_signature);
> +    delete [] m_parameters;

remove the space before []

>  };
>  
>  // JNI method signatures use '/' between components of a class name, but
> @@ -302,244 +310,260 @@ JavaMethod::~JavaMethod() 
>  static void appendClassName(StringBuilder& builder, const char* className)
>  {
>      ASSERT(JSLock::lockCount() > 0);
> -    
> -    char *result, *cp = strdup(className);
> -    
> +
> +    char* result;

Would be nice to move result to where it is initialized.

> +    char* cp = strdup(className);

cp is a rather bad variable name but I guess we don't need one for this code. Maybe just use "c"?

> +
>      result = cp;
>      while (*cp) {
>          if (*cp == '.')
>              *cp = '/';
>          cp++;
>      }
> -        
> +
>      builder.append(result);
>  
>      free(result);
>  }

>  JSValue JavaArray::valueAt(ExecState* exec, unsigned index) const
>  {
> +            if (m_type[1] == '[')
> +                return JavaArray::convertJObjectToArray(exec, anObject, m_type+1, rootObject());

Add spaces around +

> Index: WebCore/bridge/jni/JNIBridge.h
> +class JavaParameter {
> +    JavaParameter() : m_JNIType(invalid_type) { };

No ; needed after the }

> +    JavaParameter(JNIEnv* env, jstring type);

No need to put "env" here.


> +class JavaField : public Field {
> +    JavaField(JNIEnv* env, jobject aField);

remove "env"

> +    virtual JSValue valueFromInstance(ExecState* exec, const Instance* instance) const;

"exec" and "instance" may be removed here.

> +    virtual void setValueToInstance(ExecState* exec, const Instance* instance, JSValue aValue) const;

"exec", "instance", and "aValue" may be removed here.

> +    void dispatchSetValueToInstance(ExecState* exec, const JavaInstance* instance, jvalue javaValue, const char* name, const char* sig) const;

"exec" and "instance" may be removed here. ("javaValue" also seems redundant to me.)


> +    jvalue dispatchValueFromInstance(ExecState* exec, const JavaInstance* instance, const char* name, const char* sig, JNIType returnType) const;

"exec" and "instance" may be removed here.


> +class JavaMethod : public Method {
>  public:
>      JavaMethod(JNIEnv* env, jobject aMethod);

remove "env"


> +    jmethodID methodID(jobject obj) const;

remove "obj"


> +    virtual void setValueAt(ExecState* exec, unsigned int index, JSValue aValue) const;

remove "exec"

> +    virtual JSValue valueAt(ExecState* exec, unsigned int index) const;

remove "exec"

>      virtual unsigned int getLength() const;
> -    
> -    jobject javaArray() const { return _array->m_instance; }
>  
> -    static JSValue convertJObjectToArray (ExecState* exec, jobject anObject, const char* type, PassRefPtr<RootObject>);
> +    jobject javaArray() const { return m_array->m_instance; }
> +
> +    static JSValue convertJObjectToArray(ExecState* exec, jobject anObject, const char* type, PassRefPtr<RootObject>);

remove "exec", "anOjbect"
Comment 4 Steve Block 2010-01-21 02:26:58 PST
Addressed all comments

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

Closing bug as resolved