Bug 1141931 part 4 - Make bidi reordering always in frame order. r=jfkthame,smontagu
☠☠ backed out by 44c475f5adc1 ☠ ☠
authorXidorn Quan <quanxunzhen@gmail.com>
Wed, 08 Apr 2015 08:40:31 +1200
changeset 238003 48cf9568a4b18c834dc4f7e72d9f1df7d01352f4
parent 238002 f1ab848b3fa6cb2f1af40f949922b3eadd2e6de2
child 238004 f432612b6475043f68d405eb5e38aad1be936b2e
push id58082
push userxquan@mozilla.com
push dateTue, 07 Apr 2015 20:41:55 +0000
treeherdermozilla-inbound@26118238484f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjfkthame, smontagu
bugs1141931
milestone40.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1141931 part 4 - Make bidi reordering always in frame order. r=jfkthame,smontagu
layout/base/nsBidiPresUtils.cpp
layout/base/nsBidiPresUtils.h
--- a/layout/base/nsBidiPresUtils.cpp
+++ b/layout/base/nsBidiPresUtils.cpp
@@ -1391,26 +1391,29 @@ nsBidiPresUtils::IsFirstOrLast(nsIFrame*
     if (aIsLast) {
       aFrame->AddStateBits(NS_INLINE_FRAME_BIDI_VISUAL_IS_LAST);
     } else {
       aFrame->RemoveStateBits(NS_INLINE_FRAME_BIDI_VISUAL_IS_LAST);
     }
   }
 }
 
