Closed
Bug 457821
Opened 16 years ago
Closed 16 years ago
need to rebuild user font set objects when style sheets are disabled/enabled or rules are modified/added/removed
Categories
(Core :: CSS Parsing and Computation, defect, P3)
Core
CSS Parsing and Computation
Tracking
()
VERIFIED
FIXED
mozilla1.9.1b3
People
(Reporter: jtd, Assigned: dbaron)
References
(Depends on 1 open bug)
Details
(Keywords: verified1.9.1)
Attachments
(10 files, 17 obsolete files)
26.28 KB,
patch
|
jtd
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
2.15 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
7.13 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
1.14 KB,
patch
|
jtd
:
review+
|
Details | Diff | Splinter Review |
70.27 KB,
patch
|
Details | Diff | Splinter Review | |
31.48 KB,
patch
|
Details | Diff | Splinter Review | |
13.90 KB,
patch
|
Details | Diff | Splinter Review | |
6.62 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
1.89 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
13.77 KB,
patch
|
jtd
:
review+
|
Details | Diff | Splinter Review |
To support downloadable fonts, a user font set object is created when @font-face rules are found in style content (see bug 441473). For each @font-face rule that is processed, a font entry is added to the user font set object. However, when a stylesheet is disabled, the @font-face rules for the disabled stylesheet need to also be disabled.
Discussing this with dbaron, we decided that it made sense to simply rebuild the user font set object when stylesheets containing @font-face rules are enabled or disabled (see bug 441473 comment 77).
Flags: blocking1.9.1?
Assignee | ||
Comment 1•16 years ago
|
||
Updating summary to reflect that we also need to do this when rules in sheets are modified, added, or removed. (That requires hooking into a different code path than enabling/disabling entire sheets, so it's worth noting.)
Component: GFX: Thebes → Style System (CSS)
QA Contact: thebes → style-system
Summary: need to rebuilt user font set objects when stylesheets are disabled or enabled → need to rebuild user font set objects when style sheets are disabled/enabled or rules are modified/added/removed
Reporter | ||
Updated•16 years ago
|
Priority: -- → P1
Flags: blocking1.9.1? → blocking1.9.1+
Priority: P1 → P3
Assignee | ||
Comment 2•16 years ago
|
||
So hooking in to nsCSSRuleProcessor::ClearRuleCascades or perhaps just nsCSSStyleSheet::ClearRuleCascades will catch the enabled/disabled cases and the rule insertion/removal cases -- that's probably the best place to clear the old user font set. You also need to hook into nsCSSRuleProcessor::RefreshRuleCascade to catch media-dependent style sheets.
Assignee | ||
Comment 3•16 years ago
|
||
Taking per email discussion with jdaggett.
Assignee: jdaggett → dbaron
Assignee | ||
Comment 4•16 years ago
|
||
Here's some work in progress; the marking-dirty stuff actually looks harder than I thought it would be; I think I might need to ensure that all of these things happen inside BeginUpdate/EndUpdate (UPDATE_STYLE) and then query all the rule processors for dirtyness inside the style set's EndUpdate.
Assignee | ||
Comment 5•16 years ago
|
||
How about a version that compiles and runs, at least?
Attachment #346342 -
Attachment is obsolete: true
Assignee | ||
Comment 6•16 years ago
|
||
I'm going to attach the current state of my thinking. I have a set of four patches that I think ought to work (who knows, maybe they're close), but I haven't done any testing. (For now, I'll need to switch to a different platform to do that testing.)
This is roughly the same as attachment 346347 [details] [diff] [review].
It builds the array of @font-face rules up from the levels of the cascade, since some of the time we can reprocess one level of the cascade but not others (due to various types of dynamic changes).
Since yesterday, I fixed things so we again don't have a user font set when there are no @font-face rules.
This moves InsertFontFaceRule from one file to another, which makes the patch look big. (I took out two unused variables while I was moving it.)
Attachment #346347 -
Attachment is obsolete: true
Assignee | ||
Comment 7•16 years ago
|
||
I finally came up with a simple way to do this, by observing that everything that goes through one of the two codepaths in comment 2 also goes through one of these two codepaths. The patch in comment 1 means we don't actually need any clearing at the points I mentioned in comment 2 as long as we do the rebuilding, since the patch in comment 1 means we're storing the data in the RuleCascadeData, which is already cleared or swapped out in those codepaths.
I need to verify my assertion that this should work more closely and probably add a bunch of comments pointing out that this is how things work.
Assignee | ||
Comment 8•16 years ago
|
||
This optimizes the rebuild with an array comparison. I think the main thing this saves is really not rebuilding the font set, but the ensuing style change reflow (see patch 4).
Assignee | ||
Comment 9•16 years ago
|
||
Patch 2 introduces the possibility of removing @font-face rules from the set of rules we're considering. This patch causes us to do a style change reflow in that case just like we do when font loads complete, and adds comments both similar cases pointing at the other.
Assignee | ||
Comment 10•16 years ago
|
||
Assignee | ||
Comment 11•16 years ago
|
||
I've made a bit of progress on tests, although I still need to do a lot more.
I'd like to test interaction of the dynamic cases with the ordering of rules since I think the changes I've made should have improved that, except that the static tests I've written for that don't work on the trunk.
I also need to test media changes as well.
I also need to figure out how to actually meet the security restrictions; right now running them from file: URLs hits the can't-cross-directory security restriction, I think.
I also need to make sure that the tests that should be failing on the trunk are, and that the ones that should be passing with my patch are, etc.
Assignee | ||
Comment 12•16 years ago
|
||
(In reply to comment #11)
> I'd like to test interaction of the dynamic cases with the ordering of rules
> since I think the changes I've made should have improved that, except that the
> static tests I've written for that don't work on the trunk.
Note that I might be confused about what we ought to be implementing; I'll send email about that tomorrow.
Assignee | ||
Comment 13•16 years ago
|
||
This uses relative URL resolution so that one of the URLs can be served over HTTP and the other can be about:blank or a data: URL.
Attachment #346740 -
Flags: review?
Assignee | ||
Updated•16 years ago
|
Attachment #346740 -
Flags: review? → review?(jwalden+bmo)
Assignee | ||
Comment 14•16 years ago
|
||
This allows "HTTP(..)", "HTTP(../..)", "HTTP(../../../)" for reftests so that we can relax the restriction that all the resources need to be in the same directory. This lets us use downloadable fonts in reftests from a resource directory containing fonts. (We can't use file: because of the cross-directory security restrictions on file:.)
Attachment #346741 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 15•16 years ago
|
||
More later, but here's a set that actually use downloadable fonts. They don't actually pass yet...
Attachment #346620 -
Attachment is obsolete: true
Updated•16 years ago
|
Attachment #346740 -
Flags: review?(jwalden+bmo) → review+
Comment 16•16 years ago
|
||
Comment on attachment 346740 [details] [diff] [review]
patch 5: allow about:blank as a reference with HTTP reftests
You might as well add a data: URL test with HTTP specified while you're at it, just for completeness.
Comment 17•16 years ago
|
||
Comment on attachment 346741 [details] [diff] [review]
patch 6: allow variants of HTTP so that resources can be served from parent directory
I wish reftest weren't line-based but rather whole-manifest-based, so we could just have one server rooted in one location and we wouldn't have this weird directory restriction. Probably too much effort to make the change worthwhile, alas...
Attachment #346741 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 18•16 years ago
|
||
I had to merge 2, 3, and 4. I don't think it's much of a loss.
I'm now forcing a reflow when the user font set changes (not just non-null to null), since otherwise we'll never hit the loader bits.
Attachment #346555 -
Attachment is obsolete: true
Attachment #346556 -
Attachment is obsolete: true
Attachment #346558 -
Attachment is obsolete: true
Assignee | ||
Comment 19•16 years ago
|
||
Remove an unused class that made things a little more confusing.
Attachment #346807 -
Flags: review?(jdaggett)
Assignee | ||
Comment 20•16 years ago
|
||
This prevents the font cache from returning a font metrics object that has the wrong user font set.
I had to add to nsIFontMetrics (though I could have added to nsIThebesFontMetrics, but I don't see why it matters and this is a little less ugly) because the font cache is in gkgfx, which doesn't link against thebes, so I can't call methods on a font group.
Attachment #346808 -
Flags: superreview?(roc)
Attachment #346808 -
Flags: review?(jdaggett)
Assignee | ||
Comment 21•16 years ago
|
||
There are still a whole bunch more tests I need to write, but first I need to figure out why some of these are failing for vertical positioning differences and then mark and work around the substantive failures.
Attachment #346742 -
Attachment is obsolete: true
Assignee | ||
Updated•16 years ago
|
Attachment #346553 -
Flags: superreview?(bzbarsky)
Attachment #346553 -
Flags: review?(jdaggett)
Assignee | ||
Updated•16 years ago
|
Attachment #346806 -
Flags: superreview?(bzbarsky)
Attachment #346806 -
Flags: review?(jdaggett)
Assignee | ||
Comment 22•16 years ago
|
||
This fixes the height differences I was seeing in reftests (resulting from bad metrics).
Attachment #346808 -
Attachment is obsolete: true
Attachment #346855 -
Flags: superreview?(roc)
Attachment #346855 -
Flags: review?(jdaggett)
Attachment #346808 -
Flags: superreview?(roc)
Attachment #346808 -
Flags: review?(jdaggett)
Assignee | ||
Comment 23•16 years ago
|
||
Added an up-front null-check of mShell in case the pres context has been destroyed by the time we get the event.
Attachment #346806 -
Attachment is obsolete: true
Attachment #346929 -
Flags: superreview?(bzbarsky)
Attachment #346929 -
Flags: review?(jdaggett)
Attachment #346806 -
Flags: superreview?(bzbarsky)
Attachment #346806 -
Flags: review?(jdaggett)
Comment 24•16 years ago
|
||
Comment on attachment 346553 [details] [diff] [review]
patch 1: build the list of @font-face rules from the correct pieces, guaranteeing the correct order
>+++ b/layout/base/nsPresContext.cpp
>+InsertFontFaceRule(nsCSSFontFaceRule *aRule, gfxUserFontSet* aFontSet)
NS_ABORT_IF_FALSE(NS_SUCCEEDED(aRule->GetType(type))
>+ && type == nsICSSRule::FONT_FACE_RULE,
>+ "InsertFontFaceRule passed a non-fontface CSS rule");
I'm not sure how this is expected to work.... GetType is a virtual function, and if we use the vtable for nsCSSFontFaceRule we'll always get back nsICSSRule::FONT_FACE_RULE, no? So what's this checking?
>+ if (unit == eCSSUnit_String) {
...
>+ } else {
>+ NS_ASSERTION(unit == eCSSUnit_String,
>+ "@font-face family name has non-string unit type");
I know you just moved this code, but this looks odd. Just make it an NS_ERROR if that's what we mean?
>+++ b/layout/style/nsCSSRuleProcessor.cpp
>+ * (2) Add any @font-face rules, in order, into XXX WRITE ME
Please do.
sr=bzbarsky with those addressed.
Attachment #346553 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Comment 25•16 years ago
|
||
(In reply to comment #24)
> (From update of attachment 346553 [details] [diff] [review])
> >+++ b/layout/base/nsPresContext.cpp
> >+InsertFontFaceRule(nsCSSFontFaceRule *aRule, gfxUserFontSet* aFontSet)
> NS_ABORT_IF_FALSE(NS_SUCCEEDED(aRule->GetType(type))
> >+ && type == nsICSSRule::FONT_FACE_RULE,
> >+ "InsertFontFaceRule passed a non-fontface CSS rule");
>
> I'm not sure how this is expected to work.... GetType is a virtual function,
> and if we use the vtable for nsCSSFontFaceRule we'll always get back
> nsICSSRule::FONT_FACE_RULE, no? So what's this checking?
No. It'll still call the appropriate virtual function despite the cast.
> >+ if (unit == eCSSUnit_String) {
> ...
> >+ } else {
> >+ NS_ASSERTION(unit == eCSSUnit_String,
> >+ "@font-face family name has non-string unit type");
>
> I know you just moved this code, but this looks odd. Just make it an NS_ERROR
> if that's what we mean?
I think this is already fixed in bug 460217, so I'd rather just let both patches land and whoever lands second gets to merge.
> >+++ b/layout/style/nsCSSRuleProcessor.cpp
> >+ * (2) Add any @font-face rules, in order, into XXX WRITE ME
>
> Please do.
Er, right.
Comment 26•16 years ago
|
||
Comment on attachment 346929 [details] [diff] [review]
patch 2-3-4: do the necessary rebuilding when needed
Shouldn't GetUserFontSet still do a (lazy) rebuild?
Assignee | ||
Comment 27•16 years ago
|
||
(In reply to comment #26)
> Shouldn't GetUserFontSet still do a (lazy) rebuild?
I suppose it could (that was my original approach), but any case where it matters, we're not even going to call GetUserFontSet unless we force a style change reflow. So if we're not forcing that style change reflow, gfx will never decide that it wants to load the fonts (and thus force another style change reflow) and layout will never rebuild text runs to get to a situation where it has different font groups. So I'm not sure I see the point given the rest of the changes.
Comment 28•16 years ago
|
||
Comment on attachment 346929 [details] [diff] [review]
patch 2-3-4: do the necessary rebuilding when needed
OK, then nix the comments that talk about that lazy rebuild?
Attachment #346929 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Comment 29•16 years ago
|
||
I'm probably going to land these, with the ones for this bug commented out temporarily, sometime this weekend.
Feel free to have a look through them, but I'm not going to ask that of anyone...
Attachment #346831 -
Attachment is obsolete: true
Assignee | ||
Comment 30•16 years ago
|
||
Added a few additional tests.
Attachment #347005 -
Attachment is obsolete: true
Assignee | ||
Comment 31•16 years ago
|
||
I landed:
http://hg.mozilla.org/mozilla-central/rev/25ea3e7408ba (patch 5)
http://hg.mozilla.org/mozilla-central/rev/04a589e040b9 (patch 6)
http://hg.mozilla.org/mozilla-central/rev/70826688e822 (patch 9, part a)
http://hg.mozilla.org/mozilla-central/rev/229683caa4c3 (patch 9, part b)
This patch contains the reftests that I haven't landed yet, which are those that deal with dynamic cases (i.e., are specifically for this bug, rather than testing base cases that I wanted to test to ensure that these tests are sane).
Reporter | ||
Comment 32•16 years ago
|
||
Comment on attachment 346807 [details] [diff] [review]
patch 7: remove nsGfxFontLoaderContext
My bad, missed trimming this out.
Attachment #346807 -
Flags: review?(jdaggett) → review+
Reporter | ||
Comment 33•16 years ago
|
||
(In reply to comment #20)
> Created an attachment (id=346808) [details]
> patch 8: make the font cache check for a user font set match
>
> This prevents the font cache from returning a font metrics object that has the
> wrong user font set.
> nsIFontMetrics* fm;
> PRInt32 n = mFontMetrics.Count() - 1;
> for (PRInt32 i = n; i >= 0; --i) {
> fm = static_cast<nsIFontMetrics*>(mFontMetrics[i]);
> - if (fm->Font().Equals(aFont)) {
> + nsIThebesFontMetrics* tfm = static_cast<nsIThebesFontMetrics*>(fm);
> + if (fm->Font().Equals(aFont) && tfm->GetUserFontSet() == aUserFontSet) {
> nsCOMPtr<nsIAtom> langGroup;
> fm->GetLangGroup(getter_AddRefs(langGroup));
> if (aLangGroup == langGroup.get()) {
> if (i != n) {
> // promote it to the end of the cache
> mFontMetrics.MoveElement(i, n);
> }
> + tfm->GetThebesFontGroup()->UpdateFontList();
> NS_ADDREF(aMetrics = fm);
> return NS_OK;
> }
> }
> }
I'm not sure I quite understand this. Why the need to call UpdateFontList? This will happen automatically whenever a text run is recreated, the point at which font matching is performed.
Also, isn't there a chance that after a user font set has been torn down and recreated that you could end up with an identical pointer?
Reporter | ||
Comment 34•16 years ago
|
||
Comment on attachment 346553 [details] [diff] [review]
patch 1: build the list of @font-face rules from the correct pieces, guaranteeing the correct order
Just one small nit:
+ * (2) Add any @font-face rules, in order, into XXX WRITE ME
Into the ???
Attachment #346553 -
Flags: review?(jdaggett) → review+
Reporter | ||
Updated•16 years ago
|
Attachment #346929 -
Flags: review?(jdaggett) → review+
Assignee | ||
Comment 35•16 years ago
|
||
(In reply to comment #34)
> (From update of attachment 346553 [details] [diff] [review])
> Just one small nit:
>
> + * (2) Add any @font-face rules, in order, into XXX WRITE ME
>
> Into the ???
(2) add any @font-face rules, in order, into data->mFontFaceRules.
Assignee | ||
Comment 36•16 years ago
|
||
(In reply to comment #33)
> I'm not sure I quite understand this. Why the need to call UpdateFontList?
> This will happen automatically whenever a text run is recreated, the point at
> which font matching is performed.
Failing to call UpdateFontList was resulting in incorrect metrics being returned in the function GetNormalLineHeight in nsHTMLReflowState.cpp. This function calls aFontMetrics->GetExternalLeading(), which in turn uses the metrics on the first font in the font group. If we don't call UpdateFontList, the first font in the font group isn't updated until later, and we fail a bunch of the reftests because the line spacing is incorrect (since the test ends up still using the line spacing for the fallback font, while the reference uses the line spacing from the downloaded font).
http://hg.mozilla.org/mozilla-central/annotate/d11e76e6fab9/layout/generic/nsHTMLReflowState.cpp#l1992
> Also, isn't there a chance that after a user font set has been torn down and
> recreated that you could end up with an identical pointer?
No. Since gfxFontGroup::mUserFontSet owns a reference to mUserFontSet, the old one won't actually be destroyed until after all the font groups pointing to it have gone away.
Reporter | ||
Updated•16 years ago
|
Attachment #346855 -
Flags: review?(jdaggett) → review+
Comment 37•16 years ago
|
||
(In reply to comment #36)
> (In reply to comment #33)
> > I'm not sure I quite understand this. Why the need to call UpdateFontList?
> > This will happen automatically whenever a text run is recreated, the point at
> > which font matching is performed.
>
> Failing to call UpdateFontList was resulting in incorrect metrics being
> returned in the function GetNormalLineHeight in nsHTMLReflowState.cpp. This
> function calls aFontMetrics->GetExternalLeading(), which in turn uses the
> metrics on the first font in the font group. If we don't call UpdateFontList,
> the first font in the font group isn't updated until later, and we fail a bunch
> of the reftests because the line spacing is incorrect.
Is this going to be the only situation where we hit this? I fear that if we have one fontgroup user needing to do this (from a distance), there may be others.
Or should the gfxFontGroup::GetFontAt(PRInt32) implementations do the UpdateFontList?
Assignee | ||
Comment 38•16 years ago
|
||
(In reply to comment #37)
> Is this going to be the only situation where we hit this? I fear that if we
> have one fontgroup user needing to do this (from a distance), there may be
> others.
>
> Or should the gfxFontGroup::GetFontAt(PRInt32) implementations do the
> UpdateFontList?
Yeah, I was wondering at some point if this should be elsewhere. I'm curious what John thinks.
Assignee | ||
Comment 39•16 years ago
|
||
Comment on attachment 346929 [details] [diff] [review]
patch 2-3-4: do the necessary rebuilding when needed
So for some reason this patch seems to make layout/reftests/css-import/290018-1-ref.html fail because the quirks style sheet starts applying to the reference (but not the test).
(I need to double-check that I've diagnosed things correctly...)
Assignee | ||
Comment 40•16 years ago
|
||
The problem seems to be the AppendFontFaceRules call in nsPresContext::FlushUserFontSet, which I guess causes us to build the rule cascade earlier. I suppose I should just finish bug 450191, though I should probably look into this a little more; perhaps there's something I could be testing to avoid an early call.
(I thought that reftest failure was just a random failure like a bunch of others, but today I bisected all the patches in my tree to figure out what was causing it.)
Assignee | ||
Comment 41•16 years ago
|
||
Here's a revised version of patch 2-3-4 that goes back to lazily rebuilding in the initial case and thus fixes the problem in comment 39 and comment 40.
I'll attach an interdiff for review shortly, since the previous patch was already reviewed.
Attachment #346929 -
Attachment is obsolete: true
Assignee | ||
Comment 42•16 years ago
|
||
Attachment #348462 -
Flags: superreview?(bzbarsky)
Attachment #348462 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 43•16 years ago
|
||
(In reply to comment #37)
> Is this going to be the only situation where we hit this? I fear that if we
> have one fontgroup user needing to do this (from a distance), there may be
> others.
>
> Or should the gfxFontGroup::GetFontAt(PRInt32) implementations do the
> UpdateFontList?
My inclination is to stick an assertion and comment in the GetFontAt implementations so we don't pay the performance penalty of checking every time in release builds, but if somebody else runs into the same problem they'll hit the assertion and quickly know what the problem is (and see the comment suggesting the possible alternative).
Assignee | ||
Comment 44•16 years ago
|
||
revised as described in previous comment
Attachment #346855 -
Attachment is obsolete: true
Attachment #348597 -
Flags: superreview?(roc)
Attachment #348597 -
Flags: review?(jdaggett)
Attachment #346855 -
Flags: superreview?(roc)
Assignee | ||
Comment 45•16 years ago
|
||
Attachment #348461 -
Attachment is obsolete: true
Assignee | ||
Comment 46•16 years ago
|
||
Here's a second interdiff for patch 2-3-4. It turns out that my revisions yesterday caused a bug where StyleChangedReflow would get called during the lazy build that we do during the first GetUserFontSet call.
Since that code was getting confusing (it got me, clearly), I switched from two booleans to three that better describe what I care about, and as a bonus got to deal with out-of-memory while posting events a little bit better.
At this point I'm not sure if it's easier to re-review the whole patch; I'm not sure of a good way to combine the interdiffs, which overlap a lot. If I could combine them, the interdiffs would probably be easier...
Attachment #348613 -
Flags: superreview?(bzbarsky)
Attachment #348613 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 47•16 years ago
|
||
Actually, there's this nice command called 'combinediff'.
Attachment #348462 -
Attachment is obsolete: true
Attachment #348613 -
Attachment is obsolete: true
Attachment #348614 -
Flags: superreview?(bzbarsky)
Attachment #348614 -
Flags: review?(bzbarsky)
Attachment #348462 -
Flags: superreview?(bzbarsky)
Attachment #348462 -
Flags: review?(bzbarsky)
Attachment #348613 -
Flags: superreview?(bzbarsky)
Attachment #348613 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 48•16 years ago
|
||
And this fixes the obvious mistake in my second interdiff, which, amazingly, wasn't caught by my tests. I should go add something that will catch it to my tests. (I guess none of them require rebuilding twice, at least not in a way that won't do anything at all unless we force the reflow!)
Attachment #348618 -
Flags: superreview?(bzbarsky)
Attachment #348618 -
Flags: review?(bzbarsky)
Comment 49•16 years ago
|
||
(In reply to comment #43)
> My inclination is to stick an assertion and comment in the GetFontAt
> implementations so we don't pay the performance penalty of checking every time
> in release builds, but if somebody else runs into the same problem they'll hit
> the assertion and quickly know what the problem is (and see the comment
> suggesting the possible alternative).
That sounds sensible in principle.
I don't know, however, whether that assertion is strictly required.
I wonder whether GetFontAt may be called at a time after another reflow has already been scheduled (but not started)?
If so, it's not strictly necessary to get exactly the right font (though getting the right font would still be nice).
(I don't mean this must hold up this patch. I'm asking the question because I don't yet know the answers.)
Attachment #348597 -
Flags: superreview?(roc) → superreview+
Comment 50•16 years ago
|
||
Comment on attachment 348614 [details] [diff] [review]
patch 2-3-4 combined interdiff
Looks good.
Attachment #348614 -
Flags: superreview?(bzbarsky)
Attachment #348614 -
Flags: superreview+
Attachment #348614 -
Flags: review?(bzbarsky)
Attachment #348614 -
Flags: review+
Comment 51•16 years ago
|
||
Comment on attachment 348618 [details] [diff] [review]
patch 2-3-4 interdiff #3
Doh. Of course.
Attachment #348618 -
Flags: superreview?(bzbarsky)
Attachment #348618 -
Flags: superreview+
Attachment #348618 -
Flags: review?(bzbarsky)
Attachment #348618 -
Flags: review+
Reporter | ||
Comment 52•16 years ago
|
||
(In reply to comment #38)
> (In reply to comment #37)
> > Is this going to be the only situation where we hit this? I fear that if we
> > have one fontgroup user needing to do this (from a distance), there may be
> > others.
> >
> > Or should the gfxFontGroup::GetFontAt(PRInt32) implementations do the
> > UpdateFontList?
>
> Yeah, I was wondering at some point if this should be elsewhere. I'm curious
> what John thinks.
I think we probably want to have UpdateFontList inside GetFontAt(). But we probably also need to verify that the code that's calling GetFontAt isn't assuming this won't change, especially in the case of upstream consumers of metrics info.
Assignee | ||
Comment 53•16 years ago
|
||
Could we push that part off to a separate bug?
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs re-review on one last piece]
Reporter | ||
Comment 54•16 years ago
|
||
virtual gfxFont *GetFontAt(PRInt32 i) {
+ // If it turns out to be hard for all clients that cache font
+ // groups to call UpdateFontList at appropriate times, we could
+ // instead consider just calling UpdateFontList from someplace
+ // more central (such as here).
+ NS_ASSERTION(!mUserFontSet || mCurrGeneration == GetGeneration(),
+ "Whoever was caching this font group should have "
+ "called UpdateFontList on it");
+
I don't think this code will ever execute, since each of the per-platform font group classes override this method:
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/src/gfxWindowsFonts.cpp#889
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/public/gfxAtsuiFonts.h#135
http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/src/gfxPangoFonts.cpp#1402
So I think you need to dupe the assertion into each of those methods.
Reporter | ||
Updated•16 years ago
|
Attachment #348597 -
Flags: review?(jdaggett) → review-
Reporter | ||
Comment 55•16 years ago
|
||
Assignee | ||
Comment 56•16 years ago
|
||
Good point. This copies the assertion to all GetFontAt implementations.
I reran the font-face reftests (including patch 9c) with this patch, and still didn't hit the assertion, which is good (on Linux, Windows, and Mac).
Attachment #348597 -
Attachment is obsolete: true
Attachment #348925 -
Flags: review?(jdaggett)
Reporter | ||
Updated•16 years ago
|
Attachment #348925 -
Flags: review?(jdaggett) → review+
Assignee | ||
Updated•16 years ago
|
Whiteboard: [needs re-review on one last piece] → [needs landing]
Assignee | ||
Comment 57•16 years ago
|
||
Fixed in mozilla-central (and thus also on mozilla-1.9.1):
http://hg.mozilla.org/mozilla-central/rev/91690545debc
http://hg.mozilla.org/mozilla-central/rev/71032d21e8b1
http://hg.mozilla.org/mozilla-central/rev/5bc893b6bdf2
http://hg.mozilla.org/mozilla-central/rev/8d2453e98c3f
http://hg.mozilla.org/mozilla-central/rev/6b99f95a3ee3
Then backed out due to a Mac startup crash, whose underlying cause was actually bug 466399:
http://hg.mozilla.org/mozilla-central/rev/85a49cd83f46
http://hg.mozilla.org/mozilla-central/rev/6ba942a7efa9
http://hg.mozilla.org/mozilla-central/rev/5bf014d8ab20
http://hg.mozilla.org/mozilla-central/rev/8bd6113ce27c
http://hg.mozilla.org/mozilla-central/rev/db0e5d413618
http://hg.mozilla.org/mozilla-central/rev/05ed7ddf001b
http://hg.mozilla.org/mozilla-central/rev/c965c099599b
http://hg.mozilla.org/mozilla-central/rev/3957288748e1
http://hg.mozilla.org/mozilla-central/rev/2c326abe584a
http://hg.mozilla.org/mozilla-central/rev/8b943e17629a
Then relanded, and thus again fixed in mozilla-central (and thus also on mozilla-1.9.1):
http://hg.mozilla.org/mozilla-central/rev/3946e0e1fa54
http://hg.mozilla.org/mozilla-central/rev/948149fa2c65
http://hg.mozilla.org/mozilla-central/rev/13709ba9ea21
http://hg.mozilla.org/mozilla-central/rev/cf88eac1c192
http://hg.mozilla.org/mozilla-central/rev/97214dc4dc0a
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla1.9.1
Assignee | ||
Updated•16 years ago
|
Target Milestone: mozilla1.9.1 → mozilla1.9.1b3
Comment 58•16 years ago
|
||
Running mochitest content/html/content/tests on 64bit linux asserts
###!!! ASSERTION: FlushUserFontSet should have been called first: '!mGetUserFontSetCalled', file /mozilla/layout/base/nsPresContext.cpp, line 1715
Comment 59•16 years ago
|
||
Running the whole mochitest asserts also
###!!! ASSERTION: @font-face family name has non-string unit type: 'unit == eCSSUnit_String', file /mozilla/layout/base/nsPresContext.cpp, line 1602
Assignee | ||
Comment 60•16 years ago
|
||
(In reply to comment #58)
> Running mochitest content/html/content/tests on 64bit linux asserts
> ###!!! ASSERTION: FlushUserFontSet should have been called first:
> '!mGetUserFontSetCalled', file /mozilla/layout/base/nsPresContext.cpp, line
> 1715
That's bug 466756, which is a regression from this patch.
(In reply to comment #59)
> Running the whole mochitest asserts also
> ###!!! ASSERTION: @font-face family name has non-string unit type: 'unit ==
> eCSSUnit_String', file /mozilla/layout/base/nsPresContext.cpp, line 1602
That's bug 460217, which has been waiting for checkin / approval for weeks now.
Updated•16 years ago
|
Keywords: fixed1.9.1
Comment 61•16 years ago
|
||
(In reply to comment #60)
> (In reply to comment #58)
> > Running mochitest content/html/content/tests on 64bit linux asserts
> > ###!!! ASSERTION: FlushUserFontSet should have been called first:
> > '!mGetUserFontSetCalled', file /mozilla/layout/base/nsPresContext.cpp, line
> > 1715
>
> That's bug 466756, which is a regression from this patch.
>
> (In reply to comment #59)
> > Running the whole mochitest asserts also
> > ###!!! ASSERTION: @font-face family name has non-string unit type: 'unit ==
> > eCSSUnit_String', file /mozilla/layout/base/nsPresContext.cpp, line 1602
>
> That's bug 460217, which has been waiting for checkin / approval for weeks now.
Do these mochitest testcases work for this bug? If not, do we need testcases?
Flags: in-testsuite?
Assignee | ||
Comment 62•16 years ago
|
||
No, they don't, but patch 9 contained plenty of tests.
Flags: in-testsuite? → in-testsuite+
Comment 63•16 years ago
|
||
If that's the case, there hasn't been any discussions about this bug for 4 months, I'm guessing there aren't any residual issues. I'm moving this to verified as a result. If anyone has any qualms, feel free to bring them up.
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•