Closed
Bug 97324
Opened 23 years ago
Closed 21 years ago
nsContentDLF.cpp should not use a static list of image types
Categories
(Core :: Layout: Images, Video, and HTML Frames, defect, P2)
Core
Layout: Images, Video, and HTML Frames
Tracking
()
RESOLVED
FIXED
mozilla1.4alpha
People
(Reporter: Biesinger, Assigned: Biesinger)
References
Details
Attachments
(2 files, 2 obsolete files)
41.81 KB,
patch
|
Details | Diff | Splinter Review | |
9.65 KB,
patch
|
peterlubczynski-bugs
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
content/build/nsContentDLF.cpp currently contains an array of supported Image Mime types. On your request, I file this as a bug on you. There should be some other way.
Updated•23 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.9
Updated•22 years ago
|
Component: ImageLib → Image: Layout
Assignee | ||
Comment 1•22 years ago
|
||
taking, I think I know how to fix this
Assignee: pavlov → cbiesinger
Status: ASSIGNED → NEW
Assignee | ||
Updated•22 years ago
|
Priority: -- → P2
Target Milestone: mozilla1.2alpha → mozilla1.4alpha
Comment 2•22 years ago
|
||
Take a look at this patch: http://bugzilla.mozilla.org/attachment.cgi?id=61711&action=view Decoders need to register in some fashion with the imgILoader service. People outside like nsContentDLF should simply ask the imgILoader service if it supports a certain mimetype and an answer should be given back.
Assignee | ||
Comment 3•22 years ago
|
||
yeah, well... the issue is this, basically: Currently, people look for a component with the contract id of @mozilla.org/content-viewer-factory/view;1?type=<the mime type> to check if Gecko can handle documents for a specific type... So my plan is to make them use a category instead (what some code parts already do). So imagelib would at registration time add a category entry for each mime type it supports. So the question is: pavlov, is it ok with you to do that (to call AddCategoryEntry for each supported mime type when the imagelib component gets registered)?
Assignee | ||
Comment 4•21 years ago
|
||
before I forget - the ART decoder in the commercial tree will need a change similar to the MNGDecoder change in my patch that I'll attach
Assignee | ||
Comment 5•21 years ago
|
||
(forgot the important part - it needs to do it for the mimetypes image/x-art as well as image/x-jg)
Assignee | ||
Comment 6•21 years ago
|
||
so, here's the patch. also makes users of gecko-content-viewers use do_GetService to get the DocumentLoaderFactory.
Assignee | ||
Updated•21 years ago
|
Attachment #113681 -
Flags: review?(jst)
Assignee | ||
Updated•21 years ago
|
Status: NEW → ASSIGNED
Comment 7•21 years ago
|
||
Comment on attachment 113681 [details] [diff] [review] patch Looks reasonable to me. r=jst You might be able to get suresh@netscape.com to help you out with the Netscape commercial changes when you land this.
Attachment #113681 -
Flags: review?(jst) → review+
Assignee | ||
Comment 8•21 years ago
|
||
Comment on attachment 113681 [details] [diff] [review] patch bz, do you have time to sr this? If not, just tell me, and I'll find another super-reviewer jst, thanks, I'll talk to him
Attachment #113681 -
Flags: superreview?(bzbarsky)
Comment 9•21 years ago
|
||
Comment on attachment 113681 [details] [diff] [review] patch >Index: docshell/base/nsDocShell.cpp >+ nsCOMPtr<nsICategoryManager> catMan(do_GetService(NS_CATEGORYMANAGER_CONTRACTID)); >+ nsXPIDLCString contractId; >+ catMan->GetCategoryEntry("Gecko-Content-Viewers", "text/html", getter_Copies(contractId)); What if the do_GetService fails? > // Note that we're always passing in "view" for the contractid above > // and to the docLoaderFactory->CreateInstance() at the end of this method. > // nsLayoutDLF makes the determination if it should be a "view-source" This comment no longer seems relevant. Is it? >Index: layout/build/nsContentDLF.cpp > // add the MIME types layotu can handle to the handlers category. "layout". I know you didn't write that, but fix it. Is there a reason you leave no unregistration proc for nsContentDLF (but have one for nsImageModule) ? >+/* To get a component that implements nsIDocumentLoaderFactory Start the comment like: /** * To get ... (javadoc syntax) sr=me with those nits
Attachment #113681 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Comment 10•21 years ago
|
||
>What if the do_GetService fails? Then you have a crash ;) Ok, I'll fix it >This comment no longer seems relevant. Is it? OK, I'll move the comment down to the mentioned CreateInstance line and adjust the wording. >"layout". I know you didn't write that, but fix it. ok >Is there a reason you leave no unregistration proc for nsContentDLF (but have >one for nsImageModule) ? The reason is that the current code doesn't have one either... and I'm lazy :) Do you want me to write one? I felt that the added code wouldn't be all that useful... would it ever get called? >Start the comment like:[...] >(javadoc syntax) sure, will do
Comment 11•21 years ago
|
||
> Do you want me to write one?
Please do. Dunno if it ever gets called, but it's bad not to clean up after
yourself....
Assignee | ||
Comment 12•21 years ago
|
||
ok, I'll have a new patch, with these additional changes: o) Make RegisterTypes only take two arguments, since that's all that's needed o) Fix a part of DocShell.cpp code that I missed earlier (also use do_GetService and requery category manager after reloading plugins) and with the issues mentioned by bz addressed. I'll attach it after I tested it.
No longer depends on: 104906
Assignee | ||
Comment 13•21 years ago
|
||
Attachment #113681 -
Attachment is obsolete: true
Assignee | ||
Comment 14•21 years ago
|
||
This is blocked by bug 193031, because the plugins code (after my patch) relies on the aPersist argument of AddCategoryEntry to work. I'm not requesting new r/sr because the previous ones should still apply.
Depends on: 193031
Assignee | ||
Comment 15•21 years ago
|
||
ok, ignore the modules/plugin/ part of the previous patch(es), this one is better - it unregisters the plugin types on shutdown. Now, I think there still may be edge cases when mozilla crashes at the right time that the category entries will be written to the registry, however I don't think that's much of a problem. that should be fixed once the bug this one depends on gets addressed.
Assignee | ||
Comment 16•21 years ago
|
||
Comment on attachment 115252 [details] [diff] [review] replacement patch for modules/plugin/ this is the result of my merge with the patch you sent me, with a few changes. hope it's ok with you.
Attachment #115252 -
Flags: review?(peterl)
Assignee | ||
Comment 17•21 years ago
|
||
Comment on attachment 115252 [details] [diff] [review] replacement patch for modules/plugin/ clearing review request, I am going to attach a new patch per emailed comments.
Attachment #115252 -
Flags: review?(peterl)
Assignee | ||
Comment 18•21 years ago
|
||
Attachment #115252 -
Attachment is obsolete: true
Assignee | ||
Updated•21 years ago
|
Attachment #115669 -
Flags: review?(peterl)
Comment 19•21 years ago
|
||
Comment on attachment 115669 [details] [diff] [review] better replacement patch for moduels/plugin/ r=peterl
Attachment #115669 -
Flags: review?(peterl) → review+
Assignee | ||
Updated•21 years ago
|
Attachment #115669 -
Flags: superreview?(jst)
Comment 20•21 years ago
|
||
Comment on attachment 115669 [details] [diff] [review] better replacement patch for moduels/plugin/ sr=jst
Attachment #115669 -
Flags: superreview?(jst) → superreview+
Assignee | ||
Comment 21•21 years ago
|
||
checked in
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 22•21 years ago
|
||
I just ntoiced a typo in my patch, for which I checked in this patch: --- modules/libpr0n/build/nsImageModule.cpp 27 Feb 2003 13:51:48 -0000 1.7 +++ modules/libpr0n/build/nsImageModule.cpp 27 Feb 2003 15:09:39 -0000 1.8 @@ -122,15 +122,15 @@ "image/pjpeg", "image/jpg", #endif #ifdef IMG_BUILD_bmp "image/x-icon", "image/bmp", #endif -#ifdef IMB_BUILD_png +#ifdef IMG_BUILD_png "image/png", "image/x-png", #endif #ifdef IMG_BUILD_xbm "image/x-xbitmap", "image/x-xbm", "image/xbm",
Comment 23•21 years ago
|
||
this caused a regression, see bug #195386 so if this goes on any branches (doesn't sound like it is planned to), we need to take the supplimentatl fix in that bug, too.
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
Product: Core Graveyard → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•