73535 - libpr0n needs super-review

Status: RESOLVED WORKSFORME

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

Description

[Brendan Eich [:brendan]]

I'll use this bug to review, quote code, ask questions, etc.

/be

Comment 1

[Brendan Eich [:brendan]]

Almost done -- need to look at imgContainer.cpp, and take a closer look at
nsImageFrame.cpp.  Pav what other files are there?

/be

Comment 2

[Brendan Eich [:brendan]]

Of course, imgContainer.cpp is implicated in recent crash stacks.  Pavlov, let's
finish this review during the freeze.

/be

Comment 3

[chris hofmann]

what's let to do here?

Comment 4

[chris hofmann]

s/let/left/

Comment 5

[Brendan Eich [:brendan]]

I'll get rid of this bug by tonight.

/be

Comment 6

[Brendan Eich [:brendan]]

Need to do imgContainer.cpp, at least.

/be

Comment 7

[Brendan Eich [:brendan]]

I don't see why this should hold 0.9.1, but someone else besides me has to care.
 saari, the ball may be in your court.

/be

Comment 8

[saari (gone)]

Well, I'd like to rip the gif specific stuff out of nsImageContainer in the not
too distant future, so maybe that would be a good time to review. 

Comment 9

[Brendan Eich [:brendan]]

saari, what's new?

/be

Comment 10

[saari (gone)]

That cleanup hasn't happened yet, but may soon. I'm currently figuring out what
I'm going to be working on in the near future, and libpr0n cleanup/finishing is
definitely on that list.

Comment 11

[Brendan Eich [:brendan]]

Saari, you need to own this, or find an owner.  Maybe tor can help.

/be

Comment 12

[saari (gone)]

brendan, I'd be happy to go over what is there with you or any of the other sr's
with the understanding that it is still in flux (we can talk about the future
changes too). Whatever you prefer.

Comment 13

[Judson Valeski]

This landed awhile ago. this bug is probably a stale placeholder. let's
open/fix/review separate bugs that fall out of the landing.

Comment 14

[Mike Shaver (:shaver emeritus)]

This bug was opened post-landing, in fact, to address the fact that libpr0n went
in badly under-reviewed.  Unless brendan feels that libpr0n no longer warrants
retrospective super-review attention, I think we should leave it open.

Comment 15

[scottputterman]

Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+,
topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword.  Please send any
questions or feedback about this to [e-mail].  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.

Comment 16

[Brendan Eich [:brendan]]

But moving it to 1.2 is the same as saying "never", given all the postponements
recorded here.  Saari, can you justify mozilla1.0 (topembed, nsbeta1 and
therefore MachV) without finishing this code review?

/be

Comment 17

[saari (gone)]

Brendan, in principal I agree this should be done. In reality I have known bugs
that I'm struggling to get fixed in the 1.0 time frame. I'm not going to fight
for this given the other issues on my plate. The project is never done...

Comment 18

[Brendan Eich [:brendan]]

Then I claim you won't ever finish this, so just mark it WONTFIX.  I don't think
anyone should believe that libpr0n's landing without super-review was a good and
right thing.  The next rewrite-itis attack to try that stunt will get backed out.

/be

Comment 19

[saari (gone)]

You'll note I didn't say it was a good thing that this didn't get SR. You'll
note I didn't press the commit button either. You're preaching to the choir Brendan.

You want to do this, schedule a time with me in calendar. Seriously, I'll be
happy to sit down with you and do it, but I'm swamped and I'm not chasing it as
a priority.

Comment 20

[Brendan Eich [:brendan]]

saari, I didn't imply or say anything about you not being a choirboy, honest!

I do think that targeting at 1.2 is about the same as targeting at Future, esp.
for a bug like this that's easy to deprioritize (legitimately).  If you close it
as WFM we'll get on with our lives.  No one will think what happened was good,
and I'll view it as a failure on my part, but those happen.  If you think we can
go through the code productively soon, then by all means pull the target in and
let's order pizza.

/be

Comment 21

[Stuart Parmenter]

since this is clearly not going to happen any time soon and enough of imagelib
has changed since it was checked in initially, i see no point in keeping this
around.