Closed Bug 803106 Opened 12 years ago Closed 10 years ago

Convert DOMStringList to WebIDL


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

Not set





(Reporter: bzbarsky, Assigned: peterv)


(Blocks 1 open bug)



(3 files, 4 obsolete files)

      No description provided.
As per Bug 802157 Comment 12, I believe this requires WebIDL deleter support. Is there a bug for WebIDL delter support?
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.
Assignee: bzbarsky → nobody
Ah, that was DOMStringMap, right? 

Alright, I'm un-CCing this bug as I don't understand anything of this ;-)
Blocks: SyncIDB
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.
Flags: needinfo?(peterv)
Attached patch patch for sync idb (obsolete) — Splinter Review
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?
(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.
Assignee: nobody → peterv
Flags: needinfo?(peterv)
Depends on: 923054
Blocks: 952890
Attachment #8365072 - Flags: review?(bzbarsky)
Attachment #808763 - Attachment is obsolete: true
Attachment #808764 - Attachment is obsolete: true
Attachment #8365074 - Flags: review?(bzbarsky)
Comment on attachment 8365072 [details] [diff] [review]
Move files and rename class v2

Attachment #8365072 - Flags: review?(bzbarsky) → review+
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
>+    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.
Attachment #8365074 - Flags: review?(bzbarsky) → review+
Attachment #808275 - Attachment is obsolete: true
Peter, can we get this landed?  This is blocking bug 952890...
Flags: needinfo?(peterv)
> 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.
Flags: needinfo?(peterv)
> Should it be fallible instead,

Makes the IDB code a pain.  Will leave it for now, pending bug 968520 being fixed.
Attachment #8365074 - Attachment is obsolete: true
Assignee: peterv → bzbarsky
Assignee: bzbarsky → peterv
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.
Depends on: 977553
Filed bug 977553 on that.
Depends on: 978646
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.