840841 - Remove mObserver from RasterImage and VectorImage

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

It doesn't seem like the mObserver member that RasterImage and VectorImage have adds any value. They can always get to the only observer they ever get passed - an imgStatusTrackerObserver - via mStatusTracker->GetDecoderObserver(). Also, mObserver is a weak pointer, but we know we can never lose an imgStatusTrackerObserver as long as we hold a strong reference to the corresponding imgStatusTracker. In the interest of reduced memory consumption and complexity it makes sense to eliminate mObserver.

Comment 1

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

Attachment: Remove mObserver from RasterImage and VectorImage.

Proposed patch. Try job here: https://tbpl.mozilla.org/?tree=Try&rev=6418c319c3d4

Comment 2

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

Attachment: Mark RasterImage::Initialize as static in RasterImage.cpp.

Proposed patch.

Comment 3

[Joe Drew (not getting mail)]

I'm broadly in favour of this, though it's going to conflict with my own changes when I try to rebase on top if it :)

Comment 4

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

Ack, stupid bzexport! Or possibly stupid me. Regardless, one of these got posted to the wrong bug.

Comment 5

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

Comment on attachment 713247 [details] [diff] [review]
Remove mObserver from RasterImage and VectorImage.

Try looks good, but rerequesting review because I want to be sure you looked at the right patch. =) Sorry for the mixup.

Comment 6

[Joe Drew (not getting mail)]

Comment on attachment 713247 [details] [diff] [review]
Remove mObserver from RasterImage and VectorImage.

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

::: image/src/RasterImage.cpp
@@ +2548,5 @@
>    CONTAINER_ENSURE_TRUE(type != eDecoderType_unknown, NS_IMAGELIB_ERROR_NO_DECODER);
>  
>    // Instantiate the appropriate decoder
> +  imgDecoderObserver* observer = mStatusTracker ? mStatusTracker->GetDecoderObserver()
> +                                                : nullptr;

Are there actually times when RasterImages have no status tracker?

Comment 7

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

Thanks for the review, Joe!

(In reply to Joe Drew (:JOEDREW! \o/) from comment #6)
> Are there actually times when RasterImages have no status tracker?

Yeah - ImageFactory::CreateAnonymousImage will create an image with no status tracker. This is called from imgTools::DecodeImageData.

Comment 8

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

Everything looks OK here. Requesting checkin.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/78d86cf0671b

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/78d86cf0671b