Feature/JS System Rework

The feature.xml and JS system has served us well, yet various new requirements/requests have arisen that have stressed the current implementation. Among them:
* Ability to cleanly represent "code" features and "JS" features in a dependency tree w/ one another. This allows, for instance, a clean way for "opensocial" to depend on "security-token" (not yet introduced), which in turn depends on "locked-domain" to ensure token security.
* Ability to let a gadget opt out of all "core" JavaScript, instead only including sub-components that are used, to shrink render size.
* Ability to serve JS in a context-sensitive fashion ie. browser-specific code.
* Ability to serve JS in a feature-sensitive fashion ie. only inject certain code when feature="caja" is enabled/active.
* Ability in development mode to easily serve JS that changes when edits are made on it.

For these and general code-cleanup purposes, I've rewritten the Feature/JS inclusion implementation as presented in this CL.

This implementation is functionally 100% backward-compatible with its predecessor. It is only NOT backward-compatible with existing extensions (outside of Shindig's code base) that override JsFeatureLoader/GadgetFeatureRegistry.

With this change, the unit of extension in the features system becomes the <script ...> line in a feature.xml. The processor for this line is a FeatureResourceLoader, for which a default implementation is provided. Each <script> line yields a FeatureResource object, which is in turn used to resolve content and debugContent by rendering/JS code.

Other components are overridable/subclassable as well, but to date I've yet to hear a JS/features extension use case that does not fit in the ResourceLoader model (or, in the current model with dynamically register()'ed features). Hopefully that's right.

An overview of the implementation follows.

FeatureRegistry
- Replaces GadgetFeatureRegistry and JsFeatureLoader
- Performs both register() and getFeaturesXXX() functions
- Does NOT implicitly include "core*" APIs anymore! These are included in the dependency tree. @see GadgetSpec for more below.
- register(...)
  + Registers one or more features in the system, incorporating them into the list of known features and building a validated dependency tree with them. If a circular dependency exists, an error is thrown.
  + Method may be called at ANY time -- the registry is not programmatically locked. This allows (though this use is not tested!) for dynamic feature injection at some later point.
- getFeatureResources(...)
  + Queries the feature tree for resources matching the specified context (GADGET || CONTAINER), which satisfy all the needed dependencies given in the query. Optionally, fills in unknown/unsupported features from the needed list.
- getFeatures(List<String>)
  + Returns feature strings in dependency order that satisfy the given needed features.

FeatureResourceLoader
- @Injected into FeatureRegistry, main extension point for feature system.
- Loads a given FeatureResource from a feature.xml <script src="..."/> line (inline features are handled by FeatureRegistry).
- Key API: load(URI, Map<String,String> attribs)
  + URI is pre-resolved to be absolute by FeatureParser (relative to parent, if a relative URI in feature.xml)
  + "attribs" allows for arbitrary context-sensitive extensions. Example to come soon: limiting by feature="..." to include Caja tamings.js per-feature only when Caja is available.

FeatureParser
- Mostly an implementation detail of FeatureRegistry for now (package-private).
- Parses a String into an intermediary object for further processing.
- Resolves URIs of Resources relative to parent.

GadgetSpec
- If no feature is explicitly <Optional> or <Require>'d starting with "core", automatically inserts the "core" feature for backward compatibility.
- No way (in this CL) to include "absolutely no core" yet.

Gadget
- Modified how addFeature and removeFeature work. They now manage a "directlyRequestedDeps" list, which in turn is used by rendering/processing code for feature resolution.

RenderingGadgetRewriter
- Update injectFeatureLibraries(...) to use new API.
- Hopefully more straightforward: obtain resources requested by the gadget, and resources specified externally. Subtract latter from the former and inline those. Extern the rest. No "core" assumptions, as "core" will be included by the GadgetSpec if needed.

Core features
- Break "core" into core.config, core.json, core.prefs et al.
- "core" now an aggregating feature that has no JS of its own but includes all sub-libs.

All other features
- Specific assignment of dependencies on core libs.

Other
- feature.xml updates, pom.xml updates for separated core libs
- test updates across the board for all affected classes

[johnfargo, 2009-10-28 02:13:52]

Attempt to fix missing code errors in CR tool.

[Jon Weygandt, 2009-10-28 17:51:21]

On 2009/10/28 02:13:52, johnfargo wrote:
> Attempt to fix missing code errors in CR tool.

I have a bug SHINDIG-1183, http://codereview.appspot.com/124086/show, where
there were issues with how GadgetFeatureRegistry (now FeatureRegistry) handled
caching for unsupported features. This has not been applied, and there are
conflicts. It might be best add these to this patch? Pertaining to the
"protected" vs. "private" method changes, I will attempt to download and apply
this patch to see what impact it has.

Pertaining to SHINDIG-1199, the OpenSocialI18NGadgetRewriter issues, and the
ability to extend JsLibrary/JsFeatureLoader - As part of the download and patch
application, I give it a try and respond later.

[Jon Weygandt, 2009-10-28 17:52:58]

http://codereview.appspot.com/143046/diff/80/1043
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/features/FeatureRegistry.java
(right):

