If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

meta: Land new image library.

VERIFIED FIXED in mozilla0.9

Status

()

Core
ImageLib
P1
normal
VERIFIED FIXED
17 years ago
15 years ago

People

(Reporter: Stuart Parmenter, Assigned: Stuart Parmenter)

Tracking

({meta, topperf})

Trunk
mozilla0.9
meta, topperf
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(17 attachments)

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
(Assignee)

Description

17 years ago
tracking bug for bugs that are fixed by the new image library.
(Assignee)

Comment 1

17 years ago
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
(Assignee)

Comment 2

17 years ago
few more.
(Assignee)

Updated

17 years ago
Blocks: 47659
(Assignee)

Comment 3

17 years ago
Created attachment 27173 [details] [diff] [review]
patch to pull new code on windows
(Assignee)

Comment 4

17 years ago
Created attachment 27174 [details] [diff] [review]
patch to pull new code on unix
(Assignee)

Comment 5

17 years ago
mac changes from saari will be here before we start building anything.
(Assignee)

Updated

17 years ago
Keywords: meta, topperf
r=bryner for both patches

Comment 7

17 years ago
sr=cls

Comment 8

17 years ago
sr=leaf for http://bugzilla.mozilla.org/showattachment.cgi?attach_id=27173
Created attachment 27190 [details] [diff] [review]
add --enable-new-cache and --enable-libpr0n to configure.in

Comment 10

17 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

17 years ago
Created attachment 27198 [details] [diff] [review]
content/html/document/src/nsImageDocument.cpp diff
(Assignee)

Comment 12

17 years ago
argh, i removed the #define from the top of the diff in nsImageDocument.cpp
(Assignee)

Comment 13

17 years ago
Created attachment 27201 [details] [diff] [review]
layout/xul/base/src/nsImageBoxFrame.* diff
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

17 years ago
Created attachment 27210 [details] [diff] [review]
layout/html/base/src/nsImageFrame.* diff

Comment 16

17 years ago
sr=hyatt for the nsImageBoxFrame changes
(Assignee)

Comment 17

17 years ago
r= for dbaron's configure.in changes
(Assignee)

Comment 18

17 years ago
Created attachment 27221 [details] [diff] [review]
content\html\content\src diff
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

17 years ago
Created attachment 27224 [details] [diff] [review]
gfx/* diffs
(Assignee)

Comment 21

17 years ago
Created attachment 27229 [details] [diff] [review]
unix makefile changes to build if USE_IMG2 is set
(Assignee)

Comment 22

17 years ago
Created attachment 27232 [details] [diff] [review]
new unix makefile changes
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

17 years ago
Created attachment 27235 [details] [diff] [review]
new layout/xul/base/src/nsImageBoxFrame.* diff
(Assignee)

Comment 25

17 years ago
Created attachment 27237 [details] [diff] [review]
new layout/html/base/src/nsImageFrame.* diff
(Assignee)

Comment 26

17 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

17 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

17 years ago
r=cls on attach 27232

Comment 29

17 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

17 years ago
Created attachment 27299 [details] [diff] [review]
newer layout/html/base/src/nsImageFrame.* diff
(Assignee)

Comment 31

17 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

17 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

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

Updated

17 years ago
No longer blocks: 63750
(Assignee)

Comment 35

17 years ago
Created attachment 28166 [details] [diff] [review]
windows config.mak changes to add -D's to things if USE_IMG2 or MOZ_NEW_CACHE are defined
(Assignee)

Updated

17 years ago
Depends on: 37779
(Assignee)

Comment 36

17 years ago
r=bryner for the windows -D changes
(Assignee)

Comment 37

17 years ago
sr=cls for the -D changes

Comment 38

17 years ago
sr=cls on attach 28166

Comment 39

17 years ago
sr=cls on attach 27232
(Assignee)

Comment 40

17 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

17 years ago
Created attachment 28170 [details] [diff] [review]
newer gfx diff
(Assignee)

Comment 42

17 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

17 years ago
nsImageFrame: sr=attinasi if you prefer that to my previous r=...

Comment 44

17 years ago
nsImageDocument.cpp (sr=rpotts)
nsHTMLImageElement.cpp (r=rpotts)

-- rick

Comment 45

17 years ago
nsImageFrame.[h|cpp] r=waterson
(Assignee)

Comment 46

17 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

17 years ago
Created attachment 28445 [details] [diff] [review]
shorter gfx patch without the 37779 changes
(Assignee)

Comment 48

17 years ago
Created attachment 28588 [details] [diff] [review]
build by default on windows
(Assignee)

Comment 49

17 years ago
*** Bug 70942 has been marked as a duplicate of this bug. ***

Updated

17 years ago
Depends on: 73740
(Assignee)

Comment 50

17 years ago
its on on all platforms.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 51

17 years ago
Verified on all platforms now
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.