Closed Bug 1139641 Opened 5 years ago Closed 5 years ago

Return more information from SurfaceCache::Lookup and SurfaceCache::LookupBestMatch

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: seth, Assigned: seth)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
Blocks: 1169060
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.
Attachment #8613822 - Flags: review?(dholbert)
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.)
Attachment #8613822 - Flags: review?(dholbert) → review+
Thanks for the review! I'll update the patch with those nits addressed.
Attachment #8613822 - Attachment is obsolete: true
sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=10388308&repo=mozilla-inbound
Flags: needinfo?(seth)
^ There's a Windows-specific build failure, and I didn't include Windows in my try job. Will investigate.
Blocks: 1176124
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.
Attachment #8614524 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/d0c733dc7c9f
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Target Milestone: mozilla41 → mozilla42
Flags: needinfo?(seth)
You need to log in before you can comment on or make changes to this bug.