The mail/news team eradicated their appcore way back when, but Navigator is still clinging to nsIBrowserInstance. Much of the functionality in this file could be reproduced in javascript in considerably less code. I'm opening this bug to legitimise the campaign to destroy this interface, and discuss any issues relating to it. Here are my initial ideas: Functionality relating to the browser content area itself and not specific to navigator should be placed into an XBL binding for <browser>, e.g. context menu, content area tooltip and whatnot. This would enable all usages of the browser widget within XUL to benefit from these features rather than us having to hard code them into each specific place. navigator.js needs to be tidied up. In many cases functions therein are simply calls into nsIBrowserInstance which calls scriptable methods on various APIs. This should all be in js, and applicable code shifted to the browser XBL binding. Also, some of the code in navigator.js is sloppy, or shouldn't be located in that particular file. Here's a brief rundown of what nsIBrowserInstance provides, and an analysis of potential paths to eradicate it: : void back(); : void forward(); These functions are simply calls into nsIWebNavigation, which the nsIWebBrowser on the <browser>'s box object can be QI'ed to. This code is redundant : readonly attribute boolean canGoBack; : readonly attribute boolean canGoForward; Ditto. These are attributes on nsIWebNavigation : void reload(in PRUint32 reloadType); : void stop(); : void loadUrl(in wstring url); nsIWebNavigation... : void loadInitialPage(); Slightly more complex, as it involves a separate class in nsBrowserInstance.cpp called PageCycler. I'm not quite sure what that does, but other than that everything else in the function seems to be scriptable... I'm also not sure why this function is so complex (loading the start page?!) : void backButtonPopup(in nsIDOMNode backMenu); : void forwardButtonPopup(in nsIDOMNode forwardMenu); : void updateGoMenu(in nsIDOMNode goMenu); redundant C++. This should never have been added in the first place as this functionality is completely possible via js. In fact, there appears to be a js implementation of sorts in navigator.js anyway... : void gotoHistoryIndex(in PRInt32 index); not sure about this one. involves use of non-scriptable nsIWebShell COMPtr. Is there way to work around this? : boolean walletPreview(in nsIDOMWindow win, in nsIDOMWindow form); : boolean walletQuickFillin(in nsIDOMWindow win); : PRUint32 walletRequestToCapture(in nsIDOMWindow win); uses a non-scriptable nsIPresShell. This is screaming to be in js, but alas seems not possible. At any rate it should definitely not be in nsIBrowserInstance, perhaps somewhere else like nsIWalletService? : boolean walletChangePassword(); possible in js [side note on wallet: nsIWalletService's methods do not need the "WALLET_" prefix the function names should be something like nsIWalletService::prefill, etc. but that's another bug....] : void init(); don't know. Need to investigate. : void setContentWindow( in nsIDOMWindow aWindow ); not sure what this does. involves non-scriptable code however. : void getContentDocShell(out nsIDocShell aDocShell); scriptable via the <browser>'s box object : void setWebShellWindow( in nsIDOMWindow aWindow ); not scriptable, and, sadly, compulsory. : void setTextZoom( in float aTextZoom ); : void SetDocumentCharset(in wstring charset); : wstring GetDocumentCharset(); : void SetForcedCharset(in wstring charset); : void SetForcedDetector(); a whole bunch of stuff using non-scriptable nsIContentViewer etc. How is this stuff specific to browser. Would I not want to have control over these things in mail too? : attribute nsIUrlbarHistory urlbarHistory; urlbarHistory is redundant in this class I think. : void print(); nsIContentViewer.... : void close(); need to investigate w/contributors. I don't understand all the stuff in here. : void copy(); : void selectAll(); IIRC this is all done (or is at least possible) via another, scriptable means now... : void find(); : void findNext(); Scriptable w/nsIFindComponent There's also a bunch of other stuff in there that's not part of the scriptable API, related to content handling and so forth. mscott mentioned I think that this stuff is not necessarily associated with nsBrowserInstance (?) so it might be possible to move that stuff off on its own. There may be other things too, what I'm trying to do at this point is remove the need for the XUL navigator client (or any other XUL app wishing to have browser functionality) to have to possess one of these nasties in order to get things to work. Now, comments? Who is responsible for Page Cycler? It's cvs blamed to Travis but he's not here anymore. We need to get the current authors of code in this file informed of what we want to do.

This is not to say that there is not code in nsBrowserInstance that does not need to be run or is unimportant. It just needs to be integrated with <browser> somehow such that it's attachment is automatic, rather than placed as an added burden upon the XUL content developer.

