283125 - [FIX]Add a service to allow checking whether a type is supported by Gecko

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

People shouldn't have to depend on the Gecko-Content-Viewers category...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch

Proposed patch.

If you have better interface name suggestions, please let me know.  Same for
the contractid.

Also let me know whether I should get separate review on each of the embedding
changes (they can be landed one my one once the main patch is in).  They're
really simple enough that a once-over from people on the CC list should
suffice...

Comment 2

[Boris Zbarsky [:bzbarsky]]

ccing qnx folks too.

Comment 3

[timeless]

Comment on attachment 175102 [details] [diff] [review]
Patch

>Index: docshell/base/nsIWebNavigationInfo.idl
document that this method reloads plugins?

>+  boolean isTypeSupported(in ACString aType, in nsIWebNavigation aWebNav);

>Index: docshell/base/nsWebNavigationInfo.cpp
>+  if (!*aIsTypeSupported) {
>+    // Try reloading plugins in case they've changed.

if plugins are disabled, do we still want to do this?

>+    nsCOMPtr<nsIPluginManager> pluginManager =
>+      do_GetService("@mozilla.org/plugin/manager;1");

Comment 4

[Boris Zbarsky [:bzbarsky]]

> document that this method reloads plugins?

That seems like an implementation detail, no?  Why would this matter to callers?

> if plugins are disabled, do we still want to do this?

Once we sort out what the "plugins are disabled" thing on docshell does, we
should probably take it into account here, yes (probably by looking at the
contractid we got and comparing it against the plugin factory contractid).  But
we need to sort it out first.  For now I'm just keeping the functionality the
same while centralizing the code.

Comment 5

[timeless]

maybe the caller doesn't want the perf overhead of plugins being loaded at this
time, or is afraid of leaving them loaded at all...

Comment 6

[Boris Zbarsky [:bzbarsky]]

OK.  I suppose that we should document that this may trigger registration of
internal handlers (such as plugins).

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 175102 [details] [diff] [review]
Patch

docshell/base/nsWebNavigationInfo.h
+class nsCString;

use nsStringFwd.h for this..

docshell/base/nsWebNavigationInfo.cpp
+  nsresult rv = NS_OK;
+  mCategoryManager = do_GetService(NS_CATEGORYMANAGER_CONTRACTID, &rv);

no need to initialize rv, do_GS will always set it


+  NS_PRECONDITION(aIsTypeSupported, "null out param?");

yeah well... people will notice it very fast if the out param is null ;)

+  if (!value.IsEmpty()) {
+    docLoaderFactory = do_GetService(value.get());

is that if needed?

docshell/build/nsDocShellCID.h
+ * @implements nsIWebNavigationInfo

do we use @implements anywhere else? at least for such comments that I wrote, I
used a different format ;) (f.ex. nsITransfer.idl)

stopped at embedding/browser/activex/src/control/WebBrowserContainer.cpp,  will
continue later

Comment 8

[Boris Zbarsky [:bzbarsky]]

> is that if needed?

Yes, because GetCategoryEntry could have returned NS_ERROR_NOT_AVAILABLE, in
which case the string is actually null, and passing null to GetService crashes.

> do we use @implements anywhere else?

bsmedberg recommended I do this.  If you have a preferred other method, I'd be
happy to use that, or both.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #8)
> bsmedberg recommended I do this.  If you have a preferred other method, I'd be
> happy to use that, or both.

hm, there is not really a common pattern for
http://lxr.mozilla.org/seamonkey/source/xpcom/build/nsXPCOMCID.h#41,
http://lxr.mozilla.org/seamonkey/source/xpfe/components/nsXPFEComponentsCID.h#40
and http://lxr.mozilla.org/seamonkey/source/uriloader/base/nsITransfer.idl#92

choose whatever you prefer :-)

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 175102 [details] [diff] [review]
Patch

xpfe/browser/src/nsBrowserInstance.cpp
+  if (!aContentType)
+    return NS_ERROR_WONT_HANDLE_CONTENT;

can we instead document that null is disallowed? (or if you don't want to spend
time on nsIContentHandler, just crash if null is passed). this null check seems
entirely pointless to me. 

+  PRBool typeSupported = PR_FALSE;
+  rv = webNavInfo->IsTypeSupported(nsDependentCString(aContentType), nsnull,
+				    &typeSupported);
+  NS_ENSURE_SUCCESS(rv, rv);

I'd think that a successful return should always set the out param (so the
initialization would be unneeded)....

Comment 11

[Boris Zbarsky [:bzbarsky]]

