Implement JS data fetch through GadgetHandler

[zhoresh, 2010-11-10 17:41:15]

Add one more test

[johnfargo, 2010-11-11 01:14:32]

http://codereview.appspot.com/2988042/diff/7001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java
(right):

http://codereview.appspot.com/2988042/diff/7001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java:218:
public boolean getIgnoreCahce();
s/getIgnoreCahce/getIgnoreCache/

http://codereview.appspot.com/2988042/diff/7001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java
(right):

http://codereview.appspot.com/2988042/diff/7001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:206:
}
very-common-pattern alert -- might want to extract a helper method.

http://codereview.appspot.com/2988042/diff/7001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:213:
(request.getContext() != null ? (RenderingContext)
beanDelegator.convertEnum(request.getContext())
over 100char

http://codereview.appspot.com/2988042/diff/7001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:229:
+ " was not found", HttpResponse.SC_NOT_FOUND);
micronit: can remove the word "was"

http://codereview.appspot.com/2988042/diff/7001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:232:
expireMs = timeSource.currentTimeMillis() + HttpUtil.getDefaultTtl() * 1000;
readability micronit: parentheses around the multiplication

http://codereview.appspot.com/2988042/diff/7001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:421:
.put("expiretimems", BeanDelegator.nullable(expireMs)).build()),
do we want this to be nullable, or just default to 0 (in the non-proxy-cacheable
case)?

http://codereview.appspot.com/2988042/diff/7001/java/gadgets/src/test/java/or...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java
(right):

http://codereview.appspot.com/2988042/diff/7001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java:299:
GadgetsHandlerApi.JsRequest request = createJsRequest(null, null, fields,
ImmutableList.of("rpc"));
over 100 char

http://codereview.appspot.com/2988042/diff/7001/java/gadgets/src/test/java/or...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerTest.java
(right):

http://codereview.appspot.com/2988042/diff/7001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerTest.java:387:
private JSONObject makeSimpleJsRequest(String fields, List<String> features)
throws JSONException {
over 100 char

[zhoresh, 2010-11-11 19:03:53]

http://codereview.appspot.com/2988042/diff/7001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java
(right):

http://codereview.appspot.com/2988042/diff/7001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerApi.java:218:
public boolean getIgnoreCahce();
On 2010/11/11 01:14:32, johnfargo wrote:
> s/getIgnoreCahce/getIgnoreCache/

Disappointing. Fixed.
When you use the IDE to generate calls and override method you tend to not see
this errors.

http://codereview.appspot.com/2988042/diff/7001/java/gadgets/src/main/java/or...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java
(right):

http://codereview.appspot.com/2988042/diff/7001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:206:
}
On 2010/11/11 01:14:32, johnfargo wrote:
> very-common-pattern alert -- might want to extract a helper method.

Good idea, done.

http://codereview.appspot.com/2988042/diff/7001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:213:
(request.getContext() != null ? (RenderingContext)
beanDelegator.convertEnum(request.getContext())
On 2010/11/11 01:14:32, johnfargo wrote:
> over 100char

Done.

http://codereview.appspot.com/2988042/diff/7001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:229:
+ " was not found", HttpResponse.SC_NOT_FOUND);
On 2010/11/11 01:14:32, johnfargo wrote:
> micronit: can remove the word "was"

Done.

http://codereview.appspot.com/2988042/diff/7001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:232:
expireMs = timeSource.currentTimeMillis() + HttpUtil.getDefaultTtl() * 1000;
On 2010/11/11 01:14:32, johnfargo wrote:
> readability micronit: parentheses around the multiplication

Done.

http://codereview.appspot.com/2988042/diff/7001/java/gadgets/src/main/java/or...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:421:
.put("expiretimems", BeanDelegator.nullable(expireMs)).build()),
On 2010/11/11 01:14:32, johnfargo wrote:
> do we want this to be nullable, or just default to 0 (in the
non-proxy-cacheable
> case)?

Just being cheep on bytes on the line (field name), so using null.

http://codereview.appspot.com/2988042/diff/7001/java/gadgets/src/test/java/or...
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java
(right):

http://codereview.appspot.com/2988042/diff/7001/java/gadgets/src/test/java/or...
java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerServiceTest.java:299:
GadgetsHandlerApi.JsRequest request = createJsRequest(null, null, fields,
ImmutableList.of("rpc"));
On 2010/11/11 01:14:32, johnfargo wrote:
> over 100 char

Done.

[zhoresh, 2010-11-11 19:04:28]

Update according to Fargo comments

[johnfargo, 2010-11-12 01:30:58]

LGTM

On Thu, Nov 11, 2010 at 11:04 AM, <zhoresh@gmail.com> wrote:

> Update according to Fargo comments
>
>
> http://codereview.appspot.com/2988042/
>

[zhoresh, 2010-11-12 02:06:34]

On 2010/11/12 01:30:58, johnfargo wrote:
> LGTM
> 

Thanks! committed at r1034245