1298547 - Notify ISurfaceProviders that they've been discarded, instead of notifying the Image directly

Status: RESOLVED INACTIVE

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

In order to write a GTest suite for the surface cache, we really need to eliminate dependencies between SurfaceCache and Image code. The only source of dependency is the Image::OnSurfaceDiscarded() notification. (Which is a bit of a shame, as this notification is only used in tests.) We can route this notification through ISurfaceProvider to remove the dependency and allow SurfaceCache to be tested independently of the Image class hierarchy.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Move the larger methods from ISurfaceProvider.h into ISurfaceProvider.cpp.

This is a preliminary step. The new ISurfaceProvider method added in part 2
can't be in ISurfaceProvider.h because of include order issues. We're starting
to get more and more multiline methods in ISurfaceProvider.h, so now seems like
a good time to introduce ISurfaceProvider.cpp and move things over.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Notify ISurfaceProviders when they're discarded instead of notifying the Image directly.

This patch just adds a level of indirection so that ISurfaceProviders take care
of interacting with the Image they're associated with, rather than SurfaceCache
directly talking to the Image. Adding this level of indirection will simplify
writing GTests for the surface cache. It feels cleaner anyway since
ISurfaceProviders already interact with their Images, whereas SurfaceCache
otherwise doesn't.

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8785482 [details] [diff] [review]
(Part 1) - Move the larger methods from ISurfaceProvider.h into ISurfaceProvider.cpp.

Review of attachment 8785482 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, one nit:

::: image/ISurfaceProvider.cpp
@@ +50,5 @@
> +
> +SimpleSurfaceProvider::SimpleSurfaceProvider(const ImageKey aImageKey,
> +                                             const SurfaceKey& aSurfaceKey,
> +                                             NotNull<imgFrame*> aSurface)
> +  : ISurfaceProvider(aImageKey, aSurfaceKey, AvailabilityState::StartAvailable())

Nit: this line's a bit more than 80 characters. Consider wrapping before AvailabilityState.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8785485 [details] [diff] [review]
(Part 2) - Notify ISurfaceProviders when they're discarded instead of notifying the Image directly.

Review of attachment 8785485 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, nits below:

::: image/SurfaceCache.cpp
@@ +492,5 @@
>      MOZ_ASSERT(cache, "Shouldn't try to remove a surface with no image cache");
>  
>      // If the surface was not a placeholder, tell its image that we discarded it.
>      if (!aSurface->IsPlaceholder()) {
> +      aSurface->OnDiscard();

Two nits here:
 (1) The "aSurface->IsPlaceholder()" check here is redundant now, since the new CachedSurface::OnDiscard method has its own internal IsPlaceholder check.

 (2) Perhaps the comment here should be updated ("tell its image that we discarded it").  Previously we *were* literally making a call on the Image object here -- but now there are several levels of indirection.  Maybe s/its image/its provider (via the CachedSurface)/, or something like that?

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:aosmond, could you have a look please?
For more information, please visit auto_nag documentation.