580167 - Consider having a way to reflow text frames without creating a reflow state

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

nsTextFrameThebes::Reflow uses aReflowState for the following things:

1) Calling DISPLAY_REFLOW.
2) Getting the available width (and available height, but that's debug-only).
3) Getting the nsLineLayout.
4) Getting mFlags.mBlinks (which for textframes is guaranteed to be the same as
   for its parent reflow state).
5) Getting the rendering context.
6) Calling NS_FRAME_SET_TRUNCATION.

When called from nsLineLayout (the common case) we can pass in nsLineLayout directly.  We can pass in mBlinks directly, since for a textframe this is always the same as for its parent reflow state.  We can pass in the rendering context and available width directly (the latter would be an argument to the reflow state constructor anyway).

NS_FRAME_SET_TRUNCATION is a no-op if availableHeight == NS_UNCONSTRAINEDSIZE, which it always is inside nsLineLayout::ReflowFrame.

DISPLAY_REFLOW is something I would be willing to forgo for textframes, myself.

The reflow state is also passed to DidReflow, which for textframes only needs to clear the various state flags, since textframe reflow states should never have percent height observers.

So it seems we should be quite able to skip reflow state construction here.  Any reasons not to?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1. Add the new API

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2. Make it possible to lazily construct reflow states

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 3. The nsLineLayout changes

Most of this is just reordering things so I have as few branches on isText as possible.

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 458934 [details] [diff] [review]
Part 2.  Make it possible to lazily construct reflow states

>Bug 580167 part 2.  Add a construct() signature with 4 args for js::LazilyConstructred.  r=lw

You might want to drop the extra "r" in Constructed.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Er, yes.  Done.

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 458934 [details] [diff] [review]
Part 2.  Make it possible to lazily construct reflow states

>Bug 580167 part 2.  Add a construct() signature with 4 args for js::LazilyConstructred.  r=lw

You probably want to spell LazilyConstructed correctly in the commit message here :-)

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 458932 [details] [diff] [review]
Part 1.  Add the new API

Er, clearly I should look at what I've already written on the bug (re previous comment).

