Closed Bug 1296762 Opened 4 years ago Closed 4 years ago

Remove SurfaceCache::InsertPlaceholder() and eliminate as much nullability from SurfaceCache as possible


(Core :: ImageLib, defect)

Not set



Tracking Status
firefox51 --- fixed


(Reporter: seth, Assigned: seth)


(Blocks 1 open bug)



(3 files, 1 obsolete file)

Bug 1293472 eliminate the last uses of SurfaceCache::InsertPlaceholder(). Let's remove it. It turns out that InsertPlaceholder() is the only thing that necessitated that we allow null ISurfaceProviders inside SurfaceCache, so let's capitalize on this change to eliminate as much nullability as possible.
This patch just removes InsertPlaceholder() and some other miscellaneous things
related to the old placeholder concept.
Attachment #8783102 - Flags: review?(dholbert)
The old approach to placeholders involve null ISurfaceProviders, but now we
don't need to support that anymore. That means it's a good time to introduce
NotNull everywhere and enforce that we don't deal with null ISurfaceProviders in
the surface cache code.
Attachment #8783107 - Flags: review?(dholbert)
As long as we're NotNull'ing all the things, let's use NotNull for
CachedSurfaces too, since in most situations they can't be null either. The
exception is when null is used to indicate that looking up a CachedSurface in
the per-image cache failed, so we do need to continue to handle that case.
Attachment #8783111 - Flags: review?(dholbert)
Blocks: 1296828
Comment on attachment 8783102 [details] [diff] [review]
(Part 1) - Remove SurfaceCache::InsertPlaceholder().

Review of attachment 8783102 [details] [diff] [review]:

Attachment #8783102 - Flags: review?(dholbert) → review+
Comment on attachment 8783107 [details] [diff] [review]
(Part 2) - Forbid null ISurfaceProviders in SurfaceCache.

Review of attachment 8783107 [details] [diff] [review]:

Attachment #8783107 - Flags: review?(dholbert) → review+
Comment on attachment 8783111 [details] [diff] [review]
(Part 3) - Use NotNull for all CachedSurfaces in SurfaceCache.

Review of attachment 8783111 [details] [diff] [review]:

r=me, with one request (perhaps for a subsequent patch):

::: image/SurfaceCache.cpp
@@ +95,2 @@
> +  NotNull<CachedSurface*> GetSurface() const { return mSurface; }

Can we rename this to just "Surface()"? (perhaps in a subsequent patch, if you like, to avoid scope-creep for this patch)

There's a Gecko convention that getters-for-pointer-values should be named "GetFoo()" iff they're allowed to return null, vs. "Foo()" if they're guaranteed to not return null.  Now that this function's return type is explicitly a NotNull pointer, its "GetFoo" naming scheme goes against that convention pretty directly.
Attachment #8783111 - Flags: review?(dholbert) → review+
Thanks for the review! I've updated the patch to rename GetSurface() to
Attachment #8783111 - Attachment is obsolete: true
Pushed by
(Part 1) - Remove SurfaceCache::InsertPlaceholder(). r=dholbert
(Part 2) - Forbid null ISurfaceProviders in SurfaceCache. r=dholbert
(Part 3) - Use NotNull for all CachedSurfaces in SurfaceCache. r=dholbert
Blocks: 1292392
You need to log in before you can comment on or make changes to this bug.