222730 - [FIX]Possible optimization in reflow state (call CalcQuirkContainingBlockHeight less)

Status: RESOLVED FIXED

(Core :: Layout, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

It looks like we will often call CalcQuirkContainingBlockHeight even when the
frame in question doesn't have a percent height.  Since
CalcQuirkContainingBlockHeight is not the fastest thing in the world, and since
we only need it if the frame has a percent height, I think we should add such a
check at this callsite.

Once this lands, I would be temped to change the "aRestrictToFirstLevel" switch
at this callsite to "false" and see what effect that has on Tp, as well as on
the regression tests.

Patch coming up; I've run the regression tests on it and they are happy.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 133548 [details] [diff] [review]
Patch

David, could you review?

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 133548 [details] [diff] [review]
Patch

If this did regress anything, we'd want to know what was depending on this that
shouldn't be.  r+sr=dbaron, but fix the comment 3 lines about what you changed.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 133548 [details] [diff] [review]
Patch

Checked in, but doesn't seem to have much of an effect on Tp (if any).	On the
other hand, I've just tested passing PR_FALSE for the aRestrictToFirstLevel
flag (with this patch already in, of course) and Tp was not affected.  So I
think we should remove the whole aRestrictToFirstLevel optimization; I'll be
attaching a patch to that effect once I've run some regression tests... ;)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch to remove the aRestrictToFirstLevel arg

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 133594 [details] [diff] [review]
Proposed patch to remove the aRestrictToFirstLevel arg

David, what do you think?  I ran this through the regression tests and there
are only four on which it made a difference:

5196, 50695-1, 88035-2, and 85016.  In all these cases, the change makes us
render the testcase like IE does.

In fact, I would say this patch fixes bug 88035.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Fix checked in.