clarify use of "scale" in nsDeviceContext

RESOLVED FIXED in mozilla36

Status

()

Core
Layout
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: karlt, Unassigned)

Tracking

36 Branch
mozilla36
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
The concept of UnscaledAppUnitsPerDevPixel() and mAppUnitsPerDevNotScaledPixel
for page zoom dates back to implementation in Bug 4821.

Now that we have a GetDefaultScale, which means something quite different, it
would be helpful to distinguish the "scale" from the page full zoom identifiers.
(Reporter)

Comment 1

4 years ago
Created attachment 8519655 [details] [diff] [review]
rename UnscaledAppUnitsPerDevPixel() to AppUnitsPerDevPixelAtUnitZoom()
Attachment #8519655 - Flags: review?(tnikkel)
(Reporter)

Comment 2

4 years ago
Created attachment 8519656 [details] [diff] [review]
rename UpdateScaledAppUnits() to UpdateAppUnitsForZoom()
Attachment #8519656 - Flags: review?(tnikkel)
(Reporter)

Comment 3

4 years ago
Created attachment 8519657 [details] [diff] [review]
rename nsDeviceContext PixelScale to FullZoom
Attachment #8519657 - Flags: review?(tnikkel)
(Reporter)

Comment 4

4 years ago
Created attachment 8519660 [details] [diff] [review]
rename ConvertToUnscaledDevPixels() to ConvertToUnzoomedDevPixels()

I'm still not entirely happy with the name here, but I think it is an
improvement to remove any suggestion that this is related to not applying the
window "DefaultScale".

AIUI the reason for not applying zoom is that screen coordinates do not change
under page full zoom.
Attachment #8519660 - Flags: review?(tnikkel)
I would like it if we also made it harder to confuse the concepts of the desktop "full zoom" from what we do on b2g and android, the "pinch zoom". Although "full zoom" isn't a great term, it's better than nothing.

Do you think we could put "full" everywhere you are renaming to zoom here? I'm also open to better suggestions. Only the last patch might be hard, not sure how to say un-"full zoom"'ed.
(Reporter)

Comment 6

4 years ago
I thought I might be able to get away with just Zoom on/in the device context because it is the device context zoom, but I'm happy to add "Full" or "Page", whichever you prefer, if that helps.

Re ConvertToUnscaledDevPixels, the input is screen coords but not ScreenPixels.
These are often called display pixels in widget code, though that is not a natural term to me.  They are a kind of device-independent pixel and are often called CSS Pixels.  It is really multiplying by GetWindowScale() but we have an nsPresContext instead of an nsIWidget.

Possible options:

ConvertDisplayPixelsToDevPixels
ConvertScreenCoordsToDevPixels
ConvertCSSPixelsToDevPixels
(Reporter)

Comment 7

4 years ago
So ConvertToUnscaledDevPixels is applying the window "default" scale, and perhaps the name is OKish as it is.  It just feels like a weird way of saying it to me.  It is really ScaleToDevPixels.
ConvertToUnitFullZoomDevPixels?
(Reporter)

Comment 9

4 years ago
Comment on attachment 8519660 [details] [diff] [review]
rename ConvertToUnscaledDevPixels() to ConvertToUnzoomedDevPixels()

Much of the widget code is confused about what display pixels are, and view
code uses the term for two different things.
Most code seems to assume display pixels are Gecko CSS/HTML/DOM screen units,
which are related to device pixels by nsIWidget::GetDefaultScale().

Some code assumes display pixels are the OS device-independent pixels, which
are related to device pixels by Cocoa's backingScaleFactor, or NT's dpi ratio.
Much is this is probably buggy, but nsView::CalcWidgetBounds()'s use of
nsIWidget::RoundsWidgetCoordinatesTo() looks correct.

I also see that nsIBaseWindow.idl has unscaledDevicePixelsPerCSSPixel (which is GetDefaultScale()) and I'm not wanting to change a scriptable API.  ConvertToUnscaledDevPixels() is consistent with that, so I think I'll leave that as is, and focus on nsDeviceContext in this bug.
Attachment #8519660 - Attachment is obsolete: true
Attachment #8519660 - Flags: review?(tnikkel)
Attachment #8519655 - Flags: review?(tnikkel) → review+
Attachment #8519656 - Flags: review?(tnikkel) → review+
Attachment #8519657 - Flags: review?(tnikkel) → review+
https://hg.mozilla.org/mozilla-central/rev/f2d73c49836c
https://hg.mozilla.org/mozilla-central/rev/69fc190b3118
https://hg.mozilla.org/mozilla-central/rev/5081398a2e85
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.