Backed out changeset 588c44c7a966 (bug 1058040)
authorIris Hsiao <ihsiao@mozilla.com>
Thu, 02 Mar 2017 11:24:33 +0800
changeset 374581 8c7dbec36fa77c5bb427c480f3620dd4a781260f
parent 374580 296647884376857ff7cce38795606184fb806fd0
child 374582 66535e831760421b270662aa8d0773b0fde7c9f3
child 374611 5e3c5f0f53568d7aefdc67e21d69ed7d11b141ce
push id10863
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 23:02:23 +0000
treeherdermozilla-aurora@0931190cd725 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1058040
milestone54.0a1
backs out588c44c7a96616073364b7b10324f57604ef72f9
Backed out changeset 588c44c7a966 (bug 1058040) CLOSED TREE
dom/canvas/CanvasRenderingContext2D.cpp
image/ClippedImage.cpp
image/OrientedImage.cpp
image/VectorImage.cpp
layout/base/nsLayoutUtils.cpp
layout/svg/SVGImageContext.h
layout/svg/nsSVGImageFrame.cpp
--- a/dom/canvas/CanvasRenderingContext2D.cpp
+++ b/dom/canvas/CanvasRenderingContext2D.cpp
@@ -5101,17 +5101,17 @@ CanvasRenderingContext2D::DrawDirectlyTo
                        Scale(1.0 / contextScale.width,
                              1.0 / contextScale.height).
                        Translate(aDest.x - aSrc.x, aDest.y - aSrc.y));
 
   // FLAG_CLAMP is added for increased performance, since we never tile here.
   uint32_t modifiedFlags = aImage.mDrawingFlags | imgIContainer::FLAG_CLAMP;
 
   CSSIntSize sz(scaledImageSize.width, scaledImageSize.height); // XXX hmm is scaledImageSize really in CSS pixels?
-  SVGImageContext svgContext(Some(sz), Nothing(), CurrentState().globalAlpha);
+  SVGImageContext svgContext(sz, Nothing(), CurrentState().globalAlpha);
 
   auto result = aImage.mImgContainer->
     Draw(context, scaledImageSize,
          ImageRegion::Create(gfxRect(aSrc.x, aSrc.y, aSrc.width, aSrc.height)),
          aImage.mWhichFrame, SamplingFilter::GOOD, Some(svgContext), modifiedFlags, 1.0);
 
   if (result != DrawResult::SUCCESS) {
     NS_WARNING("imgIContainer::Draw failed");
--- a/image/ClippedImage.cpp
+++ b/image/ClippedImage.cpp
@@ -462,26 +462,23 @@ ClippedImage::DrawSingleTile(gfxContext*
   aContext->Multiply(gfxMatrix::Translation(-clip.x, -clip.y));
 
   auto unclipViewport = [&](const SVGImageContext& aOldContext) {
     // Map the viewport to the inner image. Note that we don't take the aSize
     // parameter of imgIContainer::Draw into account, just the clipping region.
     // The size in pixels at which the output will ultimately be drawn is
     // irrelevant here since the purpose of the SVG viewport size is to
     // determine what *region* of the SVG document will be drawn.
+    CSSIntSize vSize(aOldContext.GetViewportSize());
+    vSize.width = ceil(vSize.width * double(innerSize.width) / mClip.width);
+    vSize.height =
+      ceil(vSize.height * double(innerSize.height) / mClip.height);
+
     SVGImageContext context(aOldContext);
-    auto oldViewport = aOldContext.GetViewportSize();
-    if (oldViewport) {
-      CSSIntSize newViewport;
-      newViewport.width =
-        ceil(oldViewport->width * double(innerSize.width) / mClip.width);
-      newViewport.height =
-        ceil(oldViewport->height * double(innerSize.height) / mClip.height);
-      context.SetViewportSize(Some(newViewport));
-    }
+    context.SetViewportSize(vSize);
     return context;
   };
 
   return InnerImage()->Draw(aContext, size, region,
                             aWhichFrame, aSamplingFilter,
                             aSVGContext.map(unclipViewport),
                             aFlags, aOpacity);
 }
--- a/image/OrientedImage.cpp
+++ b/image/OrientedImage.cpp
@@ -288,23 +288,22 @@ OrientedImage::Draw(gfxContext* aContext
   // The region is already in the orientation expected by the caller, but we
   // need it to be in the image's coordinate system, so we transform it using
   // the inverse of the orientation matrix.
   gfxMatrix inverseMatrix(OrientationMatrix(size, /* aInvert = */ true));
   ImageRegion region(aRegion);
   region.TransformBoundsBy(inverseMatrix);
 
   auto orientViewport = [&](const SVGImageContext& aOldContext) {
+    CSSIntSize viewportSize(aOldContext.GetViewportSize());
+    if (mOrientation.SwapsWidthAndHeight()) {
+      swap(viewportSize.width, viewportSize.height);
+    }
     SVGImageContext context(aOldContext);
-    auto oldViewport = aOldContext.GetViewportSize();
-    if (oldViewport && mOrientation.SwapsWidthAndHeight()) {
-      // Swap width and height:
-      CSSIntSize newViewport(oldViewport->height, oldViewport->width);
-      context.SetViewportSize(Some(newViewport));
-    }
+    context.SetViewportSize(viewportSize);
     return context;
   };
 
   return InnerImage()->Draw(aContext, size, region, aWhichFrame,
                             aSamplingFilter,
                             aSVGContext.map(orientViewport), aFlags,
                             aOpacity);
 }
--- a/image/VectorImage.cpp
+++ b/image/VectorImage.cpp
@@ -784,20 +784,18 @@ struct SVGDrawingParameters
     , samplingFilter(aSamplingFilter)
     , svgContext(aSVGContext)
     , viewportSize(aSize)
     , animationTime(aAnimationTime)
     , flags(aFlags)
     , opacity(aSVGContext ? aSVGContext->GetGlobalOpacity() : aOpacity)
   {
     if (aSVGContext) {
-      auto sz = aSVGContext->GetViewportSize();
-      if (sz) {
-        viewportSize = nsIntSize(sz->width, sz->height); // XXX losing unit
-      }
+      CSSIntSize sz = aSVGContext->GetViewportSize();
+      viewportSize = nsIntSize(sz.width, sz.height); // XXX losing unit
     }
   }
 
   gfxContext*                   context;
   IntSize                       size;
   ImageRegion                   region;
   SamplingFilter                samplingFilter;
   const Maybe<SVGImageContext>& svgContext;
@@ -851,17 +849,17 @@ VectorImage::Draw(gfxContext* aContext,
   // overwrite SVG preserveAspectRatio attibute of this image with none, and
   // always stretch this image to viewport non-uniformly.
   // And we can do this only if the caller pass in the the SVG viewport, via
   // aSVGContext.
   if ((aFlags & FLAG_FORCE_PRESERVEASPECTRATIO_NONE) && aSVGContext.isSome()) {
     Maybe<SVGPreserveAspectRatio> aspectRatio =
       Some(SVGPreserveAspectRatio(SVG_PRESERVEASPECTRATIO_NONE,
                                   SVG_MEETORSLICE_UNKNOWN));
-    svgContext = aSVGContext; // copy
+    svgContext = Some(SVGImageContext(*aSVGContext)); // copy
     svgContext->SetPreserveAspectRatio(aspectRatio);
   } else {
     svgContext = aSVGContext;
   }
 
   float animTime =
     (aWhichFrame == FRAME_FIRST) ? 0.0f
                                  : mSVGDocumentWrapper->GetCurrentTime();
--- a/layout/base/nsLayoutUtils.cpp
+++ b/layout/base/nsLayoutUtils.cpp
@@ -6535,17 +6535,17 @@ DrawImageInternal(gfxContext&           
 
     RefPtr<gfxContext> destCtx = &aContext;
 
     destCtx->SetMatrix(params.imageSpaceToDeviceSpace);
 
     Maybe<const SVGImageContext> fallbackContext;
     if (!aSVGContext) {
       // Use the default viewport.
-      fallbackContext.emplace(Some(params.svgViewportSize));
+      fallbackContext.emplace(params.svgViewportSize);
     }
 
     result = aImage->Draw(destCtx, params.size, params.region,
                           imgIContainer::FRAME_CURRENT, aSamplingFilter,
                           aSVGContext ? aSVGContext : fallbackContext,
                           aImageFlags, aOpacity);
 
   }
@@ -6720,17 +6720,17 @@ nsLayoutUtils::DrawBackgroundImage(gfxCo
                                    const nsRect&       aDirty,
                                    uint32_t            aImageFlags,
                                    ExtendMode          aExtendMode,
                                    float               aOpacity)
 {
   PROFILER_LABEL("layout", "nsLayoutUtils::DrawBackgroundImage",
                  js::ProfileEntry::Category::GRAPHICS);
 
-  const Maybe<SVGImageContext> svgContext(Some(SVGImageContext(Some(aImageSize))));
+  const Maybe<const SVGImageContext> svgContext(Some(SVGImageContext(aImageSize)));
 
   /* Fast path when there is no need for image spacing */
   if (aRepeatSize.width == aDest.width && aRepeatSize.height == aDest.height) {
     return DrawImageInternal(aContext, aPresContext, aImage,
                              aSamplingFilter, aDest, aFill, aAnchor,
                              aDirty, svgContext, aImageFlags, aExtendMode,
                              aOpacity);
   }
--- a/layout/svg/SVGImageContext.h
+++ b/layout/svg/SVGImageContext.h
@@ -22,52 +22,35 @@ namespace mozilla {
 // to the image's internal SVG document when it's drawn.
 class SVGImageContext
 {
 public:
   SVGImageContext()
     : mGlobalOpacity(1.0)
   { }
 
-  /**
-   * Currently it seems that the aViewportSize parameter ends up being used
-   * for different things by different pieces of code, and probably in some
-   * cases being used incorrectly (specifically in the case of pixel snapping
-   * under the nsLayoutUtils::Draw*Image() methods).  An unfortunate result of
-   * the messy code is that aViewportSize is currently a Maybe<T> since it
-   * is difficult to create a utility function that consumers can use up
-   * front to get the "correct" viewport size (i.e. which for compatibility
-   * with the current code (bugs and all) would mean the size including all
-   * the snapping and resizing magic that happens in various places under the
-   * nsLayoutUtils::Draw*Image() methods on the way to DrawImageInternal
-   * creating |fallbackContext|).  Using Maybe<T> allows code to pass Nothing()
-   * in order to get the size that's created for |fallbackContext|.  At some
-   * point we need to clean this code up, make our abstractions clear, create
-   * that utility and stop using Maybe for this parameter.
-   *
-   * Note: 'aIsPaintingSVGImageElement' should be used to indicate whether
-   * the SVG image in question is being painted for an SVG <image> element.
-   */
-  explicit SVGImageContext(const Maybe<CSSIntSize>& aViewportSize,
+  // Note: 'aIsPaintingSVGImageElement' should be used to indicate whether
+  // the SVG image in question is being painted for an SVG <image> element.
+  explicit SVGImageContext(const CSSIntSize& aViewportSize,
                            const Maybe<SVGPreserveAspectRatio>& aPreserveAspectRatio = Nothing(),
                            gfxFloat aOpacity = 1.0,
                            bool aIsPaintingSVGImageElement = false)
     : mViewportSize(aViewportSize)
     , mPreserveAspectRatio(aPreserveAspectRatio)
     , mGlobalOpacity(aOpacity)
     , mIsPaintingSVGImageElement(aIsPaintingSVGImageElement)
   { }
 
   bool MaybeStoreContextPaint(nsIFrame* aFromFrame);
 
-  const Maybe<CSSIntSize>& GetViewportSize() const {
+  const CSSIntSize& GetViewportSize() const {
     return mViewportSize;
   }
 
-  void SetViewportSize(const Maybe<CSSIntSize>& aSize) {
+  void SetViewportSize(const CSSIntSize& aSize) {
     mViewportSize = aSize;
   }
 
   const Maybe<SVGPreserveAspectRatio>& GetPreserveAspectRatio() const {
     return mPreserveAspectRatio;
   }
 
   void SetPreserveAspectRatio(const Maybe<SVGPreserveAspectRatio>& aPAR) {
@@ -98,33 +81,31 @@ public:
   }
 
   uint32_t Hash() const {
     uint32_t hash = 0;
     if (mContextPaint) {
       hash = HashGeneric(hash, mContextPaint->Hash());
     }
     return HashGeneric(hash,
-                       mViewportSize.map(HashSize).valueOr(0),
+                       mViewportSize.width,
+                       mViewportSize.height,
                        mPreserveAspectRatio.map(HashPAR).valueOr(0),
                        HashBytes(&mGlobalOpacity, sizeof(mGlobalOpacity)),
                        mIsPaintingSVGImageElement);
   }
 
 private:
-  static uint32_t HashSize(const CSSIntSize& aSize) {
-    return HashGeneric(aSize.width, aSize.height);
-  }
   static uint32_t HashPAR(const SVGPreserveAspectRatio& aPAR) {
     return aPAR.Hash();
   }
 
   // NOTE: When adding new member-vars, remember to update Hash() & operator==.
   RefPtr<SVGContextPaint>       mContextPaint;
-  Maybe<CSSIntSize>             mViewportSize;
+  CSSIntSize                    mViewportSize;
   Maybe<SVGPreserveAspectRatio> mPreserveAspectRatio;
   gfxFloat                      mGlobalOpacity;
   bool                          mIsPaintingSVGImageElement;
 };
 
 } // namespace mozilla
 
 #endif // MOZILLA_SVGCONTEXT_H_
--- a/layout/svg/nsSVGImageFrame.cpp
+++ b/layout/svg/nsSVGImageFrame.cpp
@@ -394,17 +394,17 @@ nsSVGImageFrame::PaintSVG(gfxContext& aC
     if (mImageContainer->GetType() == imgIContainer::TYPE_VECTOR) {
       // Package up the attributes of this image element which can override the
       // attributes of mImageContainer's internal SVG document.  The 'width' &
       // 'height' values we're passing in here are in CSS units (though they
       // come from width/height *attributes* in SVG). They influence the region
       // of the SVG image's internal document that is visible, in combination
       // with preserveAspectRatio and viewBox.
       const Maybe<const SVGImageContext> context(
-        Some(SVGImageContext(Some(CSSIntSize::Truncate(width, height)),
+        Some(SVGImageContext(CSSIntSize::Truncate(width, height),
                              Some(imgElem->mPreserveAspectRatio.GetAnimValue()),
                              1.0, /* aIsPaintingSVGImageElement */ true)));
 
       // For the actual draw operation to draw crisply (and at the right size),
       // our destination rect needs to be |width|x|height|, *in dev pixels*.
       LayoutDeviceSize devPxSize(width, height);
       nsRect destRect(nsPoint(),
                       LayoutDevicePixel::ToAppUnits(devPxSize,