Closed Bug 495002 Opened 11 years ago Closed 11 years ago

nsPresContext ought to have CSSPixelsToDevPixels and DevPixelsToCSSPixels methods

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: db48x, Assigned: db48x)

References

Details

Attachments

(1 file, 3 obsolete files)

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?
Sure, but after dinner.
Blocks: 495192
Attached patch 495002-1.diff (obsolete) — Splinter Review
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)
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-
Attached patch 495002-2.diff (obsolete) — Splinter Review
(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-
(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
Attached patch 495002-3.diff (obsolete) — Splinter Review
Attachment #380132 - Attachment is obsolete: true
Attachment #380352 - Flags: superreview?
Attachment #380352 - Flags: review?(bzbarsky)
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+
(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.
Attached patch 495002-4.diffSplinter Review
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+
Duplicate of this bug: 495192
checked this in last night.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
No longer blocks: 1083499
Depends on: 1083499
You need to log in before you can comment on or make changes to this bug.