239604 - make uriloader more doxygen-friendly

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

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

Comment 1

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

Attachment: patch

Comment 2

[Boris Zbarsky [: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.

Comment 3

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

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)

Comment 4

[Boris Zbarsky [:bzbarsky]]

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?  :(

Comment 5

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

(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.

Comment 6

[Scott MacGregor]

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....

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

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

Attachment: 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.

Comment 9

[Boris Zbarsky [: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

[Scott MacGregor]

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....

Comment 11

[Boris Zbarsky [:bzbarsky]]

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

[Darin Fisher]

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

Comment 13

[Darin Fisher]

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 14

[Boris Zbarsky [:bzbarsky]]

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?

Comment 15

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

(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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 17

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

(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.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> note that the scope of this bug already widened

Noted.  ;)

> you things like the xpinstall contenthandler?

For a glaring example, yes.

Comment 19

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

Comment on attachment 145436 [details] [diff] [review]
patch v2

(I'll make the first change mentioned in comment 14)

Comment 20

[Darin Fisher]

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

Comment 21

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

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

Comment 22

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

filed Bug 240731 for allowing passing null to nsIURILoader::Open and Bug 240732
to review nsIContentHandler implementations.

Comment 23

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

checked in with the mentioned changes made.

Comment 24

[Scott MacGregor]

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?

Comment 25

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

"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

Comment 26

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

Attachment: xremotelistener changes

Comment 27

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

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

[James Ross]

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.

Comment 29

[Boris Zbarsky [:bzbarsky]]

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.

Comment 30

[Boris Zbarsky [:bzbarsky]]

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

[James Ross]

(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.

Comment 32

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

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.

Comment 33

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

looks like the browser/ part needs relanding

Comment 34

[Ian Neal]

Relanding relevant part of patch v2 following aviary branch landing