[webkit-reviews] review denied: [Bug 50912] [BiDi] [CSS3] MASTER: Add support for the unicode-bidi:isolate CSS property : [Attachment 104963] A final working patch, which is ready for landing if I can find a brave enough reviewer

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Eric Seidel
<eric at webkit.org>'s request for review:
Bug 50912: [BiDi] [CSS3] MASTER: Add support for the unicode-bidi:isolate CSS
property
https://bugs.webkit.org/show_bug.cgi?id=50912

Attachment 104963: A final working patch, which is ready for landing if I can
find a brave enough reviewer
https://bugs.webkit.org/attachment.cgi?id=104963&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
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.