Closed Bug 1107721 Opened 5 years ago Closed 5 years ago

Connect line layout of ruby annotations to that of their bases

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(9 files, 5 obsolete files)

3.92 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
7.13 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.61 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
6.85 KB, patch
dbaron
: feedback+
Details | Diff | Splinter Review
6.01 KB, patch
dbaron
: feedback+
Details | Diff | Splinter Review
2.51 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
2.87 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
3.82 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
2.82 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
To make the line layout be aware of the annotations when doing inline size adjustion (bug 1055676 and bug 1081770), and handling the floats correctly (bug 1039009), we need to connect the line layouts between ruby annotations and ruby bases.

Conceptually, line layouts are allocated in stack, and is short life. Line layouts of ruby annotations will be destroyed as soon as their ruby segment completes reflow. Hence, to achieve this, PerFrameData and PerSpanData of annotations should be allocated by the base line layout instead of the line layouts themselves.
Blocks: 1103832
Blocks: 1108429
No longer blocks: ruby-align
Attachment #8534111 - Flags: review?(dbaron)
Comment on attachment 8534105 [details] [diff] [review]
patch 1 - Add pointer to the base line layout in nsLineLayout

>diff --git a/layout/generic/nsLineLayout.h b/layout/generic/nsLineLayout.h
>   nsLineLayout(nsPresContext* aPresContext,
>                nsFloatManager* aFloatManager,
>                const nsHTMLReflowState* aOuterReflowState,
>-               const nsLineList::iterator* aLine);
>+               const nsLineList::iterator* aLine,
>+               nsLineLayout* aBaseLineLayout = nullptr);

Given that there are only 3 callers, I'd prefer you avoid the default parameter and make all the callers pass nullptr.

Also, please add a comment saying "null if no separate base nsLineLayout is needed" or similar.

>+  // The line layout for the base text. It is usually the same as the
>+  // current line layout itself. It becomes different when the current
>+  // line layout is for annotations. All line layouts shares the same
>+  // base line layout when they are associated. The base line layout is
>+  // responsible for managing the life cycle of per-frame data and
>+  // per-span data, and handling floats.
>+  nsLineLayout* mBaseLineLayout;

"annotations" -> "ruby annotations", so that people reading this comment can understand that this is related to ruby.

"shares" -> "share"

"the current line layout itself" -> "|this|"

r=dbaron with that
Attachment #8534105 - Flags: review?(dbaron) → review+
Comment on attachment 8534105 [details] [diff] [review]
patch 1 - Add pointer to the base line layout in nsLineLayout

Actually, one more comment:

It would be good to declare it as:

 nsLineLayout* const mBaseLineLayout;

This would require that you also initialize it in an initializer rather than in the body of the constructor.

I think this would be an improvement since it makes it clearer that patch 2 is safe, i.e., that the |mBaseLineLayout == this| condition doesn't change over time.
Comment on attachment 8534107 [details] [diff] [review]
patch 2 - Allocate PerFrameData & PerSpanData from the base line layout

>+  if (mBaseLineLayout == this) {
>+    PL_INIT_ARENA_POOL(&mArena, "nsLineLayout", 1024);
>+    mFrameFreeList = nullptr;
>+    mSpanFreeList = nullptr;
>+  }

probably best to have an else that does:

  memset(&mArena, 0, sizeof(mArena));

to avoid uninitialized memory.

>+  NS_ASSERTION(mBaseLineLayout == this ||
>+               (mSpansAllocated == 0 && mSpansFreed == 0 &&
>+                mFramesAllocated == 0 && mFramesFreed == 0),
>+               "Allocated frames or spans on non-base line layout?");

Please also assert that mSpanFreeList and mFrameFreeList are null.

r=dbaron with that
Attachment #8534107 - Flags: review?(dbaron) → review+
Comment on attachment 8534108 [details] [diff] [review]
patch 3 - Add pointers to PerFrameData of different level

The comments here should explain what a "next level" is.

Also, are the extra two words easily avoidable?  Could you explain why you need this in the commit message?
Flags: needinfo?(quanxunzhen)
(And perhaps you should call it a "ruby level" to make things clearer.)
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #12)
> Comment on attachment 8534108 [details] [diff] [review]
> patch 3 - Add pointers to PerFrameData of different level
> 
> The comments here should explain what a "next level" is.

Can I just mention it as "next ruby level"?

> Also, are the extra two words easily avoidable?  Could you explain why you
> need this in the commit message?

