1030993 - Implement reflow methods for ruby frame classes

Status: RESOLVED FIXED

(Core :: Layout, defect)

Comment 1

[sgbowen]

Attachment: ruby-reflow.patch v1

This is my first stab at some of the reflow code. It builds on top of the patches from bug 1021952. No continuations frames, line breaking, or whitespace handling yet, but there is some basic positioning for the annotations.

Comment 2

[sgbowen]

Attachment: ruby-reflow.patch v2

The ruby-pairing.patch from bug 1021952 is going away. Some changes have been made here to account for this. Instead of inserting anonymous (non-wrapper) ruby elements, they are assumed to exist in the reflow code.

Comment 3

[sgbowen]

Attachment: ruby-reflow.patch v3 (part 1)

Some updates to handle the fact that ruby text frames are now inline instead of block.

Comment 4

[sgbowen]

Attachment: ruby-autohiding.patch v1 (part 2)

Autohide annotations with the same content as their corresponding bases (section 2.4 of spec).

Comment 5

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

Comment on attachment 8450424 [details] [diff] [review]
ruby-reflow.patch v3 (part 1)

Sorry for the delay getting to this.  I've tried to go through somewhat
quickly with the goal of getting rough comments sooner rather than
fully detailed comments.

>+void
>+nsLineLayout::AdvanceICoord(nscoord aAmount) {
>+  mCurrentSpan->mICoord += aAmount;
>+}

One stylistic change, throughout the patch:

