803106 - Convert DOMStringList to WebIDL

Status: RESOLVED FIXED

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

Comment 1

[Florian Bender]

As per Bug 802157 Comment 12, I believe this requires WebIDL deleter support. Is there a bug for WebIDL delter support?

Comment 2

[Boris Zbarsky [:bzbarsky]]

DOMStringList doesn't have a deleter.

That said, a bigger problem here is lack of sane parenting options, and the need to refactor our nsIDOMDOMStringList impls to have a useful common base class.

Comment 3

[Florian Bender]

Ah, that was DOMStringMap, right? 

Alright, I'm un-CCing this bug as I don't understand anything of this ;-)

Comment 4

[Jan Varga [:janv]]

Peter, I think you had patches for this, Bent will start looking at sync IDB implementation soon and we need to do something with DOMStringList before landing. I'll attach a patch here that we use at the moment for sync IDB.

Comment 5

[Jan Varga [:janv]]

Attachment: patch for sync idb

Comment 6

[Boris Zbarsky [:bzbarsky]]

Why do we need a separate worker implementation here?  Why can't we just use the normal DOMStringList on workers now that we have CC there?

Comment 7

[Jan Varga [:janv]]

(In reply to Boris Zbarsky [:bz] from comment #6)
> Why do we need a separate worker implementation here?  Why can't we just use
> the normal DOMStringList on workers now that we have CC there?

Yes, we can. I just attached what we have now for sync IDB implementation and peterv mentioned to me some time ago that he had patches for converting DOMStringList to WebIDL on main thread.

IDBKeyRange has a separate worker implementation too, but I'm working on using just one, but we need to convert main thread implementation to WebIDL first, bug 832883.

Comment 8

[Peter Van der Beken [:peterv]]

Attachment: Convert DOMStringList to WebIDL bindings - move files and rename class v1

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: Convert DOMStringList to WebIDL bindings - add WebIDL API and switch v1

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: Move files and rename class v2

Comment 11

[Peter Van der Beken [:peterv]]

Attachment: Switch DOMStringList to WebIDL v2

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8365072 [details] [diff] [review]
Move files and rename class v2

r=me

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8365074 [details] [diff] [review]
Switch DOMStringList to WebIDL v2

>+  const nsTArray<nsString>& Names() const

Is this used?  I don't see any users...

>+  virtual void EnsureFresh()

Please document.

>+++ b/content/base/src/nsDocument.cpp
>+nsDOMStyleSheetSetList::EnsureFresh()
>+    return; // Spec says "no exceptions", and we have no style sets if we have
>+            // no document, for sure

Should we clear mNames?  That would preserve the old behavior...

>+    if (!title.IsEmpty() && !mNames.Contains(title) && !Add(title)) {

Or maybe we should be clearing mNames up front in this function?  Otherwise we need code here to _remove_ things from mNames, which sounds complicated.

>+++ b/content/html/content/src/HTMLPropertiesCollection.cpp
>-      // ContainsInternal must not call EnsureFresh

Why remove the comment?

>+++ b/content/html/content/src/HTMLPropertiesCollection.h
>+  virtual void EnsureFresh();

Can be protected, right?

>+++ b/dom/base/nsDOMClassInfo.cpp

I believe nsStringArraySH is now dead code.  Nuke it too?

DOMStringList::mNames is an infallible array.  Should it be fallible instead, so the return value of Add() will actually mean something?

r=me with those fixed.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Peter, can we get this landed?  This is blocking bug 952890...

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Why remove the comment?

To answer my own question: because the callee has a comment too, and that's the right place to comment it.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Should it be fallible instead,

Makes the IDB code a pain.  Will leave it for now, pending bug 968520 being fixed.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff addressing my review comments

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 with the review comments addressed

Comment 19

[Boris Zbarsky [:bzbarsky]]

I'm getting unified build failures with this patch because wingdi.h is redefining a macro that nsStyleLinkElement also defines... presumably because the different position of our cpp in the list moves things around.

Going to see if I can find a fix for that.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Filed bug 977553 on that.

Comment 21

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/d4040ae9a5ed
   https://hg.mozilla.org/integration/mozilla-inbound/rev/95fd86027306

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d4040ae9a5ed
https://hg.mozilla.org/mozilla-central/rev/95fd86027306