> NS_IMETHODIMP
> nsTextFrame::Reflow(nsPresContext*           aPresContext,
>                     nsHTMLReflowMetrics&     aMetrics,
>                     const nsHTMLReflowState& aReflowState,
>                     nsReflowStatus&          aStatus)
> {
>   DO_GLOBAL_REFLOW_COUNT("nsTextFrame");
>   DISPLAY_REFLOW(aPresContext, this, aReflowState, aMetrics, aStatus);
>+
>+  // XXX If there's no line layout, we shouldn't even have created this
>+  // frame. This may happen if, for example, this is text inside a table
>+  // but not inside a cell. For now, just don't reflow.
>+  if (!aReflowState.mLineLayout) {
>+    ClearMetrics(aMetrics);
>+    aStatus = NS_FRAME_COMPLETE;
>+    return NS_OK;
>+  }

Maybe at some point we should add an assertion for this case, though I suppose it doesn't *have* to be here.

That said, why would we hit cases where the reflow state has no line layout, but we still have Reflow called?  :first-letter or something?

>   if (!mTextRun) {
>     ClearMetrics(aMetrics);
>     aStatus = NS_FRAME_COMPLETE;
>-    return NS_OK;
>   }

Did you mean to change this to |return| rather than removing it entirely?  I think you should.


I tend to think ReflowText would probably be better off with a local presContext variable rather than calling PresContext() each time it needs one; PresContext() involves quite a few dereferences.  I'm ok either way, though.

r=dbaron with that

sorry for taking so long to get to this

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 458936 [details] [diff] [review]
Part 3.  The nsLineLayout changes

>   NS_WARN_IF_FALSE(psd->mRightEdge != NS_UNCONSTRAINEDSIZE,
>                    "have unconstrained width; this should only result from "
>                    "very large sizes, not attempts at intrinsic width "
>                    "calculation");

Could you keep this assertion with one of the pieces of code it was related to?

>-  aFrame->DidReflow(mPresContext, &reflowState, NS_FRAME_REFLOW_FINISHED);
>+  {
>+    nsHTMLReflowState* reflowState = isText ? nsnull: reflowStateHolder.addr();
>+    aFrame->DidReflow(mPresContext, reflowState, NS_FRAME_REFLOW_FINISHED);
>+  }

Missing a space after the :.

Also, I think it's cleaner to just write:
  aFrame->DidReflow(mPresContext,
                    isText ? nsnull : reflowStateHolder.addr(),
                    NS_FRAME_REFLOW_FINISHED);
than mess with the braces.

>     // See if we can place the frame. If we can't fit it, then we
>     // return now.
>     PRBool optionalBreakAfterFits;
>-    if (CanPlaceFrame(pfd, reflowState, notSafeToBreak, continuingTextRun,
>+    NS_ASSERTION(isText ||
>+                 reflowStateHolder.ref().mStyleDisplay->mFloats ==
>+                   NS_STYLE_FLOAT_NONE,

May as well use aFrame->GetStyleDisplay()->mFloats and not bother checking isText... at least unless you're working towards not using any style data at all for text frames, which I suppose may well be a noble goal.

>+    // Direction is inherited, so using the psd direction is fine.
>+    PRUint8 direction =
>+      isText ? psd->mReflowState->mStyleVisibility->mDirection :
>+               reflowStateHolder.ref().mStyleVisibility->mDirection;

Are you doing this to save the style data computation?  If so, comment to say so, since it could easily be simplified to aFrame->GetStyleVisibility()->mDirection.

r=dbaron

Again, sorry for taking so long here.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> at least unless you're working towards not using any style data at all for text
> frames

I wasn't; I was just preserving existing logic as much as possible.  You're right that we can just GetStyleDisplay here... but I'm not sure how we can not use style data for textframes without adding some sort of special cases to the GetStyle*() nsIFrame methods.  Unless you mean making it so that we don't get style structs for textframes on normal layout codepaths (but still provide them if someone asks)?

> Are you doing this to save the style data computation? 

No, again just preserving existing logic.  I'll use GetStyleVisibility.

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to comment #9)
> > at least unless you're working towards not using any style data at all for text
> > frames
> 
> I wasn't; I was just preserving existing logic as much as possible.  You're
> right that we can just GetStyleDisplay here... but I'm not sure how we can not
> use style data for textframes without adding some sort of special cases to the
> GetStyle*() nsIFrame methods.  Unless you mean making it so that we don't get
> style structs for textframes on normal layout codepaths (but still provide them
> if someone asks)?

Yes, that was what I was thinking... and it may well be worth doing.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> That said, why would we hit cases where the reflow state has no line layout,
> but we still have Reflow called? 

I don't know.  I suspect it can't happen, and would be happy to remove that block in a followup, but for now I was just keeping existing behavior.

> Did you mean to change this to |return| rather than removing it entirely?

Yes.  Fixed.

> I tend to think ReflowText would probably be better off with a local
> presContext variable

OK, done.  Though there were only two PresContext() callers, and one was the blink stuff.

> Could you keep this assertion with one of the pieces of code it was related
> to?

Done.

> Missing a space after the :.
> Also, I think it's cleaner to just write:

Both fixed.

> Are you doing this to save the style data computation? 

I was doing it as a way of preserving existing code as much as possible, but it does tie in well with not using style data on text frames.  I added a comment.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Roll-up for approval

Requesting approval.  This should be quite safe (just moves code around a little) and is a several-percent win for reflow in various cases.  It also moves us a little closer to the general architectural direction we want to move in for inline layout, I think.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 469121 [details] [diff] [review]
Roll-up for approval

a=sicking based on previous comment

Comment 14

[Boris Zbarsky [:bzbarsky]]

Pushed:
  http://hg.mozilla.org/mozilla-central/rev/c5edc32433b5
  http://hg.mozilla.org/mozilla-central/rev/93465a6ae10f
  http://hg.mozilla.org/mozilla-central/rev/c1ef7ef1bbec