Last Comment Bug 758812 - Break up browser.js into smaller pieces, Phase 1
: Break up browser.js into smaller pieces, Phase 1
Status: RESOLVED FIXED
: dev-doc-needed
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 15
Assigned To: Justin Dolske [:Dolske]
:
Mentors:
: 381210 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-25 15:08 PDT by Justin Dolske [:Dolske]
Modified: 2013-05-15 14:26 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for gPluginHandler (61.79 KB, patch)
2012-05-25 15:55 PDT, Justin Dolske [:Dolske]
gavin.sharp: review+
Details | Diff | Review
Patch for LightWeightThemeInstaller (13.04 KB, patch)
2012-05-25 15:56 PDT, Justin Dolske [:Dolske]
gavin.sharp: review+
Details | Diff | Review
Patch for FeedHandler (113 bytes, patch)
2012-05-25 15:56 PDT, Justin Dolske [:Dolske]
no flags Details | Diff | Review
Patch for FullScreen and feeds (64.15 KB, patch)
2012-05-25 15:56 PDT, Justin Dolske [:Dolske]
gavin.sharp: review+
Details | Diff | Review
Patch for 2 addon things (19.35 KB, patch)
2012-05-25 15:57 PDT, Justin Dolske [:Dolske]
gavin.sharp: review+
Details | Diff | Review

Description Justin Dolske [:Dolske] 2012-05-25 15:08:57 PDT
It is widely acknowledged that browser.js is too damn bug. The in-tree version is currently 9431 lines long. (It's actually 13,193 lines if you count the 6 files that are already #included)

The first step to fixing our problem is to blast out some of the big and obvious chunks into separate files and #include them. If nothing else, this starts to make looking at code history easier (because history for browser-foo.js will be separate from browser-bar.js).

My plan is to start with splitting out things like "var gFoo = { ... }" in a few steps. Later on, when the dust settles, we can look at moving related global functions into their own files as well.
Comment 1 Justin Dolske [:Dolske] 2012-05-25 15:55:19 PDT
Created attachment 627395 [details] [diff] [review]
Patch for gPluginHandler

(I'm only going to set the review flag on this first patch, because multiple flags are so annoying. The intention is for all to be reviewed. Note that there are no code changes here -- just moving chunks and adding #includes)
Comment 2 Justin Dolske [:Dolske] 2012-05-25 15:56:02 PDT
Created attachment 627396 [details] [diff] [review]
Patch for LightWeightThemeInstaller
Comment 3 Justin Dolske [:Dolske] 2012-05-25 15:56:30 PDT
Created attachment 627397 [details] [diff] [review]
Patch for FeedHandler
Comment 4 Justin Dolske [:Dolske] 2012-05-25 15:56:57 PDT
Created attachment 627398 [details] [diff] [review]
Patch for FullScreen and feeds
Comment 5 Justin Dolske [:Dolske] 2012-05-25 15:57:24 PDT
Created attachment 627399 [details] [diff] [review]
Patch for 2 addon things
Comment 6 Justin Dolske [:Dolske] 2012-05-25 16:05:12 PDT
Comment on attachment 627397 [details] [diff] [review]
Patch for FeedHandler

Oops, I seem to have merged the FeedHandler and FullScreen changes. Meh.
Comment 7 Justin Dolske [:Dolske] 2012-05-29 08:16:43 PDT
I did a try run just to make sure nothing unexpected broke. It's ok.

https://tbpl.mozilla.org/?tree=Try&rev=532c60405f19

It also occurred to me that browser-lwt.js could be grouped into browser-addons.js.
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-30 16:20:18 PDT
*** Bug 381210 has been marked as a duplicate of this bug. ***
Comment 9 Dão Gottwald [:dao] 2012-05-31 03:48:54 PDT
Any reason why LightWeightThemeInstaller isn't in browser-addons.js?
Comment 10 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-05-31 10:43:37 PDT
I don't feel strongly; it seems pretty self-contained and large enough to merit its own file.
Comment 11 Dão Gottwald [:dao] 2012-05-31 11:40:22 PDT
Well, gXPInstallObserver and AddonsMgrListener are similarly self-contained.
Comment 12 Justin Dolske [:Dolske] 2012-05-31 18:55:49 PDT
(In reply to Dão Gottwald [:dao] from comment #9)
> Any reason why LightWeightThemeInstaller isn't in browser-addons.js?

From comment 7:

> It also occurred to me that browser-lwt.js could be grouped into browser-addons.js.

I was just going to do that for checkin.
Comment 15 Eric Shepherd [:sheppy] 2012-06-04 07:43:02 PDT
This will need to be mentioned to devs that poke around within the product; most already know, but it's still worth noting on Firefox 15 for developers.
Comment 16 Justin Dolske [:Dolske] 2012-06-07 14:48:38 PDT
Sheppy: doesn't hurt, but I'd be careful to note that there's essentially no change to the browser.js that _ships_ with Firefox, it's only the files as they live in mozilla-central that have been split. (The magic of #include.)

Note You need to log in before you can comment on or make changes to this bug.