Closed Bug 1301630 Opened 3 years ago Closed 3 years ago

Revise nsBlockFrame::SetFlags() usage

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

Details

Attachments

(1 file)

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 on attachment 8789694 [details]
Bug 1301630 - Remove nsBlockFrame::SetFlags().

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

r=me
Attachment #8789694 - Flags: review?(bzbarsky) → review+
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 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.
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....
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?
Flags: needinfo?(bzbarsky)
See Also: → 1301881
> 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.
Flags: needinfo?(bzbarsky)
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4fb8ce27c37f
Remove nsBlockFrame::SetFlags(). r=bz
https://hg.mozilla.org/mozilla-central/rev/4fb8ce27c37f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.