Details
- Reviewers
lsalzman emilio - Group Reviewers
gfx-reviewers - Commits
- rMOZILLACENTRALa7b4be81c1ae: Bug 1832955 - Support font-relative units when parsing…
- Bugzilla Bug ID
- 1832955
Diff Detail
- Repository
- rMOZILLACENTRAL mozilla-central
- Build Status
Buildable 565210 Build 662831: arc lint + arc unit
Event Timeline
This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of testing-approved, testing-exception-unchanged, testing-exception-ui, testing-exception-elsewhere, testing-exception-other. Tip: this Firefox add-on makes it easy!
The approach looks fine, but I have some nits.
| dom/canvas/CanvasRenderingContext2D.cpp | ||
|---|---|---|
| 2746 | Probably worth returning something saner here. 16px font-size etc. But I guess this is fine too. There's some overlap here between this and https://phabricator.services.mozilla.com/D180618 | |
| layout/style/GeckoBindings.h | ||
| 512 | Maybe mComputedEmSize or so, to as not be confused with nsFontMetrics::EmHeight etc? | |
| servo/components/style/values/specified/length.rs | ||
| 165 | It'd be more idiomatic to use get_metrics: impl Fn() -> GeckoFontMetrics, so as to keep this all safe, and keep the unsafe bits inside glue.rs | |
| 168 | nit: no need for return | |
| 169 | nit: Self::Em etc might be better than FontRelativeLength::Em | |
| 1117 | Let's format this properly: $ rustup run nightly rustfmt servo/components/style/values/specified/length.rs | |
| testing/web-platform/meta/html/canvas/offscreen/text/2d.text.drawing.style.letterSpacing.change.font.html.ini | ||
| 4 | Why only on Linux? | |
| dom/canvas/CanvasRenderingContext2D.cpp | ||
|---|---|---|
| 2746 | I think this really shouldn't happen (if we fail to create a fontgroup, we're probably about to OOM-crash or something!), but if we don't want to just return zeros, we should probably do something based on the canvas default font size (10px) rather than HTML's 16px. | |
| testing/web-platform/meta/html/canvas/offscreen/text/2d.text.drawing.style.letterSpacing.change.font.html.ini | ||
| 4 | On the other platforms we get a minuscule discrepancy due to floating-point calculations; we should just make the test do an approx_equals, like other measurement tests, but I didn't do it here because I've noticed there's a big reorg of these tests in progress from Chrome's side, likely to appear in the next wpt sync. So to avoid causing a merge conflict there, I figured I'd wait till that dust settles (then I want to add some more testcases as well). | |
| servo/ports/geckolib/glue.rs | ||
|---|---|---|
| 7816 | We never expect a null get_font_metrics function here right? same for the Option<> around in to_computed_pixel_length_with_font_metrics... Maybe consider removing the Option<>s everywhere? Looks good either way. | |
| servo/ports/geckolib/glue.rs | ||
|---|---|---|
| 7816 | I was in two minds about this (still am, I guess).... yes, as it stands we'll never get a null function; the only caller is the canvas2d code being touched here and it passes a getter. So we could simply require it. But letting the getter be optional means that the method can still provide the same functionality as previously, of parsing only absolute lengths, which seems like a possibly-useful option. (Perhaps we'll never have a need for that again, it was basically just a stopgap here?) I think I'll leave it as optional for now, so anyone just wanting to parse a css length from random style-context-less code can do so without having to think about font metrics. | |