Changeset 93559 in webkit


Ignore:
Timestamp:
Aug 22, 2011 4:34:31 PM (13 years ago)
Author:
eric@webkit.org
Message:

Attempt to clean up bidiNext usage
​https://bugs.webkit.org/show_bug.cgi?id=66721

Reviewed by Ryosuke Niwa.

bidiNext and bidiFirst are horribly confusing.
Even worse is that bidiNext takes a bunch of mutually exclusive options.
It appears that there is a "return me every inline, even if its empty"
mode which is only used for simplified inline layout in RenderBlock.cpp.
To support that mode, there is a endOfInline pointer which keeps track
of if we just returned at the end of an inline to so we don't get stuck in
and empty inline (unable to distinguish the start from the finish).

The actual bidi/line-layout code uses bidiNext/bidiFirst in a "skip empty inlines"
mode. (Since empty inlines do not participate in the Unicode Bidi Algorithm.)

This change renames bidiNext to bidiNextShared (still a horrible name) and moves
all callers to explicitly calling bidiNextSkippingEmptyInlines or bidiNextIncludingEmptyInlines.
It becomes obvious which code uses which.

In reviewing this code be aware that the previous bidiNext default was to "skip empty inlines" (skipInlines = true).
Thus any caller who didn't pass true/false should now be calling bidiNextSkippingEmptyInlines instead.

No functional change, thus no tests.

  • rendering/InlineIterator.h:

(WebCore::bidiNextShared):
(WebCore::bidiNextSkippingEmptyInlines):
(WebCore::bidiNextIncludingEmptyInlines):
(WebCore::bidiFirstSkippingEmptyInlines):
(WebCore::bidiFirstIncludingEmptyInlines):
(WebCore::InlineWalker::InlineWalker):
(WebCore::InlineWalker::advance):
(WebCore::InlineIterator::increment):
(WebCore::InlineBidiResolver::appendRun):

  • rendering/RenderBlockLineLayout.cpp:

(WebCore::RenderBlock::determineStartPosition):
(WebCore::shouldSkipWhitespaceAfterStartObject):
(WebCore::RenderBlock::LineBreaker::nextLineBreak):