I need them to traverse all PFDs which should be aligned. Actually, I think mPrevLevel can be replaced by a flag bit because I only use it for checking whether a PFD is linked. Should I do that? (and I just found that I didn't maintain it well. I meant to form a doubly linked list, but in the current code, mPrevLevel is never changed when a new PFD is inserted.)

I think that, since nsLineLayout is designed to be short-life, there won't be a large amount of PFDs exist at the same time. Therefore it should be fine to expand it a bit.
Flags: needinfo?(quanxunzhen)
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #14)
> I need them to traverse all PFDs which should be aligned. Actually, I think
> mPrevLevel can be replaced by a flag bit because I only use it for checking
> whether a PFD is linked. Should I do that? (and I just found that I didn't
> maintain it well. I meant to form a doubly linked list, but in the current
> code, mPrevLevel is never changed when a new PFD is inserted.)

OK.  Could you update the patches to do that and post the new patches for review?
Flags: needinfo?(quanxunzhen)
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #11)
> Comment on attachment 8534107 [details] [diff] [review]
> patch 2 - Allocate PerFrameData & PerSpanData from the base line layout
> 
> >+  if (mBaseLineLayout == this) {
> >+    PL_INIT_ARENA_POOL(&mArena, "nsLineLayout", 1024);
> >+    mFrameFreeList = nullptr;
> >+    mSpanFreeList = nullptr;
> >+  }
> 
> probably best to have an else that does:
> 
>   memset(&mArena, 0, sizeof(mArena));
> 
> to avoid uninitialized memory.

Considering the type safety, I tend to get rid of that condition instead of memset in the other branch. Initialization of arena doesn't seem to be an expensive procedure.
Attachment #8534105 - Attachment is obsolete: true
Attachment #8534664 - Flags: feedback?(dbaron)
Attachment #8534108 - Attachment is obsolete: true
Attachment #8534108 - Flags: review?(dbaron)
Flags: needinfo?(quanxunzhen)
Attachment #8534666 - Flags: review?(dbaron)
Patch 5 and 7 are affected by the updating of patch 3. But the changes are trivial: replacing mPrevLevel with the new flag.
(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #20)
> Patch 5 and 7 are affected by the updating of patch 3. But the changes are
> trivial: replacing mPrevLevel with the new flag.

Could you just upload the new ones anyway?
Flags: needinfo?(quanxunzhen)
Attachment #8534110 - Attachment is obsolete: true
Attachment #8534110 - Flags: review?(dbaron)
Attachment #8534725 - Flags: review?(dbaron)
Attachment #8534112 - Attachment is obsolete: true
Attachment #8534112 - Flags: review?(dbaron)
Flags: needinfo?(quanxunzhen)
Attachment #8534726 - Flags: review?(dbaron)
Comment on attachment 8534664 [details] [diff] [review]
patch 1 - Add pointer to the base line layout in nsLineLayout

nsLineLayout.h:

>+  /**
>+   * @param aBaseLineLayout the nsLineLayout for base level,
>+   * nullptr if no separate base nsLineLayout is needed.

"base" -> "ruby base"
Attachment #8534664 - Flags: feedback?(dbaron) → feedback+
Attachment #8534665 - Flags: feedback?(dbaron) → feedback+
Comment on attachment 8534666 [details] [diff] [review]
patch 3 - Add pointers to PerFrameData of different level

>+    // Link to the corresponding frame in next ruby level.  It could be
>+    // nullptr if there is no corresponding ruby annotation.
>+    PerFrameData* mNextLevel;

This comment should be clearer that:
 (1) this is a linked list through the mNextLevel pointers
 (2) the base points to all of its annotations
 (3) if PFD_ISLINKEDTOBASE is set, this is one of the annotations in the base's list, but if it's not, then this is the base and its mNextLevel is the start of the linked list

(I might prefer PFD_ISLINKEDFROMBASE over PFD_ISLINKEDTOBASE, although I think I'm ok with either.)

r=dbaron with that
Attachment #8534666 - Flags: review?(dbaron) → review+
Comment on attachment 8534109 [details] [diff] [review]
patch 4 - Separate pfd unlinking code to method UnlinkFrame

In nsLineLayout::PushFrame, please assert that pfd->mNext is null
before you call UnlinkFrame(pfd).

r=dbaron with that
Attachment #8534109 - Flags: review?(dbaron) → review+
Comment on attachment 8534725 [details] [diff] [review]
patch 5 - Handle unlinking pfds linked to other levels

I'm actually thinking that mNextRuby or mNextAnnotation would be a
clearer name than mNextLevel.  Does that make sense to you?


But r=dbaron on this patch.
Attachment #8534725 - Flags: review?(dbaron) → review+
Attachment #8534111 - Flags: review?(dbaron) → review+
Attachment #8534726 - Flags: review?(dbaron) → review+
Comment on attachment 8534113 [details] [diff] [review]
patch 8 - Link line layouts of ruby annotations to those of their ruby base

>+    lineLayout->AttachRootFrameToBaseLineLayout();

Are you sure you don't want to do this earlier, e.g., right after BeginLineReflow?  That seems like it would make more sense.

If not, maybe explain in a comment why not?


r=dbaron with that
Attachment #8534113 - Flags: review?(dbaron) → review+
(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #28)
> Comment on attachment 8534113 [details] [diff] [review]
> patch 8 - Link line layouts of ruby annotations to those of their ruby base
> 
> >+    lineLayout->AttachRootFrameToBaseLineLayout();
> 
> Are you sure you don't want to do this earlier, e.g., right after
> BeginLineReflow?  That seems like it would make more sense.

Yes, I think I can do that, and right it would be better in every sense.
For moving AttachRootFrameToBaseLineLayout right after BeginLineReflow, I need to move BeginSpan down, so that the last frame in the base level is the ruby base container when attaching.
Attachment #8534800 - Flags: review?(dbaron)
Comment on attachment 8534800 [details] [diff] [review]
patch 8.1 - Move BeginSpan of nsRubyBaseContainer down

r=dbaron; sorry for the delay
Attachment #8534800 - Flags: review?(dbaron) → review+
You need to log in before you can comment on or make changes to this bug.