Fix bug with external at-rule in conditional. If an @external rule and the definition of the style class associated are contained in a conditional node evaluated to false at compile time and the style class are not associated to a java method (valid for an external style class), the compiler throws an error "The following non-obfuscated class is present in a strict CssResource" Change-Id: Ia110a0dfedf72babc72ce2a2f9126e23bd2a94dc (cherry picked from commit b8b921d22918df694118bb3dcd7c21043e28bd6a)
diff --git a/user/src/com/google/gwt/resources/gss/ExternalClassesCollector.java b/user/src/com/google/gwt/resources/gss/ExternalClassesCollector.java index 0ab7256..ab36ebe 100644 --- a/user/src/com/google/gwt/resources/gss/ExternalClassesCollector.java +++ b/user/src/com/google/gwt/resources/gss/ExternalClassesCollector.java
@@ -16,6 +16,7 @@ package com.google.gwt.resources.gss; import com.google.gwt.thirdparty.common.css.SourceCodeLocation; +import com.google.gwt.thirdparty.common.css.compiler.ast.CssClassSelectorNode; import com.google.gwt.thirdparty.common.css.compiler.ast.CssCompilerPass; import com.google.gwt.thirdparty.common.css.compiler.ast.CssCompositeValueNode; import com.google.gwt.thirdparty.common.css.compiler.ast.CssLiteralNode; @@ -49,6 +50,7 @@ private final ErrorManager errorManager; private Set<String> externalClassNames; + private Set<String> remainingStyleClassNames; private List<String> externalClassPrefixes; private boolean matchAll; @@ -61,12 +63,19 @@ @Override public void runPass() { externalClassNames = new HashSet<String>(); + remainingStyleClassNames = new HashSet<String>(); externalClassPrefixes = new ArrayList<String>(); visitController.startVisit(this); } @Override + public boolean enterClassSelector(CssClassSelectorNode classSelector) { + remainingStyleClassNames.add(classSelector.getRefinerName()); + return true; + } + + @Override public void leaveUnknownAtRule(CssUnknownAtRuleNode node) { if (EXTERNAL_AT_RULE.equals(node.getName().getValue())) { if (!matchAll) { @@ -82,13 +91,32 @@ * {@code @external} as well as all of the class names from * {@code styleClassesSet} that match prefixes defined with {@code @external}. * + * <p>The set will contain also the class names that are not in the AST anymore (defined in a + * conditional node that has been evaluated to false) and are not associated to a java method. + * That doesn't make sense to rename these class names because they are not in the final css + * and javascript. Moreover we handle the case where an {@code @external} related to these + * style classes has been removed from the AST (because it was also defined in a conditional + * node evaluated to false) and the compiler doesn't have to throw and error for this case. + * <pre> + * /{@literal *} conditional node evaluated to false at compile time {@literal *}/ + * @if (is("property", "true")) { + * @external foo; + * .foo { + * width: 100%; + * } + * } + * </pre> + * * @param styleClassesSet a set of class names that should be filtered to * return those matching external prefixes. Note that the passed-in set is not * modified. + * @param orphanClassName a set of class names that aren't associated to a java method of the + * CssResource interface. * @return an immutable set of class names. Note that the returned names are * not prefixed with "."; they are the raw name. */ - public ImmutableSet<String> getExternalClassNames(Set<String> styleClassesSet) { + public ImmutableSet<String> getExternalClassNames(Set<String> styleClassesSet, + Set<String> orphanClassName) { if (matchAll) { return ImmutableSet.copyOf(styleClassesSet); } @@ -107,6 +135,16 @@ } } } + + // all style classes that are not in the AST anymore (mean they were part of a conditional + // node that has been evaluated to false) and that aren't associated to a method should be + // considered as external. See javadoc above + for (String className : orphanClassName) { + if (!remainingStyleClassNames.contains(className)) { + externalClassesSetBuilder.add(className); + } + } + return externalClassesSetBuilder.build(); }
diff --git a/user/src/com/google/gwt/resources/rg/GssResourceGenerator.java b/user/src/com/google/gwt/resources/rg/GssResourceGenerator.java index 08a905a..32dafb0 100644 --- a/user/src/com/google/gwt/resources/rg/GssResourceGenerator.java +++ b/user/src/com/google/gwt/resources/rg/GssResourceGenerator.java
@@ -554,7 +554,8 @@ // add the style classes that aren't associated to a method allStyleClassSet.addAll(renamingResult.externalClassCandidate); - Set<String> externalClasses = externalClassesCollector.getExternalClassNames(allStyleClassSet); + Set<String> externalClasses = externalClassesCollector.getExternalClassNames(allStyleClassSet, + renamingResult.externalClassCandidate); final Map<String, String> revertMap = new HashMap<String, String>(externalClasses.size());
diff --git a/user/test/com/google/gwt/resources/client/gss/externalClasses.gss b/user/test/com/google/gwt/resources/client/gss/externalClasses.gss index 52bbbec..c60b574 100644 --- a/user/test/com/google/gwt/resources/client/gss/externalClasses.gss +++ b/user/test/com/google/gwt/resources/client/gss/externalClasses.gss
@@ -8,29 +8,38 @@ } .obfuscatedClass { - width: 100%; + width: 100%; } .externalClass { - height: 50px; + height: 50px; } .externalClass2 { - height: 50px; + height: 50px; } .unobfuscated { - height: 50px; + height: 50px; } .unobfuscated-with-prefix-without-method { - width:15px; + width: 15px; } .unobfuscated2 { - height: 50px; + height: 50px; } .externalWithoutMethod { color: white; } + +/* will be evaluated to false, shouldn't throw an error*/ +@if (!is("test.property", "true")) { + @external class-without-method; + .class-without-method{ + display: inline-block; + } +} +
diff --git a/user/test/com/google/gwt/resources/gss/ExternalClassesCollectorTest.java b/user/test/com/google/gwt/resources/gss/ExternalClassesCollectorTest.java index fda7ff5..0ed6a1a 100644 --- a/user/test/com/google/gwt/resources/gss/ExternalClassesCollectorTest.java +++ b/user/test/com/google/gwt/resources/gss/ExternalClassesCollectorTest.java
@@ -21,6 +21,7 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import com.google.gwt.thirdparty.common.css.compiler.ast.CssClassSelectorNode; import com.google.gwt.thirdparty.common.css.compiler.ast.CssCompositeValueNode; import com.google.gwt.thirdparty.common.css.compiler.ast.CssLiteralNode; import com.google.gwt.thirdparty.common.css.compiler.ast.CssStringNode; @@ -76,6 +77,7 @@ public void testLeaveUnknownAtRule_simpleExternalAtRule_classesReturnByGetExternalClass() { // Given HashSet<String> styleClassSet = Sets.newHashSet(); + HashSet<String> orphanClassName = Sets.newHashSet(); ExternalClassesCollector externalClassesCollector = createAndInitExternalClassesCollector(); when(atRuleNameNode.getValue()).thenReturn("external"); @@ -91,7 +93,8 @@ verify(atRuleParameters).getValues(); verify(mutatingVisitController).removeCurrentNode(); - Set<String> externalClasses = externalClassesCollector.getExternalClassNames(styleClassSet); + Set<String> externalClasses = externalClassesCollector.getExternalClassNames(styleClassSet, + orphanClassName); assertEquals(2, externalClasses.size()); assertTrue(externalClasses.contains("externalClass")); assertTrue(externalClasses.contains("externalClass2")); @@ -100,6 +103,7 @@ public void testLeaveUnknownAtRule_externalAtRuleWithMatchAllPrefix_allClassesAreExternals() { // Given HashSet<String> styleClassSet = Sets.newHashSet("class1", "class2", "class3"); + HashSet<String> orphanClassName = Sets.newHashSet(); ExternalClassesCollector externalClassesCollector = createAndInitExternalClassesCollector(); when(atRuleNameNode.getValue()).thenReturn("external"); List<CssValueNode> parameters = Lists.newArrayList(stringNode("*")); @@ -113,13 +117,35 @@ verify(atRuleParameters).getValues(); verify(mutatingVisitController).removeCurrentNode(); - Set<String> externalClasses = externalClassesCollector.getExternalClassNames(styleClassSet); + Set<String> externalClasses = externalClassesCollector.getExternalClassNames(styleClassSet, + orphanClassName); assertEquals(3, externalClasses.size()); assertTrue(externalClasses.contains("class1")); assertTrue(externalClasses.contains("class2")); assertTrue(externalClasses.contains("class3")); } + public void + testLeaveUnknownAtRule_styleClassWithoutMethodAndRemovedFromAST_consideredAsExternal() { + // Given + HashSet<String> styleClassSet = Sets.newHashSet("foo", "bar"); + ExternalClassesCollector externalClassesCollector = createAndInitExternalClassesCollector(); + // AST contains only one style class named foo, bar is not in the AST anymore + CssClassSelectorNode classSelectorNode = mock(CssClassSelectorNode.class); + when(classSelectorNode.getRefinerName()).thenReturn("foo"); + externalClassesCollector.enterClassSelector(classSelectorNode); + // The style class bar is not associated to a java method + HashSet<String> orphanClassName = Sets.newHashSet("bar"); + + // When + Set<String> externalClasses = externalClassesCollector.getExternalClassNames(styleClassSet, + orphanClassName); + + // Then + assertEquals(1, externalClasses.size()); + assertTrue(externalClasses.contains("bar")); + } + public void testLeaveUnknownAtRule_externalAtRuleWithMatchAllPrefixThenAnotherExternalAtRule_anotherAtRuleNotProcessed() { // Given ExternalClassesCollector externalClassesCollector = createAndInitExternalClassesCollector(); @@ -145,6 +171,7 @@ // Given HashSet<String> styleClassSet = Sets.newHashSet("prefix", "prefix-class1", "prefi-notexternal","external"); + HashSet<String> orphanClassName = Sets.newHashSet(); ExternalClassesCollector externalClassesCollector = createAndInitExternalClassesCollector(); when(atRuleNameNode.getValue()).thenReturn("external"); List<CssValueNode> parameters = Lists.newArrayList(literalNode("external"), @@ -159,7 +186,8 @@ verify(atRuleParameters).getValues(); verify(mutatingVisitController).removeCurrentNode(); - Set<String> externalClasses = externalClassesCollector.getExternalClassNames(styleClassSet); + Set<String> externalClasses = externalClassesCollector.getExternalClassNames(styleClassSet, + orphanClassName); assertEquals(3, externalClasses.size()); assertTrue(externalClasses.contains("prefix")); assertTrue(externalClasses.contains("prefix-class1"));