[FIXr]Eliminate use of nsIPresContext in imageloader

RESOLVED FIXED in mozilla1.4alpha

Status

()

Core
ImageLib
P1
normal
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

Trunk
mozilla1.4alpha
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

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.
Created attachment 113264 [details] [diff] [review]
Something like this

This lets my soul be at peace again
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....
Attachment #113265 - Flags: superreview?(dbaron)
Attachment #113265 - Flags: review?(pavlov)
Priority: -- → P1
Summary: Eliminate use of nsIPresContext in imageloader → [FIX]Eliminate use of nsIPresContext in imageloader
Target Milestone: --- → mozilla1.4alpha

Updated

16 years ago
Attachment #113265 - Flags: review?(pavlov) → review+
Created attachment 113275 [details] [diff] [review]
forgot to diff imgContainerGIF
Attachment #113264 - Attachment is obsolete: true
Created attachment 113276 [details] [diff] [review]
Add in this hunk (for review only)
Created attachment 113277 [details] [diff] [review]
I mean this hunk....
Attachment #113276 - Attachment is obsolete: true
Attachment #113277 - Flags: superreview?(dbaron)
Attachment #113277 - Flags: review?(pavlov)

Comment 7

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

r=pavlov
Attachment #113277 - Flags: review?(pavlov) → review+
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.)
Attachment #113277 - Flags: superreview?(dbaron) → superreview+
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.
Attachment #113265 - Flags: superreview?(dbaron) → superreview-
> 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?

Created attachment 113298 [details] [diff] [review]
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.
Attachment #113265 - Attachment is obsolete: true
Attachment #113275 - Attachment is obsolete: true
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.
Attachment #113299 - Flags: superreview?(dbaron)
Attachment #113299 - Flags: review?(pavlov)

Comment 14

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

r=pavlov
Attachment #113299 - Flags: review?(pavlov) → review+
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 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?
Attachment #113299 - Flags: superreview?(dbaron) → superreview?(jst)
Comment on attachment 113299 [details] [diff] [review]
diff -w of the same

sr=jst
Attachment #113299 - Flags: superreview?(jst) → superreview+
Created attachment 115563 [details] [diff] [review]
updated to current tip...
Attachment #113277 - Attachment is obsolete: true
Attachment #113298 - Attachment is obsolete: true
Attachment #113299 - Attachment is obsolete: true
Summary: [FIX]Eliminate use of nsIPresContext in imageloader → [FIXr]Eliminate use of nsIPresContext in imageloader
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).
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.