851470 - Move Attr to Paris bindings

Status: RESOLVED FIXED

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

Comment 1

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

Any chance you would be interested in this? :)

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 4

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

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.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming

Comment 6

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 7

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

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

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

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

Comment 10

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 11

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

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

Comment 12

[Masatoshi Kimura [:emk]]

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.

Comment 13

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

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

Comment 14

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

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

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Forgot a ';' :)

Comment 16

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

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

Comment 17

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

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

Comment 18

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

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

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

Ship it.

Comment 19

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

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

Comment 20

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

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

Comment 21

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming

Comment 22

[Andrea Marchesini [:baku]]

Attachment: part 1 - renaming

Comment 23

[Andrea Marchesini [:baku]]

Attachment: part 2 - webidl

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

[Boris Zbarsky [:bzbarsky]]

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?

Comment 27

[Andrea Marchesini [:baku]]

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?

Comment 28

[David Bruant]

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

Comment 29

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

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.