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

VERIFIED FIXED in mozilla0.9.7

Status

()

VERIFIED FIXED
17 years ago
10 years ago

People

(Reporter: rpotts, Assigned: rpotts)

Tracking

Trunk
mozilla0.9.7
x86
Windows 2000
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

17 years ago
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...
(Assignee)

Updated

17 years ago
Blocks: 70229
(Assignee)

Updated

17 years ago
Target Milestone: --- → mozilla0.9.4
(Assignee)

Comment 1

17 years ago
Created attachment 45499 [details] [diff] [review]
Patch to add action flags to nsIWebNavigation::Stop(...)
(Assignee)

Comment 2

17 years ago
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

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

Comment 4

17 years ago
js and xbl changes look fine, r=jag on those.
(Assignee)

Updated

17 years ago
Blocks: 46870

Comment 5

17 years ago
r=valeski for all the c/cpp/idl changes.
(Assignee)

Updated

17 years ago
Blocks: 90722

Comment 6

17 years ago
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

17 years ago
r=vidur for the nsLocation.cpp changes.
(Assignee)

Comment 8

17 years ago
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
(Assignee)

Updated

17 years ago
Blocks: 40867
(Assignee)

Comment 9

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

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

Comment 10

17 years ago
Created attachment 46264 [details] [diff] [review]
Patch to add arguments to nsIWebNavigation::LoadURI(...)
(Assignee)

Comment 11

17 years ago
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

17 years ago
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?
(Assignee)

Comment 14

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

-- rick

Comment 15

17 years ago
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

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

Comment 17

17 years ago
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

17 years ago
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?
(Assignee)

Comment 19

17 years ago
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).

(Assignee)

Comment 20

17 years ago
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

17 years ago
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.

Updated

17 years ago
QA Contact: mdunn → depstein
(Assignee)

Comment 22

17 years ago
Created attachment 47673 [details] [diff] [review]
new patch to nsIWebNavigation which adds 'referer' 'postdata' and 'headers' arguments
(Assignee)

Comment 23

17 years ago
*** Bug 46870 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

17 years ago
Blocks: 48902

Comment 24

17 years ago
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
(Assignee)

Updated

17 years ago
Blocks: 99625

Updated

17 years ago
Blocks: 100629
(Assignee)

Comment 25

17 years ago
-> 0.9.6
Target Milestone: mozilla0.9.5 → mozilla0.9.6
(Assignee)

Comment 26

17 years ago
Created attachment 55998 [details] [diff] [review]
updated patch...
(Assignee)

Updated

17 years ago
Attachment #46264 - Attachment is obsolete: true
(Assignee)

Updated

17 years ago
Attachment #47673 - Attachment is obsolete: true

Comment 27

17 years ago
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

17 years ago
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 :)
(Assignee)

Comment 29

17 years ago
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

17 years ago
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
(Assignee)

Comment 32

17 years ago
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

17 years ago
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+
(Assignee)

Comment 35

17 years ago
-> 0.9.7
Target Milestone: mozilla0.9.6 → mozilla0.9.7

Comment 36

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

Comment 37

17 years ago
finally!! this patch has been checked into the trunk.

-- rick
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 38

17 years ago
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

17 years ago
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

17 years ago
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
(Assignee)

Comment 41

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

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

-- rick

Comment 42

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

Comment 43

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
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

17 years ago
Thanks John, we still had one in the 'shared' mozilla files. These files I
didn't check before, forgot about it, sorry (:
You need to log in before you can comment on or make changes to this bug.