Closed Bug 70938 Opened 24 years ago Closed 24 years ago

meta: Land new image library.

Categories

(Core :: Graphics: ImageLib, defect, P1)

defect

Tracking

()

VERIFIED FIXED
mozilla0.9

People

(Reporter: pavlov, Assigned: pavlov)

References

Details

(Keywords: meta, topperf)

Attachments

(17 files)

(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
tracking bug for bugs that are fixed by the new image library.
we will fix at least these bugs.
Status: NEW → ASSIGNED
OS: Solaris → All
Priority: -- → P1
Hardware: Sun → All
Summary: meta: Bugs fixed buy new image library. → meta: Land new image library.
Target Milestone: --- → mozilla0.9
few more.
Blocks: 47659
Attached patch patch to pull new code on unix (deleted) — Splinter Review
mac changes from saari will be here before we start building anything.
Keywords: meta, topperf
r=bryner for both patches
sr=cls
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
Attached patch content\html\content\src diff (deleted) — Splinter Review
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
Attached patch gfx/* diffs (deleted) — Splinter Review
Attached patch new unix makefile changes (deleted) — Splinter Review
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.
No longer blocks: 63750
Depends on: 37779
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?
Attached patch newer gfx diff (deleted) — Splinter Review
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.
Attached patch build by default on windows (deleted) — Splinter Review
*** Bug 70942 has been marked as a duplicate of this bug. ***
Depends on: 73740
its on on all platforms.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Verified on all platforms now
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: