Last Comment Bug 537890 - Implement new DPI system
: Implement new DPI system
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 All
: -- normal with 2 votes (vote)
: ---
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
: Milan Sreckovic [:milan]
Mentors:
: 507201 555665 573359 (view as bug list)
Depends on: 588627 588664 588977 589002 595039
Blocks: 588464
  Show dependency treegraph
 
Reported: 2010-01-05 00:41 PST by Robert O'Callahan (:roc) (email my personal email if necessary)
Modified: 2016-10-01 01:11 PDT (History)
35 users (show)
roc: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1: Create new DPI APIs in nsIWidget (11.13 KB, patch)
2010-01-05 17:17 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
Part 2: Fetch DPI from the system (6.23 KB, patch)
2010-01-05 17:18 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
karlt: review+
Details | Diff | Splinter Review
Part 3: remove gfxPlatform::GetDPI (11.99 KB, patch)
2010-01-05 17:22 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
vladimir: review+
Details | Diff | Splinter Review
Part 4: Make pt a non-physical unit (14.60 KB, patch)
2010-01-05 17:23 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
dbaron: review+
Details | Diff | Splinter Review
Part 5: Make picas a non-physical unit (3.36 KB, patch)
2010-01-05 17:25 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
dbaron: review+
Details | Diff | Splinter Review
Part 6: switch DPI dynamically when a window moves across screens on Mac (6.28 KB, patch)
2010-01-05 18:19 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
dbaron: review+
Details | Diff | Splinter Review
Part 1 v2 (11.71 KB, patch)
2010-01-05 18:38 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
vladimir: review+
dbaron: superreview+
Details | Diff | Splinter Review
Part 2 v2 (6.25 KB, patch)
2010-01-05 18:39 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
jmathies: review+
jaas: review+
Details | Diff | Splinter Review
Part 7: Tests (2.86 KB, patch)
2010-01-05 19:07 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
Part 8: Define MM_PER_INCH_FLOAT and use it (11.88 KB, patch)
2010-01-06 14:17 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
dbaron: review+
Details | Diff | Splinter Review
Part 9: Fix accessibility (1.16 KB, patch)
2010-01-06 14:21 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
surkov.alexander: review+
Details | Diff | Splinter Review
Part 10: Force DPI for reftest-print to 96 (6.73 KB, patch)
2010-01-06 14:22 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
Part 11: fix other tests that ssume a relationship between inches and pt (20.40 KB, patch)
2010-01-06 14:25 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
Part 4 v2 (15.53 KB, patch)
2010-06-28 04:47 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
dbaron: review+
Details | Diff | Splinter Review
Part 4.5: Create mozmm physical unit (4.52 KB, patch)
2010-06-28 04:48 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
dbaron: review+
Details | Diff | Splinter Review
Part 5 v2: make pc, in, cm and mm all fixed multiples of CSS pixels (5.71 KB, patch)
2010-06-28 04:52 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
dbaron: review+
Details | Diff | Splinter Review
Part 7 v2: Updated tests (3.43 KB, patch)
2010-06-28 04:54 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
Part 10: Fix nsPresContext and nsIDeviceContext unit-conversion methods to be explicit about using CSS logical units instead of physical units (47.95 KB, patch)
2010-06-29 03:59 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
dbaron: review+
Details | Diff | Splinter Review
Part 11: Audit _TO_TWIPS macro usage (15.46 KB, patch)
2010-08-10 21:07 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
dbaron: review+
Details | Diff | Splinter Review
Firefox 6 at 120 DPI. Note how the browser chrome is correct but the CSS dimensions are wrong (180.59 KB, image/png)
2011-09-04 06:45 PDT, Tom Edwards
no flags Details

Description Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-05 00:41:20 PST
We want to disable automatic scaling for now. When automatic scaling is reenabled, it should depend on platform settings such as "UI Resolution" on Mac.

We should get the true DPI for the screen and report it on all platforms.

