Improve nsRegionRectIterator and nsIntRegionRectIterator

RESOLVED FIXED in Firefox 47

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(11 attachments, 1 obsolete attachment)

14.49 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.90 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.30 KB, patch
baku
: review+
Details | Diff | Splinter Review
7.69 KB, patch
nical
: review+
Details | Diff | Splinter Review
8.77 KB, patch
nical
: review+
Details | Diff | Splinter Review
12.48 KB, patch
nical
: review+
Details | Diff | Splinter Review
8.34 KB, patch
nical
: review+
Details | Diff | Splinter Review
5.12 KB, patch
nical
: review+
Details | Diff | Splinter Review
15.53 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
11.35 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.92 KB, patch
roc
: review+
Details | Diff | Splinter Review
nsRegionRectIterator and nsIntRegionRectIterator are awkward to use. Most of the work is done by a single function, Next(), which does three things:

- checks if we've reached the end (and returns nullptr if so);

- if not, returns the current element;

- advances to the next element.

We get a much nicer (and more typical) interface if we split those up into three separate functions: Done(), Get(), and Next().

With the current interface, the iterators are used in one of the three following ways:

> nsRegionRectIterator iter(region);
> for (const nsRect* r = iter.Next(); r; r = iter.Next()) {
>   f(*r);
> }
> 
> nsRegionRectIterator iter(region);
> while (const nsRect* r = iter.Next()) {
>   f(*r);
> }
> 
> const nsRect* r;
> for (nsRegionRectIterator iter(region); (r = iter.Next());) {
>   f(*r);
>  }

With the new interface, it'll look like this:

> for (auto iter = region.RectIter(); !iter.Done(); iter.Next()) {
>   const nsRect& r = iter.Get();   // can be inlined if r has a single use
>   f(r);
> }

This is more standard (same as PLDHashTable/nsTHashTable, for example), easier to read, and there is no need for local variables whose scope goes outside the loop. The code for the iterator itself is also simpler.
This variation has come up too, even worse:

> LayoutDeviceIntRegion::RectIterator iter(aRegion);
> for (;;) {
>   const LayoutDeviceIntRect* r = iter.Next();
>   if (!r) {
>     break;
>   }
>   f(*r);
> }
This requires renaming the existing nsIntRegion::RectIterator as
nsIntRegion::OldRectIterator to make way for the new nsIntRegion::RectIterator.
This doesn't require many knock-on changes because most existing uses of
that type use the nsIntRegionRectIterator typedef.
Attachment #8709701 - Flags: review?(roc)
Attachment #8709707 - Flags: review?(nical.bugzilla)
Attachment #8709708 - Flags: review?(nical.bugzilla)
Attachment #8709709 - Flags: review?(nical.bugzilla)
Attachment #8709710 - Flags: review?(nical.bugzilla)
Attachment #8709711 - Flags: review?(nical.bugzilla)
(Reposting this patch; I fixed the log message.)
Attachment #8709717 - Flags: review?(roc)
Attachment #8709716 - Attachment is obsolete: true
Attachment #8709716 - Flags: review?(roc)
Comment on attachment 8709715 [details] [diff] [review]
(part 9) - Use the new rect iterators in layout/ and ipc/

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

r=me, just 2 nits:

::: ipc/ipdl/test/cxx/TestDataStructures.cpp
@@ +451,2 @@
>          // if |ra| has been realloc()d and given a different allocator
> +        // chunk, this loop will nondeterministically crash or iloop

Please add a period at the end of this comment, while you're modifying it.

::: layout/base/nsDisplayList.cpp
@@ +3632,3 @@
>      }
> +  }
> +  // Finish the in-flight rectangle, if there is one.

Please put a blank line before this comment (just after the loop), to more clearly separate this final chunk from the loop.
Attachment #8709715 - Flags: review?(dholbert) → review+
Attachment #8709705 - Flags: review?(amarchesini) → review+
Attachment #8709707 - Flags: review?(nical.bugzilla) → review+
Attachment #8709708 - Flags: review?(nical.bugzilla) → review+
Attachment #8709709 - Flags: review?(nical.bugzilla) → review+
Attachment #8709710 - Flags: review?(nical.bugzilla) → review+
Attachment #8709711 - Flags: review?(nical.bugzilla) → review+
This is nice but I'm curious: Why didn't you pick an iterator API that would have let us write
for (nsRect rect : region.RectIter()) { ... } ?
> This is nice but I'm curious: Why didn't you pick an iterator API that would
> have let us write
> for (nsRect rect : region.RectIter()) { ... } ?

Two reasons:

- The Iter/Next/Done style of iterator has lots of precedence in Mozilla code.

- I'm not a much of a fan of range-based iterators. See bug 1149833 for some background.

Feel free to change the API to allow range-based iterators if you like :)
You need to log in before you can comment on or make changes to this bug.