Closed Bug 1341009 Opened 7 years ago Closed 7 years ago

Eliminate unneeded nsReflowStatus::Reset() calls

Categories

(Core :: Layout, defect, P3)

Product:
Core
Component:
Layout
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: TYLin, Assigned: TYLin)

References

Details

Attachments

(7 files)

Bug 1341009 - Remove nsReflowStatus::Reset() in BlockReflowInput's constructor.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1341009 - Pass const reference instead of value for nsReflowStatus.
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1341009 - Add nsReflowStatus::IsEmpty() assertion to nsAbsoluteContainingBlock::ReflowAbsoluteFrame().
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1341009 - Reset aStatus before calling nsLineLayout::ReflowFrame().
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1341009 - Reset aStatus before calling nsContainerFrame::ReflowChild().
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1341009 - Add nsReflowStatus::IsEmpty() assertions to all nsIFrame::Reflow() methods and some reflow helpers, and remove unneeded Reset().
59 bytes, text/x-review-board-request
dholbert
: review+
Details
Bug 1341009 - Convert nsReflowStatus::Reset() to nsReflowStatus::IsEmpty() assertion in ReflowText().
59 bytes, text/x-review-board-request
dholbert
: review+
Details 
File per bug 775624 comment 58 and bug 775624 comment 59.

In bug 775624, both Part 21 and Part 22 did semantics-preserving conversion that calling nsReflowStatus::Reset() before calling SetIncomplete(). 

For those Reset() calls in the beginning of the Reflow(), we could remove them or convert them to use IsEmpty() assert. 

For those Reset() calls in the end of the Reflow(), we could convert them to use IsEmpty() or IsComplete() assert.

We could even change nsIFrame::Reflow() to return nsReflowStatus so that the each Reflow() is guaranteed to use a fresh local nsReflowStatus without having to reset the one from the caller. And if the Reflow() is simply, it could return an empty nsReflowStatus() directly. 

[1] http://searchfox.org/mozilla-central/rev/12cf11303392edac9f1da0c02e3d9ad2ecc8f4d3/layout/generic/nsIFrame.h#2283
Would it be a perf win? Or simply a refactoring work?
Priority: -- → P3
(In reply to Astley Chen [:astley] (UTC+8) from comment #1)
> Would it be a perf win? Or simply a refactoring work?

Unlikely to be a noticeable perf win; this is just post-refactoring cleanup, basically.

Thanks for following up on it, TYLin!
Comment on attachment 8907076 [details]
Bug 1341009 - Remove nsReflowStatus::Reset() in BlockReflowInput's constructor.

https://reviewboard.mozilla.org/r/178776/#review183880
Attachment #8907076 - Flags: review?(dholbert) → review+
Comment on attachment 8907077 [details]
Bug 1341009 - Pass const reference instead of value for nsReflowStatus.

https://reviewboard.mozilla.org/r/178778/#review183906

::: commit-message-39897:1
(Diff revision 1)
> +Bug 1341009 - Ensure nsReflowStatus is empty before calling nsIFrame::Reflow() and ReflowText.
> +
> +After this patch, we can replace most of nsReflowStatus::Reset() calls by
> +asserting nsReflowStatus::IsEmpty() in Reflow() and other reflow helper
> +functions.

It's unclear from the commit message whether or not this patch is intended to change functionality.  Does it?

Initially I thought that it did potentially change functionality (clobbering reflow statuses that would otherwise "accumulate").

But maybe these are cases where our callers or nsIFrame::Reflow impls are already resetting this reflow status internally? (and if that's the case, this patch is just adding some redundant [until the subsequent patch] *additional* resetting, and hence not changing behavior)

Please clarify the commit message on this point.  Though, see the rest of my comments first -- I think some of these changes may be redundant *even after* the next patch, or may want to move around a bit, which might change what you want to say in the commit message.

::: layout/generic/nsAbsoluteContainingBlock.cpp:721
(Diff revision 1)
>      }
>    }
>  
>    // Do the reflow
>    ReflowOutput kidDesiredSize(kidReflowInput);
> +  aStatus.Reset();

Is this really necessary?  This is ReflowAbsoluteFrame, and the only caller declares |nsReflowStatus kidStatus;| right before passing it in.

So I think we're safe to assume aStatus is already blank here.  Resetting it is confusing because it implies that there's the possibility that it contains something that'd need to be cleared.

::: layout/generic/nsBlockReflowContext.cpp:306
(Diff revision 1)
> +  aFrameReflowStatus.Reset();
>    mFrame->Reflow(mPresContext, mMetrics, aFrameRI, aFrameReflowStatus);

As above, is this really necessary?

::: layout/generic/nsContainerFrame.cpp:932
(Diff revision 1)
>      PositionFrameView(aKidFrame);
>      PositionChildViews(aKidFrame);
>    }
>  
>    // Reflow the child frame
> +  aStatus.Reset();

I'd rather not do this in ReflowChild (which is just a lightweight wrapper around aKidFrame->Reflow).

To the extent that we need this is necessary for your assertions in the next patch to pass: I'd rather we do the resetting in whichever callers are problematic.  (Or perhaps-better, make those callers use an dedicated local ReflowStatus which will be guaranteed to be empty.)

::: layout/generic/nsContainerFrame.cpp:977
(Diff revision 1)
>      PositionFrameView(aKidFrame);
>      PositionChildViews(aKidFrame);
>    }
>  
>    // Reflow the child frame
> +  aStatus.Reset();

same here (in another ReflowChild impl) -- if we need this, let's fix the callers.

::: layout/generic/nsFirstLetterFrame.cpp:205
(Diff revision 1)
>                                aReflowInput.AvailableHeight()));
>      rs.mLineLayout = ≪
>      ll.SetInFirstLetter(true);
>      ll.SetFirstLetterStyleOK(true);
>  
> +    aReflowStatus.Reset();

Again, is this needed?

Since this is a reflow method, I'd expect that the next patch should be giving it an assertion like so:
  MOZ_ASSERT(aReflowStatus.IsEmpty(), "Caller should pass a fresh reflow status!");

(The next patch doesn't currently touch this file, but it perhaps should touch this file and add that ^^ assertion.)

And if that hypotehtical assertion holds, then we don't need to explicitly reset aReflowStatus here, because we should already know it's empty.

::: layout/generic/nsLineLayout.cpp:919
(Diff revision 1)
>  
> +  aReflowStatus.Reset();
>    if (!isText) {
>      aFrame->Reflow(mPresContext, reflowOutput, *reflowInputHolder, aReflowStatus);
>    } else {
>      static_cast<nsTextFrame*>(aFrame)->
>        ReflowText(*this, availableSpaceOnLine,

As with nsContainerFrame::ReflowChild, this function (nsLineLayout::ReflowFrame) seems like it's just a wrapper for aFrame->Reflow, which doesn't intend to do anything special with aReflowStatus.

So, our callers should probably already be passing in a fresh aReflowStatus -- and if any aren't, we should fix/investigate those callers rather than adding a Reset() call here.
Attachment #8907077 - Flags: review?(dholbert) → review-
Comment on attachment 8907078 [details]
Bug 1341009 - Add nsReflowStatus::IsEmpty() assertion to nsAbsoluteContainingBlock::ReflowAbsoluteFrame().

https://reviewboard.mozilla.org/r/178780/#review183914

::: layout/forms/nsDateTimeControlFrame.cpp:319
(Diff revision 1)
>      ConsiderChildOverflow(aDesiredSize.mOverflowAreas, inputAreaFrame);
>    }
>  
>    FinishAndStoreOverflow(&aDesiredSize);
>  
> -  aStatus.Reset();
> +  MOZ_ASSERT(aStatus.IsEmpty(), "Caller should pass a fresh reflow status!");

This assertion (especially its blaming of "the caller") doesn't make sense here, at the *very end* of our reflow function.  The message is phrased like it's a precondition on the args, but its code-placement makes it effectively a post-condition on what our own function just finished doing.

To resolve this problem, please move this assertion to the top of the function (with its current message about the caller).  It probably should be placed after DISPLAY_REFLOW, for consistency with the rest of your patch.

This issue applies to many of the files in this patch -- nsDateTimeControlFrame, nsNumberControlFrame, nsRangeFrame, nsBackdropFrame, nsBulletFrame, 
nsHTMLFramesetBorderFrame, nsPlaceholderFrame, several MathML frames, nsSVGForeignObjectFrame, and nsTableColFrame.

In some cases (particularly for more complex container-ish frames), it'd perhaps be worth *also* leaving behind a postcondition (like what you've got here), to document/enforce that we should be returning a complete status & don't support being split.  But the wording of that postcondition should be more about our own reflow (and what we're returning) rather than about the caller.

::: layout/forms/nsFormControlFrame.cpp:114
(Diff revision 1)
>    MarkInReflow();
>    DO_GLOBAL_REFLOW_COUNT("nsFormControlFrame");
>    DISPLAY_REFLOW(aPresContext, this, aReflowInput, aDesiredSize, aStatus);
>    NS_FRAME_TRACE(NS_FRAME_TRACE_CALLS,
>                   ("enter nsFormControlFrame::Reflow: aMaxSize=%d,%d",
>                    aReflowInput.AvailableWidth(), aReflowInput.AvailableHeight()));
>  
>    if (mState & NS_FRAME_FIRST_REFLOW) {
>      RegUnRegAccessKey(static_cast<nsIFrame*>(this), true);
>    }
>  
> -  aStatus.Reset();
> +  MOZ_ASSERT(aStatus.IsEmpty(), "Caller should pass a fresh reflow status!");
>    aDesiredSize.SetSize(aReflowInput.GetWritingMode(),

Could you move this up a few lines to be alongside the start-of-reflow boilerplate?  This is a precondition, so it makes the most sense at the very beginning of the function.

In fact: since pretty much every Reflow method should have this, it'd be nice if we put it at a *consistent position* with respect to the other boilerplate (at least in cases where the other boilerplate is already somewhat consistent).

Maybe it should go right after DISPLAY_REFLOW? (I noticed you sort of standardized on that for some of the changes, and that seems fine.)  And for functions without that, like nsFrame::Reflow, just put it after the other code that normally comes alongside DISPLAY_REFLOW? (MarkInReflow/DO_GLOBAL_REFLOW_COUNT)

::: layout/generic/nsLeafFrame.cpp:79
(Diff revision 1)
>    NS_ASSERTION(NS_INTRINSICSIZE != aReflowInput.ComputedHeight(),
>                 "Shouldn't have unconstrained stuff here "
>                 "thanks to ComputeAutoSize");
>  
>    // XXX how should border&padding effect baseline alignment?
>    // => descent = borderPadding.bottom for example
>    WritingMode wm = aReflowInput.GetWritingMode();
>    aMetrics.SetSize(wm, aReflowInput.ComputedSizeWithBorderPadding());
>  
> -  aStatus.Reset();
> +  MOZ_ASSERT(aStatus.IsEmpty(), "Caller should pass a fresh reflow status!");

This should probably move up a bit to be alongside the other precondition-ish assertions.
Attachment #8907078 - Flags: review?(dholbert) → review-
Comment on attachment 8907077 [details]
Bug 1341009 - Pass const reference instead of value for nsReflowStatus.

https://reviewboard.mozilla.org/r/178778/#review183906

> Is this really necessary?  This is ReflowAbsoluteFrame, and the only caller declares |nsReflowStatus kidStatus;| right before passing it in.
> 
> So I think we're safe to assume aStatus is already blank here.  Resetting it is confusing because it implies that there's the possibility that it contains something that'd need to be cleared.

You're right. The only caller is `nsAbsoluteContainingBlock::Reflow`, which passes a new `nsReflowStatus` before passing it in.

> I'd rather not do this in ReflowChild (which is just a lightweight wrapper around aKidFrame->Reflow).
> 
> To the extent that we need this is necessary for your assertions in the next patch to pass: I'd rather we do the resetting in whichever callers are problematic.  (Or perhaps-better, make those callers use an dedicated local ReflowStatus which will be guaranteed to be empty.)

I'm considering making `nsContainerFrame::ReflowChild` and other reflow helperers return `nsReflowStatus` so that we don't need to worry about whether the status passing from caller is empty or not. Also, if the caller don't care about the status (such as `nsComboboxControlFrame::ReflowDropdown`), it doesn't need to create a dummy status.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/forms/nsComboboxControlFrame.cpp?q=nsComboboxControlFrame.cpp&redirect_type=direct#475-478
(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #10)
> I'm considering making `nsContainerFrame::ReflowChild` and other reflow
> helperers return `nsReflowStatus` so that we don't need to worry about
> whether the status passing from caller is empty or not. Also, if the caller
> don't care about the status (such as
> `nsComboboxControlFrame::ReflowDropdown`), it doesn't need to create a dummy
> status.

I give up making reflow helpers return nsReflowStatus because there are too many helpers to convert, and doing so makes the API inconsistent if we don't convert nsIFrame::Reflow() methods.
Comment on attachment 8907077 [details]
Bug 1341009 - Pass const reference instead of value for nsReflowStatus.

https://reviewboard.mozilla.org/r/178778/#review185584

::: commit-message-39897:5
(Diff revision 2)
> +Also, make nsBlockFrame::SplitFloat() return void because the only
> +caller (BlockReflowInput::FlowAndPlaceFloat()) doesn't check its return
> +value.

Please add at the end here:
 "and (more importantly) because it only ever returns NS_OK."

That's the more important justification for dropping its return value, IMO.   (The justification that you're currently citing -- the caller's failure to check the rv -- is a *hint* that it might be OK to remove the rv, but it's not really solid justification. For example, if the function *tried* to return failure codes and the caller just dropped them on the floor without checking them, that would suggest that the *caller* would need to be fixed. Fortunately that's not the case here.)
Attachment #8907077 - Flags: review?(dholbert) → review+
Comment on attachment 8907078 [details]
Bug 1341009 - Add nsReflowStatus::IsEmpty() assertion to nsAbsoluteContainingBlock::ReflowAbsoluteFrame().

https://reviewboard.mozilla.org/r/178780/#review185586
Attachment #8907078 - Flags: review?(dholbert) → review+
Comment on attachment 8907987 [details]
Bug 1341009 - Reset aStatus before calling nsLineLayout::ReflowFrame().

https://reviewboard.mozilla.org/r/179632/#review185596

r=me with nit:

::: layout/generic/nsInlineFrame.cpp:795
(Diff revision 1)
>    bool reflowingFirstLetter = lineLayout->GetFirstLetterStyleOK();
> +  nsReflowStatus reflowStatus;
>    bool pushedFrame;
> -  lineLayout->ReflowFrame(aFrame, aStatus, nullptr, pushedFrame);
> +  lineLayout->ReflowFrame(aFrame, reflowStatus, nullptr, pushedFrame);
> +  aStatus = reflowStatus;

Instead of adding this local variable, let's just do:
  aStatus.Reset();
...just before the call to lineLayout. That's a bit more direct.


(Note that this method already directly manipulates aStatus (including already having a "Reset()" call) further down. So we might as well stick with that, rather than introducing an extra nsReflowStatus which would create ambiguity about which variable the rest of the function should work with.)

This means the commit message will need to change a bit, too. (Right now it's "Pass a local...")
Attachment #8907987 - Flags: review?(dholbert) → review-
Comment on attachment 8907987 [details]
Bug 1341009 - Reset aStatus before calling nsLineLayout::ReflowFrame().

https://reviewboard.mozilla.org/r/179632/#review185596

(Er, sorry -- I initially had "r=me with nit", but then I realized I should probably call it "r-" since the whole (small) patch will change, including the commit message, so it's probably worth one more lookover. :)  And I just forgot to clear the r=me comment.)
Comment on attachment 8907988 [details]
Bug 1341009 - Reset aStatus before calling nsContainerFrame::ReflowChild().

https://reviewboard.mozilla.org/r/179634/#review185610

::: layout/generic/nsColumnSetFrame.cpp:806
(Diff revision 1)
> +      nsReflowStatus childStatus;
>        ReflowChild(child, PresContext(), kidDesiredSize, kidReflowInput,
> -                  wm, origin, containerSize, 0, aStatus);
> -
> +                  wm, origin, containerSize, 0, childStatus);
> +      aStatus = childStatus;

As with the previous patch: this extra local variable seems redundant (and it seems to introduce some ambiguity about what the difference is between aStatus vs. childStatus, and which variable the rest of this function should use.  (e.g. we have checks like "aStatus.IsFullyComplete()" a few lines down from this, which now look like it might be dealing with something other than this ReflowChild()'s output.)

So, I'd rather we not introduce this variable.

Here, it looks like aStatus is just a reflow status that we reuse for multiple reflows of the same child here (and we intend to stomp on with each reflow).  

Given that, I'd prefer we just explicitly call Reset() on it right before the ReflowChild() call.  That's not really destroying anything, because ReflowChild is already promising to stomp on whatever value was previously there anyway.

::: layout/tables/nsTableFrame.cpp:3388
(Diff revision 1)
>        LogicalPoint kidPosition(wm, aReflowInput.iCoord, aReflowInput.bCoord);
> +      nsReflowStatus childStatus;
>        ReflowChild(kidFrame, presContext, desiredSize, kidReflowInput,
> -                  wm, kidPosition, containerSize, 0, aStatus);
> +                  wm, kidPosition, containerSize, 0, childStatus);
> +      aStatus = childStatus;

Same here -- let's just call aStatus.Reset() before the ReflowChild call, for same reasons discussed above.
Attachment #8907988 - Flags: review?(dholbert) → review-
Sorry, I know comment 23 and comment 25 might seem to superficially contradict my earlier "dedicated local ReflowStatus" hypothetical-alternative-suggestion from comment 8.

I hope it doesn't seem like I'm reversing myself entirely. :)  Really, it's entirely possible that we *could* make the "dedicated local ReflowStatus" strategy make sense in these callers!  But that would require a more thorough refactoring of the surrounding code to actually *use* that reflow status when we're reasoning about the child's reflow.  We would want to differentiate between "the reflow status that was passed into my own Reflow method" vs. "the reflow status I'm using to reason about the reflow of this child".  We have that sort of differentiation in many of our container classes, and it's nice.  But right now, the patches here aren't setting up a meaningful differentiation like that, which is why I'd rather go with the simpler Reset() strategy.  The local-variable-refactoring might be worth it at some point but is probably beyond the scope of this bug.
Comment on attachment 8907989 [details]
Bug 1341009 - Add nsReflowStatus::IsEmpty() assertions to all nsIFrame::Reflow() methods and some reflow helpers, and remove unneeded Reset().

https://reviewboard.mozilla.org/r/179636/#review185622

Thank you for going through & fixing all of these up!  This is great.

r=me with the following nits:

::: layout/generic/nsFlexContainerFrame.cpp
(Diff revision 1)
> -  aStatus.Reset();
> -

This Reset() call is actually still needed, because nsFlexContainerFrame::Reflow can potentially make *two* calls to DoFlexLayout, reusing the same outparam.

So to fix up this code, could you please do the following:

 (1) Move this Reset() call up one level to be inside the "if (!struts.IsEmpty())" check in nsFlexContainerFrame::Reflow (just before the second DoFlexLayout call).

 (2) Add a copy of your standard aStatus.IsEmpty() assertion here -- at the beginning of DoFlexLayout -- to be sure that that we indeed don't need to bother with this Reset() here anymore.

::: layout/generic/nsGridContainerFrame.cpp:5669
(Diff revision 1)
>                                       const LogicalRect&   aContentArea,
>                                       ReflowOutput& aDesiredSize,
>                                       nsReflowStatus&      aStatus)
>  {
>    MOZ_ASSERT(aState.mReflowInput);
> +  MOZ_ASSERT(aStatus.IsEmpty(), "Caller should pass a fresh reflow status!");

This assertion seems valid here (in ReflowChildren), so that's good, but please *also* insert it in nsGridContainerFrame::Reflow.

(This patch doesn't touch nsGridContainerFrame::Reflow yet, but it should.)

::: layout/generic/nsInlineFrame.cpp
(Diff revision 1)
> -  aStatus.Reset();
> -

Could you add your standard aStatus.IsEmpty() assertion here ("Caller should pass..."), in place of the Reset that you're removing?

(...just in case this function ends up getting an optional second call in the future, kind of like how nsFlexContainerFrame::DoFlexReflow does.)

::: layout/generic/nsLeafFrame.cpp:52
(Diff revision 1)
>  nsLeafFrame::Reflow(nsPresContext* aPresContext,
>                      ReflowOutput& aMetrics,
>                      const ReflowInput& aReflowInput,
>                      nsReflowStatus& aStatus)
>  {
>    MarkInReflow();
>    DO_GLOBAL_REFLOW_COUNT("nsLeafFrame");
> +  MOZ_ASSERT(aStatus.IsEmpty(), "Caller should pass a fresh reflow status!");

I realized nsLeafFrame::Reflow and DoReflow are actually dead/unused, so I'm deleting them entirely in bug 1400421.  Assuming I'm not missing something over there, let's just revert your changes to this file (nsLeafFrame.cpp).  

(No sense in churning dead code; and it's pretty likely my tiny dead-code-removal patches will land before this mega-patch anyway.)
Attachment #8907989 - Flags: review?(dholbert) → review+
Comment on attachment 8907989 [details]
Bug 1341009 - Add nsReflowStatus::IsEmpty() assertions to all nsIFrame::Reflow() methods and some reflow helpers, and remove unneeded Reset().

https://reviewboard.mozilla.org/r/179636/#review185640

Sorry, I forgot about 2 commit message nits that I was going to make:

::: commit-message-849db:1
(Diff revision 1)
> +Bug 1341009 - Add IsEmpty() assertions to all nsIFrame::Reflow() methods, and remove unneeded Reset().

s/IsEmpty()/nsReflowStatus::IsEmpty()/

Otherwise it sounds like you're talking about some nsIFrame::IsEmpty API, and it's less clear why that hypothetical API would be a reasonable thing to assert about.

::: commit-message-849db:6
(Diff revision 1)
> +In the end of the Reflow(), if the Reset() is still needed because aStatus
> +is modified in the middle, add a comment like "This type of frame can't be
> +split." Otherwise, if aStatus is not modified, use IsEmpty() assertion to
> +ensure that.

The wording here is a little bit confusing.  How about the following (which is a little more verbose but gets the idea across more clearly I think):

A few Reflow() implementations have Reset() calls at the end which are left in place by this patch (with an explanatory comment added to each).  These ending Reset()s are only needed for cases where a non-splittable frame passes its own nsReflowStatus to a child's reflow method.  Just in case the child leaves a "not fully complete" value in the nsReflowStatus, the non-splittable parent frame must clear out the nsReflowStatus before returning, so that its own parent doesn't then try to split it.
Comment on attachment 8907990 [details]
Bug 1341009 - Convert nsReflowStatus::Reset() to nsReflowStatus::IsEmpty() assertion in ReflowText().

https://reviewboard.mozilla.org/r/179662/#review185696

r=me, just one commit message clarification:

::: commit-message-dd694:1
(Diff revision 1)
> +Bug 1341009 - Convert Reset() to IsEmpty() assertion in ReflowText().

s/Reset()/nsReflowStatus::Reset()/
Attachment #8907990 - Flags: review?(dholbert) → review+
Comment on attachment 8907989 [details]
Bug 1341009 - Add nsReflowStatus::IsEmpty() assertions to all nsIFrame::Reflow() methods and some reflow helpers, and remove unneeded Reset().

https://reviewboard.mozilla.org/r/179636/#review185622

> This assertion seems valid here (in ReflowChildren), so that's good, but please *also* insert it in nsGridContainerFrame::Reflow.
> 
> (This patch doesn't touch nsGridContainerFrame::Reflow yet, but it should.)

Yes. This patch should add assert to `nsGridContainerFrame::Reflow`. Thanks for catching this!
Comment on attachment 8907987 [details]
Bug 1341009 - Reset aStatus before calling nsLineLayout::ReflowFrame().

https://reviewboard.mozilla.org/r/179632/#review186084
Attachment #8907987 - Flags: review?(dholbert) → review+
Comment on attachment 8907988 [details]
Bug 1341009 - Reset aStatus before calling nsContainerFrame::ReflowChild().

https://reviewboard.mozilla.org/r/179634/#review186086
Attachment #8907988 - Flags: review?(dholbert) → review+
Looks like this is good to land, I think? \o/
Flags: needinfo?(tlin)
(In reply to Daniel Holbert [:dholbert] from comment #40)
> Looks like this is good to land, I think? \o/

Yep. Try results look good. Thank you for the review!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=44d37ebd0970
Flags: needinfo?(tlin)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 9acefd8ed7af -d 45461d47b376: rebasing 421178:9acefd8ed7af "Bug 1341009 - Remove nsReflowStatus::Reset() in BlockReflowInput's constructor. r=dholbert"
rebasing 421179:0254a128cd5a "Bug 1341009 - Pass const reference instead of value for nsReflowStatus. r=dholbert"
rebasing 421180:306efd765a87 "Bug 1341009 - Add nsReflowStatus::IsEmpty() assertion to nsAbsoluteContainingBlock::ReflowAbsoluteFrame(). r=dholbert"
rebasing 421181:f58fed3765bd "Bug 1341009 - Reset aStatus before calling nsLineLayout::ReflowFrame(). r=dholbert"
rebasing 421182:ea93a4d9aa29 "Bug 1341009 - Reset aStatus before calling nsContainerFrame::ReflowChild(). r=dholbert"
rebasing 421183:6177c295d2d1 "Bug 1341009 - Add nsReflowStatus::IsEmpty() assertions to all nsIFrame::Reflow() methods and some reflow helpers, and remove unneeded Reset(). r=dholbert"
merging layout/generic/nsRubyBaseContainerFrame.cpp
merging layout/generic/nsRubyFrame.cpp
merging layout/generic/nsRubyTextContainerFrame.cpp
merging layout/generic/nsTextFrame.cpp
warning: conflicts while merging layout/generic/nsRubyTextContainerFrame.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s dea11a10cd2b -d 93a1630a8911: rebasing 421179:dea11a10cd2b "Bug 1341009 - Remove nsReflowStatus::Reset() in BlockReflowInput's constructor. r=dholbert"
rebasing 421180:051f14b6fc35 "Bug 1341009 - Pass const reference instead of value for nsReflowStatus. r=dholbert"
rebasing 421181:8f156d933ea3 "Bug 1341009 - Add nsReflowStatus::IsEmpty() assertion to nsAbsoluteContainingBlock::ReflowAbsoluteFrame(). r=dholbert"
rebasing 421182:cfa146f1be18 "Bug 1341009 - Reset aStatus before calling nsLineLayout::ReflowFrame(). r=dholbert"
rebasing 421183:4152b68b00a9 "Bug 1341009 - Reset aStatus before calling nsContainerFrame::ReflowChild(). r=dholbert"
rebasing 421184:fb8f25b5b81a "Bug 1341009 - Add nsReflowStatus::IsEmpty() assertions to all nsIFrame::Reflow() methods and some reflow helpers, and remove unneeded Reset(). r=dholbert"
merging layout/generic/nsRubyBaseContainerFrame.cpp
merging layout/generic/nsRubyFrame.cpp
merging layout/generic/nsRubyTextContainerFrame.cpp
merging layout/generic/nsTextFrame.cpp
warning: conflicts while merging layout/generic/nsRubyTextContainerFrame.cpp! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by tlin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a6211a66f585
Remove nsReflowStatus::Reset() in BlockReflowInput's constructor. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/25757b404bb6
Pass const reference instead of value for nsReflowStatus. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/5b661188c603
Add nsReflowStatus::IsEmpty() assertion to nsAbsoluteContainingBlock::ReflowAbsoluteFrame(). r=dholbert
https://hg.mozilla.org/integration/autoland/rev/b500a58183a4
Reset aStatus before calling nsLineLayout::ReflowFrame(). r=dholbert
https://hg.mozilla.org/integration/autoland/rev/d85071e23dac
Reset aStatus before calling nsContainerFrame::ReflowChild(). r=dholbert
https://hg.mozilla.org/integration/autoland/rev/c0b4af15d17a
Add nsReflowStatus::IsEmpty() assertions to all nsIFrame::Reflow() methods and some reflow helpers, and remove unneeded Reset(). r=dholbert
https://hg.mozilla.org/integration/autoland/rev/f6c6f4f6bd34
Convert nsReflowStatus::Reset() to nsReflowStatus::IsEmpty() assertion in ReflowText(). r=dholbert
Depends on: 1401807
Depends on: 1405830
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: