Closed Bug 744901 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: Layout, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14
Tracking Status
blocking-fennec1.0 --- beta+

People

(Reporter: jrmuizel, Assigned: jrmuizel)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 742134
Attachment #614514 - Flags: review?(jones.chris.g)
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.
Attachment #614514 - Flags: review?(jones.chris.g)
(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.
Attachment #614514 - Flags: review?(jones.chris.g)
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.
Attachment #614514 - Flags: review?(jones.chris.g) → review+
blocking-fennec1.0: --- → ?
Assignee: nobody → jmuizelaar
Please do a complete risk assessment of all three pieces before landing, it seems like we could break a lot here.
blocking-fennec1.0: ? → beta+
This patch has very minimal risk, it adds some fields and uses them in one consumer in a place that was previously incorrect.
Attached patch typo's fixedSplinter Review
Attachment #614514 - Attachment is obsolete: true
Attachment #615998 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/e9053acaa5e1
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Depends on: 749953
Depends on: 753762
You need to log in before you can comment on or make changes to this bug.