biesi had the idea of returning not a boolean, but an enum saying what sort of
support we have (with 0 being "no support" and nonzero values indicating whether
it's a "nsContentDLF" or a "plugin" or a "something else" or something like that).

Darin, thoughts?  Ben, that would allow us to remove some existing
Gecko-Content-Viewers munging in the Firefox ui, I think.

Comment 12

[Darin Fisher]

Comment on attachment 175102 [details] [diff] [review]
Patch

>Index: docshell/base/nsIWebNavigationInfo.idl

>+/**
>+ * The nsIWebNavigationInfo interface exposes a way to get information
>+ * on the capabilities of Gecko webnavigation objects.
>+ *
>+ * @status UNDER_REVIEW
>+ */
>+
>+[scriptable, uuid(62a93afb-93a1-465c-84c8-0432264229de)]

nit: kill the extra newline between comment and "[scriptable..."


>Index: docshell/base/nsWebNavigationInfo.h

>+#ifndef nsWebNavigationInfo__h
>+#define nsWebNavigationInfo__h

style nit: convention for this module is nsFooBar_h__


>+public:
...
>+  ~nsWebNavigationInfo() {}

nit: non-virtual = no inheritance.. let's make this private.


>+#endif
>+

nit: kill extra newline at end of file.


>Index: docshell/base/nsWebNavigationInfo.cpp

>+nsWebNavigationInfo::Init()
>+{
>+  nsresult rv = NS_OK;
>+  mCategoryManager = do_GetService(NS_CATEGORYMANAGER_CONTRACTID, &rv);

nit: no reason to initialize rv before calling do_GetService.


>+  if (!*aIsTypeSupported) {

nit: early return if (*aIsTypeSupported) to reduce indenting.


>Index: docshell/base/nsDSURIContentListener.cpp
...
> nsDSURIContentListener::Init() 
> {
>     nsresult rv = NS_OK;
>+    mNavInfo = do_GetService(NS_WEBNAVIGATION_INFO_CONTRACTID, &rv);

nit: again, no reason to initialize rv.


>Index: xpfe/browser/src/nsBrowserInstance.cpp

>+    do_GetService("@mozilla.org/webnavigation-info;1", &rv);

we have that nifty #define in nsDocShellCID.h -- use it?


>Index: browser/components/nsBrowserContentHandler.js

>-      var catMan = Components.classes["@mozilla.org/categorymanager;1"]
>-                             .getService(nsICategoryManager);
>-      var entry = catMan.getCategoryEntry("Gecko-Content-Viewers",
>-                                          contentType);
>+      var webNavInfo = Components.classes["@mozilla.org/webnavigation-info;1"]
>+          .getService(nsIWebNavigationInfo);

the prevailing style is to line up the dots.  i know it's wierd,
but that's what people tend to do.


sr=darin

Comment 13

[Boris Zbarsky [:bzbarsky]]

> we have that nifty #define in nsDocShellCID.h -- use it?

Is that publicly exported?  If so, gladly.

What about comment 11?  I'd like to get that sorted out before I land anything
here...

Comment 14

[Darin Fisher]

(In reply to comment #13)
> > we have that nifty #define in nsDocShellCID.h -- use it?
> 
> Is that publicly exported?  If so, gladly.

yeah, maybe it should be.  or some such header.


> What about comment 11?  I'd like to get that sorted out before I land anything
> here...

i like it.  build in flexibility from the start.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Address comments, implement comment 11

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 175363 [details] [diff] [review]
Address comments, implement comment 11

docshell/base/nsIWebNavigationInfo.idl
+   * Returned by isTypeSupported to indicate that a type is supported by the
+   * HTML/XML/CSS renderer in Gecko.
+   */
+  const unsigned long INTERNAL = 1;

it seems to me that this is somewhat underdefined... maybe better would be to
split this into HTMLMARKUP and XMLMARKUP, which are much easier to define (type
of parser moz uses). of course that's not so easy to implement, unless you use
.EqualsLiteral("text/html").

hm, what about plain text...


I don't know, maybe it's ok. I guess most code won't care too much about
this...

+   * Returned by isTypeSupported to indicate that a type is supported as an
+   * image.

"raster image", maybe? it returns false for SVG.

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

I meant to say "returns NATIVE for SVG"

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 175363 [details] [diff] [review]
Address comments, implement comment 11

Yea, I agree INTERNAL is underdefined.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Without INTERNAL

I realized none of the callers actually used INTERNAL, so it's safe to take it
out for the time being (and put it, or better-defined things in later, when we
can do said better defining).

The only other change here from the previous patch is that I patched
docshell/build/Makefile.in to actually REQUIRE imglib2...

As for SVG, I would rather not get involved in it here; the question of whether
SVG is an image or a document is still open, may end up depending on the actual
MIME type of the SVG in question, etc....

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 175391 [details] [diff] [review]
Without INTERNAL

r=biesi

Comment 21

[Darin Fisher]

Comment on attachment 175391 [details] [diff] [review]
Without INTERNAL

looks good, sr=darin

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to tip

Comment 23

[Boris Zbarsky [:bzbarsky]]

Fix checked in (with some build bustage fixups).