Closed Bug 1359842 Opened 7 years ago Closed 7 years ago

Add strongly typed coordinate systems to gfx/layers/wr

Categories

(Core :: Graphics: WebRender, enhancement, P3)

Product:
Core
Component:
Graphics: WebRender
Version:
Other Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(11 files)

Bug 1359842 - Do some cleanup of GetWrRelBounds.
59 bytes, text/x-review-board-request
nical
: review+
Details
Bug 1359842 - Do some cleanup of GetWrClipRect.
59 bytes, text/x-review-board-request
nical
: review+
Details
Bug 1359842 - Convert some functions to deal in LayerRect instead of gfx::Rect.
59 bytes, text/x-review-board-request
nical
: review+
Details
Bug 1359842 - Replace use of NSRectToRect in WebRender-related code with proper LayoutDevicePixel types.
59 bytes, text/x-review-board-request
nical
: review+
Details
Bug 1359842 - Convert RelativeToParent to deal with typed units.
59 bytes, text/x-review-board-request
nical
: review+
Details
Bug 1359842 - Remove unused function.
59 bytes, text/x-review-board-request
nical
: review+
Details
Bug 1359842 - Rename ParentStackingContextBounds to ParentBounds.
59 bytes, text/x-review-board-request
nical
: review+
Details
Bug 1359842 - Add a StackingContextHelper to reduce duplicated code.
59 bytes, text/x-review-board-request
nical
: review+
Details
Bug 1359842 - Remove if/else branch in WebRenderContainerLayer so it goes through a single PushStackingContext call.
59 bytes, text/x-review-board-request
nical
: review+
Details
Bug 1359842 - Use the StackingContextHelper for WebRenderContainerLayer as well.
59 bytes, text/x-review-board-request
nical
: review+
Details
Bug 1359842 - Remove a bunch of now unused functions.
59 bytes, text/x-review-board-request
nical
: review+
Details 
The code in gfx/layers/wr mostly just uses gfx::Rect and gfx::Point and other untyped things which is not great. Specially since we are applying transforms in some places to convert between coordinate spaces. It would be much better to use strongly typed classes for this.
Assignee: nobody → bugmail
Comment on attachment 8862048 [details]
Bug 1359842 - Convert some functions to deal in LayerRect instead of gfx::Rect.

https://reviewboard.mozilla.org/r/134024/#review136918
Attachment #8862048 - Flags: review+
Comment on attachment 8862049 [details]
Bug 1359842 - Replace use of NSRectToRect in WebRender-related code with proper LayoutDevicePixel types.

https://reviewboard.mozilla.org/r/134026/#review136926

Note that using FromAppUnits instead of NSRectToRect means we are doing 32 bits float divisions instead of 64 bits, so there may be some precision differences with this commit to watch out for (r=me assuming that's OK).
Attachment #8862049 - Flags: review+
Comment on attachment 8862052 [details]
Bug 1359842 - Rename ParentStackingContextBounds to ParentBounds.

https://reviewboard.mozilla.org/r/134032/#review136930
Attachment #8862052 - Flags: review+
(In reply to Nicolas Silva [:nical] (away until April 18th) from comment #10)
> Note that using FromAppUnits instead of NSRectToRect means we are doing 32
> bits float divisions instead of 64 bits, so there may be some precision
> differences with this commit to watch out for (r=me assuming that's OK).

Good point. The try push I did was green, so to the extent that this code is getting exercised it seems that the precision is ok. It might show up as a problem later after we enable more advanced layers by default, but I'd be fine with making FromAppUnits use the 64-bit division if we need it.
Comment on attachment 8862050 [details]
Bug 1359842 - Convert RelativeToParent to deal with typed units.

https://reviewboard.mozilla.org/r/134028/#review136932
Attachment #8862050 - Flags: review+
Comment on attachment 8862047 [details]
Bug 1359842 - Do some cleanup of GetWrClipRect.

https://reviewboard.mozilla.org/r/134022/#review136934
Attachment #8862047 - Flags: review+
I wrote a few more patches which add a helper class to simplify dealing with stacking contexts. It removes a bunch of duplicated code and makes things cleaner. I'll tack them on here since the tree is closed and I can't land anything right now anyway.
Comment on attachment 8862142 [details]
Bug 1359842 - Remove a bunch of now unused functions.

https://reviewboard.mozilla.org/r/134114/#review137230
Attachment #8862142 - Flags: review?(nical.bugzilla) → review+
There was another missing include (pre-existing this time), so with that fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d800608b2348dc75ff03d9d0fa8edb4e934daa1

I'll have to rebase this all onto graphics tip anyway.
I have more patches but I'll put them on a part 2 bug because I'd like to get this landed before it rots (and before more calls to NSRectToRect get added, such as will happen in bug 1360112).
Comment on attachment 8862139 [details]
Bug 1359842 - Add a StackingContextHelper to reduce duplicated code.

https://reviewboard.mozilla.org/r/134108/#review137350
Attachment #8862139 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8862140 [details]
Bug 1359842 - Remove if/else branch in WebRenderContainerLayer so it goes through a single PushStackingContext call.

https://reviewboard.mozilla.org/r/134110/#review137352
Attachment #8862140 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8862141 [details]
Bug 1359842 - Use the StackingContextHelper for WebRenderContainerLayer as well.

https://reviewboard.mozilla.org/r/134112/#review137354
Attachment #8862141 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8862046 [details]
Bug 1359842 - Do some cleanup of GetWrRelBounds.

https://reviewboard.mozilla.org/r/134020/#review137356
Attachment #8862046 - Flags: review?(nical.bugzilla) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/69e23059260e
Do some cleanup of GetWrRelBounds. r=nical
https://hg.mozilla.org/projects/graphics/rev/b80b628fc84d
Do some cleanup of GetWrClipRect. r=nical
https://hg.mozilla.org/projects/graphics/rev/3d7c750e62c7
Convert some functions to deal in LayerRect instead of gfx::Rect. r=nical
https://hg.mozilla.org/projects/graphics/rev/1e8212d50179
Replace use of NSRectToRect in WebRender-related code with proper LayoutDevicePixel types. r=nical
https://hg.mozilla.org/projects/graphics/rev/c53b024ad6be
Convert RelativeToParent to deal with typed units. r=nical
https://hg.mozilla.org/projects/graphics/rev/3f475f4737ea
Remove unused function. r=nical
https://hg.mozilla.org/projects/graphics/rev/3e288b211e76
Rename ParentStackingContextBounds to ParentBounds. r=nical
https://hg.mozilla.org/projects/graphics/rev/ddeda3ad6b14
Add a StackingContextHelper to reduce duplicated code. r=nical
https://hg.mozilla.org/projects/graphics/rev/007c9fb80cfa
Remove if/else branch in WebRenderContainerLayer so it goes through a single PushStackingContext call. r=nical
https://hg.mozilla.org/projects/graphics/rev/f0d513a6709f
Use the StackingContextHelper for WebRenderContainerLayer as well. r=nical
https://hg.mozilla.org/projects/graphics/rev/258e92719dfe
Remove a bunch of now unused functions. r=nical
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: