GadgetHandlerApi cleanups

- Resolve naming confusion by renaming token request input params to AuthContext.
- Add Rendering type
- With the new caja function, proxy should not support cajoling.
- clean minor compiler warnings

[Jasvir, 2010-12-10 05:25:01]

In the interest of clean up
org.apache.shindig.gadgets.render.CajaResponseRewriter and associated tests
should probably also be deleted.

http://codereview.appspot.com/3574041/diff/1/java/gadgets/src/main/java/org/a...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java
(right):

http://codereview.appspot.com/3574041/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:423:
if ("1".equals(param)) {
Would this be better handled by making the query parameter values human reable -
param=container|gadget|configured etc vs 0|1|2.

[zhoresh, 2010-12-10 17:10:46]

On Thu, Dec 9, 2010 at 9:25 PM, <jasvir@gmail.com> wrote:

> In the interest of clean up
> org.apache.shindig.gadgets.render.CajaResponseRewriter and associated
> tests should probably also be deleted.
>
Which tests? Actually I am tempted to add a TODO to add tests for
GadgetHandlerService.caja (which will require mock of the rewriter)


>
>
>
>
http://codereview.appspot.com/3574041/diff/1/java/gadgets/src/main/java/org/a...
> File
>
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java
> (right):
>
>
>
http://codereview.appspot.com/3574041/diff/1/java/gadgets/src/main/java/org/a...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:423:
> if ("1".equals(param)) {
> Would this be better handled by making the query parameter values human
> reable - param=container|gadget|configured etc vs 0|1|2.

For sake of consistency with the js servlet I prefer to stay with the number
notation.


>
>
> http://codereview.appspot.com/3574041/
>

[johnfargo, 2010-12-10 19:33:03]

http://codereview.appspot.com/3574041/diff/1/java/gadgets/src/main/java/org/a...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java
(right):

http://codereview.appspot.com/3574041/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:423:
if ("1".equals(param)) {
Seems like a good idea. We could include this logic in the RenderingContext enum
itself, as Michael is mapping in existing values "1", "2" etc already in a
parallel CL.

On 2010/12/10 05:25:01, jasvir wrote:
> Would this be better handled by making the query parameter values human reable
-
> param=container|gadget|configured etc vs 0|1|2.

http://codereview.appspot.com/3574041/diff/1/java/gadgets/src/main/java/org/a...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java
(right):

http://codereview.appspot.com/3574041/diff/1/java/gadgets/src/main/java/org/a...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:309:
}
FMI, where did the IOException even go? Was it never thrown?

[zhoresh, 2010-12-10 20:56:58]

Consalidate request params to an enum, s/ignoreCache/nocache/ and
s/mime-type/mime_type/

[zhoresh, 2010-12-10 21:02:23]

I updated the patch to make the request parameters for the json request more
consistent with the rest of shindig servlets. PTAL


On Fri, Dec 10, 2010 at 11:33 AM, <johnfargo@gmail.com> wrote:

>
>
>
http://codereview.appspot.com/3574041/diff/1/java/gadgets/src/main/java/org/a...
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java
> (right):
>
>
>
http://codereview.appspot.com/3574041/diff/1/java/gadgets/src/main/java/org/a...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:423:
> if ("1".equals(param)) {
> Seems like a good idea. We could include this logic in the
> RenderingContext enum itself, as Michael is mapping in existing values
> "1", "2" etc already in a parallel CL.

It is a different enum (same name and values), so a common code is not
trivial

>
>
> On 2010/12/10 05:25:01, jasvir wrote:
>
>> Would this be better handled by making the query parameter values
>>
> human reable -
>
>> param=container|gadget|configured etc vs 0|1|2.
>>
>
>
>
http://codereview.appspot.com/3574041/diff/1/java/gadgets/src/main/java/org/a...
> File
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java
> (right):
>
>
>
http://codereview.appspot.com/3574041/diff/1/java/gadgets/src/main/java/org/a...
>
>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:309:
> }
> FMI, where did the IOException even go? Was it never thrown?

I don't think it was ever thrown, maybe it was at one point before it got
committed.

>
>
> http://codereview.appspot.com/3574041/
>

[johnfargo, 2010-12-10 21:18:41]

LGTM

Re: the enum, you could always impl something like:

static <T extends Enum<?>> getRenderingContext(String key, Iterable<T>
allValues) {
  if ("1".equals(key) || "container".equals(key)) { return
findContainerFromAllValues(); }
  else if ("2".equals(key) || "compiled_gadget".equals(key)) { return
findCompiledGadgetFromAllValues(); }
  ...
}

:)

On Fri, Dec 10, 2010 at 1:02 PM, Ziv Horesh <zhoresh@gmail.com> wrote:

> I updated the patch to make the request parameters for the json request
> more consistent with the rest of shindig servlets. PTAL
>
>
> On Fri, Dec 10, 2010 at 11:33 AM, <johnfargo@gmail.com> wrote:
>
>>
>>
>>
http://codereview.appspot.com/3574041/diff/1/java/gadgets/src/main/java/org/a...
>> File
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java
>> (right):
>>
>>
>>
http://codereview.appspot.com/3574041/diff/1/java/gadgets/src/main/java/org/a...
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:423:
>> if ("1".equals(param)) {
>> Seems like a good idea. We could include this logic in the
>> RenderingContext enum itself, as Michael is mapping in existing values
>> "1", "2" etc already in a parallel CL.
>
> It is a different enum (same name and values), so a common code is not
> trivial
>
>>
>>
>> On 2010/12/10 05:25:01, jasvir wrote:
>>
>>> Would this be better handled by making the query parameter values
>>>
>> human reable -
>>
>>> param=container|gadget|configured etc vs 0|1|2.
>>>
>>
>>
>>
http://codereview.appspot.com/3574041/diff/1/java/gadgets/src/main/java/org/a...
>> File
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java
>> (right):
>>
>>
>>
http://codereview.appspot.com/3574041/diff/1/java/gadgets/src/main/java/org/a...
>>
>>
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandlerService.java:309:
>> }
>> FMI, where did the IOException even go? Was it never thrown?
>
> I don't think it was ever thrown, maybe it was at one point before it got
> committed.
>
>>
>>
>> http://codereview.appspot.com/3574041/
>>
>
>

[Jasvir, 2010-12-10 21:45:12]

LGTM

http://codereview.appspot.com/3574041/diff/12001/java/gadgets/src/main/java/o...
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java
(right):

http://codereview.appspot.com/3574041/diff/12001/java/gadgets/src/main/java/o...
java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/GadgetsHandler.java:591:
this.mimeType = getParam(request, Param.MIME_TYPE, "text/html");
ok

[zhoresh, 2010-12-11 00:08:27]

Thanks for the review. submitted to r1044534

The change modify the API but since it is brand new service I don't worry about
backward compatibility.
We hope to start to use it in production very soon (Jan), so if anyone have any
other comments or suggestions that will modify the API, now is the time!