On Fri, Dec 5, 2014 at 11:56 AM, Andrea Del Bene <[email protected]> wrote:
> On 04/12/14 21:48, Martin Grigorov wrote: > >> On Thu, Dec 4, 2014 at 8:03 PM, <[email protected]> wrote: >> >> Repository: wicket >>> Updated Branches: >>> refs/heads/master f7fc5d095 -> f4b51cd57 >>> >>> >>> WICKET-5780 Add a resource reference for ContextRelativeResource >>> >>> Project: http://git-wip-us.apache.org/repos/asf/wicket/repo >>> Commit: http://git-wip-us.apache.org/repos/asf/wicket/commit/f4b51cd5 >>> Tree: http://git-wip-us.apache.org/repos/asf/wicket/tree/f4b51cd5 >>> Diff: http://git-wip-us.apache.org/repos/asf/wicket/diff/f4b51cd5 >>> >>> Branch: refs/heads/master >>> Commit: f4b51cd577699b1a7d3f1463ce239386cfa68ce5 >>> Parents: f7fc5d0 >>> Author: Andrea Del Bene <[email protected]> >>> Authored: Thu Dec 4 17:38:39 2014 +0100 >>> Committer: Andrea Del Bene <[email protected]> >>> Committed: Thu Dec 4 17:58:02 2014 +0100 >>> >>> ---------------------------------------------------------------------- >>> .../ContextRelativeResourceReference.java | 173 >>> +++++++++++++++++++ >>> .../resource/PackageResourceReference.java | 22 +-- >>> .../ContextRelativeResourceReferenceTest.java | 85 +++++++++ >>> .../wicket/util/resource/ResourceUtils.java | 37 +++- >>> 4 files changed, 294 insertions(+), 23 deletions(-) >>> ---------------------------------------------------------------------- >>> >>> >>> >>> http://git-wip-us.apache.org/repos/asf/wicket/blob/ >>> f4b51cd5/wicket-core/src/main/java/org/apache/wicket/request/resource/ >>> ContextRelativeResourceReference.java >>> ---------------------------------------------------------------------- >>> diff --git >>> a/wicket-core/src/main/java/org/apache/wicket/request/resource/ >>> ContextRelativeResourceReference.java >>> b/wicket-core/src/main/java/org/apache/wicket/request/resource/ >>> ContextRelativeResourceReference.java >>> new file mode 100644 >>> index 0000000..4d7b81f >>> --- /dev/null >>> +++ >>> b/wicket-core/src/main/java/org/apache/wicket/request/resource/ >>> ContextRelativeResourceReference.java >>> @@ -0,0 +1,173 @@ >>> +/* >>> + * Licensed to the Apache Software Foundation (ASF) under one or more >>> + * contributor license agreements. See the NOTICE file distributed with >>> + * this work for additional information regarding copyright ownership. >>> + * The ASF licenses this file to You under the Apache License, Version >>> 2.0 >>> + * (the "License"); you may not use this file except in compliance with >>> + * the License. You may obtain a copy of the License at >>> + * >>> + * http://www.apache.org/licenses/LICENSE-2.0 >>> + * >>> + * Unless required by applicable law or agreed to in writing, software >>> + * distributed under the License is distributed on an "AS IS" BASIS, >>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or >>> implied. >>> + * See the License for the specific language governing permissions and >>> + * limitations under the License. >>> + */ >>> +package org.apache.wicket.request.resource; >>> + >>> +import org.apache.wicket.Application; >>> +import org.apache.wicket.util.lang.Args; >>> +import org.apache.wicket.util.resource.ResourceUtils; >>> + >>> +/** >>> + * This is a ResourceReference to handle context-relative resources such >>> as js, css and >>> + * picture files placed in a folder on the context root (ex: >>> '/css/coolTheme.css'). >>> + * The class has a flag (see {@link #isMinifyIt()}) to decide if >>> referenced resource can be >>> + * minified (ex: '/css/coolTheme.min.css') or not. >>> + * >>> + * @author Andrea Del Bene >>> + */ >>> +public class ContextRelativeResourceReference extends ResourceReference >>> +{ >>> + >>> + /** The Constant serialVersionUID. */ >>> + private static final long serialVersionUID = 1L; >>> + >>> + /** Says if the resource name can be minified or not. */ >>> + private final boolean minifyIt; >>> + >>> + /** The minfied postfix. */ >>> + private final String minPostfix; >>> + >>> + /** The context relative resource. */ >>> + private final ContextRelativeResource contextRelativeResource; >>> + >>> + /** >>> + * Instantiates a new context relative resource reference for the >>> given name. The resource >>> + * will be minified in DEPLOYMENT mode and "min" will be used as >>> postfix. >>> + * >>> + * @param name >>> + * the resource name >>> + */ >>> + public ContextRelativeResourceReference(final String name) >>> + { >>> + this(name, ResourceUtils.MIN_POSTFIX_DEFAULT, true); >>> + } >>> + >>> + >>> + /** >>> + * Instantiates a new context relative resource reference for the >>> given name. >>> + * Parameter {@code minifyIt} says if the resource can be >>> minified >>> (true) or not (false). >>> + * >>> + * @param name >>> + * the resource name >>> + * @param minifyIt >>> + * says if the resource name can be >>> minified or not >>> + */ >>> + public ContextRelativeResourceReference(final String name, final >>> boolean minifyIt) >>> + { >>> + this(name, ResourceUtils.MIN_POSTFIX_DEFAULT, minifyIt); >>> + } >>> + >>> + /** >>> + * Instantiates a new context relative resource reference for the >>> given name. We can >>> + * specify which postfix we want to use for minification with >>> parameter @code minPostfix} >>> + * >>> + * @param name >>> + * the resource name >>> + * @param minPostfix >>> + * the minfied postfix >>> + */ >>> + public ContextRelativeResourceReference(final String name, final >>> String minPostfix) >>> + { >>> + this(name, minPostfix, true); >>> + } >>> + >>> + /** >>> + * Instantiates a new context relative resource reference for the >>> given name. We can >>> + * specify which postfix we want to use for minification with >>> parameter @code minPostfix} >>> + * while parameter {@code minifyIt} says if the resource can be >>> minified (true) or not (false). >>> + * @param name >>> + * the resource name >>> + * @param minPostfix >>> + * the minfied postfix >>> + * @param minifyIt >>> + * says if the resource name can be >>> minified or not >>> + */ >>> + public ContextRelativeResourceReference(final String name, final >>> String minPostfix, final boolean minifyIt) >>> + { >>> + super(name); >>> + >>> + Args.notNull(minPostfix, "minPostfix"); >>> + >>> + this.minPostfix = minPostfix; >>> + this.minifyIt = minifyIt; >>> >>> >> + this.contextRelativeResource = >>> buildContextRelativeResource(name, minPostfix); >>> >>> What is the reason the resource has to be pre-built. A new instance >> could >> be instantiated on each request in #getReource() too. >> >> Just for performance reasons. Do you think it's neglectable? I think the performance shouldn't be a problem. > > + } >>> + >>> + /** >>> + * Build the context-relative resource for this resource >>> reference. >>> + * >>> + * @param name >>> + * the resource name >>> + * @param minPostfix >>> + * the postfix to use to minify the >>> resource name (typically "min") >>> + * @return the context-relative resource >>> + */ >>> + protected ContextRelativeResource >>> buildContextRelativeResource(final String name, final String minPostfix) >>> >>> Overrideable method called by the constructor. May lead to subtle bugs >> ... >> >> Ok, better make it final.... > > + { >>> + String minifiedName = name; >>> + >>> + if (canBeMinified()) >>> + { >>> + minifiedName = ResourceUtils.getMinifiedName( >>> name, >>> minPostfix); >>> + } >>> + >>> + return new ContextRelativeResource(minifiedName); >>> + } >>> + >>> + /** >>> + * Says if the referenced resource can be minified. It returns >>> {@code true} if >>> + * both flag {@link #minifyIt} and application's resource >>> settings >>> method >>> + * {@link >>> org.apache.wicket.settings.ResourceSettings#getUseMinifiedResources()}} >>> + * are true. >>> + * >>> + * @return {@code true} if resource can be minified, {@code >>> false} >>> otherwise >>> + */ >>> + protected boolean canBeMinified() >>> + { >>> + return minifyIt && Application.exists() >>> + && >>> Application.get().getResourceSettings().getUseMinifiedResources(); >>> + } >>> + >>> + /* (non-Javadoc) >>> + * @see >>> org.apache.wicket.request.resource.ResourceReference#getResource() >>> + */ >>> >>> This kind of javadoc is just noise. I'm trying to remove them from the >> rest >> of the code anyway. >> > Ok > >> >> >> + @Override >>> + public final ContextRelativeResource getResource() >>> + { >>> + return contextRelativeResource; >>> + } >>> + >>> + /** >>> + * Returns the flag that says if the resource can be minified >>> (true) or not (false). >>> + * >>> + * @return true, if resource can be minified >>> + */ >>> + public final boolean isMinifyIt() >>> >>> Why 'final' ? >> This just reduces the flexibility. >> And people complain about the overusa of 'final' in Wicket every second >> day >> ... >> >> Ok. Remove 'final' only if a new resource is constructed for every call to #getResource(). Otherwise there is no need to remove the 'final'. > > + { >>> + return minifyIt; >>> + } >>> + >>> + >>> + /** >>> + * Gets the minified postfix we use for this resource. >>> + * >>> + * @return the minified postfix >>> + */ >>> + public final String getMinPostfix() >>> >>> final >> >> >> + { >>> + return minPostfix; >>> + } >>> +} >>> >>> >>> http://git-wip-us.apache.org/repos/asf/wicket/blob/ >>> f4b51cd5/wicket-core/src/main/java/org/apache/wicket/request/resource/ >>> PackageResourceReference.java >>> ---------------------------------------------------------------------- >>> diff --git >>> a/wicket-core/src/main/java/org/apache/wicket/request/resource/ >>> PackageResourceReference.java >>> b/wicket-core/src/main/java/org/apache/wicket/request/resource/ >>> PackageResourceReference.java >>> index 1c874ad..749769e 100644 >>> --- >>> a/wicket-core/src/main/java/org/apache/wicket/request/resource/ >>> PackageResourceReference.java >>> +++ >>> b/wicket-core/src/main/java/org/apache/wicket/request/resource/ >>> PackageResourceReference.java >>> @@ -21,10 +21,11 @@ import java.util.concurrent.ConcurrentMap; >>> >>> import org.apache.wicket.Application; >>> import org.apache.wicket.Session; >>> +import >>> org.apache.wicket.core.util.resource.locator.IResourceStreamLocator; >>> import org.apache.wicket.util.lang.Generics; >>> import org.apache.wicket.util.lang.Packages; >>> import org.apache.wicket.util.resource.IResourceStream; >>> -import >>> org.apache.wicket.core.util.resource.locator.IResourceStreamLocator; >>> +import org.apache.wicket.util.resource.ResourceUtils; >>> import org.slf4j.Logger; >>> import org.slf4j.LoggerFactory; >>> >>> @@ -216,24 +217,7 @@ public class PackageResourceReference extends >>> ResourceReference >>> protected String getMinifiedName() >>> { >>> String name = super.getName(); >>> - String minifiedName; >>> - int idxOfExtension = name.lastIndexOf('.'); >>> - if (idxOfExtension > -1) >>> - { >>> - String extension = name.substring(idxOfExtension) >>> ; >>> - final String baseName = name.substring(0, >>> name.length() - extension.length() + 1); >>> - if (!".min".equals(extension) && >>> !baseName.endsWith(".min.")) >>> - { >>> - minifiedName = baseName + "min" + >>> extension; >>> - } else >>> - { >>> - minifiedName = name; >>> - } >>> - } else >>> - { >>> - minifiedName = name + ".min"; >>> - } >>> - return minifiedName; >>> + return ResourceUtils.getMinifiedName(name, >>> ResourceUtils.MIN_POSTFIX_DEFAULT); >>> } >>> >>> /** >>> >>> >>> http://git-wip-us.apache.org/repos/asf/wicket/blob/ >>> f4b51cd5/wicket-core/src/test/java/org/apache/wicket/request/resource/ >>> ContextRelativeResourceReferenceTest.java >>> ---------------------------------------------------------------------- >>> diff --git >>> a/wicket-core/src/test/java/org/apache/wicket/request/resource/ >>> ContextRelativeResourceReferenceTest.java >>> b/wicket-core/src/test/java/org/apache/wicket/request/resource/ >>> ContextRelativeResourceReferenceTest.java >>> new file mode 100644 >>> index 0000000..80c31bd >>> --- /dev/null >>> +++ >>> b/wicket-core/src/test/java/org/apache/wicket/request/resource/ >>> ContextRelativeResourceReferenceTest.java >>> @@ -0,0 +1,85 @@ >>> +/* >>> + * Licensed to the Apache Software Foundation (ASF) under one or more >>> + * contributor license agreements. See the NOTICE file distributed with >>> + * this work for additional information regarding copyright ownership. >>> + * The ASF licenses this file to You under the Apache License, Version >>> 2.0 >>> + * (the "License"); you may not use this file except in compliance with >>> + * the License. You may obtain a copy of the License at >>> + * >>> + * http://www.apache.org/licenses/LICENSE-2.0 >>> + * >>> + * Unless required by applicable law or agreed to in writing, software >>> + * distributed under the License is distributed on an "AS IS" BASIS, >>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or >>> implied. >>> + * See the License for the specific language governing permissions and >>> + * limitations under the License. >>> + */ >>> +package org.apache.wicket.request.resource; >>> + >>> +import org.apache.wicket.mock.MockApplication; >>> +import org.apache.wicket.util.tester.WicketTester; >>> +import org.junit.Assert; >>> +import org.junit.BeforeClass; >>> +import org.junit.Test; >>> + >>> + >>> +public class ContextRelativeResourceReferenceTest >>> +{ >>> + private static WicketTester tester; >>> >>> Why static ? >> It begs for memory/thread local leaks. >> Why not extending from WicketTestCase ? >> > Well, actually I don't need WicketTester for my tests, I just need to have > a valid Application in the ThreadContext. I will remove static variable and > if I don't find better solutions I will use the classic WicketTestCase. > >> + >>> + static final String RESOURCE_NAME = "/foo/baar/myLibrary"; >>> + static final String ALREADY_MINIFIED = RESOURCE_NAME + ".min.js"; >>> + static final String TO_BE_MINIFIED = RESOURCE_NAME + ".js"; >>> + static final String CUSTOM_SUFFIX = "compress"; >>> + >>> + @BeforeClass >>> + static public void setUp() >>> + { >>> + MockApplication application = new MockApplication() >>> + { >>> + @Override >>> + protected void init() >>> + { >>> + super.init(); >>> + >>> getResourceSettings().setUseMinifiedResources(true); >>> + } >>> + }; >>> + >>> + tester = new WicketTester(application); >>> + } >>> + >>> + >>> + @Test >>> + public void testMinifyResource() throws Exception >>> + { >>> + ContextRelativeResourceReference resourceReference = new >>> ContextRelativeResourceReference(TO_BE_MINIFIED); >>> + Assert.assertTrue(testResourceKey(resourceReference, >>> ALREADY_MINIFIED)); >>> >>> no tester.destroy() => thread local leak >> >> >> + } >>> + >>> + @Test >>> + public void testDontMinifyResource() throws Exception >>> + { >>> + ContextRelativeResourceReference resourceReference = new >>> ContextRelativeResourceReference(ALREADY_MINIFIED, false); >>> + Assert.assertTrue(testResourceKey(resourceReference, >>> ALREADY_MINIFIED)); >>> + >>> + resourceReference = new >>> ContextRelativeResourceReference(TO_BE_MINIFIED, false); >>> + Assert.assertTrue(testResourceKey(resourceReference, >>> TO_BE_MINIFIED)); >>> >>> no tester.destroy() => thread local leak >> >> >> + >>> + } >>> + >>> + @Test >>> + public void testCustomSuffix() throws Exception >>> + { >>> + ContextRelativeResourceReference resourceReference = new >>> ContextRelativeResourceReference(TO_BE_MINIFIED, CUSTOM_SUFFIX); >>> + Assert.assertTrue(testResourceKey(resourceReference, >>> RESOURCE_NAME + "." + CUSTOM_SUFFIX + ".js")); >>> >>> no tester.destroy() => thread local leak >> >> >> + } >>> + >>> + private boolean testResourceKey(ContextRelativeResourceReference >>> resourceReference, String expectedName) >>> + { >>> + ContextRelativeResource resource = >>> resourceReference.getResource(); >>> + String resourceKey = resource.getCacheKey().toString(); >>> + >>> + return resourceKey.endsWith(expectedName); >>> + } >>> + >>> +} >>> >>> >>> http://git-wip-us.apache.org/repos/asf/wicket/blob/ >>> f4b51cd5/wicket-util/src/main/java/org/apache/wicket/util/ >>> resource/ResourceUtils.java >>> ---------------------------------------------------------------------- >>> diff --git >>> a/wicket-util/src/main/java/org/apache/wicket/util/ >>> resource/ResourceUtils.java >>> b/wicket-util/src/main/java/org/apache/wicket/util/ >>> resource/ResourceUtils.java >>> index a51f359..5fd28dc 100644 >>> --- >>> a/wicket-util/src/main/java/org/apache/wicket/util/ >>> resource/ResourceUtils.java >>> +++ >>> b/wicket-util/src/main/java/org/apache/wicket/util/ >>> resource/ResourceUtils.java >>> @@ -31,6 +31,8 @@ import org.apache.wicket.util.string.Strings; >>> */ >>> public class ResourceUtils >>> { >>> + public static final String MIN_POSTFIX_DEFAULT = "min"; >>> >>> missing javadoc >> >> ok > >> + >>> private static final Pattern LOCALE_PATTERN = >>> Pattern.compile("_([a-z]{2})(_([A-Z]{2})(_([^_]+))?)?$"); >>> >>> private final static Set<String> isoCountries = new HashSet<>( >>> @@ -38,14 +40,41 @@ public class ResourceUtils >>> >>> private final static Set<String> isoLanguages = new HashSet<>( >>> Arrays.asList(Locale.getISOLanguages())); >>> - >>> + >>> /** >>> - * Construct. >>> + * Return the minified version for a given resource name. >>> + * For example '/css/coolTheme.css' becomes >>> '/css/coolTheme.min.css' >>> + * >>> + * @param name >>> + * The original resource name >>> + * @param minPostfix >>> + * The postfix to use for minified name >>> + * @return The minified resource name >>> */ >>> >>> >> - private ResourceUtils() >>> >>> Why did you remove the private constructor ? >> Is this class supposed to be instantiated now ? >> > No no, it's not intended to be instantiated. I just realized now that this > is a practice to prevent "static" classes from being instantiated. I will > revert this change. > > >> >> + public static String getMinifiedName(String name, String >>> minPostfix) >>> { >>> + String minifiedName; >>> + int idxOfExtension = name.lastIndexOf('.'); >>> + final String dottedPostfix = "." + minPostfix; >>> + >>> + if (idxOfExtension > -1) >>> + { >>> + String extension = name.substring(idxOfExtension) >>> ; >>> + final String baseName = name.substring(0, >>> name.length() - extension.length() + 1); >>> + if (!dottedPostfix.equals(extension) && >>> !baseName.endsWith(dottedPostfix + ".")) >>> + { >>> + minifiedName = baseName + minPostfix + >>> extension; >>> + } else >>> + { >>> + minifiedName = name; >>> + } >>> + } else >>> + { >>> + minifiedName = name + dottedPostfix; >>> >> + } >> >>> + return minifiedName; >>> } >>> - >>> + >>> /** >>> * Extract the locale from the filename >>> * >>> >>> >>> > Thanks!
