Opened 5 years ago

Last modified 18 months ago

#13198 new defect

clean up torbutton use of Mozilla services

Reported by: arthuredelstein Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: MikePerry201409R
Cc: mcs, brade, gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Most of the invocations to Cc...getService in the torbutton JS code are unnecessary. Writing a patch to clean it up.

Child Tickets

Attachments (2)

0001-Bug-13198-Code-cleanup-use-global-Services-object-wh.2.patch (62.8 KB) - added by arthuredelstein 5 years ago.
0001-Bug-13198-Code-cleanup-use-global-Services-object-wh.patch (62.8 KB) - added by arthuredelstein 5 years ago.

Download all attachments as: .zip

Change History (7)

comment:1 Changed 5 years ago by arthuredelstein

Keywords: MikePerry201409R added
Status: newneeds_review

comment:2 Changed 5 years ago by mikeperry

Cc: mcs brade gk added

Hrmm. I'm a little worried about the scoping here for the changes in src/chrome/content/torbutton.js. This block at the top in particular:

let { getBoolPref, setBoolPref, getCharPref, setCharPref,
      getIntPref, setIntPref, clearUserPref, prefHasUserValue
      addObserver : addPrefObserver , removeObserver : removePrefObserver } = Services.prefs; 

I know 'let' uses block scoping, but in this case, we're at the top level of the torbutton.js file and not technically in a block. This is the browser XUL tag's 'window' scope, and is shared with all other browser scripts and extension bindings. Does 'let' also make this scope local to torbutton.js, considering torbutton.js to be a block (even though it's not explicitly one)? Because if it ends up in 'window' scope, then we're risking collisions with other addons variable names in 'window'.

It also looks like there is a comma missing after prefHasUserValue.

comment:3 in reply to:  2 Changed 5 years ago by arthuredelstein

Replying to mikeperry:

Hrmm. I'm a little worried about the scoping here for the changes in src/chrome/content/torbutton.js. This block at the top in particular:

let { getBoolPref, setBoolPref, getCharPref, setCharPref,
      getIntPref, setIntPref, clearUserPref, prefHasUserValue
      addObserver : addPrefObserver , removeObserver : removePrefObserver } = Services.prefs; 

I know 'let' uses block scoping, but in this case, we're at the top level of the torbutton.js file and not technically in a block. This is the browser XUL tag's 'window' scope, and is shared with all other browser scripts and extension bindings. Does 'let' also make this scope local to torbutton.js, considering torbutton.js to be a block (even though it's not explicitly one)? Because if it ends up in 'window' scope, then we're risking collisions with other addons variable names in 'window'.

Good point. We could enclose the whole torbutton.js file in a scope like:

/* api methods */ = (function() {
  /* torbutton.js code */
  return api_methods;
})();

which would help avoid any collisions. But for now I'll just leave out that let statement at the top and instead use m_tb_prefs as an alias for Services.prefs (which reduces the number of changes in the patch).

comment:4 Changed 5 years ago by arthuredelstein

Status: needs_reviewnew

comment:5 Changed 18 months ago by teor

Severity: Normal

Set all open tickets without a severity to "Normal"

Note: See TracTickets for help on using tickets.