Closed Bug 94205 Opened 19 years ago Closed 19 years ago

Change the nsIWebNavigation interface as detailed in the API Review notes...

Categories

(Core Graveyard :: Embedding: APIs, defect)

x86
Windows 2000
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.7

People

(Reporter: rpotts, Assigned: rpotts)

References

()

Details

Attachments

(2 files, 2 obsolete files)

Make the following changes to the nsIWebNavigation interface:
1. Add action flags to Stop(...).

2. Add support for passing Postdata as part of a URI load...

3. Clean up the load flags associated with LoadURI(...).  See the discussion on
n.p.embedding...

4. Add a referrer attribute...
Blocks: 70229
Target Milestone: --- → mozilla0.9.4
I've attached a patch which adds action flags (ie. STOP_CONTENT, STOP_NETWORK,
STOP_ALL) to nsIWebNavigation::Stop(...)

It also removes the following redundant Stop(...) apis...
  1. nsIDocShell::StopLoad() - this is identical to 
     Stop(nsIWebNavigation::STOP_NETWORK)  
  2. nsIDocShellLoadInfo::StopActiveDocument - this is identical to calling
     Stop(nsIWebNavigation::STOP_CONTENT) before loading the URI...

It turns out that some/many of the calls to nsIWebNavigation::Stop() are inside
of JS and XUL files which are not compiled and do not provide any error info :-(

So, please look at the diffs carefully and make sure that i didn't introduce any
syntax errors, or miss any places where Stop() is called !!

The patch does *not* fixup mozilla/extensions/manticore/browser/browserwindow.cs
because it is not part of the build and i do not know the C-Sharp syntax for
accessing interface constants :-)

Also, the patch to the GTK embedding widget is very simplistic.  It does *not*
expose the new nsIWebNavigation flags out through the public gtk embedding
widget APIs..

I've built this on windows, but i have not yet verified it on Linux, or Mac...
cc: jaggernaut for JS webnav changes, and ben for Manticore. See above.
js and xbl changes look fine, r=jag on those.
Blocks: 46870
r=valeski for all the c/cpp/idl changes.
Blocks: 90722
As mentioned in the embedding meeting:

It might be possible for the embedder to set/get postdata, referrer, headers
etc.from the start notification on their web progress listener right after the
loadURI but before the channel is opened. Rick can you verify?

If so it means we can do all these things right now. It's cumbersome and
probably shorts circuit the session history code so maybe we need postdata and
referrer arguments on loadURI anyway. We shouldn't need new attributes because
that information can be gotten from the progress listener.
r=vidur for the nsLocation.cpp changes.
hey Adam,

i totally agree with you (now) :-)  My current plan is to add a postData
argument to LoadURI(...) but *not* expose it as an attribute.  I hope to totally
hide the 'referer' at this level.

Really, the only reason to have a 'postData' attribute was keep the partial
patch for bug #40867 working.  But, this is a pretty lame excuse :-)

I'm going to bite the bullet and try to fix bug #40867 'the right way'.

-- rick
Blocks: 40867
I've just checked in the patch for nsIWebNavigation::Stop(...).

Next up is nsIWebNavigation::LoadURI(...) !!
I've just attached a patch which adds the following arguments to LoadURI(...):
 * targetWindow:
     PRUnichar string indicating the target window for the load.

 * postData:
     nsIInputStream containing POST data.
     this argument is ignored if the URI scheme is not HTTP.

 * headers:
     nsIInputStream containing extra HTTP headers to add to the http request.
     this argument is ignored if the URI scheme is not HTTP.
This patch only addresses fixing up the signature of nsIWebShell::LoadURI(...).
 There is still more work that needs to be done within nsDocShell.cpp and necko
to correctly support these arguments.

In particular, the headers stream and the referer are not passed through (yet).
 I'll address these issues in a subsequent patch.
Why would one pass in a target window to loadURI?
I would like to have targetted loading because I need it for proper x-remote
support.  Speaking of which, what about adding something that lets you pass in a
raise/no-raise flag?  Is that appropriate here?  Opinions?
take a look at bug #90722 for a discussion of why providing a window target is
useful.

-- rick
IMO, raise/no-raise doesn't belong here. the embeddor has access to the focus
ifaces and should use those instead (as intended). we're already cobbling a
bunch of stuff into this iface/method, and I'd rather not lop more on.
Targetted loading is at a higher level IMO than nsIWebNavigation. See my
comments in bug 90722.
As comments in this bug illustrate, referring setting isn't always
straightforward, hence, IMHO, burying it is the right thing to do. If we surface
it, then impls have to do X, Y, and Z to ensure they're setting the referer,
*and* ensure they're setting it properly. That's more responsibility than we
want to hand out.
As it stands, some simple documentation like:

* aReferer: the referer uri you want to send to the site you're loading
            the URI from, null if you don't want to send it.
            The referer uri is the document from which this uri was
            linked to.

            (Assuming nsIURI aReferer, nsAString aReferer would be
             another option, then s/null/empty string/).

should suffice. I don't think the comments in bug 90722 (mine and Stuart
Ballard's) illustrate setting the referer isn't always straightforward, it just
illustrates that I needed to think a little longer. Generally I doubt people
will have trouble deciding when to pass it in and when not. Making it explicit
will also allow us to pass it in from things like "Open Link In New Window",
which I suspect to be harder when we bury this.

But, what do we currently do to determine the referer? Is there an "always hit"
way to do it behind the scenes without resorting to ugliness and implicit
dependencies?
I think that if we *do* provide a 'referer' argument to LoadURI(...) it should
really be thought of as an 'override' of the default way of determining the
'referer'...

In general, I imagine that this argument would be mostly nsnull, indicating that
the normal referer (if any) should be used... It would only be specified in
those situations where it was difficult or impossible to otherwise determine the
referer ...

I think that a load flag should be used to indicate when the referer should NOT
be sent (this flag would override the current prefs based system which defines
its default behavior).

So, we need to come to closure on this bug for 0.9.4 !!

I don't really have any strong feelings either way...  My only concern is
whether it is too dificult to do targeted window loads from javascript if there
isn't an explicit 'target window' argument to loadURI(...).

If javascript users think it is ok to do the following three steps:
  1. Try to get an existing window from the window watcher (via new API)
  2. create a new window via window watcher (if on window was found)
  3. QI resulting nsIDOMWindow to nsIWebNavigation and call loadURI(...)

Then i'm fine passing a 'referer' instead of the 'target window'

I need some feedback FAST!

thanks,
-- rick
I'm fine with those steps. They can easily be put in a helper function in e.g.
navigator.js if we need them more than once.

var ww = ... // get window watcher service
var w = ww.getWindowByName("...");
if (!w)
  w = ww.createWindow(...);

w.location.href = urlToLoad;

Something like that should do the trick.
QA Contact: mdunn → depstein
*** Bug 46870 has been marked as a duplicate of this bug. ***
Blocks: 48902
rick, we are moving this to 0.9.5, please let us know if that is not a good 
idea.  per pdt mtg
Target Milestone: mozilla0.9.4 → mozilla0.9.5
Blocks: 99625
Blocks: 100629
-> 0.9.6
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Attached patch updated patch...Splinter Review
Attachment #46264 - Attachment is obsolete: true
Attachment #47673 - Attachment is obsolete: true
Comment on attachment 55998 [details] [diff] [review]
updated patch...

- in nsDocShell::LoadURI() you're not passing in the extra headers nor the referrer (your "XXX" comments). should we take care of that in this pass too? If not, can you file a new bug?
- in nsIWebNavigation.idl you add the headers arg, but, there's no comment for the headers.
- should the referer param comment try and describe how the "refering URI will be inferred internally"?
- any commercial tree changes?
Rick,

The patch looks reasonably straightforward but I noticed a few issues:

* Headers & referrer not being set on load info
   +    // XXX: Need to pass in the extra headers stream too...
   +    // XXX: Need to pass in referer...
* Missing documentation on nsIWebNavigation::LoadURI for headers param
* Unimplemented GetReferingURI method on session history.
* Refering is spelt referring, referer is spelt referrer :)
yep... i wanted to get all of the API changes in and then i'm going to go back
and make sure that the 'postdata', 'headers' and 'referer' are being handled
correctly.

i really don't know what to do about 'referrer' :-(  should i correct the
spelling or should we live with this montulli'ism for the next few years?

