1222079 - Implement nsDOMAttributeMap::GetSupportedNames

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Now that we have a way to leave it empty when asked for enumerable ones, we should implement it and return the actual list when asked for non-enumerable ones.  Testcase:

  var elem = document.createElement("div");
  elem.setAttribute("foo", "");
  console.log(Object.getOwnPropertyNames(elem.attributes)+"");

This should log something like "0,foo" but just logs "0".

Comment 1

[Salah]

Hi, I would like to work on this bug, could you provide me with some starter instructions on how to begin please.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Sure.

Assuming you have a working build environment, what you need to do is the following:

1)  Change nsDOMAttributeMap::NameIsEnumerable (in nsDOMAttributeMap.cpp) to always return false.

2)  Implement nsDOMAttributeMap::GetSupportedNames in a useful way.  Specifically, it should return an empty array if !(aFlags & JSITER_HIDDEN) and otherwise return a list of the qualified names of the attributes on the element, making sure to avoid duplicates.  You should be able to do this by getting mContent->GetAttrCount(), then using GetAttrNameAt() to iterate along the attribute names.  In the common case that the nsAttrName tests true for IsAtom(), just append its Atom()'s string to the list.  Otherwise, get NodeInfo(), check that its QualifedName() is not in the list already, and if so append it.

Comment 3

[Boris Zbarsky [:bzbarsky]]

And of course feel free to ask me if you have any questions.  At least for the next week and a half or so.

Comment 4

[Salah]

Attachment: patch1222079.patch

Here is the patch with the proposed changes.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8696382 [details] [diff] [review]
patch1222079.patch

This generally looks good.  Just some formatting nits:

>+++ b/dom/base/nsDOMAttributeMap.cpp
>+void nsDOMAttributeMap::GetSupportedNames(unsigned aFlags, nsTArray<nsString>& aNames)

If you look at other functions in this file, the return type is on a separate line from the rest of the function declaration, and lines are 80 characters or shorter.  So this should look like so:

void
nsDOMAttributeMap::GetSupportedNames(unsigned aFlags, 
                                     nsTArray<nsString>& aNames)

>+
>+if (!(aFlags & JSITER_HIDDEN)){

Please fix this toe be indented correctly (two spaces) and not have the random blank line before it.  And one space before the '{', please.

>+    
>+}

Again, there is a stray blank line, and the '}' is not indented correctly.

>+const uint32_t count = mContent->GetAttrCount();
>+bool seenNonAtomName = false;
>+for (uint32_t i = 0; i < count; i++) {

These all need to be indented two spaces.

>+    if (seenNonAtomName){
>+        
>+    }

This part is wrong.  You want the aNames.Contains() test to be inside this if, and it's not, right?

Or perhaps even more simply:

    if (seenNonAtomName && aNames.Contains(qualifiedName)) {
      continue;
    }

>+        if(aNames.Contains(qualifiedName)) {

If you keep this as a nested if, space after the "if", please.  And fix the indent: this should be indented 6 spaces total (2 spaces from the block it's in).

>+            continue;

Fix the indent: this should be indented 8 spaces total if you keep the nested if, 6 spaces otherwise.

>+    aNames.AppendElement(qualifiedName);
>+}

Fix the indent on that '}': should be 2 spaces.

>+    
>+}

No need for that stray blank line.

>+
>+

And one of those two blank lines can also go away.

Also, this whole function should probably be right after the NameIsEnumerable implementation, not right above the constructor as you have it.

>+++ b/dom/base/nsDOMAttributeMap.h
>+void
>+GetSupportedNames(unsigned aFlags, nsTArray<nsString>& aNames);

Please fix the indent: should be indented two spaces like everything around it.

>     // No supported names we want to show up in iteration.

This comment should just go away.

>+ 
>   size_t SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf) const;

Please don't add that stray blank line.

Marking r-, but again this is very close.  Do please fix the above formatting issues.

Also, this could use tests.  Probably the best thing to do is to add them to testing/web-platform/tests/dom/nodes/attributes.html and then run that test locally like so:

  mach web-platform-tests testing/web-platform/tests/dom/nodes/attributes.html

The tests should probably assert the correctness of the following arrays, using assert_array_equals:

1)  Object.getOwnPropertyNames(el.attributes)
2)  Object.getOwnPropertyNames(el.attributes).filter((name) => Object.getOwnPropertyDescriptor(el.attributes, name).enumerable)
3)  getEnumerableProps(el.attributes)

where getEnumerableProps is defined like so:

  function getEnumerableProps(obj) {
    var arr = [];
    for (var prop in obj) arr.push(prop);
    return arr;
  }

for several different situations, with one test() for each situation.  At a minimum, the following situations:

* el = document.createElement("div"); el.setAttribute("a", ""); el.setAttribute("b", "");

* el = document.createElement("div"); el.setAttributeNS("", "a", ""); el.setAttribute("b", ""); el.setAttributeNS("foo", "a", "");

* el = document.createElement("div"); el.setAttributeNS("foo", "a", ""); el.setAttribute("b", ""); el.setAttributeNS("", "a", "");

* el = document.createElement("div"); el.setAttributeNS("", "a:b", "");
el.setAttributeNS("", "c:d", ""); el.setAttributeNS("foo", "a:b", "");

* el = document.createElement("div"); el.setAttributeNS("foo", "a:b", "");
el.setAttributeNS("", "c:d", ""); el.setAttributeNS("", "a:b", "");

Please let me know if this is sounding too overwhelming...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, and ideally the updated patch should have a commit message describing the change.  Something like:

  Bug 1222079.  Fix the behavior of Object.getOwnPropertyNames for nsDOMAttributeMap.  r=bz

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Diff for tests to be incorporated in the fix

Comment 8

[Salah]

Attachment: Indentations and final implementation patch changes made

I have made the final changes as requested. Everything should be fine now, hopefully.. :)

Comment 9

[Boris Zbarsky [:bzbarsky]]

That last patch is neither against tip nor on top of the other patch you posted in the bug....

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/87caa5721cd9

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/87caa5721cd9

Comment 12

[Arkadiusz Michalski (Spirit)]

Can enyone explain why for this testcase in DevTools we see such strange result:

<!DOCTYPE html>
<html>
<body>
    <script>
        
        var newP = document.createElement("P");
        newP.setAttributeNS("www.test1.com", "ID", "Test1");
        newP.setAttributeNS("www.test2.com", "Class", "Test2");
        newP.setAttribute("id", "SomeIdInHTMLNamespace");
        newP.className = "Class1 Class2 Class3";
        newP.setAttributeNS("www.test3.com", "A:NEW", "Test3");
        newP.setAttributeNS("www.test4.com", "a:new", "Test4");
        
        console.log(newP.attributes);
        console.log(Object.getOwnPropertyNames(newP.attributes));

    </script>
</body>
</html>

getOwnPropertyNames() product correct value but in DevTools we get 6 unique qualified names and this qualified names refer to the same attributes, so finaly wee get 3 attributes (not 6):

A:NEW a:new="Test4"
Class class="Class1 Class2 Class3"
ID SomeIdInHTMLNamespace
a:new a:new="Test4"
class class="Class1 Class2 Class3"
id SomeIdInHTMLNamespace

Comment 13

[Arkadiusz Michalski (Spirit)]

Ohh now realized that DevTools shows potential attributes which will returned when we using this named getters and behaviour for getting this attributes was described via getNamedItem() methods. In my example I use p element in HTML namespace in HTML document so that is why I get only 3 attributes (because nameed getters is conversing to lowercase). So everything looks correct.