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)

2.50 KB, patch
Details | Diff | Splinter Review
2.03 KB, patch
Details | Diff | Splinter Review
1.35 KB, patch
Details | Diff | Splinter Review
3.78 KB, patch
Details | Diff | Splinter Review
15.79 KB, patch
Details | Diff | Splinter Review
28.08 KB, patch
Details | Diff | Splinter Review
10.86 KB, patch
Details | Diff | Splinter Review
19.75 KB, patch
Details | Diff | Splinter Review
1.70 KB, patch
Details | Diff | Splinter Review
1.73 KB, patch
Details | Diff | Splinter Review
17.00 KB, patch
Details | Diff | Splinter Review
28.70 KB, patch
Details | Diff | Splinter Review
28.31 KB, patch
Details | Diff | Splinter Review
588 bytes, patch
Details | Diff | Splinter Review
38.34 KB, patch
Details | Diff | Splinter Review
14.51 KB, patch
Details | Diff | Splinter Review
538 bytes, 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
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
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/* diffsSplinter 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 diffSplinter 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.
*** 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: