99625 - Freeze nsIWebNavigation

Status: RESOLVED WONTFIX

(Core Graveyard :: Embedding: APIs, defect, P5)

Description

[Chak Nanga]

Freeze nsIWebNavigation

Please refer to
http://www.mozilla.org/projects/embedding/EmbedInterfaceFreeze.html for the
issues to be addressed, if any, for this interface.

Please follow the guidelines outlined in "How to mark an interface as frozen?"
section of the document above.

Comment 1

[Chak Nanga]

->0.9.6

Comment 2

[rpotts (gone)]

-> 0.9.7

Comment 3

[Adam Lock]

referingURI attribute needs to be spelt properly before freezing...

Comment 4

[Darin Fisher]

mass reassign of various rpotts bugs to me

Comment 5

[Vedran Miletic]

retargeting

Comment 6

[Alex Vincent [:WeirdAl]]

re comment 3:  http://bugzilla.mozilla.org/attachment.cgi?id=134486&action=edit

Thanks, timeless, for the cc.

Comment 7

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

ok, some comments reading this interface:
o) The comment describing it (line 25) needs to be directly above the interface
declaration to allow doxygen to extract documentation for it (i.e. below the
interface forward declarations)
o) The documentation for the /* loadURI() & reload() specific flags */ should
use some doxygen-like style
o) loadURI documentation does not document the "headers" parameter. It would be
nice if it described the format of this header stream... Also, wouldn't this be
easier to use if the headers were passed as a string? And should the uri maybe
be passed as an nsIURI rather than wstring?
o) Would it be useful to add a load function that takes a channel rather than a
uri? that scales better when more things need to be configured than the current
loadURI signature allows
o) LOAD_FLAGS_MASK could use a description that describes what it really is.

Comment 8

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

hmm, there's also bug 123960

o) For the @return parts, each line that documents a return value should be
prefixed with @return, that produces nicer doxygen documentation (otherwise all
those lines end up as a single line)

Comment 9

[timeless]

 62   * there is back session history available for navigation.
 68   * there is forward session history available for navigation

punctuation...

I think those @returns are probably supposed to be @throws

185   * pending Javascript timeouts.

case :)

209   * The currently loaded URI or null.

when would this be null?

214   * The referring URI.

but not null?

I'd like to object to this freeze:

222   attribute nsISHistory sessionHistory;

nsISHistory was frozen with a dependency on the flash frozen
nsISHistoryListener. nsISHistoryListener has incorrect method case and IMO we
should not propogate it into the new world.

I'd rather we not freeze with a dependency on nsISHistory and instead replace
that interface.

Comment 10

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

adding bug 170439 (since it would mean a documentation change), although I
personally think it should just be wontfix. especially as I think it should be
nsIURI :)

Comment 11

[Darin Fisher]

Attachment: v0 patch

This patch contains a first pass over cleaning up the comments in
nsIWebNavigation.idl.  I'm unsure how to explain the meaning of
LOAD_FLAGS_IS_REFRESH and LOAD_FLAGS_IS_LINK.  LOAD_FLAGS_IS_REFRESH appears to
be unused and can probably be removed, but LOAD_FLAGS_IS_LINK is used (by
mailnews at least), and it seems to have an affect on the aIsContentPreferred
boolean passed to nsIURILoader::openURI.  Hmm...

Comment 12

[Darin Fisher]

Comment on attachment 162639 [details] [diff] [review]
v0 patch

besides LOAD_FLAGS_IS_REFRESH and LOAD_FLAGS_IS_LINK, do you see anything else
that needs to be clarified before freezing this interface?

Comment 13

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

Comment on attachment 162639 [details] [diff] [review]
v0 patch

sorry for the delay...

LOAD_FLAGS_IS_REFRESH does seem to be used:
/docshell/base/nsDocShell.h, line 123 -- LOAD_REFRESH =
MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_NORMAL,
nsIWebNavigation::LOAD_FLAGS_IS_REFRESH),

and http://lxr.mozilla.org/seamonkey/search?string=LOAD_REFRESH

+   *	Specifies that the load should have the semantics of a link click.

hm, wanna mention what the semantics of a link click _are_? :)

I am not sure that freezing an interface with XXX comments in it is a good
thing.

+   * LOAD_FLAGS_BYPASS_HISTORY
+   *	Specifies that history should not be updated. 

this means that no history entry should be created, right? what will it mean to
call goBack() after this? and how does this relate to REPLACE_HISTORY?

what are the semantics of LOAD_FLAGS_CHARSET_CHANGE from an implementor's point
of view? ("always use the cache if we have an entry"?)

+   * @param aURI
+   *	     The URI string to load.

so characters above U+007F will be sent as url-escaped UTF-8? (in the HTTP and
FTP case)

I wonder whether this should be documented... on the other hand, it's probably
obvious, since there is no charset to pass to NewURI


