nsPresContext ought to have CSSPixelsToDevPixels and DevPixelsToCSSPixels methods

RESOLVED FIXED

Status

()

Core
Layout
RESOLVED FIXED
9 years ago
3 years ago

People

(Reporter: db48x, Assigned: db48x)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

9 years ago
They're just AppUnitsToDevPixels ∘ CSSPixelsToAppUnits and AppUnitsToCSSPixels ∘ DevPixelsToAppUnits respectively, so it's not much more than an annoyance. It's already got every other combination though, so why not this one?
Daniel, unless roc objects, want to just post a patch?
(Assignee)

Comment 2

9 years ago
Sure, but after dinner.
(Assignee)

Updated

9 years ago
Blocks: 495192
(Assignee)

Comment 3

9 years ago
Created attachment 380111 [details] [diff] [review]
495002-1.diff

This just adds the functions; I haven't gone looking for any of the places that could call them.
Assignee: nobody → db48x
Status: NEW → ASSIGNED
Attachment #380111 - Flags: review?(bzbarsky)
(Assignee)

Updated

9 years ago
Attachment #380111 - Attachment is patch: true
Attachment #380111 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 380111 [details] [diff] [review]
495002-1.diff

DevPixelsToFloatCSSPixels should return a float.

There should be a version of CSSPixelsToDevPixels that takes a float, right?
Attachment #380111 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 5

9 years ago
Created attachment 380132 [details] [diff] [review]
495002-2.diff

(In reply to comment #4)
> (From update of attachment 380111 [details] [diff] [review])
> DevPixelsToFloatCSSPixels should return a float.

Yes, I suppose that would be best.
Attachment #380111 - Attachment is obsolete: true
Attachment #380132 - Flags: review?(bzbarsky)
Attachment #380132 - Flags: superreview?(roc)
Attachment #380132 - Flags: review?(bzbarsky)
Attachment #380132 - Flags: review+
Comment on attachment 380132 [details] [diff] [review]
495002-2.diff

Looks good; roc should sr, though
Attachment #380132 - Flags: superreview?(roc) → superreview+
Comment on attachment 380132 [details] [diff] [review]
495002-2.diff

Wait ...

+  PRInt32 CSSPixelsToDevPixels(float aPixels)
+  { return AppUnitsToDevPixels(CSSPixelsToAppUnits(aPixels)); }

Shouldn't this be CSSPixelsToFloatDevPixels, and return a float?
Attachment #380132 - Flags: superreview+ → superreview-
(Assignee)

Comment 8

9 years ago
(In reply to comment #7)
> (From update of attachment 380132 [details] [diff] [review])
> Wait ...
> 
> +  PRInt32 CSSPixelsToDevPixels(float aPixels)
> +  { return AppUnitsToDevPixels(CSSPixelsToAppUnits(aPixels)); }
> 
> Shouldn't this be CSSPixelsToFloatDevPixels, and return a float?

no, device pixels are never given as floats.
But if the input is a float, surely the output should be a float? The caller should round the result as they see fit
(Assignee)

Comment 10

9 years ago
Created attachment 380352 [details] [diff] [review]
495002-3.diff
Attachment #380132 - Attachment is obsolete: true
Attachment #380352 - Flags: superreview?
Attachment #380352 - Flags: review?(bzbarsky)
(Assignee)

Updated

9 years ago
Attachment #380352 - Flags: superreview? → superreview?(roc)
Attachment #380352 - Flags: superreview?(roc) → superreview+
Comment on attachment 380352 [details] [diff] [review]
495002-3.diff

It would be cool if you could find some existing places in the tree that should actually use these, so we're not just adding dead code. And I don't mean the screen patch I'm not keen on :-)
Attachment #380352 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 12

9 years ago
(In reply to comment #11)
> (From update of attachment 380352 [details] [diff] [review])
> It would be cool if you could find some existing places in the tree that should
> actually use these, so we're not just adding dead code. And I don't mean the
> screen patch I'm not keen on :-)

Yes, indeed. I took a side trip through the dehydra/treehydra documentation to see if I could quickly write a script to find them all. Perhaps I'll still end up doing it manually, but I figure this is a good way to learn dehydra/treehydra.
(Assignee)

Comment 13

9 years ago
Created attachment 382218 [details] [diff] [review]
495002-4.diff

oops. I suppose I would have a review already if I had remembered to attach this…
Attachment #380352 - Attachment is obsolete: true
Attachment #382218 - Flags: superreview?(roc)
Attachment #382218 - Flags: review?(bzbarsky)
Attachment #382218 - Flags: review?(bzbarsky) → review+
Comment on attachment 382218 [details] [diff] [review]
495002-4.diff

I'm not sure about the analysis scripts. I'd probably put them under layout/analysis instead of layout/base/analysis, but there should also be some documentation of what they do and how to use them.
Attachment #382218 - Flags: superreview?(roc) → superreview+
(Assignee)

Updated

9 years ago
Duplicate of this bug: 495192
(Assignee)

Comment 16

9 years ago
checked this in last night.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Blocks: 1083499
No longer blocks: 1083499
Depends on: 1083499
You need to log in before you can comment on or make changes to this bug.