775624 - Refactor nsReflowStatus

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[fantasai]

Attachment: bitrotted patch

Refactor nsReflowStatus per
  https://groups.google.com/group/mozilla.dev.tech.layout/browse_thread/thread/ace65a53c7871c9b

(I was sure I filed this bug, but I can't find it now, so maybe I didn't. Oops.)

Comment 1

[fantasai]

Attachment: patch

Comment 2

[fantasai]

So, please take a careful look at the nsListControlFrame bit. I'm not sure that it's correct, it just tries to duplicate what was there. But what was there was throwing a type mismatch, so I'm not sure it makes entirely much sense.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Review of attachment 644499 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/generic/nsIFrame.h
@@ +368,5 @@
> +   * for a frame that is not complete.)
> +   *
> +   * A frame NeedsPush if the part of the frame before the first
> +   * possible break point was unable to fit in the available vertical
> +   * space.  Therefore, the entire frame should be pushed to the next page.

Remove the word "vertical" here I think. And instead of "next page" say something more generic like "next continuation of the parent frame".

@@ +381,5 @@
> +private:
> +  Completion mCompletion      :2;
> +  bool       mReflowNextInFlow:1;
> +  bool       mTruncated       :1;
> +  bool                        :4;

This padding is not needed.

@@ +404,5 @@
> +  { // In a macro-based bitfield, this would be a single operation. :/
> +    mCompletion = static_cast<Completion>(aStatus.mCompletion | mCompletion);
> +    mReflowNextInFlow |= aStatus.mReflowNextInFlow;
> +    mTruncated        |= aStatus.mTruncated;
> +  }

Document these!

@@ +434,5 @@
> +  }
> +  void SetClearAfter(PRUint32 aClearType = NS_STYLE_CLEAR_LINE) {
> +    mClear = aClearType;
> +    mClearIsAfter = true;
> +  }

Document all the clear stuff.

@@ +443,5 @@
> +private:
> +  bool mCompletedFirstLetter:1;
> +public:
> +  bool CompletedFirstLetter()    { return mCompletedFirstLetter;    }
> +  void SetCompletedFirstLetter() { mCompletedFirstLetter = true; }

Document these.

@@ +452,5 @@
> +    mTruncated        = false;
> +    mClear            = NS_STYLE_CLEAR_NONE;
> +    mClearIsAfter     = false;
> +    mCompletedFirstLetter = false;
> +  }

Do we need the separate Init(), or can we just use the constructor? If we only need Init() in rare cases, we could just use status = nsReflowStatus();

@@ +463,5 @@
> +           (mTruncated            << 3 ) +
> +           (mClear                << 8 ) +
> +           (mClearIsAfter         << 16) +
> +           (mCompletedFirstLetter << 17);
> +  }

Instead of Bits() (which is just for debug dumps?) I think it would make more sense to return a string that is somewhat readable.

Comment 4

[Florian Bender]

Except for the review comments (and bitrot), does this patch fix this bug? Fantasai, still working on this?

Comment 5

[fantasai]

Attachment: updated patch

Should do, yeah. It's a fairly straightforward refactoring. Here's the latest I have. I think I was partway through addressing comments on it at some point.

Comment 6

[Florian Bender]

Looks like fantasai will not proceed with this bug (but feel free to retake), so resetting assignee.

Comment 7

[Xidorn Quan [:xidorn] UTC+11]

I think it is still something worth doing.

Comment 8

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

I'll take up the torch. The patch by fantasai is rotten, so I'll start from the scratch.

Comment 9

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

Attachment: Bug 775624 Part 1 - Convert nsReflowStatus to a class mimicking uint32_t.

Lay down the foundation for this refactor work so that nsReflowStatus could
be converted to bit-fields piece by piece, and each patch can be built (but
not testable).

Warnings are spit due to some debug logs printing nsReflowStatus as an
integer, but that will be fix by Part 24 later.

All the operators related to uint32_t will be removed at the end of this
patch series by Part 23.

The yoda conditions are swapped in order to build successfully.

DisplayReflowExit() incorrectly declares aStatus as uint32_t. Change it to
const reference because nsReflowStatus is now a class.

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

Comment 10

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

Attachment: Bug 775624 Part 2 - Add bit-fields and methods for frame completion status.

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

Comment 11

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

Attachment: Bug 775624 Part 3 - Remove NS_FRAME_IS_NOT_COMPLETE.

This patch is written by the help of the following script.

function rename() {
find layout\
     -type f\
     \( -name "*.cpp" -or\
        -name "*.h" \)\
        -exec sed -i -r "s/$1/$2/g" "{}" \;
}

rename "NS_FRAME_IS_NOT_COMPLETE\(([a-zA-Z0-9.*]*)\)" "\1.IsIncomplete()"

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

Comment 12

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

Attachment: Bug 775624 Part 4 - Remove NS_FRAME_IS_FULLY_COMPLETE.

This patch is written by the help of the following script.

function rename() {
find layout\
     -type f\
     \( -name "*.cpp" -or\
        -name "*.h" \)\
        -exec sed -i -r "s/$1/$2/g" "{}" \;
}

rename "NS_FRAME_IS_FULLY_COMPLETE\(([a-zA-Z0-9.*]*)\)" "\1.IsFullyComplete()"

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

Comment 13

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

Attachment: Bug 775624 Part 5 - Remove NS_FRAME_IS_COMPLETE.

This patch is written by the help of the following script.

function rename() {
find layout\
     -type f\
     \( -name "*.cpp" -or\
        -name "*.h" \)\
        -exec sed -i -r "s/$1/$2/g" "{}" \;
}

rename "NS_FRAME_IS_COMPLETE\(([a-zA-Z0-9.*]*)\)" "\1.IsComplete()"

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

Comment 14

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

Attachment: Bug 775624 Part 6 - Remove NS_FRAME_OVERFLOW_IS_INCOMPLETE.

This patch is written by the help of the following script.

function rename() {
find layout\
     -type f\
     \( -name "*.cpp" -or\
        -name "*.h" \)\
        -exec sed -i -r "s/$1/$2/g" "{}" \;
}

rename "NS_FRAME_OVERFLOW_IS_INCOMPLETE\(([a-zA-Z0-9.*]*)\)" "\1.IsOverflowIncomplete()"

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

Comment 15

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

Attachment: Bug 775624 Part 7 - Remove NS_FRAME_SET_INCOMPLETE.

This patch is written by the help of the following script.

function rename() {
find layout\
     -type f\
     \( -name "*.cpp" -or\
        -name "*.h" \)\
        -exec sed -i -r "s/$1/$2/g" "{}" \;
}

rename "NS_FRAME_SET_INCOMPLETE\(\*([a-zA-Z0-9.*]*)\)" "\1->SetIncomplete()"
rename "NS_FRAME_SET_INCOMPLETE\(([a-zA-Z0-9.*]*)\)" "\1.SetIncomplete()"

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

Comment 16

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

Attachment: Bug 775624 Part 8 - Remove NS_FRAME_SET_OVERFLOW_INCOMPLETE.

This patch is written by the help of the following script.

function rename() {
find layout\
     -type f\
     \( -name "*.cpp" -or\
        -name "*.h" \)\
        -exec sed -i -r "s/$1/$2/g" "{}" \;
}

rename "NS_FRAME_SET_OVERFLOW_INCOMPLETE\(\*([a-zA-Z0-9.*]*)\)" "\1->SetOverflowIncomplete()"
rename "NS_FRAME_SET_OVERFLOW_INCOMPLETE\(([a-zA-Z0-9.*]*)\)" "\1.SetOverflowIncomplete()"

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

Comment 17

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

Attachment: Bug 775624 Part 9 - Convert NS_FRAME_REFLOW_NEXTINFLOW to use bit-field and methods.

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

Comment 18

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

Attachment: Bug 775624 Part 10b - Convert NS_FRAME_TRUNCATED to use bit-field and methods.

NS_FRAME_TRUNCATED will be removed when NS_MergeReflowStatusInto() is
removed.

NS_FRAME_SET_TRUNCATION is removed with the help of the following script.

function rename() {
find layout\
     -type f\
     \( -name "*.cpp" -or\
        -name "*.h" \)\
        -exec sed -i -r "s/$1/$2/g" "{}" \;
}

rename "NS_FRAME_SET_TRUNCATION\(([a-zA-Z0-9.*]*),\ ([a-zA-Z0-9.*]*),\ ([a-zA-Z0-9.*]*)\)" "\2.SetTruncated(\3, \&\1)"

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

Comment 19

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

Attachment: Bug 775624 Part 11 - Convert NS_MergeReflowStatusInto() to a method.

Also, remove NS_FRAME_TRUNCATED and NS_FRAME_REFLOW_NEXTINFLOW because both
are used only by NS_MergeReflowStatusInto().

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

Comment 20

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

Attachment: Bug 775624 Part 12 - Add bit-fields for inline break status, and convert NS_INLINE_LINE_BREAK_BEFORE.

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

Comment 21

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

Attachment: Bug 775624 Part 13 - Convert NS_INLINE_LINE_BREAK_AFTER to a method.

NS_INLINE_MAKE_BREAK_TYPE is used only in BRFrame. Delete it, too.

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

Comment 22

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

Attachment: Bug 775624 Part 14 - Convert NS_INLINE_GET_BREAK_TYPE to a method.

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

Comment 23

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

Attachment: Bug 775624 Part 15 - Remove NS_INLINE_BREAK_TYPE_MASK.

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

Comment 24

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

Attachment: Bug 775624 Part 16 - Convert NS_INLINE_IS_BREAK_BEFORE to a method.

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

Comment 25

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

Attachment: Bug 775624 Part 17 - Convert NS_INLINE_IS_BREAK_AFTER to a method.

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

Comment 26

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

Attachment: Bug 775624 Part 18 - Convert NS_INLINE_IS_BREAK to a method.

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

Comment 27

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

Attachment: Bug 775624 Part 19 - Convert NS_INLINE_BREAK_FIRST_LETTER_COMPLETE to use bit-field and methods.

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

Comment 28

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

Attachment: Bug 775624 Part 20 - Remove NS_FRAME_OVERFLOW_INCOMPLETE.

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

Comment 29

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

Attachment: Bug 775624 Part 21 - Remove NS_FRAME_NOT_COMPLETE.

To preserve the semantics, Reset() is called to clear other bits in the
status prior to set the incomplete bit. Though some of them might not be
necessary.

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

Comment 30

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

Attachment: Bug 775624 Part 22 - Remove NS_FRAME_COMPLETE.

This patch is written by the following script with some manual adjustment to
the comment in nsRubyTextContainerFrame.cpp and nsRubyFrame.cpp, and
nsColumnSetFrame's constructor.

