Closed
Bug 1186805
Opened 10 years ago
Closed 10 years ago
Replace nsBaseHashtable::EnumerateRead() calls in dom/datastore/ with iterators
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: baku)
References
Details
Attachments
(1 file, 1 obsolete file)
10.12 KB,
patch
|
Details | Diff | Splinter Review |
Because iterators are so much nicer than enumerate functions.
There are five occurrences of EnumerateRead() in this directory.
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•10 years ago
|
||
Attachment #8667321 -
Flags: review?(n.nethercote)
![]() |
Reporter | |
Comment 2•10 years ago
|
||
Comment on attachment 8667321 [details] [diff] [review]
ds.patch
Review of attachment 8667321 [details] [diff] [review]:
-----------------------------------------------------------------
Such an improvement. Thank you. r=me with the nits below addressed.
::: dom/datastore/DataStoreService.cpp
@@ -331,5 @@
> -
> - auto* data = static_cast<GetDataStoreInfosData*>(aUserData);
> - if (aAppId == data->mAppId) {
> - return PL_DHASH_NEXT;
> - }
This if-block didn't survive the transition. Is that intended? I can't see why you would remove it.
@@ +1023,5 @@
> + continue;
> + }
> +
> + if (!aOwner.IsEmpty() &&
> + !aOwner.Equals(iter.UserData()->mManifestURL)) {
There are five occurrences of iter.UserData() in this function. Please create a local variable for it.
@@ +1229,5 @@
> + for (auto iter = apps->ConstIter(); !iter.Done(); iter.Next()) {
> + bool readOnly = aReadOnly || iter.UserData()->mReadOnly;
> +
> + rv = ResetPermission(iter.Key(), iter.UserData()->mOriginURL,
> + iter.UserData()->mManifestURL,
Again, please factor out the repeated iter.UserData() calls.
Attachment #8667321 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 3•10 years ago
|
||
Attachment #8667321 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → amarchesini
Keywords: checkin-needed
Assignee | ||
Comment 4•10 years ago
|
||
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•