I don't have any tests that exercise this code, and I can't even find a
codepath that demonstrates that it's needed, since the lazy
reconstruction that happens during style-triggered frame reconstruction
all appears to go through PostRestyleEvent rather than
MaybeConstructLazily.
But I think we should either do this or add an assertion that it's not
needed, and given that it's one line, it seems like we may as well just
do it. (Note also that we're currently calling CreateNeededFrames at
the start of style reresolution, in
RestyleManager::ProcessPendingRestyles; this adds a call at the end.)
This makes the ReframingStyleContexts live across the lifetime of the
processing of a full queue of posted restyles.
This depends on bug 1115812 to behave sensibly (and not assert) when
rebuilding the rule tree (RebuildAllStyleData, etc.).
This handles the form of lazy frame construction that is done in
nsCSSFrameConstructor::RecreateFramesForContent, which posts a restyle.
Patch 7 handles any use of the lazy frame construction mechanism.
This patch (with patches 4 and 5 under it, but without patches 1-3)
fixes the original testcase in bug 1110277, except for some flashing of
the final position as the transition starts.
Also fixes bug 1111451.
These assertions are as requested in comment 30 in the bug.
(If the first one doesn't fire, it's possible we could even get rid of
the handling of mDoRebuildAllStyleData that it's within.)
If we discover that we've set mDoRebuildAllStyleData in the middle of
ProcessPendingRestyles(), now that ProcessPendingRestyles() fully
handles mDoRebuildAllStyleData, we only need to make a recursive call to
ProcessPendingRestyles, rather than calling RebuildAllStyleData to call
ProcessPendingRestyles.
This fixes another pre-existing bug in the rebuild-all codepath; it
didn't handle the animation-only update correctly, which could have
caused bugs in transitions with OMT animations enabled.
This means that instead of recurring into DoRebuildAllStyleData, we'll
call StartRebuildAllStyleData in the middle of processing the restyle
queue (which is fine). StartRebuildAllStyleData will move the old rule
tree out of the way and immediately do a full-tree restyle, before
returning to any queue processing that might be left (the full-tree
restyle should have consumed all remaining restyle hints, but might have
posted some new ones for handling reframes that require reframing
ancestors). And, more importantly, the EndReconstruct() call to get rid
of the old rule tree won't happen until after we're done processing the
containing RestyleTracker's queue of restyles, which reduces the risk of
having dangling old style contexts and makes it easier (in bug 1110277)
to have a ReframingStyleContexts with the right lifetime.
This changes what was probably a silly design choice when I wrote the
code for 'rem'-basis handling; we shouldn't try continuing through the
rest of RestyleElement() here, but instead repost the hint to the
rebuild-all process.
This switches RebuildAllStyleData() to the normal
ProcessPendingRestyles() manner of restyle processing. This means a
rebuild-all going through this codepath (the main rebuild-all codepath)
only sets up for non-animation restyle processing once rather than doing
it twice (and potentially having reframes posted in
DoRebuildAllStyleData() that don't get processed until
ProcessPendingRestyles(), which causes a variant of bug 1110277 with
transitions on reframed elements failing to start because it doesn't
match the lifetime of the ReframingStyleContexts).
In the new way of doing a rebuild-all, StartRebuildAllStyleData might be
called directly from ProcessPendingRestyles rather than from
RebuildAllStyleData (which null-checks the root frame) or from within
processing restyles (which can only happen when there's a root frame).
This means it needs its own null-check of the root frame.
Here we call StartRebuildAllStyleData from BeginProcessingRestyles (much
like patch 9 and EndProcessingRestyles). But we will later also call it
from the code that handles a root element font size change when we have
'rem' units. That's because it's fine to *start* the rebuild process in
the middle of processing the queue of pending restyles. (We have to end
after the whole process is done, though, in order to avoid wanting to
destroy the old rule tree while we still have style contexts referencing
it.)
We only call StartRebuildAllStyleData in this case when we're processing
our primary restyle queue (mPendingRestyles), not the animation restyles
(to be removed in bug 960465) or the animation-only restyles, since a
rebuild-all should be processed (in terms of animation phases, or in
terms of having an animation-only update before it) like a normal
restyle. (This isn't true for the 'rem' unit restyle, which could
happen during any sort of update.)
This moves the code that finishes the rebuild-all process into
EndProcessingRestyles(), which is part of the main restyling codepath.
Patch 7 ensures that we'll always get to EndProcessingRestyles in this
case, when we're going through the normal ProcessPendingRestyles()
codepath rather than the special DoRebuildAllStyleData() codepath (which
will be removed later in this patch series).
This fixes one of the omissions in the rebuild-all codepaths (where it
incorrectly differs from the regular ProcessPendingRestyles codepath).
Note that the explicit FlushOverflowChangedTracker() is no longer needed
because that's part of EndProcessingRestyles.
(This will all get refactored more substantially in the following
patches.)
This is needed for patch 9 (once patch 9 is used via the
ProcessPendingRestyles() codepath in patch 13); it ensures that when we
use the new way of rebuilding, we don't bail out early because we think
we have nothing to do.
This adds a member variable that is currently only used within a single
function, but that function will be split apart so that different parts
of it can be called from different places within ProcessPendingRestyles.
This is the variable that says we *need to* rebuild style data. Since
the next patch will introduce a variable that says we're *currently*
rebuilding all style data, renaming this one makes things clearer.
Part of this refactoring involves the ability to start the rebuild-all
process within the processing of restyles. This means we can't pass
parameters directly from RebuildAllStyleData into DoRebuildAllStyleData.
So this continues storing the hints as member variables a little bit
deeper into the process.
(I tried to move in a different direction in this patch queue, and store
these hints in mPendingRestyles, for the root element. But that broke
layout/style/test/test_counter_style.html and
layout/style/test/test_font_loading_api.html, and I didn't want to
figure out why. It would be somewhat better in the long run, since
currently these hints will get processed if we do a rebuild-all on a
RestyleTracker other than mPendingRestyles, which can happen if we have
'rem' units and have a root element font size change in the
animation-only update or in mPendingAnimationRestyles.)
The patches in this series refactor the process of rebuilding all style
data (RestyleManager::RebuildAllStyleData and
RestyleManager::DoRebuildAllStyleData) so that the process of rebuilding
all style data uses the existing restyle processing loops in
ProcessPendingRestyles. (Rebuilding all style data is what we do when
we need to throw away the rule tree because something has invalidated
the cached data in it.) This removes (increasing, especially with bug
960465 coming) code duplicated between the two codepaths, fixes some
omissions from the separate rebuild-all codepath, and (more immediately)
allows fixing lifetime issues of ReframingStyleContexts objects in bug
1110277 so that we can have a single ReframingStyleContexts for all of
the restyle processing in each restyle processing operation. In other
words, the goal is to change the rebuild-all process from a separate
codepath to a few variables that modify the way ProcessPendingRestyles
works (and make it do the extra work).
This is just a small first step in that process, which moves one piece
of code from a chunk of duplicated and to-be-removed code into a chunk
of code that will be preserved.
This means that we avoid an O(N^2) calling pattern of IsVisible when we
call IsVisible for all tabs of a tabbrowser, since in a tabbrowser
(which uses an nsDeckFrame), IsVisibleConsideringAncestors is O(N) in
number of earlier tabs.
The change to GetAfterFrameForContent prevents the reframe that is part
of the chain of events leading to this bug, and thus fixes the bug on
its own. The change to GetBeforeFrameForContent seems desirable for
symmetry.
Note that patch 6 also independently fixes the reported bug.
This probably needs somewhat careful review. We should examine:
(1) what the rules for calling nsLayoutUtils::GetBeforeFrame and
nsLayoutUtils::GetAfterFrame are, and whether both (or neither)
need to be patched.
(2) What the rules are for which frame the GenConProperty() lives on,
and whether we should adjust nsIFrame::GetGenConPseudos() to either
do something more intelligent, or assert about callers.
(We should probably clean up some of these things in a followup bug.)
Since the symptom of this bug is (once patch 4 is in the tree) only
causing extra reframes, it can only be tested using the new API (from
bug 1115691) for observing reframes. I confirmed that the test for this
bug fails without the patch and passes with the patch (as noted by the
removal of its todo annotation).
This patch fixes the assertion on layout/generic/crashtests/600100.xhtml,
though I haven't investigated why.
This patch is not needed to fix the bug, but it seems like it's probably
desirable. It's not needed for this bug because
MaybeReframeForBeforePseudo and MaybeReframeForAfterPseudo are already
called (by ElementRestyler::RestyleChildren) on only the first and last
continuation or ib-split sibling with the same style. So this patch
should only actually change anything for cases like a block-in-inline
split whose initial inline part is inside of a ::first-line (where
different parts of the block-in-inline split chain have different style).
Since the symptom of this bug is (once patch 6 is in the tree) only
causing extra reframes, it can only be tested using the new API (from
bug 1115691) for observing reframes. I confirmed that the test for this
bug fails without the patch and passes with the patch (as noted by the
removal of its todo annotation).
