744901 - Store the page size in FrameMetrics in CSS pixels in addition to device pixels.

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Some of the consumers (The Java frontend and RenderFrameParent) need the value in CSS pixels and using the device pixels gives slightly incorrect results because of rounding.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: Store the page size in FrameMetrics in CSS pixels in addition to device pixels

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 614514 [details] [diff] [review]
Store the page size in FrameMetrics in CSS pixels in addition to device pixels

What consumer wants CSS-pixel values, and how do they fix rounding errors?  This change on its own is fine, but I'm skeptical that it would legitimately fix anything.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #2)
> Comment on attachment 614514 [details] [diff] [review]
> Store the page size in FrameMetrics in CSS pixels in addition to device
> pixels
> 
> What consumer wants CSS-pixel values, and how do they fix rounding errors? 
> This change on its own is fine, but I'm skeptical that it would legitimately
> fix anything.

The consumer that this patch fixes is RenderFrameParent. It seems to currently assume that rounding doesn't happen.

The Java front end currently uses device pixel page sizes to calculate new device pixel page size which is obviously wrong. This patch is a step toward fixing that.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 614514 [details] [diff] [review]
Store the page size in FrameMetrics in CSS pixels in addition to device pixels

>diff --git a/gfx/layers/Layers.h b/gfx/layers/Layers.h

>+  // Consumer's often want to know the size before scaling to pixels
>+  // so we record this size as well.
>+  gfx::Size mCSSContentSize;
>+

I think this member is pretty self-explanatory, not sure this comment
is helpful.  (And it has a typo.)  Would drop it, up to you.

Comment 5

[JP Rosevear [:jpr]]

Please do a complete risk assessment of all three pieces before landing, it seems like we could break a lot here.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

This patch has very minimal risk, it adds some fields and uses them in one consumer in a place that was previously incorrect.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Attachment: typo's fixed

Comment 8

[Joe Drew (not getting mail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e9053acaa5e1 with the typos actually fixe'd.

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e9053acaa5e1