Marking as duplicate of 1030993 because the fix has been posted there.

Oops, the duplicate marking was supposed to go on a different bug. Please disregard.

After a long thinking and several attempts to implement the whole thing, I decide to do it step by step from the current code...

This patch adds a unified enumerator for iterating ruby base container with its corresponding text containers simultaneously. It helps clean reflow code, so that we don't need to duplicate code for reflowing ruby text frames.

Currently this patch drops the align of base and text frames. I'm going to implement the default behavior "ruby-align: space-round" after my patch in bug 1063857 gets landed. I may implement bug 1055676 directly later instead of only the default behavior.

This patch resolves most warnings unrelated to line breaking. To achieve this, it starts using nsInlineFrame as the base class of nsRubyBaseFrame and nsRubyTextFrame, which simplifies code of the latters.

This patch uses line layout to reflow ruby base container, so that the latter, along with all the corresponding text containers, could be placed correctly.

This is a trivial and independent patch, which simply removes the unnecessary prerequisite of calling CreateContinuationFor.

Comment on attachment 8503867 [details] [diff] [review] patch 5 - change a parameter of nsBlockFrame::CreateContinuationFor This patch is useless...

Comment on attachment 8502161 [details] [diff] [review] patch 1 - simplify some current code >+ uint32_t rtcNum = mTextContainers.Length(); Maybe "const uint32_t" instead? Also probably rtcCount rather than rtcNum. >+ if (rbFrame) { >+ if (rbFrame->GetType() != nsGkAtoms::rubyBaseFrame) { >+ NS_ASSERTION(false, "Unrecognized child type for ruby base container"); >+ continue; >+ } I realize you're preserving the behavior that used to be here, but it would make slightly more sense to do: + NS_ASSERTION(!rbFrame || rbFrame->GetType() == nsGkAtoms::rubyBaseFrame, + "Unrecognized child type for ruby base container"); + if (rbFrame && rbFrame->GetType() == nsGkAtoms::rubyBaseFrame) { so that you don't also skip the ruby text in this case. >- // Update the inline coord to make space for subsequent ruby annotations >- // (since there is no corresponding base inline size to use). > baseStart += rtcMetrics.ISize(lineWM); This addition should be conditional on there being no base frame. And you should leave the comment. (I think that's the easiest path for now, and the one by which this patch doesn't change behavior, which I think was the intent.) That said, we should really handle better the case of a ruby text that is wider than the ruby base (unless that's handled some other way I don't see). And, also, the code is wrong since it's adding for every rtc rather than just taking the largest. There should probably be a test that fails for what you broke here. So you should file a followup bug on those issues, blocking bug enable-css-ruby, on both fixing this and adding some tests. And you should also add a reftest for what you broke above, here. r=dbaron with that

This patch rewrites CalculateColSizes in nsRubyFrame. I wrote it before I actually started working on this bug. Without this patch, my test xul program crashes frequently in that function.

(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #11) > Comment on attachment 8502161 [details] [diff] [review] > patch 1 - simplify some current code > > And, also, the code is wrong since it's adding for every rtc rather than just > taking the largest. > > There should probably be a test that fails for what you broke here. > > So you should file a followup bug on those issues, blocking bug > enable-css-ruby, on both fixing this and adding some tests. And you > should also add a reftest for what you broke above, here. Did you mean I broke the spanning processing? I think the previous code is not so correct either. I just temporarily drop the support of spanning, and I plan to fix them in some later patch in this bug, after I make the simplest line breaking work. BTW, I plan to only implement line breaking between ruby bases in current stage (slightly different from what the spec requires). I may either leave line breaking within ruby base in a followup bug, or ask the editor to remove this section, as there is no supporting usecase yet.

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #13) > Did you mean I broke the spanning processing? I think the previous code is > not so correct either. I just temporarily drop the support of spanning, and > I plan to fix them in some later patch in this bug, after I make the > simplest line breaking work. Oh, actually... now that I look more closely, the thing you break in patch 1 (making baseStart wrong), you probably fix in patch 2 (which removes baseStart), although I haven't yet read patch 2 in detail.

... so, given that it's probably not worth fixing in detail in patch 1, I'd suggest just removing the line: > baseStart += rtcMetrics.ISize(lineWM); as well (in patch 1), so that at least the basic cases aren't broken by patch 1.

(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #15) > ... so, given that it's probably not worth fixing in detail in patch 1, I'd > suggest just removing the line: > > baseStart += rtcMetrics.ISize(lineWM); > as well (in patch 1), so that at least the basic cases aren't broken by > patch 1. OK. Since patch 1 and patch 2 are mostly for refactoring and simplifing, I'm going to merge these two patches in the next update (with some reverts happened in some later patch). Do you think that's fine, or would you prefer I keep them separate as it is now?

This patch has implemented push frames, so that ruby could be broken into different lines. Pulling frames will be implemented soon.

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #16) > OK. Since patch 1 and patch 2 are mostly for refactoring and simplifing, I'm > going to merge these two patches in the next update (with some reverts > happened in some later patch). Do you think that's fine, or would you prefer > I keep them separate as it is now? It's probably worth keeping them separate, but just have a comment in the commit message explaining that some edge cases break between them.

Could you explain the logic of what you're changing in patch 0? (In general, it would be a good idea to have a commit message in the patch that says what it's doing when you upload it for review.)

(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #23) > Could you explain the logic of what you're changing in patch 0? (In > general, it would be a good idea to have a commit message in the patch that > says what it's doing when you upload it for review.) Sorry about no commit message before submitting for review. I'll add some comments soon to explain.

Comment on attachment 8504367 [details] [diff] [review] patch 0 - fix CalculateColSizes Cancel the review, and add a needinfo? for myself for updating it with comments.

I think it would actually be helpful if you gave more explanation of all 5 of the unreviewed patches; it makes it substantially easier and faster to review.

(In reply to David Baron [:dbaron] (UTC-7, busy Oct 7-9) (needinfo? for questions) from comment #26) > I think it would actually be helpful if you gave more explanation of all 5 > of the unreviewed patches; it makes it substantially easier and faster to > review. I have added description of important changes in commit messages of patch 2-4. I feel most of them are moving or removing existing code, hence few additional comments are necessary, whereas patch 5 would be a relatively large patch, in which I'll try to add more comments for explaination.

This patch completes line breaking for simple case where there is no span. I think there is only one more patch left for this bug now.

Some small changes on comments.

Comment on attachment 8504623 [details] [diff] [review] patch 2 - Simplify reflow code nsFrame.cpp: I agree with this change (even with my later comments about continuing to inherit from block frame), but you should remove this comment: > // Ruby text containers are excluded here because they inherit from block > // (should not be considered inline). also. nsLineLayout.h: Just null-checking mBlockRS and failing seems bad. Why is this needed? And does it produce correct behavior when we fail the null-check? (review- for this, at least to understand why) >+// An estimate of the number of containers. >+// For being used as the preallocation parameter of nsAutoTArray. >+#define CONTAINER_NUM 5 I think you should actually make this 1 or 2. It's going to be quite rare to have more than 1 rtc. Please also rename CONTAINER_NUM to RTC_ARRAY_SIZE or something like that. >+ nsHTMLReflowState* reflowState = reflowStates.AppendElement( >+ nsHTMLReflowState(aPresContext, *aReflowState.parentReflowState, >+ mTextContainers[i], availSize)); I'm not sure nsHTMLReflowState is really meant to be used this way -- and in particular, I'm not sure that it's copy-constructor is actually perfect. Please check that its copy-constructor is really a copy-constructor (there might be reasons for it not to be), and is efficient, or find an alternative here (e.g., using Maybe<> inside the array), and then using its construct(). >+ // XXX nullptr here may cause problem >+ lineLayout->Init(nullptr, reflowState->CalcLineHeight(), -1); Yeah, I guess it's better than the dangling pointer from before, though. But maybe it's better to also maintain an array of nsBlockReflowState, just like you do with the others? And it seems scary to try to use an nsLineLayout in something other than a block frame. I'd rather produce fewer unusual cases that we have to deal with. I'm inclined to think you should keep the base class being nsBlockFrame rather than nsContainerFrame, and you should construct nsBlockReflowStates here. (I like the rest of the refactoring, though.) >+ LogicalMargin borderPadding = reflowState->ComputedLogicalBorderPadding(); >+ nscoord containerWidth = >+ reflowState->ComputedWidth() + borderPadding.LeftRight(lineWM); >+ >+ lineLayout->BeginLineReflow(borderPadding.IStart(lineWM), >+ borderPadding.BStart(lineWM), >+ reflowState->ComputedISize(), >+ NS_UNCONSTRAINEDSIZE, >+ false, false, lineWM, containerWidth); This math also makes me think it might be better to construct nsBlockReflowState objects, so you don't have to duplicate the work they do to compute their mContentArea. Also, with the new code, what keeps the ruby centered over the base when the ruby is narrower than the base? >+ bool allAtLast = true; This is a confusing name. I don't understand what it means. >+ if (allAtLast) { >+ // There is no span, hence all text frames contribute to >+ // the ISize of this column. >+ columnISize = maxISize; >+ } I don't understand this. I haven't been through this in great detail, though, since there are major pieces I don't understand.

Comment on attachment 8504625 [details] [diff] [review] patch 3 - Resolve some warnings nsRubyBaseFrame::IsFrameOfType should still exist, and should say: if (aFlags & eBidiInlineContainer) { return false; } so that the base class doesn't return true for that case. (We don't want bidi reordering across these frames.) The same for nsRubyTextFrame. nsRubyFrame.h: >+ virtual mozilla::LogicalSize >+ ComputeSize(nsRenderingContext *aRenderingContext, >+ mozilla::WritingMode aWritingMode, >+ const mozilla::LogicalSize& aCBSize, >+ nscoord aAvailableISize, >+ const mozilla::LogicalSize& aMargin, >+ const mozilla::LogicalSize& aBorder, >+ const mozilla::LogicalSize& aPadding, >+ uint32_t aFlags) MOZ_OVERRIDE; Please indent everything other than the first of these lines by 2 more spaces. Also, while you're here (since I reviewed the IsFrameOfType methods while reviewing this), I just noticed that nsRubyTextContainerFrame should have an IsFrameOfType that does: if (aFlags & eSupportsCSSTransforms) { return false; } before calling the base class's IsFrameOfType, since per the spec, it should be more like an inline.

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #30) > Comment on attachment 8504623 [details] [diff] [review] > patch 2 - Simplify reflow code > > nsFrame.cpp: > > I agree with this change (even with my later comments about continuing > to inherit from block frame), but you should remove this comment: > > // Ruby text containers are excluded here because they inherit from block > > // (should not be considered inline). > also. > > nsLineLayout.h: > > Just null-checking mBlockRS and failing seems bad. Why is this needed? > And does it produce correct behavior when we fail the null-check? > (review- for this, at least to understand why) I admit I'm not sure about this. But mBlockRS's being null is only for the ruby text container, where we don't want to deal with floats currently. I prefer adding a comment here and leave it until we start trying to dealing with floats. > > >+ nsHTMLReflowState* reflowState = reflowStates.AppendElement( > >+ nsHTMLReflowState(aPresContext, *aReflowState.parentReflowState, > >+ mTextContainers[i], availSize)); > > I'm not sure nsHTMLReflowState is really meant to be used this way -- > and in particular, I'm not sure that it's copy-constructor is actually > perfect. Please check that its copy-constructor is really a > copy-constructor (there might be reasons for it not to be), and is > efficient, or find an alternative here (e.g., using Maybe<> inside the > array), and then using its construct(). nsHTMLReflowState has no user-defined copy constructor at all, and the implicitly-declared one is not deleted. Hence I think it's fine to use it here. If you prefer using Maybe<>, I can do so as well. > >+ LogicalMargin borderPadding = reflowState->ComputedLogicalBorderPadding(); > >+ nscoord containerWidth = > >+ reflowState->ComputedWidth() + borderPadding.LeftRight(lineWM); > >+ > >+ lineLayout->BeginLineReflow(borderPadding.IStart(lineWM), > >+ borderPadding.BStart(lineWM), > >+ reflowState->ComputedISize(), > >+ NS_UNCONSTRAINEDSIZE, > >+ false, false, lineWM, containerWidth); > > This math also makes me think it might be better to construct > nsBlockReflowState objects, so you don't have to duplicate the work > they do to compute their mContentArea. Actually, I'm going to get rid of this math completely, since the spec says: the UA is not required to support any of the box properties (borders, margins, padding) ... on ruby base container boxes, ruby annotation container boxes, or ruby-internal ruby container boxes. > >+ // XXX nullptr here may cause problem > >+ lineLayout->Init(nullptr, reflowState->CalcLineHeight(), -1); > > Yeah, I guess it's better than the dangling pointer from before, though. > > But maybe it's better to also maintain an array of nsBlockReflowState, > just like you do with the others? > > And it seems scary to try to use an nsLineLayout in something other than > a block frame. I'd rather produce fewer unusual cases that we have > to deal with. > > I'm inclined to think you should keep the base class being nsBlockFrame > rather than nsContainerFrame, and you should construct nsBlockReflowStates > here. (I like the rest of the refactoring, though.) In either way, it is an unusual case. I found nsBlockFrame seems to have a different way for dealing with continuation, since it was not meant to be split itself. And it takes a bunch of code dealing with its line list, which we do not need in this container at all. In this regard, the container acts much more like an inline frame instead of a block. For the particular nullptr here, the only usage of the block frame in line layout is where I add the null check before. It might be better to add a condition in line layout to prevent invoking AddFloat for text container, instead of only a simple null check. Do you think it is more reasonable to do so? > Also, with the new code, what keeps the ruby centered over the base > when the ruby is narrower than the base? No code does that. This feature is temporary removed. I mentioned that in comment 5 but forgot to include it in the commit message. Sorry about that. > >+ bool allAtLast = true; > > This is a confusing name. I don't understand what it means. It means that all ruby text frame in this turn is the last child of its container.

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #32) > > nsLineLayout.h: > > > > Just null-checking mBlockRS and failing seems bad. Why is this needed? > > And does it produce correct behavior when we fail the null-check? > > (review- for this, at least to understand why) > > I admit I'm not sure about this. But mBlockRS's being null is only for the > ruby text container, where we don't want to deal with floats currently. I > prefer adding a comment here and leave it until we start trying to dealing > with floats. I worry that just returning null risks crashing in exploitable ways, for example due to dangling pointers from or to frames that we've forgotten about. > > >+ nsHTMLReflowState* reflowState = reflowStates.AppendElement( > > >+ nsHTMLReflowState(aPresContext, *aReflowState.parentReflowState, > > >+ mTextContainers[i], availSize)); > > > > I'm not sure nsHTMLReflowState is really meant to be used this way -- > > and in particular, I'm not sure that it's copy-constructor is actually > > perfect. Please check that its copy-constructor is really a > > copy-constructor (there might be reasons for it not to be), and is > > efficient, or find an alternative here (e.g., using Maybe<> inside the > > array), and then using its construct(). > > nsHTMLReflowState has no user-defined copy constructor at all, and the > implicitly-declared one is not deleted. Hence I think it's fine to use it > here. If you prefer using Maybe<>, I can do so as well. I guess it should be OK; there's no copy-constructor and no destructor. > > >+ LogicalMargin borderPadding = reflowState->ComputedLogicalBorderPadding(); > > >+ nscoord containerWidth = > > >+ reflowState->ComputedWidth() + borderPadding.LeftRight(lineWM); > > >+ > > >+ lineLayout->BeginLineReflow(borderPadding.IStart(lineWM), > > >+ borderPadding.BStart(lineWM), > > >+ reflowState->ComputedISize(), > > >+ NS_UNCONSTRAINEDSIZE, > > >+ false, false, lineWM, containerWidth); > > > > This math also makes me think it might be better to construct > > nsBlockReflowState objects, so you don't have to duplicate the work > > they do to compute their mContentArea. > > Actually, I'm going to get rid of this math completely, since the spec says: > > the UA is not required to support any of the box properties (borders, > margins, padding) ... on ruby base container boxes, ruby annotation > container boxes, or ruby-internal ruby container boxes. I don't think that's a good idea. UAs are allowed to ignore them. But UAs are not allowed to half-support them in silly combinations where the property has some of its normal effects but not others. So I think they should behave like margin/padding/border do on inlines. > In either way, it is an unusual case. I found nsBlockFrame seems to have a > different way for dealing with continuation, since it was not meant to be > split itself. And it takes a bunch of code dealing with its line list, which > we do not need in this container at all. In this regard, the container acts > much more like an inline frame instead of a block. Yes, it does. So perhaps I should think about this more. However, I'm still uncomfortable having lines inside something other than an nsBlockFrame. (Also, where do you get the line box in your approach? nsLineLayout expects to have an nsLineBox, which would normally have been created by the nsBlockFrame.) > For the particular nullptr here, the only usage of the block frame in line > layout is where I add the null check before. It might be better to add a > condition in line layout to prevent invoking AddFloat for text container, > instead of only a simple null check. Do you think it is more reasonable to > do so? I think we should be able to: (1) ensure that any floats are associated with the real block and not the ruby text container. This may already be done correctly. (Does the ruby text container not having NS_BLOCK_FLOAT_MGR set do this?) (2) add an assertion in AddFloat that we're not calling it when the block is null. The assertion could mention ruby text containers as a possible cause. But I think it's better to risk crashing safely with a null-dereference (not exploitable) than add a null-check and risk worse problems lke forgetting about frames. > > Also, with the new code, what keeps the ruby centered over the base > > when the ruby is narrower than the base? > > No code does that. This feature is temporary removed. I mentioned that in > comment 5 but forgot to include it in the commit message. Sorry about that. Hmmm. How do you plan to restore that? > > >+ bool allAtLast = true; > > > > This is a confusing name. I don't understand what it means. > > It means that all ruby text frame in this turn is the last child of its > container. Why is that interesting? (It could even be misleading once you start getting line breaking, and you're dealing with a ruby text frame that is the last child of its container, but the container has a continuation that has more children.)

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #34) > (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #32) > > > nsLineLayout.h: > > > > > > Just null-checking mBlockRS and failing seems bad. Why is this needed? > > > And does it produce correct behavior when we fail the null-check? > > > (review- for this, at least to understand why) > > > > I admit I'm not sure about this. But mBlockRS's being null is only for the > > ruby text container, where we don't want to deal with floats currently. I > > prefer adding a comment here and leave it until we start trying to dealing > > with floats. > > I worry that just returning null risks crashing in exploitable ways, for > example due to dangling pointers from or to frames that we've forgotten > about. > > > In either way, it is an unusual case. I found nsBlockFrame seems to have a > > different way for dealing with continuation, since it was not meant to be > > split itself. And it takes a bunch of code dealing with its line list, which > > we do not need in this container at all. In this regard, the container acts > > much more like an inline frame instead of a block. > > Yes, it does. So perhaps I should think about this more. > > However, I'm still uncomfortable having lines inside something other than an > nsBlockFrame. I can understand, but it might require more work to break the nsBlockFrame and put them in flow. > (Also, where do you get the line box in your approach? nsLineLayout expects > to have an nsLineBox, which would normally have been created by the > nsBlockFrame.) I provide a nullptr for the line_iterator just like code in nsFirstLetterFrame. I'm not sure about the impact though. > > For the particular nullptr here, the only usage of the block frame in line > > layout is where I add the null check before. It might be better to add a > > condition in line layout to prevent invoking AddFloat for text container, > > instead of only a simple null check. Do you think it is more reasonable to > > do so? > > I think we should be able to: > (1) ensure that any floats are associated with the real block and not the > ruby text container. This may already be done correctly. (Does the ruby > text container not having NS_BLOCK_FLOAT_MGR set do this?) In the new code, it is not even a block frame, hence should not have NS_BLOCK_FLOAT_MGR bit. > (2) add an assertion in AddFloat that we're not calling it when the block > is null. The assertion could mention ruby text containers as a possible > cause. But I think it's better to risk crashing safely with a > null-dereference (not exploitable) than add a null-check and risk worse > problems lke forgetting about frames. Agree. > > > Also, with the new code, what keeps the ruby centered over the base > > > when the ruby is narrower than the base? > > > > No code does that. This feature is temporary removed. I mentioned that in > > comment 5 but forgot to include it in the commit message. Sorry about that. > > Hmmm. How do you plan to restore that? Why do we need to restore that exactly? I prefer implement the space-around instead of center. > > It means that all ruby text frame in this turn is the last child of its > > container. > > Why is that interesting? (It could even be misleading once you start > getting line breaking, and you're dealing with a ruby text frame that is the > last child of its container, but the container has a continuation that has > more children.) The last ruby text frame in a container could be a span across multiple columns, hence I need to process them differently. Regarding line breaking, I want to keep the invariant that the span and all its columns will be push/pull together. I haven't implement it in patch 5, but I'm planing to do it in patch 6.

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #35) > (In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from > comment #34) > > (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #32) > > However, I'm still uncomfortable having lines inside something other than an > > nsBlockFrame. > > I can understand, but it might require more work to break the nsBlockFrame > and put them in flow. I suspect if we tell the block frame that it has 0 available height, and also give it mIsTopOfPage true, then it will place one line and then break. (That seems a little risky, but I think less risky than having lines inside something other than a block.) > > (Also, where do you get the line box in your approach? nsLineLayout expects > > to have an nsLineBox, which would normally have been created by the > > nsBlockFrame.) > > I provide a nullptr for the line_iterator just like code in > nsFirstLetterFrame. I'm not sure about the impact though. I guess nsLineBox does handle this. I guess using your approach here (nsContainerFrame rather than nsBlockFrame) is ok to try, although it might prove to have problems. > > I think we should be able to: > > (1) ensure that any floats are associated with the real block and not the > > ruby text container. This may already be done correctly. (Does the ruby > > text container not having NS_BLOCK_FLOAT_MGR set do this?) > > In the new code, it is not even a block frame, hence should not have > NS_BLOCK_FLOAT_MGR bit. Ah, right. > > (2) add an assertion in AddFloat that we're not calling it when the block > > is null. The assertion could mention ruby text containers as a possible > > cause. But I think it's better to risk crashing safely with a > > null-dereference (not exploitable) than add a null-check and risk worse > > problems lke forgetting about frames. > > Agree. OK, so change this null-check to be an assert that mentions ruby. > > > > Also, with the new code, what keeps the ruby centered over the base > > > > when the ruby is narrower than the base? > > > > > > No code does that. This feature is temporary removed. I mentioned that in > > > comment 5 but forgot to include it in the commit message. Sorry about that. > > > > Hmmm. How do you plan to restore that? > > Why do we need to restore that exactly? I prefer implement the space-around > instead of center. I guess that is the default value of ruby-align. You'll still need to do something to make sure the ruby text and corresponding ruby base have the same width (and then you'll need to align them). How do you plan to do that? > > > It means that all ruby text frame in this turn is the last child of its > > > container. > > > > Why is that interesting? (It could even be misleading once you start > > getting line breaking, and you're dealing with a ruby text frame that is the > > last child of its container, but the container has a continuation that has > > more children.) > > The last ruby text frame in a container could be a span across multiple > columns, hence I need to process them differently. Regarding line breaking, > I want to keep the invariant that the span and all its columns will be > push/pull together. I haven't implement it in patch 5, but I'm planing to do > it in patch 6. I'm not sure what you mean by "span across multiple columns" or "span and all its columns". What do you mean by a column

(In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions) from comment #36) > (That seems a little risky, but I think less risky than having lines inside > something other than a block.) And, actually, you're not having lines inside something other than a block, since the new approach has neither a block nor lines, though it does use nsLineLayout without lines.

Comment on attachment 8504627 [details] [diff] [review] patch 4 - Give rbc & rtc correct position nsLineLayout.cpp: Why do you need this change? The existing code for setting isEmpty should work. pfd->mSpan should be non-null, so you should enter the code: isEmpty = !pfd->mSpan->mHasNonemptyContent && pfd->mFrame->IsSelfEmpty(); So I think you should just remove this change. nsRubyBaseContainerFrame.cpp: >+ aReflowState.mLineLayout->BeginSpan(this, &aReflowState, 0, >+ aReflowState.AvailableISize(), >+ &mBaseline); This should account for aReflowState.ComputedBorderPadding like nsInlineFrame::ReflowFrames does. >+ nscoord spanSize = aReflowState.mLineLayout->EndSpan(this); >+ MOZ_ASSERT(isize == spanSize || mFrames.IsEmpty()); This should have an explanation of why these numbers are different when mFrames.IsEmpty(). nsRubyFrame.cpp: This is OK as long as the following patch fixes things up to actually handle line breaking. It can't land on its own. >+ nscoord containerWidth = GetParent()->GetLogicalSize().ISize(lineWM); This seems like it's not OK. In Reflow, we set sizes when we're returning, so the parent's logical size will be its old size rather than its new one. New sizes are stored in the reflow state. Then again, we don't even *know* the new size for the parent yet, so maybe this is the right thing. We should at the very least document better what is supposed to happen here. (Yes, there was existing code that you removed that used the same calculation.) r=dbaron with that

(In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions) from comment #36) > (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #35) > > (In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from > > comment #34) > > > (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #32) > > > However, I'm still uncomfortable having lines inside something other than an > > > nsBlockFrame. > > > > I can understand, but it might require more work to break the nsBlockFrame > > and put them in flow. > > I suspect if we tell the block frame that it has 0 available height, and > also give it mIsTopOfPage true, then it will place one line and then break. > (That seems a little risky, but I think less risky than having lines inside > something other than a block.) I can see the code path. It keeps the first line and pushes others to its continuation. I wonder there might be a problem that the two blocks have different width and inline coordinate. I feel it looks more tricky than the current code. > > > (Also, where do you get the line box in your approach? nsLineLayout expects > > > to have an nsLineBox, which would normally have been created by the > > > nsBlockFrame.) > > > > I provide a nullptr for the line_iterator just like code in > > nsFirstLetterFrame. I'm not sure about the impact though. > > I guess nsLineBox does handle this. > > I guess using your approach here (nsContainerFrame rather than nsBlockFrame) > is ok to try, although it might prove to have problems. What problems or risks do you think it might have? It looks good to me currently. The main challenge I guess would be vertical position of text, like vertical align or something. I'm not sure. Browsing code of nsLineLayout, it seems to me that there are few special cases for block frame. But there are some interactions with line box, which might later be proved to be necessary, but not yet. And line box is currently not used by anywhere other than block frame. Anyway, the line breaking has been implemented with (in my opinion) little magic by these patches. If there is no serious problem identified currently, I suggest we open a bug later once we find one, and discuss about the alternatives then. It should not require many changes to other code, hence it could be an independent one. > > > > > Also, with the new code, what keeps the ruby centered over the base > > > > > when the ruby is narrower than the base? > > > > > > > > No code does that. This feature is temporary removed. I mentioned that in > > > > comment 5 but forgot to include it in the commit message. Sorry about that. > > > > > > Hmmm. How do you plan to restore that? > > > > Why do we need to restore that exactly? I prefer implement the space-around > > instead of center. > > I guess that is the default value of ruby-align. > > You'll still need to do something to make sure the ruby text and > corresponding ruby base have the same width (and then you'll need to align > them). How do you plan to do that? I'm going to do so in bug 1055676 now, so it might be better to set that bug to block bug 1039006 then. I have had some ideas about how to implement the whole alignment. I'd like to discuss about it in that bug. > > The last ruby text frame in a container could be a span across multiple > > columns, hence I need to process them differently. Regarding line breaking, > > I want to keep the invariant that the span and all its columns will be > > push/pull together. I haven't implement it in patch 5, but I'm planing to do > > it in patch 6. > > I'm not sure what you mean by "span across multiple columns" or "span and > all its columns". What do you mean by a column Per the spec: If there are not enough ruby annotations in a ruby annotation container, the last one is paired with (spans across) any excess ruby bases. The "column" I mentioned is linked to the term ruby base here. The reason I use column instead is that, empty ruby bases are assumed to exist if there are more ruby annotations in some container, but we don't create frame for those bases.

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #39) > (In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions) from > comment #36) > > I suspect if we tell the block frame that it has 0 available height, and > > also give it mIsTopOfPage true, then it will place one line and then break. > > (That seems a little risky, but I think less risky than having lines inside > > something other than a block.) > > I can see the code path. It keeps the first line and pushes others to its > continuation. I wonder there might be a problem that the two blocks have > different width and inline coordinate. I feel it looks more tricky than the > current code. Ah, yes, widths would be a problem. Probably your approach is best. > > I guess using your approach here (nsContainerFrame rather than nsBlockFrame) > > is ok to try, although it might prove to have problems. > > What problems or risks do you think it might have? It looks good to me > currently. The main challenge I guess would be vertical position of text, > like vertical align or something. I'm not sure. It's hard to know in advance, but there are a lot of assumptions in the code, so there will probably be something. > Anyway, the line breaking has been implemented with (in my opinion) little > magic by these patches. If there is no serious problem identified currently, > I suggest we open a bug later once we find one, and discuss about the > alternatives then. It should not require many changes to other code, hence > it could be an independent one. OK. > > You'll still need to do something to make sure the ruby text and > > corresponding ruby base have the same width (and then you'll need to align > > them). How do you plan to do that? > > I'm going to do so in bug 1055676 now, so it might be better to set that bug > to block bug 1039006 then. I have had some ideas about how to implement the > whole alignment. I'd like to discuss about it in that bug. yes, it probably should block, then. They still need to be given the same width, though. > > > The last ruby text frame in a container could be a span across multiple > > > columns, hence I need to process them differently. Regarding line breaking, > > > I want to keep the invariant that the span and all its columns will be > > > push/pull together. I haven't implement it in patch 5, but I'm planing to do > > > it in patch 6. > > > > I'm not sure what you mean by "span across multiple columns" or "span and > > all its columns". What do you mean by a column > > Per the spec: > > If there are not enough ruby annotations in a ruby annotation container, the > last one is paired with (spans across) any excess ruby bases. > > The "column" I mentioned is linked to the term ruby base here. The reason I > use column instead is that, empty ruby bases are assumed to exist if there > are more ruby annotations in some container, but we don't create frame for > those bases. I think "column" is a bad term to use here, since it has another meaning in layout code. I think I probably prefer "pair", even though there are sometimes (though rarely) more than two elements involved.

(In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions) from comment #40) > (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #39) > > > > Per the spec: > > > > If there are not enough ruby annotations in a ruby annotation container, the > > last one is paired with (spans across) any excess ruby bases. > > > > The "column" I mentioned is linked to the term ruby base here. The reason I > > use column instead is that, empty ruby bases are assumed to exist if there > > are more ruby annotations in some container, but we don't create frame for > > those bases. > > I think "column" is a bad term to use here, since it has another meaning in > layout code. I think I probably prefer "pair", even though there are > sometimes (though rarely) more than two elements involved. The current code has been using the term "column" to describe pair, as it is very similar to "column" in tables. What's the other meaning in layout code? If that is the term for table, I don't think it is a big problem. Anyway, if you prefer, I will change my word.

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #41) > The current code has been using the term "column" to describe pair, as it is > very similar to "column" in tables. > > What's the other meaning in layout code? If that is the term for table, I > don't think it is a big problem. Anyway, if you prefer, I will change my > word. It's also used to refer to http://dev.w3.org/csswg/css-multicol/ and nsColumnSetFrame, which is a context (like pages) in which we do block breaking. I'd really prefer to match the spec's terminology. If you feel strongly that column is a better term than pair, you should try to get the spec changed.

Could you add a commit message to patch 0 explaining how the new code differs from the old code?

(In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions) from comment #44) > Could you add a commit message to patch 0 explaining how the new code > differs from the old code? I feel it is hard. There are many problems in the old code. I wanted to fix them, but ended up rewriting it. Besides the problems that the old code is broken in implementation, the algorithm cannot process spans correctly. For example, the following case: --------------- | 3 | 9 | ------------------ | 15 | ------------------ | 9 | 3 | --------------- The result produced by the old code would be 15, but the correct answer should be 9+9=18. Though, I cannot confirm that my code is very correct, because I didn't find a good way to test those code. Any suggestion?

(In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions) from comment #43) > (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #41) > > The current code has been using the term "column" to describe pair, as it is > > very similar to "column" in tables. > > > > What's the other meaning in layout code? If that is the term for table, I > > don't think it is a big problem. Anyway, if you prefer, I will change my > > word. > > It's also used to refer to http://dev.w3.org/csswg/css-multicol/ and > nsColumnSetFrame, which is a context (like pages) in which we do block > breaking. > > I'd really prefer to match the spec's terminology. If you feel strongly > that column is a better term than pair, you should try to get the spec > changed. OK, I'll use "pair" instead.

Per our discussion above, I didn't change the basic structure of this patch, but I added some comments. Hope they are helpful.

David, have you started reviewing patch 5? If you haven't, I'm going to submit the new version which rebases some of the changes in previous patches and uses the new word (pair instead of column).

Go ahead and post the new version (at least if it's before I wake up in ~12 hours).

Comment on attachment 8508404 [details] [diff] [review] patch 5 - Implement basic line breaking Here are my comments so far: >Bug 1052924 - Implement basic line breaking Implement basic line breaking for CSS ruby. nsContainerFrame.h/cpp: If the class is called ContinuationTraversingState, the parameter should be aState rather than aStatus. (You use aState in the .h but aStatus in the .cpp.) >+ /** >+ * Find the next child in continuation containers. >+ */ I don't know what "child in continuation containers" means. Please make this comment say instead: Find the first frame that is a child of this frame's next-in-flows, considering both their principal child lists and overflow lists. Please rename this to GetNextInFlowChild to be consistent with other names that use "NextInFlowChild". >+ /** >+ * Pull the next child in continuation containers, and insert it >+ * into the principal frame list. >+ */ This comment should say: Remove the result of GetNextInFlowChild from its current parent and append it to this frame's principal child list. The method should be called PullNextInFlowChild. In PullChildNextInFlow: >+ nextInFlow->mFrames.SetFrames(frame); >+ } >+ >+ // Move the frame to the principal frame list of this container >+ nextInFlow->mFrames.RemoveFirstChild(); Remove the first of the lines quoted above, and then put the RemoveFirstChild in an else. It seems silly to transfer the frame to the principal list temporarily just to remove it again on the next line. >+ mFrames.AppendFrame(this, frame); >+ nsContainerFrame::ReparentFrameView(frame, nextInFlow, this); You also need to do frame->SetParent(this). It might help to move the ReparentFrame helper function in nsBlockFrame.cpp to a static method on nsContainerFrame. nsRubyBaseContainerFrame.cpp: This patch would have been easier to review if, for example, the movement of a bunch of code into a separate ReflowFrames method were in a separate patch from the other changes. Could you explain why mStartPair and mPairCount are member variables rather than state we keep during Reflow? (Please don't update the patch now; it will just make it harder to review.)

(In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions) (away/busy Oct. 24-31) from comment #51) > Comment on attachment 8508404 [details] [diff] [review] > patch 5 - Implement basic line breaking > > nsRubyBaseContainerFrame.cpp: > > This patch would have been easier to review if, for example, the > movement of a bunch of code into a separate ReflowFrames method were > in a separate patch from the other changes. Sorry about that. > Could you explain why mStartPair and mPairCount are member variables > rather than state we keep during Reflow? They are indeed state during reflow, but we have to notify the continuations about these values, because continuations share a same content, and I use them to notify the line layout about break opportunities. I didn't find an easy way to transfer them to continuations other than make them member variables.

I met some assertions because of using nsLineLayout in non-block frame during running some reftests without "white-space: nowrap" being specified. It seems that nsLineLayout put the outer reflow state in "mBlockReflowState", which is the source of all the assertions. After browsering the code, I think there are two main areas which non-blocks should not touch, or should pass to its nearest block ancestor: 1. Handling floats. This might be the main source of NS_ASSERTION. 2. Styles only applied to block. This problem is more indiscoverable (but less risky as well) since they will be applied without assertions around. It is also a problem we had before changing the base class of nsRubyTextContainer, because rtc is never a block element in the spec. To solve these problems, I think what we should do is to check every reference of mBlockReflowState, and divide them into two categories: one indeed requires a block frame, and the other doesn't. Then separate the mBlockReflowState into two members, say mOuterReflowState and mNearestBlockReflowState. After that, we use mOuterReflowState for cases in the second category, and make the references in first category either skip or use mNearestBlockReflowState instead. Some code in nsInlineFrame and nsTextFrame also needs to be changed in a similar way. Do you think it is a reasonable project? Or we should change the base class back to nsBlockFrame and try to find a way to maintain the continuations of it differently?

BTW, the current implementation does not match the spec in two ways: 1. The spec says whether line can break between two ruby bases is controlled by normal line-breaking rules. [1] But currently, I'm making it always possible to break between pairs when no span across them. 2. The spec says it is possible to break inside a ruby base if the line-breaking rules allow. [2] Currently I doesn't handle this case at all, and hope they will never be able to break inside. For the both cases, I didn't find any reasonable use case or example from the spec or JLREQ which depends on those behaviors. I wonder should we obey the spec or get the spec changed? In addition, if we don't want to allow any line-breaking inside the ruby base and text, what can we do to force this requirement? [1]: http://dev.w3.org/csswg/css-ruby/#break-between [2]: http://dev.w3.org/csswg/css-ruby/#break-within

Fix a crash when all ruby text containers of a segment are empty.

Comment on attachment 8508404 [details] [diff] [review] patch 5 - Implement basic line breaking continued from comment 51, here are some further comments on the patch: >+ * This struct stores the continuations after this frame and >+ * corresponding text containers. It is used to accelerate looking >+ * for next nonempty continuations. "for next nonempty continuations" isn't quite grammatical. How about "It is used to speed up looking ahead for nonempty continuations." However, I'm a little concerned here -- why are you distinguishing between empty and nonempty continuations? If you distinguish, it seems like there's a risk of losing track of the frames that are empty continuations rather than deleting them when needed. ReflowFrames: Maybe call this ReflowChildren instead of ReflowFrames? (Though nsInlineFrame does use ReflowFrames, but it's an odd name and could perhaps be changed to ReflowChildren, to match many other classes.) >+#ifdef DEBUG >+ bool breakAtStart = >+#endif >+ aReflowState.mLineLayout->NotifyOptionalBreakPosition( >+ GetParent()->GetContent(), mStartPair, >+ true, gfxBreakPriority::eNormalBreak); >+ MOZ_ASSERT(!breakAtStart, "This should be done by previous frame"); This seems like you're always allowing a break between ruby bases. That might be ok for an initial patch, as long as you're aware it needs to be fixed in a later patch. But I'm pretty sure it's wrong, and it certainly disagrees with the spec, which says: Whether ruby can break between two adjacent ruby bases is controlled by normal line-breaking rules for the base text, exactly as if the ruby bases were adjacent inline boxes. (The annotations are ignored when determining soft wrap opportunities for the base level.) If you intended it this way for now, you should file a bug on fixing this that blocks bug enable-css-ruby. >+ if (aStatus != NS_FRAME_COMPLETE) { I suspect you want NS_FRAME_IS_COMPLETE(aStatus) instead, although really instead you might want to assert that they're equivalent, since you're not handling NS_FRAME_OVERFLOW_IS_INCOMPLETE (nor should you be). + if (GetNextInFlow() && !isComplete) { + // What ever happens in the reflow loop, if there still exist + // next-in-flows and we haven't pull all of their children, + // the status shouldn't be set complete. + NS_FRAME_SET_INCOMPLETE(aStatus); + } I think this is a fine thing to do for safety, but I think, inside the if, you should assert that NS_FRAME_IS_COMPLETE(aStatus) is already true, before setting it. >+ // Try pull some frames from next continuations. Currently it only >+ // pull one pair of frames. It should be implemented to be able >+ // to pull all frames of a span. >+ PullFrames(aReflowState.mLineLayout, >+ pullFrameState, baseFrame, textFrames, isComplete); Please add a comment here that PullFrames replaces baseFrame and textFrames. This makes reading this part of the code clearer. DoReflowFrames: Same naming comment here. Also please add comments to explain that ReflowChildren reflows all of the children, and DoReflowChildren reflows a subset of them. >+ canPlace = !NS_INLINE_IS_BREAK(reflowStatus); >+ if (!canPlace) { >+ // If any breaking occurs when reflowing a ruby text frame, >+ // we should abandon reflowing this pair. >+ break; >+ } >+ MOZ_ASSERT(!pushedFrame, "Shouldn't push frame if there is no break"); I suspect this is ok, but I'm not 100% sure. It's worth checking to see if there are other cases where we reflow an inline frame, see that it breaks, and then reflow it again before creating and reflowing its next continuation. (I know blocks do this sort of thing, but I'm not sure inlines do.) Or, alternatively, it's worth checking that inlines handle this case, and deal with things being on their own overflow list when they're being reflowed. >+ if (NS_INLINE_IS_BREAK(reflowStatus)) { >+ // It is assumed that ruby base frame is unbreakable, >+ // hence it should never break after. >+ MOZ_ASSERT(NS_INLINE_IS_BREAK_BEFORE(reflowStatus), >+ "Currently we only process this only case"); >+ break; What ensures this? (Why not just do the same thing you do for ruby text?) Also, is there something you need to do to handle pushedFrame being true? That is, do you need to do something to undo what the nsLineLayout did when it pushed the frame? > if (rtFrame->GetNextSibling()) { >+ // If there is a next sibling, the isize of this frame >+ // must contribute to the pair isize. > pairISize = std::max(pairISize, isize); > allFramesAreLastChild = false; > } I don't understand this at all. Why is this condition about rtFrame->GetNextSibling() here? Why do you allow ruby text that doesn't fit? And doesn't a GetNextSibling condition mean that the behavior will differ based on the prior-to-Reflow line breaking of frames, which is something that shouldn't happen? >+ // Record a optional break position after each pair. >+ // Break position is recorded as ruby container's content with >+ // the pair offset. >+ bool force = aReflowState.mLineLayout->NotifyOptionalBreakPosition( >+ GetParent()->GetContent(), mStartPair + mPairCount, >+ icoord <= aReflowState.AvailableISize(), >+ gfxBreakPriority::eNormalBreak); Same comment applies here as before. nsRubyBaseContainerFrame::PullFrames: >+ if (!aIsComplete) { >+ aLineLayout->SetDirtyNextLine(); >+ } This needs a comment explaining why it's here. (I don't know.) Sorry I still haven't gotten through the whole thing.

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) (away/busy Oct. 24-31) from comment #56) > Comment on attachment 8508404 [details] [diff] [review] > patch 5 - Implement basic line breaking > > continued from comment 51, here are some further comments on the patch: > > >+ * This struct stores the continuations after this frame and > >+ * corresponding text containers. It is used to accelerate looking > >+ * for next nonempty continuations. > > "for next nonempty continuations" isn't quite grammatical. How about > "It is used to speed up looking ahead for nonempty continuations." > > However, I'm a little concerned here -- why are you distinguishing > between empty and nonempty continuations? If you distinguish, it seems > like there's a risk of losing track of the frames that are empty > continuations rather than deleting them when needed. Because nonempty continuations don't contain any frame, do they? I consulted nsInlineFrame::PullOneFrame to write the functions, and the prototype of this structure is mNextInFlow of InlineReflowState. About the risk of losing track, AFAIK, deleting them is the work of nsLineLayout::ReflowFrame, after a frame reports complete. As commented in nsRubyFrame::ReflowSegment (find "DeleteNextInFlowChild"), when the ruby base container reports complete, we will remove the next-in-flows of text containers. That structure is never used to track this. The only function of the struct is to make finding the next pullable frame quicker. > >+#ifdef DEBUG > >+ bool breakAtStart = > >+#endif > >+ aReflowState.mLineLayout->NotifyOptionalBreakPosition( > >+ GetParent()->GetContent(), mStartPair, > >+ true, gfxBreakPriority::eNormalBreak); > >+ MOZ_ASSERT(!breakAtStart, "This should be done by previous frame"); > > This seems like you're always allowing a break between ruby bases. > > That might be ok for an initial patch, as long as you're aware it needs > to be fixed in a later patch. But I'm pretty sure it's wrong, and it > certainly disagrees with the spec, which says: > > Whether ruby can break between two adjacent ruby bases is controlled > by normal line-breaking rules for the base text, exactly as if the > ruby bases were adjacent inline boxes. (The annotations are ignored > when determining soft wrap opportunities for the base level.) > > If you intended it this way for now, you should file a bug on fixing > this that blocks bug enable-css-ruby. I mentioned that in comment 54. Filed bug 1089431 for this. In addition, the spec also allows break inside ruby base, which I found hard to implement now, and think it doesn't even worth to implement for now. Should I file another bug for that as well? Even if yes, I don't think it should block enable-css-ruby. > >+ if (aStatus != NS_FRAME_COMPLETE) { > > I suspect you want NS_FRAME_IS_COMPLETE(aStatus) instead, although > really instead you might want to assert that they're equivalent, since > you're not handling NS_FRAME_OVERFLOW_IS_INCOMPLETE (nor should you be). By that way, I have to check NS_INLINE_BREAK_BEFORE(aStatus) in addition, because complete flag is not reliable when break before happens. I think it might be better to assert NS_FRAME_OVERFLOW_IS_INCOMPLETE is false with this condition unchanged. Do you agree with that? > >+ canPlace = !NS_INLINE_IS_BREAK(reflowStatus); > >+ if (!canPlace) { > >+ // If any breaking occurs when reflowing a ruby text frame, > >+ // we should abandon reflowing this pair. > >+ break; > >+ } > >+ MOZ_ASSERT(!pushedFrame, "Shouldn't push frame if there is no break"); > > I suspect this is ok, but I'm not 100% sure. It's worth checking > to see if there are other cases where we reflow an inline frame, see > that it breaks, and then reflow it again before creating and reflowing > its next continuation. (I know blocks do this sort of thing, but I'm > not sure inlines do.) Or, alternatively, it's worth checking that > inlines handle this case, and deal with things being on their own > overflow list when they're being reflowed. With the current code of nsLineLayout, it is 100% sure that pushedFrame can be true only when reflowStatus is NS_INLINE_BREAK_BEFORE: The only place sets aPushedFrame to true: http://dxr.mozilla.org/mozilla-central/source/layout/generic/nsLineLayout.cpp#1060 The condition is that CanPlaceFrame returns false. Then: CanPlaceFrame always set aStatus to NS_INLINE_BREAK_BEFORE before returning false: http://dxr.mozilla.org/mozilla-central/source/layout/generic/nsLineLayout.cpp#1302 > >+ if (NS_INLINE_IS_BREAK(reflowStatus)) { > >+ // It is assumed that ruby base frame is unbreakable, > >+ // hence it should never break after. > >+ MOZ_ASSERT(NS_INLINE_IS_BREAK_BEFORE(reflowStatus), > >+ "Currently we only process this only case"); > >+ break; > > What ensures this? Nothing ensures this... > (Why not just do the same thing you do for ruby text?) I think I wrote this code based on some misunderstanding of the line layout code before. I would change that. > Also, is there something you need to do to handle pushedFrame being > true? That is, do you need to do something to undo what the > nsLineLayout did when it pushed the frame? It seems that nsLineLayout pushes frame only with aStatus set to NS_INLINE_BREAK_BEFORE. In that case, we will push our frames as well. Do we need to do anything special here? The only thing I'm concerned here is that, I do want to ensure ruby base is never broken inside, as it would make things a lot more complex if it can be broken. But I have no idea about how to ensure that. Any suggestions? > > if (rtFrame->GetNextSibling()) { > >+ // If there is a next sibling, the isize of this frame > >+ // must contribute to the pair isize. > > pairISize = std::max(pairISize, isize); > > allFramesAreLastChild = false; > > } > > I don't understand this at all. Why is this condition about > rtFrame->GetNextSibling() here? As I mentioned in the last paragraph of comment 35, it is the logic for check span. If the current frame is not the last frame of its parent, it must not be a span, and should contribute to the pair isize. If it is the last frame, it is possible, but not always, a span. > Why do you allow ruby text that doesn't fit? I do not allow it. But this code is not directly used for reflowing frames, it is used to track the possible isize of the current pair. > And doesn't a GetNextSibling condition mean that the behavior will > differ based on the prior-to-Reflow line breaking of frames, which > is something that shouldn't happen? With the invariant mentioned in the last paragraph of comment 35, if the line break happens after one pair, all of the frames of the pair would be the last frame in its parent, then allFramesAreLastChild will be true, and finally they all contribute their isize to the pairISize. Hence the behavior won't differ here. > > >+ // Record a optional break position after each pair. > >+ // Break position is recorded as ruby container's content with > >+ // the pair offset. > >+ bool force = aReflowState.mLineLayout->NotifyOptionalBreakPosition( > >+ GetParent()->GetContent(), mStartPair + mPairCount, > >+ icoord <= aReflowState.AvailableISize(), > >+ gfxBreakPriority::eNormalBreak); > > Same comment applies here as before. > > nsRubyBaseContainerFrame::PullFrames: > > >+ if (!aIsComplete) { > >+ aLineLayout->SetDirtyNextLine(); > >+ } > > This needs a comment explaining why it's here. (I don't know.) Can I say, I don't know either? I found that nsInlineFrame do this when pulling frame: http://dxr.mozilla.org/mozilla-central/source/layout/generic/nsInlineFrame.cpp#863 I guess the reason is that one of the frames in that line is stolen by us, and we need to notify the line to reflow again.

Comment on attachment 8508404 [details] [diff] [review] patch 5 - Implement basic line breaking continued from comment 51 and comment 56 nsRubyBaseContainerFrame.h: >+ void SetStartPair(uint32_t aPair) { mStartPair = aPair; } Maybe this should be mPairStart / SetPairStart, so it's clear it lines up with mPairCount / SetPairCount? (And same for mStartPair in nsRubyFrame.h.) nsRubyFrame.h: nsRubyFrame.cpp: >+ * This method reflows one ruby segment, including the ruby base >+ * container as well as its corresponding ruby text containers. I think "including the ruby base container as well as" should be "which is a ruby base container and". >+ * This method also responds to push children if line breaking occurs. "responds to push" -> "pushes I think what you do with passing aStatus through to the children is a little unusual, although I think it's ok. I think it's more typical to have a separate local status variable that records the child status, and then you set aStatus appropriately based on the child status. But I think what you have is probably ok, but please keep that in mind in case it causes problems. >+ if (NS_FRAME_IS_NOT_COMPLETE(aStatus) && >+ NS_INLINE_IS_BREAK_AFTER(aStatus)) { Perhaps assert that NS_FRAME_IS_NOT_COMPLETE(aStatus) == NS_INLINE_IS_BREAK_AFTER(aStatus)? Why would they ever be different at this point? Does it make sense to rename prevSibling to lastChild or newLastChild? I feel like that makes a little more sense. >+ nsresult rv = CreateNextInFlow(aBaseContainer, newBaseContainer); >+ if (NS_FAILED(rv)) { >+ return; >+ } >+ if (newBaseContainer) { I don't think you need either of these tests (NS_FAILED(rv) or newBaseContainer). You should file a followup bug on making CreateNextInFlow return the frame pointer instead of returning nsresult and using an out-parameter. >+ mFrames.RemoveFrame(newBaseContainer); ... >+ mFrames.RemoveFrame(newTextContainer); If these are existing next-in-flows they might not be in mFrames. You should handle that correctly. But I suspect you don't need the RemoveFrame and InsertFrame calls at all. You just need to condition the PushChildren call on prevSibling (or newLastChild) actually having a next sibling. This also means less moving frames around if there are existing continuations. >+ nsresult rv = CreateNextInFlow(textContainers[i], newTextContainer); >+ if (NS_FAILED(rv)) { >+ return; >+ } Again, no need to check rv. >+ // there are few issues need to be considered here. "there are few issues" isn't normally a phrase used in English, but it's very similar to "there are a few issues" which means that there are issues of concern. But I think what you really mean is "this is relatively simple". >+ childReflowState.mLineLayout = aReflowState.mLineLayout; Could you put a FIXME here (I know it's existing code) saying that we probably shouldn't be using the same nsLineLayout for the text containers. >+ NS_ASSERTION(frameReflowStatus == NS_FRAME_COMPLETE, >+ "Ruby text container must not break itself inside"); This needs a bit more of a comment. Since ruby text containers return NS_FRAME_COMPLETE even when they have continuations, because the breaking has already been handled when reflowing the base containers, you should document that carefully here, in addition to your comments in nsRubyTextContainer::Reflow. nsRubyTextContainerFrame.h: These changes appear not to be needed anymore. I think this is mostly good, but I'd like to have a quick look again after you figure out how to address all the comments here.

