399850 - Find out if the bidi resolver doesn't need to split frames with rtl and ltr characters but the same embedding level

Status: NEW

(Core :: Layout: Text and Fonts, defect)

Description

[Simon Montagu :smontagu]

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.

Comment 1

[Simon Montagu :smontagu]

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?

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 3

[timeless]

they can try to hack around anything.

Comment 4

[(no longer active)]

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

Comment 5

[Simon Montagu :smontagu]

Attachment: Patch part 1

This is the easy part, removing CalculateCharType from Resolve().

For the change to nsTextFrameThebes, see bug 393758 comment 2 and 3.

Comment 6

[(no longer active)]

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?

Comment 7

[Simon Montagu :smontagu]

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

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Nice!

Comment 9

[Simon Montagu :smontagu]

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).

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 12

[Simon Montagu :smontagu]

Attachment: Backout the assertion change

Comment 13

[Jesse Ruderman]

Attachment: another testcase that triggers the assertion

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think it can just be landed.

Comment 16

[Simon Montagu :smontagu]

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.

Comment 17

[Simon Montagu :smontagu]

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

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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)

Comment 19

[Simon Montagu :smontagu]

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.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 21

[Simon Montagu :smontagu]

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?

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

Comment 23

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.