1097432 - Rename imgStatusTracker to ProgressTracker and simplify its interface

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

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.

Comment 1

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

Attachment: (Part 1) - Remove imgStatusTracker::Send* methods and clean up

This patch removes the imgStatusTracker::Send* family of methods, and simplifies further where we can.

Comment 2

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

Attachment: (Part 2) - Clean up imgStatusTracker's interface

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.

Comment 3

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

Attachment: (Part 3) - Rename imgStatusTracker to ProgressTracker and ImageStatusDiff to Progress

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.

Comment 4

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

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

Comment 5

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

Attachment: (Part 3) - Rename imgStatusTracker to ProgressTracker and ImageStatusDiff to Progress

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.)

Comment 6

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

Here's a new try job:

https://tbpl.mozilla.org/?tree=Try&rev=49674922fdc2

Comment 7

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

Attachment: (Part 3) - Rename imgStatusTracker to ProgressTracker and ImageStatusDiff to Progress

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.

Comment 8

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

OK, that try job looks good. Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a958224850fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dee00e11ec2
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2dd570e9350

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a958224850fb
https://hg.mozilla.org/mozilla-central/rev/6dee00e11ec2
https://hg.mozilla.org/mozilla-central/rev/a2dd570e9350