Closed Bug 1022381 Opened 7 years ago Closed 7 years ago

TransformTo functions in UnitTransforms.h allow templating with rects, creating footgun

Categories

(Core :: Panning and Zooming, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: kats, Assigned: botond)

Details

Attachments

(1 file, 1 obsolete file)

While working on bug 1018387, I accidentally changed a call from
  TransformTo<LayoutDevicePixel>(...)
to:
  TransformTo<LayerRect>(...)

Note the use of LayerRect rather than LayerPixel. This compiled and ran just fine, but had very peculiar runtime behaviour which seemed mostly correct but wasn't actually. I ended up wasting a lot of time before I tracked this down. It would be nice if we could prevent this at compile-time.
The root of the problem here is that templates like PointTyped or RectTyped can be instantiated with LayerRect (which is RectTyped<LayerPixel>) as an argument, instead of LayerPixel, without the compiler complaining.

I propose the following solution:

  - Introduce a type trait IsPixel<T>, which would evaluate to false by default,
    but would be specialized to return true for CSSPixel, LayerPixel etc.

  - In PointTyped, RectTyped, etc., static_assert that the template argument
    satisfies IsPixel.

(With Concepts Lite (proposed for post-C++14), this would look much nicer: we would make a Pixel concept, have CSSPixel, LayerPixel etc. model it, and then declare the "type" of PointTyped's template parameter to be Pixel, which would cause the compiler to enforce that it is only instantiated with a type modeling the Pixel concept.)
That sounds like a reasonable solution.
Attached patch bug1022381.patch (obsolete) — Splinter Review
This patch implements the proposed resolution. Please feel free to flag for additional review if you feel it's appropriate.
Assignee: nobody → botond
Attachment #8437774 - Flags: review?(bugmail.mozilla)
Attached patch bug1022381.patchSplinter Review
Whoops, had some typos in there. Fixed.
Attachment #8437774 - Attachment is obsolete: true
Attachment #8437774 - Flags: review?(bugmail.mozilla)
Attachment #8437776 - Flags: review?(bugmail.mozilla)
Comment on attachment 8437776 [details] [diff] [review]
bug1022381.patch

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

r=me for the FrameMetrics and Units changes. gfx/2d/ should be reviewed by Bas.
Attachment #8437776 - Flags: review?(bugmail.mozilla)
Attachment #8437776 - Flags: review?(bas)
Attachment #8437776 - Flags: review+
Build-only Try push: https://tbpl.mozilla.org/?tree=Try&rev=b9bd7ac1eb7b
https://hg.mozilla.org/mozilla-central/rev/f35714275523
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Attachment #8437776 - Flags: review?(bas) → review+
Thanks Bas! (I didn't realize I landed this without waiting for your review - sorry!)
You need to log in before you can comment on or make changes to this bug.