324285 - Improve nsISHistoryListener.idl documentation

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Nickolay_Ponomarev]

Someone in irc.mozilla.org/#extdev was confused at nsISHistoryListener::OnHistoryGotoIndex documentation stating that "Called to notify a listener when the user visits a page using the 'Go' menu of the browser", which is not true in Firefox.

The documentation of this core interface shouldn't depend on browser UI.

There were other issues with the documentation: inconsistent terminology (pages/documents/entries), repeated (and unnecessary wordy) text in @return, incorrect javadoc usage ("@return aReturn"), docs that don't belong in this file (a note in OnHistoryPurge). I believe my version is actually more useful for the users of this interface.

Some questions:
1) What's this and can it be moved from the IDL, doesn't really belong here. It appears to be used in some test code, but I just don't see where some object is registered for that contract id.
%{C++
#define NS_SHISTORYLISTENER_CONTRACTID "@mozilla.org/browser/shistorylistener;1"
%}
2) When talking about nsIWebNavigation, I followed this pattern: "for example due to a nsIWebNavigation::*() call". In fact, the listener methods only get called from nsSHistory's methods corresponding to nsIWebNavigation. Should this be noted in the documentation?

Comment 1

[Nickolay_Ponomarev]

Attachment: patch

Comment 2

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

hm, yeah, that contract id does look unused (it wouldn't belong in the IDL if it were used either, though)

while touching this, you should probably move the comment block directly in front of the interface decl, so that automatic comment extractors can figure out what it belongs to.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 209231 [details] [diff] [review]
patch

Do what biesi said as far as moving the comment block?

>Index: nsISHistoryListener.idl
>+ * correspondent callback method.

corresponding.

>+ * Session history listener can be registered using
>+ * nsISHistory::addSHistoryListener() method. Note that 

"A session history listener can be registered on a particular nsISHistory instance via the nsISHistory::addSHistoryListener() method.  Note that"

Though I'm not sure that the "Note that" part is relevant -- it's documented on addSHistoryListener, and that's where the documentation belongs, imo.

Though it would be nice if nsISHistory made it clear that only one listener is supported at a time (or if we fixed the impl to support multiple listeners, which the interface implies are supported).

>+   * Called when navigating to a previous session history entry, for example
>+   * due to a nsIWebNavigation::goBack() call, which may happen when the user
>+   * clicks the 'Back' button in the browser.

What's a "browser"?  What's a "'Back' button"?

I don't really think this should be talking about UI at all.  Especially since I suspect that in the near future (next year or two) we'll want to fundamentally revamp our session history concept (and hence UI).  Of course then we'd have to deprecate this interface....

But seriously, I'd like to see mention of "browser" and buttons gone from these comments.

Other than that, looks fine.  r=bzbarsky with the nits picked (though you may want to see whether darin agrees with me before making changes to the patch).

Comment 4

[Nickolay_Ponomarev]

Attachment: patch for checkin [checked in]

Updated to comments. Will remove the contract id in a separate patch.

> Though I'm not sure that the "Note that" part is relevant -- it's documented on
> addSHistoryListener, and that's where the documentation belongs, imo.
I thought that it would be helpful for someone who implements the interface. You're right, that doesn't belong in this IDL.

Comment 5

[Matthew Waymost]

If the IDL documentation for SHistoryListener is going to be modified, so we wait for bug 320742 to be fixed? That way, the documentation can be more clear as to whether the listener is called before or after the entry is added.

Comment 6

[:Gavin Sharp [email: [e-mail]]]

mozilla/docshell/shistory/public/nsISHistoryListener.idl; new revision: 1.4;

Comment 7

[Nickolay_Ponomarev]

Attachment: remove all mentions of NS_SHISTORYLISTENER_CONTRACTID [checked in]

I'd also like to remove all mentions of NS_SHISTORYLISTENER_CONTRACTID, since it's not used:
 http://lxr.mozilla.org/seamonkey/search?string=STORYLISTENER_CO
 http://lxr.mozilla.org/seamonkey/search?string=browser/shistorylistener

Comment 8

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 210758 [details] [diff] [review]
remove all mentions of NS_SHISTORYLISTENER_CONTRACTID  [checked in]

mozilla/docshell/shistory/public/nsISHistoryListener.idl; new revision: 1.5;
mozilla/embedding/qa/testembed/BrowserView.cpp.mod; new revision: 1.8;
mozilla/embedding/qa/testembed/nsiHistory.cpp; new revision: 1.19;