loadURI takes a string for the uri to load, but an nsIURI object for the
referring uri... ah, consistency :/


 This stream must
+   *	     contain a "\r\n" sequence 

at the beginning? at the end? both?

+   * @throw NS_BINDING_ABORTED
+   *	     Indicating that the user canceled the reload.

this is for the postdata case, right? I think you should mention in this
method's (reload's) description somewhere that there can be cases where the
user will be asked to confirm the reload, in cases where the request isn't
idempotent (this describes all the cases where we prompt, right?).


What does stop return when there is nothing to stop?


marking r- for now

Comment 14

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

oh, wanted to mention one more thing
I kinda prefer having the documentation for each flag, instead of in one block.
semantically, and from the point of automatic IDL parsers, the single block is
the wrong thing. quoting my example from IRC, if someone were to write
autocompletion code using this IDL file, which would give the flag description
in a tooltip or something, the single block would do rather the wrong thing.

(this file and nsIWebProgress and nsIWPL should use the same style for this...)

Comment 15

[Darin Fisher]

> LOAD_FLAGS_IS_REFRESH does seem to be used:
> /docshell/base/nsDocShell.h, line 123 -- LOAD_REFRESH =
> MAKE_LOAD_TYPE(nsIDocShell::LOAD_CMD_NORMAL,
> nsIWebNavigation::LOAD_FLAGS_IS_REFRESH),

right.. but how is that used?  you see.. i think it might be used internally,
but i don't see it being used with nsIWebNavigation.  it's unclear why we need
to expose that flag on nsIWebNavigation.  it's also unclear what it really
means.  i don't fully grok nsDocShell :(


> +   *	Specifies that the load should have the semantics of a link click.
> 
> hm, wanna mention what the semantics of a link click _are_? :)

i wish... again, i don't fully grok nsDocShell.  need to figure out what it
means.  the XXX comments exist because these need to be sorted out before
freezing.  this patch is just v0 because i'm not done.. wanted to get the rest
of it reviewed and moved into the trunk.


> +   * LOAD_FLAGS_BYPASS_HISTORY
> +   *	Specifies that history should not be updated. 
> 
> this means that no history entry should be created, right? what will it mean to
> call goBack() after this? and how does this relate to REPLACE_HISTORY?

very good question.  we should figure that out and document it.


> what are the semantics of LOAD_FLAGS_CHARSET_CHANGE from an implementor's point
> of view? ("always use the cache if we have an entry"?)

i have no idea.  another thing to try to figure out.


> +   * @param aURI
> +   *	     The URI string to load.
> 
> so characters above U+007F will be sent as url-escaped UTF-8? (in the HTTP and
> FTP case)
> 
> I wonder whether this should be documented... on the other hand, it's probably
> obvious, since there is no charset to pass to NewURI

yeah, we should document that.


> loadURI takes a string for the uri to load, but an nsIURI object for the
> referring uri... ah, consistency :/

i know... i really hate this interface, but it has to be frozen or we'll burn
way too many people.


>  This stream must
> +   *	     contain a "\r\n" sequence 
> 
> at the beginning? at the end? both?

ok, i'll clarify that.


> +   * @throw NS_BINDING_ABORTED
> +   *	     Indicating that the user canceled the reload.
> 
> this is for the postdata case, right? I think you should mention in this
> method's (reload's) description somewhere that there can be cases where the
> user will be asked to confirm the reload, in cases where the request isn't
> idempotent (this describes all the cases where we prompt, right?).

yeah... good call!


> What does stop return when there is nothing to stop?

hmm... not sure.

Comment 16

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

ah, bug 123960 also mentions some issues... I also noticed the "you" in the
comment, which I didn't really like. that bug seems to have some more issues
which this patch doesn't address yet (for example, on "inferred internally").

Comment 17

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

this patch leaves in these comments:
  const unsigned long LOAD_FLAGS_BYPASS_CACHE    = 0x0100; // Bypass the cache
  const unsigned long LOAD_FLAGS_BYPASS_PROXY    = 0x0200; // Bypass the proxy
  const unsigned long LOAD_FLAGS_CHARSET_CHANGE  = 0x0400; // Reload because of
charset change, 


they seem redundant with the comments you add

Comment 18

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

bz: any idea about LOAD_REFRESH / semantics of a link click /
LOAD_FLAGS_BYPASS_HISTORY / charset change? (see comment 15)

Comment 19

[Boris Zbarsky [:bzbarsky]]

> any idea about LOAD_REFRESH

It looks like the nsIWebNavigation flag for this is unused, as is the
nsDocShellLoadInfo flag... except by refresh timers in certain circumstances. 
So yes, this is totally an internal detail of docshell.  We should not be
exposing it.

> semantics of a link click

The only places I see LOAD_LINK being treated differently from LOAD_NORMAL is in
this shrimp hack code and the call to the URILoader.  I thought the former got
removed as part of a security fix?  Or did that only land on aviary branch?  I
wish I could recall who the patch author there was....

