tracking bug for bugs that are fixed by the new image library.

we will fix at least these bugs.

few more.

mac changes from saari will be here before we start building anything.

r=bryner for both patches

sr=cls

sr=leaf for http://bugzilla.mozilla.org/showattachment.cgi?attach_id=27173

Ugh. I hate adding configure options that we know are going to disappear in 2 weeks...especially during a milestone release. But I hate having people mucking around with .mozconfig more. *sigh* r=cls

argh, i removed the #define from the top of the diff in nsImageDocument.cpp

I had a look at the patch and appart from ... @@ -132,7 +151,22 @@ rv = channel->GetURI(&uri); if (NS_FAILED(rv)) return rv; +#ifdef USE_IMG2 + nsCOMPtr<nsIStreamListener> kungFuDeathGrip(this); + nsCOMPtr<imgILoader> il(do_GetService("@mozilla.org/image/loader;1")); You really need a null/result check here to make sure you can get at the image loader service. + il->LoadImageWithChannel(channel, nsnull, nsnull, getter_AddRefs(mNextStream), + getter_AddRefs(mDocument->mImageRequest)); ... the changes look good to me, r=jst with the above change.

sr=hyatt for the nsImageBoxFrame changes

r= for dbaron's configure.in changes

I had a look at the patch, and... +NS_IMETHODIMP nsHTMLImageElement::OnStartContainer(imgIRequest *request, nsISupports *cx, gfxIImageContainer *image) +{ + nsCOMPtr<nsIPresContext> pc(do_QueryInterface(cx)); + + float t2p; + pc->GetTwipsToPixels(&t2p); ^^ | You really need a null check here, there's no guarantee that the QI will always succeed. With that change, sr=jst

r=dbaron on attachment 27232 [details] [diff] [review], although I'm not sure whether it's worth the bother to make the allmakefiles.sh stuff conditional (since it'll just have to be undone later), and assuming you add modules/libpr0n/decoders/gif/Makefile

new layout image frame(s) patches that set the frame on the listener object to null when destroy is called so that the listener doesn't continue to send messages to a frame after it is dead.

nsImageFrame and nsImageBoxFrame: - nsImageListener is cut-and-pasted in both nsImageFrame and nsImageBoxFrame. \ You only need one copy, right? - Seems like nsImageListener could outlive the frame the it refers to (e.g., page destroyed or image frame display type set to ``none'' while image is loading). All the pumping routines should check mFrame or something. Or will Cancel() guarantee that no more data will be delivered? (Should maybe add some debug-only sanity checks, at least.) - I don't know what the implications of passing NS_ERROR_FAILURE to the image request's Cancel() method are. Does necko want specific error codes? mscott? - You should check the NS_NewURI() result when you get the lowURI. It will return a failure code if it couldn't create a URI for you. (Or, just check the lowURI != 0) - Probably the same for the normal image URI. Can't trust them HTML attributes, y'know. - Should OnStartDecode() just return NS_OK? Or is there something you really haven't done yet? (Same with OnStartFrame(), ...) - You've got wacky tabs in OnStartContainer() and OnDataAvailable(). - Why the #if 0'd code in OnDataAvailable()? - scc would tell you to make MINMAX a static inline function instead of a macro. He'd probably call it something else, too... - I remember that the case where image height/width is specified as a percentage is nasty. attinasi, do you want to look at pav's new code? - You still have to display the broken image icon, huh? (Looking at the XXX code immediately beneath the ``display the icon'' comment) - When the image is scaled, why don't we intersect with aDirtyRect? (No API?) What does the DrawImage() call that takes a rect and a point do? (Looks like this might've moved to IDL. Hope you had nice pretty comments like the old nsIRenderingContext did!)

r=cls on attach 27232

Regarding nsImageFrame.h/cpp: what Chris said, and the following comments. A bunch of arguments that need to start with 'a' to be friendly with the rest of the layout code and the 'coding stds' - request, cx, frame, image to name a few. Error checking or asserting: QI's need to be checked for success or null output (listener->QI for example), and some other calls like GetLoadGroup, GetShell, do_GetService need to check for success and either assert or handle the errors. Preconditions: a bunch of the new methods soulc use preconditions, to check for valid arguments (null pointers) and valid member data states: OnStartContainer, OnDataAvailable, FrameChanged, GetDesiredSize, GetBaseURI, GetLoadGroup, and the new nsImageListener methods. What is with the NS_ERROR_NOT_IMPLEMENTED calls? Should they be implemented, or should they return NS_OK, or is the NS_ERROR_NOT_IMPLEMENTED return handled correctly? And what 'bout the #if 0 code? To be honest, I'm unclear what the purpose of the USE_IMG2 define is, as some of the code has had the counterpart removed (Paint, specifically). Finally, should the contractID for the loader (@mozilla.org/image/loader:1) be a constant somewhere? These comments are from reviewing the diffs - I have not applied them yet - sorry if my context is mixed up somewhere.

> What is with the NS_ERROR_NOT_IMPLEMENTED calls? Should they be implemented, > or should they return NS_OK, or is the NS_ERROR_NOT_IMPLEMENTED return > handled correctly? they are handled correctly, but i went ahead and returned NS_OK since the methods didn't need to do anything > And what 'bout the #if 0 code? To be honest, I'm unclear what the purpose > of the USE_IMG2 define is, as some of the code has had the counterpart > removed (Paint, specifically). I removed some of the #if 0 code in my latest patch.. it was just stuff I had been doing. ignore it. I shouldn't have changed the way outside of the ifdefs that things work. I've built and run many times with and without it defined, and all works well. > Finally, should the contractID for the loader (@mozilla.org/image/loader:1) > be a constant somewhere? Yes, it probably should be.... it isn't anywhere right now. I'll see about finding a good home for it. These comments are from reviewing the diffs - I have not applied them yet - sorry if my context is mixed up somewhere.

I think something has changed outside of the #ifdefs in nsImageFrame::Init: there are a bunch of lines removed and they are not inside of #ifdefs (AFAICT), check the lines starting with the comment // Set the image loader's source URL and base URL There are still some QI's you have not checked, like listener->QI(imgIDecoderObserver) - probably just need to assert in those spots anyway. Also, a couple of NS_NewURI calls that have no error checking - please grep for those and add the NS_SUCCEEDED macros or check for null nsIURI output values. There are a couple of deref's of mImageRequest that are unchecked - need to check for null or assert (search for mImageRequest->GetImageStatus). In GetNBaturalImageSize the out params are not check for null before being deref'd - maybe preconditions are needed, or null checks? Check those out, and r=attinasi on the ImageFrame stuff.

Did someone forget to checking the allmakefiles.sh patch or is expected to patch it manually even when specifying the --enable-libpr0n ? As modules/libpr0n on linux is not built.

Most of the needed patches haven't been landed on the trunk yet, so --enable-libpr0n doesn't work yet. Perhaps we should comment it out in configure.in until it does work? I was expecting things to land on the trunk sooner.

r=bryner for the windows -D changes

sr=cls for the -D changes

sr=cls on attach 28166

sr=cls on attach 27232

By my count it looks like: nsImageFrame r=attinasi nsImageBoxFrame r=danm sr=hyatt nsHTMLImageElement sr=jst nsImageDocument r=jst unix build changes r=dbaron sr=cls windows build changes r=bryner sr=cls so can I get someone to sr= nsImageDocument changes (rpotts?) someone to sr= nsImageFrame (waterson?) and someone to review the nsHTMLImageFrame changes?

this gfx patch includes the gfx changes in bug 37779 as well as other changes. I need an r= and sr= for this patch. (mac changes arn't in this patch. saari will need to post those seperatly).

nsImageFrame: sr=attinasi if you prefer that to my previous r=...

nsImageDocument.cpp (sr=rpotts) nsHTMLImageElement.cpp (r=rpotts) -- rick

nsImageFrame.[h|cpp] r=waterson

everything except for the mac build changes and the gfx changes are checked in. i need reviewers for the gfx changes... the image changes are part of 37779, so i'll borrow the reviewers from that for that part. i need reviewers for the new apis, etc.

*** Bug 70942 has been marked as a duplicate of this bug. ***

its on on all platforms.

Verified on all platforms now