[RFC][icedtea-web] Fix usage of == -> .equals for various String comparisons

[Jiri Vanek]

On 05/15/2012 10:15 PM, Adam Domurad wrote:
> Hey, first try at a commit.
>
> I went through the source of IcedTeaWeb with FindBugs and went over all reported cases of == being used to compare String's. Some usage cases were valid (eg, .equals eventually called, magic String value). I noted one such usage case. The others were changed to .equals calls.

I have little bit more issues with this, see below and do not shoot me;)
>
> Changelog entry:
> 2012-05-15  Adam Domurad<adomurad at redhat.com>
>
> 	Fixed uses of == to compare String objects to .equals where appropriate
> 	noted a non-obvious use of == to compare a 'magic' String reference.
> 	* netx/net/sourceforge/jnlp/JNLPFile.java: and
> 	* plugin/icedteanp/java/sun/applet/GetMemberPluginCallRequest.java: and
> 	* plugin/icedteanp/java/sun/applet/PluginCallRequestFactory.java:
> 	changed calls that compare String contents from == to .equals
> 	* netx/net/sourceforge/jnlp/Version.java: added comment explaining why
> 	== was used vs .equals
>
>

As Deepak already mentioned.
> findbugs-assisted-string-equals-fix.patch
>
>
> diff --git a/netx/net/sourceforge/jnlp/JNLPFile.java b/netx/net/sourceforge/jnlp/JNLPFile.java
> --- a/netx/net/sourceforge/jnlp/JNLPFile.java
> +++ b/netx/net/sourceforge/jnlp/JNLPFile.java
> @@ -208,7 +208,7 @@ public class JNLPFile {
>           //(i.e. If the jnlp file being launched exist locally, but it
>           //originated from a website, then download the one from the website
>           //into the cache).
> -        if (sourceLocation != null&&  location.getProtocol() == "file") {
> +        if (sourceLocation != null&&  location.getProtocol().equals("file")) {
>               openURL(sourceLocation, version, policy);
>           }
>

definitely: if (sourceLocation != null &&  "file".equals(location.getProtocol())) {

In your case NpE is possible, in my it will return false instead. And even if it is not possible to 
have null, then it is good habit to have constant ewuals variable and not vice versa.

I would also recommend move "file" to constant. Not sure if there is some exact place for this.


> diff --git a/netx/net/sourceforge/jnlp/SecurityDesc.java b/netx/net/sourceforge/jnlp/SecurityDesc.java
> --- a/netx/net/sourceforge/jnlp/SecurityDesc.java
> +++ b/netx/net/sourceforge/jnlp/SecurityDesc.java
> @@ -202,7 +202,7 @@ public class SecurityDesc {
>           PermissionCollection permissions = getSandBoxPermissions();
>
>           // discard sandbox, give all
> -        if (type == ALL_PERMISSIONS) {
> +        if (type.equals(ALL_PERMISSIONS)) {

Same as above

>               permissions = new Permissions();
>               if (customTrustedPolicy == null) {
>                   permissions.add(new AllPermission());
> @@ -213,7 +213,7 @@ public class SecurityDesc {
>           }
>
>           // add j2ee to sandbox if needed
> -        if (type == J2EE_PERMISSIONS)
> +        if (type.equals(J2EE_PERMISSIONS))

Same as above
>               for (int i = 0; i<  j2eePermissions.length; i++)
>                   permissions.add(j2eePermissions[i]);
>
> diff --git a/netx/net/sourceforge/jnlp/Version.java b/netx/net/sourceforge/jnlp/Version.java
> --- a/netx/net/sourceforge/jnlp/Version.java
> +++ b/netx/net/sourceforge/jnlp/Version.java
> @@ -230,6 +230,7 @@ public class Version {
>           Integer number2 = Integer.valueOf(0);
>
>           // compare as integers
> +        // for normalization key, compare exact object, not using .equals
>           try {
>               if (!(part1 == emptyString)) // compare to magic normalization key
>                   number1 = Integer.valueOf(part1);
> @@ -242,9 +243,9 @@ public class Version {
>               // means to compare as strings
>           }
>
> -        if (part1 == emptyString)
> +        if (part1 == emptyString) // compare to magic normalization key
>               part1 = "";

well this is really funny :)

> -        if (part2 == emptyString)
> +        if (part2 == emptyString) // compare to magic normalization key
>               part2 = "";
>
>           return part1.compareTo(part2);
> diff --git a/plugin/icedteanp/java/sun/applet/GetMemberPluginCallRequest.java b/plugin/icedteanp/java/sun/applet/GetMemberPluginCallRequest.java
> --- a/plugin/icedteanp/java/sun/applet/GetMemberPluginCallRequest.java
> +++ b/plugin/icedteanp/java/sun/applet/GetMemberPluginCallRequest.java
> @@ -50,7 +50,7 @@ public class GetMemberPluginCallRequest
>           String[] args = message.split(" ");
>           // FIXME: Is it even possible to distinguish between null and void
>           // here?
> -        if (args[3] != "null"&&  args[3] != "void")
> +        if (args[3].equals("null")&&  args[3].equals("void"))\

The same as (2x) above  and the same with constant extraction.

hmhm If those "null" "void" or "file" are too wide-spread then it will be probably better as 
separate patch - so as You wish!

>               object = AppletSecurityContextManager.getSecurityContext(0).getObject(Integer.parseInt(args[3]));
>           setDone(true);
>       }
> diff --git a/plugin/icedteanp/java/sun/applet/PluginCallRequestFactory.java b/plugin/icedteanp/java/sun/applet/PluginCallRequestFactory.java
> --- a/plugin/icedteanp/java/sun/applet/PluginCallRequestFactory.java
> +++ b/plugin/icedteanp/java/sun/applet/PluginCallRequestFactory.java
> @@ -41,15 +41,15 @@ public class PluginCallRequestFactory {
>
>       public PluginCallRequest getPluginCallRequest(String id, String message, Long reference) {
>
> -        if (id == "member") {
> +        if (id.equals("member")) {
>               return new GetMemberPluginCallRequest(message, reference);
> -        } else if (id == "void") {
> +        } else if (id.equals("void")) {
>               return new VoidPluginCallRequest(message, reference);
> -        } else if (id == "window") {
> +        } else if (id.equals("window")) {
>               return new GetWindowPluginCallRequest(message, reference);
> -        } else if (id == "proxyinfo") {
> +        } else if (id.equals("proxyinfo")) {
>               return new PluginProxyInfoRequest(message, reference);
> -        } else if (id == "cookieinfo") {
> +        } else if (id.equals("cookieinfo")) {
>               return new PluginCookieInfoRequest(message, reference);
>           } else {
>               throw new RuntimeException("Unknown plugin call request type requested from factory");
>
Again  - swap constant and variable.


Deepak:

 >> -        if (args[3] != "null" && args[3] != "void")
 >> +        if (args[3].equals("null") && args[3].equals("void"))
 >
 >This changes the logic. Should be:
 >
 >	if (!args[3].equals("null") && !args[3].equals("void")) .

Again swap of constant x variable, and I would definitely like to see the final patch once more.

 >
 > Rest looks good. After the fixing the above, OK for HEAD/1.1/1.2	

Are you sure with branches?

Imho - I' m not against, although I think it is not necessary. It is not doing any harm in any of 
both ways. But I would like to wait a while before head and back-ports.  eg week?


Adam tahnx for running find bugs! No one wanted to do it (fear from bugs? :)

Deepak - sorry partial contradiction :(( /me ashamed


J.