Closed Bug 46200 Opened 20 years ago Closed 11 years ago

remove parts of nsIBrowserInstance (was: nsIBrowserInstance must die!)


(Core :: XPCOM, defect, major)

Not set





(Reporter: bugs, Assigned: jag+mozilla)




(25 files)

6.08 KB, patch
Details | Diff | Splinter Review
3.17 KB, patch
Details | Diff | Splinter Review
2.68 KB, patch
Details | Diff | Splinter Review
58.27 KB, patch
Details | Diff | Splinter Review
15.24 KB, patch
Details | Diff | Splinter Review
960 bytes, patch
Details | Diff | Splinter Review
23.84 KB, patch
Details | Diff | Splinter Review
49.97 KB, patch
Details | Diff | Splinter Review
33.45 KB, patch
Details | Diff | Splinter Review
1.22 KB, patch
Details | Diff | Splinter Review
10.57 KB, patch
Details | Diff | Splinter Review
38.08 KB, patch
Details | Diff | Splinter Review
3.15 KB, patch
Details | Diff | Splinter Review
3.03 KB, patch
Details | Diff | Splinter Review
19.97 KB, patch
Details | Diff | Splinter Review
33.19 KB, patch
Details | Diff | Splinter Review
7.00 KB, patch
Details | Diff | Splinter Review
11.36 KB, patch
Details | Diff | Splinter Review
4.71 KB, patch
Details | Diff | Splinter Review
39.39 KB, patch
Details | Diff | Splinter Review
18.85 KB, patch
Details | Diff | Splinter Review
7.03 KB, patch
: review+
Details | Diff | Splinter Review
12.53 KB, patch
: review+
Details | Diff | Splinter Review
4.49 KB, patch
: review+
Details | Diff | Splinter Review
33.22 KB, patch
Details | Diff | Splinter Review
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);

: 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

: 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_" 
 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();

: 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. 
Summary: nsIBrowserInstance must die! → nsIBrowserInstance must die!
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...
Depends on: 57984
I'm now at the point where I need to do bug 57984 before I can continue move
stuff out of nsBrowserInstance.


There are three files which intensively use appCore for testing:

Then there are a couple of files, in addition to the above, which use

Also note the usage of rdfCore, which should be moved to the relevant rdf

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. Die, nsBrowserInstance, die! Die, redundant C++, die!

Nice job, Alec.
ok, wallet crap is in.
This will make it easier for me to add accessibility features.
Keywords: access
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 :-)
Keywords: approval, patch, review
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)
The nsIContentViewer move looks good to me except for 
docshell/base/MANIFEST_IDL missing a newline,
And for the docshell/webshell bits too
r=erik for Step1-update2
Blocks: 52859
Comments on the latest js stuff, all really stylistic nitpicks rather than any

>-          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.
Blocks: 55798
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.
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.
Assignee: ben → disttsc
Blocks: 64596
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.
Depends on: 65616
r=jag on that patch.
Ah yes, time to remove hyatt's shitty code from nsBrowserInstance.
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

Okay, I'll take a look at this soon.
coolness! I'll sr= after moz 0.8
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.
Depends on: 73640
This looks good to me. 
r=adamlock for the mini-nav stuff. 
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
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

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.

Mass move to
Assignee: jag → jaggernaut
Target Milestone: --- → mozilla1.0
on second thought, ->0.9.3, possible limbo candidate
Target Milestone: mozilla1.0 → mozilla0.9.3
Keywords: access
And once again ... -> 0.9.4
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Next one out.
Target Milestone: mozilla0.9.4 → mozilla0.9.5
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.
Target Milestone: mozilla0.9.5 → mozilla0.9.6
Target Milestone: mozilla0.9.6 → mozilla0.9.8
-> 1.1
Target Milestone: mozilla0.9.8 → mozilla1.1
Comment on attachment 65394 [details] [diff] [review]
Remove a bunch of things.
Attachment #65394 - Flags: superreview+
Attachment #65394 - Flags: review+
Comment on attachment 65394 [details] [diff] [review]
Remove a bunch of things.

hot damn!
Attached patch More clean-upSplinter Review
Comment on attachment 66262 [details] [diff] [review]
More clean-up

everything looks fine except that printf() :)

sr=alecf with that removed
Attachment #66262 - Flags: superreview+
I'm not adding any printfs, merely changing existing ones.
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.
Attachment #118219 - Flags: superreview?(alecf)
Attachment #118219 - Flags: review?(jaggernaut)
Comment on attachment 118219 [details] [diff] [review]
some more removals (checked in)

nice.. as long as view source works of course :)
Attachment #118219 - Flags: superreview?(alecf) → superreview+
Comment on attachment 118219 [details] [diff] [review]
some more removals (checked in)

Attachment #118219 - Flags: review?(jaggernaut) → review+
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 :)
Attachment #118219 - Attachment description: some more removals → some more removals (checked in)
Some patches have been checked in. What is the current status of this bug?
Product: Browser → Seamonkey
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.
Priority: P3 → --
QA Contact: doronr → general
Target Milestone: mozilla1.1alpha → ---
Component: General → XPCOM
Product: SeaMonkey → Core
QA Contact: general → xpcom
So... and 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. 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[";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)
> and
> 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.
Closed: 11 years ago
Resolution: --- → INVALID
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.
Depends on: 513469
Bug 511710 removed the SeaMonkey references, actually, so once bug 513469 is fixed we can just remove it. I filed bug 513507.
Depends on: 513507
Resolution: INVALID → FIXED
Summary: nsIBrowserInstance must die! → remove some parts of (was: nsIBrowserInstance must die!)
No longer depends on: 513507
Summary: remove some parts of (was: nsIBrowserInstance must die!) → remove parts of nsIBrowserInstance (was: nsIBrowserInstance must die!)
You need to log in before you can comment on or make changes to this bug.