Last Comment Bug 560112 - (dataset) Implement HTML5 dataset attribute
(dataset)
: Implement HTML5 dataset attribute
Status: RESOLVED FIXED
[parity-webkit][parity-opera][evang-w...
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- enhancement with 8 votes (vote)
: mozilla6
Assigned To: William Chen [:wchen]
:
: Andrew Overholt [:overholt]
Mentors:
http://www.whatwg.org/html/#dom-dataset
Depends on: 658746 669903
Blocks: html5
  Show dependency treegraph
 
Reported: 2010-04-18 07:40 PDT by karl155
Modified: 2012-08-15 08:23 PDT (History)
39 users (show)
dveditz: sec‑review+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed


Attachments
Patch for implementation of dataset. (28.79 KB, patch)
2011-05-10 18:16 PDT, William Chen [:wchen]
no flags Details | Diff | Splinter Review
revised patch for implementation of dataset (38.60 KB, text/plain)
2011-05-12 02:30 PDT, William Chen [:wchen]
no flags Details
dataset implementation (38.60 KB, patch)
2011-05-12 02:32 PDT, William Chen [:wchen]
no flags Details | Diff | Splinter Review
dataset implementation (37.83 KB, patch)
2011-05-12 16:03 PDT, William Chen [:wchen]
no flags Details | Diff | Splinter Review
Updated dataset implementation (42.93 KB, patch)
2011-05-16 18:53 PDT, William Chen [:wchen]
no flags Details | Diff | Splinter Review
Diff from dataset2 to 3 (27.12 KB, patch)
2011-05-16 18:54 PDT, William Chen [:wchen]
no flags Details | Diff | Splinter Review
Dataset implementation (43.97 KB, patch)
2011-05-17 15:13 PDT, William Chen [:wchen]
jonas: review+
mrbkap: review+
Details | Diff | Splinter Review
Updated dataset implementation (44.11 KB, patch)
2011-05-19 15:56 PDT, William Chen [:wchen]
jonas: review+
Details | Diff | Splinter Review
Diff from last patch (1.76 KB, patch)
2011-05-19 15:57 PDT, William Chen [:wchen]
no flags Details | Diff | Splinter Review
Updated dataset implementation (44.69 KB, text/plain)
2011-05-19 17:59 PDT, William Chen [:wchen]
no flags Details
diff (2.03 KB, patch)
2011-05-19 18:00 PDT, William Chen [:wchen]
no flags Details | Diff | Splinter Review
Updated dataset implementation (44.69 KB, patch)
2011-05-19 18:02 PDT, William Chen [:wchen]
jonas: review+
Details | Diff | Splinter Review
Updated dataset implementation (44.83 KB, patch)
2011-05-20 10:25 PDT, William Chen [:wchen]
no flags Details | Diff | Splinter Review
diff (1.96 KB, patch)
2011-05-20 10:26 PDT, William Chen [:wchen]
no flags Details | Diff | Splinter Review
exported dataset implementaiton (44.91 KB, patch)
2011-05-20 14:02 PDT, William Chen [:wchen]
no flags Details | Diff | Splinter Review

Description karl155 2010-04-18 07:40:20 PDT
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] 2010-07-07 15:03:17 PDT
Fixed in Webkit:
https://bugs.webkit.org/show_bug.cgi?id=41146
Comment 2 Paul Rouget [:paul] 2010-09-01 03:01:03 PDT
Test from webkit: http://trac.webkit.org/export/66582/trunk/LayoutTests/fast/dom/dataset.html
Comment 3 Simon 2011-02-23 04:16:16 PST
Is there a road map for this feature?
Comment 4 :Ms2ger (⌚ UTC+1/+2) 2011-02-23 04:40:23 PST
(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] 2011-05-10 18:16:28 PDT
Created attachment 531516 [details] [diff] [review]
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] 2011-05-10 23:25:34 PDT
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] 2011-05-11 02:30:49 PDT
I wonder if the patch is slow-ish, mainly because StringMap isn't
quickstubbed at all.
Comment 8 Mounir Lamouri (:mounir) 2011-05-11 02:55:49 PDT
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 2011-05-11 13:51:18 PDT
> >+  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] 2011-05-12 02:30:50 PDT
Created attachment 531872 [details]
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] 2011-05-12 02:32:30 PDT
Created attachment 531873 [details] [diff] [review]
dataset implementation

I forgot to click the "patch" check box while uploading my last patch.
Comment 12 Lars Gunther 2011-05-12 10:36:38 PDT
Should also be parity Opera?
Comment 13 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-12 11:38:38 PDT
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 (⌚ UTC+1/+2) 2011-05-12 12:37:02 PDT
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 (⌚ UTC+1/+2) 2011-05-12 12:41:47 PDT
And thanks a lot for working on this, William!
Comment 16 William Chen [:wchen] 2011-05-12 15:59:10 PDT
(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] 2011-05-12 16:03:15 PDT
Created attachment 532063 [details] [diff] [review]
dataset implementation

Updated patch, removed cycle collection and addressed comments.
Comment 18 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-12 23:16:27 PDT
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] 2011-05-13 06:30:17 PDT
(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) 2011-05-16 03:26:18 PDT
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] 2011-05-16 18:53:27 PDT
Created attachment 532827 [details] [diff] [review]
Updated dataset implementation

All comments addressed.
Comment 22 William Chen [:wchen] 2011-05-16 18:54:14 PDT
Created attachment 532828 [details] [diff] [review]
Diff from dataset2 to 3

Diff from previous patch.
Comment 23 William Chen [:wchen] 2011-05-17 15:13:07 PDT
Created attachment 533088 [details] [diff] [review]
Dataset implementation

Updated to add PreCreate hook.
Comment 24 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-18 15:33:03 PDT
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 (⌚ UTC+1/+2) 2011-05-19 14:34:41 PDT
(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] 2011-05-19 15:56:36 PDT
Created attachment 533831 [details] [diff] [review]
Updated dataset implementation

Updated patch to address sicking's comments.
Comment 27 William Chen [:wchen] 2011-05-19 15:57:15 PDT
Created attachment 533832 [details] [diff] [review]
Diff from last patch

Diff from last patch
Comment 28 William Chen [:wchen] 2011-05-19 15:59:09 PDT
(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 2011-05-19 17:26:42 PDT
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] 2011-05-19 17:59:46 PDT
Created attachment 533860 [details]
Updated dataset implementation

Added extra test cases.
Comment 31 William Chen [:wchen] 2011-05-19 18:00:19 PDT
Created attachment 533861 [details] [diff] [review]
diff

diff from last patch.
Comment 32 William Chen [:wchen] 2011-05-19 18:02:15 PDT
Created attachment 533863 [details] [diff] [review]
Updated dataset implementation

I forgot to hit the patch checkbox on the last attachment.
Comment 33 Blake Kaplan (:mrbkap) 2011-05-20 09:57:55 PDT
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] 2011-05-20 10:25:45 PDT
Created attachment 534018 [details] [diff] [review]
Updated dataset implementation

Addressed Blakes's comments.

Thanks for the review guys.
Comment 35 William Chen [:wchen] 2011-05-20 10:26:11 PDT
Created attachment 534019 [details] [diff] [review]
diff

Diff from last patch
Comment 36 William Chen [:wchen] 2011-05-20 14:02:14 PDT
Created attachment 534099 [details] [diff] [review]
exported dataset implementaiton

patch created using hg export.
Comment 37 :Ms2ger (⌚ UTC+1/+2) 2011-05-20 14:48:23 PDT
http://hg.mozilla.org/mozilla-central/rev/98d45ba64574
Comment 38 Jean-Yves Perrier [:teoli] 2011-05-21 08:04:40 PDT
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] 2011-05-21 08:05:37 PDT
Note that doc for DOMStringMap is missing but this may be outside the scope of this bug.
Comment 40 :Ms2ger (⌚ UTC+1/+2) 2011-05-22 09:31:49 PDT
teoli2003: not quite. Only dashes followed by a lowercase letter (a-z) are turned into the corresponding uppercase letter.
Comment 41 neil@parkwaycc.co.uk 2011-05-23 11:41:16 PDT
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] 2011-05-23 15:09:27 PDT
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] 2011-05-23 15:20:49 PDT
Added a doc for DOMStringMap (which probably needs significant help):

https://developer.mozilla.org/en/DOM/DOMStringMap
Comment 44 Mounir Lamouri (:mounir) 2011-06-17 02:09:09 PDT
Shouldn't this be tracked as a new feature?
Comment 45 Curtis Koenig [:curtisk-use curtis.koenig+bzATgmail.com]] 2011-06-28 13:46:46 PDT
dchan to lead security implementation review
Comment 46 David Chan [:dchan] 2011-08-01 15:37:44 PDT
The security implementation review of the feature has been completed. The included tests were pretty comprehensive.

Note You need to log in before you can comment on or make changes to this bug.