Closed
Bug 964613
Opened 10 years ago
Closed 10 years ago
need to check availability of local fonts when platform fontlist is rebuilt
Categories
(Core :: Graphics: Text, defect)
Core
Graphics: Text
Tracking
()
RESOLVED
FIXED
mozilla31
People
(Reporter: jtd, Assigned: jtd)
References
(Depends on 1 open bug, )
Details
Attachments
(1 file, 5 obsolete files)
17.31 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
When the platform font list is rebuilt (i.e. when a font is added/deleted/enabled/disabled), the userfont code does not explicitly confirm that local faces (i.e. @font-face rules that use src: local(xxx)) are available or not. Steps: 1. Under OSX, open up the testpage on trunk 2. Page displays two textarea elements using Georgia Bold Italic 3. Open Font Book and explicitly disable Georgia Bold Italic. Result: the bottom textarea is refreshed and uses a different font. The top textarea, which uses an @font-face rule, does not update. If you type in the textarea it appears in the disabled font. Expected result: the top textarea displays in sans serif when Georgia Bold Italic is disabled. The testcase is based on Georgia but can easily be modified to use a different font. For example, to test this on Windows, add a new font to the system fonts folder and change the testcase to use that font. After loading the modified testpage, delete the font.
Assignee | ||
Comment 1•10 years ago
|
||
While the bug here is relatively minor and unlikely to occur much in practice, the larger problem is that userfont sets aren't responsive to changes in the underlying system fontlist. This means that to replacing synchronous font loading methods (e.g. the loading of font facenames under Windows) with load-later-then-reflow-if-needed techniques won't correctly display the contents if a local font is used and not found initially.
Blocks: 752394
Assignee | ||
Comment 2•10 years ago
|
||
This patch cleans out src local fonts after platform fontlist changes by registering each userfont set with the platform fontlist object. When local fonts are looked up, each userfont set tracks the local names that were hit/missed. When an update to the platform fontlist occurs, userfont sets check local fonts to make sure all fonts in use are still available and that all missing fonts are still missing. If any change occurs in these fonts, the userfont set is marked dirty and rebuilt. As part of the rebuild process, any @font-face rule that uses local() within the src descriptor will be rebuilt from scratch.
Assignee | ||
Comment 3•10 years ago
|
||
One thing to note here is that there's currently an inefficiency in the way the platform fontlist is updated on OSX, the entire list is rebuilt *twice* when a font is enabled on OSX. See bug 968058 for more info.
Assignee | ||
Comment 4•10 years ago
|
||
Un-bit-rotted. When the platform font list is reinitialized due to a change in available local fonts, check through the userfont sets to see if any have src:local fonts that would be affected by the change in the list of platform fonts. This could either be a font being disabled or a font being enabled (see gfxUserFontSet::CheckForDirtyLocalRules).
Attachment #8370591 -
Attachment is obsolete: true
Attachment #8402497 -
Flags: review?(jfkthame)
Assignee | ||
Comment 5•10 years ago
|
||
Rebased to latest trunk code and added work around for crashing problem on Linux.
Attachment #8402497 -
Attachment is obsolete: true
Attachment #8402497 -
Flags: review?(jfkthame)
Attachment #8404446 -
Flags: review?(jfkthame)
Comment 6•10 years ago
|
||
Comment on attachment 8404446 [details] [diff] [review] patch, check userfont set local fonts when platform fontlist change occurs Review of attachment 8404446 [details] [diff] [review]: ----------------------------------------------------------------- ISTM that it's overkill to keep track of local-name hits and misses, and enumerate them looking for changes when the platform font list is updated. Updates to the PFL while the browser is running are a rare occurrence, and shouldn't be perf-critical; why not just take the simple approach here, and treat all local rules as dirty whenever there's a platform font update. Less code == more goodness. :)
Assignee | ||
Comment 7•10 years ago
|
||
This version doesn't track missed/hit names, it simply flags whether local lookups have occurred or not with a given userfont set. When platform fontlist changes occur, userfont sets with this flag set will be rebuilt and any rules containing src local rules will be reconstructed.
Attachment #8404446 -
Attachment is obsolete: true
Attachment #8404446 -
Flags: review?(jfkthame)
Attachment #8407342 -
Flags: review?(jfkthame)
Comment 8•10 years ago
|
||
Comment on attachment 8407342 [details] [diff] [review] patch v2, rebuild userfont set local fonts when platform fontlist change occurs Review of attachment 8407342 [details] [diff] [review]: ----------------------------------------------------------------- I'm much happier with this simpler version, thanks. I think the code can be cleaned up a bit further at this point - there are some now-trivial methods that add no value, IMO. Also, I'd prefer to change the naming to avoid "dirty" here, as I don't think it is a good reflection of what we're doing. See suggestions below. ::: gfx/thebes/gfxMacPlatformFontList.mm @@ +833,5 @@ > // xxx - should be carefully pruning the list of fonts, not rebuilding it from scratch > static_cast<gfxMacPlatformFontList*>(observer)->UpdateFontList(); > > // modify a preference that will trigger reflow everywhere > + static_cast<gfxMacPlatformFontList*>(observer)->ForceGlobalReflow(); The ForceGlobalReflow() helper is OK (and I realize you're intending to re-use it later), but please factor out the cast of the observer to font-list here so that it's only done once. Or how about calling ForceGlobalReflow() at the end of UpdateFontList() instead? ::: gfx/thebes/gfxPlatformFontList.cpp @@ +757,5 @@ > + > +void > +gfxPlatformFontList::RebuildUserFontSetLocalFonts() > +{ > + mUserFontSetList.EnumerateEntries(RebuildLocalFonts, nullptr); Just do this directly in UpdateFontList(); no need for a separate one-line method. ::: gfx/thebes/gfxUserFontSet.cpp @@ +544,5 @@ > > // src local ==> lookup and load immediately > > if (currSrc.mIsLocal) { > + gfxFontEntry *fe = LookupLocalFont(aProxyEntry, currSrc.mLocalName); Do "mLocalRulesUsed = true;" here, no need for an extra method. ::: gfx/thebes/gfxUserFontSet.h @@ +242,5 @@ > // increment the generation on font load > void IncrementGeneration(); > > + // rebuild if local rules have been used > + void RebuildDirtyLocalRules(); I'm uncomfortable with the "dirty" terminology here, in general. I don't think we're really tracking "dirtiness" of these rules (which would suggest the rule has changed since it was looked up, or something like that); we're just interested in whether they've been used. Suggest changing this name to simply RebuildLocalRules(). @@ +244,5 @@ > > + // rebuild if local rules have been used > + void RebuildDirtyLocalRules(); > + > + bool LocalRulesDirty() { return mDirtyLocalRules; } This accessor isn't needed, the flag is purely internal. @@ +423,5 @@ > > static bool OTSMessage(void *aUserData, const char *format, ...); > > + // helper method to call through to underlying platform lookup method > + // while keeping track of name hits/misses Update comment; we don't keep track of hits/misses, only the fact that src:local has been used. Hmm, on second thought, there's no need for this wrapper method at all any more. Just set the flag directly in the one place where we call out to the platform's LookupLocalFont. @@ +436,5 @@ > > uint64_t mGeneration; > > + // true when local names have been looked up, false otherwise > + bool mDirtyLocalRules; s/mDirtyLocalRules/mLocalRulesUsed/ ::: layout/style/nsFontFaceLoader.cpp @@ +490,5 @@ > IncrementGeneration(); > } > > + // dirty local rules have been rebuilt, so clear the flag > + mDirtyLocalRules = false; Losing 'dirty' from the terminology: // local rules have been rebuilt; the new ones have not been used yet mLocalRulesUsed = false; @@ +505,5 @@ > + size_t numSrc = aSrcArr->Count(); > + for (size_t i = 0; i < numSrc; i++) { > + val = aSrcArr->Item(i); > + unit = val.GetUnit(); > + if (unit == eCSSUnit_Local_Font) { Neither the 'val' nor 'unit' local variables are needed here, IMO; simply write the accessors directly in the if-statement and it's perfectly readable. if (aSrcArr->Item(i).GetUnit() == ...etc ::: layout/style/nsFontFaceLoader.h @@ +88,5 @@ > virtual bool GetPrivateBrowsing() MOZ_OVERRIDE; > > + virtual void DoRebuildUserFontSet() MOZ_OVERRIDE; > + > + not so many blank lines, please
Assignee | ||
Comment 9•10 years ago
|
||
Updated based on review comments.
Attachment #8407342 -
Attachment is obsolete: true
Attachment #8407342 -
Flags: review?(jfkthame)
Attachment #8407944 -
Flags: review?(jfkthame)
Comment 10•10 years ago
|
||
Comment on attachment 8407944 [details] [diff] [review] patch v3, rebuild userfont set local fonts when platform fontlist change occurs Review of attachment 8407944 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. A couple small nits, and a couple of questions that aren't immediately clear to me... ::: gfx/thebes/gfxUserFontSet.h @@ +423,5 @@ > > + // helper method to call through to underlying platform lookup method > + // while keeping track of name hits/misses > + gfxFontEntry* LookupLocalFont(const gfxProxyFontEntry *aProxyEntry, > + const nsAString& aFontName); this no longer exists; remove the declaration @@ +451,5 @@ > class gfxProxyFontEntry : public gfxFontEntry { > friend class gfxUserFontSet; > > public: > + gfxProxyFontEntry() {} why is this needed? ::: layout/style/nsFontFaceLoader.cpp @@ +501,5 @@ > +{ > + size_t numSrc = aSrcArr->Count(); > + for (size_t i = 0; i < numSrc; i++) { > + if (aSrcArr->Item(i).GetUnit() == eCSSUnit_Local_Font) { > + return true; excessive indent @@ +1016,5 @@ > +{ > + nsIPresShell* ps = mPresContext->PresShell(); > + if (!ps) { > + return; > + } why do we need to check PresShell() here? offhand, this looks redundant
Assignee | ||
Comment 11•10 years ago
|
||
Updated based on review comments.
Attachment #8407944 -
Attachment is obsolete: true
Attachment #8407944 -
Flags: review?(jfkthame)
Attachment #8408052 -
Flags: review?(jfkthame)
Updated•10 years ago
|
Attachment #8408052 -
Flags: review?(jfkthame) → review+
Assignee | ||
Comment 12•10 years ago
|
||
Pushed to inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/ff84ce7e5f85
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ff84ce7e5f85
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•