There are at least two things that may need moved out of this file. In mailnews, when we got rid of the old app core, we created a new class / set of files called: nsMsgWindow. These files contains specific information for a mail application / window. In particular, with regards to uriloading, it contains: the nsIURIContentListener implementation for the mailnews application. This listener is registerd as the parent listener on the content docshell for the message pane. nsIURIContentHandler component registration for content types the mail window should be the preferred handlers for. It contains code to invoke a new mail window when one of these components are invoked. the command line services for the mail application. Right now, the browser instance has the same pieces in it. If we kill nsBrowserInstance completely then we need to move these into a new file, nsBrowserWindow or something like that. I wouldn't be adverse to just removing the nsBrowserInstance class from this file (which contains all the cruft you are really concerned with getting rid of anyway). Just leave these three particular classes alone or create a new file for them.

Adding myself to CC. I will be throwing my back into this. I'm tempted to file a "nsMessenger" must die bug because the old mailnews appcore does exist in this form.. but that's topic for another bug.

I'll attach a few patches in a bit...

oof. So I just killed wallet from nsBrowserInstance, save for one stupid function which needs to be written in JS. But it's a start. Here come the patches, looking for review/approval...

Lots of this depends on making nsIContentViewer scriptable -> bug 57984. I'd like to do that (yet more experience) but can use some help...

I'm now at the point where I need to do bug 57984 before I can continue move stuff out of nsBrowserInstance. === http://lxr.mozilla.org/seamonkey/search?string=XPAppCoresManager There are three files which intensively use appCore for testing: embedding/browser/activex/tests/vbxml/test.xml xpfe/browser/src/navigator-test1.xul xpfe/browser/src/toolbar.xml Then there are a couple of files, in addition to the above, which use XPAppCoresManager: /editor/ui/composer/content/sb-bookmarks.js /editor/ui/composer/content/sb-file-panel.js /intl/strres/tests/strres-test.js /xpfe/browser/samples/dexparamdialog.html /xpfe/browser/samples/dexparamdialog.xul Also note the usage of rdfCore, which should be moved to the relevant rdf services. Since those all seem to be abandoned and/or test files, I suggest new tests are written where so desired. We seem to be using window._content.appCore in a number of places outside the browser code itself to access the appCore functionality, like loadUrl. For the moment I've changed these to use window._content.webNavigation, though it'd be better if we find a better way to access this functionality, for example by passing it on explicitely.

CC'ing shaver. His navigator.js clean-up and my partial clean-up are going to clash painfully.

cc'ing myself. your patch, my patch for 58333, and shaver's clean-up patch are all going to have a good ol' fashioned merge party ;-)

ok, this patch finally completely removes the wallet code from nsBrowserInstance... can I get a r=/a=? I've tested wallet, it seems to be working exactly the same as it was before.

a=ben@netscape.com. Die, nsBrowserInstance, die! Die, redundant C++, die!

r=jag Nice job, Alec.

ok, wallet crap is in.

This will make it easier for me to add accessibility features.

So here's my checkin plan: 1) Make the idl changes first 2) Update js code to not use appCore as much as possible 3) Remove all I can from nsBrowserInstance Each next step only once the previous is successful. I'll attach a patch per step. Next round of this once I've done this batch.

sr=alecf on interview.html diff. Also noted I'll need to update the mac build perl scripts for the added MANIFEST_IDL and update the project files. Note on mini-nav.xul/js: these are completely cut loose from nsBrowserInstance. For the moment the urlbar value isn't updated because registering as WebProgressListener causes a segfault. A work-around is to set it from JS whenever loadURI is called, though I'd rather just not make it segfault :-)

Patch checked in on November 2 broke password manager (single signone) and form manager (wallet). See bug 59125. Their OnEndDocumentLoad notifications are no longer getting called making password manager unusable and hampering form manager as well.

oops! yeah, fix to the other one went in - I inadvertently checked in a different nsAppRunner.cpp than the patch I included above. (the actual patch, which jag/ben reviewed would not have caused that problem)

sr=sfraser on http://bugzilla.mozilla.org/showattachment.cgi?attach_id=18818

The nsIContentViewer move looks good to me except for docshell/base/MANIFEST_IDL missing a newline, a=adamlock@netscape.com

And r=adam@netscape.com for the docshell/webshell bits too

r=erik for Step1-update2

