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

RESOLVED FIXED

Status

()

Core
XPCOM
--
major
RESOLVED FIXED
18 years ago
9 years ago

People

(Reporter: Ben Goodger (use ben at mozilla dot org for email), Assigned: jag (Peter Annema))

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(25 attachments)

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
Alec Flett
: review+
Ben Goodger (use ben at mozilla dot org for email)
: superreview+
Details | Diff | Splinter Review
12.53 KB, patch
Ben Goodger (use ben at mozilla dot org for email)
: review+
Alec Flett
: superreview+
Details | Diff | Splinter Review
4.49 KB, patch
jag (Peter Annema)
: review+
Alec Flett
: superreview+
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);
    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. 
Status: NEW → ASSIGNED
Summary: nsIBrowserInstance must die! → nsIBrowserInstance must die!

Comment 2

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

Comment 3

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

Comment 4

18 years ago
I'll attach a few patches in a bit...

Comment 5

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

Comment 6

18 years ago
Created attachment 18118 [details] [diff] [review]
Wallet: nsBrowserInstance patch #1

Comment 7

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

Comment 8

18 years ago
Created attachment 18119 [details] [diff] [review]
Wallet: nsWalletService patch #1

Comment 9

18 years ago
Created attachment 18120 [details] [diff] [review]
Wallet: JS patch #1

Comment 10

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

Comment 11

18 years ago
Created attachment 18239 [details] [diff] [review]
[patch] appCore -> WebNavigation bits

Comment 12

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

Comment 13

18 years ago
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 ;-)

Comment 14

18 years ago
Created attachment 18578 [details] [diff] [review]
complete updated patch

Comment 15

18 years ago
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!

Comment 17

18 years ago
r=jag

Nice job, Alec.

Comment 18

18 years ago
ok, wallet crap is in.

Comment 19

18 years ago
This will make it easier for me to add accessibility features.
Keywords: access

Comment 20

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

Comment 21

18 years ago
Created attachment 18677 [details] [diff] [review]
[patch] interview.html, alecf's wallet changes

Comment 22

18 years ago
Created attachment 18679 [details] [diff] [review]
[patch] Step1: add idls, update makefiles, add fu to xulBindings.xml

Comment 23

18 years ago
Created attachment 18680 [details] [diff] [review]
[patch] Step2: update javascript appCore mess

Comment 24

18 years ago
Created attachment 18681 [details] [diff] [review]
[patch] Step3: remove crap from nsBrowserInstance and friends

Comment 25

18 years ago
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 :-)

Comment 26

18 years ago
Created attachment 18818 [details] [diff] [review]
[patch] Step1-update1: add new MANIFEST_IDL to mac build scripts

Updated

18 years ago
Keywords: approval, patch, review

Comment 27

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

Comment 28

18 years ago
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)

Comment 30

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

Comment 31

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

Comment 32

18 years ago
Created attachment 19081 [details] [diff] [review]
[patch] Step1-update2: move CID and CONTRACTID out of nsIDocumentCharsetInfo.idl

Comment 33

18 years ago
r=erik for Step1-update2

Comment 34

18 years ago
Created attachment 19305 [details] [diff] [review]
[patch] updated version of step2

Updated

18 years ago
Blocks: 52859
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.

Updated

18 years ago
Blocks: 55798

Comment 36

18 years ago
Created attachment 20508 [details] [diff] [review]
[patch] interim update of mini-nav.js

Comment 37

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

Comment 38

18 years ago
Created attachment 20512 [details] [diff] [review]
[patch] some more js updates to allow the next patch

Comment 39

18 years ago
Created attachment 20513 [details] [diff] [review]
[patch] Remove bits 'n' pieces from nsBrowserInstance

Comment 41

18 years ago
sr=alecf on those last patches. nice job!

Comment 42

18 years ago
Created attachment 21375 [details] [diff] [review]
[patch] refactor loadInitialPage related code

Comment 43

18 years ago
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
Status: ASSIGNED → NEW

Updated

18 years ago
Blocks: 64596

Comment 44

18 years ago
Created attachment 22670 [details] [diff] [review]
move button hiding/showing to navigator.js

Comment 45

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

Comment 46

18 years ago
r=jag on that patch.
Ah yes, time to remove hyatt's shitty code from nsBrowserInstance. 

a=ben@netscape.com

Comment 48

