Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators

RESOLVED FIXED in Firefox 45

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

Attachments

(14 attachments, 2 obsolete attachments)

10.09 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
3.27 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
5.73 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
3.93 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
3.04 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
1.90 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
4.88 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
5.56 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
4.69 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
5.21 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
3.30 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
3.47 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
4.71 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
5.07 KB, patch
khuey
: review+
njn
: checkin+
Details | Diff | Splinter Review
(Assignee)

Description

3 years ago
Because iterators are so much nicer than enumerate functions.

There are nine occurrences of EnumerateRead() in these directories.

A note to the assignee: to preserve existing behaviour, you should probably use
nsBaseHashtable::Iterator::UserData() rather than nsBaseHashtable::Iterator::Data(). (The latter should be used when replacing nsBaseHashtable::Enumerate()).
(Assignee)

Comment 1

3 years ago
> There are nine occurrences of EnumerateRead() in these directories.

There are now a bunch more because some of the patches in bug 1186780 were backed out.
(Assignee)

Comment 2

3 years ago
Created attachment 8679261 [details] [diff] [review]
(part 1) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators
Attachment #8679261 - Flags: review?(khuey)
(Assignee)

Updated

3 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 3

3 years ago
Created attachment 8679262 [details] [diff] [review]
(part 2) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators
Attachment #8679262 - Flags: review?(khuey)
(Assignee)

Comment 4

3 years ago
Created attachment 8679263 [details] [diff] [review]
(part 3) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators
Attachment #8679263 - Flags: review?(khuey)
(Assignee)

Comment 5

3 years ago
Created attachment 8679264 [details] [diff] [review]
(part 5) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators
Attachment #8679264 - Flags: review?(khuey)
(Assignee)

Comment 6

3 years ago
Created attachment 8679265 [details] [diff] [review]
(part 4) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators
Attachment #8679265 - Flags: review?(khuey)
Comment on attachment 8679262 [details] [diff] [review]
(part 2) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators

Review of attachment 8679262 [details] [diff] [review]:
-----------------------------------------------------------------

I think this would be nicer if we made a DisconnectGroupMessageManagers and called it from both places.
Attachment #8679262 - Flags: review?(khuey) → review-
(Assignee)

Comment 8

3 years ago
Created attachment 8679781 [details] [diff] [review]
(part 2) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators

New version, as requested.
Attachment #8679781 - Flags: review?(khuey)
(Assignee)

Updated

3 years ago
Attachment #8679262 - Attachment is obsolete: true
Comment on attachment 8679264 [details] [diff] [review]
(part 5) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators

