Simple testcase for that bug: var a1 = document.createAttribute("aa"); a1.nodeValue = "lowercase"; var a2 = document.createAttribute("AA"); a2.nodeValue = "UPPERCASE"; document.documentElement.setAttributeNode(a1); document.documentElement.setAttributeNode(a2); alert(document.documentElement.getAttributeNS("", "aa")); // should alert null per spec alert(document.documentElement.getAttributeNS("", "AA")); // should alert UPPERCASE

Comment on attachment 8498906 [details] [diff] [review] bug1075702.diff >+ nsRefPtr<Attr> attribute = GetAttribute(oldNi, true); Maybe call this oldAttr? >+ if(attribute == aAttr) { Space before "(", please. Also, does this actually compile? I would have thought you need "attribute == &aAttr". >+ if(attribute) { Also, space before "(". >+ attr = RemoveAttribute(oldNi); This should return the same thing as oldAttr, right? Please assert that? >+ ni = aAttr.NodeInfo(); Move the declaration of "ni" down here too? Does this fix the testcase? Please add tests!

When I apply this patch and try to setAttributeNode, tab crashes. Can you check that?

Comment on attachment 8499093 [details] [diff] [review] bug1075702.diff >+ nsRefPtr<Attr> attribute = GetAttribute(oldAttr, true); No, I meant naming the nsRefPtr<Attr> oldAttr. In any case, this is presumably crashing if GetExistingAttrNameFromQName() returned null. Which it can do. If that happens, that means there is no oldAttr. So all this stuff about GetAttribute and RemoveAttribute() can be conditioned on the nodeinfo being non-null.

For testcase mentioned in description: //Patch throws lowercase instead of null alert(document.documentElement.getAttributeNS("", "aa")); //Patch throws uppercase alert(document.documentElement.getAttributeNS("", "AA"));

Comment on attachment 8499555 [details] [diff] [review] bug1075702.diff >//Patch throws lowercase instead of null Er, why? Are you not removing the lowercase version of the attribute? Oh, I see. RemoveAttribute() doesn't remove it from the _element_. You need to UnsetAttr on mContent as well. Perhaps create a RemoveNamedItem version that takes a nodeinfo and call it from here, RemoveNamedItem, and RemoveNamedItemNS?

Now I'm calling UnsetAttr on mContent as well. But still it's showing same behavior as comment 5.

Hmm. Do aNodeInfo and attrNi actually match? You really need to get a C++ debugger working here and figure out what is being passed to the RemoveAttr call and whether it matches what you expect to get passed.

@bz: Thanks! Now it works! :D

Comment on attachment 8505628 [details] [diff] [review] bug1075702.diff You don't need the "mozilla::dom" bits, by the way, given the "uses" declarations at the top of the file. Please add a test? r=me with that. Thank you!

Try : https://tbpl.mozilla.org/?tree=Try&rev=363c1c5cec35

Comment on attachment 8506276 [details] [diff] [review] test_bug1075702.diff Did you run this test? Does it actually pass? There is no "NULL" in JS. There is "null". Apart from that, this looks reasonable.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6dc9a6503ad1

Comment on attachment 8510154 [details] [diff] [review] bug1075702-part2.diff >+ is(document.body.getAttribute('aa'), null, "Wrong value (1)"); Maybe change the string to indicate why null is expected? Because the property that's set has the localName "AA" and not "aa". Also add getAttributeNS tests here as well? >+ is(s, "AA=", "Wrong attribute!"); This part doesn't look right to me. It should be "AA=UPPERCASE", I'd think. Why isn't it? >- document.body.getAttributeNode("AA").nodeValue = "FOO"; >- is(document.body.getAttribute("AA"), "FOO", "Wrong value!"); This test should stay, but replace getAttribute with getAttributeNS, no? > ok(!document.body.hasAttribute("AA"), "Should not have attribute!"); > ok(!document.body.hasAttribute("aa"), "Should not have attribute!"); These tests are meaningless, since getAttribute returned null even when the attribute was there. Add tests here using getAttributeNS as well, please.

Comment on attachment 8507038 [details] [diff] [review] test_bug1075702.diff r=me

TRY: https://tbpl.mozilla.org/?tree=Try&rev=0383e33067d4

Finally try is green! :D https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2bfd058bd2a1

Comment on attachment 8532765 [details] [diff] [review] bug1075702.diff >+ if (attrNS == aAttr.NodeInfo()->NamespaceID() && >+ aAttr.NodeInfo()->Equals(nameAtom)) { if (aAttr.NodeInfo()->Equals(nameAtom, attrNS)) { and probably add a comment that we're purposefully ignoring the prefix. >+ GetNodeInfo(nameAtom, name->GetPrefix(), aAttr.NodeInfo()->NamespaceID(), I'd prefer you use attrNS instead of aAttr.NodeInfo()->NamespaceID() for the third arg here, just so we're consistent and using |name| for everything. >+ NS_ASSERTION(attr->NodeInfo()->NameAndNamespaceEquals(oldNi), "RemoveNamedItem() called, attr->NodeInfo() should be equal to oldNi!"); Please wrap to 80 columns. >+nsDOMAttributeMap::RemoveNamedItem(NodeInfo* aNodeInfo) This function doesn't look quite right to me: it'll fire an incorrect mutation event from inside the UnsetAttr, because it has already removed the Attr, no? Seems to me like this should be doing the GetAttribute() bit and we should still be calling it from nsDOMAttributeMap::RemoveNamedItemNS. Please add a test for this mutation event case. Also, this needs a test (that fails without these patches) for the incorrect mutation event we fire right now on trunk with nsDOMAttributeMap::RemoveNamedItemNS. r=me with those fixed.

Comment on attachment 8532766 [details] [diff] [review] bug1075702-part2.diff The checkin comment here should mention "on Attr nodes" or something. But this needs to be folded with part 1 to not have an intermediate build that fails tests, right? r=me with that done.

Comment on attachment 8532767 [details] [diff] [review] bug1075702-part3.diff r=me, but again need to fold this into part 1.

Comment on attachment 8532768 [details] [diff] [review] test_bug1075702.diff r=me

Comment on attachment 8532769 [details] [diff] [review] test_fix.diff >+ is(document.body.getAttribute('aa'), null, "property has the localName AA and not aa."); >+ is(document.body.getAttribute('AA'), null, "property has the localName AA and not aa."); Would be nice to have different description things there. Also, "attribute" instead of "property". >+ is(document.body.getAttributeNS("", "aa"), null, "Should be NULL!"); And here s/NULL/null/, or better yet "Attribute should have localName AA". ;) >+ is(document.body.getAttributeNS("", "aa"), null, "Should be NULL!"); Again, "null". And again, unique descriptive strings preferred. This also needs to be folded into part 1. r=me with those bits fixed.

Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=84387afa7cdf

Comment on attachment 8548201 [details] [diff] [review] bug1075702.diff This part of comment 31 wasn't addressed and should be: > if (aAttr.NodeInfo()->Equals(nameAtom, attrNS)) { And you still need to add the mutation event tests for removeNamedItem/removeNamedItemNS. The actual behavior looks good in this patch. The rest looks good, thanks!

Comment on attachment 8548202 [details] [diff] [review] test_bug1075702.diff Looks great.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=14f5e8b97239

Comment on attachment 8548820 [details] [diff] [review] bug1075702.diff This didn't address the review comment from comment 38.

Comment on attachment 8548821 [details] [diff] [review] test_bug1075702.diff >+ ok(removedNodeAccordingToEvent == removedNodeAccordingToRemoveNamedItemNS, "Node removed according to event is not same as node removed by removeNamedItemNS."); ise(removedNodeAccordingToEvent, removedNodeAccordingToRemoveNamedItemNS, "...") instead of ok(). Same in runTest2(). Also, runTest1() and runTest2() need better names. r=me with that fixed.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d9f3800d0fce

Comment on attachment 8549600 [details] [diff] [review] test_bug1075702.diff r=me

Comment on attachment 8549599 [details] [diff] [review] bug1075702.diff r=me

Try run's looking pretty good!

https://hg.mozilla.org/integration/mozilla-inbound/rev/acc9dcd07546 https://hg.mozilla.org/integration/mozilla-inbound/rev/bafcd4598f5d

https://hg.mozilla.org/mozilla-central/rev/acc9dcd07546 https://hg.mozilla.org/mozilla-central/rev/bafcd4598f5d