[ 
https://issues.apache.org/jira/browse/SHINDIG-88?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12571411#action_12571411
 ] 

Kevin Brown commented on SHINDIG-88:
------------------------------------

Index: 
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/CrossServletState.java
===================================================================
--- 
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/CrossServletState.java
   (revision 629861)
+++ 
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/CrossServletState.java
   (working copy)
@@ -101,7 +101,7 @@
    * context object). A better choice would probably be to add the view params
    * to ProcessingOptions and pass that here.
    */
-  public abstract String getIframeUrl(Gadget gadget,  ProcessingOptions opts);
+  public abstract String getIframeUrl(Gadget gadget, String viewName, 
ProcessingOptions opts);

I don't think this signature should be changed -- instead, the view should be 
obtained from ProcessingOptions (as would anything that comes from a url 
parameter).  It might actually make more sense to remove this method altogether 
since only RpcServlet uses it.
 

Index: 
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/GadgetRenderingServlet.java
===================================================================
--- 
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/GadgetRenderingServlet.java
      (revision 629861)
+++ 
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/GadgetRenderingServlet.java
      (working copy)
-    markup.append("</head><body>");
+    markup.append(
+        "<html>\n" +
+        "  <head>\n" +
+        "    <style type=\"text/css\">\n" +
+        "      body,td,div,span,p{font-family:arial,sans-serif;}\n" +
+        "      a {color:#0000cc;}a:visited {color:#551a8b;}\n" +
+        "      a:active {color:#ff0000;}\n" +
+        "      body{margin: 0px;padding: 0px;background-color:white;}\n" +
+        "    </style>\n" +
+        "  </head>\n" +
+        "  <body>\n");

Leave off the newlines here. They don't make any difference in the java code, 
and the html output is unreadable anyway.  Eventually all markup will be 
minified anyway, so this situation will only get worse.

     StringBuilder externJs = new StringBuilder();
     StringBuilder inlineJs = new StringBuilder();
-    String externFmt = "<script src=\"%s\"></script>\n";
+    String externFmt =
+        "    <script src=\"%s\"></script>\n";

Leave this out as well. It actually makes the java harder to read.

 
     markup.append(content);
-    markup.append("<script>gadgets.util.runOnLoadHandlers();</script>");
-    markup.append("</body></html>");
+    markup.append(
+        "    <script>gadgets.util.runOnLoadHandlers();</script>\n");
+    markup.append(
+        "  </body>\n" +
+        "</html>");

And these.
 
Index: java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java
===================================================================
--- java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java   
(revision 629861)
+++ java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java   
(working copy)
@@ -17,6 +17,8 @@
-  public ContentType getContentType() {
-    return baseSpec.getContentType();
+  public String debugString() {
+    return baseSpec.debugString();

toString is supposed to serve this purpose.

Index: 
java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetSpecParser.java
===================================================================
--- java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetSpecParser.java 
(revision 629861)
+++ java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetSpecParser.java 
(working copy)
+    for (int i = 0; i < attrs.getLength(); i++) {
+      Node attr = attrs.item(i);
+      if (!MODULEPREFS_KNOWN_ATTRS_SET.contains(attr.getNodeName())) {
+        // TODO: in debug mode, this should warn developer about unknown 
attributes
+      }
+    }

This belongs in a static block, but I don't really think it's the right way to 
deal with warning developers about unknown attributes -- we should pass the 
gadget through the XSD validator and look for errors produced in there instead. 
This way we can maintain a single XSD for everything that shindig supports 
(most likely a superset of the extended spec). 
 
+    NamedNodeMap attrsNodeMap = content.getAttributes();
+    String contentData = content.getTextContent();
+    Map<String, String> attributes = new HashMap<String, String>();
+    for (int idx = 0; idx < attrsNodeMap.getLength(); idx++) {
+      Node attr = attrsNodeMap.item(idx);
+      String attrName = attr.getNodeName();
+      if (attrName == "view")
+        continue;
+      attributes.put(attrName, attr.getNodeValue());
     }

Copy attributes here is wasteful -- NamedNodeMap is already a map (though it 
doesn't implement the Java map interface). If you want to pass a map of 
attributes to addContent, just pass the NamedNodeMap here.

+          if (href == null)

Brackets

+      private static final String quirksAttrName = "quirks";
+      private static final String typeAttrName = "type";
+      private static final String hrefAttrName = "href";

constants should be ALL_CAPS.

+        if (attrs == null)
+          attrs = attributes;
+        if (attrs.containsKey(name) == false)
+          return defaultValue;
+        String value = attrs.get(name);
+        if (value.equals("true"))
+          return true;
+        if (value.equals("false"))
+          return false;

brackets

Index: 
java/gadgets/src/main/java/org/apache/shindig/gadgets/SpecParserException.java
===================================================================
--- 
java/gadgets/src/main/java/org/apache/shindig/gadgets/SpecParserException.java  
    (revision 629861)
+++ 
java/gadgets/src/main/java/org/apache/shindig/gadgets/SpecParserException.java  
    (working copy)
@@ -28,7 +28,7 @@
     super(GadgetException.Code.MALFORMED_XML_DOCUMENT, message);
   }
 
-  public SpecParserException(GadgetException.Code code) {
-    super(code);
+  public SpecParserException(GadgetException.Code code, String message) {
+    super(code, message);

Make this a new method and leave the existing ctor as is. 

> "quirks" attribute, multiple view refactoring
> ---------------------------------------------
>
>                 Key: SHINDIG-88
>                 URL: https://issues.apache.org/jira/browse/SHINDIG-88
>             Project: Shindig
>          Issue Type: Improvement
>          Components: Gadgets Server - Java
>            Reporter: Bruno Bowden
>            Assignee: Kevin Brown
>         Attachments: quirks-begone.patch
>
>
> - <Content quirks="false"> allows developer to request standards mode
>    <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" 
> "http://www.w3.org/TR/html4/strict.dtd";>  
> - Moves view information in to separate class
> - Adds extensive test cases for multiple views, content / moduleprefs 
> attributes and parse errors

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.

Reply via email to