Closed Bug 283125 Opened 20 years ago Closed 20 years ago

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

Categories

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

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.8beta2

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(1 file, 3 obsolete files)

People shouldn't have to depend on the Gecko-Content-Viewers category...
Attached patch Patch (obsolete) — Splinter Review
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...
Attachment #175102 - Flags: superreview?(darin)
Attachment #175102 - Flags: review?(cbiesinger)
ccing qnx folks too.
Priority: -- → P1
Summary: Add a service to allow checking whether a type is supported by Gecko → [FIX]Add a service to allow checking whether a type is supported by Gecko
Target Milestone: --- → mozilla1.8beta2
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");
> 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.
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...
OK.  I suppose that we should document that this may trigger registration of
internal handlers (such as plugins).
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
> 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.
(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 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)....
Attachment #175102 - Flags: review?(cbiesinger) → review+
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 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
Attachment #175102 - Flags: superreview?(darin) → superreview+
> 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...
(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.
Attachment #175363 - Flags: superreview?(darin)
Attachment #175363 - Flags: review?(cbiesinger)
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.
Attachment #175363 - Flags: review?(cbiesinger) → review+
I meant to say "returns NATIVE for SVG"
Comment on attachment 175363 [details] [diff] [review]
Address comments, implement comment 11

Yea, I agree INTERNAL is underdefined.
Attachment #175363 - Flags: superreview?(darin) → superreview-
Attached patch Without INTERNAL (obsolete) — Splinter Review
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....
Attachment #175391 - Flags: superreview?(darin)
Attachment #175391 - Flags: review?(cbiesinger)
Comment on attachment 175391 [details] [diff] [review]
Without INTERNAL

r=biesi
Attachment #175391 - Flags: review?(cbiesinger) → review+
Comment on attachment 175391 [details] [diff] [review]
Without INTERNAL

looks good, sr=darin
Attachment #175391 - Flags: superreview?(darin) → superreview+
Attached patch Merged to tipSplinter Review
Attachment #175102 - Attachment is obsolete: true
Attachment #175363 - Attachment is obsolete: true
Attachment #175391 - Attachment is obsolete: true
Fix checked in (with some build bustage fixups).
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: