Closed
Bug 1096132
Opened 10 years ago
Closed 10 years ago
clarify use of "scale" in nsDeviceContext
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: karlt, Unassigned)
Details
Attachments
(3 files, 1 obsolete file)
14.68 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
2.38 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
6.47 KB,
patch
|
tnikkel
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
Attachment #8519655 -
Flags: review?(tnikkel)
Reporter | ||
Comment 2•10 years ago
|
||
Attachment #8519656 -
Flags: review?(tnikkel)
Reporter | ||
Comment 3•10 years ago
|
||
Attachment #8519657 -
Flags: review?(tnikkel)
Reporter | ||
Comment 4•10 years ago
|
||
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)
Comment 5•10 years ago
|
||
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•10 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•10 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.
Comment 8•10 years ago
|
||
ConvertToUnitFullZoomDevPixels?
Reporter | ||
Comment 9•10 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)
Updated•10 years ago
|
Attachment #8519655 -
Flags: review?(tnikkel) → review+
Updated•10 years ago
|
Attachment #8519656 -
Flags: review?(tnikkel) → review+
Updated•10 years ago
|
Attachment #8519657 -
Flags: review?(tnikkel) → review+
Reporter | ||
Comment 10•10 years ago
|
||
Landed temporarily on inbound, but backed out because I missed an UnscaledAppUnitsPerDevPixel call in nsNativeThemeCocoa.mm. https://hg.mozilla.org/integration/mozilla-inbound/rev/db8796b9eea3 https://hg.mozilla.org/integration/mozilla-inbound/rev/aa4fc081b48e https://hg.mozilla.org/integration/mozilla-inbound/rev/b932dbcaf0c1 https://hg.mozilla.org/integration/mozilla-inbound/rev/f96e3c57e1d8
Keywords: leave-open
Reporter | ||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2d73c49836c https://hg.mozilla.org/integration/mozilla-inbound/rev/69fc190b3118 https://hg.mozilla.org/integration/mozilla-inbound/rev/5081398a2e85
Keywords: leave-open
Comment 12•10 years ago
|
||
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
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•