Cleaning up warnings

Simple clean up
* getting rid of the unused imports
* added .equals() on classes that have a .compare()
* corrected misuse of == instead of .equals() for testing strings
* replace repeated string concat with a stringbuffer

[henry.saputra, 2010-07-12 17:15:07]

http://codereview.appspot.com/1807042/diff/1/2
File
java/common/src/main/java/org/apache/shindig/protocol/DefaultHandlerRegistry.java
(right):

http://codereview.appspot.com/1807042/diff/1/2#newcode624
java/common/src/main/java/org/apache/shindig/protocol/DefaultHandlerRegistry.java:624:
public boolean equals(Object other) {
We should also override the hashCode() function as well when overriding the
equals() to get consistent behavior.

[Jasvir, 2010-07-12 19:51:21]

http://codereview.appspot.com/1807042/diff/1/2
File
java/common/src/main/java/org/apache/shindig/protocol/DefaultHandlerRegistry.java
(right):

http://codereview.appspot.com/1807042/diff/1/2#newcode624
java/common/src/main/java/org/apache/shindig/protocol/DefaultHandlerRegistry.java:624:
public boolean equals(Object other) {
On 2010/07/12 17:15:07, henry.saputra wrote:
> We should also override the hashCode() function as well when overriding the
> equals() to get consistent behavior.

Good catch.  Done.

[johnfargo, 2010-07-12 20:36:33]

http://codereview.appspot.com/1807042/diff/5001/6003
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java
(right):

http://codereview.appspot.com/1807042/diff/5001/6003#newcode65
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java:65:
if (HtmlAccelServlet.ACCEL_GADGET_PARAM_NAME.equals(name)) {
this isn't a misuse -- the ==-check ensures that a requestor cannot fake the
accel_gadget_param_name by passing the param on the query string.

http://codereview.appspot.com/1807042/diff/5001/6004
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java
(right):

http://codereview.appspot.com/1807042/diff/5001/6004#newcode84
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java:84:
query.append('&' + params[i] + '=' + Utf8UrlCoder.encode(params[i+1]));
any reason you chose not to go whole-hog and get rid of the +'s?

[Jasvir, 2010-07-12 21:53:01]

http://codereview.appspot.com/1807042/diff/5001/6003
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java
(right):

http://codereview.appspot.com/1807042/diff/5001/6003#newcode65
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java:65:
if (HtmlAccelServlet.ACCEL_GADGET_PARAM_NAME.equals(name)) {
Ah.  In that case, the original code is incorrect by spec.  The Java spec
requires  identical string literals must refer to the same instance of class
String.  I'll change A_G_P_N to new String().
 
On 2010/07/12 20:36:34, johnfargo wrote:
> this isn't a misuse -- the ==-check ensures that a requestor cannot fake the
> accel_gadget_param_name by passing the param on the query string.

http://codereview.appspot.com/1807042/diff/5001/6004
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java
(right):

http://codereview.appspot.com/1807042/diff/5001/6004#newcode84
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/ProxyHandlerTest.java:84:
query.append('&' + params[i] + '=' + Utf8UrlCoder.encode(params[i+1]));
Going whole-hog!

On 2010/07/12 20:36:34, johnfargo wrote:
> any reason you chose not to go whole-hog and get rid of the +'s?

[henry.saputra, 2010-07-13 01:22:09]

http://codereview.appspot.com/1807042/diff/14001/15001
File
java/common/src/main/java/org/apache/shindig/protocol/DefaultHandlerRegistry.java
(right):

http://codereview.appspot.com/1807042/diff/14001/15001#newcode629
java/common/src/main/java/org/apache/shindig/protocol/DefaultHandlerRegistry.java:629:
that.operationPath == this.operationPath);
Should be using String.equals() method instead of == since
DefaultHandlerRegistry.operationPath has a type of String.

[Jasvir, 2010-07-13 05:32:58]

http://codereview.appspot.com/1807042/diff/14001/15001
File
java/common/src/main/java/org/apache/shindig/protocol/DefaultHandlerRegistry.java
(right):

http://codereview.appspot.com/1807042/diff/14001/15001#newcode629
java/common/src/main/java/org/apache/shindig/protocol/DefaultHandlerRegistry.java:629:
that.operationPath == this.operationPath);
On 2010/07/13 01:22:10, henry.saputra wrote:
> Should be using String.equals() method instead of == since
> DefaultHandlerRegistry.operationPath has a type of String.

Done.

[Jasvir, 2010-07-20 21:32:40]

ping

[Paul Lindner, 2010-07-20 21:56:16]

generally fine..

http://codereview.appspot.com/1807042/diff/18001/19001
File
java/common/src/main/java/org/apache/shindig/protocol/DefaultHandlerRegistry.java
(right):

http://codereview.appspot.com/1807042/diff/18001/19001#newcode629
java/common/src/main/java/org/apache/shindig/protocol/DefaultHandlerRegistry.java:629:
this.operationPath.equals(that.operationPath));
suggest Objects.equal(this.operationPath, that.operationPath) to handle nulls

http://codereview.appspot.com/1807042/diff/18001/19006
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java
(right):

http://codereview.appspot.com/1807042/diff/18001/19006#newcode45
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/HtmlAccelServlet.java:45:
public static final String ACCEL_GADGET_PARAM_NAME = new String("accelGadget");
ugh...

http://codereview.appspot.com/1807042/diff/18001/19003
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java
(right):

http://codereview.appspot.com/1807042/diff/18001/19003#newcode65
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java:65:
if (HtmlAccelServlet.ACCEL_GADGET_PARAM_NAME == name) {
remind me why we can't use .equals() here?

[Jasvir, 2010-07-20 23:18:53]

http://codereview.appspot.com/1807042/diff/18001/19001
File
java/common/src/main/java/org/apache/shindig/protocol/DefaultHandlerRegistry.java
(right):

http://codereview.appspot.com/1807042/diff/18001/19001#newcode629
java/common/src/main/java/org/apache/shindig/protocol/DefaultHandlerRegistry.java:629:
this.operationPath.equals(that.operationPath));
On 2010/07/20 21:56:16, Paul Lindner wrote:
> suggest Objects.equal(this.operationPath, that.operationPath) to handle nulls

Done.

http://codereview.appspot.com/1807042/diff/18001/19003
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java
(right):

http://codereview.appspot.com/1807042/diff/18001/19003#newcode65
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java:65:
if (HtmlAccelServlet.ACCEL_GADGET_PARAM_NAME == name) {
On 2010/07/20 21:56:16, Paul Lindner wrote:
> remind me why we can't use .equals() here?

From johnfargo 2010/07/12 20:36:34 
this isn't a misuse -- the ==-check ensures that a requestor cannot fake the
accel_gadget_param_name by passing the param on the query string.

...hence the "Ugh" inducing change to HtmlAccelServlet.java

[Paul Lindner, 2010-07-21 00:24:23]

On 2010/07/20 23:18:53, jasvir wrote:
> http://codereview.appspot.com/1807042/diff/18001/19003#newcode65
>
java/gadgets/src/test/java/org/apache/shindig/gadgets/render/HtmlRendererTest.java:65:
> if (HtmlAccelServlet.ACCEL_GADGET_PARAM_NAME == name) {
> On 2010/07/20 21:56:16, Paul Lindner wrote:
> > remind me why we can't use .equals() here?
> 
> From johnfargo 2010/07/12 20:36:34 
> this isn't a misuse -- the ==-check ensures that a requestor cannot fake the
> accel_gadget_param_name by passing the param on the query string.
> 
> ...hence the "Ugh" inducing change to HtmlAccelServlet.java

Aha, okay.  Maybe a comment in both places mentioning this.

[Jasvir, 2010-07-21 02:03:40]

Better yet - syncing to head removed all that guff anyways.  I'll commit unless
someone objects.

[Paul Lindner, 2010-07-21 02:18:47]

go for it!
On Jul 20, 2010, at 7:03 PM, jasvir@gmail.com wrote:

> Better yet - syncing to head removed all that guff anyways.  I'll commit
> unless someone objects.
> 
> http://codereview.appspot.com/1807042/show

[fargo, 2010-07-21 03:56:21]

LGTM (too)

On Tue, Jul 20, 2010 at 7:18 PM, Paul Lindner <lindner@inuus.com> wrote:

> go for it!
> On Jul 20, 2010, at 7:03 PM, jasvir@gmail.com wrote:
>
> > Better yet - syncing to head removed all that guff anyways.  I'll commit
> > unless someone objects.
> >
> > http://codereview.appspot.com/1807042/show
>
>