Last Comment Bug 720294 - Refactor style sheet switching code in browser.js
: Refactor style sheet switching code in browser.js
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 13
Assigned To: Justin Dolske [:Dolske]
:
:
Mentors:
Depends on: 733339
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-22 20:06 PST by Justin Dolske [:Dolske]
Modified: 2013-11-12 00:56 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v.1 (7.97 KB, patch)
2012-01-22 20:07 PST, Justin Dolske [:Dolske]
gavin.sharp: feedback+
Details | Diff | Splinter Review
Patch v.2 (7.96 KB, patch)
2012-02-29 21:28 PST, Justin Dolske [:Dolske]
gavin.sharp: review+
Details | Diff | Splinter Review
Patch v.3 (7.72 KB, patch)
2012-03-04 18:30 PST, Justin Dolske [:Dolske]
no flags Details | Diff | Splinter Review

Description Justin Dolske [:Dolske] 2012-01-22 20:06:10 PST
As is well-known, browser.js is just a little bit of a mess. Lots of old global functions, lots of... well, lots and lots of just stuff.

As a first step towards fixing the madness, seems like it would be good to start grouping together bits of related functionality into methods on a new gRelatedStuff object. To retain backwards compatibility, we can keep the old function identifiers as pointers into the new object.

Example:

  function bar() { ... }
  function baz() { ... }

becomes

  var gFoo = {
    bar : function() { ... }
    baz : function() { ... }
  };
  var bar = gFoo.bar;
  var baz = gFoo.baz;

Eventually, window.bar and window.baz could go away, and gFoo can be migrated to a Foo.jsm. (I suppose the steps are technically independent).


Risk here is fairly low, the two main issues I can think of are:

* Need to watch for places where |this| is affected (was |window|, now |gFoo|). Given low test coverage of some of the more crufty code, this will need manual verification. But it's pretty mechanical.
* Addons that override / intercept / alter the functions that move would need to update sooner rather than later. Otherwise code calling |gFoo.bar()| directly would not be caught.
* Code blame will be off, but that's going to happen anyway if/when we split things into JSMs.
Comment 1 Justin Dolske [:Dolske] 2012-01-22 20:07:27 PST
Created attachment 590623 [details] [diff] [review]
Patch v.1

Here's a first-take of doing so with the View -> Page Style menu helpers.

I should probably make and attach a part 2 for updating existing callers if this makes sense...
Comment 2 Justin Dolske [:Dolske] 2012-01-22 20:08:45 PST
Oh, I should note that other than fixing indenting and changing |function foo(a,b)| to |foo : function(a,b)| there are no other changes to the code.

A whitespace diff viewer would sure be nice in bugzilla. :(
Comment 3 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-23 15:52:25 PST
Comment on attachment 590623 [details] [diff] [review]
Patch v.1

Yeah, this seems like a good plan. I think that for some of these, we can just go ahead and remove the globals in one shot, but we'll have to decide that case-by-case. You should probably change the implementations to use this.foo() rather than foo(), in the cases where you're referring to other functions that have moved.

For the cases where we do want to preserve compat, we could have a generic makeCompatWrapper() function that does something like:

function makeCompatWrapper(helperObjName, funName) {
  window[funName] = function () {
    Components.utils.reportError("Please don't call " + funName + ", use " +
                                 helperObjName + "." + funName + "() instead.")
    window[helperObjName][funName].apply(this, arguments);
  };
}

And then use it like:
makeCompatWrapper("gPageStyleHelper", "getAllStyleSheets");
Comment 4 Dão Gottwald [:dao] 2012-01-24 03:05:33 PST
Comment on attachment 590623 [details] [diff] [review]
Patch v.1

>+var gPageStyleHelper = {

gPageStyleMenu or even just gPageStyle will do. "helper" is kind of meaningless. (I'm quite happy that LocationBarHelpers is gone: <http://mxr.mozilla.org/mozilla1.9.2/search?string=LocationBarHelpers>)

I'm not sure why this should be moved to a jsm (to make browser.js easier to hack? #include can help with that), but if so, you could drop the "g" right now.

>+  getAllStyleSheets : function (frameset) {

nit: no space before the colon
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-24 13:01:11 PST
(In reply to Dão Gottwald [:dao] from comment #4)
> I'm not sure why this should be moved to a jsm (to make browser.js easier to
> hack? #include can help with that), but if so, you could drop the "g" right
> now.

Having this code in a module would mean one copy of the code (both to load, and in memory), which might result in some Txul/footprint wins if we do enough of it. It also means needing to worry less about scope pollution and different code interfering with each other, generally.
Comment 6 Dão Gottwald [:dao] 2012-01-24 13:29:50 PST
What scale of txul and footprint reduction are we talking about? I would assume this to be neglectable.

With regards to scope pollution, this doesn't seem much different either way. We need to be deliberate about what to define directly in browser.js and we need to be deliberate about what to export from modules. The main difference is with private code, which should be underscore-prefixed in browser.js implementations and can be spread out in modules. There's a special scope pollution problem with modules, though: people import them carelessly and pollute the global scope. I'm dealing with this right now, e.g.: https://hg.mozilla.org/mozilla-central/rev/fd894e4a7569

Another problem with modules is that we often don't want the modularity. The page style functions are only needed in the browser window, nowhere else. They don't need to be pluggable. What they need is access to the chrome and content window; passing these around isn't going to simplify the code.
Comment 7 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-01-24 15:19:23 PST
(In reply to Dão Gottwald [:dao] from comment #6)
> Another problem with modules is that we often don't want the modularity. The
> page style functions are only needed in the browser window, nowhere else.
> They don't need to be pluggable. What they need is access to the chrome and
> content window; passing these around isn't going to simplify the code.

Yes, this is a good point. This particular code may not be suitable for modularization. I'm confident that there exists code in browser.js that is, though :)
Comment 8 Justin Dolske [:Dolske] 2012-02-29 21:28:14 PST
Created attachment 601868 [details] [diff] [review]
Patch v.2

Fixed style nits.

I'll likely do the makeCompatWrapper() suggestion in a followup, along with fixing existing callers. Just want to get a few of these refactorings in to see what makes sense to do and what doesn't.
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-04 18:21:24 PST
Comment on attachment 601868 [details] [diff] [review]
Patch v.2

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>+var gPageStyleMenu = {

>+  getAllStyleSheets: function (frameset) {

>+      let frameSheets = getAllStyleSheets(frameset.frames[i]);

this.getAllStyleSheets

>+  stylesheetFillPopup: function (menuPopup) {

>+    var styleSheets = getAllStyleSheets(window.content);

this.getAllStyleSheets
Comment 10 Justin Dolske [:Dolske] 2012-03-04 18:30:18 PST
Created attachment 602786 [details] [diff] [review]
Patch v.3
Comment 12 Matt Brubeck (:mbrubeck) 2012-03-05 13:31:12 PST
https://hg.mozilla.org/mozilla-central/rev/4be83b3f229b

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