Comment on attachment 8517064 [details] [diff] [review] patch 5.1 - Move ReparentFrame from nsBlockFrame to a static method of nsContainerFrame >+inline void >+nsContainerFrame::ReparentFrame(nsIFrame* aFrame, nsContainerFrame* aOldParent, >+ nsContainerFrame* aNewParent) drop "inline", add "/* static */" in its place r=dbaron with that

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #60) > Comment on attachment 8517064 [details] [diff] [review] > patch 5.1 - Move ReparentFrame from nsBlockFrame to a static method of > nsContainerFrame > > >+inline void > >+nsContainerFrame::ReparentFrame(nsIFrame* aFrame, nsContainerFrame* aOldParent, > >+ nsContainerFrame* aNewParent) > > drop "inline", add "/* static */" in its place > > r=dbaron with that Actually we don't need this. In the original place you commented, AppendFrame() reparents the frame, hence we only need to reparent frame view after that. I'll add comment there.

BTW, what about patch 2?

Comment on attachment 8508341 [details] [diff] [review] patch 2 - Simplify reflow code (My previous comments on patch 2 were in comment 30.) nsFrame.cpp: These changes are fine, but should have been a separate patch since they're separate from the rest of the code. The patch description should have been: Treat ruby text containers as inline elements for the purposes of finding a font inflation container so that ruby text gets the same font inflation as its ruby bases. nsLineLayout.h: This should also have been its own patch. The changes look fine. The patch description should have been: Add assertion and comments pointing out that ruby base container frames do not have an mBlockRS, but that's OK since they are never the containing block for floats. nsRubyBaseContainerFrame.cpp: I'm not convinced that nsLineLayout can be safely copy-constructed given its mArena member. The copy will end up having a destroyed arena after the original is destroyed. It would be better to avoid copy-construction if possible, and perhaps make it harder to mess up on the nsLineLayout side as well. >+ nscoord maxISize = 0; >+ nscoord pairISize = 0; This needs a comment to explain what it is doing. After reading the code carefully I finally figured it out, but it should be explained, perhaps with a comment like: // Any ruby texts that do not have following siblings span all // remaining bases. Therefore, for this pair, we want to consider the // widths of only texts and bases that have following siblings // (because they do not span), unless we're actually at the last pair, // in which case everything ends here and we want to consider // all texts and bases. Now that I understand what this is doing, I can point out that this doesn't work correctly in the presence of line breaking. For example, if you have <ruby> <rbc><rb>A</rb><rb>B</rb></rbc> <rtc><rt>a</rb></rtc> </ruby> and there is a line break between A and B, then when you reflow a second time (after doing the line breaking), you will end up with allFramesAreLastChild being true even though <rb>A</rb> conceptually has a next sibling; it's just in the rbc's continuation. You could fix that by using a helper function that looks in the parent's next-in-flow for a sibling, though it seems like there should perhaps be a better way to handle that. >+ nsHTMLReflowMetrics metrics(frameWM, aDesiredSize.mFlags); Maybe keep using the nsHTMLReflowMetrics constructor that takes an nsHTMLReflowState, and pass reflowStates[i]? >+ nsAutoTArray<nsHTMLReflowState, RTC_ARRAY_SIZE> reflowStates; >+ nsAutoTArray<nsLineLayout, RTC_ARRAY_SIZE> lineLayouts; Maybe add a brief comment pointing out that we have a reflow state for each RTC and a line layout for the children of each RTC? And I think (assuming I'm correct) pointing out that the reflow state is conceptually the reflow state for the rtc, but we don't actually reflow the rtc in this code. >+ nscoord curICoord = lineLayouts[i].GetCurrentICoord(); >+ if (curICoord < icoord) { >+ lineLayouts[i].AdvanceICoord(icoord - curICoord); It seems like you should be able to just use AdvanceICoord(icoord) like you do on the "outer" line layout, at least for now. If you can't, you should have a comment explaining why not. (Is it because you've already added some spacing, or will in the future?) >+ mTextContainers[i]->SetISize(isize); Could you add a comment explaining that this happens before the ruby text container is reflowed, and that when it is reflowed it will just use this size? From comment 32: >> Also, with the new code, what keeps the ruby centered over the base >> when the ruby is narrower than the base? > >No code does that. This feature is temporary removed. I mentioned that in comment 5 but forgot to include it in the commit message. Sorry about that. This should be mentioned in the commit message, and a followup bug should be filed that is also mentioned in the commit message. You should also file a followup bug on making spanning ruby texts contribute to the width of the ruby when they need to; your current patch appears to just ignore their width, even if it is wider than all spanned bases. (And in general, if you know that things are wrong, you should really have that in the commit message and/or in code comments. Then I don't have to spend the time to figure out things that you already know and could have told me.) From your commit message: >Important changes: These should all be explained more clearly, e.g., as follows: > * Use metrics instead of GetPrefISize Avoid using GetPrefISize on the ruby texts in nsRubyBaseContainerFrame::Reflow, since the size it produces might not match the size produced by Reflow. The old code calls GetPrefISize on all the ruby texts (to determine how big they are), then reflows all the ruby bases, and then reflows all the ruby texts. The new code instead processes one pair at a time, and for each pair reflows the ruby texts and then the ruby base. For now we don't worry about stretching the ruby text frames (and probably centering the text in those frames) to match the widths of the ruby bases, but that will be fixed in bug [IS THIS THE BUG ON ruby-position OR A SEPARATE BUG?]. > * Change base class of nsRubyTextContainerFrame to nsContainerFrame Change the base class of nsRubyTextContainerFrame from nsBlockFrame to nsContainerFrame, and stop constructing an nsBlockReflowState for its reflow. > * Move ruby text reflow code to nsRubyBaseContainerFrame Move the code for reflowing ruby texts from nsRubyTextContainerFrame and to nsRubyBaseContainerFrame. r=dbaron with those things fixed

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #64) > Comment on attachment 8508341 [details] [diff] [review] > patch 2 - Simplify reflow code > > nsRubyBaseContainerFrame.cpp: > > I'm not convinced that nsLineLayout can be safely copy-constructed > given its mArena member. The copy will end up having a destroyed > arena after the original is destroyed. It would be better to avoid > copy-construction if possible, and perhaps make it harder to mess up > on the nsLineLayout side as well. > > >+ nscoord maxISize = 0; > >+ nscoord pairISize = 0; > > This needs a comment to explain what it is doing. After reading the > code carefully I finally figured it out, but it should be explained, > perhaps with a comment like: > > // Any ruby texts that do not have following siblings span all > // remaining bases. Therefore, for this pair, we want to consider the > // widths of only texts and bases that have following siblings > // (because they do not span), unless we're actually at the last pair, > // in which case everything ends here and we want to consider > // all texts and bases. > > Now that I understand what this is doing, I can point out that this > doesn't work correctly in the presence of line breaking. For example, > if you have > <ruby> > <rbc><rb>A</rb><rb>B</rb></rbc> > <rtc><rt>a</rb></rtc> > </ruby> > and there is a line break between A and B, then when you reflow a second > time (after doing the line breaking), you will end up with > allFramesAreLastChild being true even though <rb>A</rb> conceptually has > a next sibling; it's just in the rbc's continuation. > > You could fix that by using a helper function that looks in the parent's > next-in-flow for a sibling, though it seems like there should perhaps > be a better way to handle that. Sorry, I wrote the comments for these variables in the wrong patch. You can see more detailed comments in the latest patch 5. I'll move them back to this patch.

Comment on attachment 8517203 [details] [diff] [review] patch 5 - Implement basic line breaking I feel it might cause problems that I use the content of ruby frame to track the break position, because ruby frame does not always have its own content node. A ruby frame can be a pseudo frame. If multiple incontinuous pseudo ruby frames are constructed in a same parent, they will use incorrect break positions. David, do you have any suggestion for this problem?

Comment on attachment 8511608 [details] [diff] [review] patch 0 - Rewrite pref isize and min isize computation >+ // The word "pair" in this method means the smallest pair with at >+ // most one base frame and no more than one text frame in each level. >+ // The word "unbreakable" in the other hand means pairs considering >+ // spans which consist of multiple pairs because of the shortage of >+ // text frames. This is a little bit unclear. Does the following comment match what you're trying to say: // Ruby texts are associated with ruby bases by matching indices in // the ruby base container with indices in the ruby text container. // These sets of ruby base plus one or more ruby texts are known as // "pairs". However, if one of the ruby text containers has fewer // items, its last item spans all the remaining bases. (This is not // true if the ruby base container is short items.) So this method is // concerned with "unbreakable" units, where multiple pairs are // unbreakable if there is a ruby text that spans across them. But also see: http://lists.w3.org/Archives/Public/www-style/2014Nov/0110.html which makes me wonder if we should do this at all. >+ // Every base container frame could be followed by several text containers, >+ // which compose a segment together. According to Unit Pairing and Spanning >+ // Annotations in CSS Ruby, number of unbreakable pairs in each segment is >+ // min(number of children of each non-empty text container). "which compose a segment together" -> "which together make a segment". "According to" -> "According to the section" "number of" -> "the number of" (Also, the comment seems wrong if there are no text containers at all, in which case it would be the number of children in the base container.) >+ // Sort spans from the smallest (the maxmium start index) to >+ // the largest (the minimum start index) >+ std::sort(lastFrames.Elements(), >+ lastFrames.Elements() + lastFrames.Length(), >+ [](FrameInfo lhs, FrameInfo rhs) { >+ return lhs.mIndex > rhs.mIndex; >+ }); Using sort is unnecessarily complex here. In fact, a lot of the method seems unnecessarily complex. Why can't you just: * have the pairSizes array, but get rid of lastFrames * whenever you hit a ruby text container frame that has fewer children than the count in the pairSizes array, merge the later elements in pairSizes into its last element * move this to be common code that lives outside of the type checks: > if (!nextSibling || > nextSibling->GetType() != nsGkAtoms::rubyTextContainerFrame) { > // There is no ruby text container next to this base container. > // We have to append the pairs to unbreakables now. > aUnbreakableSizes.AppendElements(pairSizes); > } (or alternatively, and perhaps preferably, move it to the rubyBaseContainerFrame case and then just repeat it at the end of the method)

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #68) > I feel it might cause problems that I use the content of ruby frame to track > the break position, because ruby frame does not always have its own content > node. A ruby frame can be a pseudo frame. If multiple incontinuous pseudo > ruby frames are constructed in a same parent, they will use incorrect break > positions. > > David, do you have any suggestion for this problem? You mean that you're using the ruby frame's content node in calls to NotifyOptionalBreakPosition? I guess whether that's ok depends on what the ruby frame's content node ends up being when we construct an anonymous ruby frame (I don't know what we do here), and whether that content node might end up with other calls to NotifyOptionalBreakPosition from other callers.

