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

[Deepak Bhole]

* Jiri Vanek <jvanek at redhat.com> [2012-05-16 04:51]:
> 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


No worries. I had missed the NPE possibilities, glad you and Pavel
reviewed it and caught them. Thanks!

+1 on waiting for stable branches as this seems to be more risky than I
had thought.

Cheers,
Deepak

> 
> 
> J.