Closed Bug 1187139 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(3 files)

Because iterators are so much nicer than enumerate functions.

There are seven 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()).
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Comment on attachment 8691754 [details] [diff] [review]
(part 1) - Replace nsBaseHashtable::Enumerate() calls in accessible/ with iterators

># HG changeset patch
># User Nicholas Nethercote <nnethercote@mozilla.com>
># Date 1448428032 28800
>#      Tue Nov 24 21:07:12 2015 -0800
># Node ID 5f8fdc4a57e710eace7fe83cc095c48ab14cdd98
># Parent  83b622d14755ae051788858efbea067ac48d7e33
>Bug 1187139 (part 1) - Replace nsBaseHashtable::Enumerate() calls in accessible/ with iterators. r=tbsaunde.

I'm glad you split the patches, but 3 identical looking commit messages is kind of funny ;)

> ClearCache(nsRefPtrHashtable<nsPtrHashKey<const void>, T>& aCache)
> {
>-  aCache.Enumerate(ClearCacheEntry<T>, nullptr);
>+  for (auto iter = aCache.Iter(); !iter.Done(); iter.Next()) {
>+    RefPtr<T> accessible = iter.Data();

can we do Accessibl*or if not const RefPtr<Accessible>& ? I don't see a good reason to add more refcounting here.

>+    NS_ASSERTION(accessible, "Calling ClearCacheEntry with a nullptr pointer!");

the message doesn't make much sense now, I guesssomething like cache has null accessible value? or just make it a MOZ_ASSERT and drop the string?

r=me if you fix up the ref counting issue.
Attachment #8691754 - Flags: review?(tbsaunde+mozbugs) → review+
Comment on attachment 8691755 [details] [diff] [review]
(part 2) - Replace nsBaseHashtable::Enumerate() calls in accessible/ with iterators

>+UnbindCacheEntriesFromDocument(
>+  nsRefPtrHashtable<nsPtrHashKey<const void>, T>& aCache)
> {
>-  MOZ_ASSERT(aAccessible && !aAccessible->IsDefunct());
>-  aAccessible->Document()->UnbindFromDocument(aAccessible);
>-
>-  return PL_DHASH_REMOVE;
>+  for (auto iter = aCache.Iter(); !iter.Done(); iter.Next()) {
>+    RefPtr<T> accessible = iter.Data();

please use Accessible* or const RefPtr<Accessible>& to avoid extra ref counting.

r=me with that fixed
Attachment #8691755 - Flags: review?(tbsaunde+mozbugs) → review+
Attachment #8691757 - Flags: review?(tbsaunde+mozbugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7ba739d20a99e0808eed568175aa9404f2b4929
Bug 1187139 (part 1) - Replace nsBaseHashtable::Enumerate() calls in accessible/ with iterators. r=tbsaunde.

https://hg.mozilla.org/integration/mozilla-inbound/rev/06f4c59269952cb31914815e3b69d038f0da3c8d
Bug 1187139 (part 2) - Replace nsBaseHashtable::Enumerate() calls in accessible/ with iterators. r=tbsaunde.

https://hg.mozilla.org/integration/mozilla-inbound/rev/32528aa361207af33918e2986342bf176d8e4749
Bug 1187139 (part 3) - Replace nsBaseHashtable::Enumerate() calls in accessible/ with iterators. r=tbsaunde.
https://hg.mozilla.org/mozilla-central/rev/a7ba739d20a9
https://hg.mozilla.org/mozilla-central/rev/06f4c5926995
https://hg.mozilla.org/mozilla-central/rev/32528aa36120
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.