Stuart Parmenter Assignee
	Description • 24 years ago

Bug for removing the old imagelib.

	Angela Butler
	Comment 1 • 24 years ago

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...

	Brendan Eich [:brendan]
	Comment 2 • 24 years ago

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

/be

	Stuart Parmenter Assignee
	Comment 3 • 23 years ago

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

	Stuart Parmenter Assignee
	Comment 4 • 23 years ago

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

	selmer (gone)
	Comment 5 • 23 years ago

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.

	Brendan Eich [:brendan]
	Comment 6 • 23 years ago

0.9.3?

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

/be

	Cathleen
	Comment 7 • 23 years ago

pav, come to the performance meeting to discuss what's left to do.

we want this in 0.9.2

- cathleen, chofmann, waterson

	Christopher Blizzard (:blizzard)
	Comment 8 • 23 years ago

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

	Stuart Parmenter Assignee
	Comment 9 • 23 years ago

its being worked on.

	Cathleen
	Comment 10 • 23 years ago

pav, how are you doing on this?

	Stuart Parmenter Assignee
	Comment 11 • 23 years ago

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

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 23 years ago

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

	Stuart Parmenter Assignee
	Comment 17 • 23 years ago

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

	Stuart Parmenter Assignee
	Comment 18 • 23 years ago

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.

	selmer (gone)
	Comment 19 • 23 years ago

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

	tor
	Comment 20 • 23 years ago

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

	Stuart Parmenter Assignee
	Comment 21 • 23 years ago

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).

	selmer (gone)
	Comment 22 • 23 years ago

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.

	Gagan
	Comment 23 • 23 years ago

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. 

	Asa Dotzler [:asa]
	Comment 24 • 23 years ago

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)

	Cathleen
	Comment 25 • 23 years ago

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.

	Stuart Parmenter Assignee
	Comment 26 • 23 years ago

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

	saari (gone)
	Comment 27 • 23 years ago

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

	Stuart Parmenter Assignee
	Comment 29 • 23 years ago

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).

	selmer (gone)
	Comment 30 • 23 years ago

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.

	Brian Ryner (not reading)
	Comment 31 • 23 years ago

r=bryner

	Stuart Parmenter Assignee
	Comment 32 • 23 years ago

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.

	Johnny Stenback (:jst)
	Comment 33 • 23 years ago

sr=jst

	Blake Ross
	Comment 34 • 23 years ago

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

	chris hofmann
	Comment 35 • 23 years ago

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

	Stuart Parmenter Assignee
	Comment 36 • 23 years ago

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.

	Tomi Leppikangas
	Comment 37 • 23 years ago

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

	Stuart Parmenter Assignee
	Comment 40 • 23 years ago

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

	Chris Waterson
	Comment 42 • 23 years ago

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?

	Stuart Parmenter Assignee
	Comment 43 • 23 years ago

> 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.

	Johnny Stenback (:jst)
	Comment 44 • 23 years ago

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

	Stuart Parmenter Assignee
	Comment 45 • 23 years ago

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.

	Conrad Carlen (not reading bugmail)
	Comment 46 • 23 years ago

> 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 ;-)

	Stuart Parmenter Assignee
	Comment 47 • 23 years ago

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

	Chris Waterson
	Comment 48 • 23 years ago

sr=waterson

	Stuart Parmenter Assignee
	Comment 49 • 23 years ago

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

	Pierre Saslawsky
	Comment 50 • 23 years ago

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

	timeless
	Comment 51 • 23 years ago

Photon is probably busted too.

	timeless
	Comment 53 • 23 years ago

xlib, qt and photon patches checked in.

	John C. Griggs
	Comment 54 • 23 years ago

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^)!!

	rubydoo123
	Comment 55 • 23 years ago

What's the status on this bug?

	Stuart Parmenter Assignee
	Comment 56 • 23 years ago

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

	timeless
	Comment 57 • 23 years ago

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..

	John C. Griggs
	Comment 58 • 23 years ago

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.

	Stuart Parmenter Assignee
	Comment 59 • 23 years ago

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.

	Terri Preston
	Comment 60 • 23 years ago

Verified files checked into lxr.mozilla.org