If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Convert DOMStringList to WebIDL

RESOLVED FIXED in mozilla30

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bz, Assigned: peterv)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla30
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

Comment hidden (empty)

Comment 1

5 years ago
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

Comment 3

5 years ago
Ah, that was DOMStringMap, right? 

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

Updated

5 years ago
Blocks: 798875

Comment 4

4 years ago
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)

Comment 5

4 years ago
Created attachment 808275 [details] [diff] [review]
patch for sync idb
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

4 years ago
(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)

Comment 8

4 years ago
Created attachment 808763 [details] [diff] [review]
Convert DOMStringList to WebIDL bindings - move files and rename class v1
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Flags: needinfo?(peterv)
(Assignee)

Comment 9

4 years ago
Created attachment 808764 [details] [diff] [review]
Convert DOMStringList to WebIDL bindings - add WebIDL API and switch v1
(Assignee)

Updated

4 years ago
Depends on: 923054
Blocks: 952890
(Assignee)

Comment 10

4 years ago
Created attachment 8365072 [details] [diff] [review]
Move files and rename class v2
Attachment #8365072 - Flags: review?(bzbarsky)
(Assignee)

Comment 11

4 years ago
Created attachment 8365074 [details] [diff] [review]
Switch DOMStringList to WebIDL v2
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

r=me
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
>+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.
Attachment #8365074 - Flags: review?(bzbarsky) → review+

Updated

4 years ago
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.
Created attachment 8382457 [details] [diff] [review]
Interdiff addressing my review comments
Created attachment 8382458 [details] [diff] [review]
Part 2 with the review comments addressed
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.
   https://hg.mozilla.org/integration/mozilla-inbound/rev/d4040ae9a5ed
   https://hg.mozilla.org/integration/mozilla-inbound/rev/95fd86027306
Flags: in-testsuite+
Target Milestone: --- → mozilla30
https://hg.mozilla.org/mozilla-central/rev/d4040ae9a5ed
https://hg.mozilla.org/mozilla-central/rev/95fd86027306
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Depends on: 978646
You need to log in before you can comment on or make changes to this bug.