830688 - Make use of the Services.jsm module in all lib modules and tests

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

We should not request XPCOM interfaces on our own via CC but let this do the Services.jsm module:

var observerService = Cc["@mozilla.org/observer-service;1"].
		      getService(Ci.nsIObserverService);
		
vs.

Components.utils.import("resource://gre/modules/Services.jsm");

The observer service in this case will be available as `Services.obs`. See the attached URL as reference.

Comment 1

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v1

Refactored where Services.jsm had a getter, also left the variable only where it was used at least twice in the test, otherwise inserted it in the line itself.

Reports - functional:
OS X: http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb510df37b
* http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb510e0346

Windows: http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb510df49f
* http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb510e0f7a

Linux: http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb510e116c

For endurance I'm still waiting reports, I'll add them later.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 704898 [details] [diff] [review]
patch v1

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

I love those changes, which make it way easier to read the tests. There are some parts which need a change thought.

::: lib/addons.js
@@ +5,5 @@
>  var frame = {};
>  Components.utils.import('resource://mozmill/modules/frame.js', frame);
>  
>  Components.utils.import("resource://gre/modules/AddonManager.jsm");
> +Components.utils.import("resource://gre/modules/Services.jsm");

Can you please file a follow-up bug which is about the change from Components.* to C*?

@@ +19,5 @@
>  const TIMEOUT = 5000;
>  const TIMEOUT_DOWNLOAD = 15000;
>  const TIMEOUT_REMOTE = 30000;
>  
> +var pm = Services.perms;

Please kill this and use Services.perms directly.

