820835 - Wrap all of the internal Social API objects and constants in a function scope within browser-social.js

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

From #socialdev,

<gavin> hrm, I kind of managed to forget the effects of browser-social being #included in browser.js
<gavin> we're somewhat "polluting" the global window scope with things like "SharedFrame" and PANEL_MIN_HEIGHT and SocialErrorListener
<gavin> would probably be nice to wrap all that up in a function scope

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

This patch introduces a new global object, BrowserSocial, which encapsulates the currently used-external objects from browser-social.js. SocialMenu and SocialToolbar aren't used outside of browser-social.js, so they aren't exported.

As we talked about on IRC, this approach isn't exactly the one that you had in mind, but since I already wrote it I figured I would put it up here and see what you thought about it.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v.2

This version changes the name from BrowserSocial to SocialUI as requested on IRC. I'll upload a diff without the whitespace changes next.

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v.2 without whitespace changes

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 691480 [details] [diff] [review]
Patch v.2

>+let SocialUI = (function () {
>+  let exports = {};
> 
>-XPCOMUtils.defineLazyModuleGetter(this, "SharedFrame",
>-  "resource:///modules/SharedFrame.jsm");
>+  // The minimum sizes for the auto-resize panel code.
>+  const PANEL_MIN_HEIGHT = 100;
>+  const PANEL_MIN_WIDTH = 330;

How about:

let SocialUI = {
  PANEL_MIN_HEIGHT: 100,
  PANEL_MIN_WIDTH: 330,
etc.

Wrapping everything in a function and returning an object that you're incrementally building seems like playing with language features that we don't really need here.

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to Dão Gottwald [:dao] from comment #4)
> How about:
> 
> let SocialUI = {
>   PANEL_MIN_HEIGHT: 100,
>   PANEL_MIN_WIDTH: 330,
> etc.

That makes it more annoying to use these (need this.PANEL_MIN_HEIGHT, and to make sure you have the right "this", etc.). I think the goal should be to avoid changing the code as much as possible, but also avoid leaking all this random stuff into the window scope.

I don't much like leading caps for non-global property names, so I'd prefer SocialUI.chatBar over SocialUI.ChatBar.

It's a bit confusing to have the "primary" objects just all mixed in with various little helper functions, but I'm not sure I see a great way to avoid that. I guess maybe it would help to define "exports" in one place, rather than piece-by-piece, so it's more obvious what the exported properties are.

Comment 6

[Dão Gottwald [:dao]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #5)
> (In reply to Dão Gottwald [:dao] from comment #4)
> > How about:
> > 
> > let SocialUI = {
> >   PANEL_MIN_HEIGHT: 100,
> >   PANEL_MIN_WIDTH: 330,
> > etc.
> 
> That makes it more annoying to use these (need this.PANEL_MIN_HEIGHT, and to
> make sure you have the right "this", etc.). I think the goal should be to
> avoid changing the code as much as possible, but also avoid leaking all this
> random stuff into the window scope.

Each of these two constants are used once or twice; they shouldn't dictate how we organize 1000+ lines of code. The anonymous function is only really useful if you want to hide code from the outside, i.e. if you have lots of stuff that you don't want to "export", but I'm not sure why that would be preferable over just using the underscore prefix, which is less magic, results in a simpler code structure and makes "private" stuff more obvious when reading the code.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

I ran into some weird issue when wrapping the entire file in a function block and then defining the exported objects on |window|. I probably had the same issue with the other patch, but my build system wasn't working as I expected and so I wasn't seeing the issue before.

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Attachment: Simple approach

This patch is very simple - it just declares the stuff we care about outside the function scope, so assignments to them within the function do the right thing.  Tests all pass.

Comment 9

[Mark Hammond [:markh] [:mhammond]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a33ff156a45

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9a33ff156a45

Comment 11

[u279076]

Please advise of regression testing is needed from QA.