850219 - Mark nsIContentPrefService as deprecated in favor of nsIContentPrefService2

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

When we remove all the usage of nsIContentPrefService from the tree (bug 831208), we'll still need to keep the old API for a while for add-ons which are using it, but we should then mark it as deprecated and possibly add warnings to avoid its usage in new add-ons or even in new code in the tree

Comment 1

[Marco Bonardo [:mak]]

we should try to mark deprecation before next version merge, if possible.

Comment 2

[Drew Willcoxon :adw]

I'm planning on working on this this week.  Is there anything to do besides the following?

* Add [deprecated] to the nsIContentPrefService interface declaration
* Call Deprecated.warning (in Deprecated.jsm) from all the nsIContentPrefService methods, pointing to https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsIContentPrefService, which has a deprecated warning

Comment 3

[Marco Bonardo [:mak]]

I think that's a good plan of action.
Though iirc [deprecated] is not very well supported by xpidl, so we should just add @deprecated to the javadoc.
I think it may also be worth to cross post to mozilla.dev.extensions and firefox-dev to notify the change, and involve jorge (cc-ing him) for AMO.

Comment 4

[Drew Willcoxon :adw]

Attachment: patch

CPS2 calls CPS1's grouper, addObserver, and removeObserver, so I added back doors for each so CPS2 can call them without warning.  CPS1 implementations that CPS2 uses will be moved to CPS2 when we remove CPS1.

Comment 5

[Drew Willcoxon :adw]

Gavin mentioned linking to the CPS2 doc would be better than linking to the CPS1 doc, and I agree, so I'll make that change locally.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 776712 [details] [diff] [review]
patch

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

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +696,1 @@
>      return this._dbConnection;

This is used by ContentPrefInstance, that afaict is only used by tests. Should we deprecate that module as well? I'm not sure why it is in the component folder and not in the tests folder.

There are a lot of tests that are using nsIContentPrefService yet:

http://mxr.mozilla.org/mozilla-central/search?string=contentprefservice)
http://mxr.mozilla.org/mozilla-central/search?string=ContentPrefInstance

Do all of them already have an equivalent cps2 test?
I think it may be worth to try converting them (Well, apart the main cps test) before removing the old service completely. We could gain some test coverage on the new service that way.  I think it's worth a new bug blocking bug 886907.

Comment 7

[Marco Bonardo [:mak]]

Ugh, we forgot one consumer

http://mxr.mozilla.org/mozilla-central/search?string=.contentPrefs.&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

please fix it before deprecating.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

We didn't quite forget it, we just left it in for backwards-compat in bug 854299:
http://hg.mozilla.org/mozilla-central/annotate/604c8518cc50/toolkit/mozapps/downloads/DownloadLastDir.jsm#l104

We should probably also add a deprecation warning there. And/or just do what Felipe was going to do originally in bug 854299 comment 6 (see also bug 854299 comment 15).

Comment 9

[Marco Bonardo [:mak]]

I see, I should have verified the comment on that method, my fault!
I agree we should add a deprecation warning there.

Comment 10

[Drew Willcoxon :adw]

Yes, that's what I was going to type, but there are some tests and maybe other things that use DownloadLastDir in a way that ends up calling getFile, so those need to be fixed before adding a deprecation warning.  I'll file a bug for that but not right now because I'm busy doing a million other things.

Comment 11

[Marco Bonardo [:mak]]

getFileAsync() calls getFile() internally!
I will file the bug.

Comment 12

[Marco Bonardo [:mak]]

Bug 895187 for the DownloadsLastDir problem.

I also filed bug 895189 to convert old cps tests (apart from main test) to cps2 before removing the old service, as I said in comment 6. This is not blocking deprecation though.

Comment 13

[Drew Willcoxon :adw]

There are no tests that need to be converted.  The CPS2 tests are comprehensive.  We should leave the CPS1 tests alone until we remove CPS1.  Bug 895189 is invalid.

Comment 14

[Drew Willcoxon :adw]