http://codereview.appspot.com/143046/diff/80/1043#newcode177
Line 177: public List<FeatureResource> getFeatureResources(
See details in SHINDIG-1183
I believe the same bug is present, where the JS servlet calls this with
unsupported == null, therefore a cache entry is made with unsupported features,
but then when called with unspported != null, the cache is used and unspported
is never filled in.

http://codereview.appspot.com/143046/diff/80/1033
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java
(right):

http://codereview.appspot.com/143046/diff/80/1033#newcode185
Line 185: throw new RuntimeException(e);
Perhaps this should:
throw new RewritingException(e.getLocalizedMessage(), e);
per SHINDIG-1183

[johnfargo, 2009-10-28 20:15:18]

Thanks for the comments, Jon. I'll update a patch shortly that incorporates your
fixes. What I haven't done (locally) yet is make RenderingGadgetRewriter methods
protected. What was the rationale there again? Seems reasonable, just curious.

http://codereview.appspot.com/143046/diff/80/1043
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/features/FeatureRegistry.java
(right):

http://codereview.appspot.com/143046/diff/80/1043#newcode177
Line 177: public List<FeatureResource> getFeatureResources(
Updated patch forthcoming. I thought this logic was weird too when I ported it
over.

http://codereview.appspot.com/143046/diff/80/1033
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java
(right):

http://codereview.appspot.com/143046/diff/80/1033#newcode185
Line 185: throw new RuntimeException(e);
Agreed... I'll merge this in.

[Jon Weygandt, 2009-10-28 23:15:09]

I just tried to "svn update" and apply the patch. There are issues with the
deleted versions of JsFeatureLoader.java,
BrowserSpecificRpcJsFeatureLoader.java, OpenSocialI18NGadgetRewriter.java.
Depending upon your code base, the SHINDIG-1199 patch was recently applied to
those files (OpenSocialI18NGadgetRewriter was modified on 10/14, prior to the
10/27 SHINDIG-1199 patch). If that's the only issue, let's discard SHINDIG-1199,
and try to apply them manually to the latest version.

As to BrowserSpecificFeatureResource.java (the new one), it does not patch
right. Take a look at the diff, most of the new files have (revision 0) for both
versions, and simply all added lines, but not BrowserSpecificFeatureResource.

[johnfargo, 2009-10-29 01:00:39]

Cleaner patch (hopefully)

[Jon Weygandt, 2009-10-29 15:55:06]

On 2009/10/29 01:00:39, johnfargo wrote:
> Cleaner patch (hopefully)

The patch applied cleanly, but seem to be missing the moved feature files (e.g.
features\src\main\javascript\features\core.config\config.js)

[johnfargo, 2009-10-29 16:42:20]

Source control and patch don't play very nicely at all in anything but M
situations. Trying again.

[Jon Weygandt, 2009-10-29 22:23:06]

On 2009/10/29 16:42:20, johnfargo wrote:
> Source control and patch don't play very nicely at all in anything but M
> situations. Trying again.

I got the patch downloaded, built and tried it out in my build - just to the
point of compiling. Looks OK, I applied the "cache" fix from:
http://codereview.appspot.com/124086/show to it. Plus did the private to public
changes.

Since I cannot post a patch to this review, create a new one:
http://codereview.appspot.com/144055.

BTW - noticed in my build some tests failed when building, don't know if that is
simply me, or there are some issues with the patch.

[Jon Weygandt, 2009-10-29 22:30:13]

On 2009/10/29 22:23:06, Jon Weygandt wrote:
> On 2009/10/29 16:42:20, johnfargo wrote:
> > Source control and patch don't play very nicely at all in anything but M
> > situations. Trying again.
> 
> I got the patch downloaded, built and tried it out in my build - just to the
> point of compiling. Looks OK, I applied the "cache" fix from:
> http://codereview.appspot.com/124086/show to it. Plus did the private to
public
> changes.
> 
> Since I cannot post a patch to this review, create a new one:
> http://codereview.appspot.com/144055.
> 
> BTW - noticed in my build some tests failed when building, don't know if that
is
> simply me, or there are some issues with the patch.

I'd like to add, I do like the FeatureResource interface. Early on JsLibrary was
a "final" class, then final was removed and some method to enhance substitution,
now we have a real interface to work with.

[johnfargo, 2009-10-30 01:12:53]

Integrates Jon Weygandt's changes from http://codereview.appspot.com/144055

[Jon Weygandt, 2009-10-30 18:21:08]

http://codereview.appspot.com/143046/diff/2111/2134
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java
(right):

http://codereview.appspot.com/143046/diff/2111/2134#newcode330
Line 330: private String getLibraryConfig(Gadget gadget, List<String> reqs)
Can this be made protected?

[johnfargo, 2009-11-02 20:39:51]

Committed the latest patch in this review, as I'd not received any negative
reviews, and incorporated all of Jon's feedback except the very last note (happy
to make that method protected in a separate followup btw).

This commit broke taming of core APIs - I'm working with Jas to get that
remedied. I committed as seen here to avoid what was continuing to become a
worse merge situation.