Closed Bug 1482707 Opened 3 years ago Closed 3 years ago

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

Categories

(SeaMonkey :: Tabbed Browser, enhancement)

enhancement
Not set
normal

Tracking

(seamonkey2.49esr wontfix, seamonkey2.53 fixed, seamonkey2.57esr fixed, seamonkey2.60 fixed)

RESOLVED FIXED
seamonkey2.60
Tracking Status
seamonkey2.49esr --- wontfix
seamonkey2.53 --- fixed
seamonkey2.57esr --- fixed
seamonkey2.60 --- fixed

People

(Reporter: frg, Assigned: frg)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
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.
Flags: needinfo?(frgrahl)
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.
Flags: needinfo?(frgrahl)
2.53 version
2.57 version
Blocks: 1323494
Attachment #8999448 - Flags: review?(iann_bugzilla)
Attachment #8999448 - Flags: approval-comm-esr60?
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
Attachment #8999448 - Flags: review?(iann_bugzilla)
Attachment #8999448 - Flags: review+
Attachment #8999448 - Flags: approval-comm-esr60?
Attachment #8999448 - Flags: approval-comm-esr60+
> >+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.
Flags: needinfo?(iann_bugzilla)
(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.
Flags: needinfo?(iann_bugzilla)
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/3fdac2ab1f40
Merge browser.js and navigator.js in suite/browser. r=IanN
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Blocks: 1484830
You need to log in before you can comment on or make changes to this bug.