a couple versions after the deprecation in bug 850219 we should remove the old synchronous service and, if possible, merge the new service module into the service. As reported in the module: 17 // The plan is to eventually remove nsIContentPrefService and its 18 // implementation, nsContentPrefService.js. At such time this file can stop 19 // being a JSM, and the "_cps" parts that ContentPrefService2 relies on and 20 // NSGetFactory and all the other XPCOM initialization goop in 21 // nsContentPrefService.js can be moved here.

note also Services.contentPrefs, I'm not sure if we should just remove it, replace it, or introduce a contentPrefs2. It is only used in a couple places in our codebase, so maybe it's not worth to have it in Services.

This should also be good to kill the nsContentPrefService.js compartment at startup

Drew, would you be interested in mentoring the remaining work here?

Sure!

I wonder if we can call this a "first" bug...

Drew, is this something easy enough to make it a [good first bug]?

Sure, probably. :-)

Raj contacted me through email saying he'd like to work on this bug. I've assigned him.

Hey Raj, Do you have a build of Firefox on your machine ready to go? If not, we can help you through that step.

I've been communicating with Raj over email, and is getting his build set up. In the meantime, adw, did you have some starting tips for Raj?

There's kind of a lot of stuff to do, but fortunately it's pretty straightforward I think. The steps are below. And I'm probably forgetting things. Let me know when you run into problems or need more details. 1. Modify ContentPrefService2.jsm This file implements nsIContentPrefService2. There's a separate file, nsContentPrefService.js, that implements the old service we're removing. There's overlap in the functionality of the old and new service, so currently, to avoid reimplementing that overlap, ContentPrefService2.jsm relies on some implementation bits in nsContentPrefService.js. We need to copy those bits into ContentPrefService2.jsm so that nsContentPrefService.js can be removed. Please read the big comment at the top of ContentPrefService2.jsm for some info on that. Sub-steps: * Rename ContentPrefService2.jsm to ContentPrefService2.js (take off the "m" at the end): https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/ContentPrefService2.jsm * Remove the big comment starting here: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/ContentPrefService2.jsm#5 * Remove EXPORTED_SYMBOLS: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/ContentPrefService2.jsm#25 * Replace the usage of the _cps member with the bits copied over from nsContentPrefService.js. That includes the HostnameGrouper prototype from nsContentPrefService.js and the "XPCOM Plumbing" at the bottom of that file. 2. Update nsContentPrefService.manifest https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/nsContentPrefService.manifest Replace the "nsContentPrefService.js" references in this file with "ContentPrefService2.js". 3. Remove the old service * Remove the idl file: https://dxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIContentPrefService.idl * Remove the reference to that file here, in this moz.build file: https://dxr.mozilla.org/mozilla-central/source/dom/interfaces/base/moz.build * Remove this whole directory, which contains tests for the old service: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/tests/unit * Remove the reference to that directory here: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/moz.build#8 * Remove ContentPrefInstance.jsm, since it's a companion to the old service, and there aren't any non-test consumers of it in m-c anymore: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/ContentPrefInstance.jsm * Remove the reference to that file here: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/moz.build#22 * Remove this test file: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/tests/unit_cps2/test_service.js * Remove the reference to that file here, in the tests manifest: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/contentprefs/tests/unit_cps2/xpcshell.ini#7 4. Update places that reference the old service * Find cpp files that #include nsIContentPrefService.h, remove the #include, and if the file does not already #include nsIContentPrefService2.h (note the 2), add an #include for it. You can find those cpp files here (there are only two): https://dxr.mozilla.org/mozilla-central/search?q=%23include%20%22nsIContentPrefService.h%22 * This line should reference nsIContentPrefService2 (the new service), not nsIContentPrefService (the old one): https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Services.jsm#71 * Same here: https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/tests/xpcshell/test_Services.js#36 * Remove this other deprecated method that relies on the old service: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/DownloadLastDir.jsm#113

okay sir,@adw Thanks a lot..!!

hello sir @adw in your instructions : Replace the usage of the _cps member with the bits copied over from nsContentPrefService.js. That includes the HostnameGrouper prototype from nsContentPrefService.js and the "XPCOM Plumbing" at the bottom of that file. In this instruction, what does _cps member mean ?Sir,Can you please explain this in detail ?

Hello sir,I've created this uncommited.patch file,please check it and suggest me anything is wrong..Thank you..!!

Comment on attachment 8825048 [details] [diff] [review] All changes are done..!!! Requesting review on behalf of Raj.

(qf:p3 since this is removing dead code as far as I can tell. ;-)

Drew, do you still intend to mentor this? If this is indeed the way to resolve the startup perf hit I'm seeing in bug 1361095 comment 5, it's pretty important to resolve this for photon.

Sure.

Raj, sorry for not reviewing this. Would you still like to work on it?

Raj, see comment #19. I'll take over if you don't respond within two weeks.

(In reply to Masatoshi Kimura [:emk] from comment #20) > Raj, see comment #19. I'll take over if you don't respond within two weeks. Raj's last bugzilla activity was in January, I don't think you need to wait more to take it over. To remind how important this bug is, here is a startup profile where this blocks us for 1617ms: https://perfht.ml/2uCYbfB

OK, taking.

Comment on attachment 8896331 [details] Bug 886907 - Remove old synchronous contentPrefService from the tree. https://reviewboard.mozilla.org/r/167592/#review173724 Thanks Masatoshi, looks good.

BTW there seems to be the idea in this bug that removing the old service alone will improve performance, but that's unlikely IMO, at least not significantly. The perf problem is the main-thread storage/SQLite calls that the service makes on init, and this patch doesn't change that (rightly, since that should be done separately).

Pushed by VYV03354@nifty.ne.jp: https://hg.mozilla.org/integration/autoland/rev/f92c249cae8b Remove old synchronous contentPrefService from the tree. r=adw

Backed out for failing editor/libeditor/tests/test_bug1368544.html on Android 4.3 debug: https://hg.mozilla.org/integration/autoland/rev/122fa30ec6605756d1221e91facd661e4b8069d7 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=f92c249cae8b855ff46035135785b73474bbc4ac&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable&filter-resultStatus=usercancel Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=123248311&repo=autoland [task 2017-08-15T12:30:21.407119Z] 12:30:21 INFO - 78 INFO None79 INFO TEST-START | editor/libeditor/tests/test_bug1368544.html [task 2017-08-15T12:35:32.798392Z] 12:35:32 INFO - Buffered messages logged at 12:30:16 [task 2017-08-15T12:35:32.798477Z] 12:35:32 INFO - 80 INFO must wait for load [task 2017-08-15T12:35:32.798718Z] 12:35:32 INFO - Buffered messages finished [task 2017-08-15T12:35:32.799494Z] 12:35:32 INFO - 81 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug1368544.html | Test timed out. [task 2017-08-15T12:35:32.800084Z] 12:35:32 INFO - reportError@SimpleTest/TestRunner.js:121:7 [task 2017-08-15T12:35:32.800731Z] 12:35:32 INFO - TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:7

I have no idea why it fails...

Ah, I got it. Mobile has its own package-manifest.in.

Try looks good, relanding. https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fb321b223a034d795b0ccb61bb39ab638935d9b

Pushed by VYV03354@nifty.ne.jp: https://hg.mozilla.org/integration/autoland/rev/832df1d6017e Remove old synchronous contentPrefService from the tree. r=adw

Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/8e0c7bfb03ce Port bug 886907 to TB/IB/SM: Package ContentPrefService2.js/manifest instead of nsContentPrefService.js/manifest. rs=bustage-fix