Closed Bug 1182980 Opened 5 years ago Closed 5 years ago

Use nsTHashTable::Iterator in dom/base/

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: njn, Assigned: poiru)

References

Details

Attachments

(11 files, 5 obsolete files)

3.63 KB, patch
khuey
: review+
Details | Diff | Splinter Review
1.85 KB, patch
khuey
: review+
Details | Diff | Splinter Review
2.08 KB, patch
khuey
: review+
Details | Diff | Splinter Review
3.97 KB, patch
khuey
: review+
Details | Diff | Splinter Review
1.37 KB, patch
khuey
: review+
Details | Diff | Splinter Review
2.64 KB, patch
khuey
: review+
Details | Diff | Splinter Review
2.89 KB, patch
khuey
: review+
Details | Diff | Splinter Review
1.69 KB, patch
khuey
: review+
Details | Diff | Splinter Review
6.37 KB, patch
khuey
: review+
Details | Diff | Splinter Review
3.59 KB, patch
khuey
: review+
Details | Diff | Splinter Review
2.80 KB, patch
khuey
: review+
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #1182979 +++

Because iterators are so much nicer than enumerate functions.

There are 19 occurrences of EnumerateEntries() in dom/base/ to be dealt with.
Duplicate of this bug: 1182725
Assignee: nobody → birunthan
Status: NEW → ASSIGNED
Attachment #8636999 - Flags: review?(ehsan)
Attachment #8636988 - Attachment is obsolete: true
Attachment #8636988 - Flags: review?(ehsan)
Attachment #8636999 - Attachment is obsolete: true
Attachment #8636999 - Flags: review?(ehsan)
Comment on attachment 8636993 [details] [diff] [review]
Part 3: Use nsTHashtable::Iterator in nsWindowRoot

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

::: dom/base/nsWindowRoot.cpp
@@ +408,5 @@
>    nsTArray<nsRefPtr<TabParent>> tabParents;
> +  for (auto iter = mWeakBrowsers.Iter(); !iter.Done(); iter.Next()) {
> +    nsCOMPtr<nsITabParent> tabParent(do_QueryReferent(iter.Get()->GetKey()));
> +    TabParent* tab = TabParent::GetFrom(tabParent);
> +    if (tab) {

I know you just copied this, but I prefer

if (TabParent* tab ...) {
Attachment #8636993 - Flags: review?(ehsan) → review+
Attachment #8637009 - Attachment is obsolete: true
Attachment #8637009 - Flags: review?(ehsan)
Comment on attachment 8637003 [details] [diff] [review]
Part 6: Use nsTHashtable::Iterator in ShadowRoot

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

::: dom/base/ShadowRoot.cpp
@@ +29,5 @@
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mStyleSheetList)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mOlderShadow)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mYoungerShadow)
>    NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mAssociatedBinding)
> +  for (auto iter = tmp->mIdentifierMap.Iter(); !iter.Done(); iter.Next()) {

You can use ConstIter here.  In fact you could probably use it in the entire patch series.

::: dom/base/nsWindowMemoryReporter.cpp
@@ +482,5 @@
>        NS_WARNING("Couldn't get window-by-id hashtable?");
>        continue;
>      }
>  
> +    nsGlobalWindow* window = windowsById->Get(iter.Get()->GetKey());

This hunk belongs in a previous patch.
Attachment #8637003 - Flags: review?(ehsan) → review+
Comment on attachment 8637007 [details] [diff] [review]
Part 7: Use nsTHashtable::Iterator in nsIdentifierMapEntry::FireChangeCallbacks

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

::: dom/base/nsDocument.cpp
@@ +558,5 @@
> +    }
> +
> +    if (entry->mKey.mCallback(aOldElement, aNewElement, entry->mKey.mData)) {
> +      continue;
> +    }

I'd write this as

if (!blah) {
  iter.Remove();
}

but I don't feel strongly about it.
Attachment #8637007 - Flags: review?(ehsan) → review+
Comment on attachment 8637015 [details] [diff] [review]
Part 9: Use nsTHashtable::Iterator in rest of nsDocument

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

::: dom/base/nsDocument.cpp
@@ +1927,5 @@
>    if (!nsINode::Traverse(tmp, cb)) {
>      return NS_SUCCESS_INTERRUPTED_TRAVERSE;
>    }
>  
> +  for (auto iter = tmp->mIdentifierMap.Iter(); !iter.Done(); iter.Next()) {

ConstIter here too.
Attachment #8637015 - Flags: review?(khuey) → review+
I modified my local patches to use AppendElements as per comment 11.
Attachment #8638007 - Flags: review?(nfroyd)
Comment on attachment 8638004 [details] [diff] [review]
Part 6: Add fallible variant of r-value nsTArray::AppendElements

Woops, wrong bug.
Attachment #8638004 - Attachment is obsolete: true
Attachment #8638004 - Flags: review?(nfroyd)
Attachment #8638007 - Attachment is obsolete: true
Attachment #8638007 - Flags: review?(nfroyd)
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/d741774b38c0
https://hg.mozilla.org/mozilla-central/rev/b6ccc4e39c84
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.