(In reply to Marco Bonardo [:mak] from comment #11)
> getFileAsync() calls getFile() internally!

That's just an implementation detail, and the CPS1 path isn't followed by that particular call.

And on second look I'm not sure anymore that what I said is true: there are tests and maybe other things that end up calling getFile, but the one case I was looking at doesn't pass anything to getFile, so the CPS1 path isn't followed.  So bug 895187 may be invalid, too.  Just please let me take a closer look when I get time, or if you don't want to wait, please find all the uses of DownloadLastDir.jsm and follow the paths.

Comment 15

[Drew Willcoxon :adw]

Attachment: patch with updated DownloadLastDir.getFile

I'm sorry for being curt, Marco. :-\

I looked at all the files that import the jsm, and there aren't any problems, so I closed bug 895187 and added a more detailed explanation there.

This patch moves getFile's non-CPS1 path, the path called internally, to _getLastFile.  getFile warns when called.  (Originally I didn't have a _getLastFile and only put the warning in getFile's CPS1 path, but getFile itself is deprecated and should warn outside callers when they don't hit the CPS1 path, so I think this patch is better.)  A kind of awkward side effect is that you get two deprecation warnings, one for getFile, and then one for CPS1.  But I think that's OK.  The warning links to MDN's DownloadLastDir doc, which hasn't been updated for getFileAsync yet.

Comment 16

[Marco Bonardo [:mak]]

(In reply to Drew Willcoxon :adw from comment #13)
> There are no tests that need to be converted.  The CPS2 tests are
> comprehensive.

is that valid also for unit_ipc? I don't think we have ipc testing in cps2, though we still have bug 838097. There are still ipc consumers afaik, bug 777196 should be fixed.

Comment 17

[Drew Willcoxon :adw]

CPS2 doesn't do anything special to handle IPC, so it doesn't need IPC tests.  Is it supposed to do something special?  Those two bugs you mention really confuse me.  I've read bug 838097 multiple times over a period of months and I still don't understand what it's about.

Comment 18

[Marco Bonardo [:mak]]

bug 838097 it's basically about "get an answer and action on bug 777196, if that will require changes to cps2, then do them, otherwise we are done"

Comment 19

[Marco Bonardo [:mak]]

Comment on attachment 777528 [details] [diff] [review]
patch with updated DownloadLastDir.getFile

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

Please, get any sort of answer from dietrich or cjones about bug 777196 and whether b2g intercepts and exposes error console logs.
If they don't care we can just proceed.

::: toolkit/components/contentprefs/ContentPrefService2.jsm
@@ +695,5 @@
>      }
>      catch (err) {
>        return groupStr;
>      }
> +    return this._cps.__grouper.group(groupURI);

since we have _addObserver and _removeObserver I'd prefer _grouper, and use __grouper as the internal cache. So it's a bit more coherent.

::: toolkit/components/contentprefs/nsContentPrefService.js
@@ +686,1 @@
>    _grouper: null,

so this becomes __grouper

@@ +686,2 @@
>    _grouper: null,
> +  get __grouper() {

and this _grouper

::: toolkit/mozapps/downloads/DownloadLastDir.jsm
@@ +95,5 @@
>    isPrivate: function DownloadLastDir_isPrivate() {
>      return PrivateBrowsingUtils.isWindowPrivate(this.window);
>    },
>    // compat shims
> +  get file() { return this._getLastFile(); },

get file() this._getLastfile(),

@@ +138,4 @@
>    },
>  
>    getFileAsync: function(aURI, aCallback) {
> +    let plainPrefFile = this._getLastFile();

yep, this looks far less scary :)

Comment 20

[Drew Willcoxon :adw]

Attachment: updated patch

Review comments addressed.

Comment 21

[Drew Willcoxon :adw]

Gabriele, we'd like to land this patch to deprecate nsIContentPrefService, but we don't want to cause a problem for b2g.  The patch logs warnings via Cu.reportError every time nsIContentPrefService is used.  Is the deprecation or the warnings going to be a problem?

We ask because there are a couple of older bugs related to nsIContentPrefService and IPC/b2g: bug 777196 and bug 838097.

Comment 22

[Gabriele Svelto [:gsvelto]]

I've investigated the issue a bit more and from what I can tell this change should not affect B2G at all as we're not using nor relying on nsIContentPrefService-specific functionality or behavior.

On this topic I will also update bug 838097 with the relevant information so that we can move forward on that topic too.

Comment 23

[Drew Willcoxon :adw]

Thanks, Gabriele.  Given that, there's nothing further blocking landing I don't think, so I landed it.  (Using inbound instead of fx-team only because it's more convenient for me right now.)

https://hg.mozilla.org/integration/mozilla-inbound/rev/914bf3adc1ed

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/914bf3adc1ed

Comment 25

[Drew Willcoxon :adw]

(In reply to Drew Willcoxon :adw from comment #15)
> The warning links to MDN's DownloadLastDir doc, which hasn't been
> updated for getFileAsync yet.

Updated: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/DownloadLastDir.jsm