[Webkit-unassigned] [Bug 23124] RTL: Directionality always reset on hard line break

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=23124


Eric Seidel <eric at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #87328|review?                     |review-
               Flag|                            |




--- Comment #24 from Eric Seidel <eric at webkit.org>  2011-03-29 08:19:33 PST ---
(From update of attachment 87328)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.