Closed
Bug 70938
Opened 24 years ago
Closed 24 years ago
meta: Land new image library.
Categories
(Core :: Graphics: ImageLib, defect, P1)
Core
Graphics: ImageLib
Tracking
()
VERIFIED
FIXED
mozilla0.9
People
(Reporter: pavlov, Assigned: pavlov)
References
Details
(Keywords: meta, topperf)
Attachments
(17 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
tracking bug for bugs that are fixed by the new image library.
Assignee | ||
Comment 1•24 years ago
|
||
we will fix at least these bugs.
Assignee | ||
Comment 2•24 years ago
|
||
few more.
Assignee | ||
Comment 3•24 years ago
|
||
Assignee | ||
Comment 4•24 years ago
|
||
Assignee | ||
Comment 5•24 years ago
|
||
mac changes from saari will be here before we start building anything.
Assignee | ||
Updated•24 years ago
|
Comment 6•24 years ago
|
||
r=bryner for both patches
Comment 8•24 years ago
|
||
Comment 9•24 years ago
|
||
Comment 10•24 years ago
|
||
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
Assignee | ||
Comment 11•24 years ago
|
||
Assignee | ||
Comment 12•24 years ago
|
||
argh, i removed the #define from the top of the diff in nsImageDocument.cpp
Assignee | ||
Comment 13•24 years ago
|
||
Comment 14•24 years ago
|
||
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.
Assignee | ||
Comment 15•24 years ago
|
||
Comment 16•24 years ago
|
||
sr=hyatt for the nsImageBoxFrame changes
Assignee | ||
Comment 17•24 years ago
|
||
r= for dbaron's configure.in changes
Assignee | ||
Comment 18•24 years ago
|
||
Comment 19•24 years ago
|
||
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
Assignee | ||
Comment 20•24 years ago
|
||
Assignee | ||
Comment 21•24 years ago
|
||
Assignee | ||
Comment 22•24 years ago
|
||
Comment 23•24 years ago
|
||
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
Assignee | ||
Comment 24•24 years ago
|
||
Assignee | ||
Comment 25•24 years ago
|
||
Assignee | ||
Comment 26•24 years ago
|
||
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.
Comment 27•24 years ago
|
||
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!)
Comment 28•24 years ago
|
||
r=cls on attach 27232
Comment 29•24 years ago
|
||
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.
Assignee | ||
Comment 30•24 years ago
|
||
Assignee | ||
Comment 31•24 years ago
|
||
> 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.
Comment 32•24 years ago
|
||
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.
Comment 33•24 years ago
|
||
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.
Comment 34•24 years ago
|
||
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.
Assignee | ||
Comment 35•24 years ago
|
||
Assignee | ||
Comment 36•24 years ago
|
||
r=bryner for the windows -D changes
Assignee | ||
Comment 37•24 years ago
|
||
sr=cls for the -D changes
Comment 38•24 years ago
|
||
sr=cls on attach 28166
Comment 39•24 years ago
|
||
sr=cls on attach 27232
Assignee | ||
Comment 40•24 years ago
|
||
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?
Assignee | ||
Comment 41•24 years ago
|
||
Assignee | ||
Comment 42•24 years ago
|
||
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).
Comment 43•24 years ago
|
||
nsImageFrame: sr=attinasi if you prefer that to my previous r=...
Comment 44•24 years ago
|
||
nsImageDocument.cpp (sr=rpotts)
nsHTMLImageElement.cpp (r=rpotts)
-- rick
Comment 45•24 years ago
|
||
nsImageFrame.[h|cpp] r=waterson
Assignee | ||
Comment 46•24 years ago
|
||
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.
Assignee | ||
Comment 47•24 years ago
|
||
Assignee | ||
Comment 48•24 years ago
|
||
Assignee | ||
Comment 49•24 years ago
|
||
*** Bug 70942 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 50•24 years ago
|
||
its on on all platforms.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•