Label nsExiprationTracker subclass gfxFontCache

RESOLVED FIXED in Firefox 55

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bevis, Assigned: vliu)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [QDL][TDC-MVP][GFX][gfx-noted])

Attachments

(1 attachment, 1 obsolete attachment)

Follow up of bug 1341537 comment 1.

nsExpirationTracker::Notify() implicitly called by an internal timer:
which implicitly apply a timer internally in nsExpirationTracker::CheckStartTimer:
http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/xpcom/ds/nsExpirationTracker.h#122

Updated

2 years ago
Whiteboard: [QDL][TDC-MVP][GFX]
Whiteboard: [QDL][TDC-MVP][GFX] → [QDL][TDC-MVP][GFX][gfx-noted]
(Assignee)

Updated

2 years ago
Assignee: nobody → vliu
(Assignee)

Comment 1

2 years ago
There were two reasons we can label gfxFontCache as System group.

1. gfxFontCache was constructed from gfxPlatform::Init(), so it was Singleton per Content process.
2. The NotifyExpired callback function didn't touch any web content or any JS operations.
(Assignee)

Comment 2

2 years ago
For the WordCacheExpirationTimerCallback callback function in [1], it also didn't touch web content. Based on this, we can also label it as system group.

[1]: http://searchfox.org/mozilla-central/source/gfx/thebes/gfxFont.cpp#183
(Assignee)

Updated

2 years ago
Priority: -- → P1
(Assignee)

Comment 3

2 years ago
Created attachment 8856861 [details] [diff] [review]
0001-Bug-1350677-Add-Labeling-for-gfxFontCache.-r-jfktham.patch

Hi jfkthame,
Based on Comment 1 and Comment 2, could you please have a review for me? Really thanks
Attachment #8856861 - Flags: review?(jfkthame)
Comment on attachment 8856861 [details] [diff] [review]
0001-Bug-1350677-Add-Labeling-for-gfxFontCache.-r-jfktham.patch

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

Yes, as I understand it SystemGroup should be fine here.
Attachment #8856861 - Flags: review?(jfkthame) → review+
(Assignee)

Comment 5

2 years ago
Created attachment 8858694 [details] [diff] [review]
0001-Bug-1350677-Add-Labeling-for-gfxFontCache.-r-jfktham.patch

There were some bad implicit conversion constructor for 'gfxFontCache' in some platforms. It seems that I missed explicit declaration for 'gfxFontCache'. Since the change is small, I would attached the modified patch and carried r+ from Comment 4. Here is the try result in [1].

[1]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2bc98bebf24354c2795e27fd4582a099d6dd4be2
Attachment #8856861 - Attachment is obsolete: true

Comment 6

2 years ago
Pushed by vliu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a0ee65d3bf8
Add Labeling for gfxFontCache. r=jfkthame
https://hg.mozilla.org/mozilla-central/rev/7a0ee65d3bf8
Status: NEW → RESOLVED
Last Resolved: 2 years 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.