803665 - convert nsGlobalWindow's timeout list to use mozilla::LinkedList

Status: RESOLVED FIXED

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

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: convert nsGlobalWindow's timeout list to use mozilla::LinkedList

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 673384 [details] [diff] [review]
convert nsGlobalWindow's timeout list to use mozilla::LinkedList

r=me, though seems like we could nix the Next/Prev/FirstTimeout/LastTimeout stuff if we wanted to now that they have sane implementations.

Comment 3

[Nathan Froyd [:froydnj]]

(In reply to Boris Zbarsky (:bz) from comment #2)
> r=me, though seems like we could nix the Next/Prev/FirstTimeout/LastTimeout
> stuff if we wanted to now that they have sane implementations.

Agreed.  I thought it would be easier to do that in a followup patch.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Sure.  Followup is great.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: Bug 803665 - part 2 - dispense with nsGlobalWindow::{First,Last}Timeout and nsTimeout::{Next,Previous}; r=bz

The LinkedList<T> methods work just fine; no need to have wrappers around them.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 673913 [details] [diff] [review]
Bug 803665 - part 2 - dispense with nsGlobalWindow::{First,Last}Timeout and nsTimeout::{Next,Previous}; r=bz

r=me

Comment 7

[Andrew McCreight [:mccr8]]

Yay!  One step closer to sanity for nsTimeout.

Comment 8

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/24d8e0cb73f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb673c95d56b

Comment 9

[Nathan Froyd [:froydnj]]

Attachment: part 0 - make nsTimeout properly initialize its fields

Hunting down some segfaults from failing tests pointed to nsTimeout's bogus
constructor.  By using memset(), it breaks LinkedListElement's invariants.

Let's not do that.  mccr8 has this bit in his patches for bug 774874, too.

Comment 10

[Nathan Froyd [:froydnj]]

Attachment: small fix for inserting timeouts, to be folded

This small patch corrects a conceptual mismatch between prclist and
LinkedList: when we're walking backwards through the list, getting to the end
of the list is represented by a sentinel node in prclist and nullptr in
LinkedList.  So we have to check for that condition specially.

Since we're walking backwards through the list, .insertFront makes sense to
me, and tests seem to work, but asking for review as a sanity check.  This
will get folded into an earlier patch in any event.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 677448 [details] [diff] [review]
part 0 - make nsTimeout properly initialize its fields

r=me

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 677451 [details] [diff] [review]
small fix for inserting timeouts, to be folded

Indeed.  Sorry I didn't catch this.  :(

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to Boris Zbarsky (:bz) from comment #12)
> Comment on attachment 677451 [details] [diff] [review]
> small fix for inserting timeouts, to be folded
> 
> Indeed.  Sorry I didn't catch this.  :(

No worries!  My job to actually run tests, not yours.

Re-landed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e32c3c7c89
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6bd66fc8988
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3a6bc0d9df5

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e2e32c3c7c89
https://hg.mozilla.org/mozilla-central/rev/b6bd66fc8988
https://hg.mozilla.org/mozilla-central/rev/b3a6bc0d9df5

Comment 15

[Ed Morley [:emorley]]

Unfortunately the inbound push this landed in broke Linux PGO, so push backed out (see bug 809756).

https://hg.mozilla.org/mozilla-central/rev/82c59210f11f

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7f38df5f2448
https://hg.mozilla.org/mozilla-central/rev/00534980e998
https://hg.mozilla.org/mozilla-central/rev/6a9e7aeb184f