[webkit-reviews] review denied: [Bug 23124] RTL: Directionality always reset on hard line break : [Attachment 87328] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has denied Levi Weintraub <leviw at chromium.org>'s
request for review:
Bug 23124: RTL:  Directionality always reset on hard line break
https://bugs.webkit.org/show_bug.cgi?id=23124

Attachment 87328: Patch
https://bugs.webkit.org/attachment.cgi?id=87328&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=87328&action=review

I think this is good,b ut needs another round of cleanup.

> Source/WebCore/platform/text/BidiContext.cpp:62
> +static inline PassRefPtr<BidiContext> clone(BidiContext* context,
BidiContext* parent)

I would separate this into two functions. One which is a copy() function on the
BidiContext, and a second where you set the level by calcuating a new level in
a local helper fuction.

> Source/WebCore/platform/text/BidiContext.cpp:69
> +	   // Go to the least greater odd integer
> +	   newLevel += 1;
> +	   newLevel |= 1;

Just put these into helper functions.  nextGreaterOdd()

> Source/WebCore/platform/text/BidiContext.cpp:78
> +// The BidiContext stack must be immutable -- they're re-used for repainting
-- so

They're re-used when we re-layout after dom modification/editing.

> Source/WebCore/platform/text/BidiContext.cpp:80
> +PassRefPtr<BidiContext> BidiContext::clearUnicodeEmbeddingContexts()

copyStackRemovingUnicodeEmbeddingContexts?

> Source/WebCore/platform/text/BidiContext.cpp:82
> +    Vector<BidiContext*> contexts;

Either this should have inline capacity of 64 (to avoid ever mallocing) or you
shoudl do this in two passes.  One which makes a clone of the list w/o
unicodeEmbeddings, and a second walk which fixes the levels.

I think I'm slightly more in favor of doing two passes.  But it's really up to
you.  A linked list walk is also expensive, but probably less expensive than a
malloc.  Although we can avoid the malloc with the inline capcity.  so again,
up to you.

> Source/WebCore/platform/text/BidiContext.h:33
> +enum BidiEmbeddingClass {

This seems confusing to me.  Maybe BidiSource?	ContextSource?	Donno.	But
currently confusing.

> Source/WebCore/platform/text/BidiContext.h:34
> +    DOMEmbedding,

could be FromDOM and FromUnicode?  BidiEmbeddingSource?  maybe?

> Source/WebCore/platform/text/BidiContext.h:65
> +    BidiEmbeddingClass m_embeddingClass : 1;

There may be some concern about makign sure this is unsigned on MSVC.  I dont'
rmember the exact rules.  but enums + bitfields + msvc = confusion.

> Source/WebCore/platform/text/BidiResolver.h:80
> +    BidiEmbedding(WTF::Unicode::Direction direction, BidiEmbeddingClass
embeddingClass = UnicodeEmbedding)

I'm not sure you want a default argumetn value here.

> Source/WebCore/platform/text/BidiResolver.h:82
> +    : m_direction(direction)
> +    , m_embeddingClass(embeddingClass)

indent.

> Source/WebCore/platform/text/BidiResolver.h:610
> +	       embed(BidiEmbedding(dirCurrent));

This can be implicitly constructed, no?  I geuss it's better to be clear.

> Source/WebCore/rendering/InlineIterator.h:161
> +		   resolver->embed(BidiEmbedding(d, DOMEmbedding));

I think it's cleaner to make embed just take two params, and ahve it construct
the Embedding thing.