18 years ago
Created attachment 22853 [details] [diff] [review]
[patch] Move character set related code from BI to navigator.js and viewsource.js

Comment 49

18 years ago
looks great! sr=alecf

Comment 51

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

Comment 52

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

Comment 53

18 years ago
cc nhotta

Comment 54

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

Comment 55

18 years ago
Created attachment 24515 [details] [diff] [review]
[patch] we can get postData from sessionHistory now

Comment 56

18 years ago
coolness! I'll sr= after moz 0.8

Comment 57

18 years ago
sr=alecf

Comment 58

18 years ago
r=law for the postData changes

Comment 59

18 years ago
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.
Status: NEW → ASSIGNED

Comment 60

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

Updated

18 years ago
Depends on: 73640

Comment 61

18 years ago
Created attachment 30055 [details] [diff] [review]
[patch] Move ProgressListener from BI to js, remove js wrapper layer, place it all in its own file.
This looks good to me. 
r=ben

Comment 63

17 years ago
r=adamlock for the mini-nav stuff. 

Comment 64

17 years ago
sr=alecf

Comment 65

17 years ago
Created attachment 30506 [details] [diff] [review]
move content listener into JS

Comment 66

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

Comment 67

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

Comment 68

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

Comment 70

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

Comment 71

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

Comment 72

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

Comment 73

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

Comment 75

17 years ago
Mass move to jaggernaut@netscape.com
Assignee: jag → jaggernaut
Status: ASSIGNED → NEW

Comment 76

17 years ago
->moz1.0
Target Milestone: --- → mozilla1.0

Comment 77

17 years ago
on second thought, ->0.9.3, possible limbo candidate
Target Milestone: mozilla1.0 → mozilla0.9.3

Updated

17 years ago
Keywords: access
(Assignee)

Comment 78

17 years ago
And once again ... -> 0.9.4
Target Milestone: mozilla0.9.3 → mozilla0.9.4
(Assignee)

Comment 79

17 years ago
Next one out.
Target Milestone: mozilla0.9.4 → mozilla0.9.5
(Assignee)

Comment 80

17 years ago
By which I meant: -> 0.9.5

Comment 81

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

Comment 82

17 years ago
->0.9.8
Target Milestone: mozilla0.9.6 → mozilla0.9.8
(Assignee)

Comment 83

17 years ago
-> 1.1
Target Milestone: mozilla0.9.8 → mozilla1.1
(Assignee)

Comment 84

17 years ago
Created attachment 65394 [details] [diff] [review]
Remove a bunch of things.
Comment on attachment 65394 [details] [diff] [review]
Remove a bunch of things.

sr=ben@netscape.com
Attachment #65394 - Flags: superreview+

Updated

17 years ago
Attachment #65394 - Flags: review+

Comment 86

17 years ago
Comment on attachment 65394 [details] [diff] [review]
Remove a bunch of things.

hot damn!
r=alecf
(Assignee)

Comment 87

17 years ago
Created attachment 66262 [details] [diff] [review]
More clean-up

Comment 88

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

Comment 89

17 years ago
I'm not adding any printfs, merely changing existing ones.
(Assignee)

Comment 91

17 years ago
filter keyword: nothingnessless
This bug had both r= and sr= before the 0.9.9 freeze, can we get it in ?
(Assignee)

Comment 93

17 years ago
The last patch was checked in a while ago. A little more work can be done here,
for 1.1 :-)
Created attachment 118219 [details] [diff] [review]
some more removals (checked in)

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 95

16 years ago
Comment on attachment 118219 [details] [diff] [review]
some more removals (checked in)

sr=alecf
nice.. as long as view source works of course :)
Attachment #118219 - Flags: superreview?(alecf) → superreview+
(Assignee)

Comment 96

16 years ago
Comment on attachment 118219 [details] [diff] [review]
some more removals (checked in)

r=jag
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)

Comment 98

15 years ago
Some patches have been checked in. What is the current status of this bug?
Product: Browser → Seamonkey

Comment 99

12 years ago
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 → ---

Updated

10 years ago
Component: General → XPCOM
Product: SeaMonkey → Core
QA Contact: general → xpcom

Comment 102

9 years ago
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?

Comment 103

9 years ago
(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.

Comment 104

9 years ago
Not an XPCOM bug.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → INVALID

Comment 105

9 years ago
Created attachment 393373 [details] [diff] [review]
nsIBrowserInstance removal from mozilla-central 

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.

Updated

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