Add the new DOM bindings API to DOM lists

RESOLVED FIXED in mozilla18

Status

()

RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: peterv, Assigned: peterv)

Tracking

(Blocks: 1 bug)

Trunk
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
Created attachment 658510 [details] [diff] [review]
Rename Length to LengthNoFlush in SVG DOM lists
Attachment #658510 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

6 years ago
Created attachment 658511 [details] [diff] [review]
Rename nsIDOMHTMLCollection::GetNodeAt to GetElementAt
Attachment #658511 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

6 years ago
Created attachment 658512 [details] [diff] [review]
Add the API to hook up DOM lists to the new bindings v1
Attachment #658512 - Flags: review?(bzbarsky)
(Assignee)

Comment 4

6 years ago
Comment on attachment 658512 [details] [diff] [review]
Add the API to hook up DOM lists to the new bindings v1

There's still some issues with this.
Attachment #658512 - Attachment is obsolete: true
Attachment #658512 - Flags: review?(bzbarsky)
(Assignee)

Comment 5

6 years ago
Created attachment 658612 [details] [diff] [review]
Add the API to hook up DOM lists to the new bindings v1
Attachment #658612 - Flags: review?(bzbarsky)
Comment on attachment 658510 [details] [diff] [review]
Rename Length to LengthNoFlush in SVG DOM lists

r=me
Attachment #658510 - Flags: review?(bzbarsky) → review+
Comment on attachment 658511 [details] [diff] [review]
Rename nsIDOMHTMLCollection::GetNodeAt to GetElementAt

Would Element* make more sense than nsGenericElement*?

r=me with that.
Attachment #658511 - Flags: review?(bzbarsky) → review+
Comment on attachment 658612 [details] [diff] [review]
Add the API to hook up DOM lists to the new bindings v1

