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.

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

Does the corresponding changes in XPIProvider.jsm

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

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

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

Made required changes in AddonRepository.jsm

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 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 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

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

I have fixed the nits in AddonRepository.jsm

(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 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.

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

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 ?

(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 :)

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

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).

(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.

Fixed the nit. :)

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

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 on attachment 642716 [details] [diff] [review] Part 1: Changes in AddonRepository.jsm https://hg.mozilla.org/integration/fx-team/rev/e414532e1c07

Comment on attachment 642739 [details] [diff] [review] Part 2: Changes in content/extensions.js https://hg.mozilla.org/integration/fx-team/rev/a345d72f9165

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.

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

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

Any process, Atul? Anything I can help with?

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

Does corresponding changes in XPIProvider.jsm.

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.

fixed the nits :)

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 on attachment 652112 [details] [diff] [review] Part 3: Changes in XPIProvider.jsm https://hg.mozilla.org/integration/fx-team/rev/51471fc062ec

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

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).

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

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

Made changes in remaining of the files.

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" ?

Fixed the nits.

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 ...

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

Comment on attachment 674998 [details] [diff] [review] Changes in various other files Review of attachment 674998 [details] [diff] [review]: ----------------------------------------------------------------- Good to go :)

Comment on attachment 674998 [details] [diff] [review] Changes in various other files https://hg.mozilla.org/integration/fx-team/rev/2a7e4bc714fe

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

Yay :-)