Fix for CSS concat issue related to differing media types

Fixing the CSS ConcatVisitor code for handling different media types. This patch should fix the issue reported in: https://issues.apache.org/jira/browse/SHINDIG-1085

[johnfargo, 2010-06-08 10:00:10]

Looking good - I believe this logic works but have one suggestion for
structuring the solution.

http://codereview.appspot.com/1541042/diff/1/3
File src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
(right):

http://codereview.appspot.com/1541042/diff/1/3#newcode93
src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:93: String
nextElemMediaType = next.getAttribute("media");
nit: maybe just call this mediaType

http://codereview.appspot.com/1541042/diff/1/3#newcode110
src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:110: boolean
continueExistingBatch = mediaSpecificBatches.containsKey(nextElemMediaType);
I think I see where you're going w/ the continueExistingBatch logic, but it
seems a little hard to trace for me.

What do you think about moving this equivalent logic to something like a
splitBatchOnMedia(List<Element>, List<List<Element>> output) method?

http://codereview.appspot.com/1541042/diff/1/2
File src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java
(right):

http://codereview.appspot.com/1541042/diff/1/2#newcode105
src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:105:
css5 = elem("link", "rel", "stylesheet", "type", "text/css", "media", "print",
"href", CSS5_URL_STR);
nit: 100 char line limit

http://codereview.appspot.com/1541042/diff/1/2#newcode315
src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:315:
fullListCss.addAll(seqNodes(css3, css4, css5, css6, css7, css8, css9));
to make things more difficult, we should test interleaved media use cases ie.
media=print, media=screen, media=print, media=screen

[anupama.dutta, 2010-06-08 11:07:41]

Addressing Fargo's comments related to restructuring etc.

[anupama.dutta, 2010-06-08 11:13:33]

http://codereview.appspot.com/1541042/diff/1/3
File src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
(right):

http://codereview.appspot.com/1541042/diff/1/3#newcode93
src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:93: String
nextElemMediaType = next.getAttribute("media");
On 2010/06/08 10:00:10, johnfargo wrote:
> nit: maybe just call this mediaType

Done.

http://codereview.appspot.com/1541042/diff/1/3#newcode110
src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:110: boolean
continueExistingBatch = mediaSpecificBatches.containsKey(nextElemMediaType);
On 2010/06/08 10:00:10, johnfargo wrote:
> I think I see where you're going w/ the continueExistingBatch logic, but it
> seems a little hard to trace for me.
> 
> What do you think about moving this equivalent logic to something like a
> splitBatchOnMedia(List<Element>, List<List<Element>> output) method?

Done. Please let me know if the refactoring can be refined in any way.

http://codereview.appspot.com/1541042/diff/1/2
File src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java
(right):

http://codereview.appspot.com/1541042/diff/1/2#newcode105
src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:105:
css5 = elem("link", "rel", "stylesheet", "type", "text/css", "media", "print",
"href", CSS5_URL_STR);
On 2010/06/08 10:00:10, johnfargo wrote:
> nit: 100 char line limit

Done.

http://codereview.appspot.com/1541042/diff/1/2#newcode315
src/test/java/org/apache/shindig/gadgets/rewrite/ConcatVisitorTest.java:315:
fullListCss.addAll(seqNodes(css3, css4, css5, css6, css7, css8, css9));
On 2010/06/08 10:00:10, johnfargo wrote:
> to make things more difficult, we should test interleaved media use cases ie.
> media=print, media=screen, media=print, media=screen

Done.

[Paul Lindner, 2010-06-09 05:23:18]

seems fine.   Might be a candidate for LinkedHashMultimap though...

[henry.saputra, 2010-06-10 05:23:50]

My 2 cents.

Thanks,

- Henry

http://codereview.appspot.com/1541042/diff/12001/9003
File src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java
(right):

http://codereview.appspot.com/1541042/diff/12001/9003#newcode175
src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:175:
HashMap<String, List<Element>> mediaBatchMap = new HashMap<String,
List<Element>>();
Can use Maps.newHashMap() and Map<String, List<Element>> as map type.

http://codereview.appspot.com/1541042/diff/12001/9003#newcode181
src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:181: while
(elemIter.hasNext()) {
Instead of Iterator we can use for-each loop to iterate through the List since
we support JDK5 to make it less error prone and cleaner code.

http://codereview.appspot.com/1541042/diff/12001/9003#newcode184
src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:184: if
(!curElemMediaType.equalsIgnoreCase(mediaType)) {
I dont think you need to keep track of current media value. When current element
get the "media" attribute, simply check against the Map. If its not there then
create new List.

Otherwise grab the List from Map for  that media type and add the cur element to
it.

http://codereview.appspot.com/1541042/diff/12001/9003#newcode197
src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:197:
curBatch = Lists.newLinkedList();
I think you can immediately add this to both Map and output List since it
guarantee will have at least one Element ...

http://codereview.appspot.com/1541042/diff/12001/9003#newcode206
src/main/java/org/apache/shindig/gadgets/rewrite/ConcatVisitor.java:206: if
(!mediaBatchMap.containsKey(curElemMediaType)) {
... then you dont need to check for leftover since all elements should be
accounted for.

[anupama.dutta, 2010-06-10 12:00:09]

Addressing Lindner's comment related to LinkedHashMultiMap.

[anupama.dutta, 2010-06-10 12:04:48]

Paul, Henry,

Thanks for your comments. I have changed the splitBatchOnMedia code to use
LinkedHashMultiMap and this has simplified the code considerably.
(Henry, I think many of your comments have been addressed or have become
redundant in my current patch).

Please let me know of any other refinements that I can do.

Thanks,
Anupama.