Bug 50912 – [BiDi] [CSS3] MASTER: Add support for the unicode-bidi:isolate CSS property

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Jeremy Moskovich]

Tracking bug to add support for the unicode-bidi:isolate CSS property, quoting from the spec:

"""
Add a new value to the CSS property unicode-bidi, tentatively named "isolate". It would directionally isolate an inline element from its surroundings, just like display:inline-block elements are already directionally isolated from their surroundings: neither affects the bidi ordering of the other, and no part of the surrounding content gets ordered between parts of the element's content.
"""

[Jeremy Moskovich]

Not part of html5, shouldn't have filed this, sorry.

[Jeremy Moskovich]

This is actually part of the CSS3 spec:

isolate
For the purposes of the Unicode bidirectional algorithm, the contents of the element are considered to be inside a separate, independent paragraph, and for the purpose of bidi resolution in its containing bidi paragraph (if any), the element itself is treated as if it were an Object Replacement Character (U+FFFC). (If the element is broken across multiple lines, then each box of the element is treated as an Object Replacement Character.)

[Eric Seidel (no email)]

I assume no browser supports this yet?

This is going to take several bugs to implement.  I'll use this one as a master bug.

[Eric Seidel (no email)]

It would be of great help to me if someone could either point me to a test suite for this property, or produce some test cases that I could validate our eventual implementation with.

[Eric Seidel (no email)]

Looking at this further, I don't think this will be that hard to implement.

We already have support for "BidiContext" which saves the level on a stack.  These are used for explicit embedding levels.

We also have a "BidiStatus" object which is used for saving the current state of the unicode bidi algorithm between lines.  We lay out a line at a time and save off the current state of the UBA in a BidiStatus object.

Seems with these combined, we should be able to save the whole context stack off when we hit a unicode-bidi: isolate span, and resume the previous context stack when we close the span.

I need to study the details further and speak with Aharon more to make sure I understand "isolate" correctly.

[Eric Seidel (no email)]

Created attachment 87127 [details]
My first attempt at a test case.

I wonder what neutral character I should use for testing?  (Currently I'm using ')')

Also, should I be testing with more than one character in the span?

[Xiaomei Ji]

