Closed
Bug 1265648
Opened 8 years ago
Closed 8 years ago
Remove the (redundant) nsTextFrameTextRunCache.
Categories
(Core :: Layout: Text and Fonts, defect)
Core
Layout: Text and Fonts
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: jfkthame, Assigned: jfkthame)
Details
Attachments
(2 files, 1 obsolete file)
2.82 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
18.22 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
As far as I can see, this is a vestige of the long-dead word-caching scheme we used to use, and no longer serves any purpose. nsTextFrame code still puts things into this cache, but nobody ever looks anything up there, so it's just a waste of time and space. So I propose to kill it.
Assignee | ||
Comment 1•8 years ago
|
||
Attachment #8742697 -
Flags: review?(mats)
Assignee | ||
Comment 2•8 years ago
|
||
With gTextRuns out of the picture, it's easier to see that switching mTextRunsToDelete to be an array of UniquePtr<>s, as Jeff suggested in bug 1265459 comment 5, is a reasonable thing to do.
Attachment #8742701 -
Flags: review?(mats)
Assignee | ||
Comment 3•8 years ago
|
||
(In reply to Jonathan Kew (:jfkthame) from comment #0) > As far as I can see, this is a vestige of the long-dead word-caching scheme > we used to use, and no longer serves any purpose. nsTextFrame code still > puts things into this cache, but nobody ever looks anything up there, so > it's just a waste of time and space. So I propose to kill it. Hmm, on looking again, I'm not entirely sure this is true; we may still depend on the cache's expiration behavior to delete uninflated runs stashed in the textframe's UninflatedTextRunProperty(). Clearing r? for now, until I'm more confident what's going on there.
Assignee | ||
Updated•8 years ago
|
Attachment #8742697 -
Flags: review?(mats)
Assignee | ||
Updated•8 years ago
|
Attachment #8742701 -
Flags: review?(mats)
I *think* we're not shipping font inflation anywhere anymore, so it may be safe to start letting it bitrot.
Assignee | ||
Comment 5•8 years ago
|
||
We still have font-inflation reftests in the tree, AFAICS; and as far as tryserver can tell, they don't seem to be leaking textruns with the patches here,[1] so I think my concern in comment 3 may have been unfounded. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=de46a1a7a32f
Assignee | ||
Comment 6•8 years ago
|
||
OK, this doesn't lead to us leaking anything; it's simply old code that we no longer need, as we don't attempt to cache shaped words in terms of ranges of other textruns. We can also remove the related gtest file (which was disabled 'on suspicion' some time ago anyhow, and then forgotten about AFAICT).
Assignee | ||
Updated•8 years ago
|
Attachment #8742697 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8742997 -
Flags: review?(mats)
Assignee | ||
Updated•8 years ago
|
Attachment #8742701 -
Flags: review?(mats)
Comment 7•8 years ago
|
||
Comment on attachment 8742997 [details] [diff] [review] Remove the global nsTextFrameTextRunCache, as it no longer serves any useful purpose Nice!
Attachment #8742997 -
Flags: review?(mats) → review+
Updated•8 years ago
|
Attachment #8742701 -
Flags: review?(mats) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/77ba0dcb977a https://hg.mozilla.org/integration/mozilla-inbound/rev/20956a16ffb8
Comment 9•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/77ba0dcb977a https://hg.mozilla.org/mozilla-central/rev/20956a16ffb8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•