::: lib/downloads.js
@@ +41,5 @@
>  function downloadManager() {
>    this._controller = null;
>    this.downloadState = downloadState;
>  
> +  this._dms = Services.downloads;

I wonder if we should expose this service via the downloadManager class or not. I would remove it and access Services.downloads directly whereever it's needed in this module.

@@ +365,1 @@
>      // XXX it's possible that using a null char-set here is bad

Please kill that line. We should be fine with that.

::: lib/places.js
@@ +55,5 @@
>   * Instance of the observer service to gain access to the observer API
>   *
>   * @see http://mxr.mozilla.org/mozilla-central (nsIObserverService.idl)
>   */
> +var observerService = Services.obs;

We don't need that extra variable.

::: lib/prefs.js
@@ +202,5 @@
>  /**
>   * Preferences object to simplify the access to the nsIPrefBranch.
>   */
>  var preferences = {
> +  _prefService : Services.prefs,

Same as for the downloads module. Should we really expose it?

::: lib/screenshot.js
@@ +77,5 @@
>    file.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, parseInt("0666", 8));
>  
>    // create a data url from the canvas and then create URIs of the source
>    // and targets
> +  var io = Services.io;

Kill that variable please.

::: lib/search.js
@@ +345,5 @@
>   */
>  function searchBar(controller)
>  {
>    this._controller = controller;
> +  this._bss = Services.search;

Same as above. Do we want that?

::: lib/software-update.js
@@ +85,5 @@
>    this._aus = Cc["@mozilla.org/updates/update-service;1"].
>                getService(Ci.nsIApplicationUpdateService);
>    this._ums = Cc["@mozilla.org/updates/update-manager;1"].
>                getService(Ci.nsIUpdateManager);
> +  this._vc = Services.vc;

Again regarding exposing it.

@@ +234,5 @@
>     * @returns {String} OS version
>     */
>    get OSVersion() {
>      let osVersion;
> +    let sysInfo = Services.sysinfo;

Kill this variable.

@@ +400,5 @@
>    /**
>     * Update the update.status file and set the status to 'failed:6'
>     */
>    forceFallback : function softwareUpdate_forceFallback() {
> +    var dirService = Services.dirsvc;

Kill this variable.

::: lib/tabs.js
@@ +51,5 @@
>    var tabs = [ ];
>  
>    var uri = utils.createURI(aUrl, null, null);
>  
> +  var winEnum = Services.wm.getEnumerator("navigator:browser");

Can you move this line below the comment and rename the variable to windows?

::: lib/utils.js
@@ +31,5 @@
>     * @type nsiXULRuntime
>     */
>    get appInfo() {
>      if (!this._appInfo) {
> +      this._service = Services.appinfo;

I think we should get rid of that member now.

::: tests/functional/restartTests/testRestartChangeArchitecture/test1.js
@@ +7,5 @@
>  // Include required modules
>  var {expect} = require("../../../../lib/assertions");
>  
>  if (mozmill.isMac) {
> +  var runtime = Services.appinfo;

Kill this variable please.

::: tests/functional/testInstallation/testBreakpadInstalled.js
@@ +24,5 @@
>  {
>    controller = mozmill.getBrowserController();
>  
>    // Get the application folder
> +  module.appDir = Services.dirsvc.get("XCurProcD", Ci.nsILocalFile);

Lets get this moved down where we need it. I don't think we need the extra variable at all.

::: tests/functional/testPasswordManager/testPasswordNotificationBar.js
@@ +18,5 @@
>  var setupModule = function() {
>    controller = mozmill.getBrowserController();
>    locationBar =  new toolbars.locationBar(controller);
>  
> +  pm = Services.logins;

Kill this variable.

::: tests/functional/testPreferences/testDisableCookies.js
@@ +16,5 @@
>  
>  var setupModule = function() {
>    controller = mozmill.getBrowserController();
>  
> +  cm = Services.cookies;

Kill this variable.

::: tests/functional/testPreferences/testEnableCookies.js
@@ +16,5 @@
>  
>  var setupModule = function() {
>    controller = mozmill.getBrowserController();
>  
> +  cm = Services.cookies;

Kill this variable.

::: tests/functional/testPreferences/testPasswordNotSaved.js
@@ +16,5 @@
>  
>  var setupModule = function() {
>    controller = mozmill.getBrowserController();
>  
> +  pm = Services.logins;

Kill this variable.

::: tests/functional/testPreferences/testPasswordSavedAndDeleted.js
@@ +19,5 @@
>  function setupModule() {
>    controller = mozmill.getBrowserController();
>    locationBar = new toolbars.locationBar(controller);
>  
> +  pm = Services.logins;

Kill this variable.

::: tests/functional/testPreferences/testRemoveCookie.js
@@ +16,5 @@
>  
>  var setupModule = function() {
>    controller = mozmill.getBrowserController();
>  
> +  cm = Services.cookies;

Kill this variable.

::: tests/functional/testPrivateBrowsing/testAboutCache.js
@@ +16,5 @@
>    controller = mozmill.getBrowserController();
>    pbWindow = new privateBrowsing.PrivateBrowsingWindow();
>  
>    // Clear cache
> +  cs = Services.cache;

Kill this variable.

::: tests/functional/testSecurity/testDVCertificate.js
@@ +11,5 @@
>  var setupModule = function(module) {
>    controller = mozmill.getBrowserController();
>  
>    // Effective TLD Service for grabbing certificate info
> +  gETLDService = Services.eTLD;

Kill this variable.

::: tests/functional/testSecurity/testGreenLarry.js
@@ +13,5 @@
>  var setupModule = function(module) {
>    controller = mozmill.getBrowserController();
>  
>    // Effective TLD Service for grabbing certificate info
> +  gETLDService = Services.eTLD;

Kill this variable.

Comment 3

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v2

Updated the requested changes.
Reports:
* http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb51228dea
* http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb5123368b

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 705359 [details] [diff] [review]
patch v2

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

Looks great. Pushed to default:
http://hg.mozilla.org/qa/mozmill-tests/rev/2153fcde2957

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

There is no flag anymore for Firefox 10esr. So we might not have to backport it to this branch anymore given that it will be obsolete soon.

Comment 6

[Andreea Matei [:AndreeaMatei]]

Attachment: [release, ESR17] patch

Applies to release and ESR17 (some of the tests still contain assertJSProperty).

Reports:
* http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb51d4f9c4
* http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb51d7b34f

Comment 7

[Andreea Matei [:AndreeaMatei]]

Attachment: [beta] patch

Patch for Beta.

Report:
* http://mozmill-crowd.blargon7.com/#/functional/report/72e15dc943833b8fcba70aeb51d8680d

The one for default applies cleanly to Aurora as well.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Landed aurora and beta:
http://hg.mozilla.org/qa/mozmill-tests/rev/95bb57964ae3 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/427690b9af16 (beta)

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 707621 [details] [diff] [review]
[release, ESR17] patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/f36eb967fb4e (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/a10dab69c13e (esr17)