651018 - Clean up device-invariant scale conversions

Status: NEW

(Core :: Graphics, enhancement)

Description

[Zack Weinberg (:zwol)]

Presently conversions between app units and CSS pixels/inches are done with device context and/or pres context methods, even though they're fixed.  This patch series will move these conversions to free functions in nsCoord.h (thus, code that needs them can get at them without even including nsIDeviceContext.h or nsIPresContext.h -- yes, there are files that include one of those headers exclusively for those conversions).

This patch series will also purge as many remaining references to "twips" from the codebase as possible.  In many places "twips" was a misnomer for app units; in almost all of the remaining places, we were double-converting, some external unit (inches, points, centimeters) to twips to app units, or vice versa.  There remains one lousy preference that is being written out in actual twips :-( -- if anything's to be done about that it'll be when I get round to vivisecting nsIPrintSettings.

Comment 1

[Zack Weinberg (:zwol)]

Attachment: part 1: prune nsCoord.h

nsCoord.h has a fair bit of stuff in it that nobody uses.  This patch prunes all of that and makes the scale-factor #defines uniform.  There's also a little en-passant tidying in the SVG code.  And NS_IEEEPositiveInfinity() and NS_IEEEIsNan() are moved to nsMathUtils.h and renamed to match C99.

Comment 2

[Zack Weinberg (:zwol)]

Attachment: part 2: AppUnitsPerCSS* out of pres/device context

This removes most of the device-independent scaling functions from nsPresContext and nsDeviceContext.  They're replaced either with the constants from the previous patch, or with free functions in nsCoord.h (similar to the conversion functions that are already there).

Comment 3

[Zack Weinberg (:zwol)]

Attachment: part 3: zap all references to "twips" that really mean app units

An awful lot of the places that refer to "twips" really mean app units; fix 'em.

There will be one more piece of this bug, which gets rid of all but one of the places where twips - 1/20th of a Postscript point - were actually being used; but it blew up on the try server, so I'm not posting it yet. :)

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 526915 [details] [diff] [review]
part 1: prune nsCoord.h

+#define PIXELS_PER_INCH      96.0f
+
+#define PIXELS_PER_MM        (PIXELS_PER_INCH /     MM_PER_INCH)
+#define PIXELS_PER_CM        (PIXELS_PER_INCH /     CM_PER_INCH)
+#define PIXELS_PER_POINT     (PIXELS_PER_INCH / POINTS_PER_INCH)
+#define PIXELS_PER_PICA      (PIXELS_PER_INCH /  PICAS_PER_INCH)
+
+#define APP_UNITS_PER_PIXEL  60.0f
+#define APP_UNITS_PER_INCH   (APP_UNITS_PER_PIXEL * PIXELS_PER_INCH)
+#define APP_UNITS_PER_MM     (APP_UNITS_PER_PIXEL * PIXELS_PER_MM)
+#define APP_UNITS_PER_CM     (APP_UNITS_PER_PIXEL * PIXELS_PER_CM)
+#define APP_UNITS_PER_POINT  (APP_UNITS_PER_PIXEL * PIXELS_PER_POINT)
+#define APP_UNITS_PER_PICA   (APP_UNITS_PER_PIXEL * PIXELS_PER_PICA)

These are all CSS-specific. I think these should all include "CSS" in their names to make it clear that's what we're talking about. So CSS_PIXELS_PER_*, and APP_UNITS_PER_CSS_*.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

-    nsRect r(nsPresContext::CSSPixelsToAppUnits(aX),
-             nsPresContext::CSSPixelsToAppUnits(aY),
-             nsPresContext::CSSPixelsToAppUnits(aW),
-             nsPresContext::CSSPixelsToAppUnits(aH));
+    nsRect r(NSCSSPixelsToAppUnits(aX),
+             NSCSSPixelsToAppUnits(aY),
+             NSCSSPixelsToAppUnits(aW),
+             NSCSSPixelsToAppUnits(aH));

I can live with this pattern, but I'd actually prefer to use helper functions to convert these rects (and similar stuff for points).
E.g. rewrite this to be
  nsRect r = nsIntRect(aX, aY, aW, aH).ToAppUnits(APP_UNITS_PER_CSS_PIXEL);

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #4)
> These are all CSS-specific. I think these should all include "CSS" in their
> names to make it clear that's what we're talking about. So CSS_PIXELS_PER_*,
> and APP_UNITS_PER_CSS_*.

Also I wonder if we should worry about these not being namespaced in any way. It would be nice if they were in "namespace mozilla;" but I guess that would require a lot of "using namespace mozilla;" additions...

Comment 7

[Zack Weinberg (:zwol)]

(In reply to comment #4)
> flag: review?([e-mail]) => review+Comment on attachment 526915 [details] [diff] [review]
> 
> These are all CSS-specific. I think these should all include "CSS" in their
> names to make it clear that's what we're talking about. So CSS_PIXELS_PER_*,
> and APP_UNITS_PER_CSS_*.

Ok, will change.

(In reply to comment #5)
> +    nsRect r(NSCSSPixelsToAppUnits(aX),
> +             NSCSSPixelsToAppUnits(aY),
> +             NSCSSPixelsToAppUnits(aW),
> +             NSCSSPixelsToAppUnits(aH));
> 
> I can live with this pattern, but I'd actually prefer to use helper functions
> to convert these rects (and similar stuff for points).

This feels like inappropriate mission creep for *this* bug.  How's this sound: you add the helper functions in your Rect/Size/Point/etc merge bug, and then if you file a bug to find patterns like this and switch 'em, and assign it to me, I'll do the gruntwork.

(In reply to comment #6)
> (In reply to comment #4)
> > These are all CSS-specific. I think these should all include "CSS" in their
> > names to make it clear that's what we're talking about. So CSS_PIXELS_PER_*,
> > and APP_UNITS_PER_CSS_*.
> 
> Also I wonder if we should worry about these not being namespaced in any way.
> It would be nice if they were in "namespace mozilla;" but I guess that would
> require a lot of "using namespace mozilla;" additions...

Also feels like mission creep, particularly as I hesitate to move only *part* of the contents of this file to namespace mozilla, but moving all of it would be gigantic (and I'd want better tools than grep -w to do it with, i.e. I'd rather one of the static-analysis-and-rewrite guys did it :-)

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #7)
> (In reply to comment #5)
> > +    nsRect r(NSCSSPixelsToAppUnits(aX),
> > +             NSCSSPixelsToAppUnits(aY),
> > +             NSCSSPixelsToAppUnits(aW),
> > +             NSCSSPixelsToAppUnits(aH));
> > 
> > I can live with this pattern, but I'd actually prefer to use helper functions
> > to convert these rects (and similar stuff for points).
> 
> This feels like inappropriate mission creep for *this* bug.  How's this sound:
> you add the helper functions in your Rect/Size/Point/etc merge bug, and then if
> you file a bug to find patterns like this and switch 'em, and assign it to me,
> I'll do the gruntwork.

Fair enough.

> (In reply to comment #6)
> > (In reply to comment #4)
> > > These are all CSS-specific. I think these should all include "CSS" in their
> > > names to make it clear that's what we're talking about. So CSS_PIXELS_PER_*,
> > > and APP_UNITS_PER_CSS_*.
> > 
> > Also I wonder if we should worry about these not being namespaced in any way.
> > It would be nice if they were in "namespace mozilla;" but I guess that would
> > require a lot of "using namespace mozilla;" additions...
> 
> Also feels like mission creep, particularly as I hesitate to move only *part*
> of the contents of this file to namespace mozilla, but moving all of it would
> be gigantic (and I'd want better tools than grep -w to do it with, i.e. I'd
> rather one of the static-analysis-and-rewrite guys did it :-)

Sounds reasonable except that you're adding more of these not-at-all-namespaced names. But I guess it doesn't matter enough.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Call me nitpicky, but ROUND_TO_AU should be ROUND_TO_APPUNITS IMHO, partly because "AU" is an actual length unit which is rather different from an appunit :-). Ditto for the other AU macros.

Comment 10

[Benjamin Smedberg]

Resetting owner to default per Zack's request.