Closed Bug 851470 Opened 8 years ago Closed 8 years ago

Move Attr to Paris bindings

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: Ms2ger, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(2 files, 11 obsolete files)

No description provided.
Any chance you would be interested in this? :)
Assignee: nobody → amarchesini
Attached patch part 1 - renaming (obsolete) — Splinter Review
Attachment #732742 - Flags: review?(Ms2ger)
Attached patch part 2 - webidl (obsolete) — Splinter Review
Attachment #732743 - Flags: review?(Ms2ger)
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.
Attachment #732742 - Flags: review?(Ms2ger) → review-
Attached patch part 1 - renaming (obsolete) — Splinter Review
Attachment #732742 - Attachment is obsolete: true
Attachment #732774 - Flags: review?(Ms2ger)
Attached patch part 2 - webidl (obsolete) — Splinter Review
Attachment #732743 - Attachment is obsolete: true
Attachment #732743 - Flags: review?(Ms2ger)
Attachment #732775 - Flags: review?(Ms2ger)
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
Attachment #732774 - Flags: review?(Ms2ger) → review+
Attached patch part 1 - renaming (obsolete) — Splinter Review
Attachment #732774 - Attachment is obsolete: true
Attachment #733892 - Flags: review+
Attached patch part 2 - webidl (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=a44f89edb9ba maybe this is green...
Attachment #732775 - Attachment is obsolete: true
Attachment #732775 - Flags: review?(Ms2ger)
Attachment #733893 - Flags: review?(Ms2ger)
Attached patch part 2 - webidl (obsolete) — Splinter Review
Attachment #733893 - Attachment is obsolete: true
Attachment #733893 - Flags: review?(Ms2ger)
Attachment #734209 - Flags: review?(Ms2ger)
You'll need some changes to dom/imptests/failures/webapps/DOMCore/tests/approved/test_interfaces.html.json too, it seems.
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.
Yes. That's unrelated to this bug, though.
Attached patch part 2 - webidl (obsolete) — Splinter Review
I changed something in dom/imptests/webapps/DOMCore/tests/approved/test_interfaces.html and marked 2 attributes as mozilla extensions.
Attachment #734209 - Attachment is obsolete: true
Attachment #734209 - Flags: review?(Ms2ger)
Attachment #734279 - Flags: review?(Ms2ger)
Attached patch part 2 - webidl (obsolete) — Splinter Review
Forgot a ';' :)
Attachment #734279 - Attachment is obsolete: true
Attachment #734279 - Flags: review?(Ms2ger)
Attachment #734281 - Flags: review?(Ms2ger)
I did mean dom/imptests/failures/webapps/DOMCore/tests/approved/test_interfaces.html.json, not dom/imptests/webapps/DOMCore/tests/approved/test_interfaces.html.
Attached patch part 2 - webidl (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=05b98cfddb7e green on try.
Attachment #734281 - Attachment is obsolete: true
Attachment #734281 - Flags: review?(Ms2ger)
Attachment #734457 - Flags: review?(Ms2ger)
Comment on attachment 734457 [details] [diff] [review]
part 2 - webidl

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

Ship it.
Attachment #734457 - Flags: review?(Ms2ger) → review+
Just make sure that you don't land the first part with "Rename nsDOMAttribute to DOMAttribute" as the commit message.
I pushed to try to see if anything broke in the meantime: https://tbpl.mozilla.org/?tree=Try&rev=58fbdab519a2
Attached patch part 1 - renaming (obsolete) — Splinter Review
Attachment #733892 - Attachment is obsolete: true
Attachment #735203 - Attachment is obsolete: true
Attached patch part 2 - webidlSplinter Review
Attachment #734457 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/bb57393cb0d4
https://hg.mozilla.org/mozilla-central/rev/dc65fd307477
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Depends on: 865008
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?
Keywords: dev-doc-needed
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?
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
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.
Depends on: 1038243
You need to log in before you can comment on or make changes to this bug.