Anyway, the URILoader call just means we'll use IsPreferred() instead of
CanHandleContent() when looking for a handler. Not like those are too clearly
defined either...

There are a few consumers of this LOAD_LINK thing (notably actual link clicks
and some code in mailnews that uses the nsIDocShellLoadInfo way of getting at
these load flags).

> LOAD_FLAGS_BYPASS_HISTORY

Yeah... this seems to skip both the global and session history.  I'm not quite
sure why we have it.

> charset change

I don't quite recall what this does either...

I think at least figuring out which parts we don't know is a good start, so we
can start trying to try to figure them out....

For the rest, the load flag system is way too complicated, with at least 4 sets
of load flags about (docshell, webnavigation, loadinfo, and necko).  We should
really only have two -- webnavigation and necko, with docshell mapping between
the two.  Loadinfo in particular really needs to go away.  :(

Comment 20

[Darin Fisher]

bz: thanks for the comments...


> > any idea about LOAD_REFRESH
...
> So yes, this is totally an internal detail of docshell.  We should not be
> exposing it.

alright then, that's what i'll do :)


> The only places I see LOAD_LINK being treated differently from LOAD_NORMAL is in
> this shrimp hack code and the call to the URILoader.  I thought the former got
> removed as part of a security fix?  Or did that only land on aviary branch?  I
> wish I could recall who the patch author there was....

dveditz did that patch, and yeah... it doesn't appear to have landed on the
trunk yet.


> There are a few consumers of this LOAD_LINK thing (notably actual link clicks
> and some code in mailnews that uses the nsIDocShellLoadInfo way of getting at
> these load flags).

One place in mailnews does use LOAD_FLAGS_IS_LINK, and that's here:
http://lxr.mozilla.org/seamonkey/source/mailnews/imap/src/nsImapMailFolder.cpp#3563


> > LOAD_FLAGS_BYPASS_HISTORY
> 
> Yeah... this seems to skip both the global and session history.  I'm not quite
> sure why we have it.

It looks like the ActiveX and OSX Cocoa embedding use this flag.


> > charset change
> 
> I don't quite recall what this does either...

This is set whenever the user requests that the current page be viewed using a
different character encoding.  I'm not sure what the semantics of it actually
are though.


> For the rest, the load flag system is way too complicated, with at least 4 sets
> of load flags about (docshell, webnavigation, loadinfo, and necko).  We should
> really only have two -- webnavigation and necko, with docshell mapping between
> the two.  Loadinfo in particular really needs to go away.  :(

Agreed, but it's going to take some work to be sure.

Comment 21

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

So, are there any chances to add a loadChannel(nsIChannel, flags) method to this
interface? It would load a channel (which has not been asyncOpen'd yet); similar
semantics as loadURI.

Comment 22

[Darin Fisher]

I like that idea a lot, except that I don't think we should change the vtable of
nsIWebNavigation.  Adding that method means that we'd need to change the UUID,
and that will break existing embedders.  If we don't change the UUID then it
makes it very difficult to create an embedding that optionally uses the new
method.  I'd much rather design nsIWebNavigation2 with that method and other
improvements.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Note that the api, as written, has at least one major issue -- the URI passed to
loadURI() doesn't know the origin charset.  Don't we decide what to send to the
server based on that charset, if set?  If so, the interface, as written, can't
be used to do something as simple as implement "open in new tab" on
middle-clicking a link...  It's really only usable for "load this URI the user
typed in the URL bar" type things.

Comment 24

[Darin Fisher]

On the contrary, it only requires that the URL be properly formatted before it
is sent through this API.  We can document that any non-ASCII characters are
converted to UTF-8 before being %-escaped (IDN handled using the normal ACE
encoding rules).

Comment 25

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  Sounds like this is freezable with some documentation, then.  I agree
that any improvements should go into a different interface.

Comment 26

[Darin Fisher]

I went ahead and checked in some of the comment cleanups from this bug:

Checking in nsIWebNavigation.idl;
/cvsroot/mozilla/docshell/base/nsIWebNavigation.idl,v  <--  nsIWebNavigation.idl
new revision: 1.21; previous revision: 1.20
done

I took biesi's advice and split up the mega load flags comment block.  There's
more work to be done to freeze this interface.  The XXX comments in the IDL file
serve as a rough guide.

Comment 27

[Darin Fisher]

Adding this here so I don't forget.

Boris Zbarsky wrote:
> Darin Fisher wrote:
> > As for nsIWebNavigation::LoadURI, that method should be treated like a URL 
> > string entered into the URL bar.
> 
> That makes sense, but then we should clearly document this (and in particular 
> clearly document that strings that are not trusted to have come from the page in 
> question or the user should never be passed to this method).

Comment 28

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We no longer freeze interfaces.