Comments on the latest js stuff, all really stylistic nitpicks rather than any problems. >- appCore.loadUrl(bundle.GetStringFromName("webmailKeyword")); >+ webNavigation.loadURI(bundle.GetStringFromName("webmailKeyword"), > Components.interfaces.nsIWebNavigation.LOAD_FLAGS_NONE); --- [style] Why not create a const for part/all of the constant second parameter, and shorten the line? > if (getWebNavigation().currentURI.spec) { --- Rather than doing this over and over throughout functions, why not store in a local variable? in function Startup(): > + var url_bar = document.getElementById("urlbar"); > + if ( url_bar ) > url_bar.focus(); --- several lines later, gURLBar is defined to be the same node. Why not change url_bar to be an assignment to global gURLBar (the ditch the later assignment). > + document.getElementById('urlbar').value = getWebNavigation().currentURI.spec; --- gURLBar.value = ... Also, 'window.' in usages like window._content, window.openDialog, etc is implicit, and unnecessary. Ideally, I think navigator.js should be re-org'ed into a js object, with getter functions for all the functions you have named 'get<Foo>', so that you can just go this.webNav, etc.

This updates mini-nav.js so I can start chopping bits out of nsBrowserInstance.cpp. I haven't found out yet why using nsIWebProgressListener segfaults, so I can't quite get rid of appCore yet.

oh yeah. r=ben@netscape.com

sr=alecf on those last patches. nice job!

Hrm, the description on that attachment isn't sufficient. On top of refactoring loadInitialPage, it also removes now unused code and merges other code, fixes an old bug (bug 58363) in GetDefaultArgs and adds a contract ID for the command line service. Actually, I'll add a new patch... I should probably use a #define for that contract ID (put it in nsAppShellCIDs.h?), and the mCmdLineURLUsed isn't really a mFoo but a gFoo... Also, finally taking this patch from Ben. Sorry for the spam.

the attached patch requires a fix to libpref to make pref observers work correctly.. just created bug 65616 on this, and set up a dependancy.

r=jag on that patch.

Ah yes, time to remove hyatt's shitty code from nsBrowserInstance. a=ben@netscape.com

looks great! sr=alecf

Patch went in. Suggestions for more killing? Ooh, right. find and findNext. I was getting some weird "Components not in scope: 0" error when I last tried doing that from js. Hrm.

jag, I had to back out your charset stuff, it caused a blocker (bug 65988)

cc nhotta

Hrm... Grrr... I should've gone to hawaii to keep in line with an old Netscape tradition! Okay, I'll take a look at this soon.

coolness! I'll sr= after moz 0.8

sr=alecf

r=law for the postData changes

one thing I realized today is that we can rewrite the nsIURIContentListener part of nsBrowserInstance in C++. The only trick which I haven't figured out is how to get the content area's docshell... I might be able to do it by going through the appcore.getContentDocShell().. patch forthcoming.

ok, the one problem I've run into is that I need to get the docshell for the current window, so that I can call docShell->SetParentURIContentListener(this).. that may involve exposing the docshell through the appcore, I'm not certain. Still working on it.

This looks good to me. r=ben

r=adamlock for the mini-nav stuff.

sr=alecf

We will kill this thing! the latest patch moves another interface implementation into JS, and has some general cleanup.. I haven't quite resolved that bit about getting to QueryInterface from the "window" object, thus the test for "QueryInterface"

ok, the one really really wierd thing with that patch (and I'm not sure if it's actually the patch, or something in my tree) is that I can't load non-query URLs - it seems to be cache related though, I'm not 100% sure.

ok, I was imagining things - I think I was running two instances at the same time. the build works fine with that last patch

I'm going to be nuking appcore.Find and appcore.FindNext as part of the fix for bug 68307

I think we should be moving towards the embedding APIs as much as possible while removing nsBrowserInstance. See bug 76585 for more info.

on my personal taskbar i have link to "todays bugs" in bugzilla. The link is simply named "bugs" and it is the same as found under this text at http://bugzilla.mozilla.org/ View Bugs Already Reported Today Now: When i start mozilla (blank page) i see this in console: Creating a content listener.. nsBrowserContentListener.init([object Window], [object XULElement] When i then click the link on personal toolbar i see: [Exception... "Could not convert JavaScript argument (NULL value can not be used for a C++ reference type) arg 0 [nsIDocShell.QueryInterface]" nsresult: "0x8057000b (NS_ERROR_XPC_BAD_CONVERT_JS_NULL_REF)" location: "JS frame :: chrome://navigator/content/nsBrowserContentListener.js :: anonymous :: line 131" data: no] Tested with a few other links and don't see the same error, but the link as mentioned triggers the exeption each time. This is with a current CVS build, Linux.

sorry, the dump statement was erroneous - this works either way, with or without the dump() of the exception

alecf: nsBrowserContentListener.js contains an erroneous reference to text/xul. That should be application/vnd.mozilla.xul+xml :-) Also, heikki has fixed bug 75031 and bug 65848 so you may want application/xml and application/xhtml+xml, I don't know. Gerv

Mass move to jaggernaut@netscape.com

->moz1.0

on second thought, ->0.9.3, possible limbo candidate

And once again ... -> 0.9.4

Next one out.

By which I meant: -> 0.9.5

Mass-moving lower-priority 0.9.5 bugs off to 0.9.6 to make way for remaining 0.9.4/eMojo bugs, and MachV planning, performance and feature work. If you disagree with any of these targets, please let me know.

->0.9.8

-> 1.1

Comment on attachment 65394 [details] [diff] [review] Remove a bunch of things. sr=ben@netscape.com

Comment on attachment 65394 [details] [diff] [review] Remove a bunch of things. hot damn! r=alecf

Comment on attachment 66262 [details] [diff] [review] More clean-up everything looks fine except that printf() :) sr=alecf with that removed

I'm not adding any printfs, merely changing existing ones.

Comment on attachment 66262 [details] [diff] [review] More clean-up r=ben@netscape.com

filter keyword: nothingnessless

This bug had both r= and sr= before the 0.9.9 freeze, can we get it in ?

The last patch was checked in a while ago. A little more work can be done here, for 1.1 :-)

this patch removes the init function which does nothing and stops viewsource from using nsIBrowserInstance because it doesn't need it.

Comment on attachment 118219 [details] [diff] [review] some more removals (checked in) sr=alecf nice.. as long as view source works of course :)

Comment on attachment 118219 [details] [diff] [review] some more removals (checked in) r=jag

Comment on attachment 118219 [details] [diff] [review] some more removals (checked in) ok, I just checked this patch in alecf: it sure does, it was the first thing I tested :)

Some patches have been checked in. What is the current status of this bug?

These patches should be checked in finally! This bug was last changed 3 years ago, and it is still not fixed!

Which patches haven't been checked in? Comment 93 and comment 97 make it sound like all of them _are_ checked in. Please check CVS logs to confirm?

nsIBrowserInstance still exists, so even though the patches have been checked in, this bug isn't fixed.

So... http://mxr.mozilla.org/mozilla-central/search?string=nsibrowserinstance and http://mxr.mozilla.org/mozilla-central/search?string=component/browser/instance only list the interface/component used in (Firefox's) browser.js In browser.js the component is created only once and stored in a global variable appCore. http://mxr.mozilla.org/mozilla-central/search?string=appCore shows this variable is only used in browser.js: # line 924 -- appCore.startPageCycler(); -- #ifdef'ed ENABLE_PAGE_CYCLER -- I don't see this being defined anywhere. Plus anyway nsBrowserInstance::StartPageCycler is a no-op. # line 1107 -- appCore = Components.classes["@mozilla.org/appshell/component/browser/instance;1"] # line 1145 -- appCore.setWebShellWindow(window); -- startup initialization. The component stores references to the chrome |window| and the content window, but doesn't appear to do anything else useful. Also has some debug code, which doesn't appear useful. # line 1433 -- if (appCore) # line 1434 -- appCore.close(); -- on shutdown. Doesn't do anything useful (sets a flag, which doesn't seem to affect anything). Looks like it can be killed. Anything I missed?

(In reply to comment #102) > http://mxr.mozilla.org/mozilla-central/search?string=nsibrowserinstance and > http://mxr.mozilla.org/mozilla-central/search?string=component/browser/instance > only list the interface/component used in (Firefox's) browser.js Generally, it's more useful to search comm-central instead of mozilla-central if you don't want just Firefox results but TB/SM also. > Looks like it can be killed. Anything I missed? SM uses two more members in navigator.js, but I'm not sufficiently familiar with this code to judge it.

Not an XPCOM bug.

Just so that it's not thrown away, here's the patch removing nsIBrowserInstance from mozilla-central. As Karsten noted, in this form it would break comm-central.

Bug 511710 removed the SeaMonkey references, actually, so once bug 513469 is fixed we can just remove it. I filed bug 513507.