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

NEW
Assigned to

Status

()

11 years ago
9 years ago

People

(Reporter: smontagu, Assigned: smontagu)

Tracking

(Depends on: 2 bugs, {rtl})

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

11 years ago
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.
(Assignee)

Updated

11 years ago
Depends on: 402427
(Assignee)

Comment 1

11 years ago
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.

Comment 3

11 years ago
they can try to hack around anything.

Comment 4

11 years ago
Mass-assigning the new rtl keyword to RTL-related (see bug 349193).
Keywords: rtl

Updated

11 years ago
Component: Layout: BiDi Hebrew & Arabic → Layout: Text
QA Contact: layout.bidi → layout.fonts-and-text
(Assignee)

Comment 5

9 years ago
Created attachment 443850 [details] [diff] [review]
Patch part 1

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

For the change to nsTextFrameThebes, see bug 393758 comment 2 and 3.
Attachment #443850 - Flags: review?(ehsan)

Comment 6

9 years ago
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?
Attachment #443850 - Flags: review?(ehsan) → review+
(Assignee)

Updated

9 years ago
Attachment #443850 - Flags: review?(roc)
(Assignee)

Comment 7

9 years ago
BTW, I checked in a bunch of reftests for this some time back, layout/reftests/bidi/mixedChartype*
Flags: in-testsuite+
(Assignee)

Comment 9

9 years ago
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.
(Assignee)

Comment 12

9 years ago
Created attachment 444820 [details] [diff] [review]
Backout the assertion change

Comment 13

9 years ago
Created attachment 444829 [details]
another testcase that triggers the assertion
(Assignee)

Comment 16

9 years ago
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.
(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)
(Assignee)

Comment 19

9 years ago
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?
(Assignee)

Comment 21

9 years ago
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.
(Assignee)

Updated

9 years ago
Depends on: 570378
(Assignee)

Updated

9 years ago
Depends on: 571185
You need to log in before you can comment on or make changes to this bug.