Closed Bug 78690 Opened 19 years ago Closed 19 years ago

WRMB: Remove the old imagelib.

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla0.9.4

People

(Reporter: pavlov, Assigned: pavlov)

References

Details

(Keywords: memory-footprint, perf, topembed)

Attachments

(9 files)

Bug for removing the old imagelib.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla0.9.1
Depends on: 78015
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...
Target Milestone: mozilla0.9.1 → mozilla0.9.2
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.
Target Milestone: mozilla0.9.2 → mozilla0.9.1
Blocks: 77262
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.
Target Milestone: mozilla0.9.1 → mozilla0.9.3
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
Target Milestone: mozilla0.9.3 → mozilla0.9.2
The last update in this bug was a week ago.  Anything going on here?
Keywords: footprint
its being worked on.
Blocks: 59667
Blocks: 78649
pav, how are you doing on this?
trying to get my @#$@#$! windows build to stop dying for random reasons.
Blocks: 84654
Blocks: 52912
Attached patch almost there...Splinter Review
*** 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.
Depends on: 86694
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.
Keywords: perf
Whiteboard: PDT+
Pav, doing everything right, when would you be ready to check this in?
Whiteboard: PDT+ → PDT+; need eta
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).
Whiteboard: PDT+; need eta → PDT+; ETA 06/25
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.
Whiteboard: PDT+; ETA 06/25
Target Milestone: mozilla0.9.2 → mozilla0.9.3
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. 
Target Milestone: mozilla0.9.3 → mozilla0.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)
Target Milestone: mozilla0.9.2 → mozilla0.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
Blocks: 88125
according to pav, this isn't landing for 0.9.3.
Target Milestone: mozilla0.9.3 → mozilla0.9.4
at least parts of this are needed for a key embedding customer
Keywords: topembed
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/
Blocks: 91034
Attached patch "Final" patch.Splinter Review
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?
Summary: Remove the old imagelib. → WRMB: Remove the old imagelib.
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.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Verified files checked into lxr.mozilla.org
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.