767236 - Name all anonymous functions in Add-ons Manager code

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

The Add-ons Manager has a lot of anonymous functions - functions without names, assigned to a variable/property on an object. Unfortunately, that can make debugging code awkward, as they're all just called "anonymous" in errors and stack dumps. So we should name them!

The code is in:
* toolkit/mozapps/extensions/
* toolkit/mozapps/extensions/content/
(I'm less concerned about the tests in toolkit/mozapps/extensions/test/, so they can be skipped.)


Typical example is:
  var MyObject = {
    doSomething: function() {
      ...
    }
  };

Which should end up as:
  var MyObject = {
    doSomething: function MyObject_doSomething() {
      ...
    }
  };


Note that some objects have a mix of named and anonymous functions. In that case, when naming a function, use the existing naming style for that object. For instance, AddonManagerInternal has functions named AMI_doSomething (using an abbreviation of the object's name).

Since this is quite a number of changes over several files, it would be easier to have the changes split into multiple patches.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Oh, another common pattern is functions passed as parameters. eg:
  doSomething(1, 2, function() { ... });

Naming is a bit more ambiguous there, but it'd end up as something like:
  doSomething(1, 2, function doSomethingCallback() { ... });
Or:
  doSomething(1, 2, function handleError() { ... });
etc

Comment 2

[Atul Jangra [:atuljangra]]

Attachment: XPIProvider.jsm

Does the corresponding changes in XPIProvider.jsm

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 636223 [details] [diff] [review]
XPIProvider.jsm

Review of attachment 636223 [details] [diff] [review]:
-----------------------------------------------------------------

Wow, that's a lot more than I expected! 130, I think. Had I know there were that many, I would have suggested starting with a file that wasn't so intimidating - so good work :)

When you attach a patch, you can set the review flag to "?" and enter the email of the person you want to review it - patches can sometimes get lost otherwise.

Since there are so many changes here, the following are general notes that apply throughout the patch.


Generally, functions in JS have a lowercase first letter. There are some exceptions:
* Constructors
* Function names that include the object they belong to. ie: MyObject_doSomething()

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +152,4 @@
>   */
>  var gIDTest = /^(\{[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\}|[a-z0-9-\._]*\@[a-z0-9-\._]+)$/i;
>  
> +["LOG", "WARN", "ERROR"].forEach(function LogWarnErrorCall(aName) {

For functions passed to forEach (which iterates over an array), the name should reflect that it's iterating over something, and what it iterates over. So for instance, I'd name this loggerTypeForEach

*However*, uses of forEach() will end up being replaced with normal loops in bug 758950. So you can can safely ignore functions passed to forEach() - that will save either you or Marcos having to deal with bitrot :) (Sorry, I should have remembered this sooner)

@@ +198,4 @@
>    _installedFiles: null,
>    _createdDirs: null,
>  
> +  _installFile: function SIOInstallFile(aFile, aTargetDirectory, aCopy) {

For functions that are properties of objects like this, separate the object name and the rest of the name with a '_'. Doing so makes it much easier to read, as it's easier to tell whats the object and whats the property name.

So for instance, I'd name this SIO_installFile

@@ +274,4 @@
>      this._installedFiles.push({ oldFile: aDirectory, newFile: newDir });
>    },
>  
> +  _installDirEntry: function SIODirectoryCopy(aDirEntry, aTargetDirectory, aCopy) {

If a function is a property of an object, reuse the name of the property - it gets very confusing otherwise :) So: SIO_installDirEntry

@@ +539,4 @@
>    return aAddon.appDisabled || aAddon.softDisabled || aAddon.userDisabled;
>  }
>  
> +this.__defineGetter__("gRDF", function defineGetterCallback() {

defineGetterCallback is a very generic name - __defineGetter__ is used everywhere. So how about: gRDFGetter

@@ +1314,4 @@
>      cacheEntries.push(entry);
>    entries.close();
>  
> +  cacheEntries.sort(function sortCacheEntriescallback(a, b) {

cacheEntriesSort

@@ +1372,4 @@
>     * @return the default value of the preference or aDefaultValue if there is
>     *         none
>     */
> +  getDefaultCharPref: function getDefaultCharPrefCallback(aName, aDefaultValue) {

These aren't callbacks, they're properties on an object. 
So: Prefs_getDefaultCharPref
("Prefs" is short enough that it doesn't need to be abbreviated)

@@ +6815,4 @@
>   * @param  aFile
>   *         The file to install
>   */
> +AddonInstall.createInstall = function createAddonInstallCallback(aCallback, aFile) {

AddonInstall_createInstall

@@ +6899,5 @@
>   */
>  function AddonInstallWrapper(aInstall) {
>    ["name", "type", "version", "iconURL", "releaseNotesURI", "file", "state", "error",
> +   "progress", "maxProgress", "certificate", "certName"].forEach(function addonInstallWrapperCallback(aProp) {
> +    this.__defineGetter__(aProp, function addonInstallWrapperInternal() aInstall[aProp]);

The awkward thing about appending "Internal" to a function name is that there can be several "internal" functions. For this I'd use something like AIW_propsGetter

@@ +6904,3 @@
>    }, this);
>  
> +  this.__defineGetter__("existingAddon", function addonExistCallback() {

AIW_existingAddonGetter

Comment 4

[Atul Jangra [:atuljangra]]

Attachment: XPIProvider.jsm

Have cleaned up the code accordingly.
Sorry for the delay, was busy with GSoC work :-)
Cheers

Comment 5

[Atul Jangra [:atuljangra]]

Attachment: Changes in content/extensions.js

Have cleaned up the code in /contents/extensions.js .

Comment 6

[Atul Jangra [:atuljangra]]

Attachment: Changes in AddonRepository.jsm

Made required changes in AddonRepository.jsm

Comment 7

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 637871 [details] [diff] [review]
XPIProvider.jsm

Review of attachment 637871 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +140,4 @@
>  var gIDTest = /^(\{[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}\}|[a-z0-9-\._]*\@[a-z0-9-\._]+)$/i;
>  
>  ["LOG", "WARN", "ERROR"].forEach(function(aName) {
> +  this.__defineGetter__(aName, function aNameGetter() {

aNameGetter doesn't really help with figuring out what the function is for (which is a getter for the logging functions). So how about: logFuncGetter

@@ +168,4 @@
>  }
>  
>  for (let name of LAZY_OBJECTS) {
> +  gGlobalScope.__defineGetter__(name, function gGLobalScopeGetter() {

Ditto: lazyObjectGetter

@@ +329,4 @@
>     *         The directory to move into, this is expected to be an empty
>     *         directory.
>     */
> +  move: function SIO_moveFile(aFile, aTargetDirectory) {

SIO_moveFile is confusing, as it's used as SafeInstallOperation.move, not SafeInstallOperation.moveFile. Rename to SIO_move

@@ +349,4 @@
>     *         The directory to copy into, this is expected to be an empty
>     *         directory.
>     */
> +  copy: function SIO_copyFile(aFile, aTargetDirectory) {

Ditto: SIO_copy

@@ +364,4 @@
>     * occurs here then both old and new directories are left in an indeterminate
>     * state
>     */
> +  rollback: function SIO_Rollback() {

No capital R

@@ +1628,4 @@
>      // Let these shutdown a little earlier when they still have access to most
>      // of XPCOM
>      Services.obs.addObserver({
> +      observe: function XPIP_addObserver(aSubject, aTopic, aData) {

This is a property on an object passed into addObserver, though it's the only property. So lets name this shutdownObserver

@@ +3124,4 @@
>     */
>    getInstallForURL: function XPI_getInstallForURL(aUrl, aHash, aName, aIconURL,
>                                                    aVersion, aLoadGroup, aCallback) {
> +    AddonInstall.createDownload(function AddonCreateDownload(aInstall) {

The naming for all these callback functions isn't working as well as I hoped - they end up being quite ambiguous, or don't really convey what they do/how they're used.

We have a bunch of cases like this where we pass a function as a callback to another function, but it's really just a way of asynchronously continuing the remainder of the original (outer) function. In cases like that it's useful to make the function name link to both the original function, and to this new function, which is essentially a labeled block of code.

So I've been thinking it'd be nice to have these cases named like originalFunction_subFunction (ignoring the prefix of the object name, since it gets too long otherwise).

ie, for this, it would be: getInstallForURL_createDownload

What do you think?

@@ +3153,4 @@
>     *         The AddonInstall to remove
>     */
>    removeActiveInstall: function XPI_removeActiveInstall(aInstall) {
> +    this.installs = this.installs.filter(function aInstallCheck(i) i != aInstall);

installFilter

@@ +5183,4 @@
>  function AddonInstallWrapper(aInstall) {
>    ["name", "type", "version", "iconURL", "releaseNotesURI", "file", "state", "error",
>     "progress", "maxProgress", "certificate", "certName"].forEach(function(aProp) {
> +    this.__defineGetter__(aProp, function aPropGetter() aInstall[aProp]);

AIW_propertyGetter, AIW_existingAddonGetter, etc for these getters?

@@ +5481,4 @@
>      return matchedOS && !needsABI;
>    },
>  
> +  isCompatibleWith: function AddonInternalIsCompatibleWith(aAppVersion, aPlatformVersion) {

AddonInternal_isCompatibleWith (same with the other functions for this object).

@@ +5636,4 @@
>   * have all data available.
>   */
>  function DBAddonInternal() {
> +  this.__defineGetter__("targetApplications", function targetApplicationsGetter() {

DBA_targetApplicationsGetter, etc for getters on this object?

@@ +6068,4 @@
>      return val;
>    });
>  
> +  this.isCompatibleWith = function AppVersionCompatibility(aAppVersion, aPlatformVersion) {

AddonWrapper_isCompatiblewith (there's a few to fix up on this object).

@@ +6072,4 @@
>      return aAddon.isCompatibleWith(aAppVersion, aPlatformVersion);
>    };
>  
> +  this.uninstall = function () {

Missed adding a name for this one?

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 638348 [details] [diff] [review]
Changes in content/extensions.js

Review of attachment 638348 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/content/extensions.js
@@ +165,4 @@
>                   .canGoForward;
>    },
>  
> +  back: function HTML5HistoryBack() {

HTML5History_back - underscore to separate the object name fro the function name, and lowercase letter after that. Similar thing for other functions on this object, and other objects in this file.

@@ +634,4 @@
>  
>    commands: {
>      cmd_back: {
> +      isEnabled: function BackEnabled() {

For commands, we have a lot of them and they all have the same naming system for the object names and function names. It's also useful to explicitly know it's a command object, since they're kinda special. So how about cmd_NAME_isEnabled, cmd_NAME_doCommand? ie cmd_back_isEnabled, cmd_back_doCommand

Comment 9

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 638644 [details] [diff] [review]
Changes in AddonRepository.jsm

Review of attachment 638644 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +374,4 @@
>     *         A platform version to test against
>     * @return Boolean representing if the add-on is compatible
>     */
> +  isCompatibleWith: function ASRIsCompatibleWith(aAppVerison, aPlatformVersion) {

Same as what I've said before about using Object_functionName naming system for functions that are properties of an object - ASR_isCompatibleWith, etc

Comment 10

[Atul Jangra [:atuljangra]]

I have noted the key points. I will update the patches accordingly.
Thanks for the reviews :)

Comment 11

[Atul Jangra [:atuljangra]]

Attachment: Changes in AddonRepository.jsm

I have fixed the nits in AddonRepository.jsm

Comment 12

[Atul Jangra [:atuljangra]]

(In reply to Atul Jangra from comment #11)
> Created attachment 640734 [details] [diff] [review]
> Changes in AddonRepository.jsm
> 
> I have fixed the nits in AddonRepository.jsm

shutdown_databaseShutdown in place of shutdown_shutdown. 
Will update it in next patch :)

Comment 13

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 640734 [details] [diff] [review]
Changes in AddonRepository.jsm

Review of attachment 640734 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good! Just a small amount of things to fix up in this one.

::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +560,4 @@
>     * @param  aCallback
>     *         The optional callback to call once complete
>     */
> +  repopulateCache: function AddonRepo_repopulateCacheCall(aIds, aCallback) {

Ugh, we don't have good naming for these two functions (repopulateCache and _repopulateCache).

How about AddonRepo_repopulateCache for this one.

@@ +564,4 @@
>      this._repopulateCache(aIds, aCallback, false);
>    },
>  
> +  _repopulateCache: function AddonRepo_repopulateCache(aIds, aCallback, aSendPerformance) {

And AddonRepo_repopulateCacheInternal for this one, since the property should ideally be named _repopulateCacheInternal anyway.

If you want, you can update the property name to _repopulateCacheInternal, and the 2 places where we call it in this file. Otherwise we can fix that up at a later date.

@@ +584,4 @@
>        }
>  
>        self._beginGetAddons(aAddons, {
> +        searchSucceeded: function getAddonsSearchSuccess(aAddons) {

repopulateCacheInternal_searchSucceeded

Similar for other cases of searchSucceeded/searchFailed below.

@@ +1108,4 @@
>            break;
>          case "all_compatible_os":
>            let nodes = node.getElementsByTagName("os");
> +          addon.isPlatformCompatible = Array.some(nodes, function PlatformCompatibility(aNode) {

parseAddon_platformCompatFilter ? .some() takes a function that is basically acts as a filter on the array items.

Same with the other funtcions passed into .some() below.

@@ +1357,4 @@
>      this._request.overrideMimeType("text/xml");
>  
>      let self = this;
> +    this._request.addEventListener("error", function errorListener(aEvent) {

beginSearch_errorListener ?
Same for loadListener below.

Comment 14

[Atul Jangra [:atuljangra]]

Attachment: Changes in AddonRepository.jsm

Fixed the nits as pointed out by Unfocused.
Have also renamed _repopulateCache to _repopulateCacheInternal and changed the 2 calls in this file.

Comment 15

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 640998 [details] [diff] [review]
Changes in AddonRepository.jsm

Review of attachment 640998 [details] [diff] [review]:
-----------------------------------------------------------------

One down, good job :) 

And I ran the test suites, and everything was fine.

::: toolkit/mozapps/extensions/AddonRepository.jsm
@@ +491,5 @@
>      this.cancelSearch();
>  
>      this._addons = null;
>      this._pendingCallbacks = null;
> +    AddonDatabase.shutdown(function shutdown_shutdown() {

Did you intend to name this shutdown_databaseShutdown ?

Comment 16

[Atul Jangra [:atuljangra]]

(In reply to Blair McBride (:Unfocused) from comment #15)
> Comment on attachment 640998 [details] [diff] [review]
> Changes in AddonRepository.jsm
> 
> Review of attachment 640998 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> One down, good job :) 
> 
> And I ran the test suites, and everything was fine.
> 
> ::: toolkit/mozapps/extensions/AddonRepository.jsm
> @@ +491,5 @@
> >      this.cancelSearch();
> >  
> >      this._addons = null;
> >      this._pendingCallbacks = null;
> > +    AddonDatabase.shutdown(function shutdown_shutdown() {
> 
> Did you intend to name this shutdown_databaseShutdown ?
 Yes, I guess I missed it. will fix it in next patch :)

Comment 17

[Atul Jangra [:atuljangra]]

Attachment: Changes in content/extensions.js

Fixed the nits as pointed out by Unfocused in content/extensions.js

Comment 18

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 642253 [details] [diff] [review]
Changes in content/extensions.js

Review of attachment 642253 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/mozapps/extensions/content/extensions.js
@@ +219,4 @@
>      return (this.pos + 1) < this.states.length;
>    },
>  
> +  back: function FH_back() {

Hmm, you abbreviate FakeHistory here, but don't abbreviate HTML5History above. Those objects are just different implementations of the same API, so maybe FakeHistory sholodn't be abbreviated, to match HTML5History? Just pondering, will leave that up to you.

@@ +1439,4 @@
>        this.node.value = VIEW_DEFAULT;
>  
>      var self = this;
> +    this.node.addEventListener("select", function  selectListener() {

Nit: There's two spaces after "function" here.

@@ +1546,4 @@
>          }
>  
>          gEventManager.registerInstallListener({
> +          onDownloadStarted: function onDownloadStartedListener(aInstall) {

gCategories_onDownloadStarted, etc, for the functions in this object?
(Eventually, I'd like to refactor this object so that's more true - see bug 774179.)

@@ +1648,3 @@
>      this._search = document.getElementById("header-search");
>  
> +    this._search.addEventListener("command", function commandEventListener(aEvent) {

For events, its usual to name the functions like onEvent. So this can be something like search_onCommand. Similar for other cases of passing functions to addEventListener below.

@@ +2117,4 @@
>      }
>  
>      AddonRepository.searchAddons(aQuery, maxRemoteResults, {
> +      searchFailed: function addonRepositorySearchFailed() {

show_searchFailed? (And the searchSuccessed below)

@@ +3126,5 @@
>      if (aInitializing)
>        gPendingInitializations++;
>      var self = this;
> +    AddonManager.getAllInstalls(function updateAvailableCount_getAllInstalls(aInstallsList) {
> +      var count = aInstallsList.filter(function installListCounter(aInstall) {

This function is a filter, not a counter (the count is gotten from getting the length of the filtered items).

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Atul Jangra from comment #16)
> > Did you intend to name this shutdown_databaseShutdown ?
>  Yes, I guess I missed it. will fix it in next patch :)

If you want to attach that updated patch, I can land it now - so it doesn't get bitrotten.

Comment 20

[Atul Jangra [:atuljangra]]

Attachment: Part 1: Changes in AddonRepository.jsm

Fixed the nit. :)

Comment 21

[Atul Jangra [:atuljangra]]

Attachment: Part 2: Changes in content/extensions.js

Fixed the nits in /content/extensions as pointed out by Unfocused.

Comment 22

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 642739 [details] [diff] [review]
Part 2: Changes in content/extensions.js

Review of attachment 642739 [details] [diff] [review]:
-----------------------------------------------------------------

Good to go :)

Comment 23

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 642716 [details] [diff] [review]
Part 1: Changes in AddonRepository.jsm

https://hg.mozilla.org/integration/fx-team/rev/e414532e1c07

Comment 24

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 642739 [details] [diff] [review]
Part 2: Changes in content/extensions.js

https://hg.mozilla.org/integration/fx-team/rev/a345d72f9165

Comment 25

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Atul: Ok, I've landed those two completed patches on the fx-team branch, which will get merged into mozilla-central sometime within the next day or so.




Note to whoever merges these: Please leave this bug open, we're not done here yet.

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/e414532e1c07
https://hg.mozilla.org/mozilla-central/rev/a345d72f9165

Comment 27

[Atul Jangra [:atuljangra]]

Thanks Blair for landing the patches. I am working on other patches now. :)

Comment 28

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Any process, Atul? Anything I can help with?

Comment 29

[Atul Jangra [:atuljangra]]

Yes! I was working on AddonRepository.jsm. Will update the patch tomorrow.

Comment 30

[Atul Jangra [:atuljangra]]

Attachment: Changes in XPIProvider.jsm

Does corresponding changes in XPIProvider.jsm.

Comment 31

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 651178 [details] [diff] [review]
Changes in XPIProvider.jsm

Review of attachment 651178 [details] [diff] [review]:
-----------------------------------------------------------------

Making some good progress :)
I'm still amazed we have *so many* anonymous functions.

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +3184,4 @@
>     *         A callback to pass an array of Addons to
>     */
>    getAddonsByTypes: function XPI_getAddonsByTypes(aTypes, aCallback) {
> +    XPIDatabase.getVisibleAddons(aTypes, function getAddonsByType_getVisibleAddons(aAddons) {

Missing an 's' at the end of 'getAddonsByType'

@@ +3198,4 @@
>     *         A callback to pass the Addon to. Receives null if not found.
>     */
>    getAddonBySyncGUID: function XPI_getAddonBySyncGUID(aGUID, aCallback) {
> +    XPIDatabase.getAddonBySyncGUID(aGUID, function getAddonBySyncGUIDCall(aAddon) {

Not following the usual naming convention here.

@@ +3216,4 @@
>     */
>    getAddonsWithOperationsByTypes:
>    function XPI_getAddonsWithOperationsByTypes(aTypes, aCallback) {
> +    XPIDatabase.getVisibleAddonsWithPendingOperations(aTypes, function getAddonsWithOperationsByTypes_getVisibleAddonsWithPendingOperations(aAddons) {

This line is *really* long. Wrap it after the first argument, and shorten the function name to something like getAddonsWithOpsByTypes__getVisibleAddonsWithPendingOps

@@ +3908,4 @@
>          return;
>  
>        let location = XPIProvider.installLocations[aPos];
> +      XPIDatabase.getAddonInLocation(aAddon.id, location.name, function checkInstallLocation_getAddonInLocation(aNewAddon) {

Wrap this line too.

@@ +4139,4 @@
>  
>      try {
>        let self = this;
> +      this.loadManifest(function  AI_loadManifest() {

Not following the convention here (this function isn't a property on AddonInstall).

@@ +4139,5 @@
>  
>      try {
>        let self = this;
> +      this.loadManifest(function  AI_loadManifest() {
> +        XPIDatabase.getVisibleAddonForID(self.addon.id, function getVisibleAddonCall(aAddon) {

Ditto.

@@ +4302,4 @@
>     *         The InstallListener to add
>     */
>    addListener: function AI_addListener(aListener) {
> +    if (!this.listeners.some(function AI_addListenerCall(i) { return i == aListener; }))

This function is matching elements in the array to aListener. So how about addListener_matchListener ?

@@ +4313,4 @@
>     *         The InstallListener to remove
>     */
>    removeListener: function AI_removeListener(aListener) {
> +    this.listeners = this.listeners.filter(function AI_removeListenerCall(i) {

Not following the convention.

@@ +4429,4 @@
>        let count = 0;
>        let self = this;
>        files.forEach(function(file) {
> +        AddonInstall.createInstall(function AddonCreateInstall(aInstall) {

Ditto.

@@ +4530,4 @@
>  
>      if (this.addon.type == "multipackage") {
>        let self = this;
> +      this.loadMultipackageManifests(zipreader, function loadMultiManifestZipreader() {

Ditto.

@@ +4767,4 @@
>          }
>          try {
>            let self = this;
> +          this.loadManifest(function loadManifestCall() {

Ditto.

@@ +4775,4 @@
>                // TODO Should we send some event here (bug 557716)?
>                self.state = AddonManager.STATE_CHECKING;
>                new UpdateChecker(self.addon, {
> +                onUpdateFinished: function AddonOnUpdateFinished(aAddon) {

Ditto.

@@ +5018,4 @@
>          // Retrieve the new DBAddonInternal for the add-on we just added
>          let self = this;
>          XPIDatabase.getAddonInLocation(this.addon.id, this.installLocation.name,
> +                                       function getAddonInLocationCall(a) {

Ditto.

@@ +5748,5 @@
>  
>    ["fullDescription", "developerComments", "eula", "supportURL",
>     "contributionURL", "contributionAmount", "averageRating", "reviewCount",
>     "reviewURL", "totalDownloads", "weeklyDownloads", "dailyUsers",
>     "repositoryStatus"].forEach(function(aProp) {

Missed one.

@@ +5749,5 @@
>    ["fullDescription", "developerComments", "eula", "supportURL",
>     "contributionURL", "contributionAmount", "averageRating", "reviewCount",
>     "reviewURL", "totalDownloads", "weeklyDownloads", "dailyUsers",
>     "repositoryStatus"].forEach(function(aProp) {
> +    this.__defineGetter__(aProp, function AddonWrapper_propertyGetter()() {

Double () here.

Also, to avoid the naming conflict with AddonWrapper_propertyGetter just a few lines above, name this something like AddonWrapper_repoPropertyGetter ?

@@ +5758,4 @@
>      });
>    }, this);
>  
> +  this.__defineGetter__("aboutURL", function aboutURLGetter() {

This and most of the following functions within AddonWrapper aren't following the convention.

@@ +6562,4 @@
>     * @param aId
>     *        The ID of the addon
>     */
> +  isLinkedAddon: function DIR_isLinkedAddon(aId) {

The other function names for this object (DirInstallLocation) don't use an abbreviation.

Comment 32

[Atul Jangra [:atuljangra]]

Attachment: Part 3: Changes in XPIProvider.jsm

fixed the nits :)

Comment 33

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 652112 [details] [diff] [review]
Part 3: Changes in XPIProvider.jsm

Review of attachment 652112 [details] [diff] [review]:
-----------------------------------------------------------------

Just one nit, so this is good to go :) I'll land it with that indentation change.

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +3216,5 @@
>     */
>    getAddonsWithOperationsByTypes:
>    function XPI_getAddonsWithOperationsByTypes(aTypes, aCallback) {
> +    XPIDatabase.getVisibleAddonsWithPendingOperations(aTypes,
> +    function getAddonsWithOpsByTypes_getVisibleAddonsWithPendingOps(aAddons) {

Nit: When wrapping lines, indent the second (and third etc) lines, so its more obvious that they're part of the previous line.

Comment 34

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 652112 [details] [diff] [review]
Part 3: Changes in XPIProvider.jsm

https://hg.mozilla.org/integration/fx-team/rev/51471fc062ec

Comment 35

[Atul Jangra [:atuljangra]]

Attachment: Part 3: Changes in XPIProvider.jsm

Fixed the nit. 
I guess this is good to go :-)

Comment 36

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 652410 [details] [diff] [review]
Part 3: Changes in XPIProvider.jsm

Beat you to it (already landed with it fixed), but thanks nevertheless :)

That's probably the last really big file, just a bunch of smaller files now (remember not to bother with the tests).

Comment 37

[Atul Jangra [:atuljangra]]

heh Thanks :) 
Yes, I'll be doing smaller ones now.

Comment 38

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/51471fc062ec

Comment 39

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Have you been able to make any more progress here, Atul?

Comment 40

[Atul Jangra [:atuljangra]]

Attachment: Changes in various other files

Made changes in remaining of the files.

Comment 41

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 670817 [details] [diff] [review]
Changes in various other files

Review of attachment 670817 [details] [diff] [review]:
-----------------------------------------------------------------

Woo! Home stretch!

General nit: Few cases here (mostly AddonManager.jsm) where adding a function name makes a line really long - should try to wrap them to 80 chars.

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +402,5 @@
>          enumerable: true
>        }
>      },
>  
> +    getPropertyDescriptor: function getOwnPropertyDescriptor_getPropertyDescriptor(aName) {

typesProxy_whatever

@@ +424,4 @@
>        // Ignore attempts to define properties
>      },
>  
> +    fix: function getOwnPropertyDescriptor_fix() getOwnPropertyDescriptor_fix{

Typo?

@@ +843,5 @@
>  
>      // Replace custom parameters (names of custom parameters must have at
>      // least 3 characters to prevent lookups for something like %D0%C8)
>      var catMan = null;
> +    uri = uri.replace(/%(\w{3,})%/g, function parameterMatching(aMatch, aParam) {

When passing a function as the 2nd parameter to string.replace(), the function takes the found part of the substring, and returns what it should be replaced with. So it's more like parameterReplace

@@ +966,5 @@
>  
>        pendingUpdates++;
>        Components.utils.import("resource://gre/modules/AddonUpdateChecker.jsm");
>        AddonUpdateChecker.checkForUpdates(hotfixID, "extension", null, url, {
> +        onUpdateCheckComplete: function AddonUpdateChecker_onUpdateCheckComplete(aUpdates) {

You're mixing naming in this function (backgroundUpdateCheck) - existing functions you've named here use the BUC_ prefix.

@@ +1334,5 @@
>        if (callProvider(provider, "supportsMimetype", false, aMimetype)) {
>          callProvider(provider, "getInstallForURL", null,
>                       aUrl, aHash, aName, aIcons, aVersion, aLoadGroup,
> +                     function (aInstall) {
> +          safeCall getInstallForURL_safeCall(aCallback, aInstall);

Typo?

::: toolkit/mozapps/extensions/LightweightThemeManager.jsm
@@ +391,5 @@
>   * The AddonWrapper wraps lightweight theme to provide the data visible to
>   * consumers of the AddonManager API.
>   */
>  function AddonWrapper(aTheme) {
> +  this.__defineGetter__("id", function idGetter() aTheme.id + ID_SUFFIX);

Missing the usual object prefix on some of these.

::: toolkit/mozapps/extensions/XPIProviderUtils.js
@@ +17,5 @@
>                                    "resource://gre/modules/FileUtils.jsm");
>  
>  
>  ["LOG", "WARN", "ERROR"].forEach(function(aName) {
> +  this.__defineGetter__(aName, functionlogFuncGetter () {

Typo.

::: toolkit/mozapps/extensions/amWebInstallListener.js
@@ +217,5 @@
>        notifyObservers("addon-install-complete", this.window, this.url, this.installed);
>      this.installed = null;
>    },
>  
> +  onDownloadCancelled: function checkAllInstalled_onDownloadCancelled(aInstall) {

Installer_whatever

::: toolkit/mozapps/extensions/content/selectAddons.js
@@ +14,5 @@
>  const Ci = Components.interfaces;
>  
>  var gView = null;
>  
> + showView(aView) {

Unintentionally cut "function" ?

Comment 42

[Atul Jangra [:atuljangra]]

Attachment: Changes in various other files

Fixed the nits.

Comment 43

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 674482 [details] [diff] [review]
Changes in various other files

Review of attachment 674482 [details] [diff] [review]:
-----------------------------------------------------------------

Looking good :) Just a few fixups left.

Have you checked the tests run fine with this patch?

::: toolkit/mozapps/extensions/AddonManager.jsm
@@ +1593,5 @@
>      if (!aListener || typeof aListener != "object")
>        throw Components.Exception("aListener must be a InstallListener object",
>                                   Cr.NS_ERROR_INVALID_ARG);
>      
> +    if (!this.installListeners.some(function addInstallListener_pushListener(i) {

This function doesn't have anything to do with pushing a listener onto the array, it checks whether it's already in the array. So it should be named something like addInstallListener_matchListener

@@ +1847,5 @@
>      if (!aListener || typeof aListener != "object")
>        throw Components.Exception("aListener must be an AddonManagerListener object",
>                                   Cr.NS_ERROR_INVALID_ARG);
>  
> +    if (!this.managerListeners.some(function addManagerListener_pushListener(i) {

Ditto.

@@ +1883,5 @@
>      if (!aListener || typeof aListener != "object")
>        throw Components.Exception("aListener must be an AddonListener object",
>                                   Cr.NS_ERROR_INVALID_ARG);
>  
> +    if (!this.addonListeners.some(function addAddonListener_pushListener(i) {

Ditto.

@@ +1919,5 @@
>      if (!aListener || typeof aListener != "object")
>        throw Components.Exception("aListener must be a TypeListener object",
>                                   Cr.NS_ERROR_INVALID_ARG);
>  
> +    if (!this.typeListeners.some(function addTypeListener_pushListener(i) {

Ditto.

::: toolkit/mozapps/extensions/ChromeManifestParser.jsm
@@ +151,5 @@
>    *         Instruction type to filter by.
>    * @return True if any matching instructions were found in the manifest.
>    */
>    hasType: function CMP_hasType(aManifest, aType) {
> +    return aManifest.some(function hasType_returnEntryType(aEntry) {

Ditto.

::: toolkit/mozapps/extensions/LightweightThemeManager.jsm
@@ +406,5 @@
>      return "version" in aTheme ? aTheme.version : "";
>    });
>  
>    ["description", "homepageURL", "iconURL"].forEach(function(prop) {
> +    this.__defineGetter__(prop, function AddonWrapper_descriptionPropGetter() {

Hmm, this is for more than just the description property - maybe something like AddonWrapper_optionalPropGetter is more appropriate?

@@ +668,5 @@
>    return result;
>  }
>  
>  function _usedThemesExceptId(aId)
> +  LightweightThemeManager.usedThemes.filter(function usedThemesExceptId_filterID(t) "id" in t && t.id != aId);

Can this be wrapped to 80 chars?

::: toolkit/mozapps/extensions/content/extensions-content.js
@@ +224,5 @@
>    }, false);
>  }
>  
>  InstallTriggerManager.prototype = {
>    handleEvent: function handleEvent(aEvent) {

While you're at it, could you fix up this function name so it has a ITM_ prefix? (Same with InstallTriggerManager's other functions).

::: toolkit/mozapps/extensions/content/selectAddons.js
@@ +14,5 @@
>  const Ci = Components.interfaces;
>  
>  var gView = null;
>  
> + function showView(aView) {

Nit: Accidentally inserted a space at the start of this line.

@@ +66,5 @@
>      showButtons(true, false, false, false);
>      this._progress = document.getElementById("checking-progress");
>  
>      let self = this;
> +    AddonManager.getAllAddons(function gChecking_addAllAddons(aAddons) {

Typo.

@@ +328,5 @@
>      window.close();
>    }
>  };
>  
> +window.addEventListener("load", function loadEventListener() { showView(gChecking); }, false);

Wrap to 80 chars?

::: toolkit/mozapps/extensions/nsBlocklistService.js
@@ +845,4 @@
>      var addonList = [];
>  
>      var self = this;
> +    AddonManager.getAddonsByTypes(["extension", "theme", "locale", "dictionary"], function blocklistUpdated_getAddonsByTypes(addons) {

Wrap to 80 chars?

Might be worth just moving that array into a separate const, like:
  const types = [...];
  AddonManager.getAddonsByTypes(types, function ...

Comment 44

[Atul Jangra [:atuljangra]]

Attachment: Changes in various other files

Have fixed the nits. All tests in toolkit/mozapps/extensions/test are passing :-)

Comment 45

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 674998 [details] [diff] [review]
Changes in various other files

Review of attachment 674998 [details] [diff] [review]:
-----------------------------------------------------------------

Good to go :)

Comment 46

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 674998 [details] [diff] [review]
Changes in various other files

https://hg.mozilla.org/integration/fx-team/rev/2a7e4bc714fe

Comment 47

[Blair McBride [:Unfocused] (UNAVAILABLE)]

That was a marathon effort, Atul - thanks for keeping on it :)

Comment 48

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/2a7e4bc714fe

Comment 49

[Atul Jangra [:atuljangra]]

Yay :-)