Remove unnecessary LoadContextInfo.jsm

RESOLVED FIXED in Firefox 60

Status

()

enhancement
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: standard8, Assigned: hemantsingh1612, Mentored)

Tracking

({good-first-bug})

Trunk
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 wontfix, firefox60 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

a year ago
toolkit/modules/LoadContextInfo.jsm is a file that exposes the nsILoadContextInfoFactory to legacy extensions - it loads the service and exposes that.

This does exactly the same as Services.loadContextInfo.

Since we don't need to support legacy extensions now, I think we should drop the .jsm file and switch everything to use the Services variant.
(Reporter)

Updated

a year ago
Blocks: 1417944
(Reporter)

Comment 1

a year ago
I'm happy to mentor this.

Approximate steps and hints:

- The main part of this bug is to remove "LoadContextInfo.jsm". You can search for instances where it is referenced here: https://searchfox.org/mozilla-central/search?q=LoadContextInfo.jsm&case=false&regexp=false&path=

- Where a file imports the .jsm, remove the import line, and look within the file for "LoadContextInfo", that needs to change to "Services.loadContextInfo" (note also the change of case).

- Make sure you run ESLint on the files, if it shows "Services" as an undefined variable, you'll need to import Services.jsm (see other files for how to do that).

To run eslint do either:

./mach lint -l eslint path/to/file

or

./mach lint -l eslint path/to/directory

- Also, remove from the repository the actual LoadContextInfo.jsm, and remove the references to it in the toolkit/modules/moz.build file.

- Create a commit with a message of something like "Bug 1417937 - Remove LoadContextInfo.jsm and replace it by calls direct to Services.jsm. r?Standard8" and push it to mozreview (https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview-user.html)

If you have any questions please ask.
Mentor: standard8
Keywords: good-first-bug
Whiteboard: [lang=js]

Comment 2

a year ago
Hi I would be happy to take this one (my first bug) :)
(Reporter)

Comment 3

a year ago
Hi Nicola, please do start working on it if you haven't already. If you get any problems or have questions, please ask.
Assignee: nobody → nicolaptv

Comment 4

a year ago
Hi,

Will I need to import "resource://gre/modules/Services.jsm" when changing each file's occurrence of "LoadContextInfo" to "Services.loadContextInfo"?

Thanks

Updated

a year ago
Attachment #8931873 - Attachment is obsolete: true
(Reporter)

Comment 7

a year ago
mozreview-review
Comment on attachment 8931874 [details]
Bug 1417937 - Remove LoadContextInfo.jsm and replace it by calls direct to Services.jsm.

https://reviewboard.mozilla.org/r/202972/#review208548

Thank you for the patch, this is a good start. Next time you push, please push without the additional commit - otherwise mozreview won't let us push the final patch.

Did you run build & eslint after completing the patch? It looks like you didn't as there's a build error (mentioned below), plus there's a couple more issues that ESLint is pulling up:

browser/components/places/tests/unit/test_clearHistory_shutdown.js
  127:40  error  'LoadContextInfo' is not defined.  no-undef (eslint)
  162:40  error  'LoadContextInfo' is not defined.  no-undef (eslint)

It is usually worth checking the patch before publishing as it avoids extra round trips for reviews, and helps for a faster landing.

(Also, it might just be nice to ask before taking on a patch when someone else has recently asked to take it - they might have been in the middle of the patch when you took it on).

::: browser/base/content/test/general/browser_sanitizeDialog.js:555
(Diff revision 1)
>    Services.perms.addFromPrincipal(principal, "offline-app", Ci.nsIOfflineCacheUpdateService.ALLOW_NO_WARN);
>  
>    // Store something to the offline cache
>    var appcacheserv = Cc["@mozilla.org/network/application-cache-service;1"]
>                       .getService(Ci.nsIApplicationCacheService);
> -  var appcachegroupid = appcacheserv.buildGroupIDForInfo(makeURI(URL + "/manifest"), LoadContextInfo.default);
> +  var appcachegroupid = appcacheserv.buildGroupIDForInfo(makeURI(URL + "/manifest"), Services.LoadContextInfo.default);

