Re: JSON-RPC Gadgets Handler (issue1722041)

[jo...@gmail.com]

Hey Paul:

Just committed this CL on your behalf, since A) it's new and thus won't
break anyone, B) we'd like to iterate on it re: comments from Ziv and
Michael, as well as other shared ideas; C) we're testing it out in our
installation and wanted to use the Shindig version ASAP.

Cheers,
John

On 2010/07/07 22:02:18, mhermanto wrote:
> Sorry for the tardy response, but this looks good. Agree with Ziv's
comments. We
> have an endpoint which clients talk to via protocol buffer. That data
model is
> unfortunately not quit the same as the one expressed here
(MetadataGadgetSpec,
> FilteringGadgetSpec), but since we build on top of Shindig, we should
continue
> use this data model. Let's have this submitted, and we'll make the
> appropriate/further changes as we go through our exercise in
converting these
> model objects to ours. Thanks Paul for doing this.

> http://codereview.appspot.com/1722041/diff/6001/7004
> File

java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java
> (right):

> http://codereview.appspot.com/1722041/diff/6001/7004#newcode73

java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:73:
> @Operation(httpMethods = {"POST","GET"}, path = "/metadata/{view}")
> FMI mostly, CC JS will call an endpoint using OSAPI lib, via a POST
/api/rpc,
> which is fixed. Can this path /metadata/{view} (which is seems to vary
with
> view) be expressed by that OSAPI call?

> http://codereview.appspot.com/1722041/diff/6001/7004#newcode75

java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:75:
> Set<String> gadgetUrls =
ImmutableSet.copyOf(request.getListParameter("ids"));
> Is the order of the response preserved, since we're turning a list
into a set?

> http://codereview.appspot.com/1722041/diff/6001/7004#newcode96

java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:96:
> throw new
ProtocolException(HttpServletResponse.SC_INTERNAL_SERVER_ERROR,
> "Processing interrupted", e);
> Instead of throwing one error due to error from subset of gadget
requests, let's
> throw per-gadget error. This will allow common container code to only
render
> gadget that work successfully (should be better than failing
altogether). It
> seems like you're already doing this for ExecutionException below.

> http://codereview.appspot.com/1722041/diff/6001/7004#newcode116

java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:116:
> return ALL_METADATA_FIELDS;
> What's the use case for this?

> http://codereview.appspot.com/1722041/diff/6001/7004#newcode168

java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:168:
> this.ignoreCache =
Boolean.valueOf(request.getParameter("ignoreCache"));
> Rename to nocache to keep it consistent with the iframe query param.

> http://codereview.appspot.com/1722041/diff/6001/7004#newcode169

java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:169:
> this.debug = Boolean.valueOf(request.getParameter("debug"));
> This should be fine, but CC will not need to call this endpoint upon
variables
> that don't require server-generated URI, and save HTTP request.

> http://codereview.appspot.com/1722041/diff/6001/7004#newcode215

java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:215:
> // TODO
> TODO: extract from request?

> http://codereview.appspot.com/1722041/diff/6001/7009
> File

java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/FakeIframeUriManager.java
> (right):

> http://codereview.appspot.com/1722041/diff/6001/7009#newcode27

java/gadgets/src/test/java/org/apache/shindig/gadgets/servlet/FakeIframeUriManager.java:27:
> public class FakeIframeUriManager implements IframeUriManager {
> Is this used by anything? If only by one class, just have it as an
inner class.



http://codereview.appspot.com/1722041/show