Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1915)

Unified Diff: pkg/csslib/lib/src/analyzer.dart

Issue 23819036: Support for @mixin, @include and @extend (Closed) Base URL: https://dart.googlecode.com/svn/branches/bleeding_edge/dart
Patch Set: mixin w/o parameters Created 7 years, 3 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: pkg/csslib/lib/src/analyzer.dart
diff --git a/pkg/csslib/lib/src/analyzer.dart b/pkg/csslib/lib/src/analyzer.dart
index 9e366a5fdfcc1b4d634dfc94512d5e809a9b5989..c16d9f07c551fbbb993be7aa08107e9794c1be41 100644
--- a/pkg/csslib/lib/src/analyzer.dart
+++ b/pkg/csslib/lib/src/analyzer.dart
@@ -23,16 +23,27 @@ part of csslib.parser;
class Analyzer {
final List<StyleSheet> _styleSheets;
final Messages _messages;
+ MixinDefinitions mixinDefs;
VarDefinitions varDefs;
Analyzer(this._styleSheets, this._messages);
void run() {
+ mixinDefs = new MixinDefinitions(_styleSheets);
nweiz 2013/09/18 22:40:54 It's not correct to locate all mixin definitions b
terry 2013/10/09 03:40:33 Yeah, I liked the idea of hoisting like a class bu
+
+ // Any mixin cycles?
+ var mixinCycles = findAllMixinCycles();
+ for (var cycle in mixinCycles) {
+ _messages.warning("mixin cycle detected @mixin ${cycle.def.name}",
nweiz 2013/09/18 22:40:54 "Mixin cycle detected for @mixin ${cycle.def.name}
terry 2013/10/09 03:40:33 Without hoisting cycle detection is removed. On 20
+ cycle.def.span);
+ // TODO(terry): What if no mixin definition for a @include an error?
nweiz 2013/09/18 22:40:54 "What if there are no mixin definitions for an @in
+ }
+
varDefs = new VarDefinitions(_styleSheets);
- // Any cycles?
- var cycles = findAllCycles();
- for (var cycle in cycles) {
+ // Any var cycles?
+ var varCycles = findAllVarCycles();
nweiz 2013/09/18 22:40:54 Variables should be eagerly evaluated, so cycle de
terry 2013/10/09 03:40:33 Yes also removed variable hoisting. On 2013/09/18
+ for (var cycle in varCycles) {
_messages.warning("var cycle detected var-${cycle.definedName}",
cycle.span);
// TODO(terry): What if no var definition for a var usage an error?
@@ -42,7 +53,28 @@ class Analyzer {
// Remove any var definition from the stylesheet that has a cycle.
_styleSheets.forEach((styleSheet) =>
- new RemoveVarDefinitions(cycles).visitStyleSheet(styleSheet));
+ new RemoveVarDefinitions(varCycles).visitStyleSheet(styleSheet));
+
+ // Expand top-level @include.
+ _styleSheets.forEach((styleSheet) =>
+ new ExpandTopLevelIncludes(mixinDefs, _messages)
+ ..visitStyleSheet(styleSheet));
nweiz 2013/09/18 22:40:54 Running separate visitors for all of these seems i
terry 2013/10/09 03:40:33 Yes, for now that is the mode we've taken in the d
+
+ // Expand @include in declarations.
nweiz 2013/09/18 22:40:54 Nit: extra space before "@include".
terry 2013/10/09 03:40:33 Done.
+ _styleSheets.forEach((styleSheet) =>
+ new ExpandDeclarationIncludes(styleSheet, mixinDefs, _messages)
+ ..visitStyleSheet(styleSheet));
+
+ // Any missing @mixins only check if everything is good up to this point.
nweiz 2013/09/18 22:40:54 I don't understand what this comment means.
terry 2013/10/09 03:40:33 Functionality removed when hoisting of mixin remov
+ if (_messages.messages.isEmpty) {
+ _styleSheets.forEach((styleSheet) =>
+ new IncludesMissingMixins(mixinDefs, this._messages)
+ ..visitStyleSheet(styleSheet));
+ }
+
+ // Remove all @mixin and @include
+ _styleSheets.forEach((styleSheet) =>
+ new RemoveMixinsAndIncludes()..visitStyleSheet(styleSheet));
nweiz 2013/09/18 22:40:54 Style nit: indent +2 spaces.
terry 2013/10/09 03:40:33 Code removed. On 2013/09/18 22:40:54, nweiz wrote:
// Expand any nested selectors using selector desendant combinator to
// signal CSS inheritance notation.
@@ -51,7 +83,83 @@ class Analyzer {
..flatten(styleSheet));
}
- List<VarDefinition> findAllCycles() {
+ List<MixinDefinition> findAllMixinCycles() {
+ var cycles = [];
+
+ mixinDefs.map.values.forEach((value) {
nweiz 2013/09/18 22:40:54 As John's mentioned elsewhere, use a for loop rath
terry 2013/10/09 03:40:33 Cycle code removed. On 2013/09/18 22:40:54, nweiz
+ if (hasMixinCycle(value.includes)) cycles.add(value);
+ });
+
+ // Update our local list of known mixinDefinitions remove any mixins with a
+ // cycle. So the same mixin cycle isn't reported for each style sheet
+ // processed.
+ for (var cycle in cycles) {
+ mixinDefs.map.remove(cycle.def.name);
+ }
+
+ return cycles;
+ }
+
+ /**
+ * [includes] list of IncludeDirective or IncludeMixinAtDeclaration.
nweiz 2013/09/18 22:40:54 If you're going to include a doc comment, it shoul
terry 2013/10/09 03:40:33 Cycle code removed. On 2013/09/18 22:40:54, nweiz
+ */
+ bool hasMixinCycle(List includes, {Set<String> visiting,
+ Set<String> visited}) {
+ if (visiting == null) visiting = new Set();
+ if (visited == null) visited = new Set();
nweiz 2013/09/18 22:40:54 Explain the function of [visited] and how it diffe
terry 2013/10/09 03:40:33 cycle code removed. On 2013/09/18 22:40:54, nweiz
+
+ bool cycleDetected = false;
+
+ var name;
+ for (var include in includes) {
+ if (include is IncludeMixinAtDeclaration) {
+ // Get to the IncludeDirective.
+ name = include.include.name;
+ } else {
+ name = include.name;
+ }
+
+ if (visiting.contains(name)) {
+ cycleDetected = true;
+ break;
+ }
+ if (!visited.contains(name)) {
+ visiting.add(name);
+ visited.add(name);
+ }
+ if (mixinDefs.map[name] != null) {
+ if (hasMixinCycle(mixinDefs.map[name].includes, visiting: visiting,
+ visited: visited)) {
+ cycleDetected = true;
+ break;
+ }
+ }
+
+
+
+/*
Jennifer Messerly 2013/09/18 06:44:15 remove?
terry 2013/10/09 03:40:33 entire function removed. On 2013/09/18 06:44:15, J
+ for (var usage in mixinInclude(mixinDefs.map[name].includes)) {
+ if (hasMixinCycle(usage.name, visiting: visiting, visited: visited)) {
+ cycleDetected = true;
+ break;
+ }
+ }
+*/
+ visiting.remove(include);
+ }
+
+ return cycleDetected;
+ }
+
+ // TODO(terry): Only checks @includes in declarations not ruleset top-level.
+ Iterable<IncludeDirective> mixinInclude(List includes) =>
+ includes.where((e) => e is IncludeMixinAtDeclaration);
+
+ // =====================
+ // Var cycles functions:
+ // =====================
+
+ List<VarDefinition> findAllVarCycles() {
var cycles = [];
varDefs.map.values.forEach((value) {
@@ -511,3 +619,253 @@ class _MediaRulesReplacer extends Visitor {
}
}
}
+
+class MixinDef {
+ MixinDefinition def;
+ List<IncludeDirective> includes;
+
+ MixinDef(this.def, this.includes);
+}
+
+/** Find all mixin definitions from a list of stylesheets. */
+class MixinDefinitions extends Visitor {
+ /** Map of variable name key to it's definition. */
+ final Map<String, MixinDef> map = new Map<String, MixinDef>();
nweiz 2013/09/18 22:40:54 This type declaration is unnecessary. The type is
terry 2013/10/09 03:40:33 Yep but editor code completion didn't work w/o it
+
+ var _includes = [];
+
+ MixinDefinitions(List<StyleSheet> styleSheets) {
+ for (var styleSheet in styleSheets) {
+ visitTree(styleSheet);
+ }
+ }
+
+ void visitIncludeDirective(IncludeDirective node) {
+ _includes.add(node);
+ }
+
+ void visitIncludeMixinAtDeclaration(IncludeMixinAtDeclaration node) {
+ _includes.add(node);
+ }
+
+ void visitMixinRulesetDirective(MixinRulesetDirective node) {
+ _includes = [];
+
+ super.visitMixinRulesetDirective(node);
+
+ // Replace with latest top-level mixin definition.
+ map[node.name] = new MixinDef(node, _includes);
+ _includes = [];
+ }
+
+ void visitMixinDeclarationDirective(MixinDeclarationDirective node) {
+ _includes = [];
+
+ super.visitMixinDeclarationDirective(node);
+
+ // Replace with latest mixin definition.
+ map[node.name] = new MixinDef(node, _includes);
+ _includes = [];
+ }
+}
+
+/**
+ * Expand all @include at the top-level the ruleset(s) associated with the
+ * mixin.
+ */
+class ExpandTopLevelIncludes extends Visitor {
+ StyleSheet _styleSheet;
+ final MixinDefinitions defs;
+ final Messages _messages;
+
+ ExpandTopLevelIncludes(this.defs, this._messages);
+
+ void visitStyleSheet(StyleSheet ss) {
+ _styleSheet = ss;
+ super.visitStyleSheet(ss);
+ _styleSheet = null;
+ }
+
+ void visitIncludeDirective(IncludeDirective node) {
+ if (defs.map.containsKey(node.name)) {
+ var mixinDef = defs.map[node.name].def;
+ if (mixinDef is MixinRulesetDirective) {
+ _TopLevelIncludeReplacer.replace(_styleSheet, node, mixinDef.rulesets);
+ }
+ }
+ super.visitIncludeDirective(node);
+ }
+}
+
+/** @include as a top-level with ruleset(s). */
+class _TopLevelIncludeReplacer extends Visitor {
+ final IncludeDirective _include;
+ final List<RuleSet> _newRules;
+ bool _foundAndReplaced = false;
+
+ /**
+ * Look for the [ruleSet] inside of an @media directive; if found then replace
+ * with the [newRules]. If [ruleSet] is found and replaced return true.
+ */
+ static bool replace(StyleSheet styleSheet, IncludeDirective include,
nweiz 2013/09/18 22:40:54 You have three different ways of invoking a visito
terry 2013/10/09 03:40:33 Function was re-written this no longer exist. On 2
+ List<RuleSet>newRules) {
+ var visitor = new _TopLevelIncludeReplacer(include, newRules);
+ visitor.visitStyleSheet(styleSheet);
+ return visitor._foundAndReplaced;
nweiz 2013/09/18 22:40:54 This value is never used.
terry 2013/10/09 03:40:33 function re-written not an issue now. On 2013/09/1
+ }
+
+ _TopLevelIncludeReplacer(this._include, this._newRules);
+
+ visitStyleSheet(StyleSheet node) {
+ var index = node.topLevels.indexOf(_include);
+ if (index != -1) {
+ node.topLevels.insertAll(index + 1, _newRules);
+ node.topLevels.replaceRange(index, index + 1, [new NoOp()]);
+ _foundAndReplaced = true;
+ }
+ super.visitStyleSheet(node);
+ }
+
+ void visitMixinRulesetDirective(MixinRulesetDirective node) {
+ var index = node.rulesets.indexOf(_include as dynamic);
nweiz 2013/09/18 22:40:54 What is "as dynamic" doing here?
terry 2013/10/09 03:40:33 _include can be different either declarative vs/ r
+ if (index != -1) {
+ node.rulesets.insertAll(index + 1, _newRules);
+ node.rulesets.replaceRange(index, index + 1, [new NoOp()]);
nweiz 2013/09/18 22:40:54 Why aren't you replacing the include with _newRule
terry 2013/10/09 03:40:33 Only the resolve the @include once. On 2013/09/18
+ _foundAndReplaced = true;
+ }
+ super.visitMixinRulesetDirective(node);
+ }
+}
+
+/** Expand all @include inside of a declaration associated with a mixin. */
+class ExpandDeclarationIncludes extends Visitor {
+ final StyleSheet _styleSheet;
+ final MixinDefinitions defs;
+ final Messages _messages;
+
+ ExpandDeclarationIncludes(this._styleSheet, this.defs, this._messages);
+
+ bool _allIncludes(rulesets) =>
+ rulesets.every((rule) => rule is IncludeDirective || rule is NoOp);
+
+ void visitIncludeMixinAtDeclaration(IncludeMixinAtDeclaration node) {
+ if (defs.map.containsKey(node.include.name)) {
nweiz 2013/09/18 22:40:54 Short-circuit if this is false to avoid extra nest
terry 2013/10/09 03:40:33 Really, seems clunky to call super for the short-c
+ var mixinDef = defs.map[node.include.name].def;
+ if (mixinDef is MixinDeclarationDirective) {
+ _IncludeReplacer.replace(_styleSheet, node,
+ mixinDef.declarations.declarations);
+ } else if (mixinDef is MixinRulesetDirective) {
+ // We're a list of @include(s) inside of a mixin ruleset if so then
+ // we'll convert to a list of IncludeMixinAtDeclaration(s).
nweiz 2013/09/18 22:40:54 It doesn't make sense to say "We're X. If so, ..."
terry 2013/10/09 03:40:33 yep, changed. On 2013/09/18 22:40:54, nweiz wrote:
+ var orgRulesets = mixinDef.rulesets;
nweiz 2013/09/18 22:40:54 What does "org" mean?
terry 2013/10/09 03:40:33 original - missing 'i' origRulesets On 2013/09/18
+ var rulesets = [];
+ if (orgRulesets.every((ruleset) => ruleset is IncludeDirective)) {
+ orgRulesets.forEach((ruleset) {
+ rulesets.add(new IncludeMixinAtDeclaration(ruleset, ruleset.span));
+ });
+ _IncludeReplacer.replace(_styleSheet, node, rulesets);
+ } else {
+ _messages.warning(
+ "Trying to use top-level ruleset in a declaration - "
+ "@include ${node.include.name}",
+ node.span);
+ }
+ }
+ }
+
+ super.visitIncludeMixinAtDeclaration(node);
+ }
+}
+
+/** @include as a top-level with ruleset(s). */
+class _IncludeReplacer extends Visitor {
+ final _include;
+ final List<Declaration> _newDeclarations;
+ bool _foundAndReplaced = false;
+
+ /**
+ * Look for the [ruleSet] inside of an @media directive; if found then replace
nweiz 2013/09/18 22:40:54 "a @media"
terry 2013/10/09 03:40:33 Done.
+ * with the [newRules]. If [ruleSet] is found and replaced return true.
+ */
+ static bool replace(StyleSheet ss, var include,
+ List<Declaration> newDeclarations) {
+ var visitor = new _IncludeReplacer(include, newDeclarations);
+ visitor.visitStyleSheet(ss);
+ return visitor._foundAndReplaced;
nweiz 2013/09/18 22:40:54 This is also never used.
terry 2013/10/09 03:40:33 Done.
+ }
+
+ _IncludeReplacer(this._include, this._newDeclarations);
+
+ void visitDeclarationGroup(DeclarationGroup node) {
+ var index = -1;
+ var i = 0;
+ for (var decl in node.declarations) {
+ if (decl == _include) {
+ index = i;
+ break;
+ } else if (decl is IncludeMixinAtDeclaration) {
+ if (decl.include == _include) {
nweiz 2013/09/18 22:40:54 Use "&&" rather than nested if statements.
terry 2013/10/09 03:40:33 While re-writing not an issue anymore. On 2013/09/
+ index = i;
+ break;
+ }
+ }
+ i++;
+ }
nweiz 2013/09/18 22:40:54 This i++/for/break stuff is really complicated and
terry 2013/10/09 03:40:33 ditto on re-write.On 2013/09/18 22:40:54, nweiz wr
+
+ if (index != -1) {
nweiz 2013/09/18 22:40:54 Use null rather than -1 as a flag value.
terry 2013/10/09 03:40:33 ditto re-wrote not an issue. On 2013/09/18 22:40:5
+ node.declarations.insertAll(index + 1, _newDeclarations);
+ // Change @include to NoOp so it's processed only once.
+ node.declarations.replaceRange(index, index + 1, [new NoOp()]);
+ _foundAndReplaced = true;
+ }
+ super.visitDeclarationGroup(node);
+ }
+}
+
+class IncludesMissingMixins extends Visitor {
+ final MixinDefinitions defs;
+ final Messages _messages;
+
+ IncludesMissingMixins(this.defs, this._messages);
+
+ void visitIncludeDirective(IncludeDirective node) {
+ if (!defs.map.containsKey(node.name)) {
+ // @mixin is undefined
+ _messages.warning("@mixin ${node.name} is undefined.", node.span);
nweiz 2013/09/18 22:40:54 This should be an error.
terry 2013/10/09 03:40:33 Class removed. On 2013/09/18 22:40:54, nweiz wrote
+ }
+
+ super.visitIncludeDirective(node);
+ }
+}
+
+/**
+ * Remove all @mixin and @include and any NoOp used as placeholder for @include.
+ */
+class RemoveMixinsAndIncludes extends Visitor {
nweiz 2013/09/18 22:40:54 Don't the other visitors remove the mixins and inc
terry 2013/10/09 03:40:33 class was eliminated when cycles removed. On 2013/
+ RemoveMixinsAndIncludes();
+
+ bool _nodesToRemove(node) =>
nweiz 2013/09/18 22:40:54 This name sounds like a variable, not a function.
+ node is IncludeDirective || node is MixinDefinition || node is NoOp;
+
+ void visitStyleSheet(StyleSheet ss) {
+ var index = ss.topLevels.length;
+ while (--index >= 0) {
nweiz 2013/09/18 22:40:54 It's never clearer to use "--variable" in an expre
+ if (_nodesToRemove(ss.topLevels[index])) {
+ ss.topLevels.removeAt(index);
+ }
+ }
+
+ super.visitStyleSheet(ss);
+ }
+
+ void visitDeclarationGroup(DeclarationGroup node) {
+ var index = node.declarations.length;
+ while (--index >= 0) {
+ if (_nodesToRemove(node.declarations[index])) {
+ node.declarations.removeAt(index);
+ }
+ }
+
+ super.visitDeclarationGroup(node);
+ }
+}

Powered by Google App Engine
This is Rietveld 408576698