If an opening { would, if on its own line, be in the first column,
then it belongs on its own line.  (I believe this helps some tools
like editors; it's also consistent with the rest of our code.)

>+  nscoord GetCurrentMinBCoord();

This appears to be unused.  Remove it.


(TODO: I'm a little unsure about exposing AdvanceICoord and
GetCurrentICoord like this, but I'll come back to that later.)


TODO: Look into how nsRubyBaseContainerFrame manages mTextContainers,
and how nsRubyTextContainerFrame manages mBaseContainer.


The GetPrefWidth implementations seem somewhat wrong.  First of all,
ruby needs to participate in line breaking, which means it needs to
participate in the AddInlineMinWidth/AddInlinePrefWidth mechanism.
Its GetPrefWidth/GetMinWidth implementations should presumably be
the same as those for nsInlineFrame, which appears to use the default
ones from nsFrame without modification.  So I think you should drop
the GetPrefWidth implementations and instead implement AddInlineMinWidth
and AddInlinePrefWidth.  They'll need to be slightly more complicated
than the ones for nsInlineFrame because they'll need to account for the
spacing from both the text and the bases.  On the other hand, you might
even want to structure the code differently in some ways since the ruby
frame really only needs to traverse its bases-and-text once.

I'm guessing that the SetMinISize method on nsRubyBaseFrame is to set
the size that comes from the ruby text?  If so, maybe just call it
SetRubyTextSize (and mRubyTextSize)?


nsRubyBaseFrame::Reflow:

>+  if (!aReflowState.mLineLayout) {
>+    aStatus = NS_FRAME_COMPLETE;
>+    return;
>+  }

You should have an assertion in error cases like this.  (Same for
the other frame classes.)

>+  nsFrameList::Enumerator e(mFrames);
>+  while (!e.AtEnd()) {
>     ...
>+    e.Next();
>+  }

Probably best to use a for loop for this pattern.  (It's safer if a
continue gets inserted.)

>+    if (metrics.Height() > height)
>+      height = metrics.Height();

You'll need to replace this at some point with code that handles vertical
alignment, although maybe nsLineLayout will take care of that.

Put braces around the if-body, though.


(Also, you have trailing whitespace on the line before it.)

>+  if (width < mMinISize) {
>+    aDesiredSize.Width() = mMinISize;
>+  } else {
>+    aDesiredSize.Width() = width;
>+  }

use std::max().

This will also eventually need to handle vertical writing mode, though
that doesn't have to be now.

nsRubyFrame::Reflow:

>+  nscoord startEdge = 0;
>+  nscoord availableISize = aReflowState.AvailableISize();
>+  NS_ASSERTION(availableISize != NS_UNCONSTRAINEDSIZE,
>+               "should no longer use available widths");
>+  // Subtract off inline axis border+padding from availableISize
>+  availableISize -= startEdge;
>+  availableISize -= framePadding.IEnd(frameWM);

Not clear what you intended startEdge to be here.

Most of the comments from nsRubyBaseContainer::Reflow also apply here.

You should assert if you get an unexpected child frame type.  (Failing
to reflow a frame is bad.)

>+      while (!segment.AtEnd() && segment.get()->GetType() !=
>+             nsGkAtoms::rubyBaseContainerFrame) {

wrap after the && (which has lower precedence than the !=)

>+      FinishReflowChild(e.get(), aPresContext, metrics, &childReflowState, 0,
>+                        rtcFrame->mBaseContainer->GetLogicalPosition(this->
>+                        GetParent()->GetLogicalSize().ISize(lineWM)).B(lineWM)
>+                        - metrics.Height(), 0);

You should explain that three line math expression in there, possibly
by using well-named variables.

nsRubyTextContainerFrame::Reflow:

It seems unfortunate that this has so much code copied (I presume) from
nsBlockFrame.  Do you have any ideas on how to avoid that?

A ruby text container should never need a float manager; no need to call
BlockNeedsFloatManager.

>+  nscoord availBSize;
>+  if (state.GetFlag(BRS_UNCONSTRAINEDBSIZE)) {
>+    availBSize = NS_UNCONSTRAINEDSIZE;
>+  } else {
>+    availBSize = lineRect.BSize(lineWM);
>+  }

I think you should always use NS_UNCONSTRAINEDSIZE for availBSize, and
add a comment saying that we don't break ruby across columns or pages
since it's within a line.  (You also don't need to worry about isTopOfPage.)

I'm a little confused about why these differ from availSize, though; I'd
have expected availSize (below) to have the same values.  Or do they need
to be different in the presence of floats?  (If you're matching something
nsBlockFrame does, that's fine.)

nsRubyTextFrame::Reflow:

Mostly the same comments as the others (other than nsRubyTextContainerFrame).




When you revise the patch, it would be useful to add a description (probably
in an extended commit message in the manner of
https://hg.mozilla.org/mozilla-central/rev/830111e10951 ) explaining:
 (1) what the strategy for ensuring that the horizontal spacing accounts
     for both bases and text is:
     (a) in Reflow
     (b) in min width and pref width calculation
 (2) what the strategy for managing (and invalidating) the pointers between
     text containers and base containers is (although it might make sense
     for this to be in a separate patch from the reflow math)

Comment 6

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

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #5)
>  (2) what the strategy for managing (and invalidating) the pointers between
>      text containers and base containers is (although it might make sense
>      for this to be in a separate patch from the reflow math)

Some possible strategies include (but aren't limited to):

 A) Set up the pointers lazily at various points, and clear them out in response to things that might invalidate them (e.g., destruction or construction of any frames within the ruby structure).

 B) Set up the pointers at the start of nsRubyFrame::Reflow and null them all out at the end, so that they can be used only during reflow.  One disadvantage here is that you might also have to do the same for AddInlineMinWidth and AddInlinePrefWidth.  It's also not clear how this would get along with continuations, since there's no guarantee that all continuations get reflowed in sequence from start to end.

Comment 7

[sgbowen]

Attachment: ruby-reflow.patch v4 (part 1)

Revised according to the review, and ruby text frames are now reflowed from the ruby base container instead.

Comment 8

[sgbowen]

Attachment: ruby-reflow.patch v5 (part 1)

Please ignore the debug printf I accidentally left in the last version.

Comment 9

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

Comment on attachment 8464216 [details] [diff] [review]
ruby-reflow.patch v5 (part 1)

This is coming along well.  I have a lot of comments, but I expect I
should be able to give you review+ the next time around once you address
these.  There's still clearly work to be done in followup patches
(line breaking, cleaning up support for margin/border/padding/line-height/
vertical-align, etc.), but landing this first is good.  (I suppose I
might have some further comments on the high-level algorithms once I
have a little more time to think about them, but that's also something
that's fixable after landing if needed.)

Anyway:

I'm curious why you don't also need eLineParticipant and
FCDATA_IS_LINE_PARTICIPANT for ruby-base-container.  I guess it's
probably ok, though.

The two new nsLineLayout methods that operate on mCurrentSpan seem a
little dangerous since it's not really obvious to callers what
mCurrentSpan is.  At the very least they should be documented in
nsLineLayout.h.  I suspect you might also want assertions so that the
callers can check that mCurrentSpan is what they think it is.

nsRubyBaseContainerFrame:

The header file should document what text containers are, when they get
appended, and when they get cleared.

Also, you shouldn't have a space alone on a line on the line below the
Reflow method.

Please declare Reflow virtual and put "/* virtual */" on the
implementation.  (Yes, it's already virtual without it, but it's clearer
that way.)

nsRubyBaseContainerFrame::Reflow:

>+  int isize = 0;
>+  int bsize = 0;
>+  int baseNum = 0;
>+  int leftoverSpace = 0;
>+  int spaceApart = 0;

All except baseNum should be nscoord rather than int.

>+    if (e.get()->GetType() == nsGkAtoms::rubyBaseFrame) {

It seems less than ideal to skip over child frames without asserting
that they shouldn't be there.  If there's really an expectation that
there not be other child frames, then you should probably assert if
you find them.  If there isn't such an expectation, then you should
probably reflow and place them.  I'm not sure which is right.

However, if the first is true, then it's probably better to turn this
into a != with a continue, and then use less indentation for the rest
of the loop body.


As far as constructing the nsHTMLReflowMetrics goes -- you should see
what the resolution of bug 1048582 is.  (I filed that while doing this
review and trying to figure out whether your code is correct, which I'm
still not sure of.)

>+      int prefWidth = e.get()->GetPrefISize(aReflowState.rendContext);
>+      int textWidth = 0;
>+      int baseStart = 0;

Some comments here:

  1. you should add a "FIXME" comment pointing out that the use of
     GetPrefISize isn't really ideal, and it should really be using
     data resulting from reflow, which can differ in some edge cases.
     I don't think it's serious enough to need fixing right now, though.

  2. all 3 of these variables should be "nscoord" rather than "int".

  3. you shouldn't declare baseStart until first use (below).  (In
     general, variables should be declared as late as possible rather
     than using the C style/requirement of early declarations.)

  4. Now that you have an rbFrame variable, use it rather than calling
     e.get() more.  (Same thing lower down in the function, too.)

>+      e.get()->SetSize(nsSize(metrics.ISize(lineWM), metrics.BSize(lineWM)));

Given that you're starting from an isize and bsize, you should
construct an nsLogicalSize instead of an nsSize and pass that to the
other SetSize overload.  (I think.)


What's supposed to happen if a ruby text container has more children
than its corresponding ruby base container?  Your current patch, I
think, fails to reflow them at all, but that's probably not the expected
behavior?


>+            nsHTMLReflowState rtcReflowState(aPresContext, *aReflowState.parentReflowState, mTextContainers.ElementAt(i),
>+                                         availSize);

Fix the 80th column violation, and wrap the later lines to line
up with aPresContext.

>+            rtcFrame->ReflowRubyTextFrame(rtFrame, rbFrame, baseStart, aPresContext, rtcMetrics, rtcReflowState).ISize(lineWM);

80th column violation again (and elsewhere).

Also, the ".ISize(lineWM)" doesn't do anything; remove it.

And also make ReflowRubyTextFrame return void.  (Although the current
pattern does help with the FIXME above, so maybe it's ok to leave the
return value.  Probably not, though.)

>+          if (textContainersSeen <= i) {

The only differences between the if and else branches are:

 (1) the previous issue

 (2) declaration of rtcReflowStatus, which is normal an out-parameter
     from reflow, so it's not clear how it's useful to pass it in to
     BeginRTCLineLayout
 
 (3) incrementing textContainersSeen

 (4) the call to BeginRTCLineLayout.

 (5) unnecessary differences between use of rtcFrame vs.
     mTextContainers.ElementAt(i), which are the same.

It seems like most of the if/else should be unified, and there should
just be an if to call BeginRTCLineLayout.

You should just skip the IsFrameTreeTooDeep check in BeginRTCLineLayout
and remove its aStatus parameter.  It's not necessary since ruby structures
can't be nested to arbitrary depth.  You also don't handle a false result
in a reasonable way.


(twice, currently)
>+            rtcReflowState.mLineLayout = aReflowState.mLineLayout;

Shouldn't you be using the nsLineLayout that you construct in
BeginRTCLineLayout?  Or does it get used some other way?

nsRubyBaseFrame.h:

SetRubyTextISize and mRubyTextISize are unused; remove them.  (Or
use them if you meant to!)

Please declare Reflow and IsFrameOfType virtual and put "/* virtual */"
on the implementation.  (Yes, it's already virtual without it, but it's
clearer that way.)  Same for all the other frame classes (I'll stop
mentioning it each time.)

nsRubyBaseFrame.cpp:

In AddInlineMinISize and AddInlinePrefISize, I don't think you should
set aData->lineContainer; that should be the block that contains the
line.

In the same two functions, you can move the declaration of e into the
declaration-part of the for().

In IsFrameOfType, I suspect based on
http://lists.w3.org/Archives/Public/www-style/2014Jul/0191.html
should not claim to be nsIFrame::eBidiInlineContainer.  So I think
you should remove that and just leave eLineParticipant.  Same for
other frame classes.

nsRubyBaseFrame::Reflow:

>+  mozilla::LogicalSize availSize(lineWM, aReflowState.AvailableWidth(), aReflowState.AvailableHeight());

Wrap before the 80th column, and line up with "lineWM".

isize and bsize should be nscoord.

And e should be declared in the for() declaration-part.

>+    e.get()->Reflow(aPresContext, metrics, childReflowState, frameReflowStatus);

You should assert after this that frameReflowStatus is
NS_FRAME_COMPLETE.  (Then when you do line breaking you can remove
the assertion.)  Same in other calls to Reflow in other files.

>+    if (metrics.BSize(lineWM) > bsize) {
>+      bsize = metrics.BSize(lineWM);
>+    }

Given that Ruby should behave like inlines, the size should be set
as a function of the font metrics.  See the last 20 lines or so of
nsInlineFrame::ReflowFrames for how to do this.  This applies to
nsRubyBaseContainerFrame as well.  (It might be worth factoring that
chunk of code into a function on nsLayoutUtils, even.)

(You can drop the bsize variable and the code that updates it.)


You should also pass NS_FRAME_NO_MOVE_VIEW |'ed with
NS_FRAME_NO_MOVE_FRAME to FinishReflowChild.  Same for
all the other classes where you pass NS_FRAME_NO_MOVE_FRAME.
(Then the positions passed to FinishReflowChild
are ignored.)

You should pass an nsLogicalSize to SetSize, just like I described
for nsRubyBaseContainerFrame.


nsRubyFrame.h:

CalculateColSizes should take nsTArray<nscoord> rather than
nsTArray<int>.  And I'd prefer the parameters in the other order,
since we tend to put inputs before outputs.

nsRubyFrame.cpp:

CalculateColSizes:

It would probably be nice to put e.get() and annotations.get() in
local variables.

>+        } else if (annotations.AtEnd() && annotationNum < baseCount - 1) {

annotations.AtEnd() can't happen given your for loop condition.

AddInline*ISize:

>+  this->CalculateColSizes(colSizes, aRenderingContext);

No need for "this->".

max and sum should be nscoord rather than int.

You should use += rather than = to modify aData->currentLine.

And you should have a FIXME comment in AddInlineMinISize saying that the code
needs to handle the ability of ruby to break.

(Is it possible to have a forced break like <br> inside ruby?  If so,
AddInlinePrefISize also needs to handle it.)

nsRubyFrame::Reflow:

>+  LogicalMargin framePadding = aReflowState.ComputedLogicalBorderPadding();

call this "borderPadding" instead.  (Since sometimes we have padding in
a variable and sometimes we have border+padding in a variable, so we
should distinguish.)

>+  // Subtract off inline axis border+padding from availableISize
>+  availableISize -= framePadding.IEnd(frameWM);

The comment sugests you should be subtracting IStartEnd() rather than
just IEnd().  Is there a reason you only subtract one side (if so,
maybe explain in comment), or should you subtract both?

>+  aReflowState.mLineLayout->BeginSpan(this, &aReflowState, 0, availableISize,
>+                                      &baseline);

Shouldn't the 0 here be framePadding.IStart(frameWM)?

>+  //TODO: line breaking / continuations not yet implemented

Please use FIXME instead of TODO, and put a space before it.

+  mozilla::LogicalSize availSize(lineWM, aReflowState.AvailableISize(),
+                   aReflowState.AvailableBSize());

Please line up aReflowState.AvailableBSize() with lineWM.

>+  nsFrameList::Enumerator e(mFrames);

Please put this in the for loop initializer.

>+  int baseBSize = 0;
>+  int annotationBSize = 0;

These should be nscoord.

Though, actually, the same comments as before apply.  These should just
go away and height of the ruby frame should just be determined from the
font and border+padding.

>+      NS_ASSERTION(
>+        false, 
>+        "Unrecognized child type for ruby frame will not be reflowed."); 

NS_NOTREACHED(...) is equivalent to NS_ASSERTION(false, ...)

Please put e.get() in a local variable.

Same comments about both SetSize calls using an nsLogicalSize.

>+        baseContainerBCoord = segmentRBC->
>+        GetLogicalPosition(this->GetParent()->GetLogicalSize().ISize(lineWM)).
>+        B(lineWM);

Please indent the second and third lines by 2 more spaces.

Also maybe explain what you're doing in a comment; I don't follow the
logic.

>+      FinishReflowChild(e.get(), aPresContext, metrics, &childReflowState, 0,
>+                        baseContainerBCoord - metrics.BSize(lineWM), 0);

I'm not sure this position calculation will be sufficient once
vertical alignment is supported, but it's probably ok for now.  Maybe
add a FIXME comment, though?  Or is that what your TODO is?

>+    if (e.get()->GetType() == nsGkAtoms::rubyBaseContainerFrame) {
>+      nsRubyBaseContainerFrame* rbcFrame = do_QueryFrame(e.get());

Given that GetType is virtual, I don't think it's worth checking
GetType before calling do_QueryFrame; they have similar cost.  So
just call do_QueryFrame and null-check instead.

nsRubyTextContainerFrame.h:

Use mozilla::UniquePtr instead of nsAutoPtr.

>+  //void SetBaseContainer(nsRubyBaseContainerFrame* aFrame);

remove this

Fix the wrapping in the declaration of ReflowRubyTextFrame, probably like:

  mozilla::LogicalSize
  ReflowRubyTextFrame(nsRubyTextFrame* rtFrame, ...
                      ...);

and also name the arguments beginning with a (aRTFrame, etc.).

>+  friend class nsRubyFrame;

Why is this needed?  Could it be avoided?

>+  //nsRubyBaseContainerFrame* mBaseContainer;

remove this

>+  nsAutoPtr<nsLineLayout> mLineLayout;

Explain that this lives only during reflow.  (That said -- what
destroys it?  We shouldn't let it live to the next reflow.)

>+  nscoord mBSize;
>+  nscoord mISize;

Explain what these are and when they're set.

nsRubyTextContainerFrame::BeginRTCLineLayout:

Please line up the function arguments with the (.

as I said before, remove the IsFrameTreeTooDeep check and the aStatus.

Drop the "reflowState" local and just use aReflowState.

>+  bool blockStartMarginRoot, blockEndMarginRoot;
>+  IsMarginRoot(&blockStartMarginRoot, &blockEndMarginRoot);

You shouldn't need this, given the limited use of actual block features.
Probably best to treat both as true.  (You may also want to set
the NS_BLOCK_MARGIN_ROOT bit on the frame, actually.)

>+  mLineLayout = new
>+  nsLineLayout(state.mPresContext,
>+                          state.mReflowState.mFloatManager,
>+                          &state.mReflowState, &firstLine);

The second line should definitely be indented, or, better, just on the
first.

>+  line_iterator firstLine = begin_lines();

Does something in frame construction guarantee that there's at least
one line?  You should probably assert it, though, since you're assuming
it.

>+  nsFlowAreaRect floatAvailableSpace = state.GetFloatAvailableSpace();

You should be able to assume that you don't have floats laying out
within the ruby, since they'd float to outside of it.  So just use the
available width and (I think) an unconstrained height where you have
floatAvailableSpace.mRect, and false for mHasFloats.

>+  nscoord availBSize;
>+  availBSize = NS_UNCONSTRAINEDSIZE;

One line.

nsRubyTextContainerFrame::ReflowRubyTextFrame:

Fix the indentation of the arguments.

>+  nscoord ICoord = baseCenter - rtFrame->GetPrefISize(aReflowState.rendContext) / 2;

Add a FIXME here that it should avoid using GetPrefISize and use real
data from reflow.

>+  bool pushedFrame;
>+  mLineLayout->ReflowFrame(rtFrame, frameReflowStatus,
>+                         &metrics, pushedFrame);

Fix indentation of arguments.

Assert that pushedFrame is false (with assertion text that breaking
isn't yet implemented).

>+  if (metrics.BSize(lineWM) > mBSize)
>+    mBSize = metrics.BSize(lineWM);

Again, you shouldn't compute the BSize from the children; it should
come from the font metrics.  This should save you the mBSize member.

>+  rtFrame->SetSize(nsSize(metrics.ISize(lineWM), metrics.BSize(lineWM)));

Again, nsLogicalSize.

(Probably the same one you're going to return, too, so you can share it.)

nsRubyTextContainerFrame::Reflow:

As I said before, should this set mLineLayout to null (deleting it)?

>+  //TODO: Deal with multiple lines

I don't think you ever want to; you should just assert that there's
only one line.

>+  mLines.begin().get()->SetLogicalAscent(mBSize);

This probably isn't right, but it could be a FIXME associated with
implementing vertical alignment correctly.

All the BSize data should come from the font, not the children.

nsRubyTextFrame.h:

Same comments about virtual on Reflow and IsFrameOfType.

Why is the CanContinueTextRun declaration commented out?  If it's
needed, explain why.

Just remove the commented out GetPrefWidth declaration.

nsRubyTextFrame.cpp:

IsFrameOfType:  again, remove BidiInlineContainer

AddInline{Min,Pref}ISize:

again, don't set aData->lineContainer, and move the declaration of e
into the for expression.

nsRubyTextFrame::Reflow:

Again, make isize an nscoord rather than an int, and remove bsize,
and have a local variable for e.get().

>+    e.get()->SetSize(nsSize(metrics.ISize(lineWM), metrics.BSize(lineWM)));

Again, nsLogicalSize.

>+  aDesiredSize.BSize(lineWM) = bsize;

Again, use a height from the font.


nsTextFrame.cpp:

The CanTextCrossFrameBoundary seem a little suspicious to me.  It seems
to me like text runs should not cross between ruby-text frames since
ruby-text frames are positioned separately.  (In other words, shaped
Arabic as ruby text should use initial/final forms at the edges of each
ruby text, we should not use ligatures if one ruby text ends in an f and
the next starts with an i, etc.)  Is that what these changes are doing, or
am I misunderstanding them?

Then again, I'm not sure if this is the desired behavior.  I posted
http://lists.w3.org/Archives/Public/www-style/2014Aug/0049.html

(If they were to get around an assertion, would implementing
CanContinueTextRun on various frame classes also do that?)

>+                  (aLineContainer->GetType() == nsGkAtoms::rubyTextContainerFrame) ||

Drop both the added ( and the added ).

Comment 10

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

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #9)
> nsLineLayout.h.  I suspect you might also want assertions so that the
> callers can check that mCurrentSpan is what they think it is.

Though given that both callers are right before a ReflowFrame, I'm ok skipping these assertions.

Comment 11

[sgbowen]

Attachment: ruby-reflow.patch diff1 v1 (part 1b)

This patch goes on top of the ruby-reflow.patch and should address everything from your review.

Some lingering comments:

Ruby base containers are not line participants because they aren't reflowed using a line layout.

The bit I added to CanContinueTextRun was indeed to ensure the ruby annotations did not continue between boxes. It was the result of an in-depth discussion with roc, I believe (though it may have been someone else).

I'm going to continue using the parent reflow state, since that seems to be mostly the consensus on bug 1048582.

Let me know if I missed anything.

Comment 12

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

Comment on attachment 8468850 [details] [diff] [review]
ruby-reflow.patch diff1 v1 (part 1b)

This is close, but unfortunately I think we still need one more round.  Comments follow, including repeating a bunch of missed comments from comment 9.


nsRubyBaseContainerFrame.h:

>+  /* The ruby text containers that belong to the ruby segment defined by
>+   * this ruby base container, These text containers are located at the start of
>+   * reflow for the ruby frame (parent) and destroyed at the end of that reflow.
>+   */

Please wrap at 72-78 characters, and also use a blank line at the start:
 /*
  * comment starts here

The first , should be a .

And I'd also change "destroyed" to "cleared" since the pointers are
nulled out but the frames aren't destroyed.

nsRubyBaseContainerFrame.h, from last time:
>Also, you shouldn't have a space alone on a line on the line below the
>Reflow method.

>+  virtual void Reflow(nsPresContext* aPresContext,
>               nsHTMLReflowMetrics& aDesiredSize,
>               const nsHTMLReflowState& aReflowState,
>               nsReflowStatus& aStatus) MOZ_OVERRIDE;

Please line up the parameters.

>+  nscoord baseStart = 0;

Shouldn't this be declared inside the loop, at first use?

>+    e.get()->SetSize(mozilla::LogicalSize(lineWM, metrics.ISize(lineWM),
...
>       FinishReflowChild(e.get(), aPresContext, metrics, &aReflowState, 0, 0,

Use rbFrame here (twice) instead of calling e.get() again.

>+        if (textContainersSeen <= i) {
>+          rtcFrame->BeginRTCLineLayout(aPresContext, rtcMetrics,
>+                                       rtcReflowState);
>+          textContainersSeen++;
>         }

Maybe you should assert inside this if that textContainersSeen == i?

(twice)

And, actually, now that I think about it, this code doesn't actually
work as needed.  In particular, if you have a ruby with two rtcs, the
first of which is empty, you'll call BeginRTCLineLayout twice on the
second one (assuming it has two children) but never call it on the first.

Instead, I think you should just call BeginRTCLineLayout eagerly on all
the rtcs before you start looking at the rbs.

From comment 9:
>Shouldn't you be using the nsLineLayout that you construct in
>BeginRTCLineLayout?  Or does it get used some other way?

nsRubyBaseFrame.cpp:

I suspect it's not possible for frameWM and lineWM to be different in
ways that matter since we'd create a block for a change between horizontal
and vertical writing, but I guess it's ok to have both.

nsRubyFrame.h:

CalculateColSizes's parameter name should be aColSizes rather than colSizes.
(.cpp too)

The arguments to Reflow should be lined up.

nsRubyFrame.cpp:

from comment 9:
>+  mozilla::LogicalSize availSize(lineWM, aReflowState.AvailableISize(),
>+                   aReflowState.AvailableBSize());
>
>Please line up aReflowState.AvailableBSize() with lineWM.

>+      childFrame->SetSize(mozilla::LogicalSize(lineWM, metrics.ISize(lineWM),
>+            metrics.BSize(lineWM)));

Please line up metrics.BSize with lineWM.

>   nsFrameList::Enumerator children(mFrames);
>+  for (; !children.AtEnd(); children.Next()) {

Please put children in the for-loop initializer.  (It avoids bugs like
the one you're fixing here!)

From comment 9:
>>+    if (e.get()->GetType() == nsGkAtoms::rubyBaseContainerFrame) {
>>+      nsRubyBaseContainerFrame* rbcFrame = do_QueryFrame(e.get());
>
>Given that GetType is virtual, I don't think it's worth checking
>GetType before calling do_QueryFrame; they have similar cost.  So
>just call do_QueryFrame and null-check instead.


nsRubyTextContainerFrame.h:

Please line up the arguments to Reflow.

You should remove mBSize; it's no longer used, except for one
use that's overwritten on the next line.

nsRubyTextContainerFrame.cpp:

nsRubyTextContainerFrame::Reflow still needs to set mLineLayout to
null at the end.  (Otherwise the comment in the header isn't true.)

>+  mLineLayout.reset(new nsLineLayout(state.mPresContext,
>+                                     state.mReflowState.mFloatManager,
>+                                     &state.mReflowState, &firstLine));

Prefer this form (see the comments in UniquePtr.h):
  mLineLayout = MakeUnique<nsLineLayout>(state.mPresContext,
                                         state.mReflowState.mFloatManager,
                                         &state.mReflowState, &firstLine);

>   nsFlowAreaRect floatAvailableSpace = state.GetFloatAvailableSpace();

remove this line


>+  LogicalRect lineRect(lineWM, 
>+                       nsRect(nsPoint(borderPadding.left, borderPadding.top),
>+                              nsSize(aReflowState.AvailableWidth(),
>+                                     NS_UNCONSTRAINEDSIZE)),
>                        state.mContainerWidth);

I should have figured this out earlier, but I think there's an
easier way, which is:
  LogicalRect lineRect(state.mContentArea);
since state.mContentArea should already be what you want.


nsRubyTextContainerFrame::ReflowRubyTextFrame:

from comment 9, three things:

>Fix the indentation of the arguments.
>
>>+  nscoord ICoord = baseCenter - rtFrame->GetPrefISize(aReflowState.rendContext) / 2;
>
>Add a FIXME here that it should avoid using GetPrefISize and use real
>data from reflow.
>
>>+  bool pushedFrame;
>>+  mLineLayout->ReflowFrame(rtFrame, frameReflowStatus,
>>+                         &metrics, pushedFrame);
>
>Fix indentation of arguments.

>+  //if (metrics.BSize(lineWM) > mBSize)
>+    //mBSize = metrics.BSize(lineWM);

remove, don't comment out

from comment 9, 2 more things:

>nsRubyTextContainerFrame::Reflow:
>
>As I said before, should this set mLineLayout to null (deleting it)?
>
>>+  //TODO: Deal with multiple lines
>
>I don't think you ever want to; you should just assert that there's
>only one line.

>+  nscoord bsize = aDesiredSize.BSize(lineWM);
>+  mLines.begin().get()->SetLogicalAscent(bsize);

Instead of this, do:
  mLines.begin()->SetLogicalAscent(metrics.BlockStartAscent());
since nsLayoutUtils::SetBSizeFromFontMetrics has just set
BlockStartAscent() correctly (which is likely worth pointing out in
a comment).

>+  mLines.begin().get()->SetBounds(aReflowState.GetWritingMode(), 0, 0, mISize,
>+                                  bsize, mISize);

Drop the ".get()" and fix the indent on the following line to match.

nsRubyTextFrame.h:

>+  virtual void Reflow(nsPresContext* aPresContext,
>               nsHTMLReflowMetrics& aDesiredSize,
>               const nsHTMLReflowState& aReflowState,
>               nsReflowStatus& aStatus) MOZ_OVERRIDE;

Fix the indentation of the arguments.

nsRubyTextFrame.cpp:

from comment 9:
>AddInline{Min,Pref}ISize:
>
>again, don't set aData->lineContainer, and move the declaration of e
>into the for expression.

>+    e.get()->SetSize(mozilla::LogicalSize(lineWM, metrics.ISize(lineWM),
>+                     metrics.BSize(lineWM)));

Line up metrics.BSize() with lineWM.

>+  nsLayoutUtils::SetBSizeFromFontMetrics(this, aDesiredSize, aReflowState,
>+                                      borderPadding, lineWM, frameWM);

Line up borderPadding with this.

Comment 13

[sgbowen]

Attachment: ruby-reflow-diff-2.patch

All right, hopefully we got everything this time!

I'm still seeing some mochitest and assertions failures though, so I'll figure out what those are about.

A couple comments:
I didn't move baseStart into the loop to the first use, because I also use it outside the loop.

I think setting the line layout was only necessary for beginRTCLineLayout and not for ReflowRubyTextFrame, so I've removed it from before those calls.

Comment 14

[sgbowen]

Attachment: ruby-reflow-diff-2.patch v2

All right, assertion failures are gone now.

Here's the try run for these patches (successful except for a timeout, which I think will be fine once the retrigger finishes):
https://tbpl.mozilla.org/?tree=Try&rev=9408e411b4b6

Comment 15

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

Comment on attachment 8470456 [details] [diff] [review]
ruby-reflow-diff-2.patch v2

nsRubyBaseContainerFrame.cpp:

Instead of switching to use mozilla::WritingMode, please add
"using namespace mozilla;" at the top of the file (below the list
of #includes).

>+  // Begin the line layout for each ruby text container in advance.
>+  for (uint32_t i = 0; i < mTextContainers.Length(); i++) {
>+      nsRubyTextContainerFrame* rtcFrame = 
>+        do_QueryFrame(mTextContainers.ElementAt(i));
>+      nsHTMLReflowMetrics rtcMetrics(*aReflowState.parentReflowState,
>+                                     aDesiredSize.mFlags);
>+      nsHTMLReflowState rtcReflowState(aPresContext,
>+                                       *aReflowState.parentReflowState,
>+                                       rtcFrame, availSize);
>+      rtcReflowState.mLineLayout = aReflowState.mLineLayout;
>+      rtcFrame->BeginRTCLineLayout(aPresContext, rtcMetrics,
>+                                   rtcReflowState);
>+  }

Two space indent, not 4.

Also, please remove the aDesiredSize parameter from BeginRTCLineLayout
and don't construct rtcMetrics here.

(In the long run it would be good to avoid the rtcReflowState argument
as well.)

Also, could you make AppendTextContainer / mTextContainers deal
with nsRubyTextContainerFrame*, and drop the do_QueryFrame here
and below?

nsRubyBaseFrame.cpp:

Same thing about mozilla:: and using namespace.

>+  nscoord baseline;
...
>+  aReflowState.mLineLayout->BeginSpan(this, &aReflowState,
>+                                      borderPadding.IStart(frameWM),
>+                                      availableISize, &baseline);

This is very bad; it tells the line layout to fill in a stack pointer
(baseline) when it does vertical alignment, which is going to happen
after this function returns.  So you'll end up overwriting a random
word on the stack.

Instead, you should (just like nsInlineFrame):
 (1) add an mBaseline member variable
 (2) add a GetLogicalBaseline member function returning it, with
     MOZ_OVERRIDE
 (3) pass &mBaseline instead of &baseline
 (4) remove the baseline variable


>     nsHTMLReflowState childReflowState(aPresContext, aReflowState, e.get(),
>                                        availSize);
> 
>-    e.get()->Reflow(aPresContext, metrics, childReflowState, frameReflowStatus);
>+    bool pushedFrame;
>+    //e.get()->Reflow(aPresContext, metrics, childReflowState, frameReflowStatus);
>+    aReflowState.mLineLayout->ReflowFrame(e.get(), frameReflowStatus,
>+                                          &metrics, pushedFrame);

Please:
 (1) remove the childReflowState variable since you no longer use it
 (2) remove the commented out line
 (3) assert that pushedFrame is false after the call to ReflowFrame


>+  //aDesiredSize.ISize(lineWM) = isize;
>+  aDesiredSize.ISize(lineWM) = aReflowState.mLineLayout->EndSpan(this);

Please:
 (1) remove the commented out line
 (2) remove the isize variable and the code that maintains it.

nsRubyFrame.cpp:

same comment about mozilla::{WritingMode,LogicalMargin} and using namespace

>+    if (rbcFrame != nullptr) {

Just use "if (rbcframe) {"

nsRubyTextContainerFrame.cpp:

same comment about mozilla:: and "using namespace".

nsRubyTetxFrame.cpp:

Again (third time), don't seet aData->lineContainer in either
AddInlineMinISize or AddInlinePrefISize.

Also, same comment about "using namespace mozilla" and dropping the
mozilla::.

>+  nscoord baseline;
...
>+  aReflowState.mLineLayout->BeginSpan(this, &aReflowState,
>+                                      borderPadding.IStart(frameWM),
>+                                      availableISize, &baseline);

Same 4 comments here as for nsRubyBaseFrame.


>     nsHTMLReflowState childReflowState(aPresContext, aReflowState, e.get(),
>                                        availSize);
> 
>-    e.get()->Reflow(aPresContext, metrics, childReflowState, frameReflowStatus);
>+    bool pushedFrame;
>+    aReflowState.mLineLayout->ReflowFrame(e.get(), frameReflowStatus,
>+                                          &metrics, pushedFrame);

Like nsRubyBaseFrame, remove childReflowState.


And like nsRubyBaseFrame, remove the isize variable and the code
that maintains it.


r=dbaron with those comments addressed, on the three patches squashed
together

Comment 16

[Daniel Holbert [:dholbert]]

Can this bug include some basic reftests, too? (I suspect that once we have reflow methods, that means we can start to render things & test our rendering.)

Comment 17

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

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #15)
> (In the long run it would be good to avoid the rtcReflowState argument
> as well.)

Could you make this a FIXME comment, actually.

> Please:
>  (1) remove the childReflowState variable since you no longer use it
>  (2) remove the commented out line
>  (3) assert that pushedFrame is false after the call to ReflowFrame

And, per IRC discussion, also (4) remove the FinishReflowChild call, since nsLineLayout::ReflowFrame does the equivalent (calls DidReflow and sets size).

Comment 18

[sgbowen]

Attachment: ruby-reflow.patch v6 (part 1)

K, those changes are made and have been folded into one patch.

At this point, let's disregard the ruby-autohiding.patch and migrate it to a different bug (marking it as obsolete now).

Here's a try run for the reflow patch after those updates. I'd expect it to go all right, since there were no major changes:
https://tbpl.mozilla.org/?tree=Try&rev=15bde4af343d

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8471084 [details] [diff] [review]
ruby-reflow.patch v6 (part 1)

>diff --git a/layout/base/nsLayoutUtils.h b/layout/base/nsLayoutUtils.h
>--- a/layout/base/nsLayoutUtils.h
>+++ b/layout/base/nsLayoutUtils.h
>+#include "nsHTMLReflowMetrics.h"
> 
> #include <limits>
> #include <algorithm>
> 
> class nsIFormControlFrame;
> class nsPresContext;
[...]
>+  static void SetBSizeFromFontMetrics(const nsIFrame* aFrame,
>+                                      nsHTMLReflowMetrics& aMetrics,

Minor nit here: We probably should move the #include for nsHTMLReflowMetrics to the nsLayoutUtils.cpp file, and use a forward-declaration in the .h file here. (Like we have for e.g. nsPresContext.)

Since the .h file only mentions "nsHTMLReflowMetrics&" and doesn't try to use any methods on nsHTMLReflowMetrics, that should be fine with the compiler.

This is sort of important because a *lot* of files #include nsLayoutUtils.h, so it's good to keep its #include-count as low as possible. (In this case, it'll mean that modifications to nsHTMLReflowMetrics.h won't require recompiling the world, or at least not as much.)

Comment 20

[sgbowen]

Attachment: ruby-whitespace-reftests.patch

These are the reftests for whitespace from bug 1039017. I'll have another patch with some basic reflow tests up shortly.

Comment 21

[sgbowen]

Attachment: ruby-reftest-fix.patch

This is a patch to fix an occasional assertion failure as described in bug 1052924.

It also updates the reftest assertion count, and changes the first test to expected failure, since I'm still figuring out some unexplained rendering behavior in the presence of whitespace. To be clear, the problem I'm seeing looks to be the ruby-reflow.patch not always handling ruby boxes with only whitespace correctly. According to the frame dumps, the whitespace itself is handled correctly (dropped or wrapped in the appropriate ruby element), but its containing frames may not be reflowed properly.

The assertion count was /increased/ because a different assertion is failing 7 times, and this still needs to be addressed. I.e., the assertion count was 1 originally (and not 8) because I was running the reftests without the reflow patch applied.

Comment 23

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

Comment on attachment 8471986 [details] [diff] [review]
ruby-reftest-fix.patch

> static nsIFrame*
> FindLineContainer(nsIFrame* aFrame)
> {
>-  while (aFrame && aFrame->CanContinueTextRun()) {
>+  while (aFrame && aFrame->IsFrameOfType(nsIFrame::eLineParticipant)) {
>     aFrame = aFrame->GetParent();
>   }
>   return aFrame;
> }

I think this will cause assertions in nsTextFrame::AddInline{Min,Pref}ISize when called on a text frame inside of a floating first letter.  I think it should instead be:

  while (aFrame && (aFrame->IsFrameOfType(nsIFrame::eLineParticipant) ||
                    aFrame->CanContinueTextRun()) {

r=dbaron with that, or without it if I'm wrong

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 8471986 [details] [diff] [review]
ruby-reftest-fix.patch

># HG changeset patch
># Parent 6dfa2d1996c3332fc87b688ca13eb61687333929
># User Susanna Bowen <[e-mail]>
>Bug 1052145 - Fix assertion failure in reftest css-ruby/ruby-whitespace-1.html. r=dbaron

Nit: The bug number here needs updating (probably wants to say "Bug 1030993 part 2"), and the commit message needs tweaking to describe what's actually changing code-wise.

Comment 26

[sgbowen]

There's a memory leak on one of the mochitests which I'm looking into right now. The problem seems to be in ruby-reftest-fix.patch. I'll have those commit message changes up when I find the problem and revise the patch (hopefully soon).

In the meantime, here's the try run that uncovered the memory leak:
https://tbpl.mozilla.org/?tree=Try&rev=27d1b4f60785

Comment 27

[Daniel Holbert [:dholbert]]

Comment on attachment 8471986 [details] [diff] [review]
ruby-reftest-fix.patch

>+++ b/layout/generic/nsTextFrame.cpp
>@@ -1117,40 +1117,30 @@ CanTextCrossFrameBoundary(nsIFrame* aFra
>-    if (continuesTextRun) {
>+    if (aFrame->IsFrameOfType(nsIFrame::eLineParticipant)) {

This ^ one-liner change, on its own (directly applied to m-c) is sufficient to introduce the nsTArray_base leak mentioned in comment 26, as shown in this Try run:
  https://tbpl.mozilla.org/?tree=Try&rev=5a5bdddc6d30  <-- LEAK

For comparison, here's another Try run that includes some of the other changes but leaves that "if" statement untouched:
  https://tbpl.mozilla.org/?tree=Try&rev=b74dbba52724  <-- no leak

I'm not 100% sure what this means, because I'm not really familiar with the line-breaking code...

But I guess there must be something going wonky here, in a case where "aFrame" is an instance of a class that returns a different value from IsFrameOfType(eLineParticipant) vs CanContinueTextRun().  (For frame classes who return the same thing from those methods, this logic-tweak would be a no-op and couldn't possibly cause a leak.)

Comment 28

[Daniel Holbert [:dholbert]]

To figure out the type of "aFrame" when that if-condition makes us leak, I tried experimentally making IsFrameOfType(eLineParticipant) return the same thing as CanContinueTextRun(), for several frame types where those functions normally differ. (If changing a given frame saves us from the leak, then that suggests that that frame is involved here.)

https://tbpl.mozilla.org/?tree=Try&rev=7d8b1a45cbd5 demonstrates (I think) that aFrame is a nsFirstLetterFrame.  (That try run has the if-condition switch from comment 27, plus a patch to make nsFirstLetterFrame reliably return true from IsFrameOfType(eLineParticipant) -- and the leak is gone).

Comment 29

[Daniel Holbert [:dholbert]]

(And in particular, it's a *floating* nsFirstLetterFrame, since that class's IsFrameOfType(eLineParticipant) normally only differs from CanContinueTextRun when the frame is floating.
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFirstLetterFrame.h?rev=91005c6cc198#40

My patch that "fixed" the leak was just removing that special-case for floated nsFirstLetterFrames, effectively.  (Do we really need that special case, I wonder?)

Comment 30

[Daniel Holbert [:dholbert]]

(the special case probably makes sense, since floated first-letter frames clearly don't participate in line layout... so probably never mind about "do we need it")

One more data point: I added a dummy assertion to fire whenever we hit this "if" clause (from comment 27) with a floated first-letter frame, to see what test(s) are involved here.  It looks like there are just 2 (in M-5): test_bug384527.html and test_bug385751.html. That try run, for reference:
https://tbpl.mozilla.org/?tree=Try&rev=1c72325e3193

sgbowen and I just discussed this a bit, and she's now investigating whether we can just leave this "if" condition un-touched (to avoid introducing this mysterious leak), and whether we can instead just change some ruby frames to return true from CanContinueTextRun() (which is probably worth doing anyway) so that they'll still take this code-path.

Comment 31

[sgbowen]

Here's the leak looking good with those changes mentioned in comment 30 AND with dbaron's change from comment 23. I'll upload the patch as a new version if everything else turns out okay.
https://tbpl.mozilla.org/?tree=Try&rev=ad0609275400

Comment 32

[Daniel Holbert [:dholbert]]

nit on the assertion-fix patch, from that Try run:
Bug 1030993 - Fix assertion failure in reftest css-ruby/ruby-whitespace-1.html. r=dbaron

+++ b/layout/reftests/css-ruby/reftest.list
-asserts(1) == ruby-whitespace-1.html ruby-whitespace-1-ref.html # bug 1052145
+asserts(7) != ruby-whitespace-1.html ruby-whitespace-1-ref.html # bug 1052924

This probably shouldn't be changing to "!=", unless the correct rendering is changing and this is turning into "we better not render like this" test (which seems unlikely).

I suspect you really want to be keeping this as "==" and marking it as "fails".

Comment 33

[sgbowen]

Attachment: ruby-reftest-fix.patch v2 (part 2)

Yep, you're right, I meant to have a "fails" instead of a "!=".

The OSX 10.8 tests seem to be taking a while, but I'm pretty confident they'll go all right. I think we should be ready to check in with these two patches and the forthcoming reflow reftest patch.

Comment 34

[sgbowen]

Attachment: ruby-reflow-reftests.patch v1 (part 3)

Here are a couple reftests. I may spin off a bug to add more depending on how much time I have left.

They have the same assertion failure as an earlier whitespace reftest due to line-breaking being unimplemented.

Comment 35

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

Comment on attachment 8473317 [details] [diff] [review]
ruby-reftest-fix.patch v2 (part 2)

Can you explain why the simplification in CanTextCrossFrameBoundary didn't work out?

(I'm not 100% sure that continuing text runs across ruby bases is actually the right thing, given that extra spacing can get inserted between them when the ruby spreads them out, but it's probably reasonable.)

r=dbaron

Comment 36

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

Comment on attachment 8473346 [details] [diff] [review]
ruby-reflow-reftests.patch v1 (part 3)

Could you rename ruby-reflow-1-ref.html to ruby-reflow-1-notref.html, since it's a != reference?

Also, it seems like it would be nice to use the same text and ruby for both -1 and -2.  Then, actually, you might rename the files as:
 ruby-reflow-1-opaqueruby.html
 ruby-reflow-1-transparentruby.html
 ruby-reflow-1-noruby.html
and the assertions would be:
  != ruby-reflow-1-opaqueruby.html ruby-reflow-1-noruby.html
  == ruby-reflow-1-transparentruby.html ruby-reflow-1-noruby.html
(and you'd be using the same reference as the notref or the first assertion and the ref for the second assertion).

r=dbaron with at least the first comment, and preferably both

Comment 37

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

(The reason I'm asking for the same text is that it makes it more likely that the pair of tests covers what they're intending to test, which is that something shows up for the ruby text.  Otherwise the pair could both pass due to having spacing differences in the ruby bases (resulting from the ruby text) for one of the files but not the other.)

Comment 38

[sgbowen]

We're not exactly sure why the simplification didn't work out. It's that checking "if (aFrame->IsFrameOfType(eLineParticipant))" always lead to a memory leak for some subtle reason.

Your point about the spacing is valid.

The spec has this to say:
"When a ruby structure is laid out, its base level is laid out on the line, aligned according to its vertical-align property exactly as if its ruby bases were a regular sequence of inline boxes."

Inline boxes return true for CanContinueTextRun, although this part of the spec is talking about vertical-align, so it's not the same thing...But the implication that ruby behaves mostly like inline boxes remains.

This is probably worth submitting a question to www-style, but the behavior should be okay for now. Changing it would just involve removing the CanContinueTextRun implementations for the ruby frames and adding them to the special case in CanTextCrossFrameBoundary.

Comment 39

[Daniel Holbert [:dholbert]]

(In reply to sgbowen from comment #38)
> We're not exactly sure why the simplification didn't work out. It's that
> checking "if (aFrame->IsFrameOfType(eLineParticipant))" always lead to a
> memory leak for some subtle reason.

(Yeah, all we know is that it seems to be a bug with the handling of floating first-letter frames, per comment 28 & 29, which only happens when such frames are made to take this code-path. And we haven't been able to hit it locally, for some reason.)

Comment 40

[sgbowen]

Attachment: ruby-reflow-reftests.patch v2 (part 3)

Changed the file names, as suggested in comment 36.

Comment 41

[Daniel Holbert [:dholbert]]

Try run:  https://tbpl.mozilla.org/?tree=Try&rev=82135e13866c

Note: In that Try run, I adjusted the "asserts" count to "asserts(3-7)" (instead of just "7") in "part 2", since an earlier Try run of sgbowen's revealed that we only assert 3 times on Android IIRC.

And after my Try run linked here, I've locally adjusted the "asserts" count on the ruby-reflow tests to asserts(0-1), since they do assert locally but they apparently don't assert on Try for some reason. (which is the cause of the Linux/Mac reftest orange in that Try run)

Also, I've locally added a patch to mark the ruby reftests directory as "skip-if(B2G)", pending investigation of b2g-only test timeouts in followup bug 1054383.

With those adjustments, I believe this should be green and good to land. I'll be landing this shortly.

Comment 42

[Daniel Holbert [:dholbert]]

Landed!

 Part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/6978c9d538d4
 Whitespace-handling reftests: https://hg.mozilla.org/integration/mozilla-inbound/rev/b2e1f5bb5e07
 Part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/c405cb65f37b
 Part 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/6aea6f80b377
 skip-if(B2G) annotation: https://hg.mozilla.org/integration/mozilla-inbound/rev/8372c105c01c

Comment 43

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/6978c9d538d4
https://hg.mozilla.org/mozilla-central/rev/c405cb65f37b
https://hg.mozilla.org/mozilla-central/rev/6aea6f80b377
https://hg.mozilla.org/mozilla-central/rev/8372c105c01c