Closed Bug 1043013 Opened 10 years ago Closed 9 years ago

Introduce strongly-typed region classes

Categories

(Core :: Graphics: Layers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: botond, Assigned: botond)

Details

Attachments

(3 files, 1 obsolete file)

We have strongly-typed rectangle classes, but sometimes we need to represent an area of a page as a region rather than a rectangle. It would be nice to be able to use strongly-typed units for that too, with the iterator producing rectangles of the correct type.
This would be particularly nice to keep APZ strongly typed as it transitions from rects to regions for hit-testing (bug 918288, though bug 973096 already introduced a region).
Assignee: nobody → botond
Attached file MozReview Request: bz://1043013/botond (obsolete) —
/r/5417 - Bug 1043013 - Generalize nsIntRegion into a BaseIntRegion template. r=jrmuizel
/r/5419 - Bug 1043013 - Introduce IntRegionTyped. r=jrmuizel
/r/5421 - Bug 1043013 - Use strongly-typed regions in HitTestingTreeNode. r=kats

Pull down these commits:

hg pull review -r 0f96681a0da6a856e602937e0b1a1c7b2787a798
Attachment #8577540 - Flags: review?(jmuizelaar)
Attachment #8577540 - Flags: review?(bugmail.mozilla)
Some commentary:

The first patch generalizes nsIntRegion into a class template BaseIntRegion. nsIntRegion becomes a derived class of it, much how nsIntPoint derives from BasePoint. I didn't move BaseIntRegion into gfx/2d, because it uses nsRegion and gfx/thebes stuff in its implementation.

The second patch introduces IntRegionTyped, which relates to BaseIntRegion much as how IntRectTyped relates to BaseRect. I added this in a separate file so nsRegion.h doesn't start depending on gfx/2d/Rect.h. I also added typedefs like CSSIntRegion.

The third patch gives the strongly-typed regions a whirl by using them in HitTestingTreeNode. There is more APZ code that can be transitioned to strongly-typed regions (e.g. changing the return type of LayerMetricsWrapper::GetClipRect() would be a natural next step), but I'd prefer to leave that to a follow-up.
https://reviewboard.mozilla.org/r/5421/#review4345

::: gfx/layers/apz/src/APZCTreeManager.cpp
(Diff revision 1)
> -        RoundedToInt(aLayer.Metrics().mCompositionBounds)));

nice!
Attachment #8577540 - Flags: review?(bugmail.mozilla) → review+
Attachment #8577540 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8577540 [details]
MozReview Request: bz://1043013/botond

https://reviewboard.mozilla.org/r/5415/#review4357

Ship It!
(In reply to Botond Ballo [:botond] from comment #4)
> Buiuld-only Try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fc8fd690b1c

Had some static analysis warnings due to implicit constructors, fixed locally.

New Try push, including b2g tests to be safe: https://treeherder.mozilla.org/#/jobs?repo=try&revision=69432819c9c3
(In reply to Botond Ballo [:botond] from comment #8)
> New Try push, including b2g tests to be safe:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=69432819c9c3

Looking green (note that [1] and [2] show that the M9 failure is not caused by these patches).

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f7317bc4bb7
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa1a810960b4
Attachment #8577540 - Attachment is obsolete: true
Attachment #8618237 - Flags: review+
Attachment #8618238 - Flags: review+
Attachment #8618239 - Flags: review+
You need to log in before you can comment on or make changes to this bug.