Hi Carsten, On Thu, Dec 19, 2013 at 6:02 AM, Carsten Ziegeler <cziege...@apache.org> wrote: > ...Everything can be found at: > http://svn.apache.org/repos/asf/sling/whiteboard/feature-flags/ ...
Today is Big Reviews Day, finally found time to look at that, thanks for it! It looks good to me for our current needs, here are my comments: 1) Why not put the API in the org.apache.sling.extensions.featureflags.api package? It's at org.apache.sling.extensions.featureflags currently. 2) How do you envision handling feature naming collisions between FeatureProviders? We might namespace feature names by requiring all values returned by the getFeatureNames() method of a given FeatureProvider to start with a common prefix followed by a dot, and the prefix must be unique system-wide. 3) Considering Roy's remark that we'll get about 1024 features in a Sling instance, and user's creativity which will probably boost that to 10'000, shall we use Iterator<String> for lists of feature names? 4) I'm not sure about FeatureProvider.hideResource(featureName, Resource) - as seen from the ResourceResolver, IMO you just care whether a given FeatureProvider wants to hide a Resource or not, you don't really care (*) which feature name causes the Resource to be hidden. So maybe just hideResource(Resource, ProviderContext) ? The same might apply to FeatureProvider.getResourceTypeMapping which would then just take a ProviderContext - but maybe I'm missing something. -Bertrand (*) Except for troubleshooting, but that's covered by RequestProgressTracker which is accessible via ProviderContext.getRequest()
