Closed Bug 1328337 Opened 3 years ago Closed 3 years ago

NativeFontResource::CreateScaledFont implicitly truncates Float glyph size to uint32_t

Categories

(Core :: Printing: Output, defect, P1)

51 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: lsalzman, Assigned: lsalzman)

References

Details

Attachments

(1 file)

Even though glyph size is passed through almost the entire printing pipeline as float, and even though RecordedScaledFontCreation records mGlyphSize as a Float, NativeFontResource::CreateScaledFont for some reason takes it as a uint32_t parameter, which causes it to implicitly truncate. This shouldn't happen.
Attachment #8823366 - Flags: review?(bobowencode)
Comment on attachment 8823366 [details] [diff] [review]
fix implicit conversion of glyph size from float to uint32_t in NativeFontResource::CreateScaledFont

Review of attachment 8823366 [details] [diff] [review]:
-----------------------------------------------------------------

I have no idea how I managed to do that, sorry.
Attachment #8823366 - Flags: review?(bobowencode) → review+
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aea479acaeb8
fix implicit conversion of glyph size from float to uint32_t in NativeFontResource::CreateScaledFont. r=bobowen
https://hg.mozilla.org/mozilla-central/rev/aea479acaeb8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Does this need uplifting?
Flags: needinfo?(lsalzman)
(In reply to Bob Owen (:bobowen) from comment #4)
> Does this need uplifting?

I think we should be okay given that sandboxing is still only just riding the trains?
Flags: needinfo?(lsalzman)
(In reply to Lee Salzman [:lsalzman] from comment #5)
> (In reply to Bob Owen (:bobowen) from comment #4)
> > Does this need uplifting?
> 
> I think we should be okay given that sandboxing is still only just riding
> the trains?

On Windows an initial sandbox is on release, so this code is being used now.
Flags: needinfo?(lsalzman)
Comment on attachment 8823366 [details] [diff] [review]
fix implicit conversion of glyph size from float to uint32_t in NativeFontResource::CreateScaledFont

Approval Request Comment
[Feature/Bug causing the regression]: bug 1156742
[User impact if declined]: Fonts may print with slightly wrong font size when using E10s.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no 
[List of other uplifts needed for the feature/fix]: aurora, beta
[Is the change risky?]: no
[Why is the change risky/not risky?]: Original behavior of truncating font sizes should never have been there in the first place, and this just gets rid of that.
[String changes made/needed]: None
Flags: needinfo?(lsalzman)
Attachment #8823366 - Flags: approval-mozilla-beta?
Attachment #8823366 - Flags: approval-mozilla-aurora?
Comment on attachment 8823366 [details] [diff] [review]
fix implicit conversion of glyph size from float to uint32_t in NativeFontResource::CreateScaledFont

avoid implicit float to int conversion, aurora52+

The first RC for 51 is already built, so it might be too late for that...
Attachment #8823366 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8823366 [details] [diff] [review]
fix implicit conversion of glyph size from float to uint32_t in NativeFontResource::CreateScaledFont

Too late for beta unless it's an emergency, but we can take this in 52.
Attachment #8823366 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in before you can comment on or make changes to this bug.