Closed Bug 1116037 Opened 5 years ago Closed 5 years ago

Move ruby reflow variables from frame class to stack

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(12 files)

6.21 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
7.09 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
21.04 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
7.85 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
14.04 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
13.59 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
2.87 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
15.36 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
20.91 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
5.62 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
16.80 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
6.27 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
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.
Assignee: nobody → quanxunzhen
Attachment #8542339 - Flags: review?(dbaron)
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
Attachment #8542339 - Flags: review?(dbaron) → review+
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
Attachment #8542340 - Flags: review?(dbaron) → review+
Attachment #8542341 - Flags: review?(dbaron) → review+
Attachment #8542342 - Flags: review?(dbaron) → review+
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?)
Attachment #8542343 - Flags: review?(dbaron) → review+
Attachment #8542344 - Flags: review?(dbaron) → review+
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.
Attachment #8542345 - Flags: review?(dbaron) → review+
(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?
Flags: needinfo?(dbaron)
(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.
Flags: needinfo?(dbaron)
(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.
Attachment #8542346 - Flags: review?(dbaron) → review+
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
Attachment #8542347 - Flags: review?(dbaron) → review+
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.
Attachment #8542348 - Flags: review?(dbaron) → review+
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.
Flags: needinfo?(quanxunzhen)
Attachment #8542351 - Flags: review?(dbaron) → review+
(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?)
Flags: needinfo?(quanxunzhen)
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.
Attachment #8542350 - Flags: review?(dbaron) → review+
(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?
Flags: needinfo?(dbaron)
That's fine.
Flags: needinfo?(dbaron)
You need to log in before you can comment on or make changes to this bug.