>+++ b/content/base/src/nsContentList.cpp
>+nsContentList::NamedItem(JSContext* cx, const nsAString& name,

The fact that this has to return "object" is really annoying.  We should really make that better somehow...

>+  JS::Value v;
>+  if (!mozilla::dom::WrapObject(cx, GetWrapper(), item, item, nullptr, &v)) {

I think you need to enter GetWrapper()'s compartment first, in cases when cx is not in that compartment already.  See bug 763225 for a similar situation where that problem bit us.

>+++ b/content/html/content/src/HTMLPropertiesCollection.cpp
>+HTMLPropertiesCollection::NamedItem(JSContext* cx, const nsAString& name,
>+                                    mozilla::ErrorResult& error)
>+{
>+  return nullptr;

You should probably document that we overrid NameGetter and don't actually call this method from our NameGetter.

>+HTMLPropertiesCollection::NamedItem(const nsAString& aName)
>+    propertyList = new PropertyNodeList(this, mRoot, aName);
>+    mNamedItemEntries.Put(aName, propertyList);

I would rather we held a ref before passing it off to code that will refcount it...  Please add a strong ref here?

>+PropertyNodeList::GetValues(JSContext* aCx, nsTArray<JS::Value >& aResult,
>+    JS::Value v = mElements.ElementAt(i)->GetItemValue(aCx, GetWrapper(),

Again, we need to enter GetWrapper()'s compartment on aCx.  :(

>+++ b/content/html/content/src/nsGenericHTMLElement.cpp
>+nsGenericHTMLElement::GetItemValue(JSContext* aCx, JSObject* aScope,
>+    return JSVAL_NULL;

JS::NullValue()

>+      return JSVAL_VOID;
>+    return JSVAL_VOID;

JS::UndefinedValue()

So far, up to the end of nsGenericElement.h.  More probably on Sunday.
(Assignee)

Comment 9

6 years ago
(In reply to Boris Zbarsky (:bz) from comment #7)
> Would Element* make more sense than nsGenericElement*?

The problem is that I've made the Element DOM binding have nsGenericElement as the native, since that's where most of that API is implemented. And since HTMLCollection returns Element (the DOM interface) this then needs to return nsGenericElement.
> The problem is that I've made the Element DOM binding have nsGenericElement as the native,

Yeah, I'd been hoping to make that be Element too, and move things to Element as needed...  Maybe it's not worth it for now, but we should really think about it.
(Assignee)

Comment 11

6 years ago
(In reply to Boris Zbarsky (:bz) from comment #8)
> >+nsContentList::NamedItem(JSContext* cx, const nsAString& name,
> 
> The fact that this has to return "object" is really annoying.  We should
> really make that better somehow...

We could make it return a union of Node and NodeList, but we don't support union return types yet. BTW, I'm not sure why HTMLPropertiesCollection overrides namedItem, given that HTMLCollection's already returns |object| which should also cover |PropertyNodeList?|?
(Assignee)

Comment 12

6 years ago
(In reply to Boris Zbarsky (:bz) from comment #10)
> Yeah, I'd been hoping to make that be Element too, and move things to
> Element as needed...  Maybe it's not worth it for now, but we should really
> think about it.

I'm not sure what the point is, wouldn't it make more sense to merge nsGenericElement and Element at some point? If we want to call concrete functions from the Element binding we'd have to move a lot of stuff to Element, which would make nsGenericElement pretty empty.
> BTW, I'm not sure why HTMLPropertiesCollection overrides namedItem

Dunno.  It certainly defines totally different behavior for it than HTMLCollection has (e.g. the "never returns null" thing)...

> I'm not sure what the point is

It used to be to not have to expose the include hell that is nsGenericElement.h all over the place.  Maybe that's gotten better, or at least Element has gotten worse with the FragmentOrElement thing?

I'd probably be OK merging nsGenericElement and Element, if we can solve the include hell issues.
>+++ b/content/html/content/src/nsHTMLFormElement.cpp
>+nsFormControlList::NamedItem(JSContext* cx, const nsAString& name,
>+  if (!mozilla::dom::WrapObject(cx, GetWrapper(), item, &v)) {

Again, have to enter the right compartment.

>+++ b/content/html/content/src/nsHTMLSelectElement.cpp

>+nsHTMLOptionCollection::NamedItem(JSContext* cx, const nsAString& name,
>+  if (!mozilla::dom::WrapObject(cx, GetWrapper(), item, item, nullptr, &v)) {

And again, compartment gunk.

>+nsHTMLOptionCollection::Remove(int32_t aIndex, ErrorResult& aError)
>+  mSelect->Remove(aIndex);

  aError = mSelect->Remove(aIndex);

>+++ b/content/html/content/src/nsHTMLSelectElement.h
>+  void SetOption(uint32_t aIndex, nsIDOMHTMLOptionElement *aOption,
>+                 mozilla::ErrorResult& aError)

Hmm.  Why is this needed?  The only caller is the IndexedSetter, right?  And that can just call the other SetOption() signature directly...

>+++ b/content/html/content/src/nsHTMLTableElement.cpp
>+TableRowsCollection::NamedItem(JSContext* cx, const nsAString& name,
>+      if (!mozilla::dom::WrapObject(cx, GetWrapper(), item, cache, nullptr,

Again, compartment gunk.

I wonder whether we should pass in the "obj" from the glue code automatically when the return value is "object"....

>+++ b/content/svg/content/src/DOMSVGLengthList.h
>+  nsIDOMSVGLength* GetItem(uint32_t index, ErrorResult& error)

Huh.  Throwing when !found here is pretty bizarre, but I guess we used to do that...  Might be worth a bug report on this, because the spec doesn't seem to say anything of the sort.

>+++ b/content/svg/content/src/DOMSVGNumberList.h

I find myself wishing these SVG list classes were just templates instead of copy/pasted.... ;)

>+++ b/content/svg/content/src/DOMSVGPathSegList.cpp
>+DOMSVGPathSegList::Initialize(nsIDOMSVGPathSeg *aNewItem, ErrorResult& aError)
>-  Clear();
>+  Clear(aError);

This sorta changes behavior.  Errors from Clear() used to be ignored.  Not that Clear can actually return an error when we get here...

Anyway, either explicitly ignore the error from Clear(), or MOZ_ASSERT that !aError.Failed() after that call.

>+++ b/content/svg/content/src/DOMSVGTransformList.cpp
>+DOMSVGTransformList::Initialize(nsIDOMSVGTransform *newItem, ErrorResult& error)
>-  Clear();
>+  Clear(error);

Same thing here.

>+DOMSVGTransformList::Consolidate(ErrorResult& error)
>-  Clear();
>+  Clear(error);

And same here.

>+++ b/dom/base/nsDOMClassInfo.cpp
>+    nsIHTMLCollection* htmlCollection;
>+    rv = mozilla::dom::UnwrapObject<nsIHTMLCollection>(cx, obj, htmlCollection);

So this seems to depend on using the Paris bindings for the collections.  But this patch doesn't seem to enable those, does it?  What am I missing?

>+++ b/dom/bindings/Bindings.conf
>+'HTMLPropertiesCollection': [
>+    'nativeType': 'mozilla::dom::HTMLPropertiesCollection',

You shouldn't need this.  That's the default value for that interface name.

>+'PropertyNodeList': [
>+    'nativeType': 'mozilla::dom::PropertyNodeList',

And here

>+++ b/dom/webidl/HTMLOptionsCollection.webidl
>+  legacycaller getter object? namedItem(DOMString name);

We don't implement the legacycaller stuff.  And I'm still not convinced we shoud.  Please just nix it from the IDL so it'll be obvious that we're not doing it for now?

>+++ b/dom/webidl/HTMLPropertiesCollection.webidl
>+  legacycaller getter PropertyNodeList? namedItem(DOMString name); // overrides inherited namedItem()

And here.

>+//typedef sequence<any> PropertyValueArray;
...
>+  sequence<any> getValues();

You can reinstate the typedef now.  It should work!

>+++ b/dom/webidl/Storage.webidl
>+++ b/dom/webidl/StorageObsolete.webidl

These don't seem to be used.  Do we really want to add them?

Why do we need all those nsContentList.h includes in frame code?  Might want to document...

r=me with the above fixed.
Comment on attachment 658612 [details] [diff] [review]
Add the API to hook up DOM lists to the new bindings v1

Ugh.  I totally failed to mark the r+ on Monday.  :(
Attachment #658612 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky (:bz) from comment #13)
> > I'm not sure what the point is
> 
> It used to be to not have to expose the include hell that is
> nsGenericElement.h all over the place.  Maybe that's gotten better, or at
> least Element has gotten worse with the FragmentOrElement thing?
> 
> I'd probably be OK merging nsGenericElement and Element, if we can solve the
> include hell issues.

Didn't I solve those at some point?
Maybe?  At least for inside libxul we might be ok now...
(Assignee)

Comment 18

6 years ago
Created attachment 661748 [details] [diff] [review]
Add the API to hook up DOM lists to the new bindings v1.1

(In reply to Boris Zbarsky (:bz) from comment #8)
> I think you need to enter GetWrapper()'s compartment first, in cases when cx
> is not in that compartment already.  See bug 763225 for a similar situation
> where that problem bit us.

Done.

> You should probably document that we overrid NameGetter and don't actually
> call this method from our NameGetter.

Done.

> I would rather we held a ref before passing it off to code that will
> refcount it...  Please add a strong ref here?

Done.

> Again, we need to enter GetWrapper()'s compartment on aCx.  :(

Done.

> JS::NullValue()
> JS::UndefinedValue()

Done.

> Again, have to enter the right compartment.

Done.

> And again, compartment gunk.

Done.

>   aError = mSelect->Remove(aIndex);

Done.

> Hmm.  Why is this needed?  The only caller is the IndexedSetter, right?  And
> that can just call the other SetOption() signature directly...

Removed.

> Again, compartment gunk.

Removed.

> I wonder whether we should pass in the "obj" from the glue code
> automatically when the return value is "object"....

I'll file a bug to sort all of this out.

> Huh.  Throwing when !found here is pretty bizarre, but I guess we used to do
> that...  Might be worth a bug report on this, because the spec doesn't seem
> to say anything of the sort.

Will file.

> I find myself wishing these SVG list classes were just templates instead of
> copy/pasted.... ;)

You and me both.

> Anyway, either explicitly ignore the error from Clear(), or MOZ_ASSERT that
> !aError.Failed() after that call.

Done the second.

> Same thing here.

Done.

> And same here.

Done.

> So this seems to depend on using the Paris bindings for the collections. 
> But this patch doesn't seem to enable those, does it?  What am I missing?

We fall back to nsXPConnect::GetNativeOfWrapper.

> You shouldn't need this.  That's the default value for that interface name.

> And here

Done.

> We don't implement the legacycaller stuff.  And I'm still not convinced we
> shoud.  Please just nix it from the IDL so it'll be obvious that we're not
> doing it for now?

> And here.

Done.

> You can reinstate the typedef now.  It should work!

Done.

> These don't seem to be used.  Do we really want to add them?

Removed.

> Why do we need all those nsContentList.h includes in frame code?  Might want
> to document...

AppendAnonymousContentTo has a |nsBaseContentList&| argument and they usually got the include already through nsGenericElement.h, but I removed that one.
Attachment #658612 - Attachment is obsolete: true
Attachment #661748 - Flags: review+
(Assignee)

Updated

6 years ago
Blocks: 791774
(Assignee)

Updated

5 years ago
Duplicate of this bug: 697440
You need to log in before you can comment on or make changes to this bug.