Closed Bug 1187152 Opened 5 years ago Closed 4 years ago

Replace nsBaseHashtable::Enumerate() calls in modules/ with iterators

Categories

(Core :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(3 files, 1 obsolete file)

Because iterators are so much nicer than enumerate functions.

There are three occurrences of Enumerate() in this directory.

A note to the assignee: to preserve existing behaviour, you should probably use
nsBaseHashtable::Iterator::Data() rather than nsBaseHashtable::Iterator::UserData(). (The latter should be used when replacing nsBaseHashtable::EnumerateRead()).
th iterators. r=mwu.
Attachment #8690592 - Flags: review?(mwu)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Attachment #8690592 - Attachment is obsolete: true
Attachment #8690592 - Flags: review?(mwu)
Comment on attachment 8690597 [details] [diff] [review]
(part 3) - Replace nsBaseHashtable::Enumerate() calls in modules/ with iterators

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

::: modules/libpref/Preferences.cpp
@@ +318,5 @@
> +    nsDependentCString prefString(pref);
> +    uint32_t oldCount = 0;
> +    prefCounter.Get(prefString, &oldCount);
> +    uint32_t currentCount = oldCount + 1;
> +    prefCounter.Put(prefString, currentCount);

This sort of API is so unfortunate. :(
Attachment #8690597 - Flags: review?(nfroyd) → review+
Comment on attachment 8690594 [details] [diff] [review]
(part 2) - Replace nsBaseHashtable::Enumerate() calls in modules/ with iterators

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

I'm not completely sure about the comment below.

::: modules/libpref/nsPrefBranch.cpp
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */

\o/

@@ +701,5 @@
> +    // nsPrefBranch::RemoveObserver, since some classes remove themselves from
> +    // the pref branch on destruction. We don't need to worry about this
> +    // causing double-frees, however, because freeObserverList sets
> +    // mFreeingObserverList to true, which prevents RemoveObserver calls from
> +    // doing anything.

I think we now want to rewrite this to something like:

Removing the current element of the iterator below might trigger a call to nsPrefBranch::RemoveObserver, since some classes remove themselves from the pref branch on destruction.  We don't need to worry about this causing double-frees, however, because we set mFreeingObserverList to true before entering this loop, which prevents RemoveObserver calls from doing anything.

(I'm assuming that all the bits about RemoveObserver are still true...)
Attachment #8690594 - Flags: review?(nfroyd) → review+
Attachment #8690593 - Flags: review?(mwu) → review+
> I think we now want to rewrite this to something like:

It's a good point that the comment needs rewriting; thank you for spotting that. But that comment basically duplicated the information in the comment above the loop, so I ended up removing it and tweaking the outer comment instead.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f161061f73a6d342481842c3e8f47c8910d405a7
Bug 1187152 (part 1) - Replace nsBaseHashtable::Enumerate() calls in modules/ with iterators. r=mwu.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce754612fecfc340b6de8c30aa6c7b944c37f747
Bug 1187152 (part 2) - Replace nsBaseHashtable::Enumerate() calls in modules/ with iterators. r=froydnj.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0aec4fd3842fcfc8d8626b6d715fae7168b31ab0
Bug 1187152 (part 3) - Replace nsBaseHashtable::Enumerate() calls in modules/ with iterators. r=froydnj.
You need to log in before you can comment on or make changes to this bug.