I also think we should get the DPI through an API on nsIWidget, so that it can be per-widget ... i.e., creating a window on a new screen should get the right DPI.
Comment 1 Vladimir Vukicevic [:vlad] [:vladv] 2010-01-05 11:42:52 PST
(In reply to comment #0)
> I also think we should get the DPI through an API on nsIWidget, so that it can
> be per-widget ... i.e., creating a window on a new screen should get the right
> DPI.

I don't know that this is a good idea -- what do you do when the window moves to another screen?  Or when it straddles two screens?  Or are you just referring to linux without xinerama or something, where an X Screen is isolated from all others?
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-05 12:31:01 PST
(In reply to comment #1)
> I don't know that this is a good idea -- what do you do when the window moves
> to another screen?  Or when it straddles two screens?

Short-term answer: you lose, in that case. But we still get things right when you open a new window on a particular screen, or even reload a document while the window is on the right screen. We don't need to make the best the enemy of the good.

Long-term answer: it wouldn't actually be that hard to detect when the window has moved from one screen to another (or when the screen that the window is "mostly on" has changed) and fire some kind of event to perform a dynamic DPI change. We already support dynamic DPI changes when the layout.css.dpi pref changes. This could also handle the case when the screen DPI changes due to a mode change or other screen reconfiguration.
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-05 17:17:30 PST
Created attachment 420228 [details] [diff] [review]
Part 1: Create new DPI APIs in nsIWidget

This patch makes layout.css.devPixelsPerPx default to 1.0, which means nsIWidget::GetDefaultScale() will be ignored even if it gets implemented. I plan to file followup bugs to implement GetDefaultScale on each platform; having its usage prefed off by default will make it easier to implement GetDefaultScale incrementally.
Comment 4 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-05 17:18:37 PST
Created attachment 420229 [details] [diff] [review]
Part 2: Fetch DPI from the system

Adds code to implement nsIWidget::GetDPI on Mac, Windows and X.
Comment 5 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-05 17:22:48 PST
Created attachment 420231 [details] [diff] [review]
Part 3: remove gfxPlatform::GetDPI

Remove gfxPlatform::GetDPI() now that it's no longer needed. For some Pango/GTK/Qt font stuff we still need some notion of DPI, but I believe this is really looking for "preferred font size" data. Anyway, I'm leaving behaviour unchanged there; Qt just uses 96 (since gfxQtPlatform doesn't implement InitDisplayCaps) and for GTK, gfxPlatformGtk::GetDPI does what gfxPlatformGtk::InitDisplayCaps used to do.
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-05 17:23:59 PST
Created attachment 420233 [details] [diff] [review]
Part 4: Make pt a non-physical unit

Also does some refactoring. I guess there is still some question about whether we should do this. I think we should.
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-05 17:25:11 PST
Created attachment 420234 [details] [diff] [review]
Part 5: Make picas a non-physical unit

There is also some question about whether we should do this.
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-05 17:29:41 PST
I think the above set of patches can land together to leave us in a good, shippable state, one that we can build on cleanly.

Supporting stuff like Windows' "font DPI" settings will require more work. I will file followup bugs to implement GetDefaultScale() on each platform, and also to add a media query feature to detect the device-pixels-per-CSS-pixel ratio. The latter is needed so that themes can offer artwork customized to particular resolutions, which we'll want before we enable scaling by default.
Comment 9 Karl Tomlinson (back Dec 13 :karlt) 2010-01-05 17:47:02 PST
Comment on attachment 420228 [details] [diff] [review]
Part 1: Create new DPI APIs in nsIWidget

>+     * Return the physical DPI of the screen containing the window ...
>+     * the number of device pixels per inch.
>+     */
>+    virtual PRInt32 GetDPI() = 0;

Shouldn't this return double (or float)?
Comment 10 Karl Tomlinson (back Dec 13 :karlt) 2010-01-05 18:11:32 PST
Comment on attachment 420229 [details] [diff] [review]
Part 2: Fetch DPI from the system

>+    Display *dpy = GDK_DISPLAY();
>+    int defaultScreen = 0;

= DefaultScreen(dpy);

>+    return NS_lround(DisplayHeight(dpy, defaultScreen)/heightInches);

Rounding may be significant with projectors, and a double return value gives a better hint as to what the function is calculating.
Comment 11 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-05 18:15:10 PST
I'll fix.
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-05 18:19:35 PST
Created attachment 420243 [details] [diff] [review]
Part 6: switch DPI dynamically when a window moves across screens on Mac

This is basically just to annoy Vlad. MacOS makes it really easy to be notified when a window moves across screens, so take advantage of that and switch the DPI dynamically. The system chooses the screen that contains the most window area. The notification also fires when you change the screen's mode, which is nice.
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-05 18:38:50 PST
Created attachment 420245 [details] [diff] [review]
Part 1 v2

Made GetDPI return float.
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-05 18:39:53 PST
Created attachment 420246 [details] [diff] [review]
Part 2 v2

Make GetDPI return float and address Karl's comments
Comment 15 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-05 19:07:12 PST
Created attachment 420249 [details] [diff] [review]
Part 7: Tests

These tests check that pc/pt don't vary with DPI, and that mm/cm/in do vary (correctly) with DPI.
Comment 16 Mark Finkle (:mfinkle) (use needinfo?) 2010-01-05 20:27:48 PST
Note to self: We have a patch in bug 524919 that creates the true DPI for Qt. We could update attachment 420246 [details] [diff] [review] or land a followup:

https://bugzilla.mozilla.org/attachment.cgi?id=414047&action=edit
Comment 17 Vladimir Vukicevic [:vlad] [:vladv] 2010-01-05 21:28:14 PST
(In reply to comment #12)
> Created an attachment (id=420243) [details]
> Part 6: switch DPI dynamically when a window moves across screens on Mac
> 
> This is basically just to annoy Vlad. MacOS makes it really easy to be notified
> when a window moves across screens, so take advantage of that and switch the
> DPI dynamically. The system chooses the screen that contains the most window
> area. The notification also fires when you change the screen's mode, which is
> nice.

Grumble. I still think that's going to annoy users, especially if it's the default -- do any other mac apps behave this way?
Comment 18 Vladimir Vukicevic [:vlad] [:vladv] 2010-01-05 21:32:28 PST
Comment on attachment 420231 [details] [diff] [review]
Part 3: remove gfxPlatform::GetDPI

Looks fine, though I guess the Gtk bit is just a temporary workaround until we figure out the right way to do this with the crazy per-screen dpi stuff?

I wonder if it would be better to move the GetDPI call into gfxPangoFonts to make sure nothing else starts using it accidentally.  It's fine either way, though.
Comment 19 Mike Beltzner [:beltzner, not reading bugmail] 2010-01-05 21:33:51 PST
In cases where it straddles, I don't think we should change the DPI unless the entire window has moved from one screen DPI to the other, or at least the vast majority (like, more than 85% of the window area). If I toss a window aside for a second in order to see something, I don't want my font sizes changing on me.
Comment 20 Josh Aas 2010-01-05 22:08:23 PST
Comment on attachment 420246 [details] [diff] [review]
Part 2 v2

>+  return (heightPx/scaleFactor)/(heightMM/25.4f);

You could probably put 25.4f in nsBaseWidget.h so we don't write it out 3 times. Maybe do the same for 96.0f.

I prefer space around operators, "x / y" instead of "x/y", that's how we've tried to do it everywhere else in Cocoa widgets. I think it is easier to read.
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-05 23:24:40 PST
(In reply to comment #18)
> (From update of attachment 420231 [details] [diff] [review])
> Looks fine, though I guess the Gtk bit is just a temporary workaround until we
> figure out the right way to do this with the crazy per-screen dpi stuff?

I dunno, there may be no way to do this right on X.

But picking an arbitrary screen and using its DPI for all windows is at least no worse from what we were doing before with a global DPI in gfxPlatform.
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-05 23:30:11 PST
(In reply to comment #17)
> Grumble. I still think that's going to annoy users, especially if it's the
> default -- do any other mac apps behave this way?

I don't know of any other Mac apps that use physical units.

(In reply to comment #19)
> In cases where it straddles, I don't think we should change the DPI unless the
> entire window has moved from one screen DPI to the other, or at least the vast
> majority (like, more than 85% of the window area). If I toss a window aside for
> a second in order to see something, I don't want my font sizes changing on me.

This only affects CSS dimensions specified in 'mm', 'cm' and 'in'. They are hardly ever used, so users aren't going to notice in general. Nor will the differences be very noticeable if your screens have similar DPI. If the DPIs are very different you'll probably be constantly annoyed that graphics spanning the screen boundary don't line up. The cases where this might be annoying are tiny tiny edge cases IMHO.

(In reply to comment #20)
> (From update of attachment 420246 [details] [diff] [review])
> >+  return (heightPx/scaleFactor)/(heightMM/25.4f);
> 
> You could probably put 25.4f in nsBaseWidget.h so we don't write it out 3
> times. Maybe do the same for 96.0f.
> 
> I prefer space around operators, "x / y" instead of "x/y", that's how we've
> tried to do it everywhere else in Cocoa widgets. I think it is easier to read.

OK.
Comment 23 Karl Tomlinson (back Dec 13 :karlt) 2010-01-05 23:41:47 PST
(In reply to comment #21)
> (In reply to comment #18)
> > Looks fine, though I guess the Gtk bit is just a temporary workaround until we
> > figure out the right way to do this with the crazy per-screen dpi stuff?
> 
> I dunno, there may be no way to do this right on X.
> 
> But picking an arbitrary screen and using its DPI for all windows is at least
> no worse from what we were doing before with a global DPI in gfxPlatform.

gfxPlatformGtk::GetDPI is a logical font dpi (just for converting Pango-points/pixels).  There is only one value for all monitors on the Screen.
(Mozilla only runs on one Screen.)

nsIWidget::GetDPI is the physical dpi.  It looks like that information would be available for each monitor through the RANDR extension (newer versions at least), but the existing info from the entire Screen is much simpler to use and is a good start.
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-06 14:06:26 PST
(In reply to comment #20)
> (From update of attachment 420246 [details] [diff] [review])
> >+  return (heightPx/scaleFactor)/(heightMM/25.4f);
> 
> You could probably put 25.4f in nsBaseWidget.h so we don't write it out 3
> times. Maybe do the same for 96.0f.

I'll add MM_PER_INCH_FLOAT to nsCoord.h. I won't do anything for 96.0f, since it's an arbitrary default and there's no real reason it needs to be consistent across widget implementations.

> I prefer space around operators, "x / y" instead of "x/y", that's how we've
> tried to do it everywhere else in Cocoa widgets. I think it is easier to read.

Will do.
Comment 25 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-06 14:17:33 PST
Created attachment 420410 [details] [diff] [review]
Part 8: Define MM_PER_INCH_FLOAT and use it

Amusingly, this fixes a bug in SVG.
Comment 26 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-06 14:21:12 PST
Created attachment 420411 [details] [diff] [review]
Part 9: Fix accessibility

Accessibility tests failed because accessibility's font-size text attribute returns values in pt, and accessibility was using the screen DPI to compute the pt, which no longer matches CSS pt when pt is non-physical. This patch fixes accessibility to report pt defined as 4/3 of a CSS pixel. I think this makes more sense for accessibility anyway; it doesn't make much sense to me for the font size reported by accessibility to depend on the DPI of the user's screen, especially if the user can't see the screen...
Comment 27 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-06 14:22:54 PST
Created attachment 420412 [details] [diff] [review]
Part 10: Force DPI for reftest-print to 96

Some print reftests failed because they make assumptions about pts and inches. It's not easy to remove these assumptions because page dimensions and margins are defined in terms of inches and twips. So this patch just forces the DPI to 96 in DocumentViewerImpl::SetPageMode.
Comment 28 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-06 14:25:03 PST
Created attachment 420413 [details] [diff] [review]
Part 11: fix other tests that ssume a relationship between inches and pt

Some other tests (mostly written by fantasai :-) ) assume there are always 72 points per inch. This changes those tests to use pt everywhere instead of inches, except for test_unit_length where we just temporarily force the DPI to 96.
Comment 29 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-07 02:57:26 PST
We probably need to do something to ensure that pt and pc have their correct physical interpretation when printing. I guess we should ensure that px is exactly 1/96 of an inch.
Comment 30 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-07 12:01:04 PST
Comment on attachment 420412 [details] [diff] [review]
Part 10: Force DPI for reftest-print to 96

This may not be necessary depending on how the DPI discussion evolves.
Comment 31 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-07 12:04:08 PST
There's an evolving discussion on www-style about what we should do here.

Partly thanks to Jonathan, I'm coming around to the view that we should redefine all existing units to be "non-physical" in non-print contexts, and create a new unit (say, "truemm") to be a physical unit in all contexts. This would let authors use in, cm, etc to get the correct dimensions when printed but still be usable on a variety of screens.
Comment 32 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-16 01:12:10 PST
The discussion on www-style seems to have mostly converged on redefining all existing units to be non-physical for non-print media.

It's less clear whether there will be a true physical unit or what it would be called. Since we have a clear use-case for chrome, I suggest we create our own new unit "mozmm".
Comment 33 David Baron :dbaron: ⌚️UTC-10 2010-01-20 17:10:57 PST
Comment on attachment 420410 [details] [diff] [review]
Part 8: Define MM_PER_INCH_FLOAT and use it

I suppose the loss in precision in the places operating on doubles isn't a real problem?

(Are there any tests about those places in SVG code that were using 24.4 instead of 25.4 for 'pc' units?)

r=dbaron
Comment 34 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-01-24 18:16:24 PST
David, does the plan in comment #32 sounds OK to you? If so, I'll implement it. That would obsolete parts 4 and 5, and make part 11 unnecessary.
Comment 35 David Baron :dbaron: ⌚️UTC-10 2010-03-13 19:46:42 PST
Comment on attachment 420245 [details] [diff] [review]
Part 1 v2

sr=dbaron
Comment 36 David Baron :dbaron: ⌚️UTC-10 2010-03-13 19:50:31 PST
Comment on attachment 420243 [details] [diff] [review]
Part 6: switch DPI dynamically when a window moves across screens on Mac

r=dbaron
Comment 37 David Baron :dbaron: ⌚️UTC-10 2010-03-13 19:52:12 PST
Comment on attachment 420234 [details] [diff] [review]
Part 5: Make picas a non-physical unit

r=dbaron
Comment 38 David Baron :dbaron: ⌚️UTC-10 2010-03-15 10:42:44 PDT
Comment on attachment 420233 [details] [diff] [review]
Part 4: Make pt a non-physical unit

>+nscoord nsCSSValue::GetPixelLength() const
>+  case eCSSUnit_Point:
>+    return nsPresContext::CSSPixelsToAppUnits(mValue.mFloat*4/3);

Maybe "* (4.0/3.0)" instead of "*4/3"?

>   PRBool    IsRelativeLengthUnit() const  
>-    { return eCSSUnit_EM <= mUnit && mUnit <= eCSSUnit_Pixel; }
>+    { return eCSSUnit_EM <= mUnit && mUnit <= eCSSUnit_RootEM; }
>+  PRBool    IsPixelLengthUnit() const
>+    { return eCSSUnit_Point <= mUnit && mUnit <= eCSSUnit_Pixel; }

Add a comment that what the spec calls relative length units is, for us, split between relative length units and pixel length units.  I like the new terminology better, but we should have a comment saying that it's different from CSS 2.1.

>-    zoom = aFontData.mSize.IsFixedLengthUnit() ||
>-           aFontData.mSize.GetUnit() == eCSSUnit_Pixel;
>+    zoom = aFontData.mSize.IsLengthUnit() &&
>+      !aFontData.mSize.IsRelativeLengthUnit();

Please keep the indentation style matching the surrounding code.

r=dbaron with that
Comment 39 David Baron :dbaron: ⌚️UTC-10 2010-03-30 17:03:59 PDT
The CSS WG seems very against breaking the relationships between pt/pc/in/cm/mm.  The current proposal in the WG seems to be to require that 3pt == 4px (i.e., 96px == 72pt == 1in), and the UA can decide whether to peg the whole set of units to physical units or display pixels.
Comment 40 Jo Hermans 2010-06-20 11:57:40 PDT
*** Bug 573359 has been marked as a duplicate of this bug. ***
Comment 41 Jo Hermans 2010-06-20 11:58:02 PDT
*** Bug 555665 has been marked as a duplicate of this bug. ***
Comment 42 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-06-28 04:44:43 PDT
Comment on attachment 420412 [details] [diff] [review]
Part 10: Force DPI for reftest-print to 96

Not needed now that we're going to make inches and points a fixed multiple of CSS pixels on screen
Comment 43 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-06-28 04:44:51 PDT
Comment on attachment 420413 [details] [diff] [review]
Part 11: fix other tests that ssume a relationship between inches and pt

Not needed now that we're going to make inches and points a fixed multiple of CSS pixels on screen
Comment 44 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-06-28 04:47:51 PDT
Created attachment 454482 [details] [diff] [review]
Part 4 v2

Addressed review comments. You might want to reread the comments in nsCSSValue.h.

As in the previous version of the patch, this patch just makes points be tied to CSS pixels instead of physical, and refactors code a bit to make that easier. I will attach a new patch to make all the other current "physical" units also be tied to CSS pixels.
Comment 45 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-06-28 04:48:52 PDT
Created attachment 454483 [details] [diff] [review]
Part 4.5: Create mozmm physical unit

Mobile people need this.
Comment 46 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-06-28 04:52:01 PDT
Created attachment 454484 [details] [diff] [review]
Part 5 v2: make pc, in, cm and mm all fixed multiples of CSS pixels

As previously discussed, this makes all the CSS standard "physical" units be fixed multiples of CSS pixels, leaving only mozmm as an actual physical unit.
Comment 47 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-06-28 04:53:44 PDT
I've also updated the other patches to trunk and updated part 7, the tests. I'll reattach that for informational purposes.
Comment 48 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-06-28 04:54:26 PDT
Created attachment 454485 [details] [diff] [review]
Part 7 v2: Updated tests
Comment 49 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-06-29 03:59:22 PDT
Created attachment 454826 [details] [diff] [review]
Part 10: Fix nsPresContext and nsIDeviceContext unit-conversion methods to be explicit about using CSS logical units instead of physical units

Running tests I found one rather large issue I'd overlooked: nsPresContext has methods for converting between appunits and twips/inches/points, and they were still using physical units, which is mostly not what we want.

This patch renames those methods to CSSPointsToAppUnits, etc, and makes them static since they no longer depend on the device. This actually simplifies a some SVG code quite a bit.
Comment 50 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-06-29 04:13:58 PDT
Note that part 10 converts all the use of twips/inches for print margins etc to use CSS twips/inches instead of physical units. I think that's fine since for actual printing, CSS units match their physical counterparts.
Comment 51 David Baron :dbaron: ⌚️UTC-10 2010-08-10 15:37:12 PDT
Comment on attachment 454482 [details] [diff] [review]
Part 4 v2

I'm a bit concerned about the change in NS_*_TO_TWIPS NS_TWIPS_TO_* (via
the change in NSUnitsToTwips and NSTwipsToUnits) that makes them not
round anymore.  I'm inclined to say you should go through all the users
that aren't passing the result to nsPresContext::TwipsToAppUnits and
either (a) add the rounding or (b) convert to floats, or that you add a
new function for floats (overloading has serious danger of compilation
errors, though) for those callers that do use TwipsToAppUnits.  If you 
want to do the go-through-all-the-users, I'd prefer to see that as an 
additional patch (on top?).

Other than that this looks fine.
Comment 52 David Baron :dbaron: ⌚️UTC-10 2010-08-10 15:47:23 PDT
Comment on attachment 454484 [details] [diff] [review]
Part 5 v2: make pc, in, cm and mm all fixed multiples of CSS pixels

>+  eCSSUnit_Pica         = 904,    // (float) 12 points == 16 CSS pixls

s/pixls/pixels/

r=dbaron
Comment 53 David Baron :dbaron: ⌚️UTC-10 2010-08-10 16:05:24 PDT
Comment on attachment 454826 [details] [diff] [review]
Part 10: Fix nsPresContext and nsIDeviceContext unit-conversion methods to be explicit about using CSS logical units instead of physical units

I'm having a little trouble understanding the changes in 
nsThebesDeviceContext::CalcPrintingSize.  Are CSS and physical units 
interchangable in that case, since it's for printing?

I'm guessing it's also the case in the DocumentViewerImpl::InitInternal
changes, but there using CSS makes sense since it'll be faster.

r=dbaron
Comment 54 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-08-10 16:17:53 PDT
(In reply to comment #51)
> I'm a bit concerned about the change in NS_*_TO_TWIPS NS_TWIPS_TO_* (via
> the change in NSUnitsToTwips and NSTwipsToUnits) that makes them not
> round anymore.  I'm inclined to say you should go through all the users
> that aren't passing the result to nsPresContext::TwipsToAppUnits and
> either (a) add the rounding or (b) convert to floats,

I'll audit the callers to the _TO_TWIPS macros. For NS_TWIPS_TO and NSTwipsToUnits, I've only changed the parameter from an nscoord to a float, so I don't think anything needs to be checked there. Right?

(In reply to comment #53)
> I'm having a little trouble understanding the changes in 
> nsThebesDeviceContext::CalcPrintingSize.  Are CSS and physical units 
> interchangable in that case, since it's for printing?

Yes. I'll add a comment.

> I'm guessing it's also the case in the DocumentViewerImpl::InitInternal
> changes, but there using CSS makes sense since it'll be faster.

Right.
Comment 55 David Baron :dbaron: ⌚️UTC-10 2010-08-10 16:29:19 PDT
(In reply to comment #54)
> (In reply to comment #51)
> > I'm a bit concerned about the change in NS_*_TO_TWIPS NS_TWIPS_TO_* (via
> > the change in NSUnitsToTwips and NSTwipsToUnits) that makes them not
> > round anymore.  I'm inclined to say you should go through all the users
> > that aren't passing the result to nsPresContext::TwipsToAppUnits and
> > either (a) add the rounding or (b) convert to floats,
> 
> I'll audit the callers to the _TO_TWIPS macros. For NS_TWIPS_TO and
> NSTwipsToUnits, I've only changed the parameter from an nscoord to a float, so
> I don't think anything needs to be checked there. Right?

Er, right.  I originally had a comment mentioning only that, and then I changed it for some reason.

So I think you should be looking at all the users of *_TO_TWIPS that pass it to something other than NS_TWIPS_TO_* or nsPresContext::TwipsToAppUnits.

Be careful to use MXR text search rather than identifier search if you're using MXR; otherwise you'll miss the .mm files.
Comment 56 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-08-10 21:07:26 PDT
Created attachment 464696 [details] [diff] [review]
Part 11: Audit _TO_TWIPS macro usage

I added NS_POINTS_TO_INT_TWIPS and NS_INCHES_TO_INT_TWIPS and used them in various places that were calling _TO_TWIPS and expecting an integer ... pretty much all related to printing. The other sites were putting the result in a float.
Comment 57 David Baron :dbaron: ⌚️UTC-10 2010-08-10 22:29:34 PDT
Comment on attachment 464696 [details] [diff] [review]
Part 11: Audit _TO_TWIPS macro usage

Maybe you want NSToCoordRound rather than NSToIntRound?  They are different.  I'm not sure it matters.

r=dbaron
Comment 58 David Baron :dbaron: ⌚️UTC-10 2010-08-10 22:29:55 PDT
Comment on attachment 454482 [details] [diff] [review]
Part 4 v2

r=dbaron given part 11
Comment 59 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-08-10 22:50:01 PDT
I think I want NSToIntRound. NSToCoordRound is optimized but only works on values in the nscoord range.

dbaron, if you agree we should land this for FF4, can you give blanket approval to the patches here? A comment in the bug would suffice.
Comment 60 David Baron :dbaron: ⌚️UTC-10 2010-08-11 23:22:43 PDT
Yeah, I think we should.  a2.0=dbaron
Comment 61 Michael Ventnor 2010-08-13 06:46:05 PDT
(In reply to comment #18)
> Comment on attachment 420231 [details] [diff] [review]
> Part 3: remove gfxPlatform::GetDPI
> 
> Looks fine, though I guess the Gtk bit is just a temporary workaround until we
> figure out the right way to do this with the crazy per-screen dpi stuff?

From GDK 2.14 you do have functions for multi-monitor setups:

http://library.gnome.org/devel/gdk/stable/GdkScreen.html

gdk_screen_get_monitor_at_window()
and
gdk_screen_get_monitor_width_mm()

I can't find a signal for when the window's screen changes like on OSX. Maybe you need to listen for when the window moves and check if the monitor number changes.
Comment 62 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-08-13 16:36:54 PDT
This seems to have been responsible for test failures, so it was backed out.

MacOSX Debug geolocation test failure:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1281715052.1281717855.27052.gz

MacOSX and MacOSX64 Debug test_popup failures:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1281715052.1281717285.24668.gz

It may also have been responsible for a Txul regression, not sure yet.

I can't reproduce those failures locally, but I can on tryserver. Not sure why I didn't see those failures when I ran these patches through tryserver before. Maybe because Tinderbox was flakey at the time and those results were reported late.

When I get a chance I will try bisecting the patch queue on tryserver. That's not expensive since I can disable all builds except for MacOSX Debug.
Comment 63 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-08-16 20:27:47 PDT
Bisection on try-server shows that the test failures appear to have been caused by part 6, dynamic DPI changing on Mac. That's fine, we can leave that patch out.
Comment 65 Christopher Blizzard (:blizzard) 2010-08-20 13:16:26 PDT
Note for docs:

http://weblogs.mozillazine.org/roc/archives/2010/08/css_units_chang.html
Comment 66 Eric Shepherd [:sheppy] 2010-10-04 14:28:22 PDT
Documented here:

https://developer.mozilla.org/en/CSS-2_Quick_Reference/Units

And linked to from Firefox 4 for developers.
Comment 67 Felix Miata 2010-10-04 18:57:51 PDT
(In reply to comment #66)
Good to have that URL, but I'm disappointed to see there "In addition, Gecko has improved ability to correctly scale absolute lengths to the actual resolution of the rendering medium, whether that's the screen or paper." without either an explanation or a link to an explanation.

On every Linux I install, like many others do, I force DPI to be accurate, so I need to know how to make pt mean pt & cm mean cm instead of the perpetuation of the unfortunate legacy explained on http://blogs.msdn.com/fontblog/archive/2005/11/08/490490.aspx that lead to this "bug" and "fix". Until users universally can easily adjust the size of their own px, px values of more than 9 in author CSS should be rejected by all web browsers as invalid, and 96 as a standard should be relegated to history books.

Meanwhile, thanks for mozmm, which keeps my http://fm.no-ip.com/Auth/dpi-screen-window.html tool viable.
Comment 68 Dominic Fandrey 2011-03-25 02:41:59 PDT
I suppose mentioning this here will reach the people who care about DPI issues. Firefox4 renders everything at 96 DPI, no matter what I do, layout.css.dpi is completely ignored.

Tracking the issue down I found that gfx/src/thebes/nsThebesDeviceContext.cpp uses the PR_MAX macro in an unsafe manner, i.e. with function calls as parameters, which leads to very weird effects on my system. The macro substitution causes each function to be called twice and apparently they somehow interfere with each other and set stuff to 0 instead of a sensible value.

The result was that dpi always resulted in 96, no matter what was set in layout.css.dpi.

After fixing the issue I found that thebes has all the correct data on screen resolution and units, but layout.css.dpi still had no effect.

So I went in search for a central change to change this and ended up changing GetPixelLength() in layout/style/nsCSSValue.cpp. The function is hard coded to 96 DPI. I changed the signature to take a presentation context as an argument (the function is only called in 2 place in the entire source, so this wasn't a big deal) and made it use the information provided by thebes.

The result is that everything scales as expected I created a little dpi-meter at http://www.home.hs-karlsruhe.de/~fado0001/testdpi.html and not only does my trusty ruler tell me that the units are rendered perfectly, changing layout.css.dpi adjusts there rendering on the fly. I don't even have to reload the page.

My fix can be found in bug 603880, comment 5. I hope you will review it and take it into consideration.
Comment 69 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-25 03:37:44 PDT
Thanks for the info. I commented about the PR_MAX issues in bug 603880.

It is intentional that DPI values other than 96 (either automatically detected, or manually forced via layout.css.dpi) have little effect in Firefox 4. See http://weblogs.mozillazine.org/roc/archives/2010/01/css_absolute_le.html and http://weblogs.mozillazine.org/roc/archives/2010/08/css_units_chang.html. Since our new behavior matches the other major browsers and is necessary for many Web pages to be usable on devices like phones, we really have no choice. If you disagree with the decision, that's fine, but please express your disagreement elsewhere, perhaps on my blog, or on your own blog :-).

You can use mozmm if you really want a physical unit. Note that a mozmm-sized element that looks fine on a desktop screen is unlikely to be suitably sized for a phone, say, so the use-cases for mozmm are quite limited.
Comment 70 Tom Edwards 2011-09-04 06:29:52 PDT
*** Bug 507201 has been marked as a duplicate of this bug. ***
Comment 71 Tom Edwards 2011-09-04 06:41:48 PDT
I cannot understand that comment, Robert.

> It is intentional that DPI values other than 96 have little effect in Firefox 4.

No it is not. From your second link: "Currently GetDefaultScale always returns 1.0 on all platforms, although on Mac we should set it to the system 'default UI scale' and on Windows we should set it based on the 'system font DPI'."

> our new behavior matches the other major browsers

No it does not. Increase your DPI and Windows and test Chrome, Firefox and IE. Chrome and IE scale things up correctly, while Firefox does not.

> and is necessary for many Web pages to be usable on devices like phones

I can understand the need for phones to have sizing hacks. But there is no reason to force the same hacks onto the desktop!

> You can use mozmm if you really want a physical unit.

Who said anything about wanting a physical unit? High DPI is used all the time by people with poor eyesight who want things *to appear larger*.
Comment 72 Tom Edwards 2011-09-04 06:45:54 PDT
Created attachment 558144 [details]
Firefox 6 at 120 DPI. Note how the browser chrome is correct but the CSS dimensions are wrong
Comment 73 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-04 14:44:56 PDT
> > our new behavior matches the other major browsers
> 
> No it does not. Increase your DPI and Windows and test Chrome, Firefox and
> IE. Chrome and IE scale things up correctly, while Firefox does not.

Bug 603880 is about automatically scaling when you increase the Windows "font DPI". That is a valid bug. This bug was about changing behavior in response to screen DPI changes. Other browsers do not do that, and we match that now.

> Who said anything about wanting a physical unit? High DPI is used all the
> time by people with poor eyesight who want things *to appear larger*.

Right, you want bug 603880.

Note You need to log in before you can comment on or make changes to this bug.