function rename() {
find layout\
     -type f\
     \( -name "*.cpp" -or\
        -name "*.h" \)\
        -exec sed -i -r "s/$1/$2/g" "{}" \;
}

rename "nsReflowStatus *([a-zA-Z0-9]*) = NS_FRAME_COMPLETE" "nsReflowStatus \1"
rename "([a-zA-Z0-9.*]*) *= NS_FRAME_COMPLETE;" "\1.Reset();"
rename "([a-zA-Z0-9.*]*) == NS_FRAME_COMPLETE" "\1.IsEmpty()"

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

Comment 31

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

Attachment: Bug 775624 Part 23 - Remove constructor and all operators related to uint32_t.

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

Comment 32

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

Attachment: Bug 775624 Part 24 - Implement operator<< for nsReflowStatus to use mozilla::ToString.

To print log in nsBlockFrame, run "GECKO_BLOCK_DEBUG_FLAGS=reflow ./mach run".

Here is a sample nsReflowStatus string:
[Complete=Y,NIF=N,Truncated=N,Break=N,FirstLetter=Y].

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

Comment 33

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8cb3cc6a46bc3bf6a03f21b6054acf96b61f19b

Comment 34

[Daniel Holbert [:dholbert]]

Comment on attachment 8837505 [details]
Bug 775624 Part 1 - Convert nsReflowStatus to a class mimicking uint32_t.

https://reviewboard.mozilla.org/r/112618/#review114346

r=me on part 1, with a few extended-commit-message tweaks, for clarity:

> [...] and each patch can be built (but
> not testable).

s/not testable/but may not pass tests/

> Warnings are spit due to some debug logs printing nsReflowStatus as an
> integer, but that will be fix by Part 24 later.

s/Warnings are spit/This change causes some build warnings,/
s/will be fix/will be fixed/

Comment 35

[Daniel Holbert [:dholbert]]

Comment on attachment 8837506 [details]
Bug 775624 Part 2 - Add bit-fields and methods for frame completion status.

https://reviewboard.mozilla.org/r/112620/#review114348

r=me on part 2

Comment 36

[Daniel Holbert [:dholbert]]

Comment on attachment 8837507 [details]
Bug 775624 Part 3 - Remove NS_FRAME_IS_NOT_COMPLETE.

https://reviewboard.mozilla.org/r/112622/#review114638

r=me

Comment 37

[Daniel Holbert [:dholbert]]

Comment on attachment 8837508 [details]
Bug 775624 Part 4 - Remove NS_FRAME_IS_FULLY_COMPLETE.

https://reviewboard.mozilla.org/r/112624/#review114676

r=me

Comment 38

[Daniel Holbert [:dholbert]]

Comment on attachment 8837509 [details]
Bug 775624 Part 5 - Remove NS_FRAME_IS_COMPLETE.

https://reviewboard.mozilla.org/r/112626/#review115048

r=me

Comment 39

[Daniel Holbert [:dholbert]]

Comment on attachment 8837510 [details]
Bug 775624 Part 6 - Remove NS_FRAME_OVERFLOW_IS_INCOMPLETE.

https://reviewboard.mozilla.org/r/112628/#review115054

r=me

Comment 40

[Daniel Holbert [:dholbert]]

Comment on attachment 8837511 [details]
Bug 775624 Part 7 - Remove NS_FRAME_SET_INCOMPLETE.

https://reviewboard.mozilla.org/r/112630/#review115062

Ship It!

Comment 41

[Daniel Holbert [:dholbert]]

