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

RESOLVED FIXED in Firefox 39

Status

()

Core
Layout: Block and Inline
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dbaron, Assigned: xidorn)

Tracking

Trunk
mozilla39
x86_64
All
Points:
---

Firefox Tracking Flags

(firefox39 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
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
Flags: needinfo?(quanxunzhen)
(Assignee)

Comment 1

3 years ago
Created attachment 8576501 [details] [diff] [review]
patch
Assignee: nobody → quanxunzhen
Flags: needinfo?(quanxunzhen)
Attachment #8576501 - Flags: review?(dbaron)
(Reporter)

Comment 2

3 years ago
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)
Attachment #8576501 - Flags: review?(dbaron) → review+
(Reporter)

Comment 3

3 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/0654036827f7
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox39: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.