Author: zhoresh
Date: Wed Aug 25 17:44:17 2010
New Revision: 989253
URL: http://svn.apache.org/viewvc?rev=989253&view=rev
Log:
url: http://codereview.appspot.com/1997041/
Fixing ConcatVisitor for cases where media type is "all", no media type and
having title attribute
Patch provided by @Satya
Modified:
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java
Modified:
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
URL:
http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java?rev=989253&r1=989252&r2=989253&view=diff
==============================================================================
---
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
(original)
+++
shindig/trunk/java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
Wed Aug 25 17:44:17 2010
@@ -82,35 +82,73 @@ public class ConcatVisitor implements Do
return VisitStatus.BYPASS;
}
-
+
+ /**
+ * For css:
+ * Link tags are first split into buckets separated by tags with mediaType
== "all"
+ * / title attribute different from their previous link tag / nodes that are
+ * not 'link' tags.
+ * This ensures that the buckets can be processed separately without losing
title /
+ * "all" mediaType information.
+ *
+ * Link tags with same mediaType are concatenated within each bucket.
+ * This exercise ensures that css information is loaded in the same relative
order
+ * as that of the original html page, and that the css information within
+ * mediaType=="all" is retained and applies to all media types.
+ *
+ * Look at the areLinkNodesBucketable method for details on mediaType=="all"
and
+ * title attribute
+ *
+ * Example: Assume we have the following node list. (all have same parent,
+ * nodes between Node6 and Node12 are non link nodes, and hence did not come
+ * to revisit() call)
+ * <link href="1.css" rel="stylesheet" type="text/css" media="screen">
-- Node1
+ * <link href="2.css" rel="stylesheet" type="text/css" media="print">
-- Node2
+ * <link href="3.css" rel="stylesheet" type="text/css" media="screen">
-- Node3
+ * <link href="4.css" rel="stylesheet" type="text/css" media="all">
-- Node4
+ * <link href="5.css" rel="stylesheet" type="text/css" media="all">
-- Node5
+ * <link href="6.css" rel="stylesheet" type="text/css" media="screen">
-- Node6
+ * <link href="12.css" rel="stylesheet" type="text/css" media="screen">
-- Node12
+ * <link href="13.css" rel="stylesheet" type="text/css" media="screen">
-- Node13
+ *
+ * First we split to buckets bassed on the adjacency and other conditions.
+ * buckets - [ [ Node1, Node2, Node3 ], [ Node4, Node 5 ], [ Node6 ], [
Node12, Node13 ]
+ * Within each bucket we group them based on media type.
+ * batches - [ Node1, Node2, Node3 ] --> [ [Node1, Node3], [Node2] ]
+ * - [ Node4, Node 5 ] --> [ [ Node4, Node 5 ] ]
+ * - [ Node6 ] --> [ [ Node6 ] ]
+ * - [ Node12, Node13 ] --> [ [ Node12, Node13 ] ]
+ *
+ * Refer Tests for more examples.
+ */
public boolean revisit(Gadget gadget, List<Node> nodes) throws
RewritingException {
- // Collate Elements into batches to be concatenated.
- List<List<Element>> concatBatches = Lists.newLinkedList();
- List<Element> curBatch = Lists.newLinkedList();
+ // Collate Elements into Buckets.
+ List<List<Element>> concatBuckets = Lists.newLinkedList();
+ List<Element> curBucket = Lists.newLinkedList();
Iterator<Node> nodeIter = nodes.iterator();
Element cur = (Element)nodeIter.next();
- curBatch.add(cur);
+ curBucket.add(cur);
while (nodeIter.hasNext()) {
Element next = (Element)nodeIter.next();
- if (!split && cur != getSibling(next, true)) {
- // Break off current batch and add to list of all.
- concatBatches.add(curBatch);
- curBatch = Lists.newLinkedList();
+ if ((!split && cur != getSibling(next, true)) ||
+ (type == ConcatUriManager.Type.CSS && !areLinkNodesBucketable(cur,
next))) {
+ // Break off current bucket and add to list of all.
+ concatBuckets.add(curBucket);
+ curBucket = Lists.newLinkedList();
}
- curBatch.add(next);
+ curBucket.add(next);
cur = next;
}
// Add leftovers.
- concatBatches.add(curBatch);
+ concatBuckets.add(curBucket);
- // Split the existing batches based on media types.
- List<List<Element>> mediaSpecificConcatBatches = Lists.newLinkedList();
- Iterator<List<Element>> batchesIter = concatBatches.iterator();
+ // Split the existing buckets based on media types into concat batches.
+ List<List<Element>> concatBatches = Lists.newLinkedList();
+ Iterator<List<Element>> batchesIter = concatBuckets.iterator();
while (batchesIter.hasNext()) {
- splitBatchOnMedia(batchesIter.next(), mediaSpecificConcatBatches);
+ splitBatchOnMedia(batchesIter.next(), concatBatches);
}
- concatBatches = mediaSpecificConcatBatches;
// Prepare batches of Uris to send to generate concat Uris
List<List<Uri>> uriBatches = Lists.newLinkedList();
@@ -178,8 +216,8 @@ public class ConcatVisitor implements Do
// Multimap to hold the ordered list of elements encountered for a given
media type.
Multimap<String, Element> mediaBatchMap = LinkedHashMultimap.create();
for (Element element : elements) {
- Element next = element;
- mediaBatchMap.put(next.getAttribute("media"), next);
+ String mediaType = element.getAttribute("media");
+ mediaBatchMap.put(mediaType.isEmpty() ? "screen" : mediaType, element);
}
Set<String> mediaTypes = mediaBatchMap.keySet();
for (String mediaType : mediaTypes) {
@@ -195,9 +233,9 @@ public class ConcatVisitor implements Do
return false;
}
if (type == ConcatUriManager.Type.CSS) {
- // rel="stylesheet" and type="css" also required.
- return ("stylesheet".equalsIgnoreCase(elem.getAttribute("rel")) ||
- elem.getAttribute("type").contains("css"));
+ // rel="stylesheet" and type="text/css" also required.
+ return ("stylesheet".equalsIgnoreCase(elem.getAttribute("rel")) &&
+ "text/css".equalsIgnoreCase(elem.getAttribute("type")));
}
return true;
}
@@ -233,4 +271,31 @@ public class ConcatVisitor implements Do
return true;
}
+ /**
+ * Checks if the css link tags can be put into the same bucket.
+ */
+ private boolean areLinkNodesBucketable(Element current, Element next) {
+ boolean areLinkNodesCompatible = false;
+ // All link tags with media='all' should be placed in their own buckets.
+ // Except for adjacent css links with media='all', which can belong to the
+ // same bucket.
+ String currMediaType = current.getAttribute("media");
+ String nextMediaType = next.getAttribute("media");
+ if (("all".equalsIgnoreCase(currMediaType) &&
"all".equalsIgnoreCase(nextMediaType)) ||
+ (!"all".equalsIgnoreCase(currMediaType) &&
!"all".equalsIgnoreCase(nextMediaType))) {
+ areLinkNodesCompatible = true;
+ }
+
+ // we can't keep the link tags with different 'title' attribute in same
+ // bucket.
+ // An example that proves the above comment.
+ // <link rel="stylesheet" type="text/css" href="a.css" />
+ // <link rel="stylesheet" type="text/css" href="b.css" title="small font"/>
+ // <link rel="stylesheet" type="text/css" href="c.css" />
+ // <link rel="alternate stylesheet" type="text/css" href="d.css"
title="large font"/>
+ // Since browser allows to switch between the perferred styles 'small
font' and 'large font',
+ // we should not batch across the links with title attribute, as it will
lead to reordering of
+ // styles.
+ return areLinkNodesCompatible &&
current.getAttribute("title").equals(next.getAttribute("title"));
+ }
}
Modified:
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java
URL:
http://svn.apache.org/viewvc/shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java?rev=989253&r1=989252&r2=989253&view=diff
==============================================================================
---
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java
(original)
+++
shindig/trunk/java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java
Wed Aug 25 17:44:17 2010
@@ -41,7 +41,7 @@ import java.util.List;
import java.util.Map;
public class ConcatVisitorTest extends DomWalkerTestBase {
- private static final String JS1_URL_STR = "http://one.com/foo.js";;
+ private static final String JS1_URL_STR =
"http://one.com/foo.js?test=1&ui=2";;
private Node js1;
private static final String JS2_URL_STR = "http://two.com/foo.js";;
@@ -85,7 +85,16 @@ public class ConcatVisitorTest extends D
private static final String CSS9_URL_STR = "http://nine.com/foo.js";;
private Node css9;
-
+
+ private static final String CSS10_URL_STR = "http://ten.com/foo.js";;
+ private Node css10;
+
+ private static final String CSS11_URL_STR = "http://eleven.com/foo.js";;
+ private Node css11;
+
+ private static final String CSS12_URL_STR = "http://twelve.com/foo.js";;
+ private Node css12;
+
private static final Uri CONCAT_BASE_URI =
Uri.parse("http://test.com/proxy";);
@Before
@@ -97,7 +106,7 @@ public class ConcatVisitorTest extends D
js4 = elem("script", "src", JS4_URL_STR);
js5 = elem("script", "src", JS5_URL_STR);
js6 = elem("script", "src", JS6_URL_STR);
- css1 = elem("link", "rel", "stylesheet", "type", "text/css", "href",
CSS1_URL_STR);
+ css1 = elem("link", "rel", "Stylesheet", "type", "Text/css", "href",
CSS1_URL_STR);
css2 = elem("link", "rel", "stylesheet", "type", "text/css", "href",
CSS2_URL_STR);
css3 = elem("link", "rel", "stylesheet", "type", "text/css", "href",
CSS3_URL_STR);
css4 = elem("link", "rel", "stylesheet", "type", "text/css", "href",
CSS4_URL_STR);
@@ -107,7 +116,9 @@ public class ConcatVisitorTest extends D
css7 = elem("link", "rel", "stylesheet", "type", "text/css", "media",
"screen", "href", CSS7_URL_STR);
css8 = elem("link", "rel", "stylesheet", "type", "text/css", "media",
"screen", "href", CSS8_URL_STR);
css9 = elem("link", "rel", "stylesheet", "type", "text/css", "href",
CSS9_URL_STR);
-
+ css10 = elem("link", "rel", "stylesheet", "type", "text/css", "media",
"all", "href", CSS10_URL_STR);
+ css11 = elem("link", "rel", "stylesheet", "type", "text/css", "media",
"all", "href", CSS11_URL_STR);
+ css12 = elem("link", "rel", "stylesheet", "type", "text/css", "media",
"all", "href", CSS12_URL_STR);
}
@Test
@@ -161,19 +172,49 @@ public class ConcatVisitorTest extends D
assertEquals(VisitStatus.BYPASS, getVisitStatusJs(config, sep2));
assertEquals(VisitStatus.BYPASS, getVisitStatusJs(config, js3));
}
-
+
+ @Test
+ public void visitValidCss() throws Exception {
+ Node textNode = doc.createTextNode("");
+ Node node = elem("link", "type", "text/css", "rel", "stylesheet", "href",
CSS1_URL_STR);
+ seqNodes(node, textNode, css1);
+ assertEquals(VisitStatus.RESERVE_NODE, getVisitStatusCss(node, null));
+ }
+
+ @Test
+ public void dontVisitCssSeperatedByNonEmptyTextNode() throws Exception {
+ Node textNode = doc.createTextNode("Data\n");
+ Node node = elem("link", "type", "text/css", "rel", "stylesheet", "href",
CSS1_URL_STR);
+ seqNodes(node, textNode, css1);
+ assertEquals(VisitStatus.BYPASS, getVisitStatusCss(node, null));
+ }
+
@Test
- public void visitRelFreeCss() throws Exception {
+ public void dontVisitRelFreeCss() throws Exception {
Node node = elem("link", "type", "text/css", "href", CSS1_URL_STR);
seqNodes(node, css1);
- assertEquals(VisitStatus.RESERVE_NODE, getVisitStatusCss(node, null));
+ assertEquals(VisitStatus.BYPASS, getVisitStatusCss(node, null));
}
@Test
- public void visitTypeCssFreeCss() throws Exception {
+ public void dontVisitTypeCssFreeCss() throws Exception {
Node node = elem("link", "rel", "stylesheet", "href", CSS1_URL_STR);
seqNodes(node, css1);
- assertEquals(VisitStatus.RESERVE_NODE, getVisitStatusCss(node, null));
+ assertEquals(VisitStatus.BYPASS, getVisitStatusCss(node, null));
+ }
+
+ @Test
+ public void dontVisitTypeCssWrongRelAttributes() throws Exception {
+ Node node = elem("link", "rel", "alternate", "type", "text/css", "href",
CSS1_URL_STR);
+ seqNodes(node, css1);
+ assertEquals(VisitStatus.BYPASS, getVisitStatusCss(node, null));
+ }
+
+ @Test
+ public void dontVisitTypeCssWrongTypeAttributes() throws Exception {
+ Node node = elem("link", "rel", "stylesheet", "type", "text/javascript",
"href", CSS1_URL_STR);
+ seqNodes(node, css1);
+ assertEquals(VisitStatus.BYPASS, getVisitStatusCss(node, null));
}
@Test
@@ -331,7 +372,7 @@ public class ConcatVisitorTest extends D
assertEquals(CSS2_URL_STR, concatUri1.getQueryParameter("2"));
assertNull(concatUri1.getQueryParameter("3"));
- assertEquals(3, parent2.getChildNodes().getLength());
+ assertEquals(2, parent2.getChildNodes().getLength());
Element cn2 = (Element)parent2.getChildNodes().item(0);
Uri concatUri2 = Uri.parse(cn2.getAttribute("href").replace("&", "&"));
assertEquals(CONCAT_BASE_URI.getScheme(), concatUri2.getScheme());
@@ -339,8 +380,10 @@ public class ConcatVisitorTest extends D
assertEquals(CONCAT_BASE_URI.getPath(), concatUri2.getPath());
assertEquals(CSS3_URL_STR, concatUri2.getQueryParameter("1"));
assertEquals(CSS4_URL_STR, concatUri2.getQueryParameter("2"));
- assertEquals(CSS9_URL_STR, concatUri2.getQueryParameter("3"));
- assertNull(concatUri2.getQueryParameter("4"));
+ assertEquals(CSS7_URL_STR, concatUri2.getQueryParameter("3"));
+ assertEquals(CSS8_URL_STR, concatUri2.getQueryParameter("4"));
+ assertEquals(CSS9_URL_STR, concatUri2.getQueryParameter("5"));
+ assertNull(concatUri2.getQueryParameter("6"));
assertEquals("", cn2.getAttribute("media"));
Element cn3 = (Element)parent2.getChildNodes().item(1);
@@ -352,18 +395,90 @@ public class ConcatVisitorTest extends D
assertEquals(CSS6_URL_STR, concatUri3.getQueryParameter("2"));
assertNull(concatUri3.getQueryParameter("3"));
assertEquals("print", cn3.getAttribute("media"));
-
- Element cn4 = (Element)parent2.getChildNodes().item(2);
+ }
+
+ @Test
+ public void concatMultiBatchCssWithAllMediaTypeAndTitle() throws Exception {
+ List<Node> fullListCss = Lists.newArrayList();
+ // modify few node to have the title attriblue.
+ ((Element) css2).setAttribute("title", "one");
+ ((Element) css3).setAttribute("title", "two");
+ ((Element) css4).setAttribute("title", "two");
+ ((Element) css10).setAttribute("title", "two");
+ fullListCss.addAll(seqNodes(css1, css2, css3, css4, css10, css11, css12,
css7, css8, css9));
+ Node parent1 = css1.getParentNode();
+ assertEquals(10, parent1.getChildNodes().getLength());
+
+ SimpleConcatUriManager mgr = simpleMgr();
+ ConcatVisitor.Css rewriter = new ConcatVisitor.Css(config(null, false),
mgr);
+ assertTrue(rewriter.revisit(gadget(), fullListCss));
+
+ // Should have been split across 'all' media type and then batches should
be independently
+ // concatenated.
+ Element cn1 = (Element)parent1.getChildNodes().item(0);
+ Uri concatUri1 = Uri.parse(cn1.getAttribute("href").replace("&", "&"));
+ assertEquals(CONCAT_BASE_URI.getScheme(), concatUri1.getScheme());
+ assertEquals(CONCAT_BASE_URI.getAuthority(), concatUri1.getAuthority());
+ assertEquals(CONCAT_BASE_URI.getPath(), concatUri1.getPath());
+ assertEquals(CSS1_URL_STR, concatUri1.getQueryParameter("1"));
+ assertNull(concatUri1.getQueryParameter("2"));
+ assertEquals("", cn1.getAttribute("media"));
+
+ Element cn2 = (Element)parent1.getChildNodes().item(1);
+ Uri concatUri2 = Uri.parse(cn2.getAttribute("href").replace("&", "&"));
+ assertEquals(CONCAT_BASE_URI.getScheme(), concatUri2.getScheme());
+ assertEquals(CONCAT_BASE_URI.getAuthority(), concatUri2.getAuthority());
+ assertEquals(CONCAT_BASE_URI.getPath(), concatUri2.getPath());
+ assertEquals(CSS2_URL_STR, concatUri2.getQueryParameter("1"));
+ assertNull(concatUri2.getQueryParameter("2"));
+ assertEquals("", cn2.getAttribute("media"));
+ assertEquals("one", cn2.getAttribute("title"));
+
+ Element cn3 = (Element)parent1.getChildNodes().item(2);
+ Uri concatUri3 = Uri.parse(cn3.getAttribute("href").replace("&", "&"));
+ assertEquals(CONCAT_BASE_URI.getScheme(), concatUri3.getScheme());
+ assertEquals(CONCAT_BASE_URI.getAuthority(), concatUri3.getAuthority());
+ assertEquals(CONCAT_BASE_URI.getPath(), concatUri3.getPath());
+ assertEquals(CSS3_URL_STR, concatUri3.getQueryParameter("1"));
+ assertEquals(CSS4_URL_STR, concatUri3.getQueryParameter("2"));
+ assertNull(concatUri3.getQueryParameter("3"));
+ assertEquals("", cn3.getAttribute("media"));
+ assertEquals("two", cn3.getAttribute("title"));
+
+ Element cn4 = (Element)parent1.getChildNodes().item(3);
Uri concatUri4 = Uri.parse(cn4.getAttribute("href").replace("&", "&"));
assertEquals(CONCAT_BASE_URI.getScheme(), concatUri4.getScheme());
assertEquals(CONCAT_BASE_URI.getAuthority(), concatUri4.getAuthority());
assertEquals(CONCAT_BASE_URI.getPath(), concatUri4.getPath());
- assertEquals(CSS7_URL_STR, concatUri4.getQueryParameter("1"));
- assertEquals(CSS8_URL_STR, concatUri4.getQueryParameter("2"));
- assertNull(concatUri4.getQueryParameter("3"));
- assertEquals("screen", cn4.getAttribute("media"));
+ assertEquals(CSS10_URL_STR, concatUri4.getQueryParameter("1"));
+ assertNull(concatUri4.getQueryParameter("2"));
+ assertEquals("all", cn4.getAttribute("media"));
+ assertEquals("two", cn4.getAttribute("title"));
+
+ Element cn5 = (Element)parent1.getChildNodes().item(4);
+ Uri concatUri5 = Uri.parse(cn5.getAttribute("href").replace("&", "&"));
+ assertEquals(CONCAT_BASE_URI.getScheme(), concatUri5.getScheme());
+ assertEquals(CONCAT_BASE_URI.getAuthority(), concatUri5.getAuthority());
+ assertEquals(CONCAT_BASE_URI.getPath(), concatUri5.getPath());
+ assertEquals(CSS11_URL_STR, concatUri5.getQueryParameter("1"));
+ assertEquals(CSS12_URL_STR, concatUri5.getQueryParameter("2"));
+ assertNull(concatUri5.getQueryParameter("3"));
+ assertEquals("all", cn5.getAttribute("media"));
+ assertEquals("", cn5.getAttribute("title"));
+
+ Element cn6 = (Element)parent1.getChildNodes().item(5);
+ Uri concatUri6 = Uri.parse(cn6.getAttribute("href").replace("&", "&"));
+ assertEquals(CONCAT_BASE_URI.getScheme(), concatUri6.getScheme());
+ assertEquals(CONCAT_BASE_URI.getAuthority(), concatUri6.getAuthority());
+ assertEquals(CONCAT_BASE_URI.getPath(), concatUri6.getPath());
+ assertEquals(CSS7_URL_STR, concatUri6.getQueryParameter("1"));
+ assertEquals(CSS8_URL_STR, concatUri6.getQueryParameter("2"));
+ assertEquals(CSS9_URL_STR, concatUri6.getQueryParameter("3"));
+ assertNull(concatUri6.getQueryParameter("4"));
+ assertEquals("screen", cn6.getAttribute("media"));
+ assertEquals("", cn6.getAttribute("title"));
}
-
+
@Test
public void concatMultiBatchJsBadBatch() throws Exception {
List<Node> fullListJs = Lists.newArrayList();