Review of attachment 8679264 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDocument.cpp
@@ +7636,5 @@
> +
> +      // This non-standard style of iteration is presumably used because some
> +      // of the code in the loop body can trigger element removal, which
> +      // invalidates the iterator.
> +      while (true) {

Unfortunately I can't actually look up blame because they blew up the CVS server ...

but yes, I assume that is the concern here.  The loop obviously never exits if BlastSubtreeToPieces and UnsetAttr don't end up removing the attribute from the map.
Attachment #8679264 - Flags: review?(khuey) → review+
(Assignee)

Updated

3 years ago
Keywords: leave-open
(Assignee)

Comment 10

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/14b02d1a0b2bc79e4804e12f029d477af98a22f7
Bug 1187782 (part 1) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators. r=khuey.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c46fb7b4d2a956d5c277c282bcbf0f3cec17012a
Bug 1187782 (part 2) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators. r=khuey.

https://hg.mozilla.org/integration/mozilla-inbound/rev/78202fd2b4d525ceb48a759492df86eca1669f2a
Bug 1187782 (part 3) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators. r=khuey.

https://hg.mozilla.org/integration/mozilla-inbound/rev/40e5654d4bdd3ed4939de46a7ad89288285c6039
Bug 1187782 (part 4) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators. r=khuey.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1e2e66bf01dbbf7d5018185e925d975aa6189786
Bug 1187782 (part 5) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators. r=khuey.
(Assignee)

Comment 11

3 years ago
The abandoned cases in bug 1186780 still need to be converted.
(Assignee)

Comment 13

3 years ago
Created attachment 8682819 [details] [diff] [review]
(part 6) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators
Attachment #8682819 - Flags: review?(khuey)
(Assignee)

Comment 14

3 years ago
Created attachment 8683456 [details] [diff] [review]
(part 7) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators
Attachment #8683456 - Flags: review?(khuey)
(Assignee)

Comment 15

3 years ago
Created attachment 8683457 [details] [diff] [review]
(part 8) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators
Attachment #8683457 - Flags: review?(khuey)
(Assignee)

Comment 16

3 years ago
Created attachment 8683490 [details] [diff] [review]
(part 8) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators

(Fixed a compile error.)
Attachment #8683490 - Flags: review?(khuey)
(Assignee)

Updated

3 years ago
Attachment #8683457 - Attachment is obsolete: true
Attachment #8683457 - Flags: review?(khuey)
(Assignee)

Comment 17

3 years ago
Created attachment 8683493 [details] [diff] [review]
(part 9) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators
Attachment #8683493 - Flags: review?(khuey)
(Assignee)

Updated

3 years ago
Attachment #8679261 - Flags: checkin+
(Assignee)

Updated

3 years ago
Attachment #8679263 - Flags: checkin+
(Assignee)

Updated

3 years ago
Attachment #8679264 - Flags: checkin+
(Assignee)

Updated

3 years ago
Attachment #8679265 - Flags: checkin+
(Assignee)

Updated

3 years ago
Attachment #8679781 - Flags: checkin+
(Assignee)

Comment 18

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/97aab3af2b2d1097e24fb9bb391baa3eb930d7cd
Bug 1187782 (part 6) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators. r=khuey.

https://hg.mozilla.org/integration/mozilla-inbound/rev/22f17b724bda3bd67747a12957cd057eddacce38
Bug 1187782 (part 7) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators. r=khuey.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a9d854faf3593509575b682caac87c84bae7a7d
Bug 1187782 (part 8) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators. r=khuey.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6011756702d34de3872314fbcb4b7c85f53b0d1b
Bug 1187782 (part 9) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators. r=khuey.
(Assignee)

Updated

3 years ago
Attachment #8682819 - Flags: checkin+
(Assignee)

Updated

3 years ago
Attachment #8683456 - Flags: checkin+
(Assignee)

Updated

3 years ago
Attachment #8683490 - Flags: checkin+
(Assignee)

Updated

3 years ago
Attachment #8683493 - Flags: checkin+
(Assignee)

Comment 20

3 years ago
Created attachment 8684713 [details] [diff] [review]
(part 10) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators
Attachment #8684713 - Flags: review?(khuey)
(Assignee)

Comment 21

3 years ago
Created attachment 8684714 [details] [diff] [review]
(part 11) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators
Attachment #8684714 - Flags: review?(khuey)
(Assignee)

Comment 22

3 years ago
Created attachment 8684715 [details] [diff] [review]
(part 12) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators
Attachment #8684715 - Flags: review?(khuey)
(Assignee)

Comment 23

3 years ago
Created attachment 8684716 [details] [diff] [review]
(part 13) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators
Attachment #8684716 - Flags: review?(khuey)
(Assignee)

Comment 24

3 years ago
Created attachment 8684717 [details] [diff] [review]
(part 14) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators
Attachment #8684717 - Flags: review?(khuey)
(Assignee)

Comment 25

3 years ago
khuey: thank you for the reviews. Your penance for abandoning bug 1186780 is now complete :)
(Assignee)

Comment 27

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/973e6cc74295021ff49cd28a6c78ef15bfb50a09
Bug 1187782 (part 10) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators. r=khuey.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e24122a5c63636a63bc0145bf76045a70523509e
Bug 1187782 (part 11) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators. r=khuey.
(Assignee)

Updated

3 years ago
Attachment #8684713 - Flags: checkin+
(Assignee)

Updated

3 years ago
Attachment #8684714 - Flags: checkin+
(Assignee)

Comment 30

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a138819046955099a8ea10a9d885ca1444edacd
Bug 1187782 (part 12) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators. r=khuey.

https://hg.mozilla.org/integration/mozilla-inbound/rev/696889d8782c8429145aeddddb5d4a3e79ee04bb
Bug 1187782 (part 13) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators. r=khuey.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bc19fde35da4ed6f26dad8ff371f291931840e4e
Bug 1187782 (part 14) - Replace nsBaseHashtable::EnumerateRead() calls in dom/base/ with iterators. r=khuey.
(Assignee)

Updated

3 years ago
Keywords: leave-open
(Assignee)

Updated

3 years ago
Attachment #8684715 - Flags: checkin+
(Assignee)

Updated

3 years ago
Attachment #8684716 - Flags: checkin+
(Assignee)

Updated

3 years ago
Attachment #8684717 - Flags: checkin+

Comment 31

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9a1388190469
https://hg.mozilla.org/mozilla-central/rev/696889d8782c
https://hg.mozilla.org/mozilla-central/rev/bc19fde35da4
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.