Fix issue http://code.google.com/p/dart/issues/detail?id=1877 without degrading performance. Impro…

lib/dom/templates/html/impl/impl_Element.darttemplate

Index: lib/dom/templates/html/impl/impl_Element.darttemplate
diff --git a/lib/dom/templates/html/impl/impl_Element.darttemplate b/lib/dom/templates/html/impl/impl_Element.darttemplate
index f7542b2b8a0356aefdfe1a3ddcebec4e97b31067..609ec0b54b719fd7b7bfd449f65ee2a33cb5a5da 100644
--- a/lib/dom/templates/html/impl/impl_Element.darttemplate
+++ b/lib/dom/templates/html/impl/impl_Element.darttemplate
@@ -305,11 +305,11 @@ class _ElementList extends _ListWrapper<Element> implements ElementList {
     new _ElementList(super.getRange(start, length));
 }
 
-class ElementAttributeMap implements Map<String, String> {
+class _ElementAttributeMap implements AttributeMap {
 
   final _ElementImpl _element;
 
-  ElementAttributeMap._wrap(this._element);
+  _ElementAttributeMap(this._element);
 
   bool containsValue(String value) {
     final attributes = _element.$dom_attributes;
@@ -329,8 +329,8 @@ class ElementAttributeMap implements Map<String, String> {
     return _element.$dom_getAttribute(key);
   }
 
-  void operator []=(String key, String value) {
-    _element.$dom_setAttribute(key, value);
+  void operator []=(String key, value) {
+    _element.$dom_setAttribute(key, '$value');
   }
 
   String putIfAbsent(String key, String ifAbsent()) {
@@ -393,6 +393,226 @@ class ElementAttributeMap implements Map<String, String> {
   }
 }
 
+/**
+ * Provides a Map abstraction on top of data-* attributes, similar to the
+ * dataSet in the old DOM.
+ */
+class _DataAttributeMap implements AttributeMap {
+
+  final Map<String, String> $dom_attributes;
+
+  _DataAttributeMap(this.$dom_attributes);
+
+  // interface Map
+
+  // TODO: Use lazy iterator when it is available on Map.
+  bool containsValue(String value) => getValues().some((v) => v == value);
+
+  bool containsKey(String key) => $dom_attributes.containsKey(_attr(key));
+
+  String operator [](String key) => $dom_attributes[_attr(key)];
+
+  void operator []=(String key, value) {
+    $dom_attributes[_attr(key)] = '$value';
+  }
+
+  String putIfAbsent(String key, String ifAbsent()) {
+    if (!containsKey(key)) {

[nweiz, 2012/03/30 00:06:23]

Can't this just call $dom_attributes.putIfAbsent?

[Jacob, 2012/03/30 00:40:53]

Yep. Done. FYI this code isn't new I just moved it because it was in the wrong
file before.

+      return this[key] = ifAbsent();
+    }
+    return this[key];
+  }
+
+  String remove(String key) => $dom_attributes.remove(_attr(key));
+
+  void clear() {
+    // Needs to operate on a snapshot since we are mutatiting the collection.

[nweiz, 2012/03/30 00:06:23]

mutating

[Jacob, 2012/03/30 00:40:53]

Done.

+    for (String key in getKeys()) {
+      remove(key);
+    }
+  }
+
+  void forEach(void f(String key, String value)) {
+    $dom_attributes.forEach((String key, String value) {
+      if (_matches(key)) {
+        f(_strip(key), value);
+      }
+    });
+  }
+
+  Collection<String> getKeys() {
+    final keys = new List<String>();
+    $dom_attributes.forEach((String key, String value) {

[nweiz, 2012/03/30 00:06:23]

This would be a little cleaner if you used this class's forEach, or
$dom_attributes.getKeys().filter(...)

[Jacob, 2012/03/30 00:40:53]

Reasonable idea but I'd rather leave it unchanged. This code didn't really
change with this CL it just moved files. If you want to tweak it to use a more
functional style you can submit a separate CL for that.  The alternate style
would probably be a bit slower but I'm alright with it as it would also be
likely have smaller code size.
I suspect there is a significantly simpler solution for all of this on modern
browsers that support the .data property directly.

+      if (_matches(key)) {
+        keys.add(_strip(key));
+      }
+    });
+    return keys;
+  }
+
+  Collection<String> getValues() {
+    final values = new List<String>();
+    $dom_attributes.forEach((String key, String value) {

[nweiz, 2012/03/30 00:06:23]

As above, a little cleaner to use this class's forEach.

[Jacob, 2012/03/30 00:40:53]

feel free to send an alternate CL.

+      if (_matches(key)) {
+        values.add(value);
+      }
+    });
+    return values;
+  }
+
+  int get length() => getKeys().length;
+
+  // TODO: Use lazy iterator when it is available on Map.
+  bool isEmpty() => length == 0;
+
+  // Helpers.
+  String _attr(String key) => 'data-$key';
+  bool _matches(String key) => key.startsWith('data-');
+  String _strip(String key) => key.substring(5);
+}
+
+class _CssClassSet implements Set<String> {
+
+  final _ElementImpl _element;
+
+  _CssClassSet(this._element);
+
+  String toString() {
+    return _formatSet(_read());

[nweiz, 2012/03/30 00:06:23]

=>, here and below.

[Jacob, 2012/03/30 00:40:53]

Done.

+  }
+
+  // interface Iterable - BEGIN
+  Iterator<String> iterator() {
+    return _read().iterator();
+  }
+  // interface Iterable - END
+
+  // interface Collection - BEGIN
+  void forEach(void f(String element)) {
+    _read().forEach(f);
+  }
+
+  Collection map(f(String element)) {
+    return _read().map(f);
+  }
+
+  Collection<String> filter(bool f(String element)) {
+    return _read().filter(f);
+  }
+
+  bool every(bool f(String element)) {
+    return _read().every(f);
+  }
+
+  bool some(bool f(String element)) {
+    return _read().some(f);
+  }
+
+  bool isEmpty() {
+    return _read().isEmpty();
+  }
+
+  int get length() {
+    return _read().length;
+  }
+  // interface Collection - END
+
+  // interface Set - BEGIN
+  bool contains(String value) {
+    return _read().contains(value);
+  }
+
+  void add(String value) {
+    // TODO - figure out if we need to do any validation here
+    // or if the browser natively does enough
+    _modify((s) => s.add(value));
+  }
+
+  bool remove(String value) {
+    Set<String> s = _read();
+    bool result = s.remove(value);

[nweiz, 2012/03/30 00:06:23]

Couldn't you use _modify here if you changed it to pass through the return value
of the function?

[Jacob, 2012/03/30 00:40:53]

i don't follow. if you want to send a cleanup cl feel free. once again i didn't
modify this code or write it in the first place. it was just moved to the
correct file.

+    _write(s);
+    return result;
+  }
+
+  void addAll(Collection<String> collection) {
+    // TODO - see comment above about validation
+    _modify((s) => s.addAll(collection));
+  }
+
+  void removeAll(Collection<String> collection) {
+    _modify((s) => s.removeAll(collection));
+  }
+
+  bool isSubsetOf(Collection<String> collection) {
+    return _read().isSubsetOf(collection);
+  }
+
+  bool containsAll(Collection<String> collection) {
+    return _read().containsAll(collection);
+  }
+
+  Set<String> intersection(Collection<String> other) {
+    return _read().intersection(other);
+  }
+
+  void clear() {
+    _modify((s) => s.clear());
+  }
+  // interface Set - END
+
+  /**
+   * Helper method used to modify the set of css classes on this element.
+   *
+   *   f - callback with:
+   *      s - a Set of all the css class name currently on this element.
+   *
+   *   After f returns, the modified set is written to the
+   *       className property of this element.
+   */
+  void _modify( f(Set<String> s)) {
+    Set<String> s = _read();
+    f(s);
+    _write(s);
+  }
+
+  /**
+   * Read the class names from the Element class property,
+   * and put them into a set (duplicates are discarded).
+   */
+  Set<String> _read() {
+    // TODO(mattsh) simplify this once split can take regex.
+    Set<String> s = new Set<String>();
+    for (String name in $dom_className().split(' ')) {
+      String trimmed = name.trim();
+      if (!trimmed.isEmpty()) {
+        s.add(trimmed);
+      }
+    }
+    return s;
+  }
+
+  /**
+   * Read the class names as a space-separated string. This is meant to be
+   * overridden by subclasses.
+   */
+  String $dom_className() => _element.$dom_className;

[nweiz, 2012/03/30 00:06:23]

Why does this need to be implemented here?

[Jacob, 2012/03/30 00:40:53]

Good catch... my regexp adding $dom to the names of things went a bit too far
and nobody noticed when they reviewed my last CL.

+
+  /**
+   * Join all the elements of a set into one string and write
+   * back to the element.
+   */
+  void _write(Set s) {
+    _element.$dom_className = _formatSet(s);
+  }
+
+  String _formatSet(Set<String> s) {
+    // TODO(mattsh) should be able to pass Set to String.joins http:/b/5398605
+    List list = new List.from(s);
+    return Strings.join(list, ' ');
+  }
+}
+
 class _SimpleClientRect implements ClientRect {
   final num left;
   final num top;
@@ -458,21 +678,11 @@ class _ElementRectImpl implements ElementRect {
 
 class $CLASSNAME$EXTENDS$IMPLEMENTS$NATIVESPEC {
 
-  // TODO(jacobr): caching these may hurt performance.
-  ElementAttributeMap _elementAttributeMap;
-  _CssClassSet _cssClassSet;
-  _DataAttributeMap _dataAttributes;
-
   /**
    * @domName Element.hasAttribute, Element.getAttribute, Element.setAttribute,
    *   Element.removeAttribute
    */
-  Map<String, String> get attributes() {
-    if (_elementAttributeMap === null) {
-      _elementAttributeMap = new ElementAttributeMap._wrap(this);
-    }
-    return _elementAttributeMap;
-  }
+  _ElementAttributeMap get attributes() => new _ElementAttributeMap(this);
 
   void set attributes(Map<String, String> value) {
     Map<String, String> attributes = this.attributes;
@@ -493,12 +703,7 @@ class $CLASSNAME$EXTENDS$IMPLEMENTS$NATIVESPEC {
   ElementList queryAll(String selectors) =>
     new _FrozenElementList._wrap($dom_querySelectorAll(selectors));
 
-  Set<String> get classes() {
-    if (_cssClassSet === null) {
-      _cssClassSet = new _CssClassSet(this);
-    }
-    return _cssClassSet;
-  }
+  _CssClassSet get classes() => new _CssClassSet(this);
 
   void set classes(Collection<String> value) {
     _CssClassSet classSet = classes;
@@ -507,14 +712,11 @@ class $CLASSNAME$EXTENDS$IMPLEMENTS$NATIVESPEC {
   }
 
   Map<String, String> get dataAttributes() {
-    if (_dataAttributes === null) {
-      _dataAttributes = new _DataAttributeMap(attributes);
-    }
-    return _dataAttributes;
+    return new _DataAttributeMap(attributes);

[nweiz, 2012/03/30 00:06:23]

=>

[Jacob, 2012/03/30 00:40:53]

Done.

   }
 
   void set dataAttributes(Map<String, String> value) {
-    Map<String, String> dataAttributes = this.dataAttributes;
+    final dataAttributes = this.dataAttributes;
     dataAttributes.clear();
     for (String key in value.getKeys()) {
       dataAttributes[key] = value[key];