(In other words, I'm less sure about the comment I made on IRC last night, when you asked whether using the first-in-flow was better, and I said it sounded like it was, now that I have a better understanding of what question you were asking.)

(I didn't realize last night that you were talking about calls to NotifyOptionalBreakPosition. Although I guess I'm still not sure that you are.)

Yes, I was talking about calls to NotifyOptionalBreakPosition. Currently an anonymous ruby frame's content node is the node contains the ruby frame. For example, if we have <p><rb></rb> hello <rb></rb></p> the frame tree would be something like <p><ruby><rbc><rb></rb></rbc></ruby> hello <ruby><rbc><rb></rb></rbc></ruby></p> and the two ruby frames end up sharing the same content node of <p>. I suspected that if one of them has a force break, both of them would be broken, but it is not true. I have no idea why.

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #69) > Comment on attachment 8511608 [details] [diff] [review] > patch 0 - Rewrite pref isize and min isize computation > > >+ // The word "pair" in this method means the smallest pair with at > >+ // most one base frame and no more than one text frame in each level. > >+ // The word "unbreakable" in the other hand means pairs considering > >+ // spans which consist of multiple pairs because of the shortage of > >+ // text frames. > > This is a little bit unclear. Does the following comment match > what you're trying to say: > > // Ruby texts are associated with ruby bases by matching indices in > // the ruby base container with indices in the ruby text container. > // These sets of ruby base plus one or more ruby texts are known as > // "pairs". However, if one of the ruby text containers has fewer > // items, its last item spans all the remaining bases. (This is not > // true if the ruby base container is short items.) So this method is > // concerned with "unbreakable" units, where multiple pairs are > // unbreakable if there is a ruby text that spans across them. > > But also see: > http://lists.w3.org/Archives/Public/www-style/2014Nov/0110.html > which makes me wonder if we should do this at all. If it ends up that we do not need to do this, much of the code can be removed here and in patch 5. I have replied that email and mentioned other complexity spanning introduces. Hopefully spanning could be removed, so that everything would be easier: not only this bug, but also bug 1055676 and bug 1081770. Shall we wait for their reply? > >+ // Sort spans from the smallest (the maxmium start index) to > >+ // the largest (the minimum start index) > >+ std::sort(lastFrames.Elements(), > >+ lastFrames.Elements() + lastFrames.Length(), > >+ [](FrameInfo lhs, FrameInfo rhs) { > >+ return lhs.mIndex > rhs.mIndex; > >+ }); > > Using sort is unnecessarily complex here. In fact, a lot of the method > seems unnecessarily complex. > > Why can't you just: > * have the pairSizes array, but get rid of lastFrames If so, we can't process spans correctly. > * whenever you hit a ruby text container frame that has fewer children > than the count in the pairSizes array, merge the later elements in > pairSizes into its last element It would cause the problem I mentioned in comment 45.

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #45) > (In reply to David Baron [:dbaron] (UTC-4) (needinfo? for questions) from > comment #44) > Though, I cannot confirm that my code is very correct, because I didn't find > a good way to test those code. Any suggestion? The most straightforward way to test these is to put the ruby markup in question inside of a block with 'width: -moz-min-content' (for GetMinISize) or 'width: -moz-max-content' (for GetPrefISize). (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #53) > I met some assertions because of using nsLineLayout in non-block frame > during running some reftests without "white-space: nowrap" being specified. > > It seems that nsLineLayout put the outer reflow state in > "mBlockReflowState", which is the source of all the assertions. After > browsering the code, I think there are two main areas which non-blocks > should not touch, or should pass to its nearest block ancestor: > 1. Handling floats. This might be the main source of NS_ASSERTION. rtc should never end up being a containing block for floats, so this shouldn't be a serious problem > 2. Styles only applied to block. This problem is more indiscoverable (but > less risky as well) since they will be applied without assertions around. It > is also a problem we had before changing the base class of > nsRubyTextContainer, because rtc is never a block element in the spec. I don't know what you mean here. > To solve these problems, I think what we should do is to check every > reference of mBlockReflowState, and divide them into two categories: one > indeed requires a block frame, and the other doesn't. Then separate the > mBlockReflowState into two members, say mOuterReflowState and > mNearestBlockReflowState. After that, we use mOuterReflowState for cases in > the second category, and make the references in first category either skip > or use mNearestBlockReflowState instead. I assume you're talking about nsLineLayout::mBlockReflowState. I don't think this helps much, because in the cases where you do need an nsBlockFrame, using the next block ancestor doesn't make particular sense. (Except perhaps for floats, except that should already be fine.) > Some code in nsInlineFrame and nsTextFrame also needs to be changed in a > similar way. I don't know what you mean here. > Do you think it is a reasonable project? Or we should change the base class > back to nsBlockFrame and try to find a way to maintain the continuations of > it differently? The current approach seems OK so far, although I'm not sure what problems you were hitting. (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #54) > BTW, the current implementation does not match the spec in two ways: > 1. The spec says whether line can break between two ruby bases is controlled > by normal line-breaking rules. [1] But currently, I'm making it always > possible to break between pairs when no span across them. > 2. The spec says it is possible to break inside a ruby base if the > line-breaking rules allow. [2] Currently I doesn't handle this case at all, > and hope they will never be able to break inside. > > For the both cases, I didn't find any reasonable use case or example from > the spec or JLREQ which depends on those behaviors. I wonder should we obey > the spec or get the spec changed? > > In addition, if we don't want to allow any line-breaking inside the ruby > base and text, what can we do to force this requirement? My guess would be that the main use case for this rule would be for Korean, when Korean uses Chinese characters as ruby over Hangul. (I still need to respond to comment 57, comment 73, and comment 74.)

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #74) > (In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from > comment #69) > If it ends up that we do not need to do this, much of the code can be > removed here and in patch 5. I have replied that email and mentioned other > complexity spanning introduces. Hopefully spanning could be removed, so that > everything would be easier: not only this bug, but also bug 1055676 and bug > 1081770. Shall we wait for their reply? Probably briefly, and then just go ahead. We might want to ask fantasai directly. (I still need to respond to comment 57 and comment 73.)

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #73) > Yes, I was talking about calls to NotifyOptionalBreakPosition. Currently an > anonymous ruby frame's content node is the node contains the ruby frame. For > example, if we have > > <p><rb></rb> hello <rb></rb></p> > > the frame tree would be something like > > <p><ruby><rbc><rb></rb></rbc></ruby> hello > <ruby><rbc><rb></rb></rbc></ruby></p> > > and the two ruby frames end up sharing the same content node of <p>. I > suspected that if one of them has a force break, both of them would be > broken, but it is not true. I have no idea why. Yes, that's problematic. I'm not sure what we'd want to do about it, although I suspect we need to somehow change the NotifyOptionalBreakPosition API. It's probably worth discussing that on dev-tech-layout in case other people see problems with it. (And maybe using the first-continuation will work, although there is a performance cost of getting to the first continuation.)

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #77) > (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #73) > > Yes, I was talking about calls to NotifyOptionalBreakPosition. Currently an > > anonymous ruby frame's content node is the node contains the ruby frame. For > > example, if we have > > > > <p><rb></rb> hello <rb></rb></p> > > > > the frame tree would be something like > > > > <p><ruby><rbc><rb></rb></rbc></ruby> hello > > <ruby><rbc><rb></rb></rbc></ruby></p> > > > > and the two ruby frames end up sharing the same content node of <p>. I > > suspected that if one of them has a force break, both of them would be > > broken, but it is not true. I have no idea why. > > Yes, that's problematic. I'm not sure what we'd want to do about it, > although I suspect we need to somehow change the NotifyOptionalBreakPosition > API. It's probably worth discussing that on dev-tech-layout in case other > people see problems with it. I have filed bug 1096152 about this. > (And maybe using the first-continuation will > work, although there is a performance cost of getting to the first > continuation.) I agree that there is a performance cost. I guess we can use the current frame directly instead of the first continuation? Is there any concern that we cannot do that?

It is proven that neither nsHTMLReflowState nor nsLineLayout can be stored inside a nsTArray directly, not even wrapped in a Maybe. For nsHTMLReflowState, the reason is that there are pointers refer to it, so the location must not be moved after it gets initialized. For nsLineLayout, the arean. mozilla::Maybe uses its internal member variable to store the data, hence it's not helpful for avoiding movement. I guess we should add some comments there to notice newcomers about this.

According to discussion in the mailing list: http://lists.w3.org/Archives/Public/www-style/2014Nov/0275.html , the spec will be updated, and the spanning case will be simplified to only when an <rtc> does not have any explicit <rt> children. I'll update the patches accordingly soon.

Comment on attachment 8523568 [details] [diff] [review] patch 0 - Rewrite pref isize and min isize computation Sorry, but I have changed this patch significantly.

Compared to the previous version, this patch additionally changes code in nsRubyFrame::Reflow.

This was part of patch 5.

Using float in text containers will crash in the assertion added in patch 2.2, hence we shouldn't do that. This patch uses the block reflow state of the line layout of the base container to initialize the line layouts of text containers. It works fine as the owner of that block reflow state is the actual handler of floats. But as the comment mentions, I don't think it is the final solution. Since we may have to make the RTC line layouts dependent in bug 1081770, I prefer to solve it then.

FYI, the spec has been updated to reflect the decision about spanning: http://dev.w3.org/csswg/css-ruby/#base-annotation-pairing

Comment on attachment 8524538 [details] [diff] [review] patch 0 - Rewrite pref isize and min isize computation (previous review of this patch was comment 69) Please comment above the definition of GetLevelCount() that it include the base level plus the annotation levels. With the separation of mSpanContainers and mTextContainers, how are you going to implement the sentence in the spec that says: If multiple ruby annotation containers have the same ruby-position, they stack along the block axis, with lower levels of annotation closer to the base text. It seems like it might be better to keep them in one array. Then again, I guess you could fix that later if you need to; this is probably fine for now. I guess this is ignoring margin, border, and padding on the rtc and rbc elements. There should be a followup bug on making intrinsic size and reflow consistent on whether to honor this or ignore it. (I think we honor it in reflow, and I tend to think that's probably better.) >+/* virtual */ void >+nsRubyBaseContainerFrame::AddInlineMinISize( >+ nsRenderingContext *aRenderingContext, nsIFrame::InlineMinISizeData *aData) >+{ >+ nscoord max = 0; >+ PairEnumerator enumerator(this, mTextContainers); >+ for (; !enumerator.AtEnd(); enumerator.Next()) { >+ max = std::max(max, CalculatePairPrefISize(aRenderingContext, enumerator)); You should have a comment pointing out that you're computing the *pref* ISize computation for the pair because ruby text and bases are unbreakable internally. >+ } >+ max = std::max(max, CalculateMaxSpanISize(aRenderingContext)); If there are spans, are they allowed to break in the middle? If not, then the presence of spans should mean that AddInlineMinISize just returns the same as AddInlinePrefISize. >+ * The arrays of ruby text containers below are filled before the ruby >+ * frame (parent) start reflowing this ruby segment, and cleared when start -> starts >+ * the reflow finishes. > */ >+ >+ // The ruby containers that contains a span, which spans all ruby contains -> contain >+ // pairs in the ruby segment. >+ nsTArray<nsRubyTextContainerFrame*> mSpanContainers; >+ // Normal ruby containers that contains multiple ruby text frame contains -> contain frame -> frames I also think you should drop "multiple" since they might contain only one ruby text frame. I don't think the name SegmentHolder is very clear. How about AutoSetTextContainers? Its constructor should also assert that aBaseContainer currently has no text containers -- perhaps through a debug-only method (AssertTextContainersEmpty) on nsRubyBaseContainerFrame. >+ * This enumerator enumerates each segment. It automatically append append -> appends >+ * and clear text containers of the base container. That said, where does this enumerator (SegmentEnumerator) append and clear text containers of the base container? Are you sure you didn't intend this comment for SegmentHolder? >+SegmentEnumerator::SegmentEnumerator(nsRubyFrame* aRubyFrame) >+{ >+ nsIFrame* frame = aRubyFrame->GetFirstPrincipalChild(); >+ MOZ_ASSERT(!frame || >+ frame->GetType() == nsGkAtoms::rubyBaseContainerFrame); >+ mBaseContainer = static_cast<nsRubyBaseContainerFrame*>(frame); >+} Does the frame construction code guarantee this MOZ_ASSERT is safe by constructing an anonymous empty nsRubyBaseContainerFrame when a ruby element starts with a ruby text container? (It would be good to have a reftest testing this case.) Maybe SegmentEnumerator::Next should assert that mBaseContainer is non-null, with the assertion text saying that the caller should have called AtEnd? r=dbaron with those things fixed

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #93) > Comment on attachment 8524538 [details] [diff] [review] > patch 0 - Rewrite pref isize and min isize computation > > > (previous review of this patch was comment 69) Since the code in this patch has been totally rewritten, it seems that the previous review dosesn't make much sense for the new patch. > With the separation of mSpanContainers and mTextContainers, how are > you going to implement the sentence in the spec that says: > If multiple ruby annotation containers have the same ruby-position, > they stack along the block axis, with lower levels of annotation > closer to the base text. > It seems like it might be better to keep them in one array. Then > again, I guess you could fix that later if you need to; this is > probably fine for now. It is implement in nsRubyFrame, where the order of the text containers are still kept. You may see my patches for bug 1055665 which has implemented this on top of patches here. > I guess this is ignoring margin, border, and padding on the rtc and > rbc elements. There should be a followup bug on making intrinsic size > and reflow consistent on whether to honor this or ignore it. (I think > we honor it in reflow, and I tend to think that's probably better.) Oh, okay, I'll add them in a separate patch in this bug. > >+/* virtual */ void > >+nsRubyBaseContainerFrame::AddInlineMinISize( > >+ nsRenderingContext *aRenderingContext, nsIFrame::InlineMinISizeData *aData) > >+{ > >+ nscoord max = 0; > >+ PairEnumerator enumerator(this, mTextContainers); > >+ for (; !enumerator.AtEnd(); enumerator.Next()) { > >+ max = std::max(max, CalculatePairPrefISize(aRenderingContext, enumerator)); > > You should have a comment pointing out that you're computing the *pref* > ISize computation for the pair because ruby text and bases are unbreakable > internally. > > >+ } > >+ max = std::max(max, CalculateMaxSpanISize(aRenderingContext)); > > If there are spans, are they allowed to break in the middle? If not, > then the presence of spans should mean that AddInlineMinISize just > returns the same as AddInlinePrefISize. Ah, that's right. That simplifies the code. > I don't think the name SegmentHolder is very clear. How about > AutoSetTextContainers? > > Its constructor should also assert that aBaseContainer currently > has no text containers -- perhaps through a debug-only method > (AssertTextContainersEmpty) on nsRubyBaseContainerFrame. > > >+ * This enumerator enumerates each segment. It automatically append > > append -> appends > > >+ * and clear text containers of the base container. > > That said, where does this enumerator (SegmentEnumerator) append > and clear text containers of the base container? Are you sure > you didn't intend this comment for SegmentHolder? Oops, I forgot to remove the comment here. I did use this class for that, but finally I decided to separate them so that everything looks more clear. > >+SegmentEnumerator::SegmentEnumerator(nsRubyFrame* aRubyFrame) > >+{ > >+ nsIFrame* frame = aRubyFrame->GetFirstPrincipalChild(); > >+ MOZ_ASSERT(!frame || > >+ frame->GetType() == nsGkAtoms::rubyBaseContainerFrame); > >+ mBaseContainer = static_cast<nsRubyBaseContainerFrame*>(frame); > >+} > > Does the frame construction code guarantee this MOZ_ASSERT is safe > by constructing an anonymous empty nsRubyBaseContainerFrame when a ruby > element starts with a ruby text container? (It would be good to have > a reftest testing this case.) Yes, it is the bug 1083004 which I'm going to land together with this bug. We will have tests for that in bug 1088489, in which I wrote a bunch of tests for box generation.

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #94) > (In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from > comment #93) > > I guess this is ignoring margin, border, and padding on the rtc and > > rbc elements. There should be a followup bug on making intrinsic size > > and reflow consistent on whether to honor this or ignore it. (I think > > we honor it in reflow, and I tend to think that's probably better.) > > Oh, okay, I'll add them in a separate patch in this bug. Please use a separate bug instead.

Comment on attachment 8524539 [details] [diff] [review] patch 1 - Use unified enumerators to simplify code previous review was in comment 11, which was a review+. Why is it up for review again? It looks like most of the code is the same, but some of it is rebased on top of patch 0. You really shouldn't be asking for re-reviews like this; it's a lot of unneeded work for me. Either stop posting patches that aren't ready to land, or structure the additional changes as new changes on top of the already-reviewed ones. (In this case, you should have not requested re-review on the nsRubyBaseContainerFrame changes here, and you should have posted the nsRubyFrame changes here as a separate new patch.) Have other changes since addressed this part of comment 11? >>- // Update the inline coord to make space for subsequent ruby annotations >>- // (since there is no corresponding base inline size to use). >> baseStart += rtcMetrics.ISize(lineWM); > >This addition should be conditional on there being no base frame. And >you should leave the comment. (I think that's the easiest path for >now, and the one by which this patch doesn't change behavior, which I >think was the intent.) > >That said, we should really handle better the case of a ruby text >that is wider than the ruby base (unless that's handled some other >way I don't see). > >And, also, the code is wrong since it's adding for every rtc rather than just >taking the largest. > >There should probably be a test that fails for what you broke here. > >So you should file a followup bug on those issues, blocking bug >enable-css-ruby, on both fixing this and adding some tests. And you >should also add a reftest for what you broke above, here. Please make sure you add tests for these issues and file any necessary followup bugs. In nsRubyFrame::Reflow, could you reflow the following variables that are used only for the annotation container: frame -> textContainer reflowStatus -> textReflowStatus reflowState -> textReflowState metrics -> textMetrics so that they're like the ones used for the ruby text container. r=dbaron with that

Comment on attachment 8524540 [details] [diff] [review] patch 2.3 - Rewrite reflow code. Review of attachment 8524540 [details] [diff] [review]: ----------------------------------------------------------------- I have found some problem in this patch, and will change some of the code in a separate patch. ::: layout/generic/nsRubyBaseContainerFrame.cpp @@ +302,5 @@ > > + nsTArray<UniquePtr<nsHTMLReflowState>> spanReflowStates(spanCount); > + for (uint32_t i = 0; i < spanCount; i++) { > + spanReflowStates.AppendElement(Move(reflowStates[i + rtcCount])); > + } There is no need to use nsTArray<UniquePtr<>> and move the ownership here. Use raw pointers should be better, as moving UniquePtr potentially causes inconsistent life time. (This actually introduces a bug in followup patches.)

Incremental patch for patch 2.3. Solve the problem mentioned in comment 97.

Comment on attachment 8524540 [details] [diff] [review] patch 2.3 - Rewrite reflow code. previous review was in comment 64, which was a review+. Since then patch 2.1 (attachment 8519606 [details] [diff] [review]) and patch 2.2 (attachment 8519607 [review]) have been separated out from it and marked review+ by Xidorn. (Those are the parts I suggested could have been separate patches in comment 64.) But comment 91 obsoleted patch 2.2 and replaced it by additional changes to this patch. There's no indication in comment 86 as to why this is a re-review. The additional changes over the review comments on the previous patch seem to be primarily adjustments on the split of mSpanContainers, and adjustments to deal with the simplification on the rules on spanning in the spec. You didn't really address this from comment 64: > >+ nscoord curICoord = lineLayouts[i].GetCurrentICoord(); > >+ if (curICoord < icoord) { > >+ lineLayouts[i].AdvanceICoord(icoord - curICoord); > > It seems like you should be able to just use AdvanceICoord(icoord) like > you do on the "outer" line layout, at least for now. If you can't, you > should have a comment explaining why not. > > (Is it because you've already added some spacing, or will in the > future?) ...although maybe that was partly because I meant to ask why you can't just use AdvanceICoord(pairSize). >+ nsTArray<UniquePtr<nsHTMLReflowState>> spanReflowStates(spanCount); Probably nsAutoTArray with RTC_ARRAY_SIZE. >+ if (isize < spanISize) { >+ aReflowState.mLineLayout->AdvanceICoord(spanISize - isize); >+ isize = spanISize; >+ } Is there a bug on properly spacing out the pairs when the span is bigger? (And does the spec say what to do?) If there isn't, please file one. r=dbaron with that

Comment on attachment 8525796 [details] [diff] [review] change to patch 2.3 I think we should not do this here, but instead file a separate bug on getting the frame construction right and fixing that crash.

Could you provide a diff that shows both patch 5.1 and 5.2 in a single diff, so that I can compare it to the previous patch, which I reviewed?

Note that this diff is slightly different from the patch 5.1 and 5.2 here because I have rebased attachment 8528143 [details] [diff] [review] in this version.

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #96) > Comment on attachment 8524539 [details] [diff] [review] > patch 1 - Use unified enumerators to simplify code > > previous review was in comment 11, which was a review+. > > Why is it up for review again? It looks like most of the code is the > same, but some of it is rebased on top of patch 0. You really shouldn't > be asking for re-reviews like this; it's a lot of unneeded work for me. > Either stop posting patches that aren't ready to land, or structure the > additional changes as new changes on top of the already-reviewed ones. > (In this case, you should have not requested re-review on the > nsRubyBaseContainerFrame changes here, and you should have posted the > nsRubyFrame changes here as a separate new patch.) Sorry about that. > Have other changes since addressed this part of comment 11? > > >>- // Update the inline coord to make space for subsequent ruby annotations > >>- // (since there is no corresponding base inline size to use). > >> baseStart += rtcMetrics.ISize(lineWM); > > > >This addition should be conditional on there being no base frame. And > >you should leave the comment. (I think that's the easiest path for > >now, and the one by which this patch doesn't change behavior, which I > >think was the intent.) > > > >That said, we should really handle better the case of a ruby text > >that is wider than the ruby base (unless that's handled some other > >way I don't see). > > > >And, also, the code is wrong since it's adding for every rtc rather than just > >taking the largest. > > > >There should probably be a test that fails for what you broke here. > > > >So you should file a followup bug on those issues, blocking bug > >enable-css-ruby, on both fixing this and adding some tests. And you > >should also add a reftest for what you broke above, here. > > Please make sure you add tests for these issues and file any necessary > followup bugs. I don't think it worth that. It has been discussed in comment 13, comment 14, comment 15. Since the regression (not handling spans correctly) has been made in patch 0, and I have mentioned it in the commit message of that patch, and it will be fixed in patch 2.3, I don't think we need to address it anymore.

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #104) > I don't think it worth that. It has been discussed in comment 13, comment > 14, comment 15. Since the regression (not handling spans correctly) has been > made in patch 0, and I have mentioned it in the commit message of that > patch, and it will be fixed in patch 2.3, I don't think we need to address > it anymore. OK, that's fine, assuming patch 2.3 has a test for it.

Comment on attachment 8524551 [details] [diff] [review] patch 5.1 - Separate reflow code to some individual methods. nsRubyFrame::ReflowSegment looks like it could be a static method; I don't see any use of |this| (though maybe I'm missing it). In ReflowOnePair: >+ if (aTextFrames[i]) { You should probably assert that aTextFrames[i]->GetType() is what you expect. >+ NS_ASSERTION(!NS_INLINE_IS_BREAK(reflowStatus), >+ "Ruby line breaking is not yet implemented"); One fewer space on the second line. >+ NS_ASSERTION(!NS_INLINE_IS_BREAK(reflowStatus), >+ "Ruby line breaking is not yet implemented"); Same. >+ aReflowState.mLineLayout->AdvanceICoord( >+ icoord - aReflowState.mLineLayout->GetCurrentICoord()); Two fewer spaces on the second line. r=dbaron with that

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #99) > Comment on attachment 8524540 [details] [diff] [review] > patch 2.3 - Rewrite reflow code. > > You didn't really address this from comment 64: > > >+ nscoord curICoord = lineLayouts[i].GetCurrentICoord(); > > >+ if (curICoord < icoord) { > > >+ lineLayouts[i].AdvanceICoord(icoord - curICoord); > > > > It seems like you should be able to just use AdvanceICoord(icoord) like > > you do on the "outer" line layout, at least for now. If you can't, you > > should have a comment explaining why not. > > > > (Is it because you've already added some spacing, or will in the > > future?) > > ...although maybe that was partly because I meant to ask why you can't > just use AdvanceICoord(pairSize). I meant to handle the case that there is a span which is longer than the current pair. It is no longer needed since now we handle span separately. > >+ nsTArray<UniquePtr<nsHTMLReflowState>> spanReflowStates(spanCount); > > Probably nsAutoTArray with RTC_ARRAY_SIZE. > > >+ if (isize < spanISize) { > >+ aReflowState.mLineLayout->AdvanceICoord(spanISize - isize); > >+ isize = spanISize; > >+ } > > Is there a bug on properly spacing out the pairs when the span is > bigger? (And does the spec say what to do?) If there isn't, please > file one. The spec dosen't say anything about it yet. I have asked in the list. Although fantasai has replied me, details are still unclear. I'm going to solve it with ruby-align together, but if the spec is still not clear about it when I start working on ruby-align, I'll file a separate bug.

Comment on attachment 8524552 [details] [diff] [review] patch 5.2 - Implement basic line breaking So for patches 5.1 and 5.2, the previous patch was patch 5, which I reviewed in comment 51, comment 56, and comment 58. (It's probably best to land them separately given that you've split them -- or if you land them together, you should certainly fix the commit message to be "Implement basic line breaking for ruby.".) It looks like much of the old patch moved into patch 0. Could you comment above the declaration of mPairCount that it's only used for state during reflow? In ShouldBreakBefore, I don't understand your use of GetLastOptionalBreakPosition. Do you mean to be using NotifyOptionalBreakPosition instead? (Presumably you need to pass a useful offset in.) Other than that, though, I gave up on re-reviewing the changes since this patch was mostly ok the previous time. But I'll go through my previous comments: Was there a reason you didn't do this bit of comment 56: > >+ if (GetNextInFlow() && !isComplete) { > >+ // What ever happens in the reflow loop, if there still exist > >+ // next-in-flows and we haven't pull all of their children, > >+ // the status shouldn't be set complete. > >+ NS_FRAME_SET_INCOMPLETE(aStatus); > >+ } > > I think this is a fine thing to do for safety, but I think, inside > the if, you should assert that NS_FRAME_IS_COMPLETE(aStatus) is already > true, before setting it. I notice the comment is gone now too. (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #57) > (In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) > (away/busy Oct. 24-31) from comment #56) > > However, I'm a little concerned here -- why are you distinguishing > > between empty and nonempty continuations? If you distinguish, it seems > > like there's a risk of losing track of the frames that are empty > > continuations rather than deleting them when needed. > > Because nonempty continuations don't contain any frame, do they? I consulted > nsInlineFrame::PullOneFrame to write the functions, and the prototype of > this structure is mNextInFlow of InlineReflowState. > > About the risk of losing track, AFAIK, deleting them is the work of > nsLineLayout::ReflowFrame, after a frame reports complete. As commented in > nsRubyFrame::ReflowSegment (find "DeleteNextInFlowChild"), when the ruby > base container reports complete, we will remove the next-in-flows of text > containers. That structure is never used to track this. The only function of > the struct is to make finding the next pullable frame quicker. OK. > I mentioned that in comment 54. Filed bug 1089431 for this. In addition, the > spec also allows break inside ruby base, which I found hard to implement > now, and think it doesn't even worth to implement for now. Should I file > another bug for that as well? Even if yes, I don't think it should block > enable-css-ruby. Yes, another bug sounds great, and I'm fine with it not blocking enable-css-ruby. > > >+ if (aStatus != NS_FRAME_COMPLETE) { > > > > I suspect you want NS_FRAME_IS_COMPLETE(aStatus) instead, although > > really instead you might want to assert that they're equivalent, since > > you're not handling NS_FRAME_OVERFLOW_IS_INCOMPLETE (nor should you be). > > By that way, I have to check NS_INLINE_BREAK_BEFORE(aStatus) in addition, > because complete flag is not reliable when break before happens. I think it > might be better to assert NS_FRAME_OVERFLOW_IS_INCOMPLETE is false with this > condition unchanged. Do you agree with that? I think asserting !NS_FRAME_OVERFLOW_IS_INCOMPLETE(aStatus) is good. But what's wrong with changing the condition to !NS_FRAME_IS_COMPLETE(aStatus)? > > >+ canPlace = !NS_INLINE_IS_BREAK(reflowStatus); > > >+ if (!canPlace) { > > >+ // If any breaking occurs when reflowing a ruby text frame, > > >+ // we should abandon reflowing this pair. > > >+ break; > > >+ } > > >+ MOZ_ASSERT(!pushedFrame, "Shouldn't push frame if there is no break"); > > > > I suspect this is ok, but I'm not 100% sure. It's worth checking > > to see if there are other cases where we reflow an inline frame, see > > that it breaks, and then reflow it again before creating and reflowing > > its next continuation. (I know blocks do this sort of thing, but I'm > > not sure inlines do.) Or, alternatively, it's worth checking that > > inlines handle this case, and deal with things being on their own > > overflow list when they're being reflowed. > > With the current code of nsLineLayout, it is 100% sure that pushedFrame can > be true only when reflowStatus is NS_INLINE_BREAK_BEFORE: > The only place sets aPushedFrame to true: > http://dxr.mozilla.org/mozilla-central/source/layout/generic/nsLineLayout. > cpp#1060 > The condition is that CanPlaceFrame returns false. Then: > CanPlaceFrame always set aStatus to NS_INLINE_BREAK_BEFORE before returning > false: > http://dxr.mozilla.org/mozilla-central/source/layout/generic/nsLineLayout. > cpp#1302 OK. > > >+ if (NS_INLINE_IS_BREAK(reflowStatus)) { > > >+ // It is assumed that ruby base frame is unbreakable, > > >+ // hence it should never break after. > > >+ MOZ_ASSERT(NS_INLINE_IS_BREAK_BEFORE(reflowStatus), > > >+ "Currently we only process this only case"); > > >+ break; > > > > What ensures this? > > Nothing ensures this... > > > (Why not just do the same thing you do for ruby text?) > > I think I wrote this code based on some misunderstanding of the line layout > code before. I would change that. You've fixed this, right? > > Also, is there something you need to do to handle pushedFrame being > > true? That is, do you need to do something to undo what the > > nsLineLayout did when it pushed the frame? > > It seems that nsLineLayout pushes frame only with aStatus set to > NS_INLINE_BREAK_BEFORE. In that case, we will push our frames as well. Do we > need to do anything special here? > > The only thing I'm concerned here is that, I do want to ensure ruby base is > never broken inside, as it would make things a lot more complex if it can be > broken. But I have no idea about how to ensure that. Any suggestions? Not right now. Please file a followup bug blocking enable-css-ruby and we can deal with it there. > > > if (rtFrame->GetNextSibling()) { > > >+ // If there is a next sibling, the isize of this frame > > >+ // must contribute to the pair isize. > > > pairISize = std::max(pairISize, isize); > > > allFramesAreLastChild = false; > > > } > > > > I don't understand this at all. Why is this condition about > > rtFrame->GetNextSibling() here? > > As I mentioned in the last paragraph of comment 35, it is the logic for > check span. If the current frame is not the last frame of its parent, it > must not be a span, and should contribute to the pair isize. If it is the > last frame, it is possible, but not always, a span. You've gotten rid of this now, right? > > Why do you allow ruby text that doesn't fit? > > I do not allow it. But this code is not directly used for reflowing frames, > it is used to track the possible isize of the current pair. OK, could you add a comment explaining that? > > And doesn't a GetNextSibling condition mean that the behavior will > > differ based on the prior-to-Reflow line breaking of frames, which > > is something that shouldn't happen? > > With the invariant mentioned in the last paragraph of comment 35, if the > line break happens after one pair, all of the frames of the pair would be > the last frame in its parent, then allFramesAreLastChild will be true, and > finally they all contribute their isize to the pairISize. Hence the behavior > won't differ here. This is no longer an issue now, right? (And spans never break.) Also, could you deal with these comments in comment 58: (In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #58) > >+ if (NS_FRAME_IS_NOT_COMPLETE(aStatus) && > >+ NS_INLINE_IS_BREAK_AFTER(aStatus)) { > > Perhaps assert that NS_FRAME_IS_NOT_COMPLETE(aStatus) == > NS_INLINE_IS_BREAK_AFTER(aStatus)? Why would they ever be different > at this point? > > > Does it make sense to rename prevSibling to lastChild or newLastChild? > I feel like that makes a little more sense. > > >+ nsresult rv = CreateNextInFlow(aBaseContainer, newBaseContainer); > >+ if (NS_FAILED(rv)) { > >+ return; > >+ } > >+ if (newBaseContainer) { > > I don't think you need either of these tests (NS_FAILED(rv) or > newBaseContainer). > > You should file a followup bug on making CreateNextInFlow return > the frame pointer instead of returning nsresult and using an > out-parameter. > > >+ mFrames.RemoveFrame(newBaseContainer); > ... > >+ mFrames.RemoveFrame(newTextContainer); > > If these are existing next-in-flows they might not be in mFrames. > You should handle that correctly. > > But I suspect you don't need the RemoveFrame and InsertFrame calls > at all. You just need to condition the PushChildren call on prevSibling > (or newLastChild) actually having a next sibling. This also means less > moving frames around if there are existing continuations. > > >+ childReflowState.mLineLayout = aReflowState.mLineLayout; > > Could you put a FIXME here (I know it's existing code) saying that > we probably shouldn't be using the same nsLineLayout for the text > containers. > > >+ NS_ASSERTION(frameReflowStatus == NS_FRAME_COMPLETE, > >+ "Ruby text container must not break itself inside"); > > This needs a bit more of a comment. Since ruby text containers return > NS_FRAME_COMPLETE even when they have continuations, because the > breaking has already been handled when reflowing the base containers, > you should document that carefully here, in addition to your comments in > nsRubyTextContainer::Reflow. r=dbaron with that, though I haven't looked at this very closely. I suspect it could have benefited from more careful review, but the way you've been changing the patches has made that hard.

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #108) > Comment on attachment 8524552 [details] [diff] [review] > patch 5.2 - Implement basic line breaking > > In ShouldBreakBefore, I don't understand your use of > GetLastOptionalBreakPosition. Do you mean to be using > NotifyOptionalBreakPosition instead? (Presumably you need to pass > a useful offset in.) I want to check if it is safe to break before. The condition means, if there is no break position before this pair, we should always fit it in. Hence it should not be NotifyOptionalBreakPosition instead. Does it sound good? > Was there a reason you didn't do this bit of comment 56: > > >+ if (GetNextInFlow() && !isComplete) { > > >+ // What ever happens in the reflow loop, if there still exist > > >+ // next-in-flows and we haven't pull all of their children, > > >+ // the status shouldn't be set complete. > > >+ NS_FRAME_SET_INCOMPLETE(aStatus); > > >+ } > > > > I think this is a fine thing to do for safety, but I think, inside > > the if, you should assert that NS_FRAME_IS_COMPLETE(aStatus) is already > > true, before setting it. > > I notice the comment is gone now too. Yes because I now use reflowStatus to track the reflow status of each pair, and then set aStatus accordingly. Hence aStatus will no longer be changed elsewhere. In the current code, the "NS_FRAME_SET_INCOMPLETE(aStatus)" is the only place where aStatus could be set to incomplete. > > I mentioned that in comment 54. Filed bug 1089431 for this. In addition, the > > spec also allows break inside ruby base, which I found hard to implement > > now, and think it doesn't even worth to implement for now. Should I file > > another bug for that as well? Even if yes, I don't think it should block > > enable-css-ruby. > > Yes, another bug sounds great, and I'm fine with it not blocking > enable-css-ruby. Filed bug 1105051. > > > >+ if (aStatus != NS_FRAME_COMPLETE) { > > > > > > I suspect you want NS_FRAME_IS_COMPLETE(aStatus) instead, although > > > really instead you might want to assert that they're equivalent, since > > > you're not handling NS_FRAME_OVERFLOW_IS_INCOMPLETE (nor should you be). > > > > By that way, I have to check NS_INLINE_BREAK_BEFORE(aStatus) in addition, > > because complete flag is not reliable when break before happens. I think it > > might be better to assert NS_FRAME_OVERFLOW_IS_INCOMPLETE is false with this > > condition unchanged. Do you agree with that? > > I think asserting !NS_FRAME_OVERFLOW_IS_INCOMPLETE(aStatus) is good. > > But what's wrong with changing the condition to > !NS_FRAME_IS_COMPLETE(aStatus)? Because NS_FRAME_IS_COMPLETE doesn't check NS_INLINE_IS_BREAK_BEFORE, where NS_FRAME_INCOMPLETE may not be set. If change, the condition would be NS_INLINE_IS_BREAK_BEFORE(aStatus) || !NS_FRAME_IS_COMPLETE(aStatus) I feel it is unnecessarily long. > > > >+ if (NS_INLINE_IS_BREAK(reflowStatus)) { > > > >+ // It is assumed that ruby base frame is unbreakable, > > > >+ // hence it should never break after. > > > >+ MOZ_ASSERT(NS_INLINE_IS_BREAK_BEFORE(reflowStatus), > > > >+ "Currently we only process this only case"); > > > >+ break; > > > > > > What ensures this? > > > > Nothing ensures this... > > > > > (Why not just do the same thing you do for ruby text?) > > > > I think I wrote this code based on some misunderstanding of the line layout > > code before. I would change that. > > You've fixed this, right? I removed the assertion. But I wonder we may add it back when we can successfully suppress line breakings inside ruby boxes. It is also possible that I change the condition in bug 1089431. > > > Also, is there something you need to do to handle pushedFrame being > > > true? That is, do you need to do something to undo what the > > > nsLineLayout did when it pushed the frame? > > > > It seems that nsLineLayout pushes frame only with aStatus set to > > NS_INLINE_BREAK_BEFORE. In that case, we will push our frames as well. Do we > > need to do anything special here? > > > > The only thing I'm concerned here is that, I do want to ensure ruby base is > > never broken inside, as it would make things a lot more complex if it can be > > broken. But I have no idea about how to ensure that. Any suggestions? > > Not right now. Please file a followup bug blocking enable-css-ruby and > we can deal with it there. Bug 1098272. > > > > if (rtFrame->GetNextSibling()) { > > > >+ // If there is a next sibling, the isize of this frame > > > >+ // must contribute to the pair isize. > > > > pairISize = std::max(pairISize, isize); > > > > allFramesAreLastChild = false; > > > > } > > > > > > I don't understand this at all. Why is this condition about > > > rtFrame->GetNextSibling() here? > > > > As I mentioned in the last paragraph of comment 35, it is the logic for > > check span. If the current frame is not the last frame of its parent, it > > must not be a span, and should contribute to the pair isize. If it is the > > last frame, it is possible, but not always, a span. > > You've gotten rid of this now, right? Right. > > > Why do you allow ruby text that doesn't fit? > > > > I do not allow it. But this code is not directly used for reflowing frames, > > it is used to track the possible isize of the current pair. > > OK, could you add a comment explaining that? I don't see where should I add additional comment for this. There have been comments around code which breaks when a ruby text doesn't fit. > > > And doesn't a GetNextSibling condition mean that the behavior will > > > differ based on the prior-to-Reflow line breaking of frames, which > > > is something that shouldn't happen? > > > > With the invariant mentioned in the last paragraph of comment 35, if the > > line break happens after one pair, all of the frames of the pair would be > > the last frame in its parent, then allFramesAreLastChild will be true, and > > finally they all contribute their isize to the pairISize. Hence the behavior > > won't differ here. > > This is no longer an issue now, right? (And spans never break.) Right. > Also, could you deal with these comments in comment 58: > > (In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from > comment #58) > > >+ if (NS_FRAME_IS_NOT_COMPLETE(aStatus) && > > >+ NS_INLINE_IS_BREAK_AFTER(aStatus)) { > > > > Perhaps assert that NS_FRAME_IS_NOT_COMPLETE(aStatus) == > > NS_INLINE_IS_BREAK_AFTER(aStatus)? Why would they ever be different > > at this point? I have asserted NS_INLINE_IS_BREAK_AFTER(aStatus) when NS_FRAME_IS_NOT_COMPLETE(aStatus). But it is not always true vice verse, because there could be a break position after all pairs being reflowed. > > Does it make sense to rename prevSibling to lastChild or newLastChild? > > I feel like that makes a little more sense. Have done. > > >+ nsresult rv = CreateNextInFlow(aBaseContainer, newBaseContainer); > > >+ if (NS_FAILED(rv)) { > > >+ return; > > >+ } > > >+ if (newBaseContainer) { > > > > I don't think you need either of these tests (NS_FAILED(rv) or > > newBaseContainer). > > > > You should file a followup bug on making CreateNextInFlow return > > the frame pointer instead of returning nsresult and using an > > out-parameter. Filed bug 1093998. > > >+ mFrames.RemoveFrame(newBaseContainer); > > ... > > >+ mFrames.RemoveFrame(newTextContainer); > > > > If these are existing next-in-flows they might not be in mFrames. > > You should handle that correctly. If there are existing next-in-flows, newBaseContainer would be nullptr. I've descibed it in the comment of that condition. > > But I suspect you don't need the RemoveFrame and InsertFrame calls > > at all. You just need to condition the PushChildren call on prevSibling > > (or newLastChild) actually having a next sibling. This also means less > > moving frames around if there are existing continuations. I don't quite get your point here. The remove/insert here is used to move the frames so that the next-in-flows could form a new segment. If we don't do so before pushing, the whole ruby would be weird. > > >+ childReflowState.mLineLayout = aReflowState.mLineLayout; > > > > Could you put a FIXME here (I know it's existing code) saying that > > we probably shouldn't be using the same nsLineLayout for the text > > containers. I added it in a previous version, but forgot to add again in the new version... > > >+ NS_ASSERTION(frameReflowStatus == NS_FRAME_COMPLETE, > > >+ "Ruby text container must not break itself inside"); > > > > This needs a bit more of a comment. Since ruby text containers return > > NS_FRAME_COMPLETE even when they have continuations, because the > > breaking has already been handled when reflowing the base containers, > > you should document that carefully here, in addition to your comments in > > nsRubyTextContainer::Reflow. ditto... > r=dbaron with that, though I haven't looked at this very closely. I > suspect it could have benefited from more careful review, but the > way you've been changing the patches has made that hard. Sorry about that. But since there was a significant change on spec, and we no longer need to handle spans when reflowing pairs, I felt this patch could be rewritten in a clearer way. That's why I use ReflowOnePair & ReflowPairs instead of {Do,}ReflowChildren now. I do realize that it would make re-review harder, but I feel it worth to make things clear. Sorry about adding your work.

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=bc599f02d2b1 inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/0a32c681cb05 https://hg.mozilla.org/integration/mozilla-inbound/rev/211e46275378 https://hg.mozilla.org/integration/mozilla-inbound/rev/2f388e358701 https://hg.mozilla.org/integration/mozilla-inbound/rev/fab3d1117a6a https://hg.mozilla.org/integration/mozilla-inbound/rev/d52caef15e10 https://hg.mozilla.org/integration/mozilla-inbound/rev/002f76d56d0f https://hg.mozilla.org/integration/mozilla-inbound/rev/33dddb6ab137 https://hg.mozilla.org/integration/mozilla-inbound/rev/0ea4763c76f3 https://hg.mozilla.org/integration/mozilla-inbound/rev/96c4dd714d56

https://hg.mozilla.org/mozilla-central/rev/0a32c681cb05 https://hg.mozilla.org/mozilla-central/rev/211e46275378 https://hg.mozilla.org/mozilla-central/rev/2f388e358701 https://hg.mozilla.org/mozilla-central/rev/fab3d1117a6a https://hg.mozilla.org/mozilla-central/rev/d52caef15e10 https://hg.mozilla.org/mozilla-central/rev/002f76d56d0f https://hg.mozilla.org/mozilla-central/rev/33dddb6ab137 https://hg.mozilla.org/mozilla-central/rev/0ea4763c76f3 https://hg.mozilla.org/mozilla-central/rev/96c4dd714d56

https://hg.mozilla.org/integration/mozilla-inbound/rev/699556d95575cdc247a501c5a5a86107b52263ab Bug 1052924 followup - Move the XXX comment to the right field. DONTBUILD