:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Description • 11 years ago

      No description provided.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 1 • 11 years ago

Any chance you would be interested in this? :)

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 4 • 11 years ago

Comment on attachment 732742 [details] [diff] [review]
part 1 - renaming

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

If you're going to rename it, rename to mozilla::dom::Attr.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 7 • 11 years ago

Comment on attachment 732774 [details] [diff] [review]
part 1 - renaming

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

r=me

::: content/base/src/nsDOMAttribute.cpp
@@ +38,4 @@
>  
> +Attr::Attr(nsDOMAttributeMap *aAttrMap,
> +           already_AddRefed<nsINodeInfo> aNodeInfo,
> +           const nsAString   &aValue, bool aNsAware)

Feel free to fix the weird spacing here

@@ +262,1 @@
>                                         ErrorResult& aError)

Indentation

@@ +319,5 @@
>  }
>  
>  nsresult
> +Attr::InsertChildAt(nsIContent* aKid, uint32_t aIndex,
> +                            bool aNotify)

Indentation

::: content/base/src/Makefile.in
@@ +47,5 @@
>  EXPORTS_mozilla/dom = \
>    Comment.h \
>    DocumentFragment.h \
>    DocumentType.h \
> +  Attr.h \

Keep sorted

::: content/base/src/nsDOMAttributeMap.cpp
@@ +34,5 @@
>  /**
>   * Clear map pointer for attributes.
>   */
>  PLDHashOperator
> +RemoveMapRef(nsAttrHashKey::KeyType aKey, nsRefPtr<dom::Attr>& aData,

Drop the dom:: in this file

::: content/base/src/nsDOMAttributeMap.h
@@ +23,5 @@
>  
>  namespace mozilla {
>  namespace dom {
>  class Element;
> +class Attr;

Sort

	Andrea Marchesini [:baku] Assignee
	Comment 9 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=a44f89edb9ba maybe this is green...

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 11 • 11 years ago

You'll need some changes to dom/imptests/failures/webapps/DOMCore/tests/approved/test_interfaces.html.json too, it seems.

	Masatoshi Kimura [:emk]
	Comment 12 • 11 years ago

Comment on attachment 734209 [details] [diff] [review]
part 2 - webidl

>diff --git a/dom/webidl/Attr.webidl b/dom/webidl/Attr.webidl
>--- a/dom/webidl/Attr.webidl
>+++ b/dom/webidl/Attr.webidl
>@@ -5,16 +5,24 @@
>-interface Attr {
>+interface Attr : Node {
>   readonly attribute DOMString name;
>+
>+           [SetterThrows]
>            attribute DOMString value;
> 
>+  readonly attribute boolean specified;
>+
>   readonly attribute DOMString? namespaceURI;
>   readonly attribute DOMString? prefix;
>   readonly attribute DOMString localName;
>+
>+  // Introduced in DOM Level 2:
>+           [GetterThrows]
>+  readonly attribute Element? ownerElement;

Add these properties as Mozilla extensions.
DOM4 dropped many Node-derived features.
http://dom.spec.whatwg.org/#interface-attr
See also bug 611787.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 13 • 11 years ago

Yes. That's unrelated to this bug, though.

	Andrea Marchesini [:baku] Assignee
	Comment 14 • 11 years ago

I changed something in dom/imptests/webapps/DOMCore/tests/approved/test_interfaces.html and marked 2 attributes as mozilla extensions.

	Andrea Marchesini [:baku] Assignee
	Comment 15 • 11 years ago

Forgot a ';' :)

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 16 • 11 years ago

I did mean dom/imptests/failures/webapps/DOMCore/tests/approved/test_interfaces.html.json, not dom/imptests/webapps/DOMCore/tests/approved/test_interfaces.html.

	Andrea Marchesini [:baku] Assignee
	Comment 17 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=05b98cfddb7e green on try.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 18 • 11 years ago

Comment on attachment 734457 [details] [diff] [review]
part 2 - webidl

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

Ship it.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 19 • 11 years ago

Just make sure that you don't land the first part with "Rename nsDOMAttribute to DOMAttribute" as the commit message.

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 20 • 11 years ago

I pushed to try to see if anything broke in the meantime: https://tbpl.mozilla.org/?tree=Try&rev=58fbdab519a2

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb57393cb0d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc65fd307477

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/bb57393cb0d4
https://hg.mozilla.org/mozilla-central/rev/dc65fd307477

	Boris Zbarsky [:bzbarsky]
	Comment 26 • 11 years ago

This changed the set of properties available on Attr objects.  That change should be documented.

Andrea, would you mind writing up a summary of what changed here, please?

	Andrea Marchesini [:baku] Assignee
	Comment 27 • 11 years ago

This code introduces these new properties on Attr objects:

. readonly attribute DOMString localName
the local attribute name.

. readonly attribute DOMString? namespaceURI;
The namespaceURI attribute must return the namespace.

. readonly attribute DOMString? prefix;
The prefix attribute must return the namespace prefix.

Then 2 mozilla-only properties:
. readonly attribute boolean specified;

. readonly attribute Element? ownerElement;
The parent element.

BTW, isId is gone... and I don't know if this is what we want. isId was introduced by Bug 71783
bz, should we introduce it again?

	David Bruant
	Comment 28 • 11 years ago

From the diff [1] I see that Attr moved from not inherting from Node, to inheriting back. This seems to go against bug 611787 as well as the latest spec [2]. Why did this change occur?

Regarding Mozilla-only extensions, are they here because they're necessary for web compat? If so, are there standards bugs?


[1] https://hg.mozilla.org/mozilla-central/diff/dc65fd307477/dom/webidl/Attr.webidl#l1.14
[2] http://dom.spec.whatwg.org/#interface-attr

	:Ms2ger (he/him; ⌚ UTC+1/+2) Reporter
	Comment 29 • 11 years ago

You're misreading the diff; Attr.webidl used to have Attr not inheriting from Node, but Attr.webidl was not being used by anything. The goal in this bug was preserving the behaviour that we had with XPConnect bindings / nsIDOMAttr.idl. Same for the Mozilla extensions; they were kept, not added. The web compat constraints are not clear yet, so we'll try figuring those out at some other time.