i'll also beef up the documentation for nsIWebNavigation::LoadURI(...).

i'll also implement getReferingURI on session history while i'm at it.

Comment on attachment 55998 [details] [diff] [review]
updated patch...

please file a new bug for the referer/postdata hookup. also, please fixup the idl comments per my comments above.
Attachment #55998 - Flags: review+
I think we should spell Referrer correctly everywhere except sending and parsing
the header itself. :-)

Gerv
well... i've made sure that the spelling is consistantly 'referrer' in the
patch... there are still some files :
http://lxr.mozilla.org/seamonkey/search?string=referer
which use the incorrect 'referer' spelling.

however, i believe that all of the public APIs use the correct 'referrer'
spelling :-)
bug 106386 deals with quite a few more spelling mistakes...
Comment on attachment 55998 [details] [diff] [review]
updated patch...

sr=jst
Attachment #55998 - Flags: superreview+
-> 0.9.7
Target Milestone: mozilla0.9.6 → mozilla0.9.7
Can we get this in please? It's blocking quite a high profile and
much-dupped-against bug :-).
finally!! this patch has been checked into the trunk.

-- rick
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Checked Stop() patch checkin against 12/3 mozilla build:
* mozilla/content/html/document/src/nsHTMLDocument.cpp
* mozilla/docshell/base/nsDSURIContentListener.cpp
* mozilla/docshell/base/nsDocShell.cpp
* mozilla/docshell/base/nsDocShellLoadInfo.cp
* mozilla/docshell/base/nsDocShellLoadInfo.h
* mozilla/docshell/base/nsIDocShell.idl
* mozilla/docshell/base/nsIDocShellLoadInfo.idl
will check the rest later.
continuing with Stop() checkin against 12/3 build. Includes implementations of
Stop() with parameter input flag:
* mozilla/docshell/base/nsIWebNavigation.idl
* mozilla/docshell/base/nsWebShell.cpp
* mozilla/dom/src/base/nsGlobalWindow.cpp
* mozilla/dom/src/base/nsLocation.cpp
* mozilla/embedding/browser/activex/src/control/MozillaBrowser.cpp
* mozilla/embedding/browser/chrome/content/mini-nav.js
* mozilla/embedding/browser/gtk/src/gtkmozembed2.cpp
* mozilla/embedding/browser/photon/src/PtMozilla.cpp
* mozilla/embedding/browser/powerplant/source/CBrowserShell.cpp
* mozilla/embedding/browser/webBrowser/nsWebBrowser.cpp
* checkins to mfcEmbed, testEmbed, and winEmbed
* mozilla/mailnews/base/src/nsMsgWindow.cpp
* mozilla/xpfe/browser/resources/content/fastnav.js
* mozilla/xpfe/browser/resources/content/navigator.js
* mozilla/xpfe/browser/src/nsBrowserInstance.cpp
* mozilla/xpfe/components/shistory/src/nsSHistory.cpp
And now for something completely different ... verification of patch 55998
against 12/3 mozilla build. I checked all the files, looks good except for a
couple of minor points:
* in nsIWebNavigation.idl, explain what headers parameter is for LoadURI().
* in mozilla/extensions/xmlterm/base/mozXMLTerminal.cpp, LoadURI() is missing
the last 3 nsnull for parameter inputs:
result = aDocShell->LoadURI(uri, nsnull, nsIWebNavigation::LOAD_FLAGS_NONE);

