Clean up/Refactor ContentRewriterFeature

This CL removes ContentRewriterFeatureFactory and instead stuffs it into ContentRewriterFeature, whose data-containing class in turn is subdivided to ContentRewriterFeature.Config for differentiation.

It (hopefully) cleans up the ContentRewriter configuration logic into a more direct inheritance model:
* DefaultConfig provides "default" values as bound previously.
* Config provides a constructor that uses a Spec's Feature values to override those found in DefaultConfig, as appropriate.
* Includes/excludes also separated into a MatchBundle class to avoid repeated logic.

[zhoresh, 2010-02-23 00:29:38]

LGTM, minor comments/questions below

http://codereview.appspot.com/218057/diff/1/21
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java
(right):

http://codereview.appspot.com/218057/diff/1/21#newcode174
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java:174:
this.includes = getMatchBundle(defaultInclude,
The original code "normalized" (trim spaces) the different paramaters, was that
removed intentionally?

http://codereview.appspot.com/218057/diff/1/21#newcode229
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java:229:
// gadget spec if !onlyAllowExcludes.
This comment should apply to include not exclude?

http://codereview.appspot.com/218057/diff/1/21#newcode370
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java:370:
public boolean isSplitJsEnabled() {
Why is it part of content rewriter now? 
Also add simple test case for this

http://codereview.appspot.com/218057/diff/1/10
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/BaseRewriterTestCase.java
(right):

http://codereview.appspot.com/218057/diff/1/10#newcode62
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/BaseRewriterTestCase.java:62:
protected Set<String> tags;
Is this tags still needed?

[johnfargo, 2010-02-23 19:48:59]

Already committed this - retrying send of responses and accommodations.

http://codereview.appspot.com/218057/diff/1/21
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java
(right):

http://codereview.appspot.com/218057/diff/1/21#newcode174
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java:174:
this.includes = getMatchBundle(defaultInclude,
Good observation. Added.

On 2010/02/23 00:29:38, zhoresh wrote:
> The original code "normalized" (trim spaces) the different paramaters, was
that
> removed intentionally?
>

http://codereview.appspot.com/218057/diff/1/21#newcode229
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java:229:
// gadget spec if !onlyAllowExcludes.
Also good note. This no longer applies (comment was copied from previous impl).
I've removed the comment, since the gadget spec can override the excludes
whenever it wants. Makes me wonder if that's actually a bug -- the existing test
cases were sort of unclear on this. It seems intuitive and reasonable to me for
exclude overrides to be accepted when specified by the gadget spec in any case.

On 2010/02/23 00:29:38, zhoresh wrote:
> This comment should apply to include not exclude?

http://codereview.appspot.com/218057/diff/1/21#newcode370
java/gadgets/src/main/java/org/apache/shindig/gadgets/rewrite/ContentRewriterFeature.java:370:
public boolean isSplitJsEnabled() {
Side-effect of this being a CL split from a gigantic one that includes a lot of
other changes.

I've added some basic test cases for this trivial value.

On 2010/02/23 00:29:38, zhoresh wrote:
> Why is it part of content rewriter now? 
> Also add simple test case for this

http://codereview.appspot.com/218057/diff/1/10
File
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/BaseRewriterTestCase.java
(right):

http://codereview.appspot.com/218057/diff/1/10#newcode62
java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/BaseRewriterTestCase.java:62:
protected Set<String> tags;
Yes and no :)

No in this CL, yes in several others that will follow.

On 2010/02/23 00:29:38, zhoresh wrote:
> Is this tags still needed?