In the old gfx implementations we couldn't render a run of text containing characters with different native directions (equivalent to the charType frame property) but the same resolved direction (the embeddingLevel property) in a single operation, so the bidi resolver splits frames so that every frame contains text with the same native direction and resolved direction. For example <span style="unicode-bidi: bidi-override; direction: ltr">world ELLOH</span> where "HELLO" is in a RTL language would be split into two frames. I'm not 100% sure, but I think that with Thebes that's no longer necessary, and we could simplify code and maybe remove the charType property altogether.

Of course, the presupposition behind this bug is "everyone uses Thebes". Is that true today? Is there a bug about disabling non-Thebes gfx implementations that this can depend on?

The only people who aren't using thebes are the Nokia MicroB team, and they already have a heavily patched build, so maybe they can patch around this too.

they can try to hack around anything.

Mass-assigning the new rtl keyword to RTL-related (see bug 349193).

This is the easy part, removing CalculateCharType from Resolve(). For the change to nsTextFrameThebes, see bug 393758 comment 2 and 3.

Comment on attachment 443850 [details] [diff] [review] Patch part 1 The patch looks sane to me, but I can't say that I'm the most familiar person with this code, so could you ask for a peer review here as well?

BTW, I checked in a bunch of reftests for this some time back, layout/reftests/bidi/mixedChartype*

Comment on attachment 443850 [details] [diff] [review] Patch part 1 Nice!

http://hg.mozilla.org/mozilla-central/rev/5ecb4057a631 Keeping this open for possibly removing CalculateCharType from nsBidiPresUtils::ProcessText (which would mean that we could remove a whole lot more).

This caused a new assertion in our reftest runs: ###!!! ASSERTION: can't continue text run across non-fluid continuations: 'aFrame1->GetContent() != aFrame2->GetContent() || aFrame1->GetNextInFlow() == aFrame2', file /builds/slave/mozilla-central-linux-debug/build/layout/generic/nsTextFrameThebes.cpp, line 1338 fires twice on: layout/generic/crashtests/472776-1.html

Simon, I guess we should turn that assertion back into a conditional check for now (partial backout, basically) while we figure out what's going on in that test.

Does that backout need review, or can it just be landed?

I think it can just be landed.

I was planning to just land it, but I wasn't online with a green tree today. I hope I'll get to it tomorrow.

Checked in attachment 444820 [details] [diff] [review] in http://hg.mozilla.org/mozilla-central/rev/80ae52a7e188

(In reply to comment #10) > This caused a new assertion in our reftest runs: > > ###!!! ASSERTION: can't continue text run across non-fluid continuations: > 'aFrame1->GetContent() != aFrame2->GetContent() || aFrame1->GetNextInFlow() == > aFrame2', file > /builds/slave/mozilla-central-linux-debug/build/layout/generic/nsTextFrameThebes.cpp, > line 1338 > > fires twice on: > > layout/generic/crashtests/472776-1.html ... and also on: layout/reftests/bidi/305643-1.html | assertion count 2 layout/reftests/bidi/492231-1.html | assertion count 1 (not sure why I didn't point those out the previous time)

So, there are two separate scenarios which cause the assertion: 1) When an element had bidirectional text, but changed leaving only single-direction text. During reflow after the change the bidi resolver leaves around the continuation frames that it created during initial reflow, but sets their length to zero. 2) In "visual order" pages we do something wildly inefficient: we do bidi resolution on the element and create continuation frames just like in logical order pages, but then set the embedding level property on each frame to the base paragraph level, so no reordering happens. This was probably necessary with the old text frame code where we had to flip visual text before passing it to the platform, but I don't think it's necessary now. No. 2 would be quite easy to fix, if we care enough about visual pages to add a bit of extra code to handle them. No. 1 I'm less sure about. Maybe we could make non-fluid continuations of zero-length frames into fluid continuations? Or we could just leave things the way they are, though we might want at least to update the comment in ContinueTextRunAcrossFrames.

Couldn't the bidi resolver delete the zero-length continuations?

Hmm, I know we have been avoiding deleting frames during bidi resolution for performance reasons (see e.g. bug 319930 comment 7 and following), but possibly bug 512336 has removed those concerns?

hmm, that's a good point. Probably we should just leave things as-is.