Bug 1341275: Add mShouldShowImage flag to FaviconView. r=sebastian draft
authorMichael Comella <michael.l.comella@gmail.com>
Fri, 14 Jul 2017 15:44:21 -0700
changeset 609851 2511fde68599e5a1ae2096ee186704e064426754
parent 609850 7a6f5bee52c39d72410ab7976ddcc53c96b17179
child 609897 67c86ec7135f293de3ca742674ddcdb60987a4f5
push id68705
push usermichael.l.comella@gmail.com
push dateMon, 17 Jul 2017 15:48:39 +0000
reviewerssebastian
bugs1341275
milestone56.0a1
Bug 1341275: Add mShouldShowImage flag to FaviconView. r=sebastian I verified this fixed the issue by: - Attaching a debugger and checking against the STR call order (comment 3). - Ensuring the favicons in the SearchEngineRow looked correct MozReview-Commit-ID: GJeh3FLim36
mobile/android/base/java/org/mozilla/gecko/widget/FaviconView.java
--- a/mobile/android/base/java/org/mozilla/gecko/widget/FaviconView.java
+++ b/mobile/android/base/java/org/mozilla/gecko/widget/FaviconView.java
@@ -31,16 +31,33 @@ import java.lang.ref.WeakReference;
  * Displays solid colour background around Favicon to fill space not occupied by the icon. Colour
  * selected is the dominant colour of the provided Favicon.
  */
 public class FaviconView extends ImageView {
 
     // Default x/y-radius of the oval used to round the corners of the background (dp)
     private static final int DEFAULT_CORNER_RADIUS_DP = 2;
 
+    /**
+     * True if we're capable of, and want to, display an image, false otherwise. This acts as a switch: if we
+     * don't want to show an image, most other state will be ignored because we don't need it to draw.
+     *
+     * We use this flag, rather than zeroing all relevant state, because if update*Image is called before
+     * onSizeChanged (during init), we would not be able to show an image and thus zero all state. When
+     * onSizeChanged is called, the state has been zeroed and we would not be able to show the image properly
+     * (bug 1341275).
+     *
+     * One notable exception is for ImageView's internal state via setImageBitmap/Drawable, which must be set to
+     * null if we don't want to draw anything.
+     *
+     * By default, we can't show an image because we don't have a size yet (onSizeChanged) or an image (update*Image),
+     * hence false.
+     */
+    private boolean mShouldShowImage = false;
+
     private Bitmap mIconBitmap;
 
     // Reference to the unscaled bitmap, if any, to prevent repeated assignments of the same bitmap
     // to the view from causing repeated rescalings (Some of the callers do this)
     private Bitmap mUnscaledBitmap;
 
     // Flag indicating if the most recently assigned image is considered likely to need scaling.
     private boolean mScalingExpected;
@@ -105,41 +122,45 @@ public class FaviconView extends ImageVi
         mBackgroundRect.right = w;
         mBackgroundRect.bottom = h;
 
         formatImage();
     }
 
     @Override
     public void onDraw(Canvas canvas) {
-        if (isDominantBorderEnabled) {
+        if (mShouldShowImage && isDominantBorderEnabled) {
             sBackgroundPaint.setColor(mDominantColor);
 
             if (areRoundCornersEnabled) {
                 canvas.drawRoundRect(mBackgroundRect, mBackgroundCornerRadius, mBackgroundCornerRadius, sBackgroundPaint);
             } else {
                 canvas.drawRect(mBackgroundRect, sBackgroundPaint);
             }
         }
 
+        // If mShouldShowImage == false, setImageDrawable should be null & we are safe to call super.
         super.onDraw(canvas);
     }
 
     /**
      * Formats the image for display, if the prerequisite data are available. Upscales tiny Favicons to
      * normal sized ones, replaces null bitmaps with the default Favicon, and fills all remaining space
      * in this view with the coloured background.
      */
     private void formatImage() {
-        // We're waiting for both onSizeChanged and updateImage to be called before scaling.
-        if (mIconBitmap == null || getWidth() == 0 || getHeight() == 0) {
+        // Both onSizeChanged and updateImage have to be called before an image can be shown.
+        final boolean canImageBeShown = (mIconBitmap != null && getWidth() != 0 && getHeight() != 0);
+        if (!canImageBeShown) {
             showNoImage();
             return;
         }
 
+        mShouldShowImage = true;
+
         if (mScalingExpected && getWidth() != mIconBitmap.getWidth()) {
             scaleBitmap();
             // Don't scale the image every time something changes.
             mScalingExpected = false;
         }
 
         // In original, there is no round corners if FaviconView has bitmap icon. But the new design
         // needs round corners all the time, so we use RoundedBitmapDrawableFactory to create round corners.
@@ -200,18 +221,18 @@ public class FaviconView extends ImageVi
         mDominantColor = response.getColor();
         mScalingExpected = allowScaling;
 
         // Possibly update the display.
         formatImage();
     }
 
     private void showNoImage() {
+        mShouldShowImage = false;
         setImageDrawable(null);
-        mDominantColor = Color.TRANSPARENT;
     }
 
     /**
      * Clear image and background shown by this view.
      */
     public void clearImage() {
         showNoImage();
         mUnscaledBitmap = null;