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)

defect

Tracking

()

RESOLVED FIXED
mozilla1.4alpha

People

(Reporter: Biesinger, Assigned: Biesinger)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.9
Depends on: 104906
Target Milestone: mozilla0.9.9 → mozilla1.2
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
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)
Attached patch patch (obsolete) — Splinter Review
so, here's the patch. also makes users of gecko-content-viewers use
do_GetService to get the DocumentLoaderFactory.
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
Attached patch patch v2Splinter Review
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
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)
Attachment #115669 - Flags: review?(peterl)
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
Closed: 21 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.
Product: Core → Core Graveyard
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: