560112 - (dataset) Implement HTML5 dataset attribute

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[karl155]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a5pre) Gecko/20100415 Minefield/3.7a5pre

I think the html5 dataset attribute is an important thing for integrating metadata into html documents.

Here is the spec (Status: Last call for comments):
http://dev.w3.org/html5/spec/Overview.html#attr-data
http://dev.w3.org/html5/spec/Overview.html#dom-dataset

And here is a testcase (I'm not the owner of that code)
Test with html5 data but without dataset (also working older browsers):
http://www.1729.com/examples/htmlannotation/Html5AnnotationUsingDataAttributes.html
Test with html5 data and dataset (not working in Firefox/Minefield)
http://www.1729.com/examples/htmlannotation/Html5AnnotationUsingDataset.html

(I know that html5 isn't finished yet. But because Firefox is already implementing other pieces of the spec I think it would be nice if this could be implemented too.)

Reproducible: Always

Comment 1

[Paul Rouget [:paul]]

Fixed in Webkit:
https://bugs.webkit.org/show_bug.cgi?id=41146

Comment 2

[Paul Rouget [:paul]]

Test from webkit: http://trac.webkit.org/export/66582/trunk/LayoutTests/fast/dom/dataset.html

Comment 3

[Simon]

Is there a road map for this feature?

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to comment #3)
> Is there a road map for this feature?

"Patches welcome". This bug will be fixed once somebody steps up to do the work. It seems, however, to be quite a bit of work for a rather small feature, and would probably end up pretty low on the (already full) to-do lists of the usual suspects.

Comment 5

[William Chen [:wchen]]

Attachment: Patch for implementation of dataset.

Here is a patch that implements dataset as defined in the spec right now. Comments would be appreciated. Thanks.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 531516 [details] [diff] [review]
Patch for implementation of dataset.


>+++ b/content/html/content/test/Makefile.in
>@@ -258,12 +258,13 @@ _TEST_FILES = \
> 		test_bug636336.html \
> 		test_bug630889.html \
> 		test_bug610212.html \
> 		test_bug633058.html \
> 		test_bug641219.html \
> 		test_bug643051.html \
> 		test_bug583514.html \
> 		test_bug514437.html \
>+		test_bug560112.html \
Looks like the actual testfile is missing from the patch.


> [scriptable, uuid(4738f75d-9c6f-40f8-81d0-84b2e4726a8f)]
> interface nsIDOMNSHTMLElement : nsISupports
You should update uuid when changing interfaces.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

I wonder if the patch is slow-ish, mainly because StringMap isn't
quickstubbed at all.

Comment 8

[Mounir Lamouri (:mounir)]

Comment on attachment 531516 [details] [diff] [review]
Patch for implementation of dataset.

>diff --git a/content/html/content/src/nsDOMStringMap.cpp b/content/html/content/src/nsDOMStringMap.cpp
>+nsDOMStringMap::nsDOMStringMap(nsGenericHTMLElement* aElement)
>+{
>+  mElement = aElement;
>+}

Please put mElement in the initialization list this way:
nsDOMStringMap::nsDOMStringMap(nsGenericHTMLElement* aElement)
  :mElement(aElement)
{
}

>+/* DOMString getDataAttr (in DOMString attr); */
>+NS_IMETHODIMP nsDOMStringMap::GetDataAttr(const nsAString& attr, nsAString& _retval NS_OUTPARAM)
>+{
>+  nsString normAttr;

You should use nsAutoString.
See: https://developer.mozilla.org/En/Mozilla_internal_string_guide

>+  nsresult rv = DataPropToAttr(attr, normAttr);
>+
>+  if (NS_FAILED(rv)) {
>+    _retval.Truncate();
>+    return NS_OK;
>+  }

Why not NS_ENSURE_SUCCESS? I don't think you should worry about .Truncate() but I might be wrong.

>+  rv = mElement->GetAttribute(normAttr, _retval);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  return NS_OK;
>+}

You can do |return rv;| instead of NS_ENSURE_SUCCESS and return NS_OK.

>+/* void setDataAttr (in DOMString attr, in DOMString value); */
>+NS_IMETHODIMP nsDOMStringMap::SetDataAttr(const nsAString& attr, const nsAString& value)
>+{
>+  nsString normAttr;

nsAutoString.

>+  nsresult rv = DataPropToAttr(attr, normAttr);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  rv = mElement->SetAttribute(normAttr, value);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  return NS_OK;
>+}

return rv;

>+/* void removeDataAttr (in DOMString attr); */
>+NS_IMETHODIMP nsDOMStringMap::RemoveDataAttr(const nsAString& attr)
>+{
>+  nsString normAttr;

nsAutoString

>+  nsresult rv = DataPropToAttr(attr, normAttr);
>+
>+  if (NS_FAILED(rv)) {
>+    return NS_OK;
>+  }

NS_ENSURE_SUCCESS?

>+
>+  mElement->RemoveAttribute(normAttr);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  return NS_OK;
>+}

return rv;

>+nsresult nsDOMStringMap::DataPropToAttr(const nsAString& aProp, nsAString& _retval)
>+{
>+  const PRUnichar* cur = aProp.BeginReading();
>+  const PRUnichar* end = aProp.EndReading();
>+
>+  nsAutoString attr;
>+  // Set the capacity to 5 ("data-") + double aProp length ("A" -> "-a").
>+  PRBool capacitySet = attr.SetCapacity(5 + aProp.Length() * 2);
>+  NS_ENSURE_TRUE(capacitySet, NS_ERROR_OUT_OF_MEMORY);
>+  attr = NS_LITERAL_STRING("data-");
>+
>+  for (; cur < end; ++cur) {
>+    if (PRUnichar('-') == *cur && cur < end - 1) {
>+      // Syntax error if character following "-" is in range "a" to "z".
>+      const PRUnichar* next = cur + 1;
>+      if (PRUnichar('a') <= *next && *next <= PRUnichar('z')) {
>+        return NS_ERROR_DOM_SYNTAX_ERR;
>+      }
>+    }
>+
>+    if (PRUnichar('A') <= *cur && *cur <= PRUnichar('Z')) {
>+      // Uncamel-case characters in the range of "A" to "Z".
>+      attr.Append(PRUnichar('-'));
>+      attr.Append(PRUnichar(NS_ToLower(*cur)));
>+    } else {
>+      attr.Append(*cur);
>+    }
>+  }
>+
>+  _retval.Assign(attr);
>+  return NS_OK;
>+}

A comment at the beginning of the function trying to explaining the algorithm would be nice ;)

>diff --git a/content/html/content/test/Makefile.in b/content/html/content/test/Makefile.in
>--- a/content/html/content/test/Makefile.in
>+++ b/content/html/content/test/Makefile.in
>@@ -258,12 +258,13 @@ _TEST_FILES = \
> 		test_bug636336.html \
> 		test_bug630889.html \
> 		test_bug610212.html \
> 		test_bug633058.html \
> 		test_bug641219.html \
> 		test_bug643051.html \
> 		test_bug583514.html \
> 		test_bug514437.html \
>+		test_bug560112.html \

As said Olli, the test is missing.

>diff --git a/dom/interfaces/html/nsIDOMNSHTMLElement.idl b/dom/interfaces/html/nsIDOMNSHTMLElement.idl

>-  readonly attribute long             offsetTop;
>-  readonly attribute long             offsetLeft;
>-  readonly attribute long             offsetWidth;
>-  readonly attribute long             offsetHeight;
>-  readonly attribute nsIDOMElement    offsetParent;
>-           attribute DOMString        innerHTML;
>+  readonly attribute long               offsetTop;
>+  readonly attribute long               offsetLeft;
>+  readonly attribute long               offsetWidth;
>+  readonly attribute long               offsetHeight;
>+  readonly attribute nsIDOMElement      offsetParent;
>+           attribute DOMString          innerHTML;
> 
>   /**
>    * Indicates that the element is not yet, or is no longer, relevant.
>    *
>    * See <http://www.whatwg.org/html5/#the-hidden-attribute>.
>    */
>-           attribute boolean          hidden;
>+           attribute boolean            hidden;
> 
>-           attribute long             tabIndex;
>+           attribute long               tabIndex;
> 
>-           attribute DOMString        contentEditable;
>-  readonly attribute boolean          isContentEditable;
>+           attribute DOMString          contentEditable;
>+  readonly attribute boolean            isContentEditable;
> 
>            // for WHAT-WG drag and drop
>-           attribute boolean          draggable;
>+           attribute boolean            draggable;

I don't think it's worth moving all those lines and breaking the blame.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

> >+  nsresult rv = DataPropToAttr(attr, normAttr);
> >+
> >+  if (NS_FAILED(rv)) {
> >+    _retval.Truncate();
> >+    return NS_OK;
> >+  }
> 
> Why not NS_ENSURE_SUCCESS? I don't think you should worry about .Truncate()
> but I might be wrong.

Generally what we do is to put a .Truncate() call at the top, and then use NS_ENSURE_SUCCESS(rv, rv) or NS_ENSURE_SUCCESS(rv, NS_OK)

You definitely do need the .Truncate() call if you're returning NS_OK;

> >+  rv = mElement->GetAttribute(normAttr, _retval);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+  return NS_OK;
> >+}
> 
> You can do |return rv;| instead of NS_ENSURE_SUCCESS and return NS_OK.

Noooo.. Always try to avoid ending functions with |return rv;|. It makes it very easy to accidentally return something you don't intend to if someone inserts some code that uses rv before the return.

You can do |return mElement->GetAttribute(normAttr, _retval);| though.

> >+  nsresult rv = DataPropToAttr(attr, normAttr);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+  rv = mElement->SetAttribute(normAttr, value);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+  return NS_OK;
> >+}
> 
> return rv;

Again: Nooooo...

> >+
> >+  mElement->RemoveAttribute(normAttr);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+  return NS_OK;
> >+}
> 
> return rv;

I'm just gonna stop here.

Comment 10

[William Chen [:wchen]]

Attachment: revised patch for implementation of dataset

I have updated the patch to address all the comments except for the following:

moving lines for alignment: I have left it as aligned because I don't want to make the code messy. However, if this is really a big problem with hg then I will change it back.

quickstubbed: I'm not sure what this is but I will look into it. I'm leaving it the way it is now because it works and I'm guessing that adding support for quickstubbed is not going to be trivial.

I have also added more code to implement enumeration.

Comment 11

[William Chen [:wchen]]

Attachment: dataset implementation

I forgot to click the "patch" check box while uploading my last patch.

Comment 12

[Lars Gunther]

Should also be parity Opera?

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I don't think we can quickstub since this object uses all named getters and setters.

The solution we're going for elsewhere is to use proxies. bz is working on a patch to do that for nodelists, but it's still being developed so I don't want to block on that.

Once we have a proxy solution we should use that here, for localStorage, for HTMLCollections, for the global object, and likely elsewhere.

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 531873 [details] [diff] [review]
dataset implementation

>diff --git a/content/html/content/src/nsDOMStringMap.cpp b/content/html/content/src/nsDOMStringMap.cpp
>new file mode 100644
>--- /dev/null
>+++ b/content/html/content/src/nsDOMStringMap.cpp
>+nsDOMStringMap::nsDOMStringMap(nsGenericHTMLElement* aElement)
>+  :mElement(aElement)

a space after the colon, please.

