1839994 - Consider rename SetForcedLineBreakBefore() in nsLineBox

Status: RESOLVED FIXED

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

Description

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

SetForcedLineBreakBefore() is confusing because it's all about float clearance for a block line, and not related forced inline line break.

Per naming suggestions by fantasai in matrix:

SetClearBefore()? :)
Or maybe SetFloatClearTypeBefore(), to parallel HasFloatClearTypeAfter() a few lines below
Or compromise and go with SetFloatClearBefore / HasFloatClearAfter / etc. ? :)
I guess options are
a) Shortest option: (Has|Set)Clear(Before|After)
b) Longest option: (Has|Set)FloatClearType(Before|After)
c) Halfway option: (Has|Set)FloatClear(Before|After)
I'm guessing for historical reasons (<br clear> existed before CSS clear) this is all mixed up with “break”
But my recommendation would be to keep Break as referring only to line break or page break, just use Clear when talking about clearance stuff

My preference would be b) since it complements the existing HasFloatClearTypeAfter().

Comment 1

[fantasai]

The idea was, if you go with a) or c) you should rename HasFloatClearTypeAfter(). :) I agree they should be consistent.

Comment 2

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1839994 Part 1 - Minor clean-up in nsLineBox interfaces. r?#layout

Hide HasForcedLineBreak() from public interface since it is used only in
nsLineBox.

Make ScrollableOverflowRect() const since there is no reason not to.

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1839994 Part 2 - Rename (Has|Set)ForcedLineBreakBefore to (Has|Set)FloatClearTypeBefore. r?#layout

This is the option b) in bug 1839994 comment 0.

Comment 4

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1839994 Part 3 - Improve the implementation and docs around mHasForcedLineBreak and mFloatClearType. r?dholbert,jfkthame

Rename mHasForcedLineBreak to mHasForcedLineBreakAftersince block lines don't need to set this bit. This patch removes setting the bit inSetFloatClearTypeBefore(), and adds an assertion inHasForcedLineBreakAfter()` to ensure that.

Improve HasFloatClearType(Before|After) implementation by using
FloatClearType(Before|After).

This patch doesn't change behavior.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2885551667c6
Part 1 - Minor clean-up in nsLineBox interfaces. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/95d4dc214cbe
Part 2 - Rename (Has|Set)ForcedLineBreakBefore to (Has|Set)FloatClearTypeBefore. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/a4d5b8607193
Part 3 - Improve the implementation and docs around mHasForcedLineBreak and mFloatClearType. r=dholbert

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2885551667c6
https://hg.mozilla.org/mozilla-central/rev/95d4dc214cbe
https://hg.mozilla.org/mozilla-central/rev/a4d5b8607193