Fake Bidi run does not need to be always created

Source/core/rendering/InlineIterator.h

Index: Source/core/rendering/InlineIterator.h
diff --git a/Source/core/rendering/InlineIterator.h b/Source/core/rendering/InlineIterator.h
index 79ddbc6fb117ef1c8007c0034c1ccb14be40b6d8..844039d0a63b5d651371397688a558c75dbd3031 100644
--- a/Source/core/rendering/InlineIterator.h
+++ b/Source/core/rendering/InlineIterator.h
@@ -467,7 +467,7 @@ static inline unsigned numberOfIsolateAncestors(const InlineIterator& iter)
 
 // FIXME: This belongs on InlineBidiResolver, except it's a template specialization
 // of BidiResolver which knows nothing about RenderObjects.
-static inline void addPlaceholderRunForIsolatedInline(InlineBidiResolver& resolver, RenderObject* obj, unsigned pos)
+static inline BidiRun* addPlaceholderRunForIsolatedInline(InlineBidiResolver& resolver, RenderObject* obj, unsigned pos)
 {
     ASSERT(obj);
     BidiRun* isolatedRun = new BidiRun(pos, pos, obj, resolver.context(), resolver.dir());
@@ -475,8 +475,7 @@ static inline void addPlaceholderRunForIsolatedInline(InlineBidiResolver& resolv
     // FIXME: isolatedRuns() could be a hash of object->run and then we could cheaply
     // ASSERT here that we didn't create multiple objects for the same inline.
     resolver.isolatedRuns().append(isolatedRun);
-    LineMidpointState& lineMidpointState = resolver.midpointState();
-    resolver.setMidpointStateForIsolatedRun(isolatedRun, lineMidpointState);
+    return isolatedRun;
 }
 
 static inline BidiRun* createRun(int start, int end, RenderObject* obj, InlineBidiResolver& resolver)
@@ -484,55 +483,6 @@ static inline BidiRun* createRun(int start, int end, RenderObject* obj, InlineBi
     return new BidiRun(start, end, obj, resolver.context(), resolver.dir());
 }
 
-enum AppendRunBehavior {
-    AppendingFakeRun,
-    AppendingRunsForObject
-};
-
-static void adjustMidpointsAndAppendRunsForObjectIfNeeded(RenderObject* obj, unsigned start, unsigned end, InlineBidiResolver& resolver, AppendRunBehavior behavior = AppendingFakeRun, BidiRunList<BidiRun>* runs = 0)
-{
-    if (start > end || RenderBlock::shouldSkipCreatingRunsForObject(obj))
-        return;
-
-    LineMidpointState& lineMidpointState = resolver.midpointState();
-    bool haveNextMidpoint = (lineMidpointState.currentMidpoint < lineMidpointState.numMidpoints);
-    InlineIterator nextMidpoint;
-    if (haveNextMidpoint)
-        nextMidpoint = lineMidpointState.midpoints[lineMidpointState.currentMidpoint];
-    if (lineMidpointState.betweenMidpoints) {
-        if (!(haveNextMidpoint && nextMidpoint.m_obj == obj))
-            return;
-        // This is a new start point. Stop ignoring objects and
-        // adjust our start.
-        lineMidpointState.betweenMidpoints = false;
-        start = nextMidpoint.m_pos;
-        lineMidpointState.currentMidpoint++;
-        if (start < end)
-            return adjustMidpointsAndAppendRunsForObjectIfNeeded(obj, start, end, resolver, behavior, runs);
-    } else {
-        if (!haveNextMidpoint || (obj != nextMidpoint.m_obj)) {
-            if (behavior == AppendingFakeRun)
-                return;
-            runs->addRun(createRun(start, end, obj, resolver));
-            return;
-        }
-
-        // An end midpoint has been encountered within our object. We
-        // need to go ahead and append a run with our endpoint.
-        if (static_cast<int>(nextMidpoint.m_pos + 1) <= end) {
-            lineMidpointState.betweenMidpoints = true;
-            lineMidpointState.currentMidpoint++;
-            if (nextMidpoint.m_pos != UINT_MAX) { // UINT_MAX means stop at the object and don't nclude any of it.
-                if (static_cast<int>(nextMidpoint.m_pos + 1) > start && behavior == AppendingRunsForObject)
-                    runs->addRun(createRun(start, nextMidpoint.m_pos + 1, obj, resolver));
-                return adjustMidpointsAndAppendRunsForObjectIfNeeded(obj, nextMidpoint.m_pos + 1, end, resolver, behavior, runs);
-            }
-        } else if (behavior == AppendingRunsForObject) {
-            runs->addRun(createRun(start, end, obj, resolver));
-        }
-    }
-}
-
 class IsolateTracker {
 public:
     explicit IsolateTracker(unsigned nestedIsolateCount)
@@ -541,6 +491,11 @@ public:
     {
     }
 
+    void setMidpointStateForRootIsolate(const LineMidpointState& midpointState)
+    {
+        m_midpointStateForRootIsolate = midpointState;
+    }
+
     void enterIsolate() { m_nestedIsolateCount++; }
     void exitIsolate()
     {
@@ -555,8 +510,7 @@ public:
     void embed(WTF::Unicode::Direction, BidiEmbeddingSource) { }
     void commitExplicitEmbedding() { }
 
-
-    void addFakeRunIfNecessary(RenderObject* obj, unsigned pos, unsigned end, InlineBidiResolver& resolver)
+    void addFakeRunIfNecessary(unsigned pos, unsigned end, RenderObject* obj, InlineBidiResolver& resolver)
     {
         // We only need to add a fake run for a given isolated span once during each call to createBidiRunsForLine.
         // We'll be called for every span inside the isolated span so we just ignore subsequent calls.
@@ -564,11 +518,10 @@ public:
         if (RenderBlock::shouldSkipCreatingRunsForObject(obj))
             return;
         if (!m_haveAddedFakeRunForRootIsolate) {
-            addPlaceholderRunForIsolatedInline(resolver, obj, pos);
+            BidiRun* run = addPlaceholderRunForIsolatedInline(resolver, obj, pos);
+            resolver.setMidpointStateForIsolatedRun(run, m_midpointStateForRootIsolate);
             m_haveAddedFakeRunForRootIsolate = true;
         }
-        adjustMidpointsAndAppendRunsForObjectIfNeeded(obj, pos, end, resolver);
-
         // obj and pos together denote a single position in the inline, from which the parsing of the isolate will start.
         // We don't need to mark the end of the run because this is implicit: it is either endOfLine or the end of the
         // isolate, when we call createBidiRunsForLine it will stop at whichever comes first.
@@ -577,8 +530,67 @@ public:
 private:
     unsigned m_nestedIsolateCount;
     bool m_haveAddedFakeRunForRootIsolate;
+    LineMidpointState m_midpointStateForRootIsolate;
+};
+
+struct AddFakeRunIfNecessaryFunctor {
+    IsolateTracker& isolateTracker;
+    void operator()(int start, int end, RenderObject* obj, InlineBidiResolver& resolver)
+    {
+        isolateTracker.addFakeRunIfNecessary(start, end, obj, resolver);
+    }
 };
 
+struct AddRunFunctor {
+    void operator()(int start, int end, RenderObject* obj, InlineBidiResolver& resolver)
+    {
+        RenderBlockFlow::appendRunsForObject(start, end, obj, resolver);
+    }
+};
+
+template <typename Functor>
+static void adjustMidpointsAndAppendRunsForObjectIfNeeded(RenderObject* obj, unsigned start, unsigned end, InlineBidiResolver& resolver, Functor addRunFunction)
+{
+    if (start > end || RenderBlock::shouldSkipCreatingRunsForObject(obj))
+        return;
+
+    LineMidpointState& lineMidpointState = resolver.midpointState();
+    bool haveNextMidpoint = (lineMidpointState.currentMidpoint < lineMidpointState.numMidpoints);
+    InlineIterator nextMidpoint;
+    if (haveNextMidpoint)
+        nextMidpoint = lineMidpointState.midpoints[lineMidpointState.currentMidpoint];
+    if (lineMidpointState.betweenMidpoints) {
+        if (!(haveNextMidpoint && nextMidpoint.m_obj == obj))
+            return;
+        // This is a new start point. Stop ignoring objects and
+        // adjust our start.
+        lineMidpointState.betweenMidpoints = false;
+        start = nextMidpoint.m_pos;
+        lineMidpointState.currentMidpoint++;
+        if (start < end)
+            return adjustMidpointsAndAppendRunsForObjectIfNeeded(obj, start, end, resolver, addRunFunction);
+    } else {
+        if (!haveNextMidpoint || (obj != nextMidpoint.m_obj)) {
+            addRunFunction(start, end, obj, resolver);
+            return;
+        }
+
+        // An end midpoint has been encountered within our object. We
+        // need to go ahead and append a run with our endpoint.
+        if (static_cast<int>(nextMidpoint.m_pos + 1) <= end) {
+            lineMidpointState.betweenMidpoints = true;
+            lineMidpointState.currentMidpoint++;
+            if (nextMidpoint.m_pos != UINT_MAX) { // UINT_MAX means stop at the object and don't nclude any of it.
+                if (static_cast<int>(nextMidpoint.m_pos + 1) > start)
+                    addRunFunction(start, nextMidpoint.m_pos + 1, obj, resolver);
+                return adjustMidpointsAndAppendRunsForObjectIfNeeded(obj, nextMidpoint.m_pos + 1, end, resolver, addRunFunction);
+            }
+        } else {
+            addRunFunction(start, end, obj, resolver);
+        }
+    }
+}
+
 template <>
 inline void InlineBidiResolver::appendRun()
 {
@@ -590,10 +602,14 @@ inline void InlineBidiResolver::appendRun()
         int start = m_sor.m_pos;
         RenderObject* obj = m_sor.m_obj;
         while (obj && obj != m_eor.m_obj && obj != endOfLine.m_obj) {
-            if (isolateTracker.inIsolate())
-                isolateTracker.addFakeRunIfNecessary(obj, start, obj->length(), *this);
-            else
-                RenderBlockFlow::appendRunsForObject(m_runs, start, obj->length(), obj, *this);
+            if (isolateTracker.inIsolate()) {
+                AddFakeRunIfNecessaryFunctor functor = {isolateTracker};
+                isolateTracker.setMidpointStateForRootIsolate(midpointState());
+                adjustMidpointsAndAppendRunsForObjectIfNeeded(obj, start, obj->length(), *this, functor);
+            } else {
+                AddRunFunctor functor;
+                adjustMidpointsAndAppendRunsForObjectIfNeeded(obj, start, obj->length(), *this, functor);
+            }
             // FIXME: start/obj should be an InlineIterator instead of two separate variables.
             start = 0;
             obj = bidiNextSkippingEmptyInlines(m_sor.root(), obj, &isolateTracker);
@@ -606,10 +622,14 @@ inline void InlineBidiResolver::appendRun()
             }
             // It's OK to add runs for zero-length RenderObjects, just don't make the run larger than it should be
             int end = obj->length() ? pos + 1 : 0;
-            if (isolateTracker.inIsolate())
-                isolateTracker.addFakeRunIfNecessary(obj, start, end, *this);
-            else
-                RenderBlockFlow::appendRunsForObject(m_runs, start, end, obj, *this);
+            if (isolateTracker.inIsolate()) {

[eseidel, 2013/10/24 18:10:05]

Why not just move this check inside adjustMidpointsAndAppendRunsForObject (or
some wrapper).  It looks like we're just repeating the same if twice?

+                AddFakeRunIfNecessaryFunctor functor = {isolateTracker};
+                isolateTracker.setMidpointStateForRootIsolate(midpointState());
+                adjustMidpointsAndAppendRunsForObjectIfNeeded(obj, start, end, *this, functor);
+            } else {
+                AddRunFunctor functor;
+                adjustMidpointsAndAppendRunsForObjectIfNeeded(obj, start, end, *this, functor);
+            }
         }
 
         m_eor.increment();