This is the primary patch in this bug, and makes the performance
improvement that fixes this bug.
The assertion count increase for layout/generic/crashtests/1015844.html
is accompanied by a layout change in the testcase as well. However, I'm
not planning to fix it in this sequence; fundamentally columnsets with
specified heights inside a paginated context (like another columnset) do
not work in any reasonable way, and changing the number of times we
reflow them can change the layout. At least, assuming I didn't lose
something in the process of simplifying the testcase.
ISSUES:
- may make block inside XUL worse in performance by marking dirty more (see subdoc in Firefox UI, or text control innards?)
MozReview-Commit-ID: GdOvPynqcFP
This fixes the regression in
layout/reftests/columns/column-balancing-nested-001.html from the
primary patch in this bug, for which a simplified testcase is
https://bugzilla.mozilla.org/attachment.cgi?id=8848293 . I believe it's
simply a pre-existing bug that wasn't previously exposed. I suspect
it may be possible to write a test that shows the bug prior to the
patch. However, it's difficult, since it requires triggering height
changes in a multicol with an 'auto' height (since a non-'auto' height
would cause the multicol to have NS_FRAME_CONTAINS_RELATIVE_BSIZE set by
nsColumnSetFrame::Reflow, which would make skipIncremental false because
ShouldReflowAllKids returns true). I suspect any working testcase would
likely be rather brittle, so I haven't pursued it further (particularly
given the complexity of the minimal testcase).
MozReview-Commit-ID: Gve3XKEPSxL
This fixes (confirmed by testing locally) a regression in
layout/reftests/w3c-css/received/css-multicol-1/multicol-nested-margin-004.xht
resulting from the primary patch in this bug, which tends to make frames
dirty less often. The problem with that test is that (at least in a
simplified form), in the final reflow of the inner ColumnSet in the
first column of the outer ColumnSet, the inner ColumnSet chooses not to
reflow its first column, thus leaving that first column having a height
that is too large for the inner ColumnSet to fit in the first column of
the outer ColumnSet, causing the entire inner ColumnSet (rather than
just part of it) to be pushed to the next column.
I believe this existing incremental reflow code just doesn't make sense.
The code I'm modifying dates back primarily to:
c237520c89 (October 2004, initial columns implementation)
ee070ec95f (March 2005)
31e3540d1e (November 2006)
The first thing that doesn't make sense is the condition modified at the
end of this patch:
(!reflowNext && (skipIncremental || skipResizeBSizeShrink))
There's simply no reason that that || isn't required to be an &&, as far
as I can tell. Even if we don't need to reflow due to any of the
standard incremental reflow conditions, we can need to reflow because
the block size is shrinking and the column no longer fits.
Note that things were already OK when we required reflow due to
NS_SUBTREE_DIRTY(this), because of the way shrinkingBSizeOnly was
initialized using !NS_SUBTREE_DIRTY(this), thus excluding such cases
from the optimization.
The rest of the patch falls out of turning the || into an && in an
efficient way (i.e., without the extra !NS_SUBTREE_DIRTY(this) test, and
avoiding doing extra tests that we know we're not going to need by
coalescing all the incremental reflow tests into a single variable).
I tested that this patch passes try on its own (on 64-bit Linux debug):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a279023fb7e8f3349d5ecbfb95807d6b097cdbcb
MozReview-Commit-ID: BD3ofmWN5Wl
This is the primary patch in this bug, and makes the performance
improvement that fixes this bug.
The assertion count increase for layout/generic/crashtests/1015844.html
is accompanied by a layout change in the testcase as well. However, I'm
not planning to fix it in this sequence; fundamentally columnsets with
specified heights inside a paginated context (like another columnset) do
not work in any reasonable way, and changing the number of times we
reflow them can change the layout. At least, assuming I didn't lose
something in the process of simplifying the testcase.
ISSUES:
- may make block inside XUL worse in performance by marking dirty more (see subdoc in Firefox UI, or text control innards?)
MozReview-Commit-ID: GdOvPynqcFP
This fixes the regression in
layout/reftests/columns/column-balancing-nested-001.html from the
primary patch in this bug, for which a simplified testcase is
https://bugzilla.mozilla.org/attachment.cgi?id=8848293 . I believe it's
simply a pre-existing bug that wasn't previously exposed. I suspect
it may be possible to write a test that shows the bug prior to the
patch. However, it's difficult, since it requires triggering height
changes in a multicol with an 'auto' height (since a non-'auto' height
would cause the multicol to have NS_FRAME_CONTAINS_RELATIVE_BSIZE set by
nsColumnSetFrame::Reflow, which would make skipIncremental false because
ShouldReflowAllKids returns true). I suspect any working testcase would
likely be rather brittle, so I haven't pursued it further (particularly
given the complexity of the minimal testcase).
MozReview-Commit-ID: Gve3XKEPSxL
This fixes (confirmed by testing locally) a regression in
layout/reftests/w3c-css/received/css-multicol-1/multicol-nested-margin-004.xht
resulting from the primary patch in this bug, which tends to make frames
dirty less often. The problem with that test is that (at least in a
simplified form), in the final reflow of the inner ColumnSet in the
first column of the outer ColumnSet, the inner ColumnSet chooses not to
reflow its first column, thus leaving that first column having a height
that is too large for the inner ColumnSet to fit in the first column of
the outer ColumnSet, causing the entire inner ColumnSet (rather than
just part of it) to be pushed to the next column.
I believe this existing incremental reflow code just doesn't make sense.
The code I'm modifying dates back primarily to:
c237520c89 (October 2004, initial columns implementation)
ee070ec95f (March 2005)
31e3540d1e (November 2006)
The first thing that doesn't make sense is the condition modified at the
end of this patch:
(!reflowNext && (skipIncremental || skipResizeBSizeShrink))
There's simply no reason that that || isn't required to be an &&, as far
as I can tell. Even if we don't need to reflow due to any of the
standard incremental reflow conditions, we can need to reflow because
the block size is shrinking and the column no longer fits.
Note that things were already OK when we required reflow due to
NS_SUBTREE_DIRTY(this), because of the way shrinkingBSizeOnly was
initialized using !NS_SUBTREE_DIRTY(this), thus excluding such cases
from the optimization.
The rest of the patch falls out of turning the || into an && in an
efficient way (i.e., without the extra !NS_SUBTREE_DIRTY(this) test, and
avoiding doing extra tests that we know we're not going to need by
coalescing all the incremental reflow tests into a single variable).
I tested that this patch passes try on its own (on 64-bit Linux debug):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a279023fb7e8f3349d5ecbfb95807d6b097cdbcb
MozReview-Commit-ID: BD3ofmWN5Wl
nsIFrame::mClass is of type enum class nsQueryFrame::ClassID which is
a strict subset of the nsQueryFrame::FrameIID values. For a concrete
frame class, its FrameIID is the same numeric value as its ClassID.
MozReview-Commit-ID: 1N0AkCGo1ol
This propagates the StackingContextHelper to the rest of the code that
builds WR display items, because we will need it in future patches to
stop using RelativeToParent.
MozReview-Commit-ID: 3PlQrJRhH36
This is needed for the next patch, because the bullet frame WR rendering
code will need a StackingContextHelper to pass to the PushGlyphs call.
It also helps with some of the later patches in this series, when we get
rid of the RelativeToParent calls.
MozReview-Commit-ID: Is0aciHhy2N
This patch is written by the following script with some manual adjustment to
the comment in nsRubyTextContainerFrame.cpp and nsRubyFrame.cpp, and
nsColumnSetFrame's constructor.
function rename() {
find layout\
-type f\
\( -name "*.cpp" -or\
-name "*.h" \)\
-exec sed -i -r "s/$1/$2/g" "{}" \;
}
rename "nsReflowStatus *([a-zA-Z0-9]*) = NS_FRAME_COMPLETE" "nsReflowStatus \1"
rename "([a-zA-Z0-9.*]*) *= NS_FRAME_COMPLETE;" "\1.Reset();"
rename "([a-zA-Z0-9.*]*) == NS_FRAME_COMPLETE" "\1.IsEmpty()"
MozReview-Commit-ID: 9tqQAHvdQex
Also, remove NS_FRAME_TRUNCATED and NS_FRAME_REFLOW_NEXTINFLOW because both
are used only by NS_MergeReflowStatusInto().
MozReview-Commit-ID: LsPOji9j2e
This patch is written by the help of the following script.
function rename() {
find layout\
-type f\
\( -name "*.cpp" -or\
-name "*.h" \)\
-exec sed -i -r "s/$1/$2/g" "{}" \;
}
rename "NS_FRAME_OVERFLOW_IS_INCOMPLETE\(([a-zA-Z0-9.*]*)\)" "\1.IsOverflowIncomplete()"
MozReview-Commit-ID: GOd4y2N6dcz
This patch is written by the help of the following script.
function rename() {
find layout\
-type f\
\( -name "*.cpp" -or\
-name "*.h" \)\
-exec sed -i -r "s/$1/$2/g" "{}" \;
}
rename "NS_FRAME_IS_FULLY_COMPLETE\(([a-zA-Z0-9.*]*)\)" "\1.IsFullyComplete()"
MozReview-Commit-ID: GOd4y2N6dcz
I think there are three advantages of this change:
1. removes some dependencies from layout / painting code to pre-computed
value stuff in the style system;
2. makes it easier to audit usage of specific fields in style structs
(which is probably a side effect of the first one);
3. potentially improves performance since it doesn't go through the
unnecessary general logic in ExtractComputedValue.
Also, combined with the part before, we get a unified list for visited-
dependent properties so that we can ensure the assertion here and the
style difference calc code are consistent.
MozReview-Commit-ID: 5B9aN7CfRgI
This patch was written with the help of the following script. Also, manually
add mozilla qualifier to the enum values in nsStyleCoord.h, gfxRect.h, and
Types.h to make it build.
function rename() {
find .\
-type f\
! -path "./obj*"\
! -path "./.git"\
! -path "./.hg"\
\( -name "*.cpp" -or\
-name "*.h" \)\
-exec sed -i -e "s/$1/$2/g" "{}" \;
}
rename "NS_SIDE_TOP" "eSideTop"
rename "NS_SIDE_RIGHT" "eSideRight"
rename "NS_SIDE_BOTTOM" "eSideBottom"
rename "NS_SIDE_LEFT" "eSideLeft"
MozReview-Commit-ID: 9T0ORsqM6nP
Note that nsMathMLContainerFrame and its subclasses are unchanged since
they are not target of fullscreen (and thus no backdrop frame), and they
have an assertion to ensure we really don't pass any unexpected list in.
