Right now, the SurfaceCache lookup functions return a DrawableFrameRef. We should start returning more information, though - this way we can help callers make more intelligent decisions. In this bug, I plan to add a type that contains a DrawableFrameRef, but can also hold additional fields. We'll add more fields later, but for now we'll only return one piece of information: whether a substitution has occurred. For SurfaceCache::Lookup, this will always be false, but for SurfaceCache::LookupBestMatch, this will indicate whether the exact surface requested by the caller is being returned or something different. This will in turn allow us to simplify code where we need to check for this.

Here's the patch. No functionality changes; this is pure refactoring. The closest thing to a functionality change is that the determination of whether LookupBestMatch returned an exact match is now done in the SurfaceCache code, instead of externally in RasterImage. This isn't necessarily that interesting since it's just a comparison of sizes, but it lets us add a nice assertion. The real point of this patch is to provide a framework for returning more metadata, which is something that we'll have a number of uses for in the near future. A note about one oddity: the LookupResult type needed its own header because SurfaceCache.h is visible outside ImageLib, but imgFrame.h (which defines DrawableFrameRef) is *not*, and I don't want to change that. Hence I couldn't define a type which included a DrawableFrameRef member variable directly in SurfaceCache.h.

Comment on attachment 8613822 [details] [diff] [review] Return more information from SurfaceCache::Lookup and SurfaceCache::LookupBestMatch Review of attachment 8613822 [details] [diff] [review]: ----------------------------------------------------------------- r=me, nits below; address as you see fit: ::: image/LookupResult.h @@ +11,5 @@ > +#ifndef mozilla_image_LookupResult_h > +#define mozilla_image_LookupResult_h > + > +#include "mozilla/Attributes.h" > +#include "imgFrame.h" Probably worth #including mozilla/Move.h here, too, since you use Move() in this file. (We're already getting Move.h indirectly via imgFrame.h, but it's better not to depend on that.) ::: image/RasterImage.cpp @@ +2168,1 @@ > return destSize; // We have an existing HQ scale for this size. Looks like this contextual line (3rd return statement in RasterImage::OptimalImageSizeForDest) is overly-indented. Could you fix that while you're here? (in the same patch or in a whitespace-only helper patch) ::: image/SurfaceCache.h @@ +196,2 @@ > */ > + static LookupResult Lookup(const ImageKey aImageKey, The general documentation for this method still has a sentence saying "an empty DrawableFrameRef is returned" -- I think that might want to be updated to say "a LookupResult with an empty DrawableFrameRef is returned", perhaps? (At least, you made a change along those lines to another chunk of documentation in FrameAnimator.h.)

Thanks for the review! I'll update the patch with those nits addressed.

Addressed nits.

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=10388308&repo=mozilla-inbound

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/9ad61b614c2d

^ There's a Windows-specific build failure, and I didn't include Windows in my try job. Will investigate.

OK, it seems that to make MSVC happy I need to manually declare a move constructor and move assignment operator and manually delete the copy constructor for LookupResult. Based on the try job above, that fixes the build on Windows.