Bug 33914 – Style in WebCore/bridge/jni/JNIBridge needs fixing

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

[Steve Block]

Style in WebCore/bridge/jni/JNIBridge needs fixing.

See Bug 33899, which this bug blocks.

[Steve Block]

Created attachment 47049 [details]
Patch

[Steve Block]

Created attachment 47050 [details]
Patch

[David Levin]

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"

[Steve Block]

Addressed all comments

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

Closing bug as resolved