714752 - Make imgIContainerObserver::FrameChanged take an imgIRequest*

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Comment 1

[Scott Johnson (:jwir3)]

> observer->FrameChanged(nsnull, this, &dirtyRect);

Is there a way we could get the imgIRequest object associated with this imgIContainer and pass it in here? It seems counter intuitive to me that we would get nsnull during some of these function calls from within RasterImage.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

That's how the rest of these work.

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Note that that call doesn't go outside of imagelib, that goes to imgRequestProxy.

Comment 4

[Joe Drew (not getting mail)]

I'm not a super-reviewer; I'll redirect to folk who are.

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 585377 [details] [diff] [review]
Patch

>--- a/image/public/imgIContainerObserver.idl
>+++ b/image/public/imgIContainerObserver.idl

>  * @author Stuart Parmenter <[e-mail]>
>  * @version 0.1

Surely this should be version 0.2 now.

>--- a/layout/base/nsImageLoader.cpp
>+++ b/layout/base/nsImageLoader.cpp
>+NS_IMETHODIMP nsImageLoader::FrameChanged(imgIRequest *aRequest,
> 
>+  NS_ASSERTION(aRequest == mRequest, "This is a neat trick.");

Fatal?

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Ms2ger from comment #5)
> Comment on attachment 585377 [details] [diff] [review]
> Patch
> 
> >--- a/image/public/imgIContainerObserver.idl
> >+++ b/image/public/imgIContainerObserver.idl
> 
> >  * @author Stuart Parmenter <[e-mail]>
> >  * @version 0.1
> 
> Surely this should be version 0.2 now.

:-P

> >--- a/layout/base/nsImageLoader.cpp
> >+++ b/layout/base/nsImageLoader.cpp
> >+NS_IMETHODIMP nsImageLoader::FrameChanged(imgIRequest *aRequest,
> > 
> >+  NS_ASSERTION(aRequest == mRequest, "This is a neat trick.");
> 
> Fatal?

nsImageLoader is going away entirely in Bug 697230, so I didn't bother.

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 585377 [details] [diff] [review]
Patch

I think joe should look at this one way or another, so I'm switching over review.

Comment 8

[Joe Drew (not getting mail)]

Comment on attachment 585377 [details] [diff] [review]
Patch

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

::: layout/base/nsImageLoader.cpp
@@ +223,5 @@
>      // We're in the middle of a paint anyway
>      return NS_OK;
>    }
>  
> +  NS_ASSERTION(aRequest == mRequest, "This is a neat trick.");

yeah, NS_ABORT_IF_FALSE is the usual in imagelib-related stuff.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/af6501ede378

Comment 10

[Eric Shepherd [:sheppy]]

These interfaces were not documented at all. I've added very rudimentary documentation that could use a lot of fleshing out, but these are documented for the purposes of this bug.

https://developer.mozilla.org/en/XPCOM_Interface_Reference/imgIContainerObserver
https://developer.mozilla.org/en/XPCOM_Interface_Reference/imgIDecoderObserver
https://developer.mozilla.org/en/XPCOM_Interface_Reference/imgIDecoder
https://developer.mozilla.org/en/XPCOM_Interface_Reference/imgIContainer

Also mentioned on Firefox 12 for developers.