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

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

 // Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 // TODO(jacobr): use _Lists.dart to remove some of the duplicated
 // functionality.
 class _ChildrenElementList implements ElementList {
   // Raw Element.
   final _ElementImpl _element;
   final _HTMLCollectionImpl _childElements;
@@ skipping 287 matching lines @@
 class _ElementList extends _ListWrapper<Element> implements ElementList {
   _ElementList(List<Element> list) : super(list);
 
   ElementList filter(bool f(Element element)) =>
     new _ElementList(super.filter(f));
 
   ElementList getRange(int start, int length) =>
     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;
     for (int i = 0, len = attributes.length; i < len; i++) {
       if(value == attributes[i].value) {
         return true;
       }
     }
     return false;
   }
 
   bool containsKey(String key) {
     return _element.$dom_hasAttribute(key);
   }
 
   String operator [](String key) {
     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()) {
     if (!containsKey(key)) {
       this[key] = ifAbsent();
     }
   }
 
   String remove(String key) {
     _element.$dom_removeAttribute(key);
@@ skipping 42 matching lines @@
   }
 
   /**
    * Returns true if there is no {key, value} pair in the map.
    */
   bool isEmpty() {
     return length == 0;
   }
 }
 
+/**
+ * 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;
   final num width;
   final num height;
   num get right() => left + width;
   num get bottom() => top + height;
 
   const _SimpleClientRect(this.left, this.top, this.width, this.height);
 
@@ skipping 45 matching lines @@
     final out = new List(_clientRects.length);
     for (num i = 0; i < _clientRects.length; i++) {
       out[i] = _clientRects.item(i);
     }
     return out;
   }
 }
 
 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;
     attributes.clear();
     for (String key in value.getKeys()) {
       attributes[key] = value[key];
     }
   }
 
   void set elements(Collection<Element> value) {
     final elements = this.elements;
     elements.clear();
     elements.addAll(value);
   }
 
   ElementList get elements() => new _ChildrenElementList._wrap(this);
 
   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;
     classSet.clear();
     classSet.addAll(value);
   }
 
   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];
     }
   }
 
   Future<ElementRect> get rect() {
     return _createMeasurementFuture(
         () => new _ElementRectImpl(this),
         new Completer<ElementRect>());
@@ skipping 66 matching lines @@
 
   /** @domName Document.createElement */
 $if FROG
   // Optimization to improve performance until the frog compiler inlines this
   // method.
   factory Element.tag(String tag) native "return document.createElement(tag)";
 $else
   factory Element.tag(String tag) => _document.$dom_createElement(tag);
 $endif
 }