1482707 - Merge browser.js and navigator.js in suite/browser

Status: RESOLVED FIXED

(SeaMonkey :: Tabbed Browser, enhancement)

Description

[Frank-Rainer Grahl (:frg)]

SeaMonkey has only some utility functions in browser.js. The whole functionality is mostly in navigator.js. Both files are only referenced and unconditionally loaded in navigatorOverlay.xul.

Unless I miss something I think it does not make sense to do this. navigator.js is somewhat similar to browser.js in Firefox so everything should go into this one.

Comment 1

[bblack]

I have some time in the next week to do this.
I do not need a compiler for this and no development environment? 
If that's not very urgent, I could do it.

Comment 2

[Frank-Rainer Grahl (:frg)]

Thanks for the offer. Did it just before I filed the bug that is why I assigned it to myself.

If you need help setting up a dev environment let me know. These things are usually done quick if they can be locally tested.

Comment 3

[Frank-Rainer Grahl (:frg)]

Attachment: 1482707-cleanbrowser-253.patch

2.53 version

Comment 4

[Frank-Rainer Grahl (:frg)]

Attachment: 1482707-cleanbrowser-257.patch

2.57 version

Comment 5

[Ian Neal]

Comment on attachment 8999448 [details] [diff] [review]
1482707-cleanbrowser-257.patch

>+++ b/suite/browser/navigator.js

>+const nsIWebNavigation = Ci.nsIWebNavigation;
As this is only used the once, it might as well be inlined.

>+var gPrintSettingsAreGlobal = true;
>+var gSavePrintSettings = true;
Are these two variables used anywhere?

>+++ b/suite/components/nsISuiteGlue.idl

> /**
>  * ***** this is the suite version of nsIBrowserGlue *****
>  * nsISuiteGlue is a dirty and rather fluid interface to host shared utility
>  * methods used by suite UI code, but which are not local to a suite window.
>  * The component implementing this interface is meant to be a singleton
>  * (service) and should progressively replace some of the shared "glue" code
>  * scattered in suite/ (e.g. bits of utilOverlay.js,
>- * contentAreaUtils.js, globalOverlay.js, browser.js), avoiding dynamic
>+ * contentAreaUtils.js, globalOverlay.js), avoiding dynamic
>  * inclusion and initialization of a ton of JS code for *each* window.
>  * Dued to its nature and origin, this interface won't probably be the most
Nit: whilst here, fix spelling should be Due rather than Dued

Is this patch a precursor to some modernisation?

r/a=me

Comment 6

[Frank-Rainer Grahl (:frg)]

> >+const nsIWebNavigation = Ci.nsIWebNavigation;
> As this is only used the once, it might as well be inlined.

It is used a few times in navigator.js. Inlining it here breaks the browser. Been there done that :) Maybe switch it over globally to Ci.nsIWebNavigation in a follow-up?

> >+var gPrintSettingsAreGlobal = true;
> >+var gSavePrintSettings = true;
> Are these two variables used anywhere?

Used in printUtils.js which is referenced in navigator.xul. It has its own global but is not a jsm so not sure which one wins. Probably find out in a follow-up bug because unrelated to the merge?

> Is this patch a precursor to some modernisation?

No. As stated above just an optimization. Pulling only one script in should be faster, uses a tiny amount less memory and browser.js is not that big either. If/when we do the overlay removal this should make things 0,1% easier.

Comment 7

[Ian Neal]

(In reply to Frank-Rainer Grahl (:frg) from comment #6)
> > >+const nsIWebNavigation = Ci.nsIWebNavigation;
> > As this is only used the once, it might as well be inlined.
> 
> It is used a few times in navigator.js. Inlining it here breaks the browser.
> Been there done that :) Maybe switch it over globally to Ci.nsIWebNavigation
> in a follow-up?
Well there should be some consistency, so yes a follow-up.
> 
> > >+var gPrintSettingsAreGlobal = true;
> > >+var gSavePrintSettings = true;
> > Are these two variables used anywhere?
> 
> Used in printUtils.js which is referenced in navigator.xul. It has its own
> global but is not a jsm so not sure which one wins. Probably find out in a
> follow-up bug because unrelated to the merge?

A follow-up if it is found they are not needed.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3fdac2ab1f40
Merge browser.js and navigator.js in suite/browser. r=IanN

Comment 9

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/releases/comm-esr60/rev/52a7c1776f354fe2471826f5e7ed034569033a66

Comment 10

[Frank-Rainer Grahl (:frg)]

https://gitlab.com/seamonkey-project/seamonkey-2.53-comm/-/commit/0cb2abeb63b29409b87a9622513792e8cd1cdf3a
Merge browser.js and navigator.js in suite/browser.
Target 2.53 beta 1