checked these:
* mozilla/docshell/base/nsDocShell.cpp
* mozilla/docshell/base/nsDocShell.h
* mozilla/docshell/base/nsDocShellLoadInfo.cpp
* mozilla/docshell/base/nsDocShellLoadInfo.h
* mozilla/docshell/base/nsIDocShellLoadInfo.idl
* mozilla/docshell/base/nsIWebNavigation.idl
* mozilla/docshell/base/nsWebShell.cpp
* mozilla/dom/src/base/nsGlobalWindow.cpp
* mozilla/editor/composer/src/nsEditorShell.cpp
* mozilla/embedding/browser/activex/src/control/MozillaBrowser.cpp
* mozilla/embedding/browser/chrome/content/mini-nav.js
* mozilla/embedding/browser/gtk/src/EmbedPrivate.cpp
* mozilla/embedding/browser/photon/src/PtMozilla.cpp
* mozilla/embedding/browser/powerplant/source/CBrowserShell.cpp
* mozilla/embedding/browser/webBrowser/nsWebBrowser.cpp
* mozilla/embedding/qa/testembed/BrowserView.cpp
* checkins for testEmbed, mfcEmbed, and winEmbed
* mozilla/extensions/help/resources/content/help.js
* mozilla/extensions/inspector/resources/content/inspector.js
* mozilla/extensions/inspector/resources/content/search/inSearchService.js
*
mozilla/extensions/transformiix/source/examples/mozilla/Transformiix/content/transformiix.js
* mozilla/extensions/xmlterm/base/mozXMLTerminal.cpp
* mozilla/htmlparser/robot/nsDebugRobot.cpp
* mozilla/mailnews/base/src/nsMessenger.cpp

* mozilla/mailnews/base/src/nsMsgPrintEngine.cpp
* mozilla/webshell/tests/viewer/nsBrowserWindow.cpp
* mozilla/webshell/tests/viewer/nsWebCrawler.cpp
* mozilla/webshell/tests/viewer/nsXPBaseWindow.cpp
* mozilla/xpfe/appshell/src/nsWebShellWindow.cpp
* mozilla/xpfe/bootstrap/appleevents/nsWindowUtils.cpp
* mozilla/xpfe/browser/resources/content/fastnav.js
* mozilla/xpfe/browser/resources/content/navigator.js
* mozilla/xpfe/browser/resources/content/viewsource.js
* mozilla/xpfe/browser/src/nsBrowserInstance.cpp
* mozilla/xpfe/components/shistory/src/nsSHistory.cpp
* mozilla/xpfe/components/xremote/src/XRemoteService.cpp
* mozilla/xpfe/global/resources/content/bindings/browser.xml

whew! 
Status: RESOLVED → VERIFIED
in mozXMLTerminal.cpp it is *actually* calling a different versino of LoadURI 
:-)

This code is calling nsIDocShell::LoadURI(...) *not* 
nsIWebNavigation:;LoadURI(...)

-- rick
Then it's OK! And loadURI() and Stop() are working fine in testEmbed.
We used loadURI() in MultiZilla all over the place, but after this patch
something strange is going on. If you, for example, click on the personal
toolbar bookmarks, no uri load is done. It seems we can't use a plain loadURI(url)
anymore!

Can you please tell me why?

Thanks,
-Neil.
Small example from navigator.js

function BrowserHome()
{
  var homePage = getHomePage();
  // loadURI(homePage);
  getWebNavigation().loadURI(homePage, nsIWebNavigation.LOAD_FLAGS_NONE, null,
null, null);
}

Ths loadURI(homePage); call does work in a plain mozilla build, but won't work
in MultiZilla so I had to change! But why does it work in mozilla and NOT in
MultiZilla, with the same function!
Er, loadURI() in the mozilla navigator.js does
  try {
    getWebNavigation().loadURI(uri, nsIWebNavigation.LOAD_FLAGS_NONE, 
                              null, null, null);
  } catch (e) {
  }

so I assume that you are calling your own version of navigator.js which has 
not been update from doing:

  try {
    getWebNavigation().loadURI(uri, nsIWebNavigation.LOAD_FLAGS_NONE);
  } catch (e) {
  }


John, this is how loadURI() looks in our copy of navigator.js:

function loadURI(uri)
{
  alert( "in" ); // just to see if this is called!
  try {
    getWebNavigation().loadURI(uri, nsIWebNavigation.LOAD_FLAGS_NONE, null,
null, null);
  } catch (e) {
  }
}

But I don't see the alert! And there is only one function with this name (a did
a search) ! I see the alert in mozilla's navigator.js, our version is a modified
copy of that file, including the recent split up. BTW, I get no errors on the JS
console!
If you don't get that alert(), then this has nothing to do with the interface 
changes here. I think you should look again in your tree; you must have 
another definition of that function in the current scope.
Thanks John, we still had one in the 'shared' mozilla files. These files I
didn't check before, forgot about it, sorry (:
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.