70938 - meta: Land new image library.

Status: VERIFIED FIXED

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

Description

[Stuart Parmenter]

tracking bug for bugs that are fixed by the new image library.

Comment 1

[Stuart Parmenter]

we will fix at least these bugs.

Comment 2

[Stuart Parmenter]

few more.

Comment 3

[Stuart Parmenter]

Attachment: patch to pull new code on windows

Comment 4

[Stuart Parmenter]

Attachment: patch to pull new code on unix

Comment 5

[Stuart Parmenter]

mac changes from saari will be here before we start building anything.

Comment 6

[Brian Ryner (not reading)]

r=bryner for both patches

Comment 7

[cls]

sr=cls

Comment 8

[Daniel (Leaf) Nunes]

sr=leaf for http://bugzilla.mozilla.org/showattachment.cgi?attach_id=27173

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: add --enable-new-cache and --enable-libpr0n to configure.in

Comment 10

[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

Comment 11

[Stuart Parmenter]

Attachment: content/html/document/src/nsImageDocument.cpp diff

Comment 12

[Stuart Parmenter]

argh, i removed the #define from the top of the diff in nsImageDocument.cpp

Comment 13

[Stuart Parmenter]

Attachment: layout/xul/base/src/nsImageBoxFrame.* diff

Comment 14

[Johnny Stenback (:jst)]

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.

Comment 15

[Stuart Parmenter]

Attachment: layout/html/base/src/nsImageFrame.* diff

Comment 16

[David Hyatt]

sr=hyatt for the nsImageBoxFrame changes

Comment 17

[Stuart Parmenter]

r= for dbaron's configure.in changes

Comment 18

[Stuart Parmenter]

Attachment: content\html\content\src diff

Comment 19

[Johnny Stenback (:jst)]

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

Comment 20

[Stuart Parmenter]

Attachment: gfx/* diffs

Comment 21

[Stuart Parmenter]

Attachment: unix makefile changes to build if USE_IMG2 is set

Comment 22

[Stuart Parmenter]

Attachment: new unix makefile changes

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 24

[Stuart Parmenter]

Attachment: new layout/xul/base/src/nsImageBoxFrame.* diff

Comment 25

[Stuart Parmenter]

Attachment: new layout/html/base/src/nsImageFrame.* diff

Comment 26

[Stuart Parmenter]

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

[Chris Waterson]

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

[cls]

r=cls on attach 27232

Comment 29

[Marc Attinasi]

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.

Comment 30

[Stuart Parmenter]

Attachment: newer layout/html/base/src/nsImageFrame.* diff

Comment 31

[Stuart Parmenter]

> 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

[Marc Attinasi]

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

[Jus]

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

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 35

[Stuart Parmenter]

Attachment: windows config.mak changes to add -D's to things if USE_IMG2 or MOZ_NEW_CACHE are defined

Comment 36

[Stuart Parmenter]

r=bryner for the windows -D changes

Comment 37

[Stuart Parmenter]

sr=cls for the -D changes

Comment 38

[cls]

sr=cls on attach 28166

Comment 39

[cls]

sr=cls on attach 27232

Comment 40

[Stuart Parmenter]

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?

Comment 41

[Stuart Parmenter]

Attachment: newer gfx diff

Comment 42

[Stuart Parmenter]

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

[Marc Attinasi]

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

Comment 44

[rpotts (gone)]

nsImageDocument.cpp (sr=rpotts)
nsHTMLImageElement.cpp (r=rpotts)

-- rick

Comment 45

[Chris Waterson]

nsImageFrame.[h|cpp] r=waterson

Comment 46

[Stuart Parmenter]

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.

Comment 47

[Stuart Parmenter]

Attachment: shorter gfx patch without the 37779 changes

Comment 48

[Stuart Parmenter]

Attachment: build by default on windows

Comment 49

[Stuart Parmenter]

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

Comment 50

[Stuart Parmenter]

its on on all platforms.

Comment 51

[Terri Preston]

Verified on all platforms now