`Services.LoadContextInfo` -> `Services.loadContextInfo` (case change for the first 'L')

::: browser/base/content/test/general/browser_sanitizeDialog.js:557
(Diff revision 1)
>    // Store something to the offline cache
>    var appcacheserv = Cc["@mozilla.org/network/application-cache-service;1"]
>                       .getService(Ci.nsIApplicationCacheService);
> -  var appcachegroupid = appcacheserv.buildGroupIDForInfo(makeURI(URL + "/manifest"), LoadContextInfo.default);
> +  var appcachegroupid = appcacheserv.buildGroupIDForInfo(makeURI(URL + "/manifest"), Services.LoadContextInfo.default);
>    var appcache = appcacheserv.createApplicationCache(appcachegroupid);
> -  var storage = Services.cache2.appCacheStorage(LoadContextInfo.default, appcache);
> +  var storage = Services.cache2.appCacheStorage(Services.LoadContextInfo.default, appcache);

`Services.LoadContextInfo` -> `Services.loadContextInfo` (case change for the first 'L')

::: browser/components/originattributes/test/browser/browser_cache.js:59
(Diff revision 1)
>          throw Components.results.NS_ERROR_NO_INTERFACE;
>        }
>      };
>      // Visiting the disk cache also visits memory storage so we do not
>      // need to use Services.cache2.memoryCacheStorage() here.
> -    let storage = Services.cache2.diskCacheStorage(loadContextInfo, false);
> +    let storage = Services.cache2.diskCacheStorage(Services.LoadContextInfo, false);

`Services.LoadContextInfo` -> `Services.loadContextInfo` (case change for the first 'L')

::: browser/modules/offlineAppCache.jsm:5
(Diff revision 1)
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> -this.EXPORTED_SYMBOLS = ["OfflineAppCacheHelper"];
> +Components.utils.import("resource://gre/modules/Services.jsm");

Please move this to after the `const Ci` line, and add a section like:

```
const Cu = Components.utils;

Cu.import("resource://gre/modules/Services.jsm");
```

::: netwerk/test/unit/test_inhibit_caching.js:1
(Diff revision 1)
> -Cu.import('resource://gre/modules/LoadContextInfo.jsm');
> +Components.utils.import("resource://gre/modules/Services.jsm");

It looks like this line can be dropped for this file, as there wasn't any LoadContextInfo used (and Services isn't used).

