nsContentDLF.cpp should not use a static list of image types

RESOLVED FIXED in mozilla1.4alpha

Status

()

Core
Layout: Images
P2
normal
RESOLVED FIXED
16 years ago
15 years ago

People

(Reporter: Biesinger, Assigned: Biesinger)

Tracking

Trunk
mozilla1.4alpha
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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

16 years ago
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.9
Blocks: 41333

Updated

16 years ago
Depends on: 104906
Target Milestone: mozilla0.9.9 → mozilla1.2

Updated

16 years ago
Component: ImageLib → Image: Layout
taking, I think I know how to fix this
Assignee: pavlov → cbiesinger
Status: ASSIGNED → NEW
Priority: -- → P2
Target Milestone: mozilla1.2alpha → mozilla1.4alpha

Comment 2

15 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.
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)?
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
(forgot the important part - it needs to do it for the mimetypes image/x-art as
well as image/x-jg)
Blocks: 192023
Created attachment 113681 [details] [diff] [review]
patch

so, here's the patch. also makes users of gecko-content-viewers use
do_GetService to get the DocumentLoaderFactory.
Attachment #113681 - Flags: review?(jst)
Status: NEW → ASSIGNED
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+
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 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+
>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
> 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....
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
Created attachment 114251 [details] [diff] [review]
patch v2
Attachment #113681 - Attachment is obsolete: true
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
Created attachment 115252 [details] [diff] [review]
replacement patch for modules/plugin/

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.
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)
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)
Created attachment 115669 [details] [diff] [review]
better replacement patch for moduels/plugin/
Attachment #115252 - Attachment is obsolete: true
Attachment #115669 - Flags: review?(peterl)

Comment 19

15 years ago
Comment on attachment 115669 [details] [diff] [review]
better replacement patch for moduels/plugin/

r=peterl
Attachment #115669 - Flags: review?(peterl) → review+
Attachment #115669 - Flags: superreview?(jst)
Comment on attachment 115669 [details] [diff] [review]
better replacement patch for moduels/plugin/

sr=jst
Attachment #115669 - Flags: superreview?(jst) → superreview+
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
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",
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.
You need to log in before you can comment on or make changes to this bug.