[webkit-reviews] review granted: [Bug 33914] Style in WebCore/bridge/jni/JNIBridge needs fixing : [Attachment 47050] Patch

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has granted Steve Block
<steveblock at google.com>'s request for review:
Bug 33914: Style in WebCore/bridge/jni/JNIBridge needs fixing
https://bugs.webkit.org/show_bug.cgi?id=33914

Attachment 47050: Patch
https://bugs.webkit.org/attachment.cgi?id=47050&action=review

------- Additional Comments from David Levin <levin at chromium.org>
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"