1142400 - nsBulletFrame sets mPadding differently in Get{Min,Pref}ISize and Reflow

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

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

After looking at the patch in bug 1142369, the code in nsBulletFrame looks somewhat broken, because:

 * nsBulletFrame::GetDesiredSize, which is called from GetMinISize, GetPrefISize, and Reflow, initializes mPadding and adds some internal padding to it

 * nsBulletFrame::Reflow also adds the CSS computed border and padding to it

 * nsBulletFrame::PaintBullet uses mPadding, presuming that Reflow was called more recently than Get{Min,Pref}ISize

However, that isn't guaranteed; it's allowed for Get{Min,Pref}ISize to have been called more recently, which might happen in response to certain dynamic changes (particularly if a block is optimizing away reflow of some of its lines, but needs to recalculate its intrinsic widths due to a change later in those lines, although there might be other ways to get it to happen).


Really, instead, either:

 (a) GetDesiredSize and AppendSpacingToPadding should append to a parameter to those functions, Reflow should pass mPadding as the parameter, and Get{Min,Pref}ISize should pass a local variable, or

 (b) Reflow should stop adding the computed border and padding to mPadding, but instead that code should add to a local variable that goes into linePadding, and the same code should be copied to nsBulletFrame::PaintBullet.

I think I probably prefer (a) at first glance.

I believe this is a regression from https://hg.mozilla.org/mozilla-central/rev/0ec4cc06f49f

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: patch

Comment 2

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

Comment on attachment 8576501 [details] [diff] [review]
patch

It would probably have matched normal conventions better to use LogicalMargin& rather than LogicalMargin*.

r=dbaron either way, though (if you want to fix it or not)

Comment 3

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

(In reply to David Baron [:dbaron] (UTC-8) from comment #2)
> Comment on attachment 8576501 [details] [diff] [review]
> patch
> 
> It would probably have matched normal conventions better to use
> LogicalMargin& rather than LogicalMargin*.

Er, as you point out on IRC, for an out-param, LogicalMargin* is better.

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0654036827f7

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0654036827f7