>+/**
>+ * Returns a list of dataset properties corresponding to the data
>+ * attributes on the element.
>+ */
>+nsresult nsDOMStringMap::GetDataPropList(nsTArray<nsString>& _retval)
>+{

Is nsIDOMNode::attributes really the best API we've got here?

>+  nsAutoString dataPrefix; dataPrefix = NS_LITERAL_STRING("data-");

NS_NAMED_LITERAL_STRING (not sure about the exact spelling)

>+  // Iterate through all the attributes and add property names corresponding to
>+  // data attributes to return array.
>+  for (PRUint32 i = 0; i < length; ++i) {

>+    // Ensure capacity of return array is large enough to store all the property
>+    // names.
>+    capacitySet = _retval.SetCapacity(length);
>+    NS_ENSURE_TRUE(capacitySet, NS_ERROR_OUT_OF_MEMORY);

Huh? What is this doing in the middle of the loop?

>+    // Iterate through attrName by character to form property name.
>+    // If there is a sequence of "-" followed by a character in the range "a" to
>+    // "z" then replace with upper case letter.
>+    // Otherwise append character to property name.
>+    for (; cur < end; ++cur) {
>+      if (PRUnichar('-') == *cur && cur < end - 1) {
>+        const PRUnichar* next = cur + 1;
>+        if (PRUnichar('a') <= *next && *next <= PRUnichar('z')) {
>+          // Camel case lower case letters that follow a "-".
>+          prop.Append(PRUnichar(NS_ToUpper(*next)));

You can do Append(*next + 'A' - 'a')

>+          // Consume character to account for "-" character.
>+          ++cur;
>+          // Move on to next character.
>+          continue;
>+        }
>+      }
>+      
>+      // Simply append character if camel case is not necessary.
>+      prop.Append(*cur);

The flow here doesn't feel as good as I'd like. What do you think about

for {
  const PRUnichar* next = cur + 1;
  if (PRUnichar('-') == *cur && next < end &&
      PRUnichar('a') <= *next && *next <= PRUnichar('z')) {
    // Do the dance
  } else {
    prop.Append(*cur);
  }
}
>+nsresult nsDOMStringMap::DataPropToAttr(const nsAString& aProp, nsAString& _retval)
>+{
>+  // Set the capacity to 5 ("data-") + double aProp length ("A" -> "-a").
>+  PRBool capacitySet = attr.SetCapacity(5 + aProp.Length() * 2);

That's the worst case... I don't think it's necessary to reserve that much.

>+  // Iterate property by character to form attribute name.
>+  // Return syntax error if there is a sequence of "-" followed by a character
>+  // in the range "a" to "z".
>+  // Replace capital characters with "-" followed by lower case character.
>+  // Otherwise, simply append character to attribute name.
>+  for (; cur < end; ++cur) {
>+    if (PRUnichar('-') == *cur && cur < end - 1) {
>+      // Syntax error if character following "-" is in range "a" to "z".
>+      const PRUnichar* next = cur + 1;
>+      if (PRUnichar('a') <= *next && *next <= PRUnichar('z')) {
>+        return NS_ERROR_DOM_SYNTAX_ERR;
>+      }
>+    }

(Similar as above, but somehow bothers me less.)

>+    if (PRUnichar('A') <= *cur && *cur <= PRUnichar('Z')) {
>+      // Uncamel-case characters in the range of "A" to "Z".
>+      attr.Append(PRUnichar('-'));
>+      attr.Append(PRUnichar(NS_ToLower(*cur)));

Same comment as above.

>diff --git a/content/html/content/src/nsDOMStringMap.h b/content/html/content/src/nsDOMStringMap.h
>new file mode 100644
>--- /dev/null
>+++ b/content/html/content/src/nsDOMStringMap.h

I'd rather see this be called mozilla::dom::DOMStringMap.

>+#ifndef nsDOMStringMap_h__

And then that would be mozilla_dom_DOMStringMap_h. Lose the trailing underscores in any case.

>+class nsDOMStringMap : public nsIDOMDOMStringMap
>+{
>+public:
>+  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
>+  NS_DECL_NSIDOMDOMSTRINGMAP
>+  NS_DECL_CYCLE_COLLECTION_CLASS(nsDOMStringMap)
>+
>+  nsDOMStringMap(nsGenericHTMLElement* aElement);
>+
>+  // GetDataPropList is not defined in IDL due to difficulty
>+  // of returning arrays in IDL. Instead, we cast to this
>+  // concrete base class if this method needs to be called.
>+  nsresult GetDataPropList(nsTArray<nsString>& _retval);

Can't you resolve that with [noxpcom]? The templated class may be a problem...

If you can't resolve it, I'm not sure if it's useful to have the others on
nsIDOMDOMStringMap... Maybe have a static DOMStringMap::FromInterface(nsIDOMDOMStringMap*) to hide the casts. (Name up for bikeshedding.)

>+  nsCOMPtr<nsGenericHTMLElement> mElement;

No! nsCOMPtr should only be used with interfaces. You want nsRefPtr.

Actually, do you need a strong pointer?

>diff --git a/content/html/content/src/nsGenericHTMLElement.h b/content/html/content/src/nsGenericHTMLElement.h
>--- a/content/html/content/src/nsGenericHTMLElement.h
>+++ b/content/html/content/src/nsGenericHTMLElement.h

>+#include "nsDOMStringMap.h"

I doubt you need this. Move it to the .cpp?

>diff --git a/dom/base/nsDOMClassInfo.cpp b/dom/base/nsDOMClassInfo.cpp
>--- a/dom/base/nsDOMClassInfo.cpp
>+++ b/dom/base/nsDOMClassInfo.cpp

>+NS_IMETHODIMP
>+nsDOMStringMapSH::Enumerate(nsIXPConnectWrappedNative *wrapper, JSContext *cx,
>+                            JSObject *obj, PRBool *_retval)
>+{

>+  for (unsigned int i = 0; i < attributes.Length(); ++i) {

PRUint32

>+NS_IMETHODIMP
>+nsDOMStringMapSH::GetProperty(nsIXPConnectWrappedNative *wrapper, JSContext *cx,
>+                              JSObject *obj, jsid id, jsval *vp,
>+                              PRBool *_retval)
>+{

>+  JSString *attrValStr = ::JS_NewUCStringCopyN(cx, reinterpret_cast<const jschar*>(
>+                                               attrVal.get()), attrVal.Length());

As usual, seeing this makes me sad.

>diff --git a/dom/interfaces/html/nsIDOMNSHTMLElement.idl b/dom/interfaces/html/nsIDOMNSHTMLElement.idl
>--- a/dom/interfaces/html/nsIDOMNSHTMLElement.idl
>+++ b/dom/interfaces/html/nsIDOMNSHTMLElement.idl

As said, please don't reindent. The non-perfect alignment is fine.


Jonas, I think you wanted this to happen?

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

And thanks a lot for working on this, William!

Comment 16

[William Chen [:wchen]]

(In reply to comment #14)
> Comment on attachment 531873 [details] [diff] [review] [review]
> dataset implementation
> 
> >diff --git a/content/html/content/src/nsDOMStringMap.cpp b/content/html/content/src/nsDOMStringMap.cpp
> >new file mode 100644
> >--- /dev/null
> >+++ b/content/html/content/src/nsDOMStringMap.cpp
> >+nsDOMStringMap::nsDOMStringMap(nsGenericHTMLElement* aElement)
> >+  :mElement(aElement)
> 
> a space after the colon, please.
done
> 
> >+/**
> >+ * Returns a list of dataset properties corresponding to the data
> >+ * attributes on the element.
> >+ */
> >+nsresult nsDOMStringMap::GetDataPropList(nsTArray<nsString>& _retval)
> >+{
> 
> Is nsIDOMNode::attributes really the best API we've got here?
> 
> >+  nsAutoString dataPrefix; dataPrefix = NS_LITERAL_STRING("data-");
> 
> NS_NAMED_LITERAL_STRING (not sure about the exact spelling)
> 
> >+  // Iterate through all the attributes and add property names corresponding to
> >+  // data attributes to return array.
> >+  for (PRUint32 i = 0; i < length; ++i) {
> 
> >+    // Ensure capacity of return array is large enough to store all the property
> >+    // names.
> >+    capacitySet = _retval.SetCapacity(length);
> >+    NS_ENSURE_TRUE(capacitySet, NS_ERROR_OUT_OF_MEMORY);
> 
> Huh? What is this doing in the middle of the loop?
I have moved it out of the loop.
> 
> >+    // Iterate through attrName by character to form property name.
> >+    // If there is a sequence of "-" followed by a character in the range "a" to
> >+    // "z" then replace with upper case letter.
> >+    // Otherwise append character to property name.
> >+    for (; cur < end; ++cur) {
> >+      if (PRUnichar('-') == *cur && cur < end - 1) {
> >+        const PRUnichar* next = cur + 1;
> >+        if (PRUnichar('a') <= *next && *next <= PRUnichar('z')) {
> >+          // Camel case lower case letters that follow a "-".
> >+          prop.Append(PRUnichar(NS_ToUpper(*next)));
> 
> You can do Append(*next + 'A' - 'a')
I left this as I had it since I think it's more readable. If you truly believe that this is a worthwhile performance gain then I don't mind doing it that way.
> 
> >+          // Consume character to account for "-" character.
> >+          ++cur;
> >+          // Move on to next character.
> >+          continue;
> >+        }
> >+      }
> >+      
> >+      // Simply append character if camel case is not necessary.
> >+      prop.Append(*cur);
> 
> The flow here doesn't feel as good as I'd like. What do you think about
> 
> for {
>   const PRUnichar* next = cur + 1;
>   if (PRUnichar('-') == *cur && next < end &&
>       PRUnichar('a') <= *next && *next <= PRUnichar('z')) {
>     // Do the dance
>   } else {
>     prop.Append(*cur);
>   }
> }
Yeah, I think that is better.
> >+nsresult nsDOMStringMap::DataPropToAttr(const nsAString& aProp, nsAString& _retval)
> >+{
> >+  // Set the capacity to 5 ("data-") + double aProp length ("A" -> "-a").
> >+  PRBool capacitySet = attr.SetCapacity(5 + aProp.Length() * 2);
> 
> That's the worst case... I don't think it's necessary to reserve that much.
It saves us from checking and adjusting capacity while building the string.
> 
> >+  // Iterate property by character to form attribute name.
> >+  // Return syntax error if there is a sequence of "-" followed by a character
> >+  // in the range "a" to "z".
> >+  // Replace capital characters with "-" followed by lower case character.
> >+  // Otherwise, simply append character to attribute name.
> >+  for (; cur < end; ++cur) {
> >+    if (PRUnichar('-') == *cur && cur < end - 1) {
> >+      // Syntax error if character following "-" is in range "a" to "z".
> >+      const PRUnichar* next = cur + 1;
> >+      if (PRUnichar('a') <= *next && *next <= PRUnichar('z')) {
> >+        return NS_ERROR_DOM_SYNTAX_ERR;
> >+      }
> >+    }
> 
> (Similar as above, but somehow bothers me less.)
Fixed.
> 
> >+    if (PRUnichar('A') <= *cur && *cur <= PRUnichar('Z')) {
> >+      // Uncamel-case characters in the range of "A" to "Z".
> >+      attr.Append(PRUnichar('-'));
> >+      attr.Append(PRUnichar(NS_ToLower(*cur)));
> 
> Same comment as above.
> 
> >diff --git a/content/html/content/src/nsDOMStringMap.h b/content/html/content/src/nsDOMStringMap.h
> >new file mode 100644
> >--- /dev/null
> >+++ b/content/html/content/src/nsDOMStringMap.h
> 
> I'd rather see this be called mozilla::dom::DOMStringMap.
I left it as is, however if someone has a strong opinion about a moving this then I can do it.
> 
> >+#ifndef nsDOMStringMap_h__
> 
> And then that would be mozilla_dom_DOMStringMap_h. Lose the trailing
> underscores in any case.
done
> 
> >+class nsDOMStringMap : public nsIDOMDOMStringMap
> >+{
> >+public:
> >+  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> >+  NS_DECL_NSIDOMDOMSTRINGMAP
> >+  NS_DECL_CYCLE_COLLECTION_CLASS(nsDOMStringMap)
> >+
> >+  nsDOMStringMap(nsGenericHTMLElement* aElement);
> >+
> >+  // GetDataPropList is not defined in IDL due to difficulty
> >+  // of returning arrays in IDL. Instead, we cast to this
> >+  // concrete base class if this method needs to be called.
> >+  nsresult GetDataPropList(nsTArray<nsString>& _retval);
> 
> Can't you resolve that with [noxpcom]? The templated class may be a
> problem...
> 
> If you can't resolve it, I'm not sure if it's useful to have the others on
> nsIDOMDOMStringMap... Maybe have a static
> DOMStringMap::FromInterface(nsIDOMDOMStringMap*) to hide the casts. (Name up
> for bikeshedding.)
I left this as is since after discussing with others, this is the what we ended up with.
> 
> >+  nsCOMPtr<nsGenericHTMLElement> mElement;
> 
> No! nsCOMPtr should only be used with interfaces. You want nsRefPtr.
done.
> 
> Actually, do you need a strong pointer?
Yes, we need a strong pointer from the dataset to the element. However, a strong reference from the element to the dataset is non necessary. It has been updated in the new patch.
> 
> >diff --git a/content/html/content/src/nsGenericHTMLElement.h b/content/html/content/src/nsGenericHTMLElement.h
> >--- a/content/html/content/src/nsGenericHTMLElement.h
> >+++ b/content/html/content/src/nsGenericHTMLElement.h
> 
> >+#include "nsDOMStringMap.h"
> 
> I doubt you need this. Move it to the .cpp?
done
> 
> >diff --git a/dom/base/nsDOMClassInfo.cpp b/dom/base/nsDOMClassInfo.cpp
> >--- a/dom/base/nsDOMClassInfo.cpp
> >+++ b/dom/base/nsDOMClassInfo.cpp
> 
> >+NS_IMETHODIMP
> >+nsDOMStringMapSH::Enumerate(nsIXPConnectWrappedNative *wrapper, JSContext *cx,
> >+                            JSObject *obj, PRBool *_retval)
> >+{
> 
> >+  for (unsigned int i = 0; i < attributes.Length(); ++i) {
> 
> PRUint32
done
> 
> >+NS_IMETHODIMP
> >+nsDOMStringMapSH::GetProperty(nsIXPConnectWrappedNative *wrapper, JSContext *cx,
> >+                              JSObject *obj, jsid id, jsval *vp,
> >+                              PRBool *_retval)
> >+{
> 
> >+  JSString *attrValStr = ::JS_NewUCStringCopyN(cx, reinterpret_cast<const jschar*>(
> >+                                               attrVal.get()), attrVal.Length());
> 
> As usual, seeing this makes me sad.
Any suggestions on what to do instead?
> 
> >diff --git a/dom/interfaces/html/nsIDOMNSHTMLElement.idl b/dom/interfaces/html/nsIDOMNSHTMLElement.idl
> >--- a/dom/interfaces/html/nsIDOMNSHTMLElement.idl
> >+++ b/dom/interfaces/html/nsIDOMNSHTMLElement.idl
> 
> As said, please don't reinvent. The non-perfect alignment is fine.
done
> 
> 
> Jonas, I think you wanted this to happen?

Thanks for the review.

Comment 17

[William Chen [:wchen]]

Attachment: dataset implementation

Updated patch, removed cycle collection and addressed comments.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 532063 [details] [diff] [review]
dataset implementation

Review of attachment 532063 [details] [diff] [review]:
-----------------------------------------------------------------

Still need to review the classinfo stuff, but this looks *awesome* so far!

::: content/html/content/src/nsDOMStringMap.cpp
@@ +63,5 @@
> +  mElement->ClearDataset();
> +}
> +
> +/* DOMString getDataAttr (in DOMString attr); */
> +NS_IMETHODIMP nsDOMStringMap::GetDataAttr(const nsAString& attr, nsAString& _retval NS_OUTPARAM)

I know that the IDL compiler generates stubs that look like this, however generally we prefix all attributes with 'a', including the return value. So the attributes here should be called aAttr and aRetval or aResult.

This applies to all other functions in this class too :(

@@ +72,5 @@
> +  nsAutoString normAttr;
> +  nsresult rv = DataPropToAttr(attr, normAttr);
> +  NS_ENSURE_SUCCESS(rv, NS_OK);
> +
> +  return mElement->GetAttribute(normAttr, _retval);

use mElement->GetAttr here as that's faster (it's defined in nsIContent.h). You'll have to convert the string to an atom which you can do using

nsCOMPtr attr = do_GetAtom(normAttr);
NS_ENSURE_TRUE(attr, NS_ERROR_OUT_OF_MEMORY);

@@ +82,5 @@
> +  nsAutoString normAttr;
> +  nsresult rv = DataPropToAttr(attr, normAttr);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  return mElement->SetAttribute(normAttr, value);

Use mElement->SetAttr

@@ +93,5 @@
> +  nsresult rv = DataPropToAttr(attr, normAttr);
> +
> +  if (NS_FAILED(rv)) {
> +    return NS_OK;
> +  }

Use NS_ENSURE_SUCCESS(rv, NS_OK) here too.

@@ +95,5 @@
> +  if (NS_FAILED(rv)) {
> +    return NS_OK;
> +  }
> +
> +  return mElement->RemoveAttribute(normAttr);

Use mElement->UnsetAttr

@@ +106,5 @@
> +nsresult nsDOMStringMap::GetDataPropList(nsTArray<nsString>& _retval)
> +{
> +  // Gets the complete list of attributes from element.
> +  nsCOMPtr<nsIDOMNamedNodeMap> attrList;
> +  nsresult rv = mElement->GetAttributes(getter_AddRefs(attrList));

Use GetAttrCount and GetAttrNameAt. Both are defined in nsIContent.h

Skipping most of the rest of this function as it's going to change heavily.

@@ +120,5 @@
> +  const PRUint32 dataPrefixLen = 5;
> +
> +  // Ensure capacity of return array is large enough to store all the property
> +  // names.
> +  PRBool capacitySet = _retval.SetCapacity(length);

I'm not sure it's worth calling SetCapacity like this since most likely many of the attributes are going to be non-data attributes. Given that this function is likely not to be called in perf-critical paths it's probably better to optimize for memory than performance.

Note that nsTArray by default uses a infallible allocator (aborts if it runs out of memory), so you don't have to add out-of-memory checks as they'll never fail.

@@ +145,5 @@
> +    // a data attribute.
> +    nsDependentSubstring prefix(attrName, 0, dataPrefixLen);
> +    if (!dataPrefix.Equals(prefix)) {
> +      continue;
> +    }

The function StringBeginsWith does this for you.

You can use NS_NAMED_LITERAL_STRING to create the prefix string outside the loop.

@@ +165,5 @@
> +      const PRUnichar* next = cur + 1;
> +      if (PRUnichar('-') == *cur && next < end && 
> +          PRUnichar('a') <= *next && *next <= PRUnichar('z')) {
> +          // Camel case lower case letters that follow a "-".
> +          prop.Append(PRUnichar(NS_ToUpper(*next)));

Given that you have to cast here, I too think this would look better as |*next - 'a' + 'A'|. As an added bonus that's slightly faster too.

@@ +184,5 @@
> +/**
> + * Converts a dataset property name to the corresponding data attribute name.
> + * (ex. aBigFish to data-a-big-fish).
> + */
> +nsresult nsDOMStringMap::DataPropToAttr(const nsAString& aProp, nsAString& _retval)

You might want to make this function simply return a bool. You're only using the actual return error code in one location anyway so you might as well put the error code there instead. That way you can do something like

if (!DataPropToAttr(attr, normAttr)) {
  return NS_OK;
}

at the call sites.

@@ +192,5 @@
> +
> +  // String corresponding to the data attribute on the element.
> +  nsAutoString attr;
> +  // Set the capacity to 5 ("data-") + double aProp length ("A" -> "-a").
> +  PRBool capacitySet = attr.SetCapacity(5 + aProp.Length() * 2);

Actually, this could have the opposite effect of what I think you're intending. Most of the time the nsAutoString will likely be able to hold the resulting string without allocating memory. By making such a conservative SetCapacity call you're running a higher risk that we'll end up having to allocate even though it otherwise wouldn't be needed. So I'd say just leave it out.

@@ +212,5 @@
> +
> +    if (PRUnichar('A') <= *cur && *cur <= PRUnichar('Z')) {
> +      // Uncamel-case characters in the range of "A" to "Z".
> +      attr.Append(PRUnichar('-'));
> +      attr.Append(PRUnichar(NS_ToLower(*cur)));

Same as above. I'd personally prefer *cur - 'A' + 'a' which avoids the casting.

::: content/html/content/src/nsGenericHTMLElement.cpp
@@ +371,5 @@
> +{
> +  nsDOMSlots *slots = DOMSlots();
> +
> +  if (!slots->mDataset) {
> +    slots->mDataset = new nsDOMStringMap(this);

Add a comment saying that this won't addref but that we'll addref below before using the new object.

::: content/html/content/src/nsGenericHTMLElement.h
@@ +45,5 @@
>  #include "nsIDOMNSHTMLFrameElement.h"
>  #include "nsFrameLoader.h"
>  #include "nsGkAtoms.h"
>  #include "nsContentCreatorFunctions.h"
> +#include "nsCOMPtr.h"

I bet you don't need this. It's technically not a bad idea, but it's probably just a waste of compile time.

::: content/html/content/test/test_bug560112.html
@@ +29,5 @@
> +{
> +  var el = document.createElement('div');
> +
> +  // Set property.
> +  el.dataset[prop] = "zzzzzz";

Can you check that |prop in el.dataset| returns false before this line and true after it?

@@ +43,5 @@
> +  is(el.getAttribute(attr), "yyyyyy", 'Attribute "' + attr + '" should have value "yyyyyy".');
> +
> +  // Delete property.
> +  delete el.dataset[prop];
> +  ok(!el.hasAttribute(attr), 'Element should not have data attribute for dataset property "' + prop + '".');

And check that |prop in el.dataset| returns false again here.

@@ +133,5 @@
> +SetGetOverwriteDel('data-data-', 'data-');
> +SetGetOverwriteDel('data-data-data-', 'dataData-');
> +
> +// Longer attribute.
> +SetGetOverwriteDel('data-long-long-long-long-long-long-long-long-long-long-long-long-long', 'longLongLongLongLongLongLongLongLongLongLongLongLong');

Might be worth generating *really* long (32k or so) attribute name using a loop.

Comment 19

[Peter Van der Beken [:peterv]]

(In reply to comment #17)
> removed cycle collection

If you hold a strong reference to a node you most likely need to participate in cycle collection.

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 532063 [details] [diff] [review]
dataset implementation

Review of attachment 532063 [details] [diff] [review]:
-----------------------------------------------------------------

I assume that all I need to look at is the nsDOMClassInfo stuff. If there's other stuff you want me to look at, let me know.

::: dom/base/nsDOMClassInfo.cpp
@@ +8555,5 @@
> +  *objp = obj;
> +
> +  *_retval = ::JS_DefineElement(cx, obj, nsnull, JSVAL_VOID, nsnull, nsnull,
> +                                JSPROP_SHARED);
> +

This is almost certainly not what you want here. At the *very* least, you have nsnull (which should be thought of as a pointer) in place of the index. But more than that, NewResolve's purpose is to lazily resolve certain properties. This implementation says that your object has *every* property.

But, if you are lazily resolving a property, you need to tell the engine about that property, which is what the JS_DefineElement is for. But instead of telling the engine about the property that it's asking about, you're always defining the element "0", which might not be the property that you're being asked about.

For an array, it's more likely that you want to do something closer to nsGenericArraySH::NewResolve.

Also, as a general comment, I know this file is inconsistent, but I find ::JS_* ugly. JS_* works just as well without those extra ::s.

@@ +8574,5 @@
> +  
> +  // Clear the properties of the dataset because they may have changed since
> +  // the last enumeration from adding or removing "data-*" attributes directly
> +  // on the element.
> +  ::JS_ClearScope(cx, obj);

This is a really big hammer to use here. Furthermore, it'll blow away expando properties that have been set. This is definitely a problem with the JSClass API (which is basically what we're implementing here) but if it is really that important to get this case right, then we'll probably have to switch to a NewEnumerate style hook here.

JS_ClearScope is really a very invasive API. That manages to break JS engine invariants. It should be used as little as possible.

Comment 21

[William Chen [:wchen]]

Attachment: Updated dataset implementation

All comments addressed.

Comment 22

[William Chen [:wchen]]

Attachment: Diff from dataset2 to 3

Diff from previous patch.

Comment 23

[William Chen [:wchen]]

Attachment: Dataset implementation

Updated to add PreCreate hook.

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 533088 [details] [diff] [review]
Dataset implementation

Review of attachment 533088 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with these changes!

::: content/html/content/src/nsDOMStringMap.cpp
@@ +219,5 @@
> +nsresult nsDOMStringMap::GetDataPropList(nsTArray<nsString>& aResult)
> +{
> +  PRUint32 attrCount = mElement->GetAttrCount();
> +
> +  // Iterate through all the attributes (in reverse order) and add property

The "in reverse order" part is no longer correct.

@@ +249,5 @@
> +  const PRUnichar* end = aProp.EndReading();
> +
> +  // String corresponding to the data attribute on the element.
> +  nsAutoString attr;
> +  attr = NS_LITERAL_STRING("data-");

just do

nsAutoString attr = NS_LITERAL_STRING("data-");

Also, you could do attr.SetCapacity(aProp.Length() + 5) since you know that you'll need at least that many characters. Sorry for not pointing this out earlier.

@@ +284,5 @@
> + */
> +PRBool nsDOMStringMap::AttrToDataProp(const nsAString& aAttr,
> +                                      nsAString& aResult)
> +{
> +  NS_NAMED_LITERAL_STRING(dataPrefix, "data-");

No need for this temporary, just use NS_LITERAL_STRING(...) in the call below instead.

::: content/html/content/src/nsGenericHTMLElement.cpp
@@ +385,5 @@
> +nsGenericHTMLElement::ClearDataset()
> +{
> +  nsDOMSlots *slots = DOMSlots();
> +
> +  NS_ASSERTION(slots->mDataset, "Dataset should not be null.");

You'd save a little bit of code if you use GetExistingDOMSlots here. Also change the assertion to
NS_ASSERTION(slots && slots->mDataset, ...)

::: content/html/content/test/test_bug560112.html
@@ +45,5 @@
> +  is(el.getAttribute(attr), "yyyyyy", 'Attribute "' + attr + '" should have value "yyyyyy".');
> +
> +  // Delete property.
> +  delete el.dataset[prop];
> +  ok(!el.hasAttribute(attr), 'Element should not have data attribute for dataset property "' + prop + '".');

Also check is(prop in el.dataset, false, "...");

::: dom/base/nsDOMClassInfo.cpp
@@ +8607,5 @@
> +  *parentObj = globalObj;
> +
> +  nsCOMPtr<nsIDOMDOMStringMap> dataset(do_QueryInterface(nativeObj));
> +  NS_ENSURE_TRUE(dataset, NS_ERROR_UNEXPECTED);
> +  nsDOMStringMap* stringMap = static_cast<nsDOMStringMap*>(dataset.get());

Actually, given that you're relying on having a real nsDOMStringMap here, there's no need to do the QI above. You can just use a simple static cast directly to nsDOMStringMap from the nsISupports

@@ +8609,5 @@
> +  nsCOMPtr<nsIDOMDOMStringMap> dataset(do_QueryInterface(nativeObj));
> +  NS_ENSURE_TRUE(dataset, NS_ERROR_UNEXPECTED);
> +  nsDOMStringMap* stringMap = static_cast<nsDOMStringMap*>(dataset.get());
> +
> +  nsIDocument* document = stringMap->GetElement()->GetDocument();

Use GetOwnerDoc() instead of GetDocument. The latter is deprecated and doesn't do quite what you want here.

::: dom/interfaces/html/nsIDOMDOMStringMap.idl
@@ +46,5 @@
> +{
> +  [noscript] boolean hasDataAttr(in DOMString prop);
> +  [noscript] DOMString getDataAttr(in DOMString prop);
> +  [noscript] void setDataAttr(in DOMString prop, in DOMString value);
> +  [noscript] void removeDataAttr(in DOMString prop);

Make these [notxpcom] instead of [noscript]. That will give them a nicer more C++ friendly call signature.

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to comment #24)
> ::: dom/base/nsDOMClassInfo.cpp
> @@ +8607,5 @@
> > +  *parentObj = globalObj;
> > +
> > +  nsCOMPtr<nsIDOMDOMStringMap> dataset(do_QueryInterface(nativeObj));
> > +  NS_ENSURE_TRUE(dataset, NS_ERROR_UNEXPECTED);
> > +  nsDOMStringMap* stringMap = static_cast<nsDOMStringMap*>(dataset.get());
> 
> Actually, given that you're relying on having a real nsDOMStringMap here,
> there's no need to do the QI above. You can just use a simple static cast
> directly to nsDOMStringMap from the nsISupports

I don't know this code well (or at all), but are we really, really sure only nsIDOMDOMStringMaps get here? If you make this change, an assert would be necessary, IMO.

Comment 26

[William Chen [:wchen]]

Attachment: Updated dataset implementation

Updated patch to address sicking's comments.

Comment 27

[William Chen [:wchen]]

Attachment: Diff from last patch

Diff from last patch

Comment 28

[William Chen [:wchen]]

(In reply to comment #27)
> Created attachment 533832 [details] [diff] [review] [review]
> Diff from last patch
> 
> Diff from last patch
Ignore, almost all of my changes didn't show up in this patch.

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 533831 [details] [diff] [review]
Updated dataset implementation

Review of attachment 533831 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/html/content/test/test_bug560112.html
@@ +24,5 @@
> + * Gets dataset property. Checks data attribute "attr".
> + * Overwrites dataset property Checks data attribute "attr".
> + * Deletes dataset property. Checks data attribute "attr".
> + */
> +function SetGetOverwriteDel(attr, prop)

Could you make this function also test

el.dataset[prop] = "foo";
el.removeAttribute(attr);
is(prop in el.dataset, false, "...");

Comment 30

[William Chen [:wchen]]

Attachment: Updated dataset implementation

Added extra test cases.

Comment 31

[William Chen [:wchen]]

Attachment: diff

diff from last patch.

Comment 32

[William Chen [:wchen]]

Attachment: Updated dataset implementation

I forgot to hit the patch checkbox on the last attachment.

Comment 33

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 533088 [details] [diff] [review]
Dataset implementation

Review of attachment 533088 [details] [diff] [review]:
-----------------------------------------------------------------

This looks pretty good! I have a couple of minor comments below. r=mrbkap with them addressed.

::: content/html/content/src/nsDOMStringMap.cpp
@@ +191,5 @@
> +
> +  jsval val;
> +  JSContext* cx = nsContentUtils::GetCurrentJSContext();
> +  nsresult rv = nsContentUtils::WrapNative(cx, JS_GetScopeChain(cx),
> +                                           this, &val, nsnull, PR_TRUE);

I think what you want here is actually:

nsContentUtils::WrapNative(cx, JS_GetScopeChain(cx),
                           this, &val, nsnull, PR_FALSE); // notice PR_FALSE for aAllowWrapping.

JSAutoEnterCompartment ac;
if (!ac.enter(cx, JSVAL_TO_OBJECT(val))
  return NS_ERROR_FAILURE;

JS_DeleteUCProperty2(...);

Otherwise you risk the delete fizzling on an Xray wrapper.

::: dom/base/nsDOMClassInfo.cpp
@@ +8569,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  if (hasProp) {
> +    *_retval = JS_DefinePropertyById(cx, obj, id, JSVAL_VOID,
> +                                     nsnull, nsnull, JSPROP_SHARED); 

It doesn't actually matter, but to be consistent with your implementation of Enumerate, below, you should probably add a JSPROP_ENUMERATE here.

Comment 34

[William Chen [:wchen]]

Attachment: Updated dataset implementation

Addressed Blakes's comments.

Thanks for the review guys.

Comment 35

[William Chen [:wchen]]

Attachment: diff

Diff from last patch

Comment 36

[William Chen [:wchen]]

Attachment: exported dataset implementaiton

patch created using hg export.

Comment 37

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/98d45ba64574

Comment 38

[Jean-Yves Perrier [:teoli]]

Regarding doc, I have updated:
https://developer.mozilla.org/en/Firefox_6_for_developers
https://developer.mozilla.org/en/HTML/Global_attributes
https://developer.mozilla.org/en/DOM/HTMLElement

and created
https://developer.mozilla.org/en/DOM/element.dataset

I have a question: if I look at the production rule and the transformation to the camelCase name, it means that both attribute data-example-with-trailing-dash- and data-example-with-trailing-dash are both allowed in HTML5 and maps to HTMLElement.exampleWithTrailingDash . Is this correct?

Comment 39

[Jean-Yves Perrier [:teoli]]

Note that doc for DOMStringMap is missing but this may be outside the scope of this bug.

Comment 40

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

teoli2003: not quite. Only dashes followed by a lowercase letter (a-z) are turned into the corresponding uppercase letter.

Comment 41

[[e-mail]]

Comment on attachment 534099 [details] [diff] [review]
exported dataset implementaiton

>+    nsDependentString prop(properties[i]);
properties[i] is already an nsString. Did you mean nsString& prop?

Comment 42

[Eric Shepherd [:sheppy]]

Updated documentation:

https://developer.mozilla.org/en/DOM/element.dataset
https://developer.mozilla.org/en/DOM/element

And mentioned on Firefox 6 for developers

Comment 43

[Eric Shepherd [:sheppy]]

Added a doc for DOMStringMap (which probably needs significant help):

https://developer.mozilla.org/en/DOM/DOMStringMap

Comment 44

[Mounir Lamouri (:mounir)]

Shouldn't this be tracked as a new feature?

Comment 45

[Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]]]

dchan to lead security implementation review

Comment 46

[David Chan [:dchan]]

The security implementation review of the feature has been completed. The included tests were pretty comprehensive.