1301630 - Revise nsBlockFrame::SetFlags() usage

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

bz said in bug 1299753 comment 20 that nsBlockFrame::SetFlags() is designed for flag propagation to continuations during block reflow, and may not be appropriate to other situations.

To avoid misuse, my solution is proposed in bug 1299753 comment 21.

Comment 1

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1301630 - Remove nsBlockFrame::SetFlags().

Per bug 1299753 comment 20, SetFlags() is designed for flag propagation
to continuations during block reflow. To avoid misuse, I expand
SetFlags() directly in the only reasonable caller nsBlackFrame::Init(),
and replace other usages by AddStateBits().

Review commit: https://reviewboard.mozilla.org/r/77832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/77832/

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8789694 [details]
Bug 1301630 - Remove nsBlockFrame::SetFlags().

https://reviewboard.mozilla.org/r/77832/#review76258

r=me

Comment 3

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8789694 [details]
Bug 1301630 - Remove nsBlockFrame::SetFlags().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77832/diff/1-2/

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8789694 [details]
Bug 1301630 - Remove nsBlockFrame::SetFlags().

https://reviewboard.mozilla.org/r/77832/#review76310

::: layout/mathml/nsMathMLContainerFrame.h
(Diff revision 2)
>  
>  protected:
> -  explicit nsMathMLmathBlockFrame(nsStyleContext* aContext) : nsBlockFrame(aContext) {
> -    // We should always have a float manager.  Not that things can really try
> +  explicit nsMathMLmathBlockFrame(nsStyleContext* aContext)
> +    : nsBlockFrame(aContext) {}
> -    // to float out of us anyway, but we need one for line layout.
> -    AddStateBits(NS_BLOCK_FLOAT_MGR);

Uh... Why are you removing that?  It's not OK to remove it afaict.

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8789694 [details]
Bug 1301630 - Remove nsBlockFrame::SetFlags().

https://reviewboard.mozilla.org/r/77832/#review76310

> Uh... Why are you removing that?  It's not OK to remove it afaict.

I got a R10 try failure on Android here so I remove it in order to launch a new try on mozreview.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=daf6340906f4

Actully, removing "AddStateBits(NS_BLOCK_FLOAT_MGR)" retains the behavior. In the old code, if the caller pass nsFrameState(0) to NS_NewMathMLmathBlockFrame(), SetFlags() will clear the NS_BLOCK_FLOAT_MGR set in nsMathMLmathBlockFrame's constructor. As a result, the block frames create by NS_CreateNewMathMLmathBlockFrame() won't have any flags.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Ugh.  That old behavior doesn't seem right at all.  The whole point of that line in the mathml code was that we always wanted that flag in MathML....

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

If I blamed the code correctly, NS_BLOCK_FLOAT_MGR was added in nsMathMLmathBlockFrame's constructor in bug 353894. One year later the flag was clear accidentally by bug 397518.

It's beyond scope of this bug to figure out why NS_BLOCK_FLOAT_MGR is not set in nsMathMLmathBlockFrame and no one complains over these years. Adding NS_BLOCK_FLOAT_MGR back in my first patchset also breaks no MathML test but one font inflation test. By the way, font inflation was turned off in bug 1127441.

Boris, if you do not object, let's land my patch. But instead of removing AddStateBits(NS_BLOCK_FLOAT_MGR) in the nsMathMLmathBlockFrame's constructor, I'll comment the line to retain the current behavior and file a bug for further investigation. What do you think?

Comment 8

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8789694 [details]
Bug 1301630 - Remove nsBlockFrame::SetFlags().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/77832/diff/2-3/

Comment 9

[Boris Zbarsky [:bzbarsky]]

> NS_BLOCK_FLOAT_MGR was added in nsMathMLmathBlockFrame's constructor in bug 353894

Yes, that sounds right.  I apparently didn't add the testcase as a test at the time; this may have predated us having a crashtest harness...  Interestingly, that testcase neither crashes nor asserts in a current debug nightly....

> One year later the flag was clear accidentally by bug 397518.

Boo, SetFlags.  ;)

OK, let's get this sorted out in bug 1301881.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4fb8ce27c37f
Remove nsBlockFrame::SetFlags(). r=bz

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4fb8ce27c37f