If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

make uriloader more doxygen-friendly

RESOLVED FIXED

Status

Core Graveyard
File Handling
RESOLVED FIXED
14 years ago
a year ago

People

(Reporter: Biesinger, Assigned: Biesinger)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

 
Created attachment 145426 [details] [diff] [review]
patch
Assignee: file-handling → cbiesinger
Status: NEW → ASSIGNED
Attachment #145426 - Flags: superreview?(darin)
Attachment #145426 - Flags: review?(bzbarsky)
Comment on attachment 145426 [details] [diff] [review]
patch

>Index: base/nsIContentHandler.idl
>   void handleContent(in string aContentType,
>                      in string aCommand,
>                      in nsISupports aWindowContext,

Mind documenting aCommand and aWindowContext while you're here?  If you can't
figure out what they are, ping me and we can try to work it out.

r+sr=bzbarsky with that.
Attachment #145426 - Flags: superreview?(darin)
Attachment #145426 - Flags: superreview+
Attachment #145426 - Flags: review?(bzbarsky)
Attachment #145426 - Flags: review+
suggestion for windowcontext:
@param aWindowContext Window context, implementing nsIInterfaceRequestor, used
to get things like the current nsIDOMWindow for this request.

(sidenote... why is the type of this argument not nsIInterfaceRequestor then? oh
well)
Probably so you can pass it around with minimal dependencies... but
nsIInterfaceRequestor is in xpcom-land.  Can't we change this api to take that?
 Or are we being consistent with all the frozen apis taking nsISupports at this
point?  :(
(In reply to comment #4)
>  Or are we being consistent with all the frozen apis taking nsISupports at this
> point?  :(

I vote for being sane :)

the apis that I'm aware of that use a window context are not frozen (IURILoader,
IExternalHelperAppService). nsIURIContentListener, which you might suspect of
using a window context, does not.

OK... I may regret this, but I'll make a patch to change the type of this
argument, and while I'm at it remove aCommand as that's a constant of "view"
apparently. If someone sees a use-case for the "command" argument, speak now or
forever hold your peace. I hope all people who care about this are cc'd.
OS: Linux → All
Hardware: PC → All

Comment 6

14 years ago
didn't (doesn't?) gecko use the command parameter to distinguish between "view"
and "view-source"? I thought they were passing view source around in this
parameter at one point in time....
Scott, that used to be the case before view-source became a protocol handler
(bug 66334, April 2001 or so).  At this point, it's unused inside Gecko.
Created attachment 145436 [details] [diff] [review]
patch v2

remove aCommand, change type of aWindowContext to nsIInterfaceRequestor (on
nsIURILoader, nsIContentHandler and nsIExternalHelperAppService)

I'd have added a comment about nsIContentHandler in general, but I don't really
know what it's supposed to be used for... and how it conceptually differs from
nsIURIContentListener.
Attachment #145426 - Attachment is obsolete: true
Attachment #145436 - Flags: review?(bzbarsky)
The major differences between nsIContentHandler and nsIURIContentListener:


1) A handler can only explicitly register for a particular MIME type (unlike a
   listener, which can register itself with the URILoader as a listener to be
   tried for every load).
2) A handler has two options: Don't handle the content, or handle it.  If it
   handles it, it just has to do its own magic; it can't return a stream
   listener to get data pumped into it.  Typically, a handler will actually
   create a new connection to get the resource or something.

Frankly, the more I think about nsIContentHandler the more it seems like a
vestigial remnant of how things used to work....

Comment 10

14 years ago
I believe nsIContentHandler is used so we can open particular windows based on
the content type. i.e if no content listeners accept the content, then we look
for a content handler and if we find it, the content handler knows how to bring
up the right chrome window (mail, browser, aim, etc.) and then load the url in
that window....
Yes, that's what it's used for.  I think those use cases could use
nsIURIContentListener just as easily, though (possibly better; it wouldn't
require reloading the content).

Perhaps this part of the discussion should move to n.p.m.embedding?

Comment 12

14 years ago
nsIContentHandler does look to be pretty redundant.  Would be nice to have
rpotts' input on this :-/

Comment 13

14 years ago
Comment on attachment 145436 [details] [diff] [review]
patch v2

so, nsIContentHandler now passes a nsIInterfaceRequestor, but
nsIURIContentListener does not have a similar parameter.  that might make it
difficult to eliminate nsIContentHandler.
Comment on attachment 145436 [details] [diff] [review]
patch v2

>Index: uriloader/base/nsIContentHandler.idl
>+   * Failure of this method will cause the request to be
>+   * cancelled.

Unless the failure code is NS_ERROR_WONT_HANDLE_CONTENT

>Index: uriloader/base/nsIURILoader.idl

>+   *        to get at the progress event sink interface.
>+   *        <b>Must not be null!</b>

Why not?  I don't see anything wrong with passing null if there is no window
context....  in fact, we should handle that fine, no?

It looks like the only nsIContentHandler that uses the window context is
bookmarks.  Does it really need that information?
Attachment #145436 - Flags: review?(bzbarsky) → review+
(In reply to comment #14)
> Why not?  I don't see anything wrong with passing null if there is no window
> context....  in fact, we should handle that fine, no?

We don't handle it. See nsDocumentOpenInfo::Open
  // ask our window context if it has a uri content listener...
  m_contentListener = do_GetInterface(m_originalContext, &rv);
  if (NS_FAILED(rv)) return rv;

So when we can't get a content listener, and if the context is null it's
guaranteed we can't get one, this function will return before calling AsyncOpen.

It should be handled, maybe. But I was just trying to document as-yet
undocumented preconditions of this function.

>It looks like the only nsIContentHandler that uses the window context is
>bookmarks.  Does it really need that information?

http://lxr.mozilla.org/mozilla/source/xpfe/browser/src/nsBrowserInstance.cpp#795
uses it as well... and
http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/src/nsAddressBook.cpp#2043
and probably others. but the C++ implementations don't really need any changes,
because do_GetInterface takes an nsISupports pointer.
> We don't handle it.

In that case the window context passed to nsIContentHandler will never be null,
will it?

I guess that can be a follow-up bug, though.

The browser instance user seems bogus, but the address book user is
legitimate... Let's do a followup on nsIContentHandler where we look at its
users.  I feel that using nsIContentHandler for anything that may return data is
just evil, and maybe we can modify this code to enforce that (at least via
asserts)....  But that's a separate bug.
(In reply to comment #16)
> In that case the window context passed to nsIContentHandler will never be null,
> will it?

Indeed, in the current implementation it will not be. But it would be nice if
uriloader didn't require a window context, and I want to allow for that.

> I guess that can be a follow-up bug, though.

yeah, I'd prefer that

note that the scope of this bug already widened from "make some stylistic
comment changes" to "change all implementations of nsIContentHandler in the tree" :)

> The browser instance user seems bogus, but the address book user is
> legitimate... Let's do a followup on nsIContentHandler where we look at its
> users.  I feel that using nsIContentHandler for anything that may return data is
> just evil, and maybe we can modify this code to enforce that (at least via
> asserts)....  But that's a separate bug.

you things like the xpinstall contenthandler? sure. I'll file a followup bug.
> note that the scope of this bug already widened

Noted.  ;)

> you things like the xpinstall contenthandler?

For a glaring example, yes.
Comment on attachment 145436 [details] [diff] [review]
patch v2

(I'll make the first change mentioned in comment 14)
Attachment #145436 - Flags: superreview?(darin)

Comment 20

14 years ago
Comment on attachment 145436 [details] [diff] [review]
patch v2

>Index: uriloader/base/nsIContentHandler.idl

>+   * @throw NS_ERROR_WONT_HANDLE_CONTENT Indicates that this handler does not
>+   *        want to handle this content. A different way for handling this
>+   *        content should be tried.
>+   */

shouldn't this be "@throws" instead of "@throw"?  unfortunately, it looks like
using "@throws" would through off your indentation by one character! :-/

a newline between the "@param" and "@throws" sections might be good :)


>Index: uriloader/base/nsIURILoader.idl

>+   *        <b>Must not be null!</b>

ugh, i'm not a big fan of markup in IDL documentation, but short of
some other way...

>Index: uriloader/exthandler/nsMIMEInfoImpl.h

>+    nsCStringArray         mExtensions; ///< array of file extensions associated w/ this MIME obj
>+    nsString               mDescription; ///< human readable description
>+    PRUint32               mMacType, mMacCreator; ///< Mac file type and creator

is this ("///<") some kind of doxygen commenting format?


sr=darin
Attachment #145436 - Flags: superreview?(darin) → superreview+
thanks for the fast sr!

> shouldn't this be "@throws" instead of "@throw"? 
from http://www.stack.nl/~dimitri/doxygen/commands.html#cmdthrow :
 	
\throw <exception-object> { exception description }
Synonymous to \exception (see section \exception).

Note: the tag \throws is a synonym for this tag.

> is this ("///<") some kind of doxygen commenting format?

yeah, it is - used for putting documentation after the member it describes.

I'll make the other change
filed Bug 240731 for allowing passing null to nsIURILoader::Open and Bug 240732
to review nsIContentHandler implementations.
checked in with the mentioned changes made.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 24

14 years ago
Christian, this change confused me:

-    rv = loader->OpenURI(channel, PR_TRUE, listenerRef);
+    rv = loader->OpenURI(channel, PR_TRUE, listener);

listener gets released several lines before you use it:

http://lxr.mozilla.org/mozilla/source/xpfe/components/xremote/src/XRemoteService.cpp#815

Do we really want to be using this variable after it was released?
"oops"
well, this is not what I intended...

In the end it doesn't matter, because the listenerRef COMPtr will keep a reference:
811     nsCOMPtr<nsISupports> listenerRef;
812     listenerRef = do_QueryInterface(NS_STATIC_CAST(nsIURIContentListener *,
813                            listener));

But there's no real point in having both the listenerRef variable and the
listener one... I'll attach a patch to make this code clearer
Created attachment 146438 [details] [diff] [review]
xremotelistener changes
Attachment #146438 - Flags: superreview?(mscott)
Attachment #146438 - Flags: review?(blizzard)

Updated

14 years ago
Attachment #146438 - Flags: superreview?(mscott) → superreview+
actually wait, NS_RELEASE sets the pointer to null. a null context makes OpenURI
fail. so this patch is rather important for this code to work

thanks for finding this mscott!

Comment 28

14 years ago
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/extensions/irc/js/lib/chatzilla-service.js&rev=1.33

Thanks guys, for the heads up on the [not backwards-compatible] change. We now
have ChatZilla not working on *any* official Mozilla.org release because of this
change.

I hope, for your sake, I can work out how to tell what parameter list the
function is getting. This code is meant to work on *all* Mozilla builds since 1.0.
James, you may be in more trouble yet, since the api you are using may end up
being removed altogether.

> I hope, for your sake

Is that a threat?  I'm sorry, but did I miss something important here?

> I can work out how to tell what parameter list the function is getting.

instanceof will help you out, I would wager.  For now.

> This code is meant to work on *all* Mozilla builds since 1.0.

In that case, why are you using unfrozen apis?  I suppose if we eliminate
nsIContentHandler we'll move you over to the frozen (since 1.0!)
nsIURIContentListener, but you may want to consider doing that yourself.
I do agree that we should have dropped you a mail about this change, by the way,
though it's getting rather hard to keep track of all the various Mozilla.org
projects using unfrozen apis and all having totally different policies about
where and when and how they want to work.

Comment 31

14 years ago
(In reply to comment #29)
> James, you may be in more trouble yet, since the api you are using may end up
> being removed altogether.

Great. I hope we'll get told when that's going to happy.

> > I hope, for your sake
> 
> Is that a threat?  I'm sorry, but did I miss something important here?

Well, it depends. Breaking other people's code isn't good. Changing other
people's code without so much as "hi, we changed this interface so we changed
this function for you" isn't good. I'm not too happy about it, having had to
spend a good hour or so trying to work out /what the heck/ was causing the
problem. Then had to work out what had actually changed. Then had to fix it.

I'm sorry if I'm not being as nice as I could be, but it's made a right mess of
my evening and irritated me greatly, not too mention the confusion of random users.

> > I can work out how to tell what parameter list the function is getting.
> 
> instanceof will help you out, I would wager.  For now.

I've got by with arguments.length.

> > This code is meant to work on *all* Mozilla builds since 1.0.
> 
> In that case, why are you using unfrozen apis?  I suppose if we eliminate
> nsIContentHandler we'll move you over to the frozen (since 1.0!)
> nsIURIContentListener, but you may want to consider doing that yourself.

It's not my code - I have no idea why it uses the interfaces it does.

(In reply to comment #30)
> I do agree that we should have dropped you a mail about this change, by the way,
> though it's getting rather hard to keep track of all the various Mozilla.org
> projects using unfrozen apis and all having totally different policies about
> where and when and how they want to work.

The fact the patch changed code in extensions/irc/* should have been enough to
have been told about it, IMHO, but this certainly isn't leveled at you or anyone
else in peticular - it's an annoying general problem.
firstly, let me apologize for not telling you about the chatzilla change :(
cc'ing the owners of similar projects, summary of the change:
nsIContentHandler::handleContent now has only three arguments - the command
argument was removed. (the type of the window context argument has also changed
to nsIInterfaceRequestor. But this doesn't break JS Code I think).

I'll also post something to npm.embedding, which I should've done before but
obviously forgot. (done now).

The code can probably be changed to use the (frozen) nsIURIContentListener; at
least if it doesn't use the window context. iirc almost none of the JS code I
changed used it.

> Changing other
> people's code without so much as "hi, we changed this interface so we changed
> this function for you" isn't good.

um. well. I kinda disagree here... I mean, (I claim that) most people don't care
so much when random apis they have to use change, and their code gets changed to
 be not broken.
I do realize that your project has a different view on this matter, as you want
to work on several releases. Again, sorry for not notifying you about this change.
But, as bz points out, unfrozen apis are always subject to change...

>I hope, for your sake, I can work out how to tell what parameter list the
>function is getting.
hm, if xpconnect lets that function be called...
you could check this.arguments I think.
looks like the browser/ part needs relanding
Keywords: aviary-landing

Comment 34

13 years ago
Relanding relevant part of patch v2 following aviary branch landing

Updated

13 years ago
Keywords: aviary-landing

Updated

12 years ago
Attachment #146438 - Flags: review?(blizzard)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.