1175810 - Remove PL_DHashTableEnumerate() use from dom/

Status: RESOLVED FIXED

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

Description

[Nicholas Nethercote [inactive]]

Because PLDHashTable::Iterator is nicer than PL_DHashTableEnumerate().

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove PL_DHashTableEnumerator() use from nsScriptNameSpaceManager

PLDHashTable::Iterator is so much better. Lots of plumbing removed here.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove PL_DHashTableEnumerate() uses from nsJSNPRuntime

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I have quite a lot of review backlog these days, and not that much time for reviews, would be great if you could assign this to another dom peer.

Comment 4

[Andrew McCreight (out of office until 8/21) [:mccr8]]

bz, mrbkap and bholley have fairly short review queues, FWIW.

Comment 5

[Nicholas Nethercote [inactive]]

(In reply to Jonas Sicking (:sicking) from comment #3)
> I have quite a lot of review backlog these days, and not that much time for
> reviews, would be great if you could assign this to another dom peer.

FWIW, when I uploaded the patches I looked at review queue lengths and yours was only 2, which was one of the lowest of the DOM peers. Oh well.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8624042 [details] [diff] [review]
(part 1) - Remove PL_DHashTableEnumerator() use from nsScriptNameSpaceManager

It really doesn't make me happy to give non-const access to the GlobalNameMapEntry to consumers here.  Is there a way to do that in the new iterator setup?  If not, should we add something to allow that?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8624043 [details] [diff] [review]
(part 2) - Remove PL_DHashTableEnumerate() uses from nsJSNPRuntime

r=me

Comment 8

[Nicholas Nethercote [inactive]]

> It really doesn't make me happy to give non-const access to the
> GlobalNameMapEntry to consumers here.  Is there a way to do that in the new
> iterator setup?  If not, should we add something to allow that?

Hmm, interesting point. Probably the best thing would be to create a wrapper type for PLDHashTable::Iterator that only gives read-only access. I'll try doing that.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8624042 [details] [diff] [review]
(part 1) - Remove PL_DHashTableEnumerator() use from nsScriptNameSpaceManager

OK, canceling review for now pending the const goodness.  ;)

Comment 10

[Nicholas Nethercote [inactive]]

(In reply to Boris Zbarsky [:bz] from comment #6)
> Comment on attachment 8624042 [details] [diff] [review]
> (part 1) - Remove PL_DHashTableEnumerator() use from nsScriptNameSpaceManager
> 
> It really doesn't make me happy to give non-const access to the
> GlobalNameMapEntry to consumers here.  Is there a way to do that in the new
> iterator setup?  If not, should we add something to allow that?

bz, would it satisfy you if PLDHashTable::Iterator::Get() returned a |const PLDHashEntryHdr*| instead of a |PLDHashEntryHdr*|? It wouldn't be ironclad because people could always cast around it, but I'm wondering if that would be a good change in general. (PLDHashTable::RemovingIterator::Get() would still return a non-const |PLDHashEntryHdr*|).

An iterator wrapper would be able to give stronger guarantees, though.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> would it satisfy you if PLDHashTable::Iterator::Get() returned a |const PLDHashEntryHdr*|
> instead of a |PLDHashEntryHdr*|?

Yes.  I mean, people can cast around the current setup in namespace manager too; it's just an immediate smell if they do.

That said, other cases may want to iterate a hashtable and update all the entries.  It's just in this particular namespace manager case that the hashtable is conceptually all readonly.

Comment 12

[Nicholas Nethercote [inactive]]

> That said, other cases may want to iterate a hashtable and update all the
> entries.  It's just in this particular namespace manager case that the
> hashtable is conceptually all readonly.

Given the improved PLDHashTable sanity-checking that I'm working on in bug 1131308, I think the const change makes sense -- you're allowed to have multiple Iterators running concurrently, which doesn't make much sense if the entries are being modified.

If someone wants non-read-only access they can use PLDHashTable::RemovingIterator(), even if they don't remove any items. (Maybe it should be renamed ModifyingIterator().)

Thank you for the quick reviews and replies.

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove PL_DHashTableEnumerator() use from nsScriptNameSpaceManager

bz, this is much the same as the previous version you looked at, but now this
patch applies on top of the patches in bug 1179071 which convert
PLDHashTable::Iterator::Get() to return a const pointer.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8628060 [details] [diff] [review]
(part 1) - Remove PL_DHashTableEnumerator() use from nsScriptNameSpaceManager

r=me, though it might have been nice to have a subclass of PLDHashTable::Iterator here whose Get() returns the right type to start with (by calling PLDHashTable::Iterator::Get() and casting) instead of having consumers cast.

Comment 15

[Nicholas Nethercote [inactive]]

> r=me, though it might have been nice to have a subclass of
> PLDHashTable::Iterator here whose Get() returns the right type to start with
> (by calling PLDHashTable::Iterator::Get() and casting) instead of having
> consumers cast.

I considered that here and in some other, similar places. It would have required a bunch of extra boilerplate code just to avoid a couple of inline casts and so I decided against it.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fe1710527c0e

Comment 17

[Nicholas Nethercote [inactive]]

(In reply to Pulsebot from comment #16)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/fe1710527c0e

Part 2 landed; part 1 is still waiting on bug 1179071.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fe1710527c0e

Comment 19

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove PL_DHashTableEnumerator() use from nsScriptNameSpaceManager

bz, apologies for r?ing you a third time. Due to uncertainty in bug 1179071 I
just went ahead and made an iterator class that returns const pointers in
Get(), like you suggested in comment 14.

Comment 20

[Nicholas Nethercote [inactive]]

The interdiff for the latest version of part 1 works well.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8629167 [details] [diff] [review]
(part 1) - Remove PL_DHashTableEnumerator() use from nsScriptNameSpaceManager

r=me

Comment 22

[Nicholas Nethercote [inactive]]

Thank you for the fast review.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/117d439135fb

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/117d439135fb