Location:
trunk/Source/WebCore
Files:
3 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r93558 r93559  
     12011-08-22  Eric Seidel  <eric@webkit.org>
     2
     3        Attempt to clean up bidiNext usage
     4        https://bugs.webkit.org/show_bug.cgi?id=66721
     5
     6        Reviewed by Ryosuke Niwa.
     7
     8        bidiNext and bidiFirst are horribly confusing.
     9        Even worse is that bidiNext takes a bunch of mutually exclusive options.
     10        It appears that there is a "return me every inline, even if its empty"
     11        mode which is only used for simplified inline layout in RenderBlock.cpp.
     12        To support that mode, there is a endOfInline pointer which keeps track
     13        of if we just returned at the end of an inline to so we don't get stuck in
     14        and empty inline (unable to distinguish the start from the finish).
     15
     16        The actual bidi/line-layout code uses bidiNext/bidiFirst in a "skip empty inlines"
     17        mode.  (Since empty inlines do not participate in the Unicode Bidi Algorithm.)
     18
     19        This change renames bidiNext to bidiNextShared (still a horrible name) and moves
     20        all callers to explicitly calling bidiNextSkippingEmptyInlines or bidiNextIncludingEmptyInlines.
     21        It becomes obvious which code uses which.
     22
     23        In reviewing this code be aware that the previous bidiNext default was to "skip empty inlines" (skipInlines = true).
     24        Thus any caller who didn't pass true/false should now be calling bidiNextSkippingEmptyInlines instead.
     25
     26        No functional change, thus no tests.
     27
     28        * rendering/InlineIterator.h:
     29        (WebCore::bidiNextShared):
     30        (WebCore::bidiNextSkippingEmptyInlines):
     31        (WebCore::bidiNextIncludingEmptyInlines):
     32        (WebCore::bidiFirstSkippingEmptyInlines):
     33        (WebCore::bidiFirstIncludingEmptyInlines):
     34        (WebCore::InlineWalker::InlineWalker):
     35        (WebCore::InlineWalker::advance):
     36        (WebCore::InlineIterator::increment):
     37        (WebCore::InlineBidiResolver::appendRun):
     38        * rendering/RenderBlockLineLayout.cpp:
     39        (WebCore::RenderBlock::determineStartPosition):
     40        (WebCore::shouldSkipWhitespaceAfterStartObject):
     41        (WebCore::RenderBlock::LineBreaker::nextLineBreak):
     42
    1432011-08-22  David Levin  <levin@chromium.org>
    244

  • trunk/Source/WebCore/rendering/InlineIterator.h

    r88122 r93559  
    140140}
    141141
     142// This enum is only used for bidiNextShared()
     143enum EmptyInlineBehavior {
     144    SkipEmptyInlines,
     145    IncludeEmptyInlines,
     146};
     147
    142148// FIXME: This function is misleadingly named. It has little to do with bidi.
    143149// This function will iterate over inlines within a block, optionally notifying
    144150// a bidi resolver as it enters/exits inlines (so it can push/pop embedding levels).
    145 static inline RenderObject* bidiNext(RenderObject* root, RenderObject* current, InlineBidiResolver* resolver = 0, bool skipInlines = true, bool* endOfInlinePtr = 0)
     151static inline RenderObject* bidiNextShared(RenderObject* root, RenderObject* current, InlineBidiResolver* resolver = 0, EmptyInlineBehavior emptyInlineBehavior = SkipEmptyInlines, bool* endOfInlinePtr = 0)
    146152{
    147153    RenderObject* next = 0;
    … …  
    160166        if (!next) {
    161167            // If it is a renderer we care about, and we're doing our inline-walk, return it.
    162             if (!skipInlines && !oldEndOfInline && current->isRenderInline()) {
     168            if (emptyInlineBehavior == IncludeEmptyInlines && !oldEndOfInline && current->isRenderInline()) {
    163169                next = current;
    164170                endOfInline = true;
    … …  
    176182
    177183                current = current->parent();
    178                 if (!skipInlines && current && current != root && current->isRenderInline()) {
     184                if (emptyInlineBehavior == IncludeEmptyInlines && current && current != root && current->isRenderInline()) {
    179185                    next = current;
    180186                    endOfInline = true;
    … …  
    188194
    189195        if (isIteratorTarget(next)
    190             || ((!skipInlines || !next->firstChild()) // Always return EMPTY inlines.
     196            || ((emptyInlineBehavior == IncludeEmptyInlines || !next->firstChild()) // Always return EMPTY inlines.
    191197                && next->isRenderInline()))
    192198            break;
    … …  
    200206}
    201207
    202 static inline RenderObject* bidiFirstSkippingInlines(RenderObject* root, InlineBidiResolver* resolver)
     208static inline RenderObject* bidiNextSkippingEmptyInlines(RenderObject* root, RenderObject* current, InlineBidiResolver* observer = 0)
     209{
     210    // The SkipEmptyInlines callers never care about endOfInlinePtr.
     211    return bidiNextShared(root, current, observer, SkipEmptyInlines);
     212}
     213
     214static inline RenderObject* bidiNextIncludingEmptyInlines(RenderObject* root, RenderObject* current, bool* endOfInlinePtr = 0)
     215{
     216    InlineBidiResolver* observer = 0; // Callers who include empty inlines, never use an observer.
     217    return bidiNextShared(root, current, observer, IncludeEmptyInlines, endOfInlinePtr);
     218}
     219
     220static inline RenderObject* bidiFirstSkippingEmptyInlines(RenderObject* root, InlineBidiResolver* resolver)
    203221{
    204222    ASSERT(resolver);
    … …  
    210228        notifyResolverEnteredObject(resolver, o);
    211229        if (o->firstChild())
    212             o = bidiNext(root, o, resolver, true);
     230            o = bidiNextSkippingEmptyInlines(root, o, resolver);
    213231        else {
    214232            // Never skip empty inlines.
    … …  
    221239    // FIXME: Unify this with the bidiNext call above.
    222240    if (o && !isIteratorTarget(o))
    223         o = bidiNext(root, o, resolver, true);
     241        o = bidiNextSkippingEmptyInlines(root, o, resolver);
    224242
    225243    resolver->commitExplicitEmbedding();
    … …  
    228246
    229247// FIXME: This method needs to be renamed when bidiNext finds a good name.
    230 static inline RenderObject* bidiFirstNotSkippingInlines(RenderObject* root)
     248static inline RenderObject* bidiFirstIncludingEmptyInlines(RenderObject* root)
    231249{
    232250    RenderObject* o = root->firstChild();
    … …  
    236254        return o;
    237255
    238     return bidiNext(root, o, 0, false);
     256    return bidiNextIncludingEmptyInlines(root, o);
    239257}
    240258
    … …  
    247265}
    248266
     267// FIXME: This is used by RenderBlock for simplified layout, and has nothing to do with bidi
     268// it shouldn't use functions called bidiFirst and bidiNext.
    249269class InlineWalker {
    250270public:
    … …  
    254274        , m_atEndOfInline(false)
    255275    {
    256         // FIXME: This class should be taught how to do the skipInlines codepath as well.
    257         m_current = bidiFirstNotSkippingInlines(m_root);
     276        // FIXME: This class should be taught how to do the SkipEmptyInlines codepath as well.
     277        m_current = bidiFirstIncludingEmptyInlines(m_root);
    258278    }
    259279
    … …  
    266286    RenderObject* advance()
    267287    {
    268         // FIXME: Eventually skipInlines and observer will be a members.
    269         bool skipInlines = false;
    270         InlineBidiResolver* observer = 0;
    271         m_current = bidiNext(m_root, m_current, observer, skipInlines, &m_atEndOfInline);
     288        // FIXME: Support SkipEmptyInlines and observer parameters.
     289        m_current = bidiNextIncludingEmptyInlines(m_root, m_current, &m_atEndOfInline);
    272290        return m_current;
    273291    }
    … …  
    275293    RenderObject* m_root;
    276294    RenderObject* m_current;
    277     bool m_skipInlines;
    278295    bool m_atEndOfInline;
    279296};
    … …  
    289306    }
    290307    // bidiNext can return 0, so use moveTo instead of moveToStartOf
    291     moveTo(bidiNext(m_root, m_obj, resolver), 0);
     308    moveTo(bidiNextSkippingEmptyInlines(m_root, m_obj, resolver), 0);
    292309}
    293310
    … …  
    344361            RenderBlock::appendRunsForObject(m_runs, start, obj->length(), obj, *this);
    345362            start = 0;
    346             obj = bidiNext(m_sor.root(), obj);
     363            obj = bidiNextSkippingEmptyInlines(m_sor.root(), obj);
    347364        }
    348365        if (obj) {

  • trunk/Source/WebCore/rendering/RenderBlockLineLayout.cpp

    r93421 r93559  
    14281428    } else {
    14291429        TextDirection direction = style()->direction();
    1430         if (style()->unicodeBidi() == Plaintext)
    1431             determineParagraphDirection(direction, InlineIterator(this, bidiFirstNotSkippingInlines(this), 0));
     1430        if (style()->unicodeBidi() == Plaintext) {
     1431            // FIXME: Why does "unicode-bidi: plaintext" bidiFirstIncludingEmptyInlines when all other line layout code uses bidiFirstSkippingEmptyInlines?
     1432            determineParagraphDirection(direction, InlineIterator(this, bidiFirstIncludingEmptyInlines(this), 0));
     1433        }
    14321434        resolver.setStatus(BidiStatus(direction, style()->unicodeBidi() == Override));
    1433         resolver.setPosition(InlineIterator(this, bidiFirstSkippingInlines(this, &resolver), 0));
     1435        resolver.setPosition(InlineIterator(this, bidiFirstSkippingEmptyInlines(this, &resolver), 0));
    14341436    }
    14351437    return curr;
    … …  
    16471649static bool shouldSkipWhitespaceAfterStartObject(RenderBlock* block, RenderObject* o, LineMidpointState& lineMidpointState)
    16481650{
    1649     RenderObject* next = bidiNext(block, o);
     1651    RenderObject* next = bidiNextSkippingEmptyInlines(block, o);
    16501652    if (next && !next->isBR() && next->isText() && toRenderText(next)->textLength() > 0) {
    16511653        RenderText* nextText = toRenderText(next);
    … …  
    19841986    EWhiteSpace lastWS = currWS;
    19851987    while (current.m_obj) {
    1986         RenderObject* next = bidiNext(m_block, current.m_obj);
     1988        RenderObject* next = bidiNextSkippingEmptyInlines(m_block, current.m_obj);
    19871989        if (next && next->parent() && !next->parent()->isDescendantOf(current.m_obj->parent()))
    19881990            includeEndWidth = true;
Note: See TracChangeset for help on using the changeset viewer.