Closed Bug 1341981 Opened 7 years ago Closed 7 years ago

Convert frame completion and inline break in nsReflowStatus from double-bools bit flags to tri-state enum

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

Details

Attachments

(3 files)

We should use tri-state enums to represent the frame completion and inline breaking bits per bug 775624 comment 99. Per bug 775624 comment 108, I'll do this as a followup here.
Comment on attachment 8841404 [details]
Bug 1341981 Part 1 - Strip trailing whitespaces in nsIFrame.h.

https://reviewboard.mozilla.org/r/115620/#review117158
Attachment #8841404 - Flags: review?(dholbert) → review+
Comment on attachment 8841405 [details]
Bug 1341981 Part 2 - Convert frame completion status to a tri-state enum class.

https://reviewboard.mozilla.org/r/115622/#review117166

r=me with nits addressed.

Commit message nit:
> Bug 1341981 Part 2 - Convert frame completion to a tri-state enum class.

Add the word "state" in there, for extra clarity -- "Convert frame completion state..."

::: layout/generic/nsContainerFrame.h:328
(Diff revision 1)
>     * list (to be drained by the next-in-flow when it calls
>     * ReflowOverflowContainerChildren). The parent continues reflow as if
> -   * the frame was complete once it ran out of computed height, but returns
> -   * with either the mIncomplete or mOverflowIncomplete bit set in reflow
> -   * status to request a next-in-flow. The parent's next-in-flow is then
> +   * the frame was complete once it ran out of computed height, but returns a
> +   * reflow status with either IsIncomplete() or IsOverflowIncomplete() equals
> +   * to true to request a next-in-flow. The parent's next-in-flow is then

s/equals/equal/

::: layout/generic/nsIFrame.h:205
(Diff revision 1)
>    using StyleClear = mozilla::StyleClear;
>  
>  public:
>    nsReflowStatus()
>      : mBreakType(StyleClear::None)
> -    , mIncomplete(false)
> +    , mCompletion(Completion::FullyComplete)

Hmm. It feels odd that we *initialize* mCompletion to FullyComplete, before the reflow has even happened yet.

I wonder if we should add an "unknown" value as the initial state for this enum?  And then we'd only update that to Complete in a spot where traditionally we would've done "aStatus = NS_FRAME_COMPLETE" or whatever.

I guess that might just create extra needless churn, though, since IIRC we tend to optimistically assume we're complete until we discover that we're not.  So this is probably fine.

::: layout/generic/nsIFrame.h:234
(Diff revision 1)
>              !mInlineBreak &&
>              !mInlineBreakAfter &&
>              !mFirstLetterComplete);
>    }
>  
> -  // mIncomplete bit flag means the frame does not map all its content, and
> +  // There are three completion statuses, represented by mCompletion.

Please add the word "possible" here, like so:

  // There are three possible completion statuses [...]

That makes it clearer that you're describing *three distinct states*, rather than *3 distinct pieces-of-state*. (Right now the wording is ambiguous about which of those it's describing.)

::: layout/generic/nsIFrame.h:246
(Diff revision 1)
> +  // FullyComplete means the frame is neither Incomplete nor
> +  // OverflowIncomplete.

Please add at the end here: ("This is the default state for a nsReflowStatus.")

(This is nice for clarity, since it's non-obvious that a just-created nsReflowStatus would report itself as being complete.)

::: layout/generic/nsIFrame.h:257
(Diff revision 1)
> +  bool IsComplete() const { return mCompletion != Completion::Incomplete; }
> +  bool IsIncomplete() const { return mCompletion == Completion::Incomplete; }
> +  bool IsOverflowIncomplete() const {
> +    return mCompletion == Completion::OverflowIncomplete;
> +  }
>    bool IsFullyComplete() const {
> -    return !IsIncomplete() && !IsOverflowIncomplete();
> +    return mCompletion == Completion::FullyComplete;
>    }

Since IsComplete is just a convenience function (rather than a distinct state), let's make that clearer in its implementation, like so:

  // Just for convenience; not a distinct state:
  bool IsComplete() const { return !IsIncomplete(); }

And let's move it below IsIncomplete, since it's implemented in terms of that now.   (Possibly even below *all* of the IsFoo() methods here, to separate it more clearly... whichever you prefer.)

::: layout/generic/nsIFrame.h:292
(Diff revision 1)
> -    mIncomplete |= aStatus.mIncomplete;
> -    mOverflowIncomplete |= aStatus.mOverflowIncomplete;
> +    if (mCompletion < aStatus.mCompletion) {
> +      mCompletion = aStatus.mCompletion;
> +    }

To ensure your comment in the enum definition is actually correct, please add 2 static asserts here, to be sure the upgrading works correctly (and to help explain why this "less-than" comparison does the right thing here). Something like:

  // These asserts ensure that the mCompletion merging works as we expect.
  // (Incomplete beats OverflowIncomplete, which beats FullyComplete.)
  static_assert(Completion::Incomplete > Completion::OverflowIncomplete);
  static_assert(Completion::OverflowIncomplete > Completion::FullyComplete);
Attachment #8841405 - Flags: review?(dholbert) → review+
Comment on attachment 8841406 [details]
Bug 1341981 Part 3 - Convert inline break status to a tri-state enum class.

https://reviewboard.mozilla.org/r/115624/#review117226

r=me with nits:

First, as with the previous part, please add "state" to the commit message:
> Bug 1341981 Part 3 - Convert inline break to a tri-state enum class.

(So this should be "inline break state" or perhaps "inline break status")

::: layout/generic/nsIFrame.h:296
(Diff revision 1)
>      }
>      mNextInFlowNeedsReflow |= aStatus.mNextInFlowNeedsReflow;
>      mTruncated |= aStatus.mTruncated;
>    }
>  
> -  // mInlineBreak bit flag means a break is requested.
> +  // There are three break condition, represented by mInlineBreak.

s/three break condition/three different inline-break states/

::: layout/generic/nsIFrame.h:299
(Diff revision 1)
> +  // "Before" means the break should occur before the frame is just reflowed.
> +  // "After" means the break should occur after the frame is just reflowed.

"...before the frame is just reflowed" is vague & grammatically-awkward here.

How about we replace that with:
  // "Before" means the break should occur before the frame.
  // "After" means the break should occur after the frame.
  // (Here, "the frame" is the frame whose reflow results are being reported by
  // this nsReflowStatus.)
Attachment #8841406 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #6)
> Comment on attachment 8841406 [details]
> Bug 1341981 Part 3 - Convert inline break to a tri-state enum class.
[...]
> > -  // mInlineBreak bit flag means a break is requested.
> > +  // There are three break condition, represented by mInlineBreak.
> 
> s/three break condition/three different inline-break states/

(Or maybe: "three possible inline-break statuses" for consistency with my suggested language for the other enum in comment 5)
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/df1c149b427f
Part 1 - Strip trailing whitespaces in nsIFrame.h. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/6e4265411854
Part 2 - Convert frame completion status to a tri-state enum class. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/693036540bcf
Part 3 - Convert inline break status to a tri-state enum class. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/df1c149b427f
https://hg.mozilla.org/mozilla-central/rev/6e4265411854
https://hg.mozilla.org/mozilla-central/rev/693036540bcf
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
> I guess that might just create extra needless churn, though, since IIRC we tend to optimistically assume we're complete until we discover that we're not.  So this is probably fine.

Yes. This makes sense because a lot of frame types don't split ever, and also the majority of the time we're not paginating anyway, meaning only inline boxes will ever be incomplete in a typical web page. :)
You need to log in before you can comment on or make changes to this bug.