(Drat -- I tried using the "Ship it" button in comment 7, for a fewer-clicks r+ experience.  [The button is hidden by default, but it's there in the DOM & can be shown with a Stylish CSS rule]. But apparently that button doesn't grant r+ anymore. I'll file a MozReview bug on that.)

Comment 42

[Daniel Holbert [:dholbert]]

Comment on attachment 8837511 [details]
Bug 775624 Part 7 - Remove NS_FRAME_SET_INCOMPLETE.

https://reviewboard.mozilla.org/r/112630/#review115068

Comment 43

[Daniel Holbert [:dholbert]]

Comment on attachment 8837512 [details]
Bug 775624 Part 8 - Remove NS_FRAME_SET_OVERFLOW_INCOMPLETE.

https://reviewboard.mozilla.org/r/112632/#review115080

r=me

Comment 44

[Daniel Holbert [:dholbert]]

Comment on attachment 8837513 [details]
Bug 775624 Part 9 - Convert NS_FRAME_REFLOW_NEXTINFLOW to use bit-field and methods.

https://reviewboard.mozilla.org/r/112634/#review115082

r=me; one followup request, though:

::: layout/generic/nsIFrame.h:282
(Diff revision 1)
> +  // mNextInFlowNeedsReflow bit flag means that the next-in-flow is dirty,
> +  // and also needs to be reflowed. This status only makes sense for a frame
> +  // that is not complete, i.e. you wouldn't set mNextInFlowNeedsReflow when
> +  // IsComplete() is true.
> +  bool NextInFlowNeedsReflow() const { return mNextInFlowNeedsReflow; }
> +  void SetNextInFlowNeedsReflow() { mNextInFlowNeedsReflow = true; }

Observation: we should probably add assertions to enforce the various comments here about "This only makes sense if..", "You wouldn't do...". (This is much easier now that we have setters for the state bits, and the state is only adjusted via those setters.)

Could you file a followup to add assertions for these sorts of things?  (It shouldn't happen as part of this bug, since this is trying to be a drop-in replacement without any behavior changes.  And I wouldn't be too surprised if it turns out we already can be made to violate some of these hypothetical assertions...)

Comment 45

[Daniel Holbert [:dholbert]]

Comment on attachment 8837514 [details]
Bug 775624 Part 10b - Convert NS_FRAME_TRUNCATED to use bit-field and methods.

https://reviewboard.mozilla.org/r/112636/#review115094

r- on this one, for the moment:

::: layout/generic/nsImageFrame.cpp:1054
(Diff revision 1)
>    }
>  
>    NS_FRAME_TRACE(NS_FRAME_TRACE_CALLS,
>                    ("exit nsImageFrame::Reflow: size=%d,%d",
>                    aMetrics.Width(), aMetrics.Height()));
> -  NS_FRAME_SET_TRUNCATION(aStatus, aReflowInput, aMetrics);
> +  aReflowInput.SetTruncated(aMetrics, &aStatus);

So this patch currently creates a new ReflowStatus method called "SetTruncated", which is called by the (preexisting) ReflowInput::SetTruncated method.

I'm uneasy with these two methods having the same name, because their signatures look so similar, and yet they do very different things.  So, it's likely to cause confusion.  (The ReflowStatus method unconditionally sets the truncation bit. Whereas, the ReflowInput method might set or clear the bit.) So, who is more deserving of this "SetTruncated" name? IMO it makes more sense on the ReflowStatus than on the ReflowInput. The existing ReflowInput method really should be named something like "UpdateTruncatedStatus" or "CheckForTruncation".

Unfortunately, the un-deserving method already exists. But fortunately, it only has one caller (in current mozilla-central without your mass-NS_FRAME_SET_TRUNCATION replacement), so renaming it will be easy.

SO: Could you please do the following:
 (1) revert the s/NS_FRAME_SET_TRUNCATION/aReflowInput.SetTruncated/ changes in this patch
 (2) Add a separate patch (separate bug if you like) to rename ReflowInput::SetTruncated to something better (maybe UpdateTruncatedStatus or CheckForTruncation)?  Ideally it'd be good for that change to happen before we create ReflowStatus::SetTruncated, so that the tree is never in a state of ambiguity (with the clashing method names).
 (3) Also in a separate patch (or a separate bug even), you could do the mass conversion from NS_FRAME_SET_TRUNCATION to the newly-renamed method (UpdateTruncatedStatus or whatever)  Though, we could also just keep using the macro, too -- this part isn't as pressing.

Comment 46

[Daniel Holbert [:dholbert]]

Comment on attachment 8837515 [details]
Bug 775624 Part 11 - Convert NS_MergeReflowStatusInto() to a method.

https://reviewboard.mozilla.org/r/112638/#review115102

r=me

Comment 47

[Daniel Holbert [:dholbert]]

Comment on attachment 8837516 [details]
Bug 775624 Part 12 - Add bit-fields for inline break status, and convert NS_INLINE_LINE_BREAK_BEFORE.

https://reviewboard.mozilla.org/r/112640/#review115106

r- for now, per nit below:

::: layout/generic/nsIFrame.h:326
(Diff revision 1)
> +  // Construct a break-before status, which defaults to line-break-before.
> +  // Note that the frame completion status will be reset because we *know*
> +  // that the frame will be reflowed later and hence its current completion
> +  // status doesn't matter.
> +  void SetBreakBefore(StyleClear aBreakType = StyleClear::Line) {

The word "Construct" in the comment isn't correct here, in terms of describing what this method does in your current patch.

BUT, I think we should perhaps adjust the code to make the method correct (and to preserve the way the equivalent macro has been invoked) -- i.e. we should make this method actually *create and return* a nsReflowStatus (and call the method something like InlineBreakBeforeStatus()).  And callers should invoke it like so:
 aStatus = InlineBreakBeforeStatus();
...instead of doing this (from the current patch):
 aStatus.SetBreakBefore();

That would make it clearer at callsites that we're stomping on all the other bits.  (Right now "SetBreakBefore" sounds like it's just a setter for a single bit.)  I suspect the current calling pattern (aStatus = NS_INLINE_LINE_BREAK_BEFORE()) is designed with this in mind, to make it absolutely clear that it's stomping on all of the bits.  And we should preserve that clarity at the callsites in this refactoring, IMO.

What do you think?

Comment 48

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #47)
> And callers should invoke it like so:
>  aStatus = InlineBreakBeforeStatus();

...er, sorry, I should've said:
 aStatus = nsReflowStatus::InlineBreakBeforeStatus();

(I'm imagining that it'd be a static function on the nsReflowStatus class.)

And since it'd be using nsReflowStatus:: in each invocation, it probably doesn't need "Status" in the function name after all, either -- maybe we could just call it "nsReflowStatus::InlineLineBreakBefore()" (directly renamed from NS_INLINE_LINE_BREAK_BEFORE)

Comment 49

[Daniel Holbert [:dholbert]]

Comment on attachment 8837517 [details]
Bug 775624 Part 13 - Convert NS_INLINE_LINE_BREAK_AFTER to a method.

https://reviewboard.mozilla.org/r/112642/#review115112

r- on part 13 for similar reasons to part 12 (and since there should probably be symmetry with whatever API we settle on for part 12)

::: layout/generic/nsIFrame.h:337
(Diff revision 1)
> +  // Construct a break-after status, which defaults to line-break-after.
> +  void SetBreakAfter(StyleClear aBreakType = StyleClear::Line) {
> +    mBreak = true;

Comment 47 applies here, too -- "Construct" isn't correct in the current patch, but we should perhaps adjust the code to make it correct, and make this e.g.
 nsReflowStatus::InlineLineBreakAfter(aBreakType, aStatusToClone)

(...with the aStatusToClone arg defaulting to a default-constructed nsReflowStatus, since that's nearly always what you want)

Comment 50

[Daniel Holbert [:dholbert]]

Comment on attachment 8837518 [details]
Bug 775624 Part 14 - Convert NS_INLINE_GET_BREAK_TYPE to a method.

https://reviewboard.mozilla.org/r/112644/#review115116

r=me

::: layout/generic/nsIFrame.h:320
(Diff revision 1)
>    // mBreak bit flag means a break is requested.
>  
>    // Suppose a break is requested. When mBreakAfter is set, the break should
>    // occur after the frame just reflowed; when mBreakAfter is clear, the
>    // break should occur before the frame just reflowed.
> +  StyleClear BreakType() const { return mBreakType; }

Observation: In isolation, this part of the patch doesn't make sense, because it's adding a method directly-adjacent to some unrelated documentation (the documentation is about *different* member-vars)

But looking ahead, I see that parts 17 and 18 insert some more getters before this one, and those getters *are* tied to this documentation. So this (eventually) makes sense. :)  So, no changes needed here.

Comment 51

[Daniel Holbert [:dholbert]]

Comment on attachment 8837519 [details]
Bug 775624 Part 15 - Remove NS_INLINE_BREAK_TYPE_MASK.

https://reviewboard.mozilla.org/r/112646/#review115118

r- on this part for now, since it uses the API from part 13 which will maybe be changing now. (per comment 49)

::: layout/generic/nsInlineFrame.cpp:812
(Diff revision 1)
> -      aStatus = NS_FRAME_NOT_COMPLETE |
> -        NS_INLINE_BREAK | NS_INLINE_BREAK_AFTER |
> -        (aStatus & NS_INLINE_BREAK_TYPE_MASK);
> +      StyleClear oldBreakType = aStatus.BreakType();
> +      aStatus.Reset();
> +      aStatus.SetIncomplete();
> +      aStatus.SetBreakAfter(oldBreakType);

So after my suggested tweak to comment 13, I think this would look something like:

  StyleClear oldBreakType = aStatus.BreakType();
  nsReflowStatus incompleteStatus;
  incompleteStatus.SetIncomplete();

  aStatus = nsReflowStatus::InlineLineBreakAfter(oldBreakType,
                                                 incompleteStatus);

Comment 52

[Daniel Holbert [:dholbert]]

Comment on attachment 8837520 [details]
Bug 775624 Part 16 - Convert NS_INLINE_IS_BREAK_BEFORE to a method.

https://reviewboard.mozilla.org/r/112648/#review115120

Sorry, I'm a little concerned about the naming here. r- for now, see below:

::: layout/forms/nsComboboxControlFrame.cpp:900
(Diff revision 1)
>    buttonRect.BSize(wm) = mDisplayFrame->BSize(wm) +
>                           this->GetLogicalUsedPadding(wm).BStartEnd(wm);
>  
>    mButtonFrame->SetRect(buttonRect, containerSize);
>  
> -  if (!NS_INLINE_IS_BREAK_BEFORE(aStatus) &&
> +  if (!aStatus.IsBreakBefore() &&

I wonder if we should really call this "IsInlineBreakBefore"?  (preserving "INLINE" from the macro that you're replacing)  Since I'm pretty sure this is an inline-layout-only thing, according to its old labeling at least.

Because, the patch's current name "IsBreakBefore()" is quite easy to misinterpret this as having something to do with the "break-before" CSS property:
  https://developer.mozilla.org/en-US/docs/Web/CSS/break-before
  https://developer.mozilla.org/en-US/docs/Web/CSS/break-after
And I'm pretty sure the method does not have anything to do with that CSS property.

(I guess this goes for the naming of the mBreak/mBreakAfter status-bits, too... Those might want to be mInlineBreak/mInlineBreakAfter, back in part 12 where they're added...?)

::: layout/generic/BlockReflowInput.cpp:922
(Diff revision 1)
>    // Likewise, if none of the float fit, and it needs to be pushed in
>    // its entirety to the next page (NS_FRAME_IS_TRUNCATED or
> -  // NS_INLINE_IS_BREAK_BEFORE), we need to do the same.
> +  // IsBreakBefore()), we need to do the same.

Nit: this comment that you're modifying here has a straggling mention of NS_FRAME_IS_TRUNCATED, which is a macro that you removed in part 10.

Please update that to say "IsTruncated()".

(Ideally that'd happen in part 10, where the macro itself is removed, for maximal-atomicity-and-cleanliness -- but you could also just remove it as part of this patch (part 16) to avoid bitrotting yourself.)

Comment 53

[Daniel Holbert [:dholbert]]

Comment on attachment 8837521 [details]
Bug 775624 Part 17 - Convert NS_INLINE_IS_BREAK_AFTER to a method.

https://reviewboard.mozilla.org/r/112650/#review115130

::: layout/generic/nsBlockFrame.cpp:4237
(Diff revision 1)
>        aLine->SetBreakTypeAfter(breakType);
>        if (frameReflowStatus.IsComplete()) {
>          // Split line, but after the frame just reflowed
>          SplitLine(aState, aLineLayout, aLine, aFrame->GetNextSibling(), aLineReflowStatus);
>  
> -        if (NS_INLINE_IS_BREAK_AFTER(frameReflowStatus) &&
> +        if (frameReflowStatus.IsBreakAfter() &&

As in part 16, we should probably keep "Inline" as part of the name here -- so this should be IsInlineBreakAfter() throughout this patch, I think...

Comment 54

[Daniel Holbert [:dholbert]]

Comment on attachment 8837522 [details]
Bug 775624 Part 18 - Convert NS_INLINE_IS_BREAK to a method.

https://reviewboard.mozilla.org/r/112652/#review115132

::: layout/generic/nsBlockFrame.cpp:262
(Diff revision 1)
>    int index = 0;
>    if (!aChildIsBlock) index |= 1;
>  
>    // Compute new status
>    uint32_t newS = record[index];
> -  if (NS_INLINE_IS_BREAK(aFrameReflowStatus)) {
> +  if (aFrameReflowStatus.IsBreak()) {

And this should perhaps be IsInlineBreak(), to keep the naming consistent with the (hypothetically-renamed) IsInlineBreakBefore/IsInlineBreakAfter methods.

Comment 55

[Daniel Holbert [:dholbert]]

Comment on attachment 8837516 [details]
Bug 775624 Part 12 - Add bit-fields for inline break status, and convert NS_INLINE_LINE_BREAK_BEFORE.

https://reviewboard.mozilla.org/r/112640/#review115134

::: layout/generic/nsIFrame.h:346
(Diff revision 1)
> +  // Inline break status bit flags.
> +  bool mBreak : 1;
> +  bool mBreakAfter : 1;

Late-breaking nit on this part:

These might really want to be named mInlineBreak/mInlineBreakAfter, per comment 52... 

(for consistency with their accessors which will probably need "Inline" in their names to avoid confusion with the "break-before"/"break-after" CSS properties)

::: layout/generic/nsIFrame.h:348
(Diff revision 1)
> +  bool mBreakAfter : 1;
> +  StyleClear mBreakType;

Another late-breaking nit for this part:

"StyleClear mBreakType" should be declared *before* all the bools, not after them.  This makes the bit packing more likely to be optimal amongst the bools (particularly when you add more bools at the end like in part 19).

(You'll need to update the constructor init list as well, to keep the ordering consistent, or else compilers will warn.)

Comment 56

[Daniel Holbert [:dholbert]]

Comment on attachment 8837523 [details]
Bug 775624 Part 19 - Convert NS_INLINE_BREAK_FIRST_LETTER_COMPLETE to use bit-field and methods.

https://reviewboard.mozilla.org/r/112654/#review115138

r=me

(Warning: this part will be bitrotted slightly by the contextual changes to part 12 that I suggested in comment 55)

Comment 57

[Daniel Holbert [:dholbert]]

Comment on attachment 8837524 [details]
Bug 775624 Part 20 - Remove NS_FRAME_OVERFLOW_INCOMPLETE.

https://reviewboard.mozilla.org/r/112656/#review115140

r=me; one wording nit for the comment change:

::: layout/generic/nsContainerFrame.h:329
(Diff revision 1)
>     * ReflowOverflowContainerChildren). The parent continues reflow as if
>     * the frame was complete once it ran out of computed height, but returns
> -   * either an NS_FRAME_NOT_COMPLETE or NS_FRAME_OVERFLOW_INCOMPLETE reflow
> +   * either the mIncomplete or mOverflowIncomplete bit set in reflow
>     * status to request a next-in-flow. The parent's next-in-flow is then

s/either/with either/

Comment 58

[Daniel Holbert [:dholbert]]

Comment on attachment 8837525 [details]
Bug 775624 Part 21 - Remove NS_FRAME_NOT_COMPLETE.

https://reviewboard.mozilla.org/r/112658/#review115142

r=me, with followup filed as requested below:

::: layout/generic/nsImageFrame.cpp:1014
(Diff revision 1)
>      aMetrics.Height() = std::max(nsPresContext::CSSPixelsToAppUnits(1), aReflowInput.AvailableHeight());
> -    aStatus = NS_FRAME_NOT_COMPLETE;
> +    aStatus.Reset();
> +    aStatus.SetIncomplete();

Observation: In this case I don't think we need to bother calling aStatus.Reset().  No flags have been set on aStatus yet at this point, so there's no need to Reset it.

Having said that -- I understand you're adding Reset() here to preserve the semantics of the "aStatus = [whatever flag]" assignment.  (You note this in the extended commit message.) That makes sense from the standpoint of this bug being an in-place largely-mechanical refactoring, I think.  But I'm worried that these extra Reset() calls will cause confusion going forward, from people wondering why they're present and what they could possibly be resetting.

SO, my request: could you file a followup on triaging each of the "Reset()" calls added in this patch and removing the ones that are discovered to be likely/definitely-unnecessary? (That'll be a semantic change but one that should have no behavioral effect.)

::: layout/tables/nsTableRowGroupFrame.cpp:1399
(Diff revision 1)
>  
>    // See if all the frames fit. Do not try to split anything if we're
>    // not paginated ... we can't split across columns yet.
>    if (aReflowInput.mFlags.mTableIsSplittable &&
>        NS_UNCONSTRAINEDSIZE != aReflowInput.AvailableHeight() &&
> -      (aStatus == NS_FRAME_NOT_COMPLETE || splitDueToPageBreak ||
> +      (aStatus.IsIncomplete() || splitDueToPageBreak ||

Observation: this is a semantic change (the old code was checking if aStatus had *exactly* this one bit set, whereas the new code is more permissive and doesn't care whether or not other bits are set).

I suspect your replacement code is what was originally intended, though, so this seems fine.

Comment 59

[Daniel Holbert [:dholbert]]

Comment on attachment 8837526 [details]
Bug 775624 Part 22 - Remove NS_FRAME_COMPLETE.

https://reviewboard.mozilla.org/r/112660/#review115144

r=me with nits addressed.

::: layout/forms/nsFieldSetFrame.cpp:341
(Diff revision 1)
>    // Initialize OUT parameter
> -  aStatus = NS_FRAME_COMPLETE;
> +  aStatus.Reset();

I suspect many of the changes like this are unnecessary/redundant.

As noted by the code-comment here, the current code thinks that "aStatus" is uninitialized & that it has to take action to initialize it (because until now, it's just a raw bitfield allocated in the caller with no value assigned yet).

But with your patches, it's a class with a constructor that initializes everything to 0 already. So there's no need to Reset() it here -- we'll be reinitializing it redundantly, basically.

Having said that -- as noted for part 21, I'm OK with having some extra/unnecessary Reset() calls in the interests of letting this be a mechanical in-place change, as long as we triage them afterwards and try to get rid of the ones that can be easily determined to be unnecessary.  So: please file a followup on that (or include it in the scope of the followup that I requested for part 21).

(In that followup, in some cases it might be worth replacing the Reset() call with an IsEmpty() call or an IsFullyComplete() call, just to make the intent clear and to sanity-check that we really don't need to adjust the state.)

::: layout/generic/nsIFrame.h:264
(Diff revision 1)
> +  // Return true if all flags are cleared.
> +  bool IsEmpty() const {
> +    return (mIncomplete == false &&
> +            mOverflowIncomplete == false &&
> +            mNextInFlowNeedsReflow == false &&
> +            mTruncated == false &&

This would better-match our coding conventions and would be a bit easier to read (IMO) if you converted these "foo == false" checks to just "!foo" checks.

Then this would look like:
 return (!mFoo &&
         !mBar && 
         !mBlahBlah)

.etc., which is shorter & more nicely lined up & hence easier to read.)

::: layout/generic/nsIFrame.h:272
(Diff revision 1)
> +            mBreakType == StyleClear::None &&
> +            mFirstLetterComplete == false);

I don't think it makes sense to check mBreakType here, because:
 - It's not a flag (and this method's documentation talks about "all flags").
 - Its value is meaningless/unused, unless mBreak is set, I think.

Not a big deal, though.  If you do want to keep it here, though, please move it up to the top of this list to keep the order here in sync with the variable declaration order, per end of comment 55.

Comment 60

[Daniel Holbert [:dholbert]]

Comment on attachment 8837527 [details]
Bug 775624 Part 23 - Remove constructor and all operators related to uint32_t.

https://reviewboard.mozilla.org/r/112662/#review115146

r=me

Comment 61

[Daniel Holbert [:dholbert]]

Comment on attachment 8837528 [details]
Bug 775624 Part 24 - Implement operator<< for nsReflowStatus to use mozilla::ToString.

https://reviewboard.mozilla.org/r/112664/#review115192

r=me, just 2 minor nits:

::: layout/generic/nsFrame.cpp:194
(Diff revision 1)
> +std::ostream& operator<<(std::ostream& aStream,
> +                         const nsReflowStatus& aStatus)

Add linebreak between return type and function name,  please (before "operator")

::: layout/generic/nsIFrame.h:344
(Diff revision 1)
>    StyleClear mBreakType;
>    bool mFirstLetterComplete : 1;
>  };
>  
> +#ifdef DEBUG
> +// Convert nsReflowStatus to a human readable string.

Add hyphen please:
 s/human readable/human-readable/

(We have both spellings in the codebase, but the latter is more prevalent & more correct grammatically)

Comment 62

[Daniel Holbert [:dholbert]]

Comment on attachment 8837525 [details]
Bug 775624 Part 21 - Remove NS_FRAME_NOT_COMPLETE.

https://reviewboard.mozilla.org/r/112658/#review115194

Comment 63

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

Comment on attachment 8837513 [details]
Bug 775624 Part 9 - Convert NS_FRAME_REFLOW_NEXTINFLOW to use bit-field and methods.

https://reviewboard.mozilla.org/r/112634/#review115082

> Observation: we should probably add assertions to enforce the various comments here about "This only makes sense if..", "You wouldn't do...". (This is much easier now that we have setters for the state bits, and the state is only adjusted via those setters.)
> 
> Could you file a followup to add assertions for these sorts of things?  (It shouldn't happen as part of this bug, since this is trying to be a drop-in replacement without any behavior changes.  And I wouldn't be too surprised if it turns out we already can be made to violate some of these hypothetical assertions...)

Filed bug 1340966.

Comment 64

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

Comment on attachment 8837526 [details]
Bug 775624 Part 22 - Remove NS_FRAME_COMPLETE.

https://reviewboard.mozilla.org/r/112660/#review115144

> I suspect many of the changes like this are unnecessary/redundant.
> 
> As noted by the code-comment here, the current code thinks that "aStatus" is uninitialized & that it has to take action to initialize it (because until now, it's just a raw bitfield allocated in the caller with no value assigned yet).
> 
> But with your patches, it's a class with a constructor that initializes everything to 0 already. So there's no need to Reset() it here -- we'll be reinitializing it redundantly, basically.
> 
> Having said that -- as noted for part 21, I'm OK with having some extra/unnecessary Reset() calls in the interests of letting this be a mechanical in-place change, as long as we triage them afterwards and try to get rid of the ones that can be easily determined to be unnecessary.  So: please file a followup on that (or include it in the scope of the followup that I requested for part 21).
> 
> (In that followup, in some cases it might be worth replacing the Reset() call with an IsEmpty() call or an IsFullyComplete() call, just to make the intent clear and to sanity-check that we really don't need to adjust the state.)

Filed bug 1341009.

> I don't think it makes sense to check mBreakType here, because:
>  - It's not a flag (and this method's documentation talks about "all flags").
>  - Its value is meaningless/unused, unless mBreak is set, I think.
> 
> Not a big deal, though.  If you do want to keep it here, though, please move it up to the top of this list to keep the order here in sync with the variable declaration order, per end of comment 55.

You're right. mBreakType is meaningless unless mInlineBreak is set, and it can only be set via InlineLineBreakBefore() or InlineLineBreakAfter().

Comment 65

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

Attachment: Bug 775624 Part 10a - Move ReflowInput::SetTruncated() into nsReflowStatus.

Rename the method to UpdateTruncatedStatus() to avoid confusion with the
nsReflowStatus::SetTruncated() added in Part 10b.

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

Comment 66

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

Attachment: Bug 775624 Part 10c - Remove NS_FRAME_SET_TRUNCATION.

NS_FRAME_SET_TRUNCATION is removed with the help of the following script.
However, there are two places which do not match the regex, and require
manual fix.

1) nsBlockFrame::Reflow() due to reflowInput is a pointer type.
2) nsFlexContainerFrame::DoFlexLayout() due to lacking of ';' at the end of
   the line.

function rename() {
find layout\
     -type f\
     \( -name "*.cpp" -or\
        -name "*.h" \)\
        -exec sed -i -r "s/$1/$2/g" "{}" \;
}

rename "NS_FRAME_SET_TRUNCATION\(([a-zA-Z0-9.*]*),\ ([a-zA-Z0-9.*]*),\ ([a-zA-Z0-9.*]*)\)" "\2.UpdateTruncatedStatus(\3, \&\1)"

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

Comment 67

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

Comment on attachment 8837505 [details]
Bug 775624 Part 1 - Convert nsReflowStatus to a class mimicking uint32_t.

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

Comment 68

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

Comment on attachment 8837506 [details]
Bug 775624 Part 2 - Add bit-fields and methods for frame completion status.

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

Comment 69

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

Comment on attachment 8837507 [details]
Bug 775624 Part 3 - Remove NS_FRAME_IS_NOT_COMPLETE.

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

Comment 70

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

Comment on attachment 8837508 [details]
Bug 775624 Part 4 - Remove NS_FRAME_IS_FULLY_COMPLETE.

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

Comment 71

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

Comment on attachment 8837509 [details]
Bug 775624 Part 5 - Remove NS_FRAME_IS_COMPLETE.

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

Comment 72

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

Comment on attachment 8837510 [details]
Bug 775624 Part 6 - Remove NS_FRAME_OVERFLOW_IS_INCOMPLETE.

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

Comment 73

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

Comment on attachment 8837511 [details]
Bug 775624 Part 7 - Remove NS_FRAME_SET_INCOMPLETE.

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

Comment 74

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

Comment on attachment 8837512 [details]
Bug 775624 Part 8 - Remove NS_FRAME_SET_OVERFLOW_INCOMPLETE.

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

Comment 75

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

Comment on attachment 8837513 [details]
Bug 775624 Part 9 - Convert NS_FRAME_REFLOW_NEXTINFLOW to use bit-field and methods.

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

Comment 76

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

Comment on attachment 8837514 [details]
Bug 775624 Part 10b - Convert NS_FRAME_TRUNCATED to use bit-field and methods.

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

Comment 77

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

Comment on attachment 8837515 [details]
Bug 775624 Part 11 - Convert NS_MergeReflowStatusInto() to a method.

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

Comment 78

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

Comment on attachment 8837516 [details]
Bug 775624 Part 12 - Add bit-fields for inline break status, and convert NS_INLINE_LINE_BREAK_BEFORE.

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

Comment 79

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

Comment on attachment 8837517 [details]
Bug 775624 Part 13 - Convert NS_INLINE_LINE_BREAK_AFTER to a method.

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

Comment 80

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

Comment on attachment 8837518 [details]
Bug 775624 Part 14 - Convert NS_INLINE_GET_BREAK_TYPE to a method.

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

Comment 81

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

Comment on attachment 8837519 [details]
Bug 775624 Part 15 - Remove NS_INLINE_BREAK_TYPE_MASK.

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

Comment 82

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

Comment on attachment 8837520 [details]
Bug 775624 Part 16 - Convert NS_INLINE_IS_BREAK_BEFORE to a method.

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

Comment 83

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

Comment on attachment 8837521 [details]
Bug 775624 Part 17 - Convert NS_INLINE_IS_BREAK_AFTER to a method.

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

Comment 84

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

Comment on attachment 8837522 [details]
Bug 775624 Part 18 - Convert NS_INLINE_IS_BREAK to a method.

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

Comment 85

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

Comment on attachment 8837523 [details]
Bug 775624 Part 19 - Convert NS_INLINE_BREAK_FIRST_LETTER_COMPLETE to use bit-field and methods.

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

Comment 86

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

Comment on attachment 8837524 [details]
Bug 775624 Part 20 - Remove NS_FRAME_OVERFLOW_INCOMPLETE.

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

Comment 87

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

Comment on attachment 8837525 [details]
Bug 775624 Part 21 - Remove NS_FRAME_NOT_COMPLETE.

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

Comment 88

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

Comment on attachment 8837526 [details]
Bug 775624 Part 22 - Remove NS_FRAME_COMPLETE.

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

Comment 89

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

Comment on attachment 8837527 [details]
Bug 775624 Part 23 - Remove constructor and all operators related to uint32_t.

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

Comment 90

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

Comment on attachment 8837528 [details]
Bug 775624 Part 24 - Implement operator<< for nsReflowStatus to use mozilla::ToString.

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

Comment 91

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

Comment on attachment 8837516 [details]
Bug 775624 Part 12 - Add bit-fields for inline break status, and convert NS_INLINE_LINE_BREAK_BEFORE.

https://reviewboard.mozilla.org/r/112640/#review115106

> The word "Construct" in the comment isn't correct here, in terms of describing what this method does in your current patch.
> 
> BUT, I think we should perhaps adjust the code to make the method correct (and to preserve the way the equivalent macro has been invoked) -- i.e. we should make this method actually *create and return* a nsReflowStatus (and call the method something like InlineBreakBeforeStatus()).  And callers should invoke it like so:
>  aStatus = InlineBreakBeforeStatus();
> ...instead of doing this (from the current patch):
>  aStatus.SetBreakBefore();
> 
> That would make it clearer at callsites that we're stomping on all the other bits.  (Right now "SetBreakBefore" sounds like it's just a setter for a single bit.)  I suspect the current calling pattern (aStatus = NS_INLINE_LINE_BREAK_BEFORE()) is designed with this in mind, to make it absolutely clear that it's stomping on all of the bits.  And we should preserve that clarity at the callsites in this refactoring, IMO.
> 
> What do you think?

Indeed, the Reset() in the SetBreakBefore is obsecure. People might not aware it's reset all of the bits. I've converted the macro to what you've suggested.

Comment 92

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

Comment on attachment 8837514 [details]
Bug 775624 Part 10b - Convert NS_FRAME_TRUNCATED to use bit-field and methods.

https://reviewboard.mozilla.org/r/112636/#review115094

> So this patch currently creates a new ReflowStatus method called "SetTruncated", which is called by the (preexisting) ReflowInput::SetTruncated method.
> 
> I'm uneasy with these two methods having the same name, because their signatures look so similar, and yet they do very different things.  So, it's likely to cause confusion.  (The ReflowStatus method unconditionally sets the truncation bit. Whereas, the ReflowInput method might set or clear the bit.) So, who is more deserving of this "SetTruncated" name? IMO it makes more sense on the ReflowStatus than on the ReflowInput. The existing ReflowInput method really should be named something like "UpdateTruncatedStatus" or "CheckForTruncation".
> 
> Unfortunately, the un-deserving method already exists. But fortunately, it only has one caller (in current mozilla-central without your mass-NS_FRAME_SET_TRUNCATION replacement), so renaming it will be easy.
> 
> SO: Could you please do the following:
>  (1) revert the s/NS_FRAME_SET_TRUNCATION/aReflowInput.SetTruncated/ changes in this patch
>  (2) Add a separate patch (separate bug if you like) to rename ReflowInput::SetTruncated to something better (maybe UpdateTruncatedStatus or CheckForTruncation)?  Ideally it'd be good for that change to happen before we create ReflowStatus::SetTruncated, so that the tree is never in a state of ambiguity (with the clashing method names).
>  (3) Also in a separate patch (or a separate bug even), you could do the mass conversion from NS_FRAME_SET_TRUNCATION to the newly-renamed method (UpdateTruncatedStatus or whatever)  Though, we could also just keep using the macro, too -- this part isn't as pressing.

The comment (1)(2)(3) were addressed in Part 10a, 10b, 10c, respectively.

Comment 93

[Daniel Holbert [:dholbert]]

Comment on attachment 8839117 [details]
Bug 775624 Part 10a - Move ReflowInput::SetTruncated() into nsReflowStatus.

https://reviewboard.mozilla.org/r/113854/#review115634

r=me

Comment 94

[Daniel Holbert [:dholbert]]

Comment on attachment 8837514 [details]
Bug 775624 Part 10b - Convert NS_FRAME_TRUNCATED to use bit-field and methods.

https://reviewboard.mozilla.org/r/112636/#review115636

r=me

Comment 95

[Daniel Holbert [:dholbert]]

Hmm. Actually, looking at the new way this will look in Part 10c, I think this needs one further tweak (which means 10a needs adjusting I think).  Sorry to go back and forth on this :-/  The ideal conversion wasn't obvious at first -- and since this is touching so much code, it'd be nice to jump straight to the optimal formulation of this rather than traversing an intermediate suboptimal formulation.

So right now as of 10c, every reflow method ends with a line like this:
>   aReflowInput.UpdateTruncatedStatus(aMetrics, &aStatus);

This makes it *look* like this is an operation that happens on the reflow input (which might modify aStatus as well). BUT REALLY, this is *purely* an operation on aStatus, which just reads some (public) state from the reflowinput.  So I'd really like this to end up looking like this:
  aStatus.UpdateTruncatedStatus(aReflowInput, aMetrics);
That makes it clearer that we're updating some state in the ReflowStatus, based on (public) information from the ReflowInput and the ReflowOutput.

Previously when aStatus was just an integer type, it made sense to put this modifier on ReflowInput (because there wasn't any better place for it, because aStatus couldn't have methods of its own).  But now that ReflowStatus is a class unto itself, we should just morph the old ReflowInput::SetTrunctated method into new home within ReflowStatus!  (say, ReflowStatus::UpdateTruncatedStatus)

Then, we won't need public ClearTruncated/SetTruncated setters on the ReflowStatus class.

What do you think?  (This will mean 10a and 10b need a bit of tweaking, and 10c will need a slightly different automated rewrite.)

Comment 96

[Daniel Holbert [:dholbert]]

Comment on attachment 8839117 [details]
Bug 775624 Part 10a - Move ReflowInput::SetTruncated() into nsReflowStatus.

https://reviewboard.mozilla.org/r/113854/#review115642

I'm updating 10a and 10b to r-, so that you'll be able to re-request review on the updated patches after addressing comment 95 (assuming you agree with my thoughts there).

Comment 97

[Daniel Holbert [:dholbert]]

Comment on attachment 8837514 [details]
Bug 775624 Part 10b - Convert NS_FRAME_TRUNCATED to use bit-field and methods.

https://reviewboard.mozilla.org/r/112636/#review115644

Comment 98

[Daniel Holbert [:dholbert]]

Comment on attachment 8837516 [details]
Bug 775624 Part 12 - Add bit-fields for inline break status, and convert NS_INLINE_LINE_BREAK_BEFORE.

https://reviewboard.mozilla.org/r/112640/#review115716

r=me on this part, with nit addressed:

::: layout/generic/nsIFrame.h:225
(Diff revision 2)
> +  nsReflowStatus& operator=(const nsReflowStatus& aRhs) {
> +    mBreakType = aRhs.mBreakType;
> +    mIncomplete = aRhs.mIncomplete;
> +    mOverflowIncomplete = aRhs.mOverflowIncomplete;
> +    mNextInFlowNeedsReflow = aRhs.mNextInFlowNeedsReflow;
> +    mTruncated = aRhs.mTruncated;
> +    mInlineBreak = aRhs.mInlineBreak;
> +    mInlineBreakAfter = aRhs.mInlineBreakAfter;
> +    return *this;
>    }

Please remove this reassignment operator -- I don't think you need to explicitly define this.  (Or if you do, please clarify why.)

The default operator should work just fine, and should do exactly what your longform version here does.  (And I can compile successfully, with your patch-stack applied up to this point & with this chunk of code manually removed.)

Comment 99

[fantasai]

Some comments:

Complete, Incomplete, and OverflowIncomplete are mutually exclusive statuses, so they should be an enum and not individual flags--see the original patch. Also, the comments on these should be updated to talk about the interfaces and how to use them and not about the private data bits and how they're stored.

Wrt SetTruncation etc--what I managed to figure out while working on this was that it wasn't a flag about truncation. It was a flag about whether the frame needs to be pushed to the next continuation, because it didn't fit and we decided it should be pushed rather than truncated. (This is why the patch renamed it to "NeedsPush".) If this is in fact still the case, I think we should take this refactoring opportunity to rename it to make more sense, because the current name definitely made it harder for me to understand what it was for and when it should be set.

UpdateTruncatedStatus should be a method on nsReflowStatus not on ReflowInput, because it is updating nsReflowStatus not ReflowInput. We weren't able to do this before because nsReflowStatus wasn't a class, but now it is, so I think we should fix that.

aState.mReflowStatus = nsReflowStatus::InlineLineBreakBefore() is a weird construction imho. Creating a method for setting the break status would probably make a lot more sense. (The old code uses assignment because it's a macro-controlled bitfield; we don't need to copy that approach here.)

Also, "no break", "break before", and "break after" are mutually exclusive states and, like the completion status, would be better represented in enum form.

The InlineBreak stuff isn't exactly about inline breaks. It's an awkward combination of line breaking and clearance, and is therefore misnamed and misdocumented; this is why the original patch renamed everything in that part of the interface. You don't have to copy it if you've got something better, but unless we extract the clearance part into a separate interface, calling it “inline” break is definitely inaccurate and should be fixed.

Note that it's likely that we'll be adding page/column break controls along the lines of the InlineBreak stuff to nsReflowStatus in the future (that was the original impetus for this bug), and those controls will need to be distinct from the clearance controls (but could possibly be merged with the line break controls, unless we end up supporting page breaking controls on inline elements).

The break type should probably be listed with the break-related flags, rather than up with the completion states.

That's all I got for now. Dholbert covered my concerns about overuse of .Reset(). :)

Comment 100

[Daniel Holbert [:dholbert]]

Thanks for your feedback, fantasai!

I think some of it can probably be deferred to a followup.  IMO, there's value in getting this hacky-bitfield ported to an actual class with strict getters/setters, as a largely direct mechanical change, and then *from there* we can refine the representation as needed.  Also, this is quite a large patch-stack, so rebasing can be painful, and I've likely already given TYLin a lot of rebasing-pain with my feedback so far.  So, I don't want to require a fully-perfect solution here when we can still do something mechanical & strictly-better-than-what-we-have & establish a baseline from which to make further targeted improvements.

Specific responses below:
(In reply to fantasai from comment #99)
> Complete, Incomplete, and OverflowIncomplete are mutually exclusive
> statuses, so they should be an enum and not individual flags--see the
> original patch.

This makes sense to me, though I'm also fine with the patch stack's current 2-bit representation for the time being (particularly since "complete-or-not" is the main thing we care about in a lot of the code).  I would be OK with a reformulation with this state into an enum, either here or in a followup. (Fortunately this can be easily changed in a targeted way under the hood in a followup, since all queries/tweaks will now be made through controlled APIs after this patch-stack.)

> Also, the comments on these should be updated to talk about
> the interfaces and how to use them and not about the private data bits and
> how they're stored.

Agreed, though again, this is easy to update after the fact. The patch stack right now is mostly porting existing comment-text from describing an old state bit to describing a new state bit, and there's some value in keeping that as a mechanical change & then refining the new language in-place afterwards. *shrug*

> Wrt SetTruncation etc-- [...]the [old] patch renamed it to "NeedsPush".
> [...] I think we
> should take this refactoring opportunity to rename it to make more sense,
> because the current name definitely made it harder for me to understand what
> it was for and when it should be set.

Alternately/also, we could punt on that renaming, & just keep the NS_FRAME_SET_TRUNCATION macro around as a layer of abstraction right now (i.e. we should do away with "part 10c").   That way, we can iterate on the exact naming of this API & state in a very targeted followup without having to touch All The Files.

That would be my preference.  (So we shouldn't take 10c here, IMO; and we should perhaps think harder about the name of this API before we get rid of NS_FRAME_SET_TRUNCATION.)

> UpdateTruncatedStatus should be a method on nsReflowStatus not on
> ReflowInput

Totally agree -- this is what I just realized in comment 95.  (I filed a MozReview issue for this already.)

> aState.mReflowStatus = nsReflowStatus::InlineLineBreakBefore() is a weird
> construction imho. Creating a method for setting the break status would
> probably make a lot more sense. (The old code uses assignment because it's a
> macro-controlled bitfield; we don't need to copy that approach here.)

Oh, darn. :(  That's sort of what TYLin was doing initially -- he had SetBreakBefore(), though it was confusing because it *also* reset all the other state, simply to preserve old assignment-macro's behavior.  I steered him towards using "aStatus = InlineLineBreakBefore()" in comment 47-48, to make the full-resetting clearer & to allow this to still be a mechanical change.

I tend think that what we've got now ("aStatus = InlineLineBreakBefore()") is probably the best solution for the purposes of this bug, in the interests of keeping this mega-refactoring as mechanical/direct as possible.  And then we can iterate on representing/modifying this piece of the state with a more targeted setter (or whatever) in a followup if that's really better.

> Also, "no break", "break before", and "break after" are mutually exclusive
> states and, like the completion status, would be better represented in enum
> form.

This makes sense, though my thoughts about the Complete, Incomplete, & OverflowIncomplete state-bits apply here too. (Could be adjusted under-the-hood in a followup, & kind of nice to let this conversion here be as direct & mechanical as possible.)

> The InlineBreak stuff isn't exactly about inline breaks. It's an awkward
> combination of line breaking and clearance, and is therefore misnamed and
> misdocumented; [...] calling it “inline” break is definitely inaccurate
> and should be fixed.

I think it's simplest to preserve the existing naming for now (i.e. using "InlineLineBreakBefore") until we come up with a better name for this combined concept.  (We certainly can't just drop "inline" and call it "BreakBefore" etc -- TYLin's first round of patches here sort of did that, but that makes this too easy to confuse with the CSS "break-before" & "break-after" properties.) As above, I think we can iterate after this lands.

Comment 101

[Daniel Holbert [:dholbert]]

Comment on attachment 8837517 [details]
Bug 775624 Part 13 - Convert NS_INLINE_LINE_BREAK_AFTER to a method.

https://reviewboard.mozilla.org/r/112642/#review115734

r=me on this part, with a documentation tweak:

::: layout/generic/nsIFrame.h:350
(Diff revision 2)
> +  // Construct an inline line-break-after status by default, and cloning
> +  // other bit flags from aStatusToClone. The break type can be changed by
> +  // setting aBreakType.
> +  static nsReflowStatus
> +  InlineLineBreakAfter(nsReflowStatus aStatusToClone = nsReflowStatus(),

This first sentence isn't accurate here. Despite what the comment says, the code does *not* necessarily default-construct a nsReflowStatus. (And it doesn't really clone bits from aStatusToClone, except to the extent that aStatusToClone is itself a clone, by virtue of pass-by-value.)

How about something like the following, which is IMO more accurate/grokkable:
"Returns a copy of aStatusToClone, modified to add the desire to have a line-break after.  The break type can be changed via the optional aBreakType param."

Comment 102

[Daniel Holbert [:dholbert]]

Comment on attachment 8837519 [details]
Bug 775624 Part 15 - Remove NS_INLINE_BREAK_TYPE_MASK.

https://reviewboard.mozilla.org/r/112646/#review115736

r=me

Comment 103

[Daniel Holbert [:dholbert]]

Comment on attachment 8837520 [details]
Bug 775624 Part 16 - Convert NS_INLINE_IS_BREAK_BEFORE to a method.

https://reviewboard.mozilla.org/r/112648/#review115738

r=me

Comment 104

[Daniel Holbert [:dholbert]]

Comment on attachment 8837521 [details]
Bug 775624 Part 17 - Convert NS_INLINE_IS_BREAK_AFTER to a method.

https://reviewboard.mozilla.org/r/112650/#review115740

r=me

Comment 105

[Daniel Holbert [:dholbert]]

Comment on attachment 8837522 [details]
Bug 775624 Part 18 - Convert NS_INLINE_IS_BREAK to a method.

https://reviewboard.mozilla.org/r/112652/#review115742

r=me

Comment 106

[Daniel Holbert [:dholbert]]

Comment on attachment 8839118 [details]
Bug 775624 Part 10c - Remove NS_FRAME_SET_TRUNCATION.

https://reviewboard.mozilla.org/r/113856/#review115744

(As noted above in my reply to fantasai, I think we should punt on 10c, since it's not a necessary part of this bug, & we might want to iterate on the "truncate" name before making this massive string-replacement.  Hence, r- on this piece for now.)

Comment 107

[fantasai]

(In reply to Daniel Holbert [:dholbert] from comment #100)

If you're concerned about rebasing pain, then TYLin can add to the end of the stack. It wouldn't be any different than filing another bug in terms of archaeology (with the added benefit that all related changes are pushed together!) and the patch set would result in the sort of completed cleanup task intended for this bug.

The double-boolean representations of tri-state enums in particular a) confuse what's really going on b) make it easier for someone to put things into an inconsistent state. The way TYLin implemented the shift from mState to various bitfields in a stepwise manner makes it clear the underlying representation is changing anyway--it's not as if we're just changing macros to functions operating on the identical data structure. We might as well clean it up properly. This is something the original patch did, so it is explicitly in the scope of fixing this bug.

> > aState.mReflowStatus = nsReflowStatus::InlineLineBreakBefore() is a weird
> > construction imho. Creating a method for setting the break status would
> > probably make a lot more sense. (The old code uses assignment because it's a
> > macro-controlled bitfield; we don't need to copy that approach here.)
> 
> Oh, darn. :(  That's sort of what TYLin was doing initially -- he had
> SetBreakBefore(), though it was confusing because it *also* reset all the
> other state, simply to preserve old assignment-macro's behavior.  I steered
> him towards using "aStatus = InlineLineBreakBefore()" in comment 47-48, to
> make the full-resetting clearer & to allow this to still be a mechanical
> change.

It's a bit of a tricky argument, because
  * If you break before, your completion status literally doesn't matter
  * Breaking after can't validly combine with Incomplete: only Complete or OverflowIncomplete can.
... however, currently we only handle breakAfter + Complete--the OverflowIncomplete + breakAfter combination only comes into play with page/column breaking.

The reason for using a method here rather than magic-constructor-assignment is that
  a) the ergonomics are better
  b) we can adjust the interaction with completion statuses later, if necessary, within the nsReflowStatus encapsulation

Comment 108

[Daniel Holbert [:dholbert]]

(In reply to fantasai from comment #107)
> If you're concerned about rebasing pain, then TYLin can add to the end of
> the stack. It wouldn't be any different than filing another bug in terms of
> archaeology (with the added benefit that all related changes are pushed
> together!) and the patch set would result in the sort of completed cleanup
> task intended for this bug.

That's fine with me, I guess. (Though it does mean nothing can land until all of the parts are done here.  Maybe that's good, though.)

> The double-boolean representations of tri-state enums in particular a)
> confuse what's really going on b) make it easier for someone to put things
> into an inconsistent state. The way TYLin implemented the shift from mState
> to various bitfields in a stepwise manner makes it clear the underlying
> representation is changing anyway--it's not as if we're just changing macros
> to functions operating on the identical data structure. We might as well
> clean it up properly. This is something the original patch did, so it is
> explicitly in the scope of fixing this bug.

Fair enough.  TYLin, would you be up for doing that here? Extra patches at the end of the queue are fine if you like (to merge bools into a single enum).

> > > aState.mReflowStatus = nsReflowStatus::InlineLineBreakBefore() is a weird
> > > construction imho. Creating a method for setting the break status would
> > > probably make a lot more sense.
> > Oh, darn. :(  That's sort of what TYLin was doing initially -- he had
> > SetBreakBefore(), though it was confusing because it *also* reset all the
> > other state [...]
> 
> It's a bit of a tricky argument, because
>   * If you break before, your completion status literally doesn't matter
>   * Breaking after can't validly combine with Incomplete: only Complete or
> OverflowIncomplete can.

OK, I think you've convinced me. TYLin, sorry for leading you astray on that.

For this setter to work as a method, I think we need to either
 (a) make it absolutely clear in the method name that it resets all the status bits (so SetLineBreakBeforeAndReset() or something).  This is kinda awkward though, so I don't love it.
My preferred option:
 (b) Don't bother resetting any status bits in our new SetLineBreakBefore() setter -- instead, just document clearly that the other state isn't expected to matter on a "inline-break-before" ReflowStatus.  (And maybe even add assertions to all of the getters to verify this that they're not called on an inline-break-before ReflowStatus? But maybe that's overkill.)

Comment 109

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

Comment on attachment 8837516 [details]
Bug 775624 Part 12 - Add bit-fields for inline break status, and convert NS_INLINE_LINE_BREAK_BEFORE.

https://reviewboard.mozilla.org/r/112640/#review115716

> Please remove this reassignment operator -- I don't think you need to explicitly define this.  (Or if you do, please clarify why.)
> 
> The default operator should work just fine, and should do exactly what your longform version here does.  (And I can compile successfully, with your patch-stack applied up to this point & with this chunk of code manually removed.)

Yes. The default `operator=` works fine, it's the `operator=(uint32_t)` preventing the compiler from generating a default one this moment. That's why I added the longform in this part to make it build, but I forgot to delete it later. Thank you for catching this.

Deleting `operator=(uint32_t)` also compiles, so I'll just remove it in this part since it'll gone in Part 23 anyway.

Comment 110

[Daniel Holbert [:dholbert]]

Comment on attachment 8837516 [details]
Bug 775624 Part 12 - Add bit-fields for inline break status, and convert NS_INLINE_LINE_BREAK_BEFORE.

https://reviewboard.mozilla.org/r/112640/#review115850

Thanks in advance for dropping the reassignment operator (sounds like you've done that locally though it doesn't show up here yet).

I meant to mark this as r+ with that, in comment 98, but I got the review flag wrong. Fixing that & marking as r+ now.  (Though per comment 107 & comment 108, we probably need to update the representation to be an enum & the setters to actually be clearly-documented setters rather than a static factory function. I'm marking this r+ regardless since I suspect you might want to change that in a followup patch at the end of this stack -- but if you want to update/replace this patch itself instead, please tag me for needinfo so I can re-review this one.)

Comment 111

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

Comment on attachment 8837505 [details]
Bug 775624 Part 1 - Convert nsReflowStatus to a class mimicking uint32_t.

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

Comment 112

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

Comment on attachment 8837506 [details]
Bug 775624 Part 2 - Add bit-fields and methods for frame completion status.

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

Comment 113

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

Comment on attachment 8837507 [details]
Bug 775624 Part 3 - Remove NS_FRAME_IS_NOT_COMPLETE.

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

Comment 114

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

Comment on attachment 8837508 [details]
Bug 775624 Part 4 - Remove NS_FRAME_IS_FULLY_COMPLETE.

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

Comment 115

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

Comment on attachment 8837509 [details]
Bug 775624 Part 5 - Remove NS_FRAME_IS_COMPLETE.

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

Comment 116

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

Comment on attachment 8837510 [details]
Bug 775624 Part 6 - Remove NS_FRAME_OVERFLOW_IS_INCOMPLETE.

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

Comment 117

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

Comment on attachment 8837511 [details]
Bug 775624 Part 7 - Remove NS_FRAME_SET_INCOMPLETE.

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

Comment 118

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

Comment on attachment 8837512 [details]
Bug 775624 Part 8 - Remove NS_FRAME_SET_OVERFLOW_INCOMPLETE.

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

Comment 119

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

Comment on attachment 8837513 [details]
Bug 775624 Part 9 - Convert NS_FRAME_REFLOW_NEXTINFLOW to use bit-field and methods.

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

Comment 120

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

Comment on attachment 8839117 [details]
Bug 775624 Part 10a - Move ReflowInput::SetTruncated() into nsReflowStatus.

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

Comment 121

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

Comment on attachment 8837514 [details]
Bug 775624 Part 10b - Convert NS_FRAME_TRUNCATED to use bit-field and methods.

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

Comment 122

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

Comment on attachment 8837515 [details]
Bug 775624 Part 11 - Convert NS_MergeReflowStatusInto() to a method.

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

Comment 123

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

Comment on attachment 8837516 [details]
Bug 775624 Part 12 - Add bit-fields for inline break status, and convert NS_INLINE_LINE_BREAK_BEFORE.

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

Comment 124

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

Comment on attachment 8837517 [details]
Bug 775624 Part 13 - Convert NS_INLINE_LINE_BREAK_AFTER to a method.

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

Comment 125

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

Comment on attachment 8837518 [details]
Bug 775624 Part 14 - Convert NS_INLINE_GET_BREAK_TYPE to a method.

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

Comment 126

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

Comment on attachment 8837519 [details]
Bug 775624 Part 15 - Remove NS_INLINE_BREAK_TYPE_MASK.

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

Comment 127

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

Comment on attachment 8837520 [details]
Bug 775624 Part 16 - Convert NS_INLINE_IS_BREAK_BEFORE to a method.

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

Comment 128

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

Comment on attachment 8837521 [details]
Bug 775624 Part 17 - Convert NS_INLINE_IS_BREAK_AFTER to a method.

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

Comment 129

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

Comment on attachment 8837522 [details]
Bug 775624 Part 18 - Convert NS_INLINE_IS_BREAK to a method.

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

Comment 130

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

Comment on attachment 8837523 [details]
Bug 775624 Part 19 - Convert NS_INLINE_BREAK_FIRST_LETTER_COMPLETE to use bit-field and methods.

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

Comment 131

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

Comment on attachment 8837524 [details]
Bug 775624 Part 20 - Remove NS_FRAME_OVERFLOW_INCOMPLETE.

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

Comment 132

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

Comment on attachment 8837525 [details]
Bug 775624 Part 21 - Remove NS_FRAME_NOT_COMPLETE.

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

Comment 133

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

Comment on attachment 8837526 [details]
Bug 775624 Part 22 - Remove NS_FRAME_COMPLETE.

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

Comment 134

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

Comment on attachment 8837527 [details]
Bug 775624 Part 23 - Remove constructor and all operators related to uint32_t.

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

Comment 135

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

Comment on attachment 8837528 [details]
Bug 775624 Part 24 - Implement operator<< for nsReflowStatus to use mozilla::ToString.

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

Comment 136

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

In the new patchset, I've changed the following. 
1) In Part 10a, ReflowInput::SetTruncated() is moved to nsReflowStatus. (Addressed comment 95 and comment 99.)

2) Due to Part 10a, Part 10b no long has the two public methods ClearTruncated() and SetTruncated() (Addressed comment 95.)

3) Drop Part 10c until we find better naming to replace NS_FRAME_IS_TRUNCATED macro (Addressed comment 100.)

4) Change Part 12 back to SetLineBreakBeforeAndReset() per comment 99 and comment 108. Yes. I use approach (a) in comment 108 :( I have to still call Reset() in the method because try is unhappy about not calling it. https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d4cf21c5c54ad3f1821ba0a562a1f7ce79bcf8a

Also, because magic-constructor-assignment is removed, I don't need to pre-drop operator=(uint32_t) to compile (comment 109).

5) Change Part 13 to use SetInlineLineBreakAfter() method.

6) Rewrite Part 15 because of the API changes in Part 12.

Daniel, could you review Part 10a, 10b, 12, and 13, again, please? I'd love to know if the current patches are all good before I do something non-mechanical.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=19b86307e8fcea5ccdcce460f1febe12b8e75dfd

fantasai, thank you for the comments! I've addressed some of them in my latest patchset. I understand you did a massive rewrite in your original patch. However, I nearly know nothing about these frame completion status or line breaking logic in action, so I fail to morph all the naming or rewrite decision you were making into my new patches. What I can do is trying to keep the naming and behavior as intact as possible so that other people (could be me in the future) could have a better foundation to come up something better. Any tiny behavior change like dropping the Reset() in SetLineBreakBeforeAndReset() is prone to fail the tests (like the point 4 above), and the patch series itself is not even bisectable. I'm happy to do more clean-up/renaming/behavior-changing followup after this conversion lands. At least I can debug the patch.

Anyway, I'll still do patches tomorrow to convert the tri-state "Complete, Incomplete, and OverflowIncomplete" to an enum as well as the "NoBreak, BreakBefore and BreakAfter."

Comment 137

[Daniel Holbert [:dholbert]]

Comment on attachment 8839117 [details]
Bug 775624 Part 10a - Move ReflowInput::SetTruncated() into nsReflowStatus.

https://reviewboard.mozilla.org/r/113854/#review115970

r=me -- thanks!

Comment 138

[Daniel Holbert [:dholbert]]

Comment on attachment 8837514 [details]
Bug 775624 Part 10b - Convert NS_FRAME_TRUNCATED to use bit-field and methods.

https://reviewboard.mozilla.org/r/112636/#review115972

r=me

Comment 139

[Daniel Holbert [:dholbert]]

Comment on attachment 8837516 [details]
Bug 775624 Part 12 - Add bit-fields for inline break status, and convert NS_INLINE_LINE_BREAK_BEFORE.

https://reviewboard.mozilla.org/r/112640/#review115986

r=me with two nits:

::: layout/generic/nsIFrame.h:326
(Diff revision 3)
> +  // Set the inline line-break-before status, and reset other bit flags. The
> +  // break type can be changed via the optional aBreakType param. Note that
> +  // other frame completion status isn't expected to matter after calling
> +  // this method.
> +  void SetInlineLineBreakBeforeAndReset(StyleClear aBreakType = StyleClear::Line) {
> +    Reset();

We probably shouldn't bother having this "aBreakType" parameter, if we never actually use it.  (And it seems like we don't ever need to use it.)

So: please drop "aBreakType" and just make this method unconditionally use StyleClear::Line. (And please update the method's documentation [quoted here] accordingly, about the break type & this param.)

(I suspect you had this param here for symmetry with the inline-break-after setter. They're different-enough that the complexity/possible-footgun cost outweighs the symmetry-elegance benefit, I think.  If it turns out we ever do need this param in the future, we could always add it then.)

::: layout/style/GenericSpecifiedValuesInlines.h:163
(Diff revision 3)
>    MOZ_STYLO_FORWARD(SetTextDecorationColorOverride, ())
>  }
>  
>  } // namespace mozilla
>  
>  #endif // mozilla_GenericSpecifiedValuesInlines_h

Please revert this patch's whitespace-only changes to GenericSpecifiedValuesInlines.h and nsSVGUtils.cpp.  (I'm guessing one of your tools might've introduced those by accident.)

(It looks like the changes were fixing the lack-of-a-trailing-newline in these files, which is a Good Thing to fix, though not as part of this patch-stack.  I took the liberty of just pushing a one-off patch to inbound to address this:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8a885b6135f022af93cdf91feb683fb4bd038125 )

Comment 140

[Daniel Holbert [:dholbert]]

Following up on comment 108 RE tri-state enums -- as noted in #layout on IRC, I think it does make sense to migrate the double-bools to tri-state enums, but I don't think it's necessary to gate the rest of these patches on that followup being ready (and incur another day or two of 24 patches' worth of bitrot, from churn happening underneath on mozilla-central).

Let's just get these patches in, and do the tri-state enum migration immediately afterwards.

Comment 141

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

Comment on attachment 8837505 [details]
Bug 775624 Part 1 - Convert nsReflowStatus to a class mimicking uint32_t.

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

Comment 142

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

Comment on attachment 8837506 [details]
Bug 775624 Part 2 - Add bit-fields and methods for frame completion status.

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

Comment 143

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

Comment on attachment 8837507 [details]
Bug 775624 Part 3 - Remove NS_FRAME_IS_NOT_COMPLETE.

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

Comment 144

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

Comment on attachment 8837508 [details]
Bug 775624 Part 4 - Remove NS_FRAME_IS_FULLY_COMPLETE.

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

Comment 145

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

Comment on attachment 8837509 [details]
Bug 775624 Part 5 - Remove NS_FRAME_IS_COMPLETE.

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

Comment 146

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

Comment on attachment 8837510 [details]
Bug 775624 Part 6 - Remove NS_FRAME_OVERFLOW_IS_INCOMPLETE.

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

Comment 147

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

Comment on attachment 8837511 [details]
Bug 775624 Part 7 - Remove NS_FRAME_SET_INCOMPLETE.

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

Comment 148

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

Comment on attachment 8837512 [details]
Bug 775624 Part 8 - Remove NS_FRAME_SET_OVERFLOW_INCOMPLETE.

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

Comment 149

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

Comment on attachment 8837513 [details]
Bug 775624 Part 9 - Convert NS_FRAME_REFLOW_NEXTINFLOW to use bit-field and methods.

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

Comment 150

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

Comment on attachment 8839117 [details]
Bug 775624 Part 10a - Move ReflowInput::SetTruncated() into nsReflowStatus.

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

Comment 151

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

Comment on attachment 8837514 [details]
Bug 775624 Part 10b - Convert NS_FRAME_TRUNCATED to use bit-field and methods.

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

Comment 152

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

Comment on attachment 8837515 [details]
Bug 775624 Part 11 - Convert NS_MergeReflowStatusInto() to a method.

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

Comment 153

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

Comment on attachment 8837516 [details]
Bug 775624 Part 12 - Add bit-fields for inline break status, and convert NS_INLINE_LINE_BREAK_BEFORE.

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

Comment 154

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

Comment on attachment 8837517 [details]
Bug 775624 Part 13 - Convert NS_INLINE_LINE_BREAK_AFTER to a method.

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

Comment 155

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

Comment on attachment 8837518 [details]
Bug 775624 Part 14 - Convert NS_INLINE_GET_BREAK_TYPE to a method.

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

Comment 156

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

Comment on attachment 8837519 [details]
Bug 775624 Part 15 - Remove NS_INLINE_BREAK_TYPE_MASK.

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

Comment 157

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

Comment on attachment 8837520 [details]
Bug 775624 Part 16 - Convert NS_INLINE_IS_BREAK_BEFORE to a method.

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

Comment 158

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

Comment on attachment 8837521 [details]
Bug 775624 Part 17 - Convert NS_INLINE_IS_BREAK_AFTER to a method.

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

Comment 159

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

Comment on attachment 8837522 [details]
Bug 775624 Part 18 - Convert NS_INLINE_IS_BREAK to a method.

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

Comment 160

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

Comment on attachment 8837523 [details]
Bug 775624 Part 19 - Convert NS_INLINE_BREAK_FIRST_LETTER_COMPLETE to use bit-field and methods.

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

Comment 161

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

Comment on attachment 8837524 [details]
Bug 775624 Part 20 - Remove NS_FRAME_OVERFLOW_INCOMPLETE.

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

Comment 162

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

Comment on attachment 8837525 [details]
Bug 775624 Part 21 - Remove NS_FRAME_NOT_COMPLETE.

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

Comment 163

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

Comment on attachment 8837526 [details]
Bug 775624 Part 22 - Remove NS_FRAME_COMPLETE.

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

Comment 164

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

Comment on attachment 8837527 [details]
Bug 775624 Part 23 - Remove constructor and all operators related to uint32_t.

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

Comment 165

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

Comment on attachment 8837528 [details]
Bug 775624 Part 24 - Implement operator<< for nsReflowStatus to use mozilla::ToString.

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

Comment 166

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

Comment on attachment 8837516 [details]
Bug 775624 Part 12 - Add bit-fields for inline break status, and convert NS_INLINE_LINE_BREAK_BEFORE.

https://reviewboard.mozilla.org/r/112640/#review115986

> We probably shouldn't bother having this "aBreakType" parameter, if we never actually use it.  (And it seems like we don't ever need to use it.)
> 
> So: please drop "aBreakType" and just make this method unconditionally use StyleClear::Line. (And please update the method's documentation [quoted here] accordingly, about the break type & this param.)
> 
> (I suspect you had this param here for symmetry with the inline-break-after setter. They're different-enough that the complexity/possible-footgun cost outweighs the symmetry-elegance benefit, I think.  If it turns out we ever do need this param in the future, we could always add it then.)

Right. We never acutally use the aBreakType parameter. I've removed it with documentation updated.

> Please revert this patch's whitespace-only changes to GenericSpecifiedValuesInlines.h and nsSVGUtils.cpp.  (I'm guessing one of your tools might've introduced those by accident.)
> 
> (It looks like the changes were fixing the lack-of-a-trailing-newline in these files, which is a Good Thing to fix, though not as part of this patch-stack.  I took the liberty of just pushing a one-off patch to inbound to address this:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8a885b6135f022af93cdf91feb683fb4bd038125 )

Oh. sed does this for me! Thank you for the fix. I've removed the trailing-newline fix from my patch.

Comment 167

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

Filed bug 1341981 to do the tri-state enum conversion. 

Try results look good. Per comment 140, let's land these patches in.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aed99b038de21b094124695a2419d135d9413d81

Comment 168

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/907e7b586ed2
Part 1 - Convert nsReflowStatus to a class mimicking uint32_t. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/0d9fd874b050
Part 2 - Add bit-fields and methods for frame completion status. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/9f1508490ddf
Part 3 - Remove NS_FRAME_IS_NOT_COMPLETE. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/385c006f46b2
Part 4 - Remove NS_FRAME_IS_FULLY_COMPLETE. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/2cb25268cfbb
Part 5 - Remove NS_FRAME_IS_COMPLETE. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/86aa651c4736
Part 6 - Remove NS_FRAME_OVERFLOW_IS_INCOMPLETE. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/167b50e9d584
Part 7 - Remove NS_FRAME_SET_INCOMPLETE. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/74d9f315b900
Part 8 - Remove NS_FRAME_SET_OVERFLOW_INCOMPLETE. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/bbe9d528bcf5
Part 9 - Convert NS_FRAME_REFLOW_NEXTINFLOW to use bit-field and methods. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/3ea22e82b7a1
Part 10a - Move ReflowInput::SetTruncated() into nsReflowStatus. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/e2e7f176f32e
Part 10b - Convert NS_FRAME_TRUNCATED to use bit-field and methods. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/6d832b39d9a0
Part 11 - Convert NS_MergeReflowStatusInto() to a method. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/43b83d0ac36b
Part 12 - Add bit-fields for inline break status, and convert NS_INLINE_LINE_BREAK_BEFORE. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/b39fdda36636
Part 13 - Convert NS_INLINE_LINE_BREAK_AFTER to a method. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/985eb1c04355
Part 14 - Convert NS_INLINE_GET_BREAK_TYPE to a method. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/4be99f927174
Part 15 - Remove NS_INLINE_BREAK_TYPE_MASK. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/333bdff178fc
Part 16 - Convert NS_INLINE_IS_BREAK_BEFORE to a method. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/8c1a80a31640
Part 17 - Convert NS_INLINE_IS_BREAK_AFTER to a method. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/cf3d22755497
Part 18 - Convert NS_INLINE_IS_BREAK to a method. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/53e23cf580b7
Part 19 - Convert NS_INLINE_BREAK_FIRST_LETTER_COMPLETE to use bit-field and methods. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/72b4e2c63237
Part 20 - Remove NS_FRAME_OVERFLOW_INCOMPLETE. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/dfe7b2bfa220
Part 21 - Remove NS_FRAME_NOT_COMPLETE. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/cb33bd098140
Part 22 - Remove NS_FRAME_COMPLETE. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/672ba79a7840
Part 23 - Remove constructor and all operators related to uint32_t. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/3095d085ef20
Part 24 - Implement operator<< for nsReflowStatus to use mozilla::ToString. r=dholbert

Comment 169

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/907e7b586ed2
https://hg.mozilla.org/mozilla-central/rev/0d9fd874b050
https://hg.mozilla.org/mozilla-central/rev/9f1508490ddf
https://hg.mozilla.org/mozilla-central/rev/385c006f46b2
https://hg.mozilla.org/mozilla-central/rev/2cb25268cfbb
https://hg.mozilla.org/mozilla-central/rev/86aa651c4736
https://hg.mozilla.org/mozilla-central/rev/167b50e9d584
https://hg.mozilla.org/mozilla-central/rev/74d9f315b900
https://hg.mozilla.org/mozilla-central/rev/bbe9d528bcf5
https://hg.mozilla.org/mozilla-central/rev/3ea22e82b7a1
https://hg.mozilla.org/mozilla-central/rev/e2e7f176f32e
https://hg.mozilla.org/mozilla-central/rev/6d832b39d9a0
https://hg.mozilla.org/mozilla-central/rev/43b83d0ac36b
https://hg.mozilla.org/mozilla-central/rev/b39fdda36636
https://hg.mozilla.org/mozilla-central/rev/985eb1c04355
https://hg.mozilla.org/mozilla-central/rev/4be99f927174
https://hg.mozilla.org/mozilla-central/rev/333bdff178fc
https://hg.mozilla.org/mozilla-central/rev/8c1a80a31640
https://hg.mozilla.org/mozilla-central/rev/cf3d22755497
https://hg.mozilla.org/mozilla-central/rev/53e23cf580b7
https://hg.mozilla.org/mozilla-central/rev/72b4e2c63237
https://hg.mozilla.org/mozilla-central/rev/dfe7b2bfa220
https://hg.mozilla.org/mozilla-central/rev/cb33bd098140
https://hg.mozilla.org/mozilla-central/rev/672ba79a7840
https://hg.mozilla.org/mozilla-central/rev/3095d085ef20