190475 - [FIXr]Eliminate use of nsIPresContext in imageloader

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

This irks me.  It irks me greatly.  Especially the strong ref we hold -- that
causes reference cycles.

We should take an nsIDocument pointer in LoadImage and we should just save it as
a PRWord in the request object.  And under no circumstances should we pass it to
observers, ever.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Something like this

This lets my soul be at peace again

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: same thing as diff -w

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 113265 [details] [diff] [review]
same thing as diff -w

Stuart, the imagelib changes all need your moa, of course.... the other changes
are pretty small.

David, could you please pay particular attention to the
nsImageFrame/nsBulletFrame/nsImageBoxFrame usage of mPresContext? I think what
I did should be safe, but....

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: forgot to diff imgContainerGIF

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Add in this hunk (for review only)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: I mean this hunk....

Comment 7

[Stuart Parmenter]

Comment on attachment 113277 [details] [diff] [review]
I mean this hunk....

r=pavlov

Comment 8

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

Comment on attachment 113277 [details] [diff] [review]
I mean this hunk....

>Index: modules/libpr0n/decoders/gif/imgContainerGIF.cpp
> NS_IMETHODIMP imgContainerGIF::GetNumFrames(PRUint32 *aNumFrames)
>-{
>+{  

What?  Adding whitespace?  Undo that typo and sr=dbaron on the imgContainerGIF
changes.  (I haven't looked at the rest yet.)

Comment 9

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

Comment on attachment 113265 [details] [diff] [review]
same thing as diff -w

in imgRequestProxy.h (and the constructor in the .cpp), it looks like
you're still using |mContext| for the load group calls.  (Can you get
rid of it at some point?)  Should it really be a weak ref?  The comment
seems inaccurate.

nsImageDocument: do_QueryInterface should work, and in fact you
might end up with less generated code if you make the nsCOMPtr
anonymous, i.e., nsCOMPtr<nsISupports>(do_QueryInterface(this))


Why not fix imgILoader::LoadImageWithChannel


In nsHTMLImageElement::OnStartContainer, perhaps use early returns
instead of reindenting?

In nsHTMLImageElement::OnStartContainer, the t2p value is unused, so
just remove all the (incorrect anyway) code to get it.	(And why are we
*setting attributes*??)

However, in nsHTMLImageElement::OnStopDecode, I think you really need a
way to get to the *right* pres context (consider printing), and this
doesn't seem to do it.

I haven't reviewed the remainder of the patch in detail since this
seems like a major problem.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> Can you get rid of it at some point?

Hmm... lemme check with pav, but I suspect I can punt mRequest completely.  That
would be nice.  ;)

> nsImageDocument: do_QueryInterface should work,

Ambiguous inheritance from nsISupports, so it does not.  See bug 181387

> Why not fix imgILoader::LoadImageWithChannel

What is broken in it?

> the t2p value is unused

Good catch.

> And why are we *setting attributes*??

It's a hack.  ;)  This code only runs for things like |var foo = new Image();
foo.src = "url";|.  We set the attributes so that GetWidth/GetHeight will work,
since the image has no frame and people expect to be able to get the dimensions.

Once I move image loading to content so that the image content has access to the
imgIRequest object at all times this code can all go away (that will be bug 83774).

> get to the *right* pres context (consider printing)

This code only runs when Image objects are created by script (when loading via
the frame, the content node is not even a listener right now).  Printing does
not run scripts.

Again, this code will be removed in bug 83774.

Perhaps I should fix this and bug 83774 in a single patch?  Or would that be too
big to review usefully?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: patch updated to dbaron's comments

Note that LoadImage and LoadImageWithChannel still need that aCX pointer
because they use it as a unique key for identifying groups of image loads that
belong to the same document.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: diff -w of the same

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 113299 [details] [diff] [review]
diff -w of the same

Changes from previous patch:

1) Eliminates mContext in imgRequestProxy completely (since it was now not
doing anything useful; the loadgroup does nothing with the context that is
relevant to images).

2) fixed whitespace thing

3) Fixed nsHTMLImageElement::OnStartContainer

4) Added some XXX comments in the right places.

Comment 14

[Stuart Parmenter]

Comment on attachment 113299 [details] [diff] [review]
diff -w of the same

r=pavlov

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 113299 [details] [diff] [review]
diff -w of the same

David, do you have any idea when you may get to this? (not trying to rush you;
just trying to budget time....)

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 113299 [details] [diff] [review]
diff -w of the same

David says he won't be able to get to this in the near future...
jst, can you sr?

Comment 17

[Johnny Stenback (:jst)]

Comment on attachment 113299 [details] [diff] [review]
diff -w of the same

sr=jst

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: updated to current tip...

Comment 19

[Boris Zbarsky [:bzbarsky]]

Fix checked in for 1.4a (I had to do a little more merging in imgGifDecoder2,
actually, so if this needs to be backed out use the bonsai output, not the last
patch I posted to do so).