1239864 - Improve nsRegionRectIterator and nsIntRegionRectIterator

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Nicholas Nethercote [inactive]]

This variation has come up too, even worse:

> LayoutDeviceIntRegion::RectIterator iter(aRegion);
> for (;;) {
>   const LayoutDeviceIntRect* r = iter.Next();
>   if (!r) {
>     break;
>   }
>   f(*r);
> }

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Add new, nicer rect-iterators for nsRegion and nsIntRegion

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.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Use the new rect iterators in nsRegion.cpp

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Use the new rect iterators in docshell/ and dom/

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Use the new rect iterators in gfx/

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Use the new rect iterators in gfx/

Comment 7

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Use the new rect iterators in gfx/

Comment 8

[Nicholas Nethercote [inactive]]

Attachment: (part 7) - Use the new rect iterators in gfx/

Comment 9

[Nicholas Nethercote [inactive]]

Attachment: (part 8) - Use the new rect iterators in gfx/

Comment 10

[Nicholas Nethercote [inactive]]

Attachment: (part 9) - Use the new rect iterators in layout/ and ipc/

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: (part 10) - Use the new rect iterators in gfx/

Comment 12

[Nicholas Nethercote [inactive]]

Attachment: (part 10) - Use the new rect iterators in view/ and widget/

(Reposting this patch; I fixed the log message.)

Comment 13

[Nicholas Nethercote [inactive]]

Attachment: (part 11) - Remove the old rect iterators

Comment 14

[Daniel Holbert [:dholbert]]

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.

Comment 15

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dbfd0199b5c78985881f84221a1a06491bf2e46c
Bug 1239864 (part 1) - Add new, nicer rect-iterators for nsRegion and nsIntRegion. r=roc.

https://hg.mozilla.org/integration/mozilla-inbound/rev/17073770898507418ace502f1f25cadaff037c3e
Bug 1239864 (part 2) - Use the new rect iterators in nsRegion.cpp. r=roc.

https://hg.mozilla.org/integration/mozilla-inbound/rev/03e0cf7f8a0e9e40d81b7ad196ef22c3cfa6a29b
Bug 1239864 (part 3) - Use the new rect iterators in docshell/ and dom/. r=baku.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f98b125e8c3b70c6efb795a073b8ebee72cc6c2b
Bug 1239864 (part 4) - Use the new rect iterators in gfx/. r=nical.

https://hg.mozilla.org/integration/mozilla-inbound/rev/94b30eb55f0c3e5b330b7ca5bc22fe3f753dfbcd
Bug 1239864 (part 5) - Use the new rect iterators in gfx/. r=nical.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dbfd0199b5c7
https://hg.mozilla.org/mozilla-central/rev/170737708985
https://hg.mozilla.org/mozilla-central/rev/03e0cf7f8a0e
https://hg.mozilla.org/mozilla-central/rev/f98b125e8c3b
https://hg.mozilla.org/mozilla-central/rev/94b30eb55f0c

Comment 17

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f260e81c228c1ee96cb7078c0d0599d492fe2ac
Bug 1239864 (part 6) - Use the new rect iterators in gfx/. r=nical.

https://hg.mozilla.org/integration/mozilla-inbound/rev/01151b9f47a2169f1499006c19fc09ff1d47c9e5
Bug 1239864 (part 7) - Use the new rect iterators in gfx/. r=nical.

https://hg.mozilla.org/integration/mozilla-inbound/rev/be04eccbd42742a04fb68815a00f1baab014aead
Bug 1239864 (part 8) - Use the new rect iterators in gfx/. r=nical.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffd9ee1eb392f4ff24092186309adf1b22daf063
Bug 1239864 (part 9) - Use the new rect iterators in layout/ and ipc/. r=dholbert.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b02ba9cfc4f686b6d485c729c31c02e2477b611
Bug 1239864 (part 10) - Use the new rect iterators in view/ and widget/. r=roc.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e671b94aac73632dc50dcb12ef26fb1045d672dd
Bug 1239864 (part 11) - Remove the old rect iterators. r=roc.

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8f260e81c228
https://hg.mozilla.org/mozilla-central/rev/01151b9f47a2
https://hg.mozilla.org/mozilla-central/rev/be04eccbd427
https://hg.mozilla.org/mozilla-central/rev/ffd9ee1eb392
https://hg.mozilla.org/mozilla-central/rev/3b02ba9cfc4f
https://hg.mozilla.org/mozilla-central/rev/e671b94aac73

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8f260e81c228
https://hg.mozilla.org/mozilla-central/rev/01151b9f47a2
https://hg.mozilla.org/mozilla-central/rev/be04eccbd427
https://hg.mozilla.org/mozilla-central/rev/ffd9ee1eb392
https://hg.mozilla.org/mozilla-central/rev/3b02ba9cfc4f
https://hg.mozilla.org/mozilla-central/rev/e671b94aac73

Comment 20

[Markus Stange [:mstange]]

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()) { ... } ?

Comment 21

[Nicholas Nethercote [inactive]]

> 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 :)