Closed Bug 99625 Opened 23 years ago Closed 14 years ago

Freeze nsIWebNavigation

Categories

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

defect

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: chak, Unassigned)

References

(Depends on 3 open bugs)

Details

(Keywords: embed, topembed-)

Attachments

(1 file)

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.
Blocks: 98417
QA Contact: mdunn → depstein
Depends on: 94205
Depends on: 90722
->0.9.6
Target Milestone: --- → mozilla0.9.6
-> 0.9.7
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Target Milestone: mozilla0.9.7 → mozilla1.0
Keywords: topembed
Keywords: topembedembed, topembed-
Depends on: 87127
QA Contact: depstein → carosendahl
referingURI attribute needs to be spelt properly before freezing...
mass reassign of various rpotts bugs to me
Assignee: rpotts → darin
Blocks: 157137
retargeting
Target Milestone: mozilla1.0 → Future
Target Milestone: Future → ---
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8alpha
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.
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)
Depends on: 123960
OS: Windows 2000 → All
Hardware: PC → All
Blocks: 248683
Target Milestone: mozilla1.8alpha1 → mozilla1.8beta
 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.
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 :)
Depends on: 170439
Attached patch v0 patchSplinter Review
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 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?
Attachment #162639 - Flags: review?(cbiesinger)
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
Attachment #162639 - Flags: review?(cbiesinger) → review-
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...)
> 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.
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").
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
bz: any idea about LOAD_REFRESH / semantics of a link click /
LOAD_FLAGS_BYPASS_HISTORY / charset change? (see comment 15)
> 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.  :(
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.
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.
Blocks: 268520
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.
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.
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).
Ah, ok.  Sounds like this is freezable with some documentation, then.  I agree
that any improvements should go into a different interface.
Depends on: 278357
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.
Priority: -- → P1
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).
Depends on: 290090
Depends on: 223808
Target Milestone: mozilla1.8beta1 → mozilla1.9alpha
Depends on: 298313
Priority: P1 → P5
Assignee: darin → nobody
Status: ASSIGNED → NEW
QA Contact: carosendahl → apis
Target Milestone: mozilla1.9alpha → ---
We no longer freeze interfaces.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → WONTFIX
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.