Simplify the default font size calculation for the spellchecker underline

RESOLVED FIXED in Firefox 55

Status

()

Core
Layout
P1
normal
RESOLVED FIXED
17 days ago
15 days ago

People

(Reporter: evelyn, Assigned: evelyn)

Tracking

(Blocks: 2 bugs, {perf})

50 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

17 days ago
+++ This bug was initially created as a clone of Bug #1354641 +++

From profile [1] and code [2] it shows we spend some time to calculate the thickness of the spellchecker underline when every time selectedframes is called. We might be able to calculate this value directly from PresContext instead of creating a temporary nsStyleFont object to do a bunch of expensive work.

[1] https://perfht.ml/2p4IE6S
[2] http://searchfox.org/mozilla-central/source/layout/generic/nsTextFrame.cpp#5798-5805
(Assignee)

Updated

17 days ago
Whiteboard: [e10s-multi:-][qf:p1] → [qf]
(Assignee)

Updated

17 days ago
Blocks: 1355600
(Assignee)

Updated

17 days ago
No longer blocks: 1355600
Comment hidden (mozreview-request)
(Assignee)

Comment 2

17 days ago
David, I saw you reviewed bug 486735 so set review request to you. Feel free to point to someone else if you are busy. Thanks!
(Assignee)

Comment 3

17 days ago
profile after applying this patch: https://perfht.ml/2opWylH.
(Assignee)

Comment 4

17 days ago
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=20f035e15d31bf2773f331c1cb6bfe48d3138fce

Comment 5

17 days ago
mozreview-review
Comment on attachment 8857223 [details]
Bug 1355595 - Simplify the default font size calculation;

https://reviewboard.mozilla.org/r/129160/#review131682

r=dbaron with these three comments:

::: layout/generic/nsTextFrame.cpp:5805
(Diff revision 1)
> -      gfxFloat fontSize = std::min(gfxFloat(defaultFontSize),
> +      int32_t scaledFontSize = aPresContext->AppUnitsToDevPixels(
> +          defaultFontSize * aPresContext->EffectiveTextZoom());

I think it's clearer to continue using the nsStyleFont::ZoomText function here, i.e., by replacing the manual multiplication of "defaultFontSize * aPresContext->EffectiveTextZoom()" with nsStyleFont::ZoomText(aPresContext, defaultFontSize)".  (I think the rounding is different, maybe, depending on what NSToCoordTruncClamped does.  It's also clearer.)

I'd also call the resulting variable zoomedFontSize rather than scaledFontSize, since scaled could mean so many other things.

::: layout/generic/nsTextFrame.cpp:5807
(Diff revision 1)
> +      gfxFloat fontSize = std::min(gfxFloat(scaledFontSize),
>                                   aFontMetrics.emHeight);

While you're here, please line up the indentation of the two arguments to std::min(), or just fit them one one line if they fit within 80 characters.  They were probably misaligned after some previous mass-edit.
Attachment #8857223 - Flags: review?(dbaron) → review+
Comment hidden (mozreview-request)

Comment 7

16 days ago
Pushed by ehung@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/abfece312d74
Simplify the default font size calculation; r=dbaron
(Assignee)

Comment 8

16 days ago
Thanks for the review! David.
commend addressed and trigger auto-land on mozreview.
(Assignee)

Updated

15 days ago
Assignee: nobody → ehung
Status: NEW → ASSIGNED

Comment 9

15 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/abfece312d74
Status: ASSIGNED → RESOLVED
Last Resolved: 15 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.