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
