Open Bug 910441 Opened 7 years ago Updated 5 years ago

Bring sanity to imagelib notifications, image status, and imgStatusTracker

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

People

(Reporter: seth, Assigned: seth)

References

Details

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.
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.
Depends on: 1084136
Depends on: 1084679
Depends on: 1087615
Blocks: 1089046
No longer blocks: 1089046
Depends on: 1089046
Depends on: 1089880
No longer depends on: 1089880
Blocks: 1093785
Depends on: 1097405
Depends on: 1097431
Depends on: 1097432
Depends on: 1098108
Depends on: 1089880
Depends on: 1098202
Depends on: 968008
Blocks: 1089561
Blocks: 1083471
Depends on: 1098645
Depends on: 1098648
Depends on: 1100497
Blocks: 1079653
You need to log in before you can comment on or make changes to this bug.