Closed Bug 720294 Opened 12 years ago Closed 12 years ago

Refactor style sheet switching code in browser.js

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 13

People

(Reporter: Dolske, Assigned: Dolske)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch Patch v.1 (obsolete) — Splinter Review
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...
Attachment #590623 - Flags: feedback?(gavin.sharp)
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 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");
Attachment #590623 - Flags: feedback?(gavin.sharp) → feedback+
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
(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.
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.
(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 :)
Attached patch Patch v.2 (obsolete) — Splinter Review
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.
Attachment #590623 - Attachment is obsolete: true
Attachment #601868 - Flags: review?(gavin.sharp)
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
Attachment #601868 - Flags: review?(gavin.sharp) → review+
Attached patch Patch v.3Splinter Review
Attachment #601868 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4be83b3f229b
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 733339
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: