Closed Bug 1097432 Opened 6 years ago Closed 6 years ago

Rename imgStatusTracker to ProgressTracker and simplify its interface

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: seth, Assigned: seth)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 2 obsolete files)

In this bug I'm trying to build on the gains of the other blockers of bug 910441 to reduce imgStatusTracker's currently rather large interface, simplify its code, eliminate duplication, and make it easier to use at call sites.

I also want to take the opportunity to move imgStatusTracker into the mozilla::image:: namespace and to rename it to something that better fits its new, reduced role. Since imgStatusTracker's state at this point consists of a set of monotonic bit flags, we can view it as representing our progress in loading the image. The term 'progress' is particularly convenient because it seems to work well for both the current state of the image, and a change to the state that something like a decoder might make. (This is because the term 'progress' leaves ambiguous what the progress is relative to.) Since both things are now represented by the same type, a typedef over a simple uint32_t, it's nice that the same word works for both instead of requiring a distinction like Status and StatusDiff.

I've tried to avoid making any changes to functionality in this bug. This bug is purely about refactoring.
This patch removes the imgStatusTracker::Send* family of methods, and simplifies further where we can.
Attachment #8521092 - Flags: review?(tnikkel)
This patch cleans up imgStatusTracker's interface - there are a lot of small things here that don't seem to justify their own patches. Methods are regrouped more logically in the header file, methods that don't need to be public are made private, and methods that are expressable in terms of the other methods are removed.

There is undoubtedly more cleanup we could do, but I think the changes here do a lot to make imgStatusTracker's interface smaller and easier to understand.
Attachment #8521096 - Flags: review?(tnikkel)
Attachment #8521092 - Flags: review?(tnikkel) → review+
Attachment #8521096 - Flags: review?(tnikkel) → review+
This patch renames imgStatusTracker to mozilla::image::ProgressTracker and ImageStatusDiff to Progress. It also changes ImageStatusDiff from a class to a typedef'd uint32_t. It then propagates these changes to all call sites. It's a pretty large patch, but it doesn't actually do anything complicated.

The improvement at the call sites is really nice, though! Now we can send notifications with just one line of code.
Attachment #8521102 - Flags: review?(tnikkel)
Wow, thanks for those lightning-quick reviews! Here's a try job for the whole series of patches:

https://tbpl.mozilla.org/?tree=Try&rev=fe6ad7fbfd0e
Attachment #8521102 - Flags: review?(tnikkel) → review+
Thanks for the review!

It looks like there's a build issue on MSVC with the previous version of the patch. The problem seems to be that the nsIRunnables used by ProgressTracker were forward declared outside of the mozilla::image namespace. I've fixed that, and as long as I was at it, cleaned up the runnables a bit. (Made parameter names match the style guide, renamed imgStatusNotifyRunnable to AsyncNotifyRunnable and imgRequestNotifyRunnable to AsyncNotifyCurrentStateRunnable to better clarify what the actual role of each runnable is.)
Attachment #8521102 - Attachment is obsolete: true
Here's the final version of part 3, assuming that the previous try job comes back green. I made a couple of minor tweaks to comments.
Attachment #8521767 - Attachment is obsolete: true
Depends on: 1133356
You need to log in before you can comment on or make changes to this bug.