40772 - Content-type based decisions are hardcoded

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect, P3)

Description

[Bruce Mitchener]

http://lxr.mozilla.org/seamonkey/source/docshell/base/nsDSURIContentListener.cpp#157
http://lxr.mozilla.org/seamonkey/source/embedding/browser/webBrowser/nsWBURIContentListener.cpp#141
http://lxr.mozilla.org/seamonkey/source/embedding/browser/gtk/src/GtkMozEmbedChrome.cpp#574

Could these all use a category?

This removes the hardcoded nature of the check, and allows for people shipping
components that implement new doctypes to get the behavior they like.

Comment 1

[Christopher Blizzard (:blizzard)]

I know that in the gtk code I had that in there as a work around until some
other bit of code was finished ( but I don't remember what. )

I'm not exactly sure what you're asking here.  Can you elaborate?

Comment 2

[Bruce Mitchener]

blizzard, did you still need an explanation after our discussion on #mozilla?

Comment 3

[Christopher Blizzard (:blizzard)]

what we talked about has drooled out of one of my ears, yeah.

Comment 4

[Paul Wyskoczka]

Updating QA Contact

Comment 5

[Judson Valeski]

bruce, are you suggesting that this list be updated by firing category
notifications for content type handling? Each "app" would fire off the
notification for the said category, and anyone listening could add a mime-type
to the list? How would they access the "list"? would they GetService() on the
nsIURILoader?

I'm thinking that rather than using a category and callback, that
nsIURIContentListener just has the ability to add/remove and enumerate it's list
dynamically.

Thoughts?

Also, this list is a statement about what mime-types the app can render. If, in
an embedding scenario, the embedding app can handle other types, shouldn't it
just impl nsIURIContentListener and hook itself up to the chain of
nsIURIContentListeners?

Comment 6

[Judson Valeski]

I misspoke, this facility describes what a "window" can handle, not what the
"app" can handle. All my questions remain however.

Comment 7

[Judson Valeski]

Attachment: diffs to layout to add types to a category

Comment 8

[Judson Valeski]

Attachment: changes to docshell to get it to dynamically determine it's abilities.

Comment 9

[Judson Valeski]

so, here's a hacky (not compiled) approach to fixing this. the code in the 
docshell can be moved around and optimized, but the point is made. the code in 
layout stinks but gets the job done.

some questions:
1. is this the "right" categorization ("viewers"->"mimetype")?
2. currently layout doesn't register any of it's viewers w/ contract ids; doh! 
what would those contract ids look like. once we have those, I can add the 
contract id as the entry value in the category manager (not that that does 
anything for us currently.
3. can I assume that doctype handling capabilities are known at runtime. If so, 
I can cache the mime type list in the docshell and only do this once. I'd rather 
not get/iterate this list everytime CanHandleContent() is called (a lot).

I'd like to stomp out nsLayoutLDF.cpp altogether, and I spent all day yesterday 
trying to do so. it makes some implied assumptions about what's registered, 
again by cid only, way over in mozilla/content/build/nsContentLDF.cpp, and is 
adding a funky layer of cid aggregation that is making me queezy.

Comment 10

[Judson Valeski]

Attachment: updated docshell patch

Comment 11

[Judson Valeski]

new patch addresses #3 completely. it caches the catmanager, and allows for 
dynamic type updating (it just lets the cat manager deal w/ it).

Comment 12

[Judson Valeski]

Attachment: updated patch to layout

Comment 13

[Judson Valeski]

I'm looking for r and sr on these two patches to close this bug out.


04/16/01 06:06 layout patch -
* updates the doc loader factory to use NS_IMPL_ISUPPORTS macro
* consolodates the category manager registration w/ the doc type contract id 
registration.

04/13/01 09:58 docshell patch

Comment 14

[rpotts (gone)]

These changes look good to me...  (r=rpotts)

Are you going to open up another bug to track the remaining work to remove the
nsIDocumentLoaderFactory all together?  Or at least to have the 'value' returned
from the mime-type catagory be the contract-id of the factory used for creation...

-- rick

Comment 15

[vidur (gone)]

You should get Heikki, as one of the owners of the module, to review the
layoutDLF changes.

I'm not sure if caching the category manager in the URI
content listener represents much of an improvement in performance - how often is
CanHandleContent actually called? I guess there's no harm in doing the caching,
but I don't see a strong reason for it.

Assuming Heikki's r= on the layout changes and reasonable testing, sr=vidur.

Comment 16

[Judson Valeski]

CanHandle() doesn't get called too often (top level loads only (link click, 
urlbar). I'm caching because it's cheap to cache here and there's *some* win.

Comment 17

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Typo in comment in nsLayoutDLF.cpp. Otherwise looks good to me.

I don't understand vidur's comment about me being layout's module owner, though.
I am the default owner of Bugzilla XML component, but I am not even sure if I am
the (code level) XML module owner. I am certainly not a module owner (or peer)
of layout or content as a whole.

Anyway, do we now really have things in place so that folks can plug in their
own components to handle other mime types, like text/sgml, without changing any
Mozilla code, and even override our handlers? I know the DocZilla folks (for
example) have wanted this functionality for a long time but I never heard of it
working...

Comment 18

[Judson Valeski]

this doesn't affect our ability to plugin new viewers or doctypes, and I'm not 
up to speed on those capabilities.

Comment 19

[Judson Valeski]

fix is in. thanks all.

I'm filing a bug on the DocLoaderFactory stuff.

Comment 20

[Michael Dunn]

Correction: Changing QA contact for the Embed API bugs to David Epstein.

Comment 21

[David Epstein]

verified.