Add position information for DrawViews.

Summary: Previously, we had no information about the positioning of the view until after we had attached it.  We have the position information attached to the shadow node, but this attaches it to the DrawView as well.  It also removes the need for AbstractClippingDrawCommand.

Reviewed By: ahmedre

Differential Revision: D3609092

ReactAndroid/src/main/java/com/facebook/react/flat/AbstractClippingDrawCommand.java

@@ -1,67 +0,0 @@
-/**
- * Copyright (c) 2015-present, Facebook, Inc.
- * All rights reserved.
- *
- * This source code is licensed under the BSD-style license found in the
- * LICENSE file in the root directory of this source tree. An additional grant
- * of patent rights can be found in the PATENTS file in the same directory.
- */
-
-package com.facebook.react.flat;
-
-import android.graphics.Canvas;
-
-/* package */ abstract class AbstractClippingDrawCommand implements DrawCommand {
-
-  protected boolean mNeedsClipping;
-  private float mClipLeft;
-  private float mClipTop;
-  private float mClipRight;
-  private float mClipBottom;
-
-  public final boolean clipBoundsMatch(
-      float clipLeft,
-      float clipTop,
-      float clipRight,
-      float clipBottom) {
-    return mClipLeft == clipLeft && mClipTop == clipTop
-        && mClipRight == clipRight && mClipBottom == clipBottom;
-  }
-
-  public final void setClipBounds(
-      float clipLeft,
-      float clipTop,
-      float clipRight,
-      float clipBottom) {
-    mClipLeft = clipLeft;
-    mClipTop = clipTop;
-    mClipRight = clipRight;
-    mClipBottom = clipBottom;
-    mNeedsClipping = mClipLeft != Float.NEGATIVE_INFINITY;
-  }
-
-  public final float getClipLeft() {
-    return mClipLeft;
-  }
-
-  public final float getClipTop() {
-    return mClipTop;
-  }
-
-  public final float getClipRight() {
-    return mClipRight;
-  }
-
-  public final float getClipBottom() {
-    return mClipBottom;
-  }
-
-  protected final void applyClipping(Canvas canvas) {
-    // We put this check here to not clip when we have the default [-infinity, infinity] bounds,
-    // since clipRect in those cases is essentially no-op anyway. This is needed to fix a bug that
-    // shows up during screenshot testing. Note that checking one side is enough, since if one side
-    // is infinite, all sides will be infinite, since we only set infinite for all sides at the
-    // same time - conversely, if one side is finite, all sides will be finite.
-    canvas.clipRect(mClipLeft, mClipTop, mClipRight, mClipBottom);
-  }
-}

ReactAndroid/src/main/java/com/facebook/react/flat/AbstractDrawCommand.java

@@ -19,8 +19,7 @@ import android.graphics.Color;
  * The idea is to be able to reuse unmodified objects when we build up DrawCommands before we ship
  * them to UI thread, but we can only do that if DrawCommands are immutable.
  */
-/* package */ abstract class AbstractDrawCommand extends AbstractClippingDrawCommand
+/* package */ abstract class AbstractDrawCommand implements DrawCommand, Cloneable {
-    implements Cloneable {
 
   private float mLeft;
   private float mTop;
@@ -28,10 +27,66 @@ import android.graphics.Color;
   private float mBottom;
   private boolean mFrozen;
 
+  protected boolean mNeedsClipping;
+  private float mClipLeft;
+  private float mClipTop;
+  private float mClipRight;
+  private float mClipBottom;
+
+  public final boolean clipBoundsMatch(
+      float clipLeft,
+      float clipTop,
+      float clipRight,
+      float clipBottom) {
+    return mClipLeft == clipLeft && mClipTop == clipTop
+        && mClipRight == clipRight && mClipBottom == clipBottom;
+  }
+
+  public final void setClipBounds(
+      float clipLeft,
+      float clipTop,
+      float clipRight,
+      float clipBottom) {
+    mClipLeft = clipLeft;
+    mClipTop = clipTop;
+    mClipRight = clipRight;
+    mClipBottom = clipBottom;
+    // We put this check here to not clip when we have the default [-infinity, infinity] bounds,
+    // since clipRect in those cases is essentially no-op anyway. This is needed to fix a bug that
+    // shows up during screenshot testing. Note that checking one side is enough, since if one side
+    // is infinite, all sides will be infinite, since we only set infinite for all sides at the
+    // same time - conversely, if one side is finite, all sides will be finite.
+    mNeedsClipping = mClipLeft != Float.NEGATIVE_INFINITY;
+  }
+
+  public final float getClipLeft() {
+    return mClipLeft;
+  }
+
+  public final float getClipTop() {
+    return mClipTop;
+  }
+
+  public final float getClipRight() {
+    return mClipRight;
+  }
+
+  public final float getClipBottom() {
+    return mClipBottom;
+  }
+
+  protected final void applyClipping(Canvas canvas) {
+    canvas.clipRect(mClipLeft, mClipTop, mClipRight, mClipBottom);
+  }
+
+  /**
+   * Don't override this unless you need to do custom clipping in a draw command.  Otherwise just
+   * override onPreDraw and onDraw.
+   */
   @Override
-  public final void draw(FlatViewGroup parent, Canvas canvas) {
+  public void draw(FlatViewGroup parent, Canvas canvas) {
     onPreDraw(parent, canvas);
-    if (shouldClip()) {
+    if (mNeedsClipping && shouldClip()) {
       canvas.save(Canvas.CLIP_SAVE_FLAG);
       applyClipping(canvas);
       onDraw(canvas);
@@ -67,8 +122,11 @@ import android.graphics.Color;
   /**
    * Updates boundaries of the AbstractDrawCommand and freezes it.
    * Will return a frozen copy if the current AbstractDrawCommand cannot be mutated.
+   *
+   * This should not be called on a DrawView, as the DrawView is modified on UI thread.  Use
+   * DrawView.collectDrawView instead to avoid race conditions.
    */
-  public final AbstractDrawCommand updateBoundsAndFreeze(
+  public AbstractDrawCommand updateBoundsAndFreeze(
       float left,
       float top,
       float right,

ReactAndroid/src/main/java/com/facebook/react/flat/ClippingDrawCommandManager.java

@@ -75,25 +75,57 @@ import com.facebook.react.views.view.ReactClippingViewGroupHelper;
   public void mountViews(ViewResolver viewResolver, int[] viewsToAdd, int[] viewsToDetach) {
     for (int viewToAdd : viewsToAdd) {
       if (viewToAdd > 0) {
+        // This view was not previously attached to this parent.
         View view = viewResolver.getView(viewToAdd);
         ensureViewHasNoParent(view);
+        DrawView drawView = Assertions.assertNotNull(mDrawViewMap.get(viewToAdd));
+        drawView.mWasMounted = true;
+        if (animating(view) || withinBounds(drawView)) {
+          // View should be drawn.  This view can't currently be clipped because it wasn't
+          // previously attached to this parent.
           mFlatViewGroup.addViewInLayout(view);
         } else {
-        View view = viewResolver.getView(-viewToAdd);
+          clip(drawView.reactTag, view);
-        ensureViewHasNoParent(view);
+        }
+      } else {
+        // This view was previously attached, and just temporarily detached.
         DrawView drawView = Assertions.assertNotNull(mDrawViewMap.get(-viewToAdd));
-        if (!drawView.mPreviouslyDrawn) {
+        View view = viewResolver.getView(drawView.reactTag);
-          // The DrawView has not been drawn before, which means the bounds changed and triggered
+        ensureViewHasNoParent(view);
-          // a new DrawView when it was collected from the shadow node.  We have a view with the
+        if (drawView.mWasMounted) {
-          // same id temporarily detached, but we no longer know the bounds.
+          // The DrawView has been mounted before.
-          unclip(drawView.reactTag);
+          if (!isClipped(drawView.reactTag)) {
-          mFlatViewGroup.attachViewToParent(view);
+            // The DrawView is not clipped.  Attach it.
-        } else if (!isClipped(drawView.reactTag)) {
-          // The DrawView has been drawn before, and is not clipped.  Attach it, and it will get
-          // removed if we update the clipping rect.
             mFlatViewGroup.attachViewToParent(view);
           }
-        // The DrawView has been previously drawn and is clipped, so don't attach it.
+          // else The DrawView has been previously mounted and is clipped, so don't attach it.
+        } else {
+          // We are mounting it, so lets get this part out of the way.
+          drawView.mWasMounted = true;
+          // The DrawView has not been mounted before, which means the bounds changed and triggered
+          // a new DrawView when it was collected from the shadow node.  We have a view with the
+          // same id temporarily detached, but its bounds have changed.
+          if (animating(view) || withinBounds(drawView)) {
+            // View should be drawn.
+            if (isClipped(drawView.reactTag)) {
+              // View was clipped, so add it.
+              mFlatViewGroup.addViewInLayout(view);
+              unclip(drawView.reactTag);
+            } else {
+              // View was just temporarily removed, so attach it.  We already know it isn't clipped,
+              // so no need to unclip it.
+              mFlatViewGroup.attachViewToParent(view);
+            }
+          } else {
+            // View should be clipped.
+            if (!isClipped(drawView.reactTag)) {
+              // View was onscreen.
+              mFlatViewGroup.removeDetachedView(view);
+              clip(drawView.reactTag, view);
+            }
+            // else view is already clipped and not within bounds.
+          }
+        }
       }
     }
 
@@ -133,6 +165,15 @@ import com.facebook.react.views.view.ReactClippingViewGroupHelper;
     }
   }
 
+  // Return true if a DrawView is currently onscreen.
+  boolean withinBounds(DrawView drawView) {
+    return mClippingRect.intersects(
+        Math.round(drawView.getLeft()),
+        Math.round(drawView.getTop()),
+        Math.round(drawView.getRight()),
+        Math.round(drawView.getBottom()));
+  }
+
   @Override
   public boolean updateClippingRect() {
     ReactClippingViewGroupHelper.calculateClippingRect(mFlatViewGroup, mClippingRect);
@@ -155,7 +196,7 @@ import com.facebook.react.views.view.ReactClippingViewGroupHelper;
             // Now off the screen.  Don't invalidate in this case, as the canvas should not be
             // redrawn unless new elements are coming onscreen.
             clip(drawView.reactTag, view);
-            mFlatViewGroup.detachView(--index);
+            mFlatViewGroup.removeViewsInLayout(--index, 1);
           }
         } else {
           // Clipped, invisible. We obviously aren't animating here, as if we were then we would not
@@ -163,7 +204,7 @@ import com.facebook.react.views.view.ReactClippingViewGroupHelper;
           if (withinBounds(view)) {
             // Now on the screen.  Invalidate as we have a new element to draw.
             unclip(drawView.reactTag);
-            mFlatViewGroup.attachViewToParent(view, index++);
+            mFlatViewGroup.addViewInLayout(view, index++);
             needsInvalidate = true;
           }
         }

ReactAndroid/src/main/java/com/facebook/react/flat/DrawImageWithPipeline.java

@@ -120,10 +120,11 @@ import com.facebook.react.views.imagehelper.MultiSourceHelper.MultiSourceResult;
     mFadeDuration = fadeDuration;
   }
 
+  /**
+   * If the bitmap isn't loaded by the first draw, fade it in, otherwise don't fade.
+   */
   @Override
   protected void onPreDraw(FlatViewGroup parent, Canvas canvas) {
-    super.onPreDraw(parent, canvas);
-
     Bitmap bitmap = Assertions.assumeNotNull(mRequestHelper).getBitmap();
     if (bitmap == null) {
       mFirstDrawTime = 0;
@@ -167,7 +168,7 @@ import com.facebook.react.views.imagehelper.MultiSourceHelper.MultiSourceResult;
       PAINT.setShader(mBitmapShader);
       canvas.drawPath(getPathForRoundedBitmap(), PAINT);
     }
-
+    bitmap = null;
     drawBorders(canvas);
   }
 

ReactAndroid/src/main/java/com/facebook/react/flat/DrawView.java

@@ -11,41 +11,51 @@ package com.facebook.react.flat;
 
 import android.graphics.Canvas;
 
-/* package */ final class DrawView extends AbstractClippingDrawCommand {
+/* package */ final class DrawView extends AbstractDrawCommand {
 
   /* package */ final int reactTag;
-  // Indicates if the DrawView is frozen.  If it is frozen then any setting of the clip bounds
+  // Indicates whether this DrawView has been previously mounted to a clipping FlatViewGroup.  This
-  // should create a new DrawView.
+  // lets us know that the bounds haven't changed, as a bounds change would trigger a new DrawView,
-  private boolean mFrozen;
+  // which will set this to false for the new DrawView.  This is safe, despite the dual access with
-  // Indicates whether this DrawView has been previously drawn.  If it has been drawn, then we know
+  // FlatViewGroup, because the FlatViewGroup copy is only ever modified by the FlatViewGroup.
-  // that the bounds haven't changed, as a bounds change would trigger a new DrawView, which will
+  // Changing how this boolean is used should be handled with caution, as race conditions are the
-  // set this to false for the new DrawView.  Leaving this as package for direct access, but this
+  // quickest way to create unreproducible super bugs.
-  // should only be set from draw in DrawView, to avoid race conditions.
+  /* package */ boolean mWasMounted;
-  /* package */ boolean mPreviouslyDrawn;
 
   public DrawView(int reactTag) {
     this.reactTag = reactTag;
   }
 
+  /**
+   * Similar to updateBoundsAndFreeze, but thread safe as the mounting flag is modified on the UI
+   * thread.
+   *
+   * @return A DrawView with the passed bounds and clipping bounds.  If we can use the same
+   *     DrawView, it will just be this, otherwise it will be a frozen copy.
+   */
   public DrawView collectDrawView(
+      float left,
+      float top,
+      float right,
+      float bottom,
       float clipLeft,
       float clipTop,
       float clipRight,
       float clipBottom) {
-    if (mFrozen) {
+    DrawView drawView = (DrawView)
-      return clipBoundsMatch(clipLeft, clipTop, clipRight, clipBottom) ?
+        updateBoundsAndFreeze(left, top, right, bottom, clipLeft, clipTop, clipRight, clipBottom);
-          this :
+    if (drawView != this) {
-          new DrawView(reactTag).collectDrawView(clipLeft, clipTop, clipRight, clipBottom);
+      // It is very important that we unset this, as our spec is that newly created DrawViews are
-    } else {
+      // handled differently by the FlatViewGroup.  This is needed because updateBoundsAndFreeze
-      mFrozen = true;
+      // uses .clone(), so we maintain the previous state.
-      setClipBounds(clipLeft, clipTop, clipRight, clipBottom);
+      drawView.mWasMounted = false;
-      return this;
     }
+    return drawView;
   }
 
   @Override
   public void draw(FlatViewGroup parent, Canvas canvas) {
-    mPreviouslyDrawn = true;
+    onPreDraw(parent, canvas);
     if (mNeedsClipping) {
       canvas.save(Canvas.CLIP_SAVE_FLAG);
       applyClipping(canvas);
@@ -56,6 +66,11 @@ import android.graphics.Canvas;
     }
   }
 
+  @Override
+  protected void onDraw(Canvas canvas) {
+    // no op as we override draw.
+  }
+
   @Override
   public void debugDraw(FlatViewGroup parent, Canvas canvas) {
     parent.debugDrawNextChild(canvas);

ReactAndroid/src/main/java/com/facebook/react/flat/FlatShadowNode.java

@@ -468,17 +468,32 @@ import com.facebook.react.views.view.ReactClippingViewGroupHelper;
     }
   }
 
-  /* package */ final DrawView collectDrawView(float left, float top, float right, float bottom) {
+  /* package */ final DrawView collectDrawView(
+      float left,
+      float top,
+      float right,
+      float bottom,
+      float clipLeft,
+      float clipTop,
+      float clipRight,
+      float clipBottom) {
     Assertions.assumeNotNull(mDrawView);
     if (mDrawView.reactTag == 0) {
       // This is the first time we have collected this DrawView, but we have to create a new
-      // DrawView anyway, as reactTag is final.
+      // DrawView anyway, as reactTag is final, and our DrawView instance is the static copy.
-      mDrawView = new DrawView(getReactTag()).collectDrawView(left, top, right, bottom);
+      mDrawView = new DrawView(getReactTag());
-    } else {
-      // We have collected the DrawView before, so the react tag is correct, but we may need a new
-      // copy with updated bounds.  If the bounds match, the same view is returned.
-      mDrawView = mDrawView.collectDrawView(left, top, right, bottom);
     }
+    // We have the correct react tag, but we may need a new copy with updated bounds.  If the bounds
+    // match or were never set, the same view is returned.
+    mDrawView = mDrawView.collectDrawView(
+        left,
+        top,
+        right,
+        bottom,
+        clipLeft,
+        clipTop,
+        clipRight,
+        clipBottom);
     return mDrawView;
   }
 

ReactAndroid/src/main/java/com/facebook/react/flat/FlatViewGroup.java

@@ -401,11 +401,7 @@ import com.facebook.react.views.view.ReactClippingViewGroup;
   @Override
   protected void onDetachedFromWindow() {
     if (!mIsAttached) {
-      // Hack.  Our current behaviour of add then immediately remove if a view is clipped pretty
+      throw new RuntimeException("Double detach");
-      // much guarantees that we kill network requests for images in feed.  We have a fix, but are
-      // going to add it in master and patch this in in the meantime.
-      return;
-      // throw new RuntimeException("Double detach");
     }
 
     mIsAttached = false;
@@ -713,12 +709,12 @@ import com.facebook.react.views.view.ReactClippingViewGroup;
     addViewInLayout(view, -1, ensureLayoutParams(view.getLayoutParams()), true);
   }
 
-  /* package */ void attachViewToParent(View view) {
+  /* package */ void addViewInLayout(View view, int index) {
-    attachViewToParent(view, -1, ensureLayoutParams(view.getLayoutParams()));
+    addViewInLayout(view, index, ensureLayoutParams(view.getLayoutParams()), true);
   }
 
-  /* package */ void attachViewToParent(View view, int index) {
+  /* package */ void attachViewToParent(View view) {
-    attachViewToParent(view, index, ensureLayoutParams(view.getLayoutParams()));
+    attachViewToParent(view, -1, ensureLayoutParams(view.getLayoutParams()));
   }
 
   private void processLayoutRequest() {
@@ -756,6 +752,8 @@ import com.facebook.react.views.view.ReactClippingViewGroup;
     int right = Integer.MIN_VALUE;
     int bottom = Integer.MIN_VALUE;
     for (int i = 0; i < childCount; i++) {
+      // This is technically a dupe, since the DrawView has its bounds, but leaving in to handle if
+      // the View is animating or rebelling against the DrawView bounds for some reason.
       View child = getChildAt(i);
       left = Math.min(left, child.getLeft());
       top = Math.min(top, child.getTop());

ReactAndroid/src/main/java/com/facebook/react/flat/StateBuilder.java

@@ -536,6 +536,10 @@ import com.facebook.react.uimanager.events.EventDispatcher;
       addNativeChild(node);
       if (!parentIsAndroidView) {
         mDrawCommands.add(node.collectDrawView(
+            left,
+            top,
+            right,
+            bottom,
             parentClipLeft,
             parentClipTop,
             parentClipRight,
@@ -544,8 +548,8 @@ import com.facebook.react.uimanager.events.EventDispatcher;
 
       updated = collectStateForMountableNode(
           node,
-          left - left,
+          0, // left - left
-          top - top,
+          0, // top - top
           right - left,
           bottom - top,
           parentClipLeft - left,