I think bidi reordering should happen during frame construction, not reflow. I don't see when the results would change as a result of a reflow -- I think it only changes in response to things that change the frame tree anyway. Fixing this would both reduce the cost of reflow and (I suspect) simplify the code involved in bidi reordering.

See also bug 365131, about ::first-letter.

I assume you're referring to bidi resolution, not bidi reordering. Bidi reordering is done per-line, and therefore depends on line-breaking, and must be done during reflow (at least as I understand this).

Sorry, right, bidi resolution -- the part where we split frames into fixed continuations.

Do we rebuild the frame tree these days after applying or changing style, for example when changing the direction of the root element? There used to be something called a "style reflow" but I think that term is obsolete now.

We could make changes to the 'direction' property cause a frame reconstruct instead of a reflow, if this requires doing so. I think dynamic changes to 'direction' are pretty rare, so I'm not too concerned about slowing them down.

This would be great. It would simplify some text frame invariants.

Actually, I need this for the new-textframe code to work well at all. We sometimes calculate the min-width and pref-width for text before we've reflowed the text, so currently sometimes we haven't split textframes by direction yet. In the new-textframe world that's very bad because we want to compute the min-width and pref-width from the frame's textrun, but textruns can't handle mixed-direction text, so we're stuck.

This has various problems, so r- is a foregone conclusion, but I would like to receive criticisms at this stage. It makes performance significantly worse in large plain text documents or documents with very long lines (common in view source). Changing direction in a textarea causes a crash, even if I only apply the changes to nsStyleStruct.cpp and none of the rest.

Looks like the textframe has no scrollable child. It must have not been reconstructed during the reframe. That is very strange. What's the cause of the performance issues? I assume they only occur on bidi pages? are we reresolving more often than we used to? Maybe you should separate out the IsBidiFormControl refactoring so we can land that right away.

(In reply to comment #10) > What's the cause of the performance issues? I assume they only occur on bidi > pages? are we reresolving more often than we used to? In view-source, for example, we call mSink->OpenContainer before and mSink->CloseContainer after every tag, and this triggers the call to ResolveBidi() in nsCSSFrameConstructor::ContentAppended(). We might well be able to solve the problem by moving that call somewhere else, but I have no suggestion for where. > Maybe you should separate out the IsBidiFormControl refactoring so we can land > that right away. What's so great about it that you want to do that? I'm not hugely happy with it, since we end up walking all the way up the content tree in more cases than in the old code. These days visual Bidi pages are thankfully rare and getting rarer, so that may not be the end of the world, but it's still not great.

If we're going to do IsBidiFormControl that way, we could land that now as a way to reduce the size of individual patches. That's all. What if we had a block frame state bit that meant "needs bidi resolution"? We could set that bit on the relevant block(s) when frames or content change, and check it when we reflow a block or call GetMinWidth/GetPrefWidth on it.

> and this triggers the call to ResolveBidi() in > nsCSSFrameConstructor::ContentAppended() Er... don't we buffer those up somewhat? In any case, roc's idea may be the way to go...

I don't see any free bits, but it looks like NS_BLOCK_SHRINK_WRAP is only set and never tested. Is it OK to recycle that one?

Isn't 0x80000000 free? But yes, I bet the reflow branch made NS_BLOCK_SHRINK_WRAP unused.

0x80000000 is NS_STATE_IS_DIRECTION_NORMAL. In practice it's only used in XUL box frames. I'm not sure if that would create a conflict, but it seems risky.

This is the per-frame-class flag set. You're not worried about NS_STATE_IS_HORIZONTAL sharing the value of NS_BLOCK_MARGIN_ROOT, right? Same thing with NS_STATE_IS_DIRECTION_NORMAL -- it's a box-frame-only bit.

OK, but should I file a bug to get rid of NS_BLOCK_SHRINK_WRAP anyway?

Absolutely. And yeah, if it's unused just use it here.

Why are you forcing bidi resolution to happen at construction time with the PR_TRUE parameter? It seems to me we should never do that. In line with the way our other dirty bits work, we should have a DIRTY bit instead of a DONE bit, and wherever we do anything that needs bidi resolution, we just set the dirty bit. In nsBlockFrame::Reflow we test the dirty bit and do resolution if necessary. We'll also need to do that in nsBlockFrame::GetMinWidth and GetPrefWidth.

I wanted to do it that way but thought it would contradict the bug summary. I guess I should trust my instincts more... As for DIRTY bit vs. DONE bit, I think it's more elegant this way than setting the dirty bit every time we create a new block frame. OTOH we could avoid that by making the test for bidi resolution ({bidi dirty bit} | NS_FRAME_FIRST_REFLOW).

> I think it's more elegant this way than setting the dirty bit every time we > create a new block frame. Why? Just setting the bit in nsBlockFrame::Init or nsBlockFrame::nsBlockFrame seems like a good idea to me, and that way we don't have to add NS_FRAME_FIRST_REFLOW checks elsewhere; keeps the handling of frame creation stuff in one place. That's how we handle the reflow dirty bits, fwiw: see nsFrame::nsFrame.

Why do we need kid->MarkBlockNeedsBidiResolution() in FrameNeedsReflow? How about we avoid the new virtual function MarkBlockNeedsBidiResolution() by putting it on nsBlockFrame* and adding a new IsFrameOfType bit for "is an nsBlockFrame"? (Is that OK with you David?)

(In reply to comment #25) > Why do we need kid->MarkBlockNeedsBidiResolution() in FrameNeedsReflow? To cause bidi resolution on style change (without reconstructing the frames and causing the crash mentioned in comment 8). It would be nice not to have to do this on *every* style change, of course. I suppose we could achieve that with a new nsChangeHint, what do you think?

I think we should reconstruct frames for 'direction' and 'bidi-override' style changes and fix that crash.

I think I now know why the crash is happening: nsEditor::SwitchTextDirection() changes the direction of the root element of the editor's document, which is a child of the textarea. nsCSSFrameConstructor::RecreateFramesForContent() then calls ContentRemoved(), which removes the child; and ContentInserted(), which does not recreate it because the textarea returns true for IsLeaf() (why?). We then try to reflow a textarea with no children whatever and crash.

> nsCSSFrameConstructor::RecreateFramesForContent() then > calls ContentRemoved() This is already broken. The node in question is anonymous, so trying to reframe it like that is bogus to start with. What's the stack to SwitchTextDirection? Ideally, in that case we'd just reframe the whole textarea and be done with it. As for why textarea returns true from IsLeaf(), that's because there should be no _normal_ frame construction child processing inside it.

OK. So given that stack, and the various other times this has come up, I think when we get an attempt to reframe a root native anonymous node we should instead reframe its parent. Does that seem reasonable?

Right, doing that prevents the crash. However, the style change seems to get dropped somewhere along the way in the reframing, so switching text direction has no effect.

> However, the style change seems to get dropped somewhere along the way in the > reframing, Hmm... because we create a new editor, I guess. <sigh>, Of course this means the style changes would also get lost if the page happened to do something that causes a reframe, so we need to fix that anyway. Who knows something about the editor style change stuff?

(In reply to comment #33) > Who > knows something about the editor style change stuff? CC-ing some editor experts

Actually, we need an expert on whoever hooked up the "change direction" UI. ;)

Mano did that in bug 279416, and he's already cc-ed.

A few quick comments (not full review, although perhaps all I'm missing is really sorting out the FrameNeedsReflow situation): nsBlockFrame::ResolveBidi needs to clear the frame state bit in most of the early-return cases. I don't think you need #ifdef IBMBIDI around every block here -- I've wanted to keep it around only as a marker for inadequately-reviewed code. I'm not sure about the changes to FrameNeedsReflow -- we should figure out exactly what the desired semantics are there. Though if we leave in the marking there we probably don't need it in a bunch of the other places. But it also seems like if you need it there for descendants, you probably also need it for at least the closest block ancestor (perhaps not all blocks, though). I'd call the nsCSSFrameConstructor method something different from the nsIFrame method to avoid confusion. Perhaps MarkNearestBlockNeedsBidiResolution? nsIFrame.h should probably have a comment that the function returns failure for non-blocks.

I was thinking about the textarea thing. As a simple fix, instead of reframing on direction changes, could we call MarkBlockNeedsBidiResolution() on the target of the style change and all descendants instead? In fact, the change to PresShell::FrameNeedsReflow already does this all the descendants for style-change reflows. We'd just need to add it for the targeted frame itself. If we don't want to do this for all style change reflows, we could add another style change hint to indicate that the target of the style change should dirty the bit... But if we're already marking the descendants I'm not sure why we shouldn't mark the target too.

(In reply to comment #38) > If we don't want to do this for all style change reflows, we could add another > style change hint to indicate that the target of the style change should dirty > the bit... Pretty much what I suggested in comment 26... ;-)

After more consideration about the style change case I think that if the target is a block, we need to dirty the bit for that block and all block descendants; otherwise we need to dirty the bit only for the target's nearest block ancestor. If people agree to that, we don't need the change in comment 31 here, so I'll file it as a separate bug.

The changes to nsPresShell are only needed for when nsPresContext::ClearStyleDataAndReflow() calls nsPresShell::StyleChangeReflow() (which happens e.g. when switching page direction). I wanted to ask why we even need nsPresShell::StyleChangeReflow(), but Eli beat me to it in bug 370952.

Does anybody want to comment on the latest iteration of the patch? dbaron, what do you think of roc's suggestion in comment 25? (I believe I've incorporated all the earlier review comments except for that one)

So I suppose adding an IsFrameOfType bit for is-a-block-frame is fine with me. (We currently use QueryInterface for that, but probably should move away from that.) I'm concerned about some other things here, though. Style change reflows mean "everything done by reflow needs to be redone". I'd rather not add something higher than that. (Is bidi resolution really that expensive?) It also seems like the additional change hint and the changes to FrameNeedsReflow are both solving the same problem -- are both really needed?

(In reply to comment #43) > So I suppose adding an IsFrameOfType bit for is-a-block-frame is fine with me. Done > (We currently use QueryInterface for that, but probably should move away from > that.) > > I'm concerned about some other things here, though. Style change reflows mean > "everything done by reflow needs to be redone". I'd rather not add something > higher than that. (Is bidi resolution really that expensive?) I'm not really following you here. I thought the point of this bug was to remove bidi resolution from "everything done by reflow" and make it a higher stage. In practice I'm doing it lazily during reflow as roc suggested in comment 12, but logically it's a separate phase. > It also seems > like the additional change hint and the changes to FrameNeedsReflow are both > solving the same problem -- are both really needed? See comment 41. Changing style-related preferences and changing an element's style through the DOM are two code paths without much overlap. In this version I've removed the code duplication that was in the last patch.

Comment on attachment 258700 [details] [diff] [review] Patch v.4 Note to self: needs merge: s/GetPresContext/PresContext/ in nsBlockFrame::ResolveBidi.

We need some traction here, this is one of the biggest things blocking new textframe turn-on.

The ball is in the reviewers' court at the moment, but I'll test whether nothing has rotted in the two months since the last patch.

Does bidi resolution take a significant amount of time compared to reflow excluding bidi resolution? If not, I think this patch shouldn't introduce a new style hint. The point was that I thought we could do it at frame construction time -- a premise which turned out to be false. This meant that I hoped we wouldn't have to worry about maintaining dirty bits and doing it lazily for both reflow and intrinsic width calculation. But we do have to go through that trouble. The only reason to go through the additional trouble of optimizing for it separately is if that optimization actually has a real performance impact? Is bidi resolution expensive enough that it does?

From profiling that I've done I would say that bidi resolution can take up something between 2% and 5% of reflow.

Alright Simon, can you give us a patch that does bidi resolution before we call the intrinsic width textframe code? We'll just do it twice for now.

Comment on attachment 266334 [details] [diff] [review] Do bidi resolution in GetMinWidth(), GetPrefWidth() and Reflow() Ow, we end up doing it three times inside tables? Still, it's probably better to get this in and optimize later. I think you need #ifdef IBMBIDI around those ResolveBidi calls. With that, r+sr=roc

File a followup on optimizing?

Attachment 266334 [details] [diff] checked in. Since it's just a stopgap, I'd planned to keep this open rather than closing it and filing a followup.

This is the previous patch (attachment 258700 [details] [diff] [review]) merged to tip without what was checked in already, and without the added style hints.

I think this should be marked FIXED, since it is, and the followup should be in a new bug for optimizing. Right now it looks like a textframe blocker, but it isn't.

OK, followup moved to bug 382422