I plan to implement the space-around first, and ruby-align later.

Comment on attachment 8538363 [details] [diff] [review] patch 1 - Add ruby utils for reserving isize of ruby boxes Review of attachment 8538363 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/RubyUtils.h @@ +24,5 @@ > + } > + > + static void SetReservedISize(nsIFrame* aFrame, nscoord aISize); > + static void ClearReservedISize(nsIFrame* aFrame); > + static nscoord GetReservedISize(nsIFrame* aFrame); You need comments explaining what the "Reserved ISize" is.

Comment on attachment 8538371 [details] [diff] [review] patch 7 - Modify jusitication computation to take ruby into account Review of attachment 8538371 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsLineLayout.cpp @@ +2490,2 @@ > PerFrameData* mLastParticipant; > + bool mAcrossRubyBaseBound; Please document what this means. @@ +2569,5 @@ > } > } > > aState.mLastParticipant = pfd; > + aState.mAcrossRubyBaseBound = isRubyBase; Why are we setting this in two places?

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9) > Comment on attachment 8538363 [details] [diff] [review] > patch 1 - Add ruby utils for reserving isize of ruby boxes > > Review of attachment 8538363 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/generic/RubyUtils.h > @@ +24,5 @@ > > + } > > + > > + static void SetReservedISize(nsIFrame* aFrame, nscoord aISize); > > + static void ClearReservedISize(nsIFrame* aFrame); > > + static nscoord GetReservedISize(nsIFrame* aFrame); > > You need comments explaining what the "Reserved ISize" is. OK, so "Reserved ISize" is the difference between the isize a frame should be and what it is currently. The difference is reserved so that we can expand the frames to the expected isize later. The isize has been reserved in line layout via AdvanceICoord.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10) > Comment on attachment 8538371 [details] [diff] [review] > patch 7 - Modify jusitication computation to take ruby into account > > Review of attachment 8538371 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: layout/generic/nsLineLayout.cpp > @@ +2490,2 @@ > > PerFrameData* mLastParticipant; > > + bool mAcrossRubyBaseBound; > > Please document what this means. It means, we are going across a boundary of a ruby base, that is, we are entering one, leaving one, or both. > @@ +2569,5 @@ > > } > > } > > > > aState.mLastParticipant = pfd; > > + aState.mAcrossRubyBaseBound = isRubyBase; > > Why are we setting this in two places? It is set here because we are leaving the ruby base.

Do you think my comments above have been clear enough for understanding the patches? If so, I'll update the patches to include them.

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #11) > OK, so "Reserved ISize" is the difference between the isize a frame should > be and what it is currently. The difference is reserved so that we can > expand the frames to the expected isize later. The isize has been reserved > in line layout via AdvanceICoord. This isn't detailed enough. You need to say what causes the difference and what exactly you mean by "now" and "later".

Comment on attachment 8538371 [details] [diff] [review] patch 7 - Modify jusitication computation to take ruby into account Review of attachment 8538371 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsLineLayout.cpp @@ +2490,2 @@ > PerFrameData* mLastParticipant; > + bool mAcrossRubyBaseBound; OK, call it "mCrossingRubyBaseBoundary".

Ruby boxes usually need to synchronize their isize among different levels, which means they might have a larger isize than what themselves require. When these ruby boxes are reflowed, they are sized like a normal inline container, but the difference between this size and the isize it should be, is reserved in line layout, and saved as "Reserved ISize" in the property of the frame.

Is comment 16 clear enough?

Can you give an example? When you say "the isize it should be", how exactly is the "isize it should be" determined?

With some exceptions, each ruby internal box has two isizes, which are intrinsic isize and intended isize. The intrinsic isize is what a box itself needs. It is determined when the box gets reflowed. The intended isize is what it should be as the final result. For an rb/rt box, the intended isize is the size of its ruby column. For an rbc/rtc box, the intended isize is the size of its ruby segment. The intended isize is never smaller than the intrinsic isize for any ruby box. It is initially determined when a ruby column/segment gets fully reflowed, and may be advanced when a box is expanded, e.g. when applying justification. The difference between the intended isize and the intrinsic isize is reserved in the line layout after reflowing the box, hence it is called reserved isize here. The reserved isize is used to expand the ruby boxes from their intrinsic isize to the intended isize during aligning the line. There are three exceptions for the intrinsic isize: 1. An rbc box does not have intended isize unless there is any span or collapsed annotation in its segment; 2. An rtc box have intended isize only if it is for a span or collapsed annotations; 3. If an rtc box have intended isize, its children must not have. Is it clear enough now? I feel that the word "intrinsic isize" here might refer to another concept in our code, but I have no idea what term should be used instead.

That's a lot clearer, thanks. That explanation should definitely be in a comment somewhere. We already have intrinsic preferred and max isizes, which seem different since they don't take line breaks into account. How is your "intrinsic isize" affected by line breaking? I can't tell from your explanation. "intended" is also too vague. Maybe we should call it "final ruby isize".

Thanks, I'll add them. I'm going to add them to RubyUtils.h. rb/rt boxes currently never have line break inside (bug 1105051), so the "intrinsic isize" I mentioned here should effectively be equal to the intrinsic preferred isize for them. (I assume the other one should be intrinsic min isize, not max). However, right, the "intrinsic isize" for rbc/rtc boxes is affected by line breaking. The "intrinsic isize" for them only takes the left parts into account, i.e. any pushed frames are not included.

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #21) > intrinsic min isize, not max). However, right, the "intrinsic isize" for > rbc/rtc boxes is affected by line breaking. The "intrinsic isize" for them > only takes the left parts into account, i.e. any pushed frames are not > included. I'd prefer not to use the term "intrinsic" for things that are affected by line breaking. Line breaking is a function of the environment (available width) outside of the element, so it's not intrinsic to the element.

Add the comment.

Comment on attachment 8539991 [details] [diff] [review] patch 1 - Add ruby utils for reserving isize of ruby boxes Review of attachment 8539991 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/RubyUtils.h @@ +11,5 @@ > + > +namespace mozilla { > + > +/** > + * Reserved ISize fix this line @@ +28,5 @@ > + * > + * The difference between the reflowed isize and the final isize is > + * reserved in the line layout after reflowing a box, hence it is called > + * "Reserved ISize" here. It is used to expand the ruby boxes from their > + * reflowed isize to the final isize during aligning the line. "during alignment of the line"

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

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/a339d7695b60 https://hg.mozilla.org/integration/mozilla-inbound/rev/f437602d958d https://hg.mozilla.org/integration/mozilla-inbound/rev/daae8202f566 https://hg.mozilla.org/integration/mozilla-inbound/rev/de8839e79001 https://hg.mozilla.org/integration/mozilla-inbound/rev/d9213948acb6 https://hg.mozilla.org/integration/mozilla-inbound/rev/28819098bfcf https://hg.mozilla.org/integration/mozilla-inbound/rev/e018b5e1f399 https://hg.mozilla.org/integration/mozilla-inbound/rev/44d2322fb050

https://hg.mozilla.org/mozilla-central/rev/a339d7695b60 https://hg.mozilla.org/mozilla-central/rev/f437602d958d https://hg.mozilla.org/mozilla-central/rev/daae8202f566 https://hg.mozilla.org/mozilla-central/rev/de8839e79001 https://hg.mozilla.org/mozilla-central/rev/d9213948acb6 https://hg.mozilla.org/mozilla-central/rev/28819098bfcf https://hg.mozilla.org/mozilla-central/rev/e018b5e1f399 https://hg.mozilla.org/mozilla-central/rev/44d2322fb050