Store ImageKeys and SurfaceKeys directly on ISurfaceProviders

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

3 years ago
SurfaceCache cache entries are CachedSurface objects that wrap ISurfaceProviders, and it happens that ISurfaceProviders generally need to know their own ImageKey and SurfaceKey. However, CachedSurface redundantly stores this information itself. It makes sense to just get the ImageKey and SurfaceKey from the ISurfaceProvider, which will both use less memory and ensure that the two sets of values can't get out of sync.
(Assignee)

Comment 1

3 years ago
Created attachment 8784586 [details] [diff] [review]
(Part 1) - Use ImageKey consistently in SurfaceCache.

This patch just fixes a couple of places where we were wrongly using Image* raw
pointers instead of ImageKey values. It'd probably be smart to prevent that
using the type system, but that's better left for a different bug.
Attachment #8784586 - Flags: review?(dholbert)
(Assignee)

Comment 2

3 years ago
Created attachment 8784587 [details] [diff] [review]
(Part 2) - Store ImageKeys and SurfaceKeys directly on ISurfaceProviders.

This patch makes ImageKeys and SurfaceKeys accessible via the ISurfaceProvider
interface. Note that things get a bit more verbose here because ISurfaceProvider
and the SurfaceCache methods are both getting passed the same values, but
that'll go away in the next part.
Attachment #8784587 - Flags: review?(edwin)
Attachment #8784587 - Flags: review?(dholbert)
(Assignee)

Comment 3

3 years ago
Created attachment 8784588 [details] [diff] [review]
(Part 3) - Update SurfaceCache API to rely on ImageKeys and SurfaceKeys stored on ISurfaceProviders.

Now that part 2 is done, we can start relying on the ISurfaceProvider to know
its own ImageKey and SurfaceKey within the SurfaceCache code.

We're gradually whittling away the need for CachedSurface to exist at all!
Attachment #8784588 - Flags: review?(dholbert)
Comment on attachment 8784586 [details] [diff] [review]
(Part 1) - Use ImageKey consistently in SurfaceCache.

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

r=me
Attachment #8784586 - Flags: review?(dholbert) → review+
Comment on attachment 8784588 [details] [diff] [review]
(Part 3) - Update SurfaceCache API to rely on ImageKeys and SurfaceKeys stored on ISurfaceProviders.

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

r=me
Attachment #8784588 - Flags: review?(dholbert) → review+
Comment on attachment 8784587 [details] [diff] [review]
(Part 2) - Store ImageKeys and SurfaceKeys directly on ISurfaceProviders.

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

r=me

(Also: I'm not sure it's worth having edwin review this as well; I'm happy to call this whole patch r=me, unless there's something that you'd like edwin to look at in particular.)
Attachment #8784587 - Flags: review?(dholbert) → review+
(Assignee)

Comment 8

3 years ago
Thanks for the reviews!

(In reply to Daniel Holbert [:dholbert] from comment #7)
> (Also: I'm not sure it's worth having edwin review this as well; I'm happy
> to call this whole patch r=me, unless there's something that you'd like
> edwin to look at in particular.)

Works for me!

Comment 9

3 years ago
Pushed by seth.bugzilla@blackhail.net:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39e0894d2dfe
(Part 1) - Use ImageKey consistently in SurfaceCache. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f8d3c106410
(Part 2) - Store ImageKeys and SurfaceKeys directly on ISurfaceProviders. r=dholbert,edwin
https://hg.mozilla.org/integration/mozilla-inbound/rev/d28b09df98ff
(Part 3) - Update SurfaceCache API to rely on ImageKeys and SurfaceKeys stored on ISurfaceProviders. r=dholbert

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/39e0894d2dfe
https://hg.mozilla.org/mozilla-central/rev/8f8d3c106410
https://hg.mozilla.org/mozilla-central/rev/d28b09df98ff
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.