I'm not able to view the changes to BrowserSpecificFeatureResource.java. The review tool is displaying "error: old chunk mismatch." Do you see the same thing? http://codereview.appspot.com/143046/diff/1/36
On Tue, Oct 27, 2009 at 5:59 PM, <[email protected]> wrote: > Reviewers: shindig.remailer_gmail.com, jasvir, Jon Weygandt, > > Description: > 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 > > > Please review this at http://codereview.appspot.com/143046 > > Affected files: > features/pom.xml > features/src/main/javascript/features/analytics/feature.xml > features/src/main/javascript/features/auth-refresh/feature.xml > features/src/main/javascript/features/core.auth/feature.xml > features/src/main/javascript/features/core.config/feature.xml > features/src/main/javascript/features/core.io/feature.xml > features/src/main/javascript/features/core.json/feature.xml > features/src/main/javascript/features/core.legacy/feature.xml > features/src/main/javascript/features/core.log/feature.xml > features/src/main/javascript/features/core.prefs/feature.xml > features/src/main/javascript/features/core.util/feature.xml > features/src/main/javascript/features/core/auth-init.js > features/src/main/javascript/features/core/auth.js > features/src/main/javascript/features/core/config.js > features/src/main/javascript/features/core/core.js > features/src/main/javascript/features/core/feature.xml > features/src/main/javascript/features/core/json.js > features/src/main/javascript/features/core/legacy.js > features/src/main/javascript/features/core/log.js > features/src/main/javascript/features/core/prefs.js > features/src/main/javascript/features/core/util.js > features/src/main/javascript/features/features.txt > features/src/main/javascript/features/flash/feature.xml > features/src/main/javascript/features/i18n/feature.xml > features/src/main/javascript/features/opensocial-0.6/feature.xml > features/src/main/javascript/features/opensocial-current/feature.xml > features/src/main/javascript/features/opensocial-data/feature.xml > features/src/main/javascript/features/opensocial-jsonrpc/feature.xml > features/src/main/javascript/features/opensocial-reference/feature.xml > features/src/main/javascript/features/osapi/feature.xml > features/src/main/javascript/features/rpc/feature.xml > features/src/main/javascript/features/setprefs/feature.xml > features/src/main/javascript/features/skins/feature.xml > features/src/main/javascript/features/views/feature.xml > java/common/src/main/java/org/apache/shindig/common/Pair.java > java/gadgets/src/main/java/org/apache/shindig/gadgets/BrowserSpecificRpcJsFeatureLoader.java > java/gadgets/src/main/java/org/apache/shindig/gadgets/DefaultUrlGenerator.java > java/gadgets/src/main/java/org/apache/shindig/gadgets/Gadget.java > java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetFeature.java > java/gadgets/src/main/java/org/apache/shindig/gadgets/GadgetFeatureRegistry.java > java/gadgets/src/main/java/org/apache/shindig/gadgets/JsFeatureLoader.java > java/gadgets/src/main/java/org/apache/shindig/gadgets/JsLibrary.java > java/gadgets/src/main/java/org/apache/shindig/gadgets/features/BrowserSpecificFeatureResource.java > java/gadgets/src/main/java/org/apache/shindig/gadgets/features/FeatureParser.java > java/gadgets/src/main/java/org/apache/shindig/gadgets/features/FeatureRegistry.java > java/gadgets/src/main/java/org/apache/shindig/gadgets/features/FeatureResource.java > java/gadgets/src/main/java/org/apache/shindig/gadgets/features/FeatureResourceLoader.java > java/gadgets/src/main/java/org/apache/shindig/gadgets/process/Processor.java > java/gadgets/src/main/java/org/apache/shindig/gadgets/render/OpenSocialI18NGadgetRewriter.java > java/gadgets/src/main/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriter.java > java/gadgets/src/main/java/org/apache/shindig/gadgets/servlet/JsServlet.java > java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/Feature.java > java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/ModulePrefs.java > java/gadgets/src/main/java/org/apache/shindig/gadgets/spec/Preload.java > java/gadgets/src/main/java/org/apache/shindig/gadgets/templates/tags/FlashTagHandler.java > java/gadgets/src/test/java/org/apache/shindig/gadgets/BrowserSpecificRpcJsFeatureLoaderTest.java > java/gadgets/src/test/java/org/apache/shindig/gadgets/DefaultUrlGeneratorTest.java > java/gadgets/src/test/java/org/apache/shindig/gadgets/GadgetFeatureRegistryTest.java > java/gadgets/src/test/java/org/apache/shindig/gadgets/GadgetTest.java > java/gadgets/src/test/java/org/apache/shindig/gadgets/HashLockedDomainServiceTest.java > java/gadgets/src/test/java/org/apache/shindig/gadgets/JsFeatureLoaderTest.java > java/gadgets/src/test/java/org/apache/shindig/gadgets/JsLibraryTest.java > java/gadgets/src/test/java/org/apache/shindig/gadgets/features/FeatureParserTest.java > java/gadgets/src/test/java/org/apache/shindig/gadgets/features/FeatureRegistryTest.java > java/gadgets/src/test/java/org/apache/shindig/gadgets/features/FeatureResourceLoaderTest.java > java/gadgets/src/test/java/org/apache/shindig/gadgets/render/RenderingGadgetRewriterTest.java > java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/PipelineDataGadgetRewriterTest.java > java/gadgets/src/test/java/org/apache/shindig/gadgets/rewrite/TemplateRewriterTest.java > java/gadgets/src/test/java/org/apache/shindig/gadgets/templates/tags/FlashTagHandlerTest.java > > >
