Bug for removing the old imagelib.

Per PDT Triage effort: changing the targeted milestone to "mozilla0.9.2". Please feel free to renominate/address bug if time is available AFTER all currently targeted 0.9.1 bugs are resolved. .Angela...

What's the rationale for not doing this sooner? Where are the dependency bugs for switching backgrounds and bullets to libpr0n? /be

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

shipping back to 0.9.1... this needs to be fixed for various reasons for 0.9.1.

No significant customer impact, moving off the radar. If there is some customer visible impact that would affect large numbers of people, please add that information here and bring this back to our attention.

0.9.3? Pavlov, can you say exactly what the "various reasons" are for your move of this bug to 0.9.1? /be

pav, come to the performance meeting to discuss what's left to do. we want this in 0.9.2 - cathleen, chofmann, waterson

The last update in this bug was a week ago. Anything going on here?

its being worked on.

pav, how are you doing on this?

trying to get my @#$@#$! windows build to stop dying for random reasons.

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

ok, i have a branch.. IMG2_20010618_BRANCH... it builds on windows and unix. mac needs some help.

I have this stuff working on mac windows and unix on the branch.. about to check in the mac changes to the branch. There is no way this is going to get through both all the process and checked in in the next 30 minutes. It also needs to have more testing done with it before it is checked in. I would still like to get this checked in for 0.9.2 though.

Pav, doing everything right, when would you be ready to check this in?

We're planning on doing test builds for this, right?

I expect to have the branch in a state i'm comfortable with tomorrow. We try and get test builds done tomorrow. So lets say I get test builds made by tomorrow afternoon... if they were perfect, than we could probably land this friday, however, since i expect that everything isn't going to be quite perfect, it will probably be the weekend or monday (if we want to carpool it).

Removing PDT+, we'd like to see this on the trunk and shake it out a little. If all looks well, we'll look at pulling it up to the branch later.

I agree that this should be tried on the trunk first but it does not mean that this is now a 0.9.3 candidate. Pulling it back in for 0.9.2.

pushing out. 0.9.2 is done. (querying for this string will get you the list of the 0.9.2 bugs I moved to 0.9.3)

pavlov, when is this ready to land on the trunk? :-) seems like we're at risk taking this change for rtm branch... time is running out to smooth out issues on the trunk.

ok, this has taken *way* longer that it should have. I expect to land this Wednesday.

pav, if you need verification on mac, do tell. Also, thoughts on getting this onto the branch? Certain embedding customers want it...

I need r= and sr= for this patch. It simply adds a nsLoadFlag param to the loadImage. This allows for the bigger patch (coming soon) to load images in the background (instead of keeping the document waiting while they load).

At this point, if this is needed on the branch for embedding customers, it will have to wait until we're done with the branch for the main release. I can't PDT+ this until then.

r=bryner

In the patch, the code in nsImageFrame.cpp in the "@@ -1237,21 +1237,14 @@" section is not being checked in (i've removed it from my tree), so please ignore it.

sr=jst

according to pav, this isn't landing for 0.9.3.

at least parts of this are needed for a key embedding customer

new branch: NOIMG_20010801_BRANCH builds on windows and linux.. will try and get mac working today. need to do test builds... hope to land monday/tuesday on trunk.

seems to miss couple of xlib changes, i'll attach patch when my build finished.

Windows and linux test builds available at ftp://ftp.mozilla.org/pub/mozilla/nightly/experimental/NOIMG_20010801_BRANCH/

Top-level comments: 1. I'm not sure if the #if 0'd code (there's a lot) is stuff you haven't implemented yet, or stuff you just didn't get around to removing. Why do I suspect it's the former? ;-) 2. I'm worried about removing nsPresContext::Stop(). Can we still stop image loads? Details: - Just remove CThrobber::Notify(), CThrobber::DrawSelf(), CThrobber::LoadImages(), etc. from embedding/browser/powerplant/src instead of #if 0? Did you break the throbber? :-) - Just remove nsIRenderingContext::DrawTile(), rather than #if 0? (Two overloads.) Same in nsRenderingContextImpl.[h|cpp]. - Kill #if 0'd code in nsBlender.cpp. Looks like maybe one method can go away altogether? - Why does nsBlender.h need to declare |class IL_ColorSpace|? - You'll probably be racing smfr with your changes to nsGfxFactoryMac.cpp! ;-) - Wanna talk to pink or smfr about nsImageMac::DrawTile()? (``this code is quite slow.'') - Remove #if 0'd code from nsImageWin.cpp? (``this code is quite slow.'') - Do ``broken'' images still work? - Why is it okay to remove ``stop'' from the pres context? What happens to in-progress image loads? - Remove #if 0'd code from nsPresContext.cpp? - Do you leak nsImageLoader objects in nsPresContext::LoadImage()? I see where you're addref'ing |newLoader|, but don't see where it's being released. Oh, are you just transferring it from |newLoader| to |loader|? In that case, why are you NS_REINTERPRET_CAST()'ing from |sup.get()|? Can't you just assign from |newLoader| and avoid the cast? (What is the deal with the QI() to nsISupports, anyway?) - Why is the body of nsPresContext::StopLoadImage() #if 0'd? Does this still need to be implemented? Or just removed? Same with StopAllLoadImagesFor(). - We really _should_ factor code between nsBulletFrame and nsImageFrame. Could you file a bug on this when you land? Give it to attinasi! ;-) - There's a bunch of #if 0'd code in nsBulletFrame::GetDesiredSize(). How come? Same in OnStopDecode(). - Should you just get rid of the #ifndef IMG2 stuff in nsImageFrame.cpp? (There's no going back, baby.) - Ick. Put this on two lines (nsCSSRendering.cpp): + if (req) req->GetImageStatus(&status); - Kill #if 0'd and commented-out code in webshell/tests/viewer/nsThrobber.[h|cpp]. Or did you just break the throbber? :-) - Maybe talk to smfr about the imageManager->FlushCache() stuff in nsMacMemoryCushion. - Why #if 0'd code in nsWindow::SetColorMap()? Why are we ignoring WM_QUERYNEWPALETTE?

> Top-level comments: > > 1. I'm not sure if the #if 0'd code (there's a lot) is stuff you > haven't implemented yet, or stuff you just didn't get around to > removing. Why do I suspect it's the former? ;-) Most of it was just stuff that should have been removed. The exception is viewer and the mac embedding test. These still need a solution, but they are both different solutions and the mac one is platform specific. > > 2. I'm worried about removing nsPresContext::Stop(). Can we still stop > image loads? Yes, see below. > > Details: > > - Just remove CThrobber::Notify(), CThrobber::DrawSelf(), > CThrobber::LoadImages(), etc. from embedding/browser/powerplant/src > instead of #if 0? Did you break the throbber? :-) The mac embedding project should not ever be using imagelib. First, the APIs are not "public" for embedders to use, and more importantly, there is no need. I've talked to valeski about this briefly and we're ok with the throbber being broken for a little while. > - Just remove nsIRenderingContext::DrawTile(), rather than #if 0? (Two > overloads.) Same in nsRenderingContextImpl.[h|cpp]. Done > - Kill #if 0'd code in nsBlender.cpp. Looks like maybe one method can > go away altogether? > > - Why does nsBlender.h need to declare |class IL_ColorSpace|? > > - You'll probably be racing smfr with your changes to > nsGfxFactoryMac.cpp! ;-) I've got a carpool scheduled for tomorrow.. I bet i'll win ;) > > - Wanna talk to pink or smfr about nsImageMac::DrawTile()? (``this > code is quite slow.'') There is a bug filed on mac tiling being slow.. It is waiting for my branch to land. (Bug 86694) > > - Remove #if 0'd code from nsImageWin.cpp? (``this code is quite > slow.'') Done > > - Do ``broken'' images still work? What do you mean by broken images? > > - Why is it okay to remove ``stop'' from the pres context? What > happens to in-progress image loads? > Image loads in the document are attached to a loadgroup. Hitting stop will cause Cancel to be called on the nsIRequest interface that the image requests implement, causing the image to stop loading. There is no need for this functionality in nsIPresContext > - Remove #if 0'd code from nsPresContext.cpp? done > > - Do you leak nsImageLoader objects in nsPresContext::LoadImage()? I > see where you're addref'ing |newLoader|, but don't see where it's > being released. Oh, are you just transferring it from |newLoader| to > |loader|? In that case, why are you NS_REINTERPRET_CAST()'ing from > |sup.get()|? Can't you just assign from |newLoader| and avoid the > cast? (What is the deal with the QI() to nsISupports, anyway?) > No, they don't leak. I've run the stuff through purify. If I don't do the QI, then it won't let me call methods on it. I don't really claim to understand why. > - Why is the body of nsPresContext::StopLoadImage() #if 0'd? Does this > still need to be implemented? Or just removed? Same with > StopAllLoadImagesFor(). I've removed these now. > > - We really _should_ factor code between nsBulletFrame and > nsImageFrame. Could you file a bug on this when you land? Give it to > attinasi! ;-) Yup. See bug 94455 > > - There's a bunch of #if 0'd code in > nsBulletFrame::GetDesiredSize(). How come? Same in OnStopDecode(). > > - Should you just get rid of the #ifndef IMG2 stuff in > nsImageFrame.cpp? (There's no going back, baby.) Yes, we should. Next pass. > > - Ick. Put this on two lines (nsCSSRendering.cpp): > > + if (req) req->GetImageStatus(&status); done :) > > - Kill #if 0'd and commented-out code in > webshell/tests/viewer/nsThrobber.[h|cpp]. Or did you just break the > throbber? :-) The throbber is busted. Since viewer is XP, something needs to be hacked together to get the throbber working again, however this is pretty low on my list of things to do. > > - Maybe talk to smfr about the imageManager->FlushCache() stuff in > nsMacMemoryCushion. I'm going to add the this to the image cache. I've filed bug 94454 for this. > > - Why #if 0'd code in nsWindow::SetColorMap()? Why are we ignoring > WM_QUERYNEWPALETTE? We no longer use palettes (which may change at a later point), so there is no need for it anymore.

In addition to the above... - nsImageLoader.h: +class nsImageLoader : imgIDecoderObserver +{ Don't you want "public imgIDecoderObserver", same goes for "class nsBulletListener : imgIDecoderObserver" in nsBulletFrame.cpp - In nsBulletFrame::GetDesiredSize(), you declare p2t and put a value in it, but I don't see it being used anywhere, could you get rid of that variable? - nsBulletFrame::OnDataAvailable(), why not nsRect r(*aRect) in stead of nsRect r(aRect->x, aRect->y, aRect->width, aRect->height);? r=jst

good call. I fixed these things as well. I believe the only thing missing is waterson's sr=. Hopefully he will get it for me early this morning so that I can land my carpool today as scheduled.

> The mac embedding project should not ever be using imagelib. First, the APIs > are not "public" for embedders to use, and more importantly, there is no need. Simon, thanks for CC'ing me. I'll have to cook up a native impl of this. The current one is bad news and was something I did long ago when first working with mozilla ;-)

cool. I was going to try and get ahold of you the other day but you were on vacation. Sounds good.

sr=waterson

Checked in on the trunk. Will check in on the 0.9.2 branch next week.

On the Mac, the landing caused further regressions to http://www.blizzard.com which was already affected by bug 91034.

Photon is probably busted too.

xlib, qt and photon patches checked in.

Is this completely in the trunk now? I have my Qt patch applied locally and I am having trouble building Qt Mozilla from the CVS tip - the build stops while trying to build modules/imglib with: syntax error near unexpected token ';' set -e; for d in ; do make -C $d export; done It looks to me like the $(DIRS) in LOOP_OVER_DIRS is expanding to an empty list in the Qt build in this Makefile. Any clues on how to fix this would be most appreciated. Having someone else fix it would be even better 8^)!!

What's the status on this bug?

it is gone from the trunk.. landing on branch by friday..

Qt can now build if you don't run into JCG's problem, xlib now builds. I have no idea about Photon (i suppose i could reboot into QNX and try to find out, but i've never built there, i'm much more comfortable in Be). Qt does work on solaris to mi/x if i use -P (well, it loads mozilla.org, that's enough for me). I'd really like to know if Photon builds..

I need some advice on the correct way to deal with the modules/libimg Makefile breaking the Qt build. What seems to be happening is that the DIRS variable is empty in modules/libimg/Makefile, so the expansion of the LOOP_OVER_DIRS rule from config/rules.mk is breaking. I am not sure if this would happen for all Qt builds or if it is just my system. I guess I need a way to tell the build system not to enter modules/imglib if the png and mng sub-directories are not going to be built (i.e. if neither MOZ_NATIVE_PNG or MOZ_NATIVE_MNG are defined). Any help would be greatly appreciated. The only way I know of to get the build to work properly at the moment is to manually remove modules/libimg from the DIRS variable in the top level Makefile after running configure, which is a slimey hack, at best.

this has now landed on the trunk and the branch. marking fixed. please file seperate bugs or find the other bugs that are already filed for the various port platforms.

Verified files checked into lxr.mozilla.org