495002 - nsPresContext ought to have CSSPixelsToDevPixels and DevPixelsToCSSPixels methods

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Brooks [:db48x]]

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?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Daniel, unless roc objects, want to just post a patch?

Comment 2

[Daniel Brooks [:db48x]]

Sure, but after dinner.

Comment 3

[Daniel Brooks [:db48x]]

Attachment: 495002-1.diff

This just adds the functions; I haven't gone looking for any of the places that could call them.

Comment 4

[Boris Zbarsky [:bzbarsky]]

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?

Comment 5

[Daniel Brooks [:db48x]]

Attachment: 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.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 380132 [details] [diff] [review]
495002-2.diff

Looks good; roc should sr, though

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 8

[Daniel Brooks [:db48x]]

(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.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

But if the input is a float, surely the output should be a float? The caller should round the result as they see fit

Comment 10

[Daniel Brooks [:db48x]]

Attachment: 495002-3.diff

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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 :-)

Comment 12

[Daniel Brooks [:db48x]]

(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.

Comment 13

[Daniel Brooks [:db48x]]

Attachment: 495002-4.diff

oops. I suppose I would have a review already if I had remembered to attach this…

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 16

[Daniel Brooks [:db48x]]

checked this in last night.