910441 - Bring sanity to imagelib notifications, image status, and imgStatusTracker

Status: NEW

(Core :: Graphics: ImageLib, defect)

Description

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

Notifications are one of the trickiest things to get right in imagelib. There are subtle dependencies throughout the code on which notifications are to be sent, which order they should be sent in, and whether they are delivered synchronously or asynchronously. Images also have a closely-related status which gets updated using complex and error-prone rules. imgStatusTracker is the code at the heart of all of this, but the implications affect the entire codebase, including content and layout.

This is a metabug which aims to tame this beast and bring some sanity to this part of the code. The real work will probably get done in a bunch of smaller bugs, but I want to use this bug to keep the big picture in mind and help focus these efforts.

Comment 1

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

Here is a brain dump of some obvious issues I'd like to solve:

- imgStatusTracker already encodes a state machine, but the specification is unclear. Let's make that more formal, refactoring the code around an internal state machine class that has documented rules for which transitions are valid and invalid in each state, and add asserts to enforce those rules. This will make it much clearer exactly what has gone wrong when debugging image-notification-related issues.

- In general I don't like that the status flags aren't monotonic. (That is, once they get set, I'd like them to _stay_ set.) That is a huge mess and makes things quite difficult to reason about. Some of them are easily fixed - for example, the error state should _not_ clear all other bits, but we need to update code everywhere to account for this change. Some of them are trickier - in particular, since images can be discarded and redecoded many times, the decode-related bits have to be handled specially. At a minimum, we should separate the non-monotonic state from the rest. Ideally we should change the design in a way to eliminate any non-monotonic state.

- We should not be clearing the invalid rectangle of the other status tracker in imgStatusTracker::Difference; that should be a pure function. This probably isn't that hard to fix. More generally, all of the complication in Difference and ApplyDifference points to questionable design. Those functions should be totally straightforward, and right now they're not. Some of this ties into the monotonicity concern.

- All the code that handles imagelib notifications, _everywhere_ in the code, should use the imagelib names for those notifications and _not_ Necko names. For example, it should be LoadComplete, not OnStopRequest.

Comment 2

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

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.