-void
+/* static */ nscoord
 nsBidiPresUtils::RepositionFrame(nsIFrame* aFrame,
                                  bool aIsEvenLevel,
-                                 nscoord& aStart,
+                                 nscoord aStartOrEnd,
                                  const nsContinuationStates* aContinuationStates,
                                  WritingMode aContainerWM,
+                                 bool aContainerReverseDir,
                                  nscoord aContainerISize)
 {
+  NS_ASSERTION(aContainerISize != NS_UNCONSTRAINEDSIZE,
+               "Unconstrained inline line size in bidi frame reordering");
   if (!aFrame)
-    return;
+    return 0;
 
   bool isFirst, isLast;
   WritingMode frameWM = aFrame->GetWritingMode();
   IsFirstOrLast(aFrame,
                 aContinuationStates,
                 aContainerWM.IsBidiLTR() == frameWM.IsBidiLTR(),
                 isFirst /* out */,
                 isLast /* out */);
@@ -1420,87 +1423,81 @@ nsBidiPresUtils::RepositionFrame(nsIFram
   // container's writing mode. To get the right values, we set start and
   // end margins on a logical margin in the frame's writing mode, and
   // then convert the margin to the container's writing mode to set the
   // coordinates.
 
   // This method is called from nsBlockFrame::PlaceLine via the call to
   // bidiUtils->ReorderFrames, so this is guaranteed to be after the inlines
   // have been reflowed, which is required for GetUsedMargin/Border/Padding
+  nscoord frameISize = aFrame->ISize();
   LogicalMargin frameMargin = aFrame->GetLogicalUsedMargin(frameWM);
   LogicalMargin borderPadding = aFrame->GetLogicalUsedBorderAndPadding(frameWM);
+  // Since the visual order of frame could be different from the
+  // continuation order, we need to remove any border/padding first,
+  // so that we can get the correct isize of the current frame.
+  if (!aFrame->GetPrevContinuation()) {
+    frameISize -= borderPadding.IStart(frameWM);
+  }
+  if (!aFrame->GetNextContinuation()) {
+    frameISize -= borderPadding.IEnd(frameWM);
+  }
   if (!isFirst) {
     frameMargin.IStart(frameWM) = 0;
     borderPadding.IStart(frameWM) = 0;
   }
   if (!isLast) {
     frameMargin.IEnd(frameWM) = 0;
     borderPadding.IEnd(frameWM) = 0;
   }
-  LogicalMargin margin = frameMargin.ConvertTo(aContainerWM, frameWM);
-  aStart += margin.IStart(aContainerWM);
+  frameISize += borderPadding.IStartEnd(frameWM);
 
-  nscoord start = aStart;
-
+  bool reverseDir = aIsEvenLevel != frameWM.IsBidiLTR();
+  nscoord icoord = 0;
   if (!IsBidiLeaf(aFrame)) {
-    // If the resolved direction of the container is different from the
-    // direction of the frame, we need to traverse the child list in reverse
-    // order, to make it O(n) we store the list locally and iterate the list
-    // in reverse
-    bool reverseOrder = aIsEvenLevel != frameWM.IsBidiLTR();
-    nsTArray<nsIFrame*> childList;
-    nsIFrame *frame = aFrame->GetFirstPrincipalChild();
-    if (frame && reverseOrder) {
-      childList.AppendElement((nsIFrame*)nullptr);
-      while (frame) {
-        childList.AppendElement(frame);
-        frame = frame->GetNextSibling();
-      }
-      frame = childList[childList.Length() - 1];
+    icoord += reverseDir ?
+      borderPadding.IEnd(frameWM) : borderPadding.IStart(frameWM);
+    // Reposition the child frames
+    for (nsFrameList::Enumerator e(aFrame->PrincipalChildList());
+         !e.AtEnd(); e.Next()) {
+      icoord += RepositionFrame(e.get(), aIsEvenLevel, icoord,
+                                aContinuationStates,
+                                frameWM, reverseDir, frameISize);
     }
-
-    // Reposition the child frames
-    int32_t index = 0;
-    nscoord iCoord = borderPadding.IStart(frameWM);
-
-    while (frame) {
-      RepositionFrame(frame,
-                      aIsEvenLevel,
-                      iCoord,
-                      aContinuationStates,
-                      frameWM,
-                      aFrame->ISize());
-      index++;
-      frame = reverseOrder ?
-                childList[childList.Length() - index - 1] :
-                frame->GetNextSibling();
-    }
-
-    aStart += iCoord + borderPadding.IEnd(frameWM);
+    icoord += reverseDir ?
+      borderPadding.IStart(frameWM) : borderPadding.IEnd(frameWM);
   } else {
-    aStart += aFrame->ISize(aContainerWM);
+    icoord +=
+      frameWM.IsOrthogonalTo(aContainerWM) ? aFrame->BSize() : frameISize;
   }
 
   // LogicalRect doesn't correctly calculate the vertical position
   // in vertical writing modes with right-to-left direction (Bug 1131451).
   // This does the correct calculation ad hoc pending the fix for that.
   nsRect rect = aFrame->GetRect();
-  NS_ASSERTION(aContainerWM.IsBidiLTR() || aContainerISize != NS_UNCONSTRAINEDSIZE,
-               "Unconstrained inline line size in bidi frame reordering");
 
-  nscoord frameIStart =
-    aContainerWM.IsBidiLTR() ? start : aContainerISize - aStart;
-  nscoord frameISize = aStart - start;
-
-  (aContainerWM.IsVertical() ? rect.y : rect.x) = frameIStart;
-  (aContainerWM.IsVertical() ? rect.height : rect.width) = frameISize;
-
+  LogicalMargin margin = frameMargin.ConvertTo(aContainerWM, frameWM);
+  // In the following variables, if aContainerReverseDir is true, i.e.
+  // the container is positioning its children in reverse of its logical
+  // direction, the "StartOrEnd" refers to the distance from the frame
+  // to the inline end edge of the container, elsewise, it refers to the
+  // distance to the inline start edge.
+  nscoord marginStartOrEnd = aContainerReverseDir ?
+    margin.IEnd(aContainerWM) : margin.IStart(aContainerWM);
+  nscoord frameStartOrEnd = aStartOrEnd + marginStartOrEnd;
+  // Whether we are placing frames from right to left.
+  // e.g. If the frames are placed reversely in LTR mode, they are
+  // actually placed from right to left.
+  bool orderingRTL = aContainerReverseDir == aContainerWM.IsBidiLTR();
+  (aContainerWM.IsVertical() ? rect.y : rect.x) = orderingRTL ?
+    aContainerISize - (frameStartOrEnd + icoord) : frameStartOrEnd;
+  (aContainerWM.IsVertical() ? rect.height : rect.width) = icoord;
   aFrame->SetRect(rect);
 
-  aStart += margin.IEnd(aContainerWM);
+  return icoord + margin.IStartEnd(aContainerWM);
 }
 
 void
 nsBidiPresUtils::InitContinuationStates(nsIFrame*              aFrame,
                                         nsContinuationStates*  aContinuationStates)
 {
   nsFrameContinuationState* state = aContinuationStates->PutEntry(aFrame);
   state->mFirstVisualFrame = nullptr;
@@ -1545,22 +1542,20 @@ nsBidiPresUtils::RepositionInlineFrames(
     limit = count;
   } else {
     index = count - 1;
     step = -1;
     limit = -1;
   }
   for (; index != limit; index += step) {
     frame = aBld->VisualFrameAt(index);
-    RepositionFrame(frame,
-                    !(IS_LEVEL_RTL(aBld->mLevels[aBld->mIndexMap[index]])),
-                    start,
-                    &continuationStates,
-                    aLineWM,
-                    aContainerISize);
+    start += RepositionFrame(
+      frame, !(IS_LEVEL_RTL(aBld->mLevels[aBld->mIndexMap[index]])),
+      start, &continuationStates,
+      aLineWM, false, aContainerISize);
   }
 }
 
 bool
 nsBidiPresUtils::CheckLineOrder(nsIFrame*  aFirstFrameOnLine,
                                 int32_t    aNumFramesOnLine,
                                 nsIFrame** aFirstVisual,
                                 nsIFrame** aLastVisual)
--- a/layout/base/nsBidiPresUtils.h
+++ b/layout/base/nsBidiPresUtils.h
@@ -415,28 +415,31 @@ private:
   /*
    * Position aFrame and its descendants to their visual places. Also if aFrame
    * is not leaf, resize it to embrace its children.
    *
    * @param aFrame               The frame which itself and its children are
    *                             going to be repositioned
    * @param aIsEvenLevel         TRUE means the embedding level of this frame
    *                             is even (LTR)
-   * @param[in,out] aStart       IN value is the starting position of aFrame
-   *                             (without considering its inline-start margin)
-   *                             OUT value will be the ending position of aFrame
-   *                             (after adding its inline-end margin)
+   * @param aStartOrEnd          The distance to the start or the end of aFrame
+   *                             without considering its inline margin. If the
+   *                             container is reordering frames in reverse
+   *                             direction, it's the distance to the end,
+   *                             otherwise, it's the distance to the start.
    * @param aContinuationStates  A map from nsIFrame* to nsFrameContinuationState
+   * @return                     The isize aFrame takes, including margins.
    */
-  static void RepositionFrame(nsIFrame* aFrame,
-                              bool aIsEvenLevel,
-                              nscoord& aStart,
-                              const nsContinuationStates* aContinuationStates,
-                              mozilla::WritingMode aContainerWM,
-                              nscoord aContainerISize);
+  static nscoord RepositionFrame(nsIFrame* aFrame,
+                                 bool aIsEvenLevel,
+                                 nscoord aStartOrEnd,
+                                 const nsContinuationStates* aContinuationStates,
+                                 mozilla::WritingMode aContainerWM,
+                                 bool aContainerReverseOrder,
+                                 nscoord aContainerISize);
 
   /*
    * Initialize the continuation state(nsFrameContinuationState) to
    * (nullptr, 0) for aFrame and its descendants.
    *
    * @param aFrame               The frame which itself and its descendants will
    *                             be initialized
    * @param aContinuationStates  A map from nsIFrame* to nsFrameContinuationState