Closed
Bug 94205
Opened 23 years ago
Closed 23 years ago
Change the nsIWebNavigation interface as detailed in the API Review notes...
Categories
(Core Graveyard :: Embedding: APIs, defect)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9.7
People
(Reporter: rpotts, Assigned: rpotts)
References
()
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
jud
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
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•23 years ago
|
Target Milestone: --- → mozilla0.9.4
Assignee | ||
Comment 1•23 years ago
|
||
Assignee | ||
Comment 2•23 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•23 years ago
|
||
cc: jaggernaut for JS webnav changes, and ben for Manticore. See above.
Comment 4•23 years ago
|
||
js and xbl changes look fine, r=jag on those.
Comment 5•23 years ago
|
||
r=valeski for all the c/cpp/idl changes.
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•23 years ago
|
||
r=vidur for the nsLocation.cpp changes.
Assignee | ||
Comment 8•23 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 | ||
Comment 9•23 years ago
|
||
I've just checked in the patch for nsIWebNavigation::Stop(...).
Next up is nsIWebNavigation::LoadURI(...) !!
Assignee | ||
Comment 10•23 years ago
|
||
Assignee | ||
Comment 11•23 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•23 years ago
|
||
Why would one pass in a target window to loadURI?
Comment 13•23 years ago
|
||
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•23 years ago
|
||
take a look at bug #90722 for a discussion of why providing a window target is
useful.
-- rick
Comment 15•23 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•23 years ago
|
||
Targetted loading is at a higher level IMO than nsIWebNavigation. See my
comments in bug 90722.
Comment 17•23 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•23 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•23 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•23 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•23 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•23 years ago
|
QA Contact: mdunn → depstein
Assignee | ||
Comment 22•23 years ago
|
||
Assignee | ||
Comment 23•23 years ago
|
||
*** Bug 46870 has been marked as a duplicate of this bug. ***
Comment 24•23 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 | ||
Comment 26•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Attachment #46264 -
Attachment is obsolete: true
Assignee | ||
Updated•23 years ago
|
Attachment #47673 -
Attachment is obsolete: true
Comment 27•23 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•23 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•23 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•23 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+
Comment 31•23 years ago
|
||
I think we should spell Referrer correctly everywhere except sending and parsing
the header itself. :-)
Gerv
Assignee | ||
Comment 32•23 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•23 years ago
|
||
bug 106386 deals with quite a few more spelling mistakes...
Comment 34•23 years ago
|
||
Comment on attachment 55998 [details] [diff] [review]
updated patch...
sr=jst
Attachment #55998 -
Flags: superreview+
Comment 36•23 years ago
|
||
Can we get this in please? It's blocking quite a high profile and
much-dupped-against bug :-).
Assignee | ||
Comment 37•23 years ago
|
||
finally!! this patch has been checked into the trunk.
-- rick
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 38•23 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•23 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•23 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•23 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•23 years ago
|
||
Then it's OK! And loadURI() and Stop() are working fine in testEmbed.
Comment 43•23 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•23 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•23 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•23 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•23 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•23 years ago
|
||
Thanks John, we still had one in the 'shared' mozilla files. These files I
didn't check before, forgot about it, sorry (:
Updated•6 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•