Closed
Bug 1186796
Opened 9 years ago
Closed 9 years ago
Replace nsBaseHashtable::EnumerateRead() calls in image/ with iterators
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla45
Tracking | Status | |
---|---|---|
firefox45 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: sotaro)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(1 file, 2 obsolete files)
17.46 KB,
patch
|
sotaro
:
review+
|
Details | Diff | Splinter Review |
Because iterators are so much nicer than enumerate functions. There are four 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 | ||
Updated•9 years ago
|
Assignee: nobody → sotaro.ikeda.g
Assignee | ||
Updated•9 years ago
|
Whiteboard: [gfx-noted]
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
Remove unnecessary functions.
Attachment #8685325 -
Attachment is obsolete: true
Assignee | ||
Updated•9 years ago
|
Attachment #8685326 -
Flags: review?(n.nethercote)
Reporter | ||
Comment 3•9 years ago
|
||
Comment on attachment 8685326 [details] [diff] [review] patch - Replace nsBaseHashtable::EnumerateRead() calls in image/ with iterators Review of attachment 8685326 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the following comments addressed. ::: image/SurfaceCache.cpp @@ -285,5 @@ > } > > // There's no perfect match, so find the best match we can. > MatchContext matchContext(aSurfaceKey); > - ForEach(TryToImproveMatch, &matchContext); You can remove MatchContext too, now -- just use local variables for its fields. @@ +714,5 @@ > + for (auto iter = cache->ConstIter(); !iter.Done(); iter.Next()) { > + CachedSurface* surface = iter.UserData(); > + if (surface->IsPlaceholder() || !surface->IsLocked()) { > + continue; > + } This loop is large enough that duplicating it is a shame. I'd prefer it if you factored it out into its own function, e.g. UnlockSurfaces(). @@ +757,5 @@ > // small, performance should be good, but if usage patterns change we should > // change the data structure used for mCosts. > + for (auto iter = cache->ConstIter(); !iter.Done(); iter.Next()) { > + CachedSurface* surface = iter.UserData(); > + StopTracking(surface); I would avoid the local variable and just do this: |StopTracking(iter.UserData());| @@ +865,5 @@ > CachedSurface::SurfaceMemoryReport report(aCounters, aMallocSizeOf); > + for (auto iter = cache->ConstIter(); !iter.Done(); iter.Next()) { > + CachedSurface* surface = iter.UserData(); > + report.Add(surface); > + } Same thing here: avoid the the local variable. ::: image/imgLoader.cpp @@ +80,5 @@ > + imgCacheEntry* entry = iter.UserData(); > + RefPtr<imgRequest> req = entry->GetRequest(); > + RecordCounterForRequest(req, > + &chrome, > + !entry->HasNoProxies()); Put this function call on a single line. @@ +83,5 @@ > + &chrome, > + !entry->HasNoProxies()); > + } > + for (auto iter = mKnownLoaders[i]->mCache.Iter(); !iter.Done(); iter.Next()) { > + imgCacheEntry* entry = iter.UserData(); Indent this line one more char. @@ +87,5 @@ > + imgCacheEntry* entry = iter.UserData(); > + RefPtr<imgRequest> req = entry->GetRequest(); > + RecordCounterForRequest(req, > + &content, > + !entry->HasNoProxies()); Put this function call on a single line.
Attachment #8685326 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 4•9 years ago
|
||
Apply the comments. Carry "r=njn".
Attachment #8685326 -
Attachment is obsolete: true
Attachment #8685842 -
Flags: review+
Comment 6•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/411f18fdffeb
Status: NEW → 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
•