This is really part of the work for bug 263359, but I want to fix it separately since it applies in all cases and I don't want to make the patch there more complicated than it already is.

This doesn't work for me, because on the initial load of the page, when we get to Reslove(), mContentLength of the text frames is not yet set correctly (it is simply initialized to "0"). So CreateBlockBuffer() returns a zero-length buffer, and things go downhill from there. The end result is that no bidi resolving happens, and RTL portions of an LTR paragraph appear reversed.

... this is because in nsBlockFrame::Reflow(), bidiUtils->Resolve() is called before calling ReflowDirtyLines() (which, eventually, assigns values to mContentLength of descendant text frames). I was about to suggest delaying the call to Resolve() until after ReflowDirtyLines(), but that won't work because bidi frame-ordering also happens under ReflowDirtyLines(), and depends on Resolve() to have already been executed.

It's odd that it worked for me, then. I have been juggling different patches, so I might not have been testing what I thought I was.

... on the other hand, I was testing what I thought I was at least some of the time, because there were initial errors which I fixed in a second iteration. What seems more likely is that it works in larger files where incremental reflow kicks in but not in smaller files.

This patch no longer has that problem. Whether it achieves the stated aim of simplifying the code is open to question.

(In reply to comment #6) > This patch no longer has that problem. Whether it achieves the stated aim of > simplifying the code is open to question. > Even if it doesn't achieve that aim, I expect it to greatly improve performance, as (upon non-initial reflows) the first in-flow will no longer be gobbling up the content from all its continuations, and thus line-breaking code won't have to do the line breaking all over again (which I think is the main cause of bug 330350). I'll do some more testing now.

This is a simple testcase which was useful to me when I was working on this code, and I'm now using it when testing the patches. Clicking on either of the links causes a reflow of that link. With patch v2 applied, clicking on the top link causes the LTR text to be reversed, and clicking on the bottom link causes the RTL word (at the end) to be reversed. I haven't diagnosed the cause of this, yet.

So, what happens is that after processing the first (RTL) frame, the condition for calling RemoveBidiContinuation() is satisfied, and that call "removes" (actually, turns into fluid continuations) the rest of the frames. We probably want to make sure we're calling RemoveBidiContinuation only after processing the entire content node, or something.

Hi, I have noticed multiple regression with respect to Bidi reordering. There is noticeable refactoring to the nsBidiPresUtils code I originally wrote around year 2000. But where can one find a description for significant changes if any?

You can find a list of all changes at http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/layout/base/nsBidiPresUtils.cpp&rev=HEAD (up to the 1.9.0 branch) and http://hg.mozilla.org/mozilla-central/log/b42b8de951c1/layout/base/nsBidiPresUtils.cpp (for current trunk). Can you file bugs for the regressions that you are seeing?

This version doesn't regress the testcase in attachment 217143 [details], but it does regress layout/reftests/bidi/with-first-letter-2a.html and with-first-letter-2b.html. It reduces the wall-clock time of loading a local copy of http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1234111896.1234114660.22461.gz from about 38 minutes to about 6 minutes.

Attaching this as checkpoint. It passes reftests, crashtests, and mochitests, and even removes a bunch of assertions in crashtests, but it still has problems with bidirectional text entry. Load time for a local copy of a tinderbox log with this patch is about 1.5 minutes.

(In reply to comment #13) > Attaching this as checkpoint. It passes reftests, crashtests, and mochitests, > and even removes a bunch of assertions in crashtests, but it still has problems > with bidirectional text entry. Can you add a mochitest for bidirectional text entry that would fail because of these problems?

What I really need is a combined mochitest and reftest, something like: simulate entering characters capture reference rendering simulate entering character simulate backspace capture test rendering compare renderings Is that possible?

Yes, see http://mxr.mozilla.org/mozilla-central/search?string=windowsnapshot

This is looking good to me, but I'll dogfood with it for a day or two before asking review. Comments welcome in the meanwhile. Uri, do you have any more suggestions for testing? I've also pushed it to the try server in http://hg.mozilla.org/try/rev/b36e6e63814a

This looks almost too good to be true... Doesn't removing the re-use of existing continuation cause excessive frame creation and deletion, which, in turb, causes a performance regression? Have you tried profiling this on large, massively-bidi, text?

(In reply to comment #18) > This looks almost too good to be true... I don't think I've ever been called that before ;-) > Doesn't removing the re-use of existing continuation cause excessive frame > creation and deletion, which, in turb, causes a performance regression? Have > you tried profiling this on large, massively-bidi, text? No, it doesn't seem to. I think that's because we are resynchronizing with the existing frames with each new frame that we process, rather than with each content node as we used to do. Note also that we still don't delete frames in RemoveBidiContinuation. I've been using the testcase from bug 319930 (attachment 206580 [details]) as an example of large, fairly massively-bidi, text, and it is about 4 times faster with this patch.

It's especially interesting to compare load times of files of different lengths derived from attachment 206580 [details]: Lines in file Load time without the patch Load time with the patch 32000 13 5 64000 56 9 128000 182 23

Comment on attachment 364999 [details] [diff] [review] patch v.5 (possibly not buggy) This is still looking good; time to request reviews.

Comment on attachment 364999 [details] [diff] [review] patch v.5 (possibly not buggy) rs=me

Mochitests for the editing issues I encountered pre-checked-in in http://hg.mozilla.org/mozilla-central/rev/b7b8d7526192

Requesting blocking1.9.1 for the sake of bug 477495 (so we don't need to wait for approval1.9.1).

Any progress here?

Comment on attachment 364999 [details] [diff] [review] patch v.5 (possibly not buggy) r=me. Sorry for the very long delay. The only thing I'd suggest is to inline EnsureBidiContinuation, as its very sort now, called from one place, and doesn't really match its name any more. Also, I wish this code had more comments - but that's at least partially my fault.

Thanks Uri!

Simon, we should get this on trunk ASAP. Add 'checkin-needed' if you want me to land it.

http://hg.mozilla.org/mozilla-central/rev/373f39a2d1ec Checked in reftests based on attachment 217143 [details], because many of my attempts to fix the regressions from this bug ended up breaking it.

tests: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9fe3063c25a4 patch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/e28fcb15919c