1116037 - Move ruby reflow variables from frame class to stack

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Xidorn Quan [:xidorn] UTC+10]

Currently, some data which is used only during reflow is placed in rbc/rtc classes. Ideally, they should all be moved to stack, so that they won't make the frames bigger.

Comment 1

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 1 - Move TextContainerIterator to RubyUtils.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 2 - Use frame state bit to mark rtc of span.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 3 - Use ruby "column" instead of "pair" to match the spec term.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 4 - Remove useless parameters in ruby reflow methods.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 5 - Use unified struct for ruby reflow states.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 6 - Use a single struct to represent ruby column.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 7 - Small fixes to ruby line breaking.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 8 - Merge two ruby text container arrays.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 9 - Move ruby text container arrays to stack.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 10 - Remove mColumnCount from nsRubyBaseContainerFrame.

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 11 - Add RubyLayout for sharing states between ruby level boxes.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch 12 - Move mLineSize to RubyLayout.

Comment 13

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

Comment on attachment 8542339 [details] [diff] [review]
patch 1 - Move TextContainerIterator to RubyUtils.

r=dbaron, but please add a brief comment to the header saying what the class does

Comment 14

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

Comment on attachment 8542340 [details] [diff] [review]
patch 2 - Use frame state bit to mark rtc of span.