(In reply to comment #6)
> Created an attachment (id=87127) [details]
> My first attempt at a test case.
> 
> I wonder what neutral character I should use for testing?  (Currently I'm using ')')
> 
you can use U+FFFC (as what the spec says).
I am not 100% sure, but I think ')' is weak type, not neutral.
http://unicode.org/reports/tr9/#Bidirectional_Character_Types

> Also, should I be testing with more than one character in the span?

It would be better to have multiple characters in the span so that you know whether they are laid our LTR or RTL.

[Nikolas Zimmermann]

(In reply to comment #0)
> Tracking bug to add support for the unicode-bidi:isolate CSS property, quoting from the spec:
> 
> """
> Add a new value to the CSS property unicode-bidi, tentatively named "isolate". It would directionally isolate an inline element from its surroundings, just like display:inline-block elements are already directionally isolated from their surroundings: neither affects the bidi ordering of the other, and no part of the surrounding content gets ordered between parts of the element's content.
> """

From IRC discussion:
For SVG text we have a similar requirement. BiDi reordering shouldn't happen across text chunks.
Each character with an assigned absolute position starts a new text chunk.
<text x="10 20 30 40">ABCDEF</text>
A, B, C, DEF are four text chunks.
If A, B, C, D, E, F are 6 hebrew chars, reordering only happens for DEF -> FED, A, B, C stay the same.

In RenderBlockLineLayout (around line 1764) we're adding two midpoints, so that bidi reordering happens from 0..pos and from pos+1...len seperated.

Maybe this helps? I thought I'll mention it.

[Eric Seidel (no email)]

(In reply to comment #8)
> (In reply to comment #0)
> > Tracking bug to add support for the unicode-bidi:isolate CSS property, quoting from the spec:
> > 
> > """
> > Add a new value to the CSS property unicode-bidi, tentatively named "isolate". It would directionally isolate an inline element from its surroundings, just like display:inline-block elements are already directionally isolated from their surroundings: neither affects the bidi ordering of the other, and no part of the surrounding content gets ordered between parts of the element's content.
> > """
> 
> From IRC discussion:
> For SVG text we have a similar requirement. BiDi reordering shouldn't happen across text chunks.
> Each character with an assigned absolute position starts a new text chunk.
> <text x="10 20 30 40">ABCDEF</text>
> A, B, C, DEF are four text chunks.
> If A, B, C, D, E, F are 6 hebrew chars, reordering only happens for DEF -> FED, A, B, C stay the same.
> 
> In RenderBlockLineLayout (around line 1764) we're adding two midpoints, so that bidi reordering happens from 0..pos and from pos+1...len seperated.
> 
> Maybe this helps? I thought I'll mention it.

Thanks for the thoughts.  Unfortunately this is different.  Example:

ABC <bdi>abc </bdi> DEF

Should be:

FED abc CBA

Inserting midpoints would do something like this:

CBA abc FED

Note that we're not separating the bidi flow at the <bdi> marker.  The <bdi> participates like how an inline-block would, in that we order the contents separately, but that it doesn't interrupt the ordering of the outside context.

[Eric Seidel (no email)]

Note I'm going to use <bdi> to mean a <span> with style="unicode-bidi: isolate".  As you can see in bug 50913, that's what it's supposed to mean (and it's much shorter to type than "unicode-bidi: isolate". :)

[Eric Seidel (no email)]

Created attachment 87532 [details]
wip

[Levi Weintraub]

View in context: https://bugs.webkit.org/attachment.cgi?id=87532&action=review

> Source/WebCore/platform/text/BidiContext.cpp:113
> +    while (BidiContext* context = parent())

Won't this be an infinite loop when there's a parent? this->parent() will always be the same.

[Eric Seidel (no email)]

So in the patch that I posted, this example fails:

<div dir="ltr"><span dir="rtl" style="unicode-bidi: -webkit-isolate">)</span>(</div>

When I hit the </span> I have no way to tell the UBA (BidiResolver::createBidiRunsForLine) to go ahead and split off the ")".  There probably is a way to get it to notice that we've exited the top context and cause an appendRuns... But even then, we'll end up having implemented the "lay out the <bdi> and set it contents all at level 0" solution, which xji already told me can't work:

>> Is it OK to run the UBA on the <bdi>'s context and then insert the result into the original string all with level 0?
>>
>> Or would that break the outer UBA?
>>
>> I think this relates to my lack of understanding of what it means for
the <bdi> contents to be treated as a "neutral" character.  It seems
inserting the resulting sub-string with level 0 wouldn't end up 
neutral.

> You are right. resetting the sub-string with bidi level 0 after you done re-ordering it wont make it as neutral.
> When you re-ordering the parent, you will end up re-ordering them too (if you are implementing UBA on > the inner layout first, and outer layer last).

[Eric Seidel (no email)]

OK.  After talking with Levi more, we have a theory as to how we might implement this.

- Keep the current patch for isolating contexts.
- Add a check after increment() (or in BidiResolver::exitIsolate) to detect that we're leaving isolate and force an appendRun().
- Keep a stack (LIFO) of all <bdi>s encountered in the resolver.
- As the first step to resolving levels, resolve levels over the subset of runs which represent the <bdi>.
- Set the run for all those levels to be an even number greater than the agacent runs.
- Run the next nested <bdi> in the stack.
- Run the rest of the normal level resolve over the entire set of runs.

I believe this approach would also work with our current resumable algorithm too.

[Eric Seidel (no email)]

(In reply to comment #14)
> OK.  After talking with Levi more, we have a theory as to how we might implement this.
> 
> - Keep the current patch for isolating contexts.
> - Add a check after increment() (or in BidiResolver::exitIsolate) to detect that we're leaving isolate and force an appendRun().
> - Keep a stack (LIFO) of all <bdi>s encountered in the resolver.
> - As the first step to resolving levels, resolve levels over the subset of runs which represent the <bdi>.
> - Set the run for all those levels to be an even number greater than the agacent runs.
> - Run the next nested <bdi> in the stack.
> - Run the rest of the normal level resolve over the entire set of runs.
> 
> I believe this approach would also work with our current resumable algorithm too.

That won't work in at least this case:

A >>> <bdi>foo</bdi> B

The > should mirror due to the B, but they won't in the above model.

New plan:

- Detect when we enter a <bdi>, set a flag on BidiResolver and record the RenderObject for the <bdi>.
- m_direction is now always OtherNeutral until we close the <bdi>.
- Remember the BidiRun created from the neutrals and associate it with the <bdi>
- After we resolve levels, run BidiResolver::createBidiRunsForLine over the <bdi> subtree.
- Replace the BidiRun originally associated with the <bdi> with the BidiRuns from the inner BidiResolver::createBidiRunsForLine.
- Repeat as necessary for any remaining <bdi>s encountered during this pass.

I think this will be correct.  I'm not entirely sure how to make it work in the resuming case, but I'm going to give it a whirl.

[Eric Seidel (no email)]

Created attachment 88055 [details]
hackish implementation which works

[Eric Seidel (no email)]

OK.  So I have a (hackish) implementation working.

I will need to do some refactoring of both BidiResolver and InlineIterator before I can post a nice clean patch.  But at least we've proven the concept now.  unicode-bidi: isolate should be possible in WebKit's engine using something like the posted patch.

[Eric Seidel (no email)]

So now is when I need some interesting test cases.

For example:
- line breaks within <bdi> (explicit or not)
- nested <bdi> elements.
- Editing with <bdi> elements (can WebKit's UBA properly resume when a line starts inside a <bdi>

[Eric Seidel (no email)]

*** Bug 57727 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

Created attachment 90249 [details]
full git patch of working hack (for transfer to my beefy machine)

[Eric Seidel (no email)]

Created attachment 90269 [details]
updated work in progress

[Aharon (Vladimir) Lanin]

(In reply to comment #18)
> So now is when I need some interesting test cases.
> 
> For example:
> - line breaks within <bdi> (explicit or not)
> - nested <bdi> elements.
> - Editing with <bdi> elements (can WebKit's UBA properly resume when a line starts inside a <bdi>

Oops - I forgot about this. Adding some test cases.

[Aharon (Vladimir) Lanin]

Created attachment 99622 [details]
Some test cases for unicode-bidi:isolate

[Eric Seidel (no email)]

Thank you for the test cases Aharon.  I'm about to leave on vacation and will be gone for 2 weeks.

[Eric Seidel (no email)]

I've resumed work on this.

[Eric Seidel (no email)]

Created attachment 104086 [details]
Updated patch which works on TOT, still a perf regression

[Eric Seidel (no email)]

A couple python regression distractions last week.  Still working on this.

[Eric Seidel (no email)]

I've written an updated version of the patch which should perform OK (isn't N^2).  But I'm testing it against Aharon's additional tests, and failing.  Debugging.

[Eric Seidel (no email)]

(In reply to comment #28)
> I've written an updated version of the patch which should perform OK (isn't N^2).  But I'm testing it against Aharon's additional tests, and failing.  Debugging.

Nevermind.  Updating Aharon's test to use "-webkit-isolate" instead of "isolate" (the property is intentionally vender-prefixed for the time being), we pass all but the last two of Aharon's tests.  Debugging.

[Eric Seidel (no email)]

@Aharon: Are you sure this test is valid?

<div dir="rtl">
  <div class="test">
    things to do: (1) <span class="isolate">&#x05D0<br></span> (2) sleep
  </div>
  <div class="reference">
    <span dir="ltr">things to do: (1) &#x05D0</span><br><span dir="ltr"> (2) sleep</span>
  </div>
</div>

If you replace the contents of the isolate with "foo" in the test, because the surrounding context in RTL the text still ends up "wrong" when compared to the reference where it has an explicit ltr override.

I may just not be understanding how isolate is supposed to work in this case. :)

[Eric Seidel (no email)]

That's from the second-to-last test in your suite, titled "same-as-base isolate containing <br> surrounded by opposite-to-base text"

[Eric Seidel (no email)]

Created attachment 104957 [details]
Should be faster and work with nested isolates, still testing

[Aharon (Vladimir) Lanin]

(In reply to comment #30)
> @Aharon: Are you sure this test is valid?
> 
> <div dir="rtl">
>   <div class="test">
>     things to do: (1) <span class="isolate">&#x05D0<br></span> (2) sleep
>   </div>
>   <div class="reference">
>     <span dir="ltr">things to do: (1) &#x05D0</span><br><span dir="ltr"> (2) sleep</span>
>   </div>
> </div>
> 
> If you replace the contents of the isolate with "foo" in the test, because the surrounding context in RTL the text still ends up "wrong" when compared to the reference where it has an explicit ltr override.
> 
> I may just not be understanding how isolate is supposed to work in this case. :)

I believe that the test case is basically correct. The <br>, which is a bidi paragraph break, should only break the text inside the isolate. The text surrounding the isolate should still form a single bidi paragraph. Thus, the LTR text before and after the isolate should form a single LTR run, and thus the "(2)" should wind up at the left end of the second line.

I will verify this with a couple of experts on a separate thread.

However, you are right that the Hebrew letter before the <br> is a red herring. The test would be a better one without it. And, of course, if it stays, it should at least get a semicolon to end the entity properly. :-)

[Eric Seidel (no email)]

I don't think we currently have any testing for interaction between bidi-isolate and dir.  I assume that the dir still applies to the span when bidi-isolate is set?

[Eric Seidel (no email)]

It's also unclear to me how plaintext should interact with dir="" or if we get that part right.  Seems we need further testing. :)

http://dev.w3.org/csswg/css3-writing-modes/#unicode-bidi

[Eric Seidel (no email)]

Created attachment 104963 [details]
A final working patch, which is ready for landing if I can find a brave enough reviewer

[Eric Seidel (no email)]

Oh, I should also note, that I ran the PLT and saw no change (saw a 0.25% improvement which I attribute to noise) after this change.

[WebKit Review Bot]

Comment on attachment 104963 [details]
A final working patch, which is ready for landing if I can find a brave enough reviewer

Attachment 104963 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9479861

New failing tests:
css3/unicode-bidi-isolate-aharon.html
css3/unicode-bidi-isolate-basic.html

[Aharon (Vladimir) Lanin]

(In reply to comment #34)
> I don't think we currently have any testing for interaction between bidi-isolate and dir.  I assume that the dir still applies to the span when bidi-isolate is set?

Yes, it does apply. I need to give more test cases.

[Aharon (Vladimir) Lanin]

(In reply to comment #35)
> It's also unclear to me how plaintext should interact with dir="" or if we get that part right.  Seems we need further testing. :)
> 
> http://dev.w3.org/csswg/css3-writing-modes/#unicode-bidi

For <div dir="rtl" style="unicode-bidi:plaintext">, the CSS direction is set to rtl. This has the usual effect on text alignment, but does *not* have any effect on the bidi ordering of the div's contents, since that is hijacked by the plaintext.

The div's CSS direction will also be inherited by the div's descendants (unless they have a dir or direction of their own), and thus affect the bidi ordering within those that have non-inline display or non-normal unicode-bidi. This is not likely to be particularly useful, but we don't really care because plaintext is intended for use on plain text.

You are right that we need test cases.

[Nikolas Zimmermann]

Comment on attachment 104963 [details]
A final working patch, which is ready for landing if I can find a brave enough reviewer

View in context: https://bugs.webkit.org/attachment.cgi?id=104963&action=review

A first round of comments for you!

> LayoutTests/css3/unicode-bidi-isolate-aharon.html:80
> +    with <span class="isolate">&#x05D0</span>=<span class="isolate">&#x05D1</span> everywhere

Do we want to land this with this typo? A semicolon is needed here to form the entity.

> LayoutTests/css3/unicode-bidi-isolate-aharon.html:101
> +    things to do: (1) <span class="isolate">&#x05D0<br></span> (2) sleep

Same here.

> LayoutTests/css3/unicode-bidi-isolate-aharon.html:104
> +    <span dir="ltr">things to do: (1) &#x05D0</span><br><span dir="ltr"> (2) sleep</span>

And here.

> Source/WebCore/ChangeLog:25
> +        Because no one (or nearly no one) understands WebKit's bidi code, I've written an exhaustive
> +        ChangeLog entry to prime any potential reviewers:

I hope that's not right- otherwise I'd feel not qualified to review this ;-)

> Source/WebCore/ChangeLog:72
> +        The way it "ignores" characters is by treating them all as neutral when inside an isolate.
> +        Thus all the characters end up grouped in a single run, but their directionality (as a group)
> +        is correctly affected by any surrounding strong characters.

Great idea!

> Source/WebCore/platform/text/BidiResolver.h:179
> +        // or should never have called enterIsolate()

Missing period.

> Source/WebCore/platform/text/BidiResolver.h:248
> +    int m_nestedIsolateCount;

An unsigned should be sufficient here, no?

> Source/WebCore/platform/text/BidiResolver.h:265
> +    // FIXME: This generic appendRun() does not know how to handle isolated runs.

This FIXME sounds as if we'd have a problem with that. If so, it should explain why it's problematic that this doesn't handle isolated runs.
From a quick glance over this patch, I don't see the problem.

> Source/WebCore/platform/text/BidiResolver.h:289
> +    ASSERT(!inIsolate()); // Isolated spans compute thier base directionatily during their own UBA run, we don't insert fake embed characters once we enter an isolated span.

Comment should be placed in the line before the assertion.
Typo: s/thier/their/

> Source/WebCore/platform/text/BidiRunList.h:146
> +    ASSERT(newRuns.runCount());

Is this function used to replace a single run by another single run as well?
If not assert that runCount > 1, so that newRuns.firstRun() is guaranteed to be unequal to newRuns.lastRun().

> Source/WebCore/platform/text/BidiRunList.h:147
> +

Can you add ASSERT(m_firstRun) here, to make it clear this is never called if it's 0.
Also ASSERT(toReplace) seems logical to add here.

> Source/WebCore/platform/text/BidiRunList.h:155
> +        Run* previousRun = m_firstRun;
> +        while (previousRun->next() != toReplace)
> +            previousRun = previousRun->next();
> +        ASSERT(previousRun);
> +        previousRun->setNext(newRuns.firstRun());

I'd prefer:
Run* currentRun = m_firstRun->next();
while (currentRun != toReplace)
    currentRun = currentRun->next();
ASSERT(currentRun);
currentRun->setNext(newRuns.firstRun());

> Source/WebCore/platform/text/BidiRunList.h:158
> +    newRuns.lastRun()->setNext(toReplace->next());

Say newRuns contains 3 Run objects.
I guess that newRuns.firstRun()->next() already points correctly to the 2nd run, and newRuns.firstRun()->next()->next() == newRuns.lastRun().
Is this correct?

> Source/WebCore/platform/text/BidiRunList.h:160
> +    // Fix up any of other pointers which may now be stale.

Ok, these are the only points in BidiRunList, so that fixes up all of them, good.
What about m_runCount? In "normal" runs added via addRun() this is incremented.
When isolated runs are created through addPlaceholderRunForIsolatedInline() you're using appendRun() which updates m_runCount.

If you're ignoring to update m_runCount here, I think it deserves a comment.
(I expected to find m_runCount += newRuns.runCount() - 1, as 'newRuns.runCount()' runs are added, and one 'toReplace' run is removed.)

> Source/WebCore/rendering/BidiRun.h:61
> +    // FIXME: I believe m_object is always a RenderText.

Hm, I don't think so. There's at least one code path, which creates BidiRuns that aren't of RenderText type.
RenderBlock::appendRunsForObject() is called for any inline renderer, eg. also for RenderTextControl, which is a RenderBlock itself.
That may invoke createRun() and thus a create a new BidiRun with m_object pointing to a non-text renderer.

> Source/WebCore/rendering/InlineIterator.h:152
> +    RenderStyle* style = object->style();
> +    EUnicodeBidi unicodeBidi = style->unicodeBidi();

You could fold this into a single statement.

> Source/WebCore/rendering/InlineIterator.h:395
> +    return object->isRenderInline() && object->style()->unicodeBidi() == Isolate;

ASSERT(object)?

> Source/WebCore/rendering/InlineIterator.h:400
> +    while (object && object != root) {

If object can be null, add an early return for it.

> Source/WebCore/rendering/InlineIterator.h:428
> +    void enterIsolate() { ASSERT(m_nestedIsolateCount >= 0); m_nestedIsolateCount++; }

Only single statements should be styled this way, just use the same style as in exitIsolate.

> Source/WebCore/rendering/InlineIterator.h:432
> +        m_nestedIsolateCount--;

Use predecrement operator.

> Source/WebCore/rendering/InlineIterator.h:445
> +    void addFakeRunIfNecessary(RenderObject* obj, InlineBidiResolver& resolver)
> +    {
> +        // We only need to lookup the root isolated span and add a fake run
> +        // for it once, but we'll be called for every span inside the isolated span
> +        // so we just ignore the call.

Why don't you use
 if (isolateTracker.needsFakeRun())
    isolateTracker.addFakeRun(obj, *this);

and needsFakeRun returns !m_hasFakeRun && inIsolate()
addFakeRun() can ASSERT(!m_hasFakeRun) and set it to true afterwards.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:793
> +static inline BidiStatus statusWithDirection(TextDirection textDirection)
> +{
> +    WTF::Unicode::Direction direction = textDirection == LTR ? LeftToRight : RightToLeft;
> +    RefPtr<BidiContext> context = BidiContext::create(textDirection == LTR ? 0 : 1, direction, false, FromStyleOrDOM);
> +
> +    // This copies BidiStatus and may churn the ref on BidiContext. I doubt it matters.
> +    return BidiStatus(direction, direction, direction, context.release());

When in doubt pass BidiStatus per reference.
Also while this is more readable, you're unnecessary ref/derefing BidiContext as well.
I'd write it this way and rename it to be more explicit:
static inline void constructStatusWithDirection(TextDirection textDirection, BidiStatus& status)
{
    status = BidiStatus(direction, direction, direction, BidiContext::create(..))
}

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:804
> +    while (!topResolver.isolatedRuns().isEmpty()) {

How about stashing topResolved.isolatedRuns() in a local reference, to avoid repeatedly calling this method.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:827
> +        // If we encountered any nested isolate runs, just move them
> +        // to the top resolver's list for later processing.

Clever!

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:-1008
> -

Unnecessary.

[Eric Seidel (no email)]

Comment on attachment 104963 [details]
A final working patch, which is ready for landing if I can find a brave enough reviewer

View in context: https://bugs.webkit.org/attachment.cgi?id=104963&action=review

Thank you for the review.  I'll post an updated patch momentarily.

>> LayoutTests/css3/unicode-bidi-isolate-aharon.html:80
>> +    with <span class="isolate">&#x05D0</span>=<span class="isolate">&#x05D1</span> everywhere
> 
> Do we want to land this with this typo? A semicolon is needed here to form the entity.

I believe they're valid in HTML5, but I've fixed them all now. :)

>> Source/WebCore/platform/text/BidiResolver.h:179
>> +        // or should never have called enterIsolate()
> 
> Missing period.

Fixed.

>> Source/WebCore/platform/text/BidiResolver.h:248
>> +    int m_nestedIsolateCount;
> 
> An unsigned should be sufficient here, no?

Sure.  Changed.

>> Source/WebCore/platform/text/BidiResolver.h:265
>> +    // FIXME: This generic appendRun() does not know how to handle isolated runs.
> 
> This FIXME sounds as if we'd have a problem with that. If so, it should explain why it's problematic that this doesn't handle isolated runs.
> From a quick glance over this patch, I don't see the problem.

Removed the comment.  There is no way to specify an isolated run outside of bidi-isolate on a span in the DOM.  The normal UBA has no concept of an isolated inline (or an inline for that matter).

>> Source/WebCore/platform/text/BidiResolver.h:289
>> +    ASSERT(!inIsolate()); // Isolated spans compute thier base directionatily during their own UBA run, we don't insert fake embed characters once we enter an isolated span.
> 
> Comment should be placed in the line before the assertion.
> Typo: s/thier/their/

fixed.

>> Source/WebCore/platform/text/BidiRunList.h:146
>> +    ASSERT(newRuns.runCount());
> 
> Is this function used to replace a single run by another single run as well?
> If not assert that runCount > 1, so that newRuns.firstRun() is guaranteed to be unequal to newRuns.lastRun().

It's very possible to have the replacement runs be a single run, yes.  the "basic" test case attached is full of such runs.

>> Source/WebCore/platform/text/BidiRunList.h:147
>> +
> 
> Can you add ASSERT(m_firstRun) here, to make it clear this is never called if it's 0.
> Also ASSERT(toReplace) seems logical to add here.

done.

>> Source/WebCore/platform/text/BidiRunList.h:155
>> +        previousRun->setNext(newRuns.firstRun());
> 
> I'd prefer:
> Run* currentRun = m_firstRun->next();
> while (currentRun != toReplace)
>     currentRun = currentRun->next();
> ASSERT(currentRun);
> currentRun->setNext(newRuns.firstRun());

That doesn't work. :)  We're not trying to find the toReplace run, we're trying to find the run previous to toReplace. :)

I've left the code as-is.

>> Source/WebCore/platform/text/BidiRunList.h:158

> 
> Say newRuns contains 3 Run objects.
> I guess that newRuns.firstRun()->next() already points correctly to the 2nd run, and newRuns.firstRun()->next()->next() == newRuns.lastRun().
> Is this correct?

Correct.  The newRuns are already an ordered list of runs.  We're just inserting them as-is in place of some placeholder run.

>> Source/WebCore/platform/text/BidiRunList.h:160
>> +    // Fix up any of other pointers which may now be stale.
> 
> Ok, these are the only points in BidiRunList, so that fixes up all of them, good.
> What about m_runCount? In "normal" runs added via addRun() this is incremented.
> When isolated runs are created through addPlaceholderRunForIsolatedInline() you're using appendRun() which updates m_runCount.
> 
> If you're ignoring to update m_runCount here, I think it deserves a comment.
> (I expected to find m_runCount += newRuns.runCount() - 1, as 'newRuns.runCount()' runs are added, and one 'toReplace' run is removed.)

Yes, good catch!

>> Source/WebCore/rendering/BidiRun.h:61
>> +    // FIXME: I believe m_object is always a RenderText.
> 
> Hm, I don't think so. There's at least one code path, which creates BidiRuns that aren't of RenderText type.
> RenderBlock::appendRunsForObject() is called for any inline renderer, eg. also for RenderTextControl, which is a RenderBlock itself.
> That may invoke createRun() and thus a create a new BidiRun with m_object pointing to a non-text renderer.

Removed the comment for now.

>> Source/WebCore/rendering/InlineIterator.h:152
>> +    EUnicodeBidi unicodeBidi = style->unicodeBidi();
> 
> You could fold this into a single statement.

Just was making it match the other one, but sure.

>> Source/WebCore/rendering/InlineIterator.h:395
>> +    return object->isRenderInline() && object->style()->unicodeBidi() == Isolate;
> 
> ASSERT(object)?

Sure.  Added.

>> Source/WebCore/rendering/InlineIterator.h:400
>> +    while (object && object != root) {
> 
> If object can be null, add an early return for it.

added ASSERT(object).

>> Source/WebCore/rendering/InlineIterator.h:428
>> +    void enterIsolate() { ASSERT(m_nestedIsolateCount >= 0); m_nestedIsolateCount++; }
> 
> Only single statements should be styled this way, just use the same style as in exitIsolate.

Removed the ASSERT now that it's unsigned.

>> Source/WebCore/rendering/InlineIterator.h:432

> 
> Use predecrement operator.

Eh.  Please fix the style guide and check-webkit-style and then I'll happily follow such. :)

>> Source/WebCore/rendering/InlineIterator.h:445
>> +        // so we just ignore the call.
> 
> Why don't you use
>  if (isolateTracker.needsFakeRun())
>     isolateTracker.addFakeRun(obj, *this);
> 
> and needsFakeRun returns !m_hasFakeRun && inIsolate()
> addFakeRun() can ASSERT(!m_hasFakeRun) and set it to true afterwards.

We're trying to avoid adding any non-fake runs too. :)   I don't think the proposed would do that.  You'd end up adding real runs once you'd added the one fake run (which would be wrong), or you'd need a separate check to make sure that we didn't add real runs when we were inside an isolate, or?

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:793
>> +    return BidiStatus(direction, direction, direction, context.release());
> 
> When in doubt pass BidiStatus per reference.
> Also while this is more readable, you're unnecessary ref/derefing BidiContext as well.
> I'd write it this way and rename it to be more explicit:
> static inline void constructStatusWithDirection(TextDirection textDirection, BidiStatus& status)
> {
>     status = BidiStatus(direction, direction, direction, BidiContext::create(..))
> }

I don't think it's worth it.  It didn't show up in profiles or the PLT.

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:804
>> +    while (!topResolver.isolatedRuns().isEmpty()) {
> 
> How about stashing topResolved.isolatedRuns() in a local reference, to avoid repeatedly calling this method.

I tried that just now, it seemed slightly confusing because there an isolatedRuns list on the isolatedResolver as well.  I've left it as-is for now.  I have no performance concerns about the current code, but I'm open to suggestions as to how to make it clearer if you feel strongly about the current verbosity.

>> Source/WebCore/rendering/RenderBlockLineLayout.cpp:-1008
>> -
> 
> Unnecessary.

Restored.

[Eric Seidel (no email)]

Created attachment 105068 [details]
Updated per Nico's review

[Eric Seidel (no email)]

The aharon test results look wrong.  Investigating why now.

[Eric Seidel (no email)]

Created attachment 105075 [details]
Fixed Aharon's test case to not use position: absolute

[Eric Seidel (no email)]

unicode-bidi-isolate-aharon.html was using position: absolute (in order to get a layer and make z-index work), unfortunately that also meant that all test content was forced to be left-aligned (since top: and left: default to 0 in CSS).  I changed the position: absolute to position: relative and made the height: 0px.  We still see the content because of overflow, but it doesn't take up any space.

[WebKit Review Bot]

Comment on attachment 105075 [details]
Fixed Aharon's test case to not use position: absolute

Attachment 105075 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9512021

New failing tests:
css3/unicode-bidi-isolate-aharon.html
css3/unicode-bidi-isolate-basic.html

[Levi Weintraub]

View in context: https://bugs.webkit.org/attachment.cgi?id=105075&action=review

I don't see any problems here, though you make me want to refactor things so plaintext can be less of an outlier.

> LayoutTests/css3/unicode-bidi-isolate-aharon.html:105
> +  <div class="test">
> +    things to do: (1) <span class="isolate">&#x05D0;<br></span> (2) sleep
> +  </div>
> +  <div class="reference">
> +    <span dir="ltr">things to do: (1) &#x05D0;</span><br><span dir="ltr"> (2) sleep</span>

Why does this appear to not work properly in the image results?

> Source/WebCore/ChangeLog:25
> +        Because no one (or nearly no one) understands WebKit's bidi code, I've written an exhaustive
> +        ChangeLog entry to prime any potential reviewers:

Don't try a career in politics ;)

> Source/WebCore/rendering/InlineIterator.h:142
> +    // FIXME: Should unicode-bidi: plaintext really be embedding override/embed characters here?
> +    observer->embed(embedCharFromDirection(style->direction(), unicodeBidi), FromStyleOrDOM);

Within the current design, this isn't possible. Essentially, when in PlainText mode, we "forget" our context whenever we hit a paragraph separator, which may just be a hard line break in pre-formatted text. This causes the specifics of plaintext to have to leak out into the resolver (or observer :)

> Source/WebCore/rendering/InlineIterator.h:243
> +// This makes callers cleaner as they don't have to specify a type for the observer when not providing one.

Good call! Way less confusing.

[Eric Seidel (no email)]

(In reply to comment #48)
> View in context: https://bugs.webkit.org/attachment.cgi?id=105075&action=review
> 
> I don't see any problems here, though you make me want to refactor things so plaintext can be less of an outlier.

Thanks for looking.

> > LayoutTests/css3/unicode-bidi-isolate-aharon.html:105
> > +  <div class="test">
> > +    things to do: (1) <span class="isolate">&#x05D0;<br></span> (2) sleep
> > +  </div>
> > +  <div class="reference">
> > +    <span dir="ltr">things to do: (1) &#x05D0;</span><br><span dir="ltr"> (2) sleep</span>
> 
> Why does this appear to not work properly in the image results?

We're still figuring out correct behavior.

> > Source/WebCore/ChangeLog:25
> > +        Because no one (or nearly no one) understands WebKit's bidi code, I've written an exhaustive
> > +        ChangeLog entry to prime any potential reviewers:
> 
> Don't try a career in politics ;)

I now see the error in my ways. :)  I will correct.  :)

> > Source/WebCore/rendering/InlineIterator.h:142
> > +    // FIXME: Should unicode-bidi: plaintext really be embedding override/embed characters here?
> > +    observer->embed(embedCharFromDirection(style->direction(), unicodeBidi), FromStyleOrDOM);
> 
> Within the current design, this isn't possible. Essentially, when in PlainText mode, we "forget" our context whenever we hit a paragraph separator, which may just be a hard line break in pre-formatted text. This causes the specifics of plaintext to have to leak out into the resolver (or observer :)
> 
> > Source/WebCore/rendering/InlineIterator.h:243
> > +// This makes callers cleaner as they don't have to specify a type for the observer when not providing one.
> 
> Good call! Way less confusing.

[Eric Seidel (no email)]

Created attachment 105380 [details]
Update per Levi's comments

[WebKit Review Bot]

Comment on attachment 105380 [details]
Update per Levi's comments

Attachment 105380 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9539194

New failing tests:
css3/unicode-bidi-isolate-aharon.html
css3/unicode-bidi-isolate-basic.html

[Levi Weintraub]

(In reply to comment #50)
> Created an attachment (id=105380) [details]
> Update per Levi's comments

IANAReviewer, but LGTM :)

[Darin Adler]

Comment on attachment 105380 [details]
Update per Levi's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=105380&action=review

I wasn’t able to review the entire patch, but I do have a few comments.

> Source/WebCore/platform/text/BidiResolver.h:147
> +    void setNext(BidiCharacterRun* next) { m_next = next; }

Having both get and set for this is unfortunate; a step backwards in encapsulation.

> Source/WebCore/platform/text/BidiResolver.h:182
> +#ifndef NDEBUG
> +    ~BidiResolver()
> +    {
> +        // The owner of this resolver should have handled the isolated runs
> +        // or should never have called enterIsolate().
> +        ASSERT(m_isolatedRuns.isEmpty());
> +    }
> +#endif

Since this is debug-only, there’s no reason that it should be inline in the class definition. It would be easier to read the class if it was down below instead.

> Source/WebCore/platform/text/BidiResolver.h:222
> +    Vector<Run*>& isolatedRuns() { return m_isolatedRuns; }

A function that returns a reference to a data member is pretty much the same thing as having a public data member, which is not great for encapsulation.

> Source/WebCore/platform/text/BidiResolver.h:288
> +    // Isolated spans compute base directionatily during their own UBA run.

Typo here in the word directionality.

[Eric Seidel (no email)]

Comment on attachment 105380 [details]
Update per Levi's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=105380&action=review

Thanks for the review!

>> Source/WebCore/platform/text/BidiResolver.h:147
>> +    void setNext(BidiCharacterRun* next) { m_next = next; }
> 
> Having both get and set for this is unfortunate; a step backwards in encapsulation.

The set it needed by BidiRunList in order to "take" the runs from another list (since it has to wire them up into its list).  However I'm happy to add BidiRunList as a friend class to BidiCaracterRun if you'd like?

>> Source/WebCore/platform/text/BidiResolver.h:182
>> +#endif
> 
> Since this is debug-only, there’s no reason that it should be inline in the class definition. It would be easier to read the class if it was down below instead.

Fixed.

>> Source/WebCore/platform/text/BidiResolver.h:222
>> +    Vector<Run*>& isolatedRuns() { return m_isolatedRuns; }
> 
> A function that returns a reference to a data member is pretty much the same thing as having a public data member, which is not great for encapsulation.

That is the current design.  It could just be a pointer.  The lifetime is tied to the Resolver, but the resolver never does anything with it, besides expose it to other code which may want to add/remove isolated runs.  It's an artifact of the template-speciailization design of BidiResolver, where all the interesting bits are kept in RenderBlockLineLayout and InlineIterator because that's the only place which has access to the RenderObject-specialization of BidiResolver.  In this case, those places need some way to hang isolated runs off the resolver (since they're tied to the lifetime of hte resolver).  But the generic resolver code knows nothing about these isolate runs, nor does it ever do anything with it.

I'm open to other designs. :)

>> Source/WebCore/platform/text/BidiResolver.h:288
>> +    // Isolated spans compute base directionatily during their own UBA run.
> 
> Typo here in the word directionality.

Fixed.

[Eric Seidel (no email)]

Comment on attachment 105380 [details]
Update per Levi's comments

View in context: https://bugs.webkit.org/attachment.cgi?id=105380&action=review

>>> Source/WebCore/platform/text/BidiResolver.h:147
>>> +    void setNext(BidiCharacterRun* next) { m_next = next; }
>> 
>> Having both get and set for this is unfortunate; a step backwards in encapsulation.
> 
> The set it needed by BidiRunList in order to "take" the runs from another list (since it has to wire them up into its list).  However I'm happy to add BidiRunList as a friend class to BidiCaracterRun if you'd like?

Actually.  Everything in this struct is already public..  I guess I justed added an explicit accessor on the dream of this struct one day becoming a class. :)

[Eric Seidel (no email)]

Created attachment 106002 [details]
Updated after Darin's review

[WebKit Review Bot]

Comment on attachment 106002 [details]
Updated after Darin's review

Attachment 106002 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9583312

New failing tests:
css3/unicode-bidi-isolate-aharon.html
css3/unicode-bidi-isolate-basic.html

[Adam Barth]

Comment on attachment 106002 [details]
Updated after Darin's review

View in context: https://bugs.webkit.org/attachment.cgi?id=106002&action=review

> LayoutTests/css3/unicode-bidi-isolate-basic.html:19
> +<div>This table shows unicode-bidi: isolate behavior (in red) with display: inline-block behavior overlapping in green.  You should see no red in this test!  Behavior between these two methods should be identical for non-wrapping strings, assuming unicode-bidi: isolate is implemented and functioning correctly.</div>

Can you remove the scrollbar from your test?  That makes life painful because every platform under the sun has different scrollbars.

> Source/WebCore/platform/text/BidiRunList.h:156
> +        // Find the run just before "toReplace" in the list of runs.
> +        Run* previousRun = m_firstRun;
> +        while (previousRun->next() != toReplace)
> +            previousRun = previousRun->next();

There's no prev(), I take it?  This seems like it could be pathologically slow if we nest isolates.

> Source/WebCore/rendering/InlineIterator.h:70
> +    unsigned offset() const { return m_pos; }

It seems unfortunate that these names don't match.

> Source/WebCore/rendering/InlineIterator.h:421
> +    IsolateTracker(bool inIsolate)

pls add the "explicit" keyword.

> Source/WebCore/rendering/InlineIterator.h:423
> +        , m_addedFakeRunForRootIsolate(false)

I would have called this m_haveAddedFakeRunForRootIsolate.  The current name makes it sound like a point to the run itself.

> Source/WebCore/rendering/InlineIterator.h:456
> +        // FIXME: Because enterIsolate is not passed a RenderObject, we have to crawl up the
> +        // tree to see which parent inline is the isolate. We could change enterIsolate
> +        // to take a RenderObject and do this logic there, but that would be a layering
> +        // violation for BidiResolver (which knows nothing about RenderObject).
> +        addPlaceholderRunForIsolatedInline(resolver, isolatedInline);

This is really too bad.  This causes a bunch of contortions to keep track of whether we've added the fake run yet.  Creating the fake run eagerly would be nicer.

[Ryosuke Niwa]

Comment on attachment 106002 [details]
Updated after Darin's review

View in context: https://bugs.webkit.org/attachment.cgi?id=106002&action=review

> LayoutTests/css3/unicode-bidi-isolate-aharon.html:91
> +    about <span class="isolate">that &#x05D0;</span> - &#x05D1;

Should we have a test case where the isolated content starts with rtl text?  Also I feel like we should have test cases where the containing block is rtl or plaintext.

Also, we probably want a test case where text inside isolate flips the order.

> LayoutTests/css3/unicode-bidi-isolate-basic.html:31
> +var neutrals = ['"', ")", "("];
> +var strongRTLs = ['ש', '× ', '×', '×', 'ק', '×', '×¢'];
> +var strongLTRs = ['a', 'b', 'c', 'd', 'e', 'f', 'g'];

Should we also include numbers and spaces?

[Ryosuke Niwa]

Comment on attachment 106002 [details]
Updated after Darin's review

View in context: https://bugs.webkit.org/attachment.cgi?id=106002&action=review

> Source/WebCore/ChangeLog:86
> +        old an very poorly understood.  We're slowly moving forward, this is another tiny step.

s/an/and/

> Source/WebCore/ChangeLog:92
> +        This is my fourth iteration of this patch (I'm happy to do more!), but I believe
> +        it's a good compromise between fixing all of the design gotcha's of our bidi
> +        system and doing the minimum amount to add this killer CSS feature.
> +
> +        I ran the PLT.  (It averaged 0.2% faster with this change, but I attribute that to noise).

I don't think this portion is helpful to be in changelog.  Probably better left in the bug instead.

> Source/WebCore/platform/text/BidiResolver.h:147
> +    void setNext(BidiCharacterRun* next) { m_next = next; }

Now that we have setNext, can we make m_next private?  It doesn't seem useful to add this method if we're keeping m_next public.
It's probably better not to add this function if we need to keep m_next public.

> Source/WebCore/platform/text/BidiResolver.h:204
> +    bool inIsolate() { return m_nestedIsolateCount; }

This function should be const.

> Source/WebCore/platform/text/BidiResolver.h:217
> +    Vector<Run*>& isolatedRuns() { return m_isolatedRuns; }

Can we return const Vector instead?

> Source/WebCore/platform/text/BidiResolver.h:228
> +    Iterator m_sor; // Points to the first character in the current run.
> +    Iterator m_eor; // Points to the last character in the current run.

My goodness these are terrible names :(  We should give them more descriptive names as a follow up.

> Source/WebCore/platform/text/BidiResolver.h:263
> +    ASSERT(m_isolatedRuns.isEmpty());

Should we also assert that m_nestedIsolateCount is zero?

> Source/WebCore/platform/text/BidiRunList.h:62
> +    void replaceRunWithRuns(Run* toReplace, BidiRunList<Run>& newRuns);

I kind of think that the meaning of arguments are clear from the function name but I'm fine with leaving argument names here.

> Source/WebCore/platform/text/BidiRunList.h:157
> +        ASSERT(previousRun);

We should also assert that previousRun->next() == toReplace as a sanity-check.

> Source/WebCore/platform/text/BidiRunList.h:171
> +    newRuns.clear(); // clear() does not delete the runs, just makes sure no other code grabs a pointer it does not own.

That is very confusing :(  How about clearPointers or clearWithoutDestroyingRuns?

> Source/WebCore/rendering/InlineIterator.h:470
> +        // FIXME: Could this initialize from this->inIsolate() instead of walking up the render tree?

It seems like we should be able to.

> Source/WebCore/rendering/InlineIterator.h:478
> +            if (isolateTracker.inIsolate())
> +                isolateTracker.addFakeRunIfNecessary(obj, *this);
> +            else
> +                RenderBlock::appendRunsForObject(m_runs, start, obj->length(), obj, *this);

I don't like the fact we have to duplicate this code twice.  Can we wrap the call to appendRunsForObject in IsolatedTracker? so that we have just one function call here.  IsolatedTracker can then check whether we're in an isolated contents or not and call appropriate function.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:809
> +        // Only inlines make sense with unicode-bidi: isolate (blocks are already isolated).
> +        RenderInline* isolatedSpan = toRenderInline(isolatedRun->object());

We should assert that they're indeed RenderInlines.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:819
> +        // FIXME: isolatedEnd should probably equal end or the last char in isolatedSpan.
> +        InlineIterator isolatedEnd = endOfLine;

I think this is okay because of the following line in appendRun:
while (obj && obj != m_eor.m_obj && obj != endOfLine.m_obj) {
eor should always be in the isolatedSpan in a sane world.

> Source/WebCore/rendering/RenderBlockLineLayout.cpp:820
> +        // FIXME: What should end and previousLineBrokeCleanly be?

previouslyLineBrokeCleanly should probably be false here.  In createBidiRunsForLine:

        if (pastEnd && (hardLineBreak || m_current.atEnd())) {
            BidiContext* c = context();
            if (hardLineBreak) {
                // A deviation from the Unicode Bidi Algorithm in order to match
                // WinIE and user expectations: hard line breaks reset bidi state
                // coming from unicode bidi control characters, but not those from
                // DOM nodes with specified directionality
                stateAtEnd.setContext(c->copyStackRemovingUnicodeEmbeddingContexts());

                dirCurrent = stateAtEnd.context()->dir();
                stateAtEnd.setEorDir(dirCurrent);
                stateAtEnd.setLastDir(dirCurrent);
                stateAtEnd.setLastStrongDir(dirCurrent);
            } else {

This is the only place where the value of hardLineBreak is used, and I don't think we need to patch winIE inside isolated contents.

[Ryosuke Niwa]

Comment on attachment 106002 [details]
Updated after Darin's review

View in context: https://bugs.webkit.org/attachment.cgi?id=106002&action=review

> Source/WebCore/rendering/InlineIterator.h:200
>              }

This if statement right here can be moved into the first if statement (one with if (!oldEndOfInline && !isIteratorTarget(current))).  Of course in a separate patch.

[Ryosuke Niwa]

The rest of the patch looks good to me (to the extent I know the code).

[Ryosuke Niwa]

Comment on attachment 106002 [details]
Updated after Darin's review

r- because you probably want to address some of my comments.

[Eric Seidel (no email)]

(In reply to comment #58)
> (From update of attachment 106002 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106002&action=review
> 
> > LayoutTests/css3/unicode-bidi-isolate-basic.html:19
> > +<div>This table shows unicode-bidi: isolate behavior (in red) with display: inline-block behavior overlapping in green.  You should see no red in this test!  Behavior between these two methods should be identical for non-wrapping strings, assuming unicode-bidi: isolate is implemented and functioning correctly.</div>
> 
> Can you remove the scrollbar from your test?  That makes life painful because every platform under the sun has different scrollbars.

Fixed.

> > Source/WebCore/platform/text/BidiRunList.h:156
> > +        // Find the run just before "toReplace" in the list of runs.
> > +        Run* previousRun = m_firstRun;
> > +        while (previousRun->next() != toReplace)
> > +            previousRun = previousRun->next();
> 
> There's no prev(), I take it?  This seems like it could be pathologically slow if we nest isolates.

Correct, it is a singly linked list.

> > Source/WebCore/rendering/InlineIterator.h:70
> > +    unsigned offset() const { return m_pos; }
> 
> It seems unfortunate that these names don't match.

offset is how all the locals call it.  m_pos is still a public member though. :(

> > Source/WebCore/rendering/InlineIterator.h:421
> > +    IsolateTracker(bool inIsolate)
> 
> pls add the "explicit" keyword.

Done.

> > Source/WebCore/rendering/InlineIterator.h:423
> > +        , m_addedFakeRunForRootIsolate(false)
> 
> I would have called this m_haveAddedFakeRunForRootIsolate.  The current name makes it sound like a point to the run itself.

Done.

> > Source/WebCore/rendering/InlineIterator.h:456
> > +        // FIXME: Because enterIsolate is not passed a RenderObject, we have to crawl up the
> > +        // tree to see which parent inline is the isolate. We could change enterIsolate
> > +        // to take a RenderObject and do this logic there, but that would be a layering
> > +        // violation for BidiResolver (which knows nothing about RenderObject).
> > +        addPlaceholderRunForIsolatedInline(resolver, isolatedInline);
> 
> This is really too bad.  This causes a bunch of contortions to keep track of whether we've added the fake run yet.  Creating the fake run eagerly would be nicer.

It's caused by InlineBidiResolver being just a template specialization of BidiResolver instead of a subclass.  We can't define any methods on InlineBidiResolver which actually deal with RenderObjects until we fix that layering oddity.

[Eric Seidel (no email)]

(In reply to comment #59)
> (From update of attachment 106002 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106002&action=review
> 
> > LayoutTests/css3/unicode-bidi-isolate-aharon.html:91
> > +    about <span class="isolate">that &#x05D0;</span> - &#x05D1;
> 
> Should we have a test case where the isolated content starts with rtl text?  Also I feel like we should have test cases where the containing block is rtl or plaintext.

unicode-bidi-isolate-basic.html has a bunch of tests which start with rtl text.  The purpose of this patch is to give us basic, non-crashing unicode-bidi: isolate support.  I expect we will need follow-up patches once people who actually understand RTL languages are given a chance to play with it and write tests for unicode-bidi: isolate.  As far as I know, this is the world's first implementation of this part of the spec. :(

> Also, we probably want a test case where text inside isolate flips the order.
> 
> > LayoutTests/css3/unicode-bidi-isolate-basic.html:31
> > +var neutrals = ['"', ")", "("];
> > +var strongRTLs = ['ש', '× ', '×', '×', 'ק', '×', '×¢'];
> > +var strongLTRs = ['a', 'b', 'c', 'd', 'e', 'f', 'g'];
> 
> Should we also include numbers and spaces?

I believe this test is sufficient for this patch, we will certainly need more tests once real RTL-experts can play with this.

[Eric Seidel (no email)]

(In reply to comment #60)
> (From update of attachment 106002 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=106002&action=review
> 
> > Source/WebCore/ChangeLog:86
> > +        old an very poorly understood.  We're slowly moving forward, this is another tiny step.
> 
> s/an/and/

Fixed.

> > Source/WebCore/ChangeLog:92
> > +        This is my fourth iteration of this patch (I'm happy to do more!), but I believe
> > +        it's a good compromise between fixing all of the design gotcha's of our bidi
> > +        system and doing the minimum amount to add this killer CSS feature.
> > +
> > +        I ran the PLT.  (It averaged 0.2% faster with this change, but I attribute that to noise).
> 
> I don't think this portion is helpful to be in changelog.  Probably better left in the bug instead.

I'm leaving it in to preempt worries from other reviewers. :)

> > Source/WebCore/platform/text/BidiResolver.h:147
> > +    void setNext(BidiCharacterRun* next) { m_next = next; }
> 
> Now that we have setNext, can we make m_next private?  It doesn't seem useful to add this method if we're keeping m_next public.
> It's probably better not to add this function if we need to keep m_next public.

This is cleanup outside the scope of this change.

> > Source/WebCore/platform/text/BidiResolver.h:204
> > +    bool inIsolate() { return m_nestedIsolateCount; }
> 
> This function should be const.

Fixed.

> > Source/WebCore/platform/text/BidiResolver.h:217
> > +    Vector<Run*>& isolatedRuns() { return m_isolatedRuns; }
> 
> Can we return const Vector instead?

No.  We need to mutate it.

> > Source/WebCore/platform/text/BidiResolver.h:228
> > +    Iterator m_sor; // Points to the first character in the current run.
> > +    Iterator m_eor; // Points to the last character in the current run.
> 
> My goodness these are terrible names :(  We should give them more descriptive names as a follow up.

Agreed.

> > Source/WebCore/platform/text/BidiResolver.h:263
> > +    ASSERT(m_isolatedRuns.isEmpty());
> 
> Should we also assert that m_nestedIsolateCount is zero?

I dont' think that's a valid assertion, but I'll try.

> > Source/WebCore/platform/text/BidiRunList.h:62
> > +    void replaceRunWithRuns(Run* toReplace, BidiRunList<Run>& newRuns);
> 
> I kind of think that the meaning of arguments are clear from the function name but I'm fine with leaving argument names here.
> 
> > Source/WebCore/platform/text/BidiRunList.h:157
> > +        ASSERT(previousRun);
> 
> We should also assert that previousRun->next() == toReplace as a sanity-check.

That's the termination condition. :)  Not necessary.

> > Source/WebCore/platform/text/BidiRunList.h:171
> > +    newRuns.clear(); // clear() does not delete the runs, just makes sure no other code grabs a pointer it does not own.
> 
> That is very confusing :(  How about clearPointers or clearWithoutDestroyingRuns?

Fixed.

> > Source/WebCore/rendering/InlineIterator.h:470
> > +        // FIXME: Could this initialize from this->inIsolate() instead of walking up the render tree?
> 
> It seems like we should be able to.

It's unclear if that's valid or not.  Leaving that for another change.

> > Source/WebCore/rendering/InlineIterator.h:478
> > +            if (isolateTracker.inIsolate())
> > +                isolateTracker.addFakeRunIfNecessary(obj, *this);
> > +            else
> > +                RenderBlock::appendRunsForObject(m_runs, start, obj->length(), obj, *this);
> 
> I don't like the fact we have to duplicate this code twice.  Can we wrap the call to appendRunsForObject in IsolatedTracker? so that we have just one function call here.  IsolatedTracker can then check whether we're in an isolated contents or not and call appropriate function.

We could, but it makes the IsolateTracker's purpose somewhat murky.  I'd like to leave this for another change.

> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:809
> > +        // Only inlines make sense with unicode-bidi: isolate (blocks are already isolated).
> > +        RenderInline* isolatedSpan = toRenderInline(isolatedRun->object());
> 
> We should assert that they're indeed RenderInlines.

toRenderInline does that already.

> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:819
> > +        // FIXME: isolatedEnd should probably equal end or the last char in isolatedSpan.
> > +        InlineIterator isolatedEnd = endOfLine;
> 
> I think this is okay because of the following line in appendRun:
> while (obj && obj != m_eor.m_obj && obj != endOfLine.m_obj) {
> eor should always be in the isolatedSpan in a sane world.
> 
> > Source/WebCore/rendering/RenderBlockLineLayout.cpp:820
> > +        // FIXME: What should end and previousLineBrokeCleanly be?
> 
> previouslyLineBrokeCleanly should probably be false here.  In createBidiRunsForLine:
> 
>         if (pastEnd && (hardLineBreak || m_current.atEnd())) {
>             BidiContext* c = context();
>             if (hardLineBreak) {
>                 // A deviation from the Unicode Bidi Algorithm in order to match
>                 // WinIE and user expectations: hard line breaks reset bidi state
>                 // coming from unicode bidi control characters, but not those from
>                 // DOM nodes with specified directionality
>                 stateAtEnd.setContext(c->copyStackRemovingUnicodeEmbeddingContexts());
> 
>                 dirCurrent = stateAtEnd.context()->dir();
>                 stateAtEnd.setEorDir(dirCurrent);
>                 stateAtEnd.setLastDir(dirCurrent);
>                 stateAtEnd.setLastStrongDir(dirCurrent);
>             } else {
> 
> This is the only place where the value of hardLineBreak is used, and I don't think we need to patch winIE inside isolated contents.

[Eric Seidel (no email)]

Created attachment 106633 [details]
Updated with rniwa's suggestions

[Ryosuke Niwa]

Comment on attachment 106633 [details]
Updated with rniwa's suggestions

Alright. Let's land this patch!

[Eric Seidel (no email)]

Committed r94775: <http://trac.webkit.org/changeset/94775>