Closed
Bug 1187139
Opened 9 years ago
Closed 9 years ago
Replace nsBaseHashtable::Enumerate() calls in accessible/ with iterators
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(3 files)
1.94 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
5.49 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
1.21 KB,
patch
|
tbsaunde
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•9 years ago
|
||
Attachment #8691754 -
Flags: review?(tbsaunde+mozbugs)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8691755 -
Flags: review?(tbsaunde+mozbugs)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8691757 -
Flags: review?(tbsaunde+mozbugs)
Comment 4•9 years ago
|
||
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 5•9 years ago
|
||
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+
Updated•9 years ago
|
Attachment #8691757 -
Flags: review?(tbsaunde+mozbugs) → review+
Assignee | ||
Comment 6•9 years ago
|
||
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.
Comment 7•9 years ago
|
||
bugherder |
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: 9 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.
Description
•