>+void
>+nsRubyTextContainerFrame::UpdateSpanFlag()
>+{
>+  bool isSpan = false;
>+  if (!GetPrevContinuation() && !GetNextContinuation()) {
>+    nsIFrame* onlyChild = mFrames.OnlyChild();
>+    if (onlyChild && onlyChild->IsPseudoFrame(GetContent())) {
>+      // Per CSS Ruby spec, if the only child of an rtc frame is
>+      // a pseudo rt frame, it spans all bases in the segment.
>+      isSpan = true;
>+    }
>+  }

It might be worth adding to the comment to explain that the continuation checks are safe because spans never break.

>+  // nsContainerFrame overrides
>+  virtual void SetInitialChildList(ChildListID aListID,
>+                                   nsFrameList& aChildList);
>+  virtual void AppendFrames(ChildListID aListID, nsFrameList& aFrameList);
>+  virtual void InsertFrames(ChildListID aListID, nsIFrame* aPrevFrame,
>+                            nsFrameList& aFrameList);
>+  virtual void RemoveFrame(ChildListID aListID, nsIFrame* aOldFrame);

Please declare all of these with MOZ_OVERRIDE.

>+  bool IsSpanContainer() const
>+  { return GetStateBits() & NS_RUBY_TEXT_CONTAINER_IS_SPAN; }

Probably best to use standard style, with the { a line earlier, and the } on its own line.  (Or, if you want to keep it this way, indent the { by 2 more spaces.)

r=dbaron with that

Comment 15

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

Comment on attachment 8542343 [details] [diff] [review]
patch 5 - Use unified struct for ruby reflow states.

r=dbaron, although it might be better to construct these using a constructor rather than a struct-initializer (maybe in an additional patch on top of this queue?)

Comment 16

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

Comment on attachment 8542345 [details] [diff] [review]
patch 7 - Small fixes to ruby line breaking.

This might have made sense as two separate patches (one for the } move, and one for the other 2 changes).  But it's ok as is.

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (UTC-5) (needinfo? for questions) from comment #15)
> Comment on attachment 8542343 [details] [diff] [review]
> patch 5 - Use unified struct for ruby reflow states.
> 
> r=dbaron, although it might be better to construct these using a constructor
> rather than a struct-initializer (maybe in an additional patch on top of
> this queue?)

I did this but later removed, becasue I found the constructor does nothing more than initializing every member from the arguments. IMO it doesn't add any information to either readers or compilers. Do you have any concerns which make you prefer a constructor over the struct-initializer?

Comment 18

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

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #17)
> I did this but later removed, becasue I found the constructor does nothing
> more than initializing every member from the arguments. IMO it doesn't add
> any information to either readers or compilers. Do you have any concerns
> which make you prefer a constructor over the struct-initializer?

Not really, other than (a) it being a more common style and (b) that you're reasonably likely to end up wanting a constructor at some point in the future.

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (UTC-5) (needinfo? for questions) from comment #18)
> (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #17)
> > I did this but later removed, becasue I found the constructor does nothing
> > more than initializing every member from the arguments. IMO it doesn't add
> > any information to either readers or compilers. Do you have any concerns
> > which make you prefer a constructor over the struct-initializer?
> 
> Not really, other than (a) it being a more common style and (b) that you're
> reasonably likely to end up wanting a constructor at some point in the
> future.

I guess we can add that constructor when we really need it at that point.

Comment 20

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

Comment on attachment 8542347 [details] [diff] [review]
patch 9 - Move ruby text container arrays to stack.

>-    mTexts.AppendElement(aFrame->mTextContainers[i]);
>+    mTexts.AppendElement(aTextContainers[i]);

It's a little surprising that this (old or new code) works given that
ContinuationTraversingState has an explicit constructor.


r=dbaron

Comment 21

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

Comment on attachment 8542348 [details] [diff] [review]
patch 10 - Remove mColumnCount from nsRubyBaseContainerFrame.

I suppose you could have passed a uint32_t& to ReflowOneColumn and kept the code closer to the old code, but this is ok too.

Comment 22

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

Comment on attachment 8542350 [details] [diff] [review]
patch 11 - Add RubyLayout for sharing states between ruby level boxes.

Perhaps this would better be called RubyLayoutState or even
RubyReflowState?

This is adding a good bit of passing things around, and it's not clear
to me based on looking at this patch and the next one why it's worthwhile.

It seems like you have future plans here (e.g., making
SetTextContainerInfo do something else), but they're not clear to me
yet.  Could you explain?


nsLineLayout::nsLineLayout should initialize mRubyLayout to nullptr.

>+  const int32_t kBaseContainerIndex = -1;

Should this be static?  Seems like you don't want it to bloat
every object.

>+  int32_t mCurrentContainerIndex = kBaseContainerIndex;

I didn't know this was legal syntax in a class definition.  Is it
portable?  Why is it preferred over initializing the variable in the
constructor?


Otherwise I guess this looks ok, but I'd like to hear why you're doing
it.

Comment 23

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (UTC-5) (needinfo? for questions) from comment #22)
> Comment on attachment 8542350 [details] [diff] [review]
> patch 11 - Add RubyLayout for sharing states between ruby level boxes.
> 
> Perhaps this would better be called RubyLayoutState or even
> RubyReflowState?

The name RubyReflowState has been used inside nsRubyBaseContainerFrame unfortunately, so maybe RubyLayoutState. But if you have any suggestion about the name of that class, I'm happy to use RubyReflowState for this class.

> This is adding a good bit of passing things around, and it's not clear
> to me based on looking at this patch and the next one why it's worthwhile.
> 
> It seems like you have future plans here (e.g., making
> SetTextContainerInfo do something else), but they're not clear to me
> yet.  Could you explain?

I opened this bug basically because of bug 1055665 comment 51. In that bug, I tried to complete relative positioning as well, but dholbert thought it is wasteful to add variables only used during reflow to the frame class.

After this bug, I want to implement bug 1055658, in which I'm going to add mOverflowAreas to the in-stack class, and let it be set via SetTextContainerInfo.

> nsLineLayout::nsLineLayout should initialize mRubyLayout to nullptr.
> 
> >+  const int32_t kBaseContainerIndex = -1;
> 
> Should this be static?  Seems like you don't want it to bloat
> every object.

I thought const implies static, but it seems I was wrong. Yes, I'll make it static, and constexpr as well.

> >+  int32_t mCurrentContainerIndex = kBaseContainerIndex;
> 
> I didn't know this was legal syntax in a class definition.  Is it
> portable?  Why is it preferred over initializing the variable in the
> constructor?

Oops, I didn't know this was illegal syntax.

No, it is not allowed in our code now. It is an C++11 extension called "member initializers" in our compiler compatibility table, and requires MSVC 2013 and GCC 4.7 (which, I guess, both should be in our requirement soon?)

Comment 24

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

Comment on attachment 8542350 [details] [diff] [review]
patch 11 - Add RubyLayout for sharing states between ruby level boxes.

ok, r=dbaron with the fixes from comment 22 / comment 23.  RubyLayoutState is OK, I guess.  Although if you can think of a better way to distinguish them, it might be better to use *ReflowState naming for both.

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

(In reply to David Baron [:dbaron] (UTC-5) (needinfo? for questions) from comment #24)
> Comment on attachment 8542350 [details] [diff] [review]
> patch 11 - Add RubyLayout for sharing states between ruby level boxes.
> 
> ok, r=dbaron with the fixes from comment 22 / comment 23.  RubyLayoutState
> is OK, I guess.  Although if you can think of a better way to distinguish
> them, it might be better to use *ReflowState naming for both.

What about renaming the current RubyReflowState to ReflowState? I think it won't confuse people since it is an inner class. Then we can call the class in patch 11 RubyReflowState. Sounds good?

Comment 26

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

That's fine.

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

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

Comment 28

[Xidorn Quan [:xidorn] UTC+10]

inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cee6e1f4eec9
https://hg.mozilla.org/integration/mozilla-inbound/rev/48d1aa74f002
https://hg.mozilla.org/integration/mozilla-inbound/rev/422908ee4209
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f691b17679e
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bf00a9544f0
https://hg.mozilla.org/integration/mozilla-inbound/rev/f02749f3c8fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/80663d48d58f
https://hg.mozilla.org/integration/mozilla-inbound/rev/769f499c51ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb2cec3732ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/dad0ce12d778
https://hg.mozilla.org/integration/mozilla-inbound/rev/848c0164755f
https://hg.mozilla.org/integration/mozilla-inbound/rev/305b1834db85

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cee6e1f4eec9
https://hg.mozilla.org/mozilla-central/rev/48d1aa74f002
https://hg.mozilla.org/mozilla-central/rev/422908ee4209
https://hg.mozilla.org/mozilla-central/rev/5f691b17679e
https://hg.mozilla.org/mozilla-central/rev/8bf00a9544f0
https://hg.mozilla.org/mozilla-central/rev/f02749f3c8fc
https://hg.mozilla.org/mozilla-central/rev/80663d48d58f
https://hg.mozilla.org/mozilla-central/rev/769f499c51ab
https://hg.mozilla.org/mozilla-central/rev/fb2cec3732ea
https://hg.mozilla.org/mozilla-central/rev/dad0ce12d778
https://hg.mozilla.org/mozilla-central/rev/848c0164755f
https://hg.mozilla.org/mozilla-central/rev/305b1834db85