::: npm-shrinkwrap.json
(Diff revision 1)
>  {
>    "name": "mozillaeslintsetup",
> -  "requires": true,

Please undo the changes to this file. They shouldn't be here (sometimes these are caused by older versions of node).

::: toolkit/modules/moz.build
(Diff revision 1)
>  
>  with Files('LightweightThemeConsumer.jsm'):
>      BUG_COMPONENT = ('Firefox', 'Toolbars and Customization')
>  
> -with Files('LoadContextInfo.jsm'):
> -    BUG_COMPONENT = ('Core', 'Networking: Cache')

You're missing removing the other instance of LoadContextInfo.jsm from this file - you can check that with a simple `./mach build` before pushing.
Attachment #8931874 - Flags: review?(standard8)
(Reporter)

Updated

a year ago
Assignee: nicolaptv → mandercs3
(Reporter)

Updated

a year ago
Blocks: 1421379

Comment 8

a year ago
After fixing the issues, I'm getting an error when pushing to MozReview

abort: cannot submit reviews referencing multiple bugs
(Reporter)

Comment 9

a year ago
(In reply to mandercs3 from comment #8)
> abort: cannot submit reviews referencing multiple bugs

You need to set your tree so that you are just pushing the changeset related to this bug. You might need to create a new bookmark (hg) / branch (git) depending on what you're using - the new bookmark would be off of default or master, and would contain just the changeset for this bug.

This has some background information for mercurial:

https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/bookmarks.html

If you're still not sure, asking in #introduction on https://wiki.mozilla.org/Irc is a good place to start (though it might be more quiet in there this week as there is a Mozilla meet-up)
(Reporter)

Comment 10

a year ago
mandercs3: Are you still working on this? See my previous comment for more information. If you didn't get anyone on irc, now might be a better time as we are all back from holiday.
Flags: needinfo?(mandercs3)
(Reporter)

Comment 11

a year ago
No response from mandercs3 unfortunately, so re-opening this for others to pick up.

The current patch needs updating as per comment 7.
Assignee: mandercs3 → nobody
Flags: needinfo?(mandercs3)
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Assignee: nobody → hemantsingh1612
(Reporter)

Updated

a year ago
Attachment #8931874 - Attachment is obsolete: true
(Reporter)

Comment 13

a year ago
mozreview-review
Comment on attachment 8946552 [details]
Bug 1417937 - Remove unnecessary LoadContextInfo.jsm

https://reviewboard.mozilla.org/r/216580/#review222264

Thank you for taking this on, I think we're not far off now. There's a few comments below. I've also just done a case-sensitive grep after applying the patch, and found a few more instances that need fixing up.

These were all getting LoadContextInfo.jsm via the head.js file, so they need the `LoadContextInfo` instances replacing.

netwerk/test/unit/test_bug248970_cache.js
netwerk/test/unit/test_bug767025.js
netwerk/test/unit/test_cache2-06-pb-mode.js
netwerk/test/unit/test_cache2-30a-entry-pinning.js
netwerk/test/unit/test_cache2-30b-pinning-storage-clear.js
netwerk/test/unit/test_cache2-30c-pinning-deferred-doom.js
netwerk/test/unit/test_cache2-30d-pinning-WasEvicted-API.js
netwerk/test/unit/test_cache2-31-visit-all.js
netwerk/test/unit/test_doomentry.js
netwerk/test/unit/test_private_necko_channel.js

Also:

toolkit/modules/moz.build needs the "with Files" entry removing for LoadContextInfo.jsm

::: browser/modules/offlineAppCache.jsm:7
(Diff revision 1)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  this.EXPORTED_SYMBOLS = ["OfflineAppCacheHelper"];
>  
> -Components.utils.import("resource://gre/modules/LoadContextInfo.jsm");
> +Components.utils.import("resource://gre/modules/Services.jsm");

Please move this to after the `const Ci` line, and make it like:

```
const Cu = Components.utils;

Cu.import("resource://gre/modules/Services.jsm");
```

::: mobile/android/modules/Sanitizer.jsm:6
(Diff revision 1)
>  // -*- indent-tabs-mode: nil; js-indent-level: 4 -*-
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  /* globals LoadContextInfo, FormHistory, Accounts */

Please remove all of this line - it is from the early days of our ESLint implementation and for this file it isn't needed any more.

::: netwerk/test/unit/test_cache2-21-anon-storage.js:1
(Diff revision 1)
> -Components.utils.import('resource://gre/modules/LoadContextInfo.jsm');
> +Components.utils.import("resource://gre/modules/Services.jsm");

You don't need Services.jsm here as it is imported via the head_cache.js file.

Please can you remove this from all the netwerk/test/unit/test_cache*.js files.
(Reporter)

Comment 14

a year ago
mozreview-review
Comment on attachment 8946552 [details]
Bug 1417937 - Remove unnecessary LoadContextInfo.jsm

https://reviewboard.mozilla.org/r/216580/#review222278
Attachment #8946552 - Flags: review?(standard8)
Comment hidden (mozreview-request)
(Reporter)

Comment 16

a year ago
mozreview-review
Comment on attachment 8946552 [details]
Bug 1417937 - Remove unnecessary LoadContextInfo.jsm

https://reviewboard.mozilla.org/r/216580/#review222528

This is great, there's one minor change needed. I've pushed it to try server (link via mozreview), to check that tests are passing, but apart from that we should hopefully be good to go.

::: mobile/android/modules/Sanitizer.jsm
(Diff revision 2)
> -// -*- indent-tabs-mode: nil; js-indent-level: 4 -*-
> -/* This Source Code Form is subject to the terms of the Mozilla Public
> - * License, v. 2.0. If a copy of the MPL was not distributed with this
> - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Please can you add the license header back in.

The indent-tabs-mode line can stay out (it is wrong anyway), but the license header should still be in the file.
Attachment #8946552 - Flags: review?(standard8)
Comment hidden (mozreview-request)
(Reporter)

Comment 18

a year ago
mozreview-review
Comment on attachment 8946552 [details]
Bug 1417937 - Remove unnecessary LoadContextInfo.jsm

https://reviewboard.mozilla.org/r/216580/#review222632

Great, looks good & try server says it is all fine as well.
Attachment #8946552 - Flags: review?(standard8) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s d42e62b4f1af486c7a9da07a326aca60ea3877df -d 334052f09a38: rebasing 444922:d42e62b4f1af "Bug 1417937 - Remove unnecessary LoadContextInfo.jsm r=standard8" (tip)
merging browser/base/content/pageinfo/pageInfo.js
merging browser/base/content/test/general/browser_sanitizeDialog.js
merging browser/base/content/test/general/browser_save_private_link_perwindowpb.js
merging browser/components/originattributes/test/browser/browser_cache.js
merging browser/components/originattributes/test/browser/browser_sanitize.js
merging browser/components/places/tests/unit/head_bookmarks.js
merging browser/components/places/tests/unit/test_clearHistory_shutdown.js
merging browser/components/preferences/in-content/main.js
merging browser/components/privatebrowsing/test/browser/browser_privatebrowsing_cache.js
merging browser/components/tests/unit/test_distribution.js
merging browser/modules/offlineAppCache.jsm
merging dom/html/test/browser_bug649778.js
merging mobile/android/modules/Sanitizer.jsm
merging netwerk/test/unit/head_cache.js
merging netwerk/test/unit/test_bug767025.js
merging netwerk/test/unit/test_cache2-21-anon-storage.js
merging netwerk/test/unit/test_cache2-22-anon-visit.js
merging netwerk/test/unit/test_cache2-23-read-over-chunk.js
merging netwerk/test/unit/test_cache2-24-exists.js
merging netwerk/test/unit/test_cache2-25-chunk-memory-limit.js
merging netwerk/test/unit/test_cache2-27-force-valid-for.js
merging netwerk/test/unit/test_inhibit_caching.js
merging netwerk/test/unit/test_predictor.js
merging netwerk/test/unit/test_private_necko_channel.js
warning: conflicts while merging browser/base/content/pageinfo/pageInfo.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/base/content/test/general/browser_sanitizeDialog.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/base/content/test/general/browser_save_private_link_perwindowpb.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/originattributes/test/browser/browser_cache.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/originattributes/test/browser/browser_sanitize.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/places/tests/unit/head_bookmarks.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/preferences/in-content/main.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/privatebrowsing/test/browser/browser_privatebrowsing_cache.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/components/tests/unit/test_distribution.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/modules/offlineAppCache.jsm! (edit, then use 'hg resolve --mark')
warning: conflicts while merging dom/html/test/browser_bug649778.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging netwerk/test/unit/head_cache.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging netwerk/test/unit/test_cache2-21-anon-storage.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging netwerk/test/unit/test_cache2-22-anon-visit.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging netwerk/test/unit/test_cache2-23-read-over-chunk.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging netwerk/test/unit/test_cache2-24-exists.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging netwerk/test/unit/test_cache2-25-chunk-memory-limit.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging netwerk/test/unit/test_cache2-27-force-valid-for.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging netwerk/test/unit/test_inhibit_caching.js! (edit, then use 'hg resolve --mark')
warning: conflicts while merging netwerk/test/unit/test_predictor.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
(Reporter)

Comment 20

a year ago
I thought that might happen, but I thought I'd try it anyway... 

Hemant, bug 1431533 landed overnight, so this needs rebasing against the latest master.

That bug basically changed:

- XPCOMUtils.defineLazyModuleGetter to ChromeUtils.defineModuleGetter
- Cu.import to ChromeUtils.import

The arguments stay the same in both instances.

If you can rebase & check if there's any changes need to be made - you should also be able to run `./mach eslint --fix` to fix those.
Flags: needinfo?(hemantsingh1612)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Flags: needinfo?(hemantsingh1612)
(Reporter)

Comment 22

a year ago
mozreview-review
Comment on attachment 8946552 [details]
Bug 1417937 - Remove unnecessary LoadContextInfo.jsm

https://reviewboard.mozilla.org/r/216580/#review222716

::: browser/components/places/tests/unit/head_bookmarks.js
(Diff revision 4)
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  var Ci = Components.interfaces;
>  var Cc = Components.classes;
>  var Cr = Components.results;
> -var Cu = Components.utils;

Try server is failing with:

2689 13:40:01 WARNING - TEST-UNEXPECTED-FAIL | browser/components/places/tests/unit/test_421483.js | xpcshell return code: 0
2692 13:40:01 INFO - ReferenceError: Cu is not defined at /builds/worker/workspace/build/tests/xpcshell/tests/browser/components/places/tests/unit/head_bookmarks.js -> file:///builds/worker/workspace/build/tests/xpcshell/tests/toolkit/components/places/tests/head_common.js:27 

You need to add just the `var Cu = Components.utils;` line back in.

If you can then verify with `./mach xpcshell-test browser/components/places/tests/unit/` (after a build if you haven't already), that'd be great.
Comment hidden (mozreview-request)
(Assignee)

Comment 24

a year ago
> If you can then verify with `./mach xpcshell-test
> browser/components/places/tests/unit/` (after a build if you haven't
> already), that'd be great.

 0:58.38 INFO INFO | Result summary:
 0:58.38 INFO INFO | Passed: 13
 0:58.38 INFO INFO | Failed: 1
 0:58.38 INFO INFO | Todo: 0
 0:58.38 INFO INFO | Retried: 1

1 failed probably because MOZ_NODE_PATH environment variable was not set.
(Reporter)

Comment 25

a year ago
mozreview-review
Comment on attachment 8946552 [details]
Bug 1417937 - Remove unnecessary LoadContextInfo.jsm

https://reviewboard.mozilla.org/r/216580/#review222772

::: browser/components/places/tests/unit/test_clearHistory_shutdown.js:127
(Diff revision 5)
>    });
>  }
>  
>  function storeCache(aURL, aContent) {
>    let cache = Services.cache2;
> -  let storage = cache.diskCacheStorage(LoadContextInfo.default, false);
> +  let storage = cache.diskCacheStorage(Services.Services.loadContextInfo.default, false);

Ah, found it, the failure was:

ERROR Unexpected exception TypeError: Services.Services is undefined at /Users/mark/dev/gecko/objdir-ff-debug/_tests/xpcshell/browser/components/places/tests/unit/test_clearHistory_shutdown.js:127

It is this line - you've got `Services.Services...` - there should only be one.
(Assignee)

Comment 26

a year ago
I should have noticed it.Sorry about that :)
Comment hidden (mozreview-request)

Comment 28

a year ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/46ccec9ffb88
Remove unnecessary LoadContextInfo.jsm r=standard8
(Reporter)

Comment 29

a year ago
No problem. It seems to be reasonable ok now, so I've pushed it to autoland and hopefully it'll stick.

Comment 30

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/46ccec9ffb88
Status: NEW → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.