Closed Bug 1417937 Opened 7 years ago Closed 6 years ago

Remove unnecessary LoadContextInfo.jsm

Categories

(Toolkit :: General, enhancement)

Product:
Toolkit
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: standard8, Assigned: hemantsingh1612, Mentored)

References

Details

(Keywords: good-first-bug, Whiteboard: [lang=js])

Attachments

(1 file, 2 obsolete files)

Bug 1417937 - Remove unnecessary LoadContextInfo.jsm
59 bytes, text/x-review-board-request
standard8
: review+
Details 
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.
Blocks: 1417944
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]
Hi I would be happy to take this one (my first bug) :)
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
Hi,

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

Thanks
Attachment #8931873 - Attachment is obsolete: true
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)
Assignee: nicolaptv → mandercs3
Blocks: 1421379
After fixing the issues, I'm getting an error when pushing to MozReview

abort: cannot submit reviews referencing multiple bugs
(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)
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)
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)
Assignee: nobody → hemantsingh1612
Attachment #8931874 - Attachment is obsolete: true
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.
Comment on attachment 8946552 [details]
Bug 1417937 - Remove unnecessary LoadContextInfo.jsm

https://reviewboard.mozilla.org/r/216580/#review222278
Attachment #8946552 - Flags: review?(standard8)
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 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)
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)
Flags: needinfo?(hemantsingh1612)
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.
> 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.
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.
I should have noticed it.Sorry about that :)
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/46ccec9ffb88
Remove unnecessary LoadContextInfo.jsm r=standard8
No problem. It seems to be reasonable ok now, so I've pushed it to autoland and hopefully it'll stick.
https://hg.mozilla.org/mozilla-central/rev/46ccec9ffb88
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: