Closed
Bug 190475
Opened 22 years ago
Closed 22 years ago
[FIXr]Eliminate use of nsIPresContext in imageloader
Categories
(Core :: Graphics: ImageLib, defect, P1)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla1.4alpha
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(1 file, 7 obsolete files)
112.07 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•22 years ago
|
||
This lets my soul be at peace again
Assignee | ||
Comment 2•22 years ago
|
||
Assignee | ||
Comment 3•22 years ago
|
||
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)
Assignee | ||
Updated•22 years ago
|
Priority: -- → P1
Summary: Eliminate use of nsIPresContext in imageloader → [FIX]Eliminate use of nsIPresContext in imageloader
Target Milestone: --- → mozilla1.4alpha
Updated•22 years ago
|
Attachment #113265 -
Flags: review?(pavlov) → review+
Assignee | ||
Comment 4•22 years ago
|
||
Attachment #113264 -
Attachment is obsolete: true
Assignee | ||
Comment 5•22 years ago
|
||
Assignee | ||
Comment 6•22 years ago
|
||
Attachment #113276 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #113277 -
Flags: superreview?(dbaron)
Attachment #113277 -
Flags: review?(pavlov)
Comment 7•22 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-
Assignee | ||
Comment 10•22 years ago
|
||
> 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?
Assignee | ||
Comment 11•22 years ago
|
||
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
Assignee | ||
Comment 12•22 years ago
|
||
Assignee | ||
Comment 13•22 years ago
|
||
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•22 years ago
|
||
Comment on attachment 113299 [details] [diff] [review] diff -w of the same r=pavlov
Attachment #113299 -
Flags: review?(pavlov) → review+
Assignee | ||
Comment 15•22 years ago
|
||
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....)
Assignee | ||
Comment 16•22 years ago
|
||
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 17•22 years ago
|
||
Comment on attachment 113299 [details] [diff] [review] diff -w of the same sr=jst
Attachment #113299 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 18•22 years ago
|
||
Attachment #113277 -
Attachment is obsolete: true
Attachment #113298 -
Attachment is obsolete: true
Attachment #113299 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Summary: [FIX]Eliminate use of nsIPresContext in imageloader → [FIXr]Eliminate use of nsIPresContext in imageloader
Assignee | ||
Comment 19•22 years ago
|
||
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
Closed: 22 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•