Closed Bug 1228627 Opened 9 years ago Closed 9 years ago

Investigate removal of #includes from browser.js

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: Felipe, Assigned: Felipe)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

An attempt to remove the #includes pre-processing from browser.js. I sent to tryserver a patch that moves all includes into script tags to global-scripts.inc. Let's see if it breaks anything and what talos looks like. Perhaps some of these could go to browser.xul instead of global-scripts.inc. This could be even a win for Mac because that would make macBrowserOverlay.xul smaller. https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1e44969f5f8
I think we should add as few as possible to global-scripts.inc since IIRC it's all duped content on Mac (not sure why chatWindow includes it...). I don't think it's a great idea to move stuff to it. I suspect most of the things we include in browser.js could be factored out to modules, some could be instanced per window, some just get window/document as argument. That's a big chunk of work though. For sure we should be more critic about what we #include, there's almost always a better alternative.
Hm should have probably built locally first before sending to try.. Obviously these files don't exist in jar.mn so it's not just moving them to a <script> tag
(In reply to Marco Bonardo [::mak] from comment #1) > I think we should add as few as possible to global-scripts.inc since IIRC > it's all duped content on Mac (not sure why chatWindow includes it...). I > don't think it's a great idea to move stuff to it. Yeah, I intend to include as little as possible in global-scripts.inc. But right now the intention is just to keep things the way they are, but remove the preprocessing in browser.js (so that we can run eslint there). In theory this change shouldn't break anything because browser.js itself is included in global-scripts.inc and all of these files where included in it. But this will open the opportunity to optimze things further.
Gonna assign it to me for the moment as I'm playing with this, but I make no promise at the moment to go with this through the end if it takes longer than I can spare :)
Assignee: nobody → felipc
Status: NEW → ASSIGNED
ah right, I didn't recall that browser.js itself was included.
Attached patch remove-includes-browserjs (deleted) — Splinter Review
Alright, this seems to be working locally with a quick sanity check, + 5min of mochitest-browser running. Now on to tryserver to do the full run. https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ad52320bf80
New push remote: Follow the progress of your build on Treeherder: remote: https://treeherder.mozilla.org/#/jobs?repo=try&revision=76e7c3aacf97 remote: remote: It looks like this try push has talos jobs. Compare performance against a baseline revision: remote: https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=76e7c3aacf97
Bug 1228627 - Remove #includes from browser.js. r?Gijs
Attachment #8693397 - Flags: review?(gijskruitbosch+bugs)
A number of these files still need to go through the preprocessor because they have #ifdefs in them. I'm removing all of that in a separate bug, bug 1228655. One minor question here is that some, but not all of these files, have emacs indent configs in the first line. I decided not to change any, but I wondered if the same settings should be added to the files that don't have it.
Comment on attachment 8693397 [details] MozReview Request: Bug 1228627 - Remove #includes from browser.js. r?Gijs https://reviewboard.mozilla.org/r/26445/#review23885 ::: browser/base/content/browser-tabview.js:1 (Diff revision 1) > -# This Source Code Form is subject to the terms of the Mozilla Public > -# License, v. 2.0. If a copy of the MPL was not distributed with this > -# file, You can obtain one at http://mozilla.org/MPL/2.0/. > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ Mumble mumble why you bitrottin' my patches. ;-)
Attachment #8693397 - Flags: review?(gijskruitbosch+bugs) → review+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Blocks: 1150859
Blocks: eslint
Wow, nice! I must admit I'm a little surprised this didn't cause a perf regression. I thought we've run into this before, where adding more code in a <script> caused a startup regression but #including it in browser.js didn't.
(In reply to Justin Dolske [:Dolske] from comment #14) > Wow, nice! > > I must admit I'm a little surprised this didn't cause a perf regression. I > thought we've run into this before, where adding more code in a <script> > caused a startup regression but #including it in browser.js didn't. I wouldn't be surprised if the regression my panorama removal code caused might have hidden this. If so though, it would have been a small regression (< 2%). I think it would be worth checking that and/or if we can add a build system hack that concats all the scripts in global-scripts.inc and uses that instead of browser.xul, and see if that gets us a noticeable perf win. Should be easy to hack up, the hard part will be ensuring we can figure out debug information and so on. Might be worth a followup bug? Justin, what do you think?
Flags: needinfo?(dolske)
I was surprised at first too but I triggered 4x talos runs for all platforms and used compare-talos against m-c before the Panorama removal and didn't spot any problems. Could be worth to double check.
If <script> isn't actually causing a regression, let's stick with that.
Flags: needinfo?(dolske)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: