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

Status: VERIFIED FIXED

(Core Graveyard :: Embedding: APIs, defect)

Description

[rpotts (gone)]

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

Comment 1

[rpotts (gone)]

Attachment: Patch to add action flags to nsIWebNavigation::Stop(...)

Comment 2

[rpotts (gone)]

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

Comment 3

[John Morrison]

cc: jaggernaut for JS webnav changes, and ben for Manticore. See above.

Comment 4

[jag (Peter Annema)]

js and xbl changes look fine, r=jag on those.

Comment 5

[Judson Valeski]

r=valeski for all the c/cpp/idl changes.

Comment 6

[Adam Lock]

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.

Comment 7

[vidur (gone)]

r=vidur for the nsLocation.cpp changes.

Comment 8

[rpotts (gone)]

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

Comment 9

[rpotts (gone)]

I've just checked in the patch for nsIWebNavigation::Stop(...).

Next up is nsIWebNavigation::LoadURI(...) !!

Comment 10

[rpotts (gone)]

Attachment: Patch to add arguments to nsIWebNavigation::LoadURI(...)

Comment 11

[rpotts (gone)]

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.

Comment 12

[jag (Peter Annema)]

Why would one pass in a target window to loadURI?

Comment 13

[Christopher Blizzard (:blizzard)]

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?

Comment 14

[rpotts (gone)]

take a look at bug #90722 for a discussion of why providing a window target is
useful.

-- rick

Comment 15

[Judson Valeski]

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.

Comment 16

[jag (Peter Annema)]

Targetted loading is at a higher level IMO than nsIWebNavigation. See my
comments in bug 90722.

Comment 17

[Judson Valeski]

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.

Comment 18

[jag (Peter Annema)]

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?

Comment 19

[rpotts (gone)]

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

Comment 20

[rpotts (gone)]

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

Comment 21

[jag (Peter Annema)]

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.

Comment 22

[rpotts (gone)]

Attachment: new patch to nsIWebNavigation which adds 'referer' 'postdata' and 'headers' arguments

Comment 23

[rpotts (gone)]

*** Bug 46870 has been marked as a duplicate of this bug. ***

Comment 24

[lchiang]

rick, we are moving this to 0.9.5, please let us know if that is not a good 
idea.  per pdt mtg

Comment 25

[rpotts (gone)]

-> 0.9.6

Comment 26

[rpotts (gone)]

Attachment: updated patch...

Comment 27

[Judson Valeski]

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?

Comment 28

[Adam Lock]

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 :)

Comment 29

[rpotts (gone)]

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 30

[Judson Valeski]

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.

Comment 31

[Gervase Markham [:gerv]]

I think we should spell Referrer correctly everywhere except sending and parsing
the header itself. :-)

Gerv

Comment 32

[rpotts (gone)]

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 :-)

Comment 33

[Adam Lock]

bug 106386 deals with quite a few more spelling mistakes...

Comment 34

[Johnny Stenback (:jst)]

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

sr=jst

Comment 35

[rpotts (gone)]

-> 0.9.7

Comment 36

[James Green]

Can we get this in please? It's blocking quite a high profile and
much-dupped-against bug :-).

Comment 37

[rpotts (gone)]

finally!! this patch has been checked into the trunk.

-- rick

Comment 38

[David Epstein]

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.

Comment 39

[David Epstein]

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

Comment 40

[David Epstein]

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! 

Comment 41

[rpotts (gone)]

in mozXMLTerminal.cpp it is *actually* calling a different versino of LoadURI 
:-)

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

-- rick

Comment 42

[David Epstein]

Then it's OK! And loadURI() and Stop() are working fine in testEmbed.

Comment 43

[Neil Pryde]

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.

Comment 44

[Neil Pryde]

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!

Comment 45

[John Morrison]

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) {
  }

Comment 46

[Neil Pryde]

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!

Comment 47

[John Morrison]

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.

Comment 48

[Neil Pryde]

Thanks John, we still had one in the 'shared' mozilla files. These files I
didn't check before, forgot about it, sorry (: