1273049 - Move the relevant info of frame bit flag from in block-and-line.html to nsFrameStateBits.h

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

Quote from mats in bug 1272840 comment 4:

I'd prefer to move the relevant info to the header file instead so that it
can be kept up-to-date, and then remove this link.
Only NS_BLOCK_MARGIN_ROOT and NS_BLOCK_SPACE_MGR have relevant info as far
as I can tell.  I assume the latter is now named NS_BLOCK_FLOAT_MGR.
The BRS_IS*MARGINROOT flags are now using logical names instead.

The info is in http://www-archive.mozilla.org/newlayout/doc/block-and-line.html

Comment 1

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

Attachment: Bug 1273049 - Migrate documentation in block-and-line.html into the tree.

I also adjust some of the old flag name to their logical name, and
rewrite some of the content where I see fit.

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

Comment 2

[Daniel Holbert [:dholbert]]

Comment on attachment 8769149 [details]
Bug 1273049 - Migrate documentation in block-and-line.html into the tree.

https://reviewboard.mozilla.org/r/63078/#review60742

Thanks for fixing this up! This is nearly there, though I'm not quite ready to grant r+; hence, marking r- for now. Nits below:

Nit on the commit message:
"Migrate content in block-and-line.html into the tree"

s/content/documentation/, to make it superficially clear what "content" means here (and that this is a documentation-only change).

::: layout/generic/nsBlockReflowState.h:24
(Diff revision 1)
> -  // block reflow state flags
> +// Block reflow state flags.
> +//
> +// BRS_UNCONSTRAINEDBSIZE is set in the nsBlockReflowState constructor when the
> +// frame being reflowed has been given NS_UNCONSTRAINEDSIZE as its available
> +// height in the nsHTMLReflowState. If set, NS_UNCONSTRAINEDSIZE is passed to
> +// nsLineLayout as the available height.

s/height/BSize/

(there are 2 mentions of "height" which both need fixing)

::: layout/generic/nsBlockReflowState.h:27
(Diff revision 1)
> +// frame being reflowed has been given NS_UNCONSTRAINEDSIZE as its available
> +// height in the nsHTMLReflowState. If set, NS_UNCONSTRAINEDSIZE is passed to
> +// nsLineLayout as the available height.
>  #define BRS_UNCONSTRAINEDBSIZE    0x00000001
> -#define BRS_ISBSTARTMARGINROOT    0x00000002  // Is this frame a root for block
> -#define BRS_ISBENDMARGINROOT      0x00000004  //  direction start/end margin collapsing?
> +// BRS_ISBSTARTMARGINROOT is set in the nsBlockReflowState constructor when
> +// reflowing a ``block margin root'' frame (i.e. a frame with the

Huh, this double-backtick/double-single-quote style in the old documentation is quote odd!

I'd rather not carry forward that stylistic cruft into this source code (in part because it makes these lines longer than they need to be, and in part bcause I'm guessing it existed purely to make this look prettier when viewed as HTML in web browsers of the 90's).

Could you do a search-and-replace, to swap out all two-backtick and all two-single-quote patterns with a normal double-quote character? I think we only need that changed for nsBlockReflowState.h and nsLineLayout.h -- those are the only places I noticed it in this patch.

::: layout/generic/nsBlockReflowState.h:30
(Diff revision 1)
>  #define BRS_UNCONSTRAINEDBSIZE    0x00000001
> -#define BRS_ISBSTARTMARGINROOT    0x00000002  // Is this frame a root for block
> -#define BRS_ISBENDMARGINROOT      0x00000004  //  direction start/end margin collapsing?
> -#define BRS_APPLYBSTARTMARGIN     0x00000008  // See ShouldApplyTopMargin
> +// BRS_ISBSTARTMARGINROOT is set in the nsBlockReflowState constructor when
> +// reflowing a ``block margin root'' frame (i.e. a frame with the
> +// NS_BLOCK_MARGIN_ROOT flag set, for which margins apply by default).
> +//
> +// The flag is also set when reflowing a frame whose computed top border padding

s/top/BStart/

::: layout/generic/nsBlockReflowState.h:37
(Diff revision 1)
> +#define BRS_ISBSTARTMARGINROOT    0x00000002
> +// BRS_ISBENDMARGINROOT is set in the nsBlockReflowState constructor when
> +// reflowing a ``block margin root'' frame (i.e. a frame with the
> +// NS_BLOCK_MARGIN_ROOT flag set, for which margins apply by default).
> +//
> +// The flag is also set when reflowing a frame whose computed bottom border

s/bottom/BEnd/

::: layout/generic/nsBlockReflowState.h:41
(Diff revision 1)
> +//
> +// The flag is also set when reflowing a frame whose computed bottom border
> +// padding is non-zero.
> +#define BRS_ISBENDMARGINROOT      0x00000004
> +// See ShouldApplyBStartMargin.
> +#define BRS_APPLYBSTARTMARGIN     0x00000008

There are a few paragraphs about this flag at http://www-archive.mozilla.org/newlayout/doc/block-and-line.html which seem to be getting lost here. (The "See ShouldApplyBStartMargin" doesn't cover them, because ShouldApplyBStartMargin() doens't seem to include much documentation.)

Can/should we add a modernized version of the paragraphs from block-and-line.html here?

::: layout/generic/nsBlockReflowState.h:57
(Diff revision 1)
>  #define BRS_PROPTABLE_FLOATCLIST  0x00000200
>  // Set when the pref layout.float-fragments-inside-column.enabled is true.
>  #define BRS_FLOAT_FRAGMENTS_INSIDE_COLUMN_ENABLED 0x00000400
>  #define BRS_LASTFLAG              BRS_FLOAT_FRAGMENTS_INSIDE_COLUMN_ENABLED
>  
> +// nsBlockReflowState contains Additional reflow state information that the

"Additional" is unnecessarily capitalized here. 

s/A/a/ please.

::: layout/generic/nsFrameStateBits.h:54
(Diff revision 1)
>  
>  // == Frame state bits that apply to all frames ===============================
>  
>  FRAME_STATE_GROUP(Generic, nsIFrame)
>  
> +// This bit is set when the frame is being actively being reflowed. It is set in

Typo (from the original text) here: "being actively being".

Collapse this to "actively being", please.

::: layout/generic/nsFrameStateBits.h:472
(Diff revision 1)
> +// This causes the nsBlockReflowState's constructor to set the
> +// BRS_ISBSTARTMARGINROOT and BRS_ISBENDMARGINROOT flags.
>  FRAME_STATE_BIT(Block, 22, NS_BLOCK_MARGIN_ROOT)
> +
> +// This indicates that a block frame should create its own float manager. This
> +// is required by each block frame that can contain floaters: the float manager

"floaters" is outdated term (and is not used anywhere else in our codebase).

Please replace it with "floats" here.

::: layout/generic/nsFrameStateBits.h:475
(Diff revision 1)
> +
> +// This indicates that a block frame should create its own float manager. This
> +// is required by each block frame that can contain floaters: the float manager
> +// is used to reserve space for the floated frames.
>  FRAME_STATE_BIT(Block, 23, NS_BLOCK_FLOAT_MGR)
>  FRAME_STATE_BIT(Block, 24, NS_BLOCK_HAS_LINE_CURSOR)

Please add a blank line between the FLOAT_MGR and HAS_LINE_CURSOR bits here.

(Otherwise, it looks like the FLOAT_MGR header comment is somehow describing all of the subsequent 4 lines, since they're all tucked closely together. Note that other bits / groups-of-bits with documentation here are separated by blank lines.)

::: layout/generic/nsLineLayout.h:527
(Diff revision 1)
>      PerFrameData* mFrame;
> +
> +    // The first PerFrameData structure in the span.
>      PerFrameData* mFirstFrame;
> +
> +    // The last PerFrameData structure frame in the span. PerFrameData

Nit: s/structure frame/structure/

(this is a typo in the original text, which we might as well fix here)

Comment 3

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

https://reviewboard.mozilla.org/r/63078/#review60742

> Huh, this double-backtick/double-single-quote style in the old documentation is quote odd!
> 
> I'd rather not carry forward that stylistic cruft into this source code (in part because it makes these lines longer than they need to be, and in part bcause I'm guessing it existed purely to make this look prettier when viewed as HTML in web browsers of the 90's).
> 
> Could you do a search-and-replace, to swap out all two-backtick and all two-single-quote patterns with a normal double-quote character? I think we only need that changed for nsBlockReflowState.h and nsLineLayout.h -- those are the only places I noticed it in this patch.

Sure. I'll replace the double-backtick/double-single-quote with normal double-quote character. I've seen this style in some of the old man pages and documentation in Emacs.

> There are a few paragraphs about this flag at http://www-archive.mozilla.org/newlayout/doc/block-and-line.html which seem to be getting lost here. (The "See ShouldApplyBStartMargin" doesn't cover them, because ShouldApplyBStartMargin() doens't seem to include much documentation.)
> 
> Can/should we add a modernized version of the paragraphs from block-and-line.html here?

Nice catch! Will add in my next patch.

> Please add a blank line between the FLOAT_MGR and HAS_LINE_CURSOR bits here.
> 
> (Otherwise, it looks like the FLOAT_MGR header comment is somehow describing all of the subsequent 4 lines, since they're all tucked closely together. Note that other bits / groups-of-bits with documentation here are separated by blank lines.)

Good idea. Addition to your recommation, I'll add blank lines in between the four lines to make them look better.

Comment 4

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

Comment on attachment 8769149 [details]
Bug 1273049 - Migrate documentation in block-and-line.html into the tree.

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

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8769149 [details]
Bug 1273049 - Migrate documentation in block-and-line.html into the tree.

https://reviewboard.mozilla.org/r/63078/#review61038

Thanks! r=me, with one nit on this version's new comment text.

::: layout/generic/nsBlockReflowState.h:44
(Diff revisions 1 - 2)
>  #define BRS_ISBENDMARGINROOT      0x00000004
> -// See ShouldApplyBStartMargin.
> +// BRS_APPLYBSTARTMARGIN is set if the BStart margin should be considered when
> +// placing a linebox that contains a block frame. It may be set as a side-effect
> +// of calling nsBlockFrame::ShouldApplyBStartMargin(); once set,
> +// ShouldApplyBStartMargin() uses it as a fast-path way to return whether the
> +// top margin should apply.

s/top/BStart/

Comment 6

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

https://reviewboard.mozilla.org/r/63078/#review61038

> s/top/BStart/

Fixed. Thank your for the thorough review!

Comment 7

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

Comment on attachment 8769149 [details]
Bug 1273049 - Migrate documentation in block-and-line.html into the tree.

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0858cc40fcde
Migrate documentation in block-and-line.html into the tree. r=dholbert

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0858cc40fcde