Open Bug 766599 Opened 8 years ago Updated 1 year ago

Reflag frame tree for font size inflation preference changes

Categories

(Core :: Layout, enhancement)

enhancement
Not set
normal

Tracking

()

People

(Reporter: jwir3, Assigned: JanH)

References

Details

Attachments

(1 file, 1 obsolete file)

After the landing of bug 749186 and bug 763702, we have a solution in place that prevents crashing within nsFontInflationData. This requires a reload after the font inflation preferences have changed. In bug 749186, another solution was proposed: generate a reframe event that reconstructs all frames when we change these preferences.

The problem with this is that there are some minor issues with this solution which cause assertions in unit tests, possibly because we're destroying the top-level presShell (chrome) when we should not be (but this is a hypothesis). Once this problem is solved, we should back out the temporary solution for the crash and push this newer, more robust solution.
Duplicate of this bug: 767351
relnote for 14: Reload of page is required for text size changes to take affect
Keywords: relnote
OS: Linux → All
Hardware: x86_64 → All
Hm, so in my testing, reverting the last patch for bug 749186, which happens to be this commit on trunk:
https://hg.mozilla.org/mozilla-central/rev/30e2c7c9f24e

Doesn't seem to cause the test_bug49186.html mochitest to fail, which is somewhat disconcerting. 

My suspicion is that the bug still exists after the patch is backed out (i.e. it's not like something else changed it, I don't think...), but that it's harder to reproduce. I'm trying to see if I can find an appropriate regression range for when this test magically started working again, so that I can verify that the bug still exists.
(In reply to Scott Johnson (:jwir3) from comment #3)
> Hm, so in my testing, reverting the last patch for bug 749186, which happens
> to be this commit on trunk:
> https://hg.mozilla.org/mozilla-central/rev/30e2c7c9f24e
> 
> Doesn't seem to cause the test_bug49186.html mochitest to fail, which is
> somewhat disconcerting. 
> 
> My suspicion is that the bug still exists after the patch is backed out
> (i.e. it's not like something else changed it, I don't think...), but that
> it's harder to reproduce. I'm trying to see if I can find an appropriate
> regression range for when this test magically started working again, so that
> I can verify that the bug still exists.

Ugh... my bad. I forgot there's a prerequisite patch I needed to push, otherwise font inflation statuses are always set in DEBUG mode.
I think we concluded this is easier to fix now that bug 862763 is fixed, though I'm not 100% sure.
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #5)
> I think we concluded this is easier to fix now that bug 862763 is fixed,
> though I'm not 100% sure.

Yes, this is true. Also, given that we want to leave this in place, I think it's probably easier to leave the caching of the font inflation preferences in place in the pres shell (as an additional measure to prevent crashing), and just trigger a reframe operation when they change. That is, I mean that the patch I'm testing at the moment doesn't actually back out the changes made in bug 749186, but rather adds to it the reframing solution. 

Thoughts?
Summary: Backout current fix and re-land permanent fix for nsFontInflationData crash → Trigger a reframe operation after font size inflation preferences are changed
Summary: Trigger a reframe operation after font size inflation preferences are changed → Reflag frame tree for font size inflation preference changes
Attached patch b766599 (obsolete) — Splinter Review
Attached patch b766599Splinter Review
This patch adds two new methods: ReflagFrameTreeForFontSizeInflation() and ReflagFrameForFontSizeInflation(). The latter method checks frame state bits for a frame and all of its children, setting those needed (as appropriate), and the former calls ReflagFrameForFontSizeInflation() on the root frame for a given presentation.

A handler is added for changes to the font.size.inflation preferences that calls the ReflagFrameTreeForFontSizeInflation() method on a given presentation when these preferences change. 

The patch also refactors two methods: SetupFontInflation() was renamed to RefreshFontSizeInflationPreferences(), which pulls values from nsLayoutUtils into the (cached versions) in the presentation shell. The initialization code for a frame that sets up the font size inflation frame state bits was extracted into a method called CheckAndSetFontSizeInflationBits() so that it can be reused in ReflagFrameForFontSizeInflation().
Attachment #798954 - Attachment is obsolete: true
Attachment #798960 - Flags: review?(dbaron)
Comment on attachment 798960 [details] [diff] [review]
b766599

Instead of adding your two new methods to nsLayoutUtils, add them to
nsPresContext instead.  The one that takes an nsPresContext as an
argument should be a member function of nsPresContext (not static,
and no argument); the other one can just be a static function in the
global scope.

Could you also rename both of them to RebuildFontInflationBits, and
rename mPendingFontInflationChangeNeedsReflag to
mPendingFontInflationPrefChange?  I'd rather avoid introducing the term
"reflag".

In what's now the static function, this:

>+  // Recursive step
>+  nsAutoTArray<FrameChildList, 4> lists;
>+  aFrame->GetChildLists(&lists);
>+  for (uint32_t i = 0, len = lists.Length(); i < len; ++i) {
>+    const nsFrameList& list = lists[i].mList;
>+    for (nsIFrame* kid = list.FirstChild(); kid; kid = kid->GetNextSibling()) {

should instead be:

>  nsIFrame::ChildListIterator lists(aFrame);
>  for (; !lists.IsDone(); lists.Next()) {
>    nsFrameList::Enumerator childFrames(lists.CurrentList());
>    for (; !childFrames.AtEnd(); childFrames.Next()) {
>      nsIFrame* kid = childFrames.get();




In nsIFrame.h, move CheckAndSetFontSizeInflationBits down after
FindCloserFrameForSelection so that it doesn't clutter up the really
basic stuff.


>+  if (IsBoxWrapped())
>+    InitBoxMetrics(false);

given that you're moving this, put braces around the if-body

>+    // Ideally, we would perform the reflag operation here, rather than waiting
>+    // until UpdateAfterPreferencesChanged() is called, but the preference cache
>+    // variables will not have been updated yet in nsLayoutUtils.

I think the "Ideally" is false because we also want the coalescing that
the timer mechanism here does.  So could you reword the comment?  Also
wrap to less than 80 characters.

(Also perhaps add a comment that it doesn't really need most of the work
that the callback does?)


You should also change nsIFrame::CheckAndSetFontSizeInflationBits
to RemoveStateBits(NS_FRAME_FONT_INFLATION_CONTAINER |
NS_FRAME_FONT_INFLATION_FLOW_ROOT) (wrapped properly) as the first
thing it does, so that it *unsets* the bits when needed.


Also, given that the test currently has the functions in the reverse
order of the order they happen, could you maintain that when inserting
the new functions?

Also, should the test at some point compare snapshots and assert that
they're not equal, to assert that something actually changed in between?

r=dbaron with those changes, and sorry for the delay getting to this
Attachment #798960 - Flags: review?(dbaron) → review+
Duplicate of this bug: 1482779
I spent enough time looking at bug 1428670 that I might as well try to revive this patch as well afterwards.
Assignee: jaywir3 → jh+bugzilla
Type: defect → enhancement
You need to log in before you can comment on or make changes to this bug.