Backed out changeset c5c63ad85aa6 (bug 1118105)
authorCarsten "Tomcat" Book <cbook@mozilla.com>
Fri, 09 Jan 2015 16:22:59 +0100
changeset 248847 98ca4954995fe3d7c997ea05c925593746ba338c
parent 248846 3282a388ad815c72e326b80ca2b978f7a524ce86
child 248848 5560d234666e79fa7f080781fa7c48b0951b8770
push id4489
push userraliiev@mozilla.com
push dateMon, 23 Feb 2015 15:17:55 +0000
treeherdermozilla-beta@fd7c3dc24146 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1118105
milestone37.0a1
backs outc5c63ad85aa6e3a568bf9d4eaae6e254942a3d29
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
Backed out changeset c5c63ad85aa6 (bug 1118105)
image/src/Decoder.cpp
image/src/SurfaceCache.cpp
image/src/SurfaceCache.h
--- a/image/src/Decoder.cpp
+++ b/image/src/Decoder.cpp
@@ -241,23 +241,23 @@ Decoder::AllocateFrameInternal(uint32_t 
   }
 
   RawAccessFrameRef ref = frame->RawAccessRef();
   if (!ref) {
     frame->Abort();
     return RawAccessFrameRef();
   }
 
-  InsertOutcome outcome =
+  bool succeeded =
     SurfaceCache::Insert(frame, ImageKey(&mImage),
                          RasterSurfaceKey(imageSize.ToIntSize(),
                                           aDecodeFlags,
                                           aFrameNum),
                          Lifetime::Persistent);
-  if (outcome != InsertOutcome::SUCCESS) {
+  if (!succeeded) {
     ref->Abort();
     return RawAccessFrameRef();
   }
 
   nsIntRect refreshArea;
 
   if (aFrameNum == 1) {
     MOZ_ASSERT(aPreviousFrame, "Must provide a previous frame when animated");
--- a/image/src/SurfaceCache.cpp
+++ b/image/src/SurfaceCache.cpp
@@ -301,32 +301,29 @@ private:
     UnregisterWeakMemoryReporter(this);
   }
 
 public:
   void InitMemoryReporter() { RegisterWeakMemoryReporter(this); }
 
   Mutex& GetMutex() { return mMutex; }
 
-  InsertOutcome Insert(imgFrame*         aSurface,
-                       const Cost        aCost,
-                       const ImageKey    aImageKey,
-                       const SurfaceKey& aSurfaceKey,
-                       Lifetime          aLifetime)
+  bool Insert(imgFrame*         aSurface,
+              const Cost        aCost,
+              const ImageKey    aImageKey,
+              const SurfaceKey& aSurfaceKey,
+              Lifetime          aLifetime)
   {
-    // If this is a duplicate surface, refuse to replace the original.
-    if (MOZ_UNLIKELY(Lookup(aImageKey, aSurfaceKey))) {
-      return InsertOutcome::FAILURE_ALREADY_PRESENT;
-    }
+    MOZ_ASSERT(!Lookup(aImageKey, aSurfaceKey),
+               "Inserting a duplicate surface into the SurfaceCache");
 
     // If this is bigger than we can hold after discarding everything we can,
     // refuse to cache it.
-    if (MOZ_UNLIKELY(!CanHoldAfterDiscarding(aCost))) {
-      return InsertOutcome::FAILURE;
-    }
+    if (!CanHoldAfterDiscarding(aCost))
+      return false;
 
     // Remove elements in order of cost until we can fit this in the cache. Note
     // that locked surfaces aren't in mCosts, so we never remove them here.
     while (aCost > mAvailableCost) {
       MOZ_ASSERT(!mCosts.IsEmpty(), "Removed everything and it still won't fit");
       Remove(mCosts.LastElement().GetSurface());
     }
 
@@ -341,26 +338,26 @@ public:
     nsRefPtr<CachedSurface> surface =
       new CachedSurface(aSurface, aCost, aImageKey, aSurfaceKey, aLifetime);
 
     // We require that locking succeed if the image is locked and the surface is
     // persistent; the caller may need to know this to handle errors correctly.
     if (cache->IsLocked() && aLifetime == Lifetime::Persistent) {
       surface->SetLocked(true);
       if (!surface->IsLocked()) {
-        return InsertOutcome::FAILURE;
+        return false;
       }
     }
 
     // Insert.
     MOZ_ASSERT(aCost <= mAvailableCost, "Inserting despite too large a cost");
     cache->Insert(aSurfaceKey, surface);
     StartTracking(surface);
 
-    return InsertOutcome::SUCCESS;
+    return true;
   }
 
   void Remove(CachedSurface* aSurface)
   {
     MOZ_ASSERT(aSurface, "Should have a surface");
     ImageKey imageKey = aSurface->GetImageKey();
 
     nsRefPtr<ImageSurfaceCache> cache = GetImageCache(imageKey);
@@ -796,24 +793,24 @@ SurfaceCache::Lookup(const ImageKey    a
   if (!sInstance) {
     return DrawableFrameRef();
   }
 
   MutexAutoLock lock(sInstance->GetMutex());
   return sInstance->Lookup(aImageKey, aSurfaceKey);
 }
 
-/* static */ InsertOutcome
+/* static */ bool
 SurfaceCache::Insert(imgFrame*         aSurface,
                      const ImageKey    aImageKey,
                      const SurfaceKey& aSurfaceKey,
                      Lifetime          aLifetime)
 {
   if (!sInstance) {
-    return InsertOutcome::FAILURE;
+    return false;
   }
 
   MutexAutoLock lock(sInstance->GetMutex());
   Cost cost = ComputeCost(aSurfaceKey.Size());
   return sInstance->Insert(aSurface, cost, aImageKey, aSurfaceKey, aLifetime);
 }
 
 /* static */ bool
--- a/image/src/SurfaceCache.h
+++ b/image/src/SurfaceCache.h
@@ -109,22 +109,16 @@ VectorSurfaceKey(const gfx::IntSize& aSi
   return SurfaceKey(aSize, aSVGContext, aAnimationTime, 0);
 }
 
 MOZ_BEGIN_ENUM_CLASS(Lifetime, uint8_t)
   Transient,
   Persistent
 MOZ_END_ENUM_CLASS(Lifetime)
 
-MOZ_BEGIN_ENUM_CLASS(InsertOutcome, uint8_t)
-  SUCCESS,                 // Success (but see Insert documentation).
-  FAILURE,                 // Couldn't insert (e.g., for capacity reasons).
-  FAILURE_ALREADY_PRESENT  // A surface with the same key is already present.
-MOZ_END_ENUM_CLASS(InsertOutcome)
-
 /**
  * SurfaceCache is an imagelib-global service that allows caching of temporary
  * surfaces. Surfaces normally expire from the cache automatically if they go
  * too long without being accessed.
  *
  * SurfaceCache does not hold surfaces directly; instead, it holds imgFrame
  * objects, which hold surfaces but also layer on additional features specific
  * to imagelib's needs like animation, padding support, and transparent support
@@ -174,57 +168,53 @@ struct SurfaceCache
    *
    * @return a DrawableFrameRef to the imgFrame wrapping the requested surface,
    *         or an empty DrawableFrameRef if not found.
    */
   static DrawableFrameRef Lookup(const ImageKey    aImageKey,
                                  const SurfaceKey& aSurfaceKey);
 
   /**
-   * Insert a surface into the cache. If a surface with the same ImageKey and
-   * SurfaceKey is already in the cache, Insert returns FAILURE_ALREADY_PRESENT.
+   * Insert a surface into the cache. It is an error to call this function
+   * without first calling Lookup to verify that the surface is not already in
+   * the cache.
    *
    * Each surface in the cache has a lifetime, either Transient or Persistent.
    * Transient surfaces can expire from the cache at any time. Persistent
    * surfaces can ordinarily also expire from the cache at any time, but if the
    * image they're associated with is locked, then these surfaces will never
    * expire. This means that surfaces which cannot be rematerialized should be
    * inserted with a persistent lifetime *after* the image is locked with
    * LockImage(); if you use the other order, the surfaces might expire before
    * LockImage() gets called.
    *
    * If a surface cannot be rematerialized, it may be important to know whether
-   * it was inserted into the cache successfully. Insert() returns FAILURE if it
+   * it was inserted into the cache successfully. Insert() returns false if it
    * failed to insert the surface, which could happen because of capacity
    * reasons, or because it was already freed by the OS. If you aren't inserting
    * a surface with persistent lifetime, or if the surface isn't associated with
-   * a locked image, checking for SUCCESS or FAILURE is useless: the surface
-   * might expire immediately after being inserted, even though Insert()
-   * returned SUCCESS. Thus, many callers do not need to check the result of
-   * Insert() at all.
+   * a locked image, the return value is useless: the surface might expire
+   * immediately after being inserted, even though Insert() returned true. Thus,
+   * most callers do not need to check the return value.
    *
    * @param aTarget      The new surface (wrapped in an imgFrame) to insert into
    *                     the cache.
    * @param aImageKey    Key data identifying which image the surface belongs to.
    * @param aSurfaceKey  Key data which uniquely identifies the requested surface.
    * @param aLifetime    Whether this is a transient surface that can always be
    *                     allowed to expire, or a persistent surface that
    *                     shouldn't expire if the image is locked.
-   * @return SUCCESS if the surface was inserted successfully. (But see above
-   *           for more information about when you should check this.)
-   *         FAILURE if the surface could not be inserted, e.g. for capacity
-   *           reasons. (But see above for more information about when you
-   *           should check this.)
-   *         FAILURE_ALREADY_PRESENT if a surface with the same ImageKey and
-   *           SurfaceKey already exists in the cache.
+   * @return false if the surface could not be inserted. Only check this if
+   *         inserting a persistent surface associated with a locked image (see
+   *         above for more information).
    */
-  static InsertOutcome Insert(imgFrame*         aSurface,
-                              const ImageKey    aImageKey,
-                              const SurfaceKey& aSurfaceKey,
-                              Lifetime          aLifetime);
+  static bool Insert(imgFrame*         aSurface,
+                     const ImageKey    aImageKey,
+                     const SurfaceKey& aSurfaceKey,
+                     Lifetime          aLifetime);
 
   /**
    * Checks if a surface of a given size could possibly be stored in the cache.
    * If CanHold() returns false, Insert() will always fail to insert the
    * surface, but the inverse is not true: Insert() may take more information
    * into account than just image size when deciding whether to cache the
    * surface, so Insert() may still fail even if CanHold() returns true.
    *