1374847 - Remove nsIPrefBranch2 and nsIPrefBranchInternal

Status: RESOLVED FIXED

(Core :: Preferences: Backend, enhancement, P3)

Description

[Nicholas Nethercote [inactive]]

nsIPrefBranch2 and nsIPrefBranchInternal are both trivial subclasses of nsIPrefBranch which exist purely for add-on compatibility.

Comment 1

[Benjamin Smedberg]

Note, there are still some references to nsIPrefBranchInternal in-tree that will need to be updated.

This is blocked until FF57 for addon compat.

Comment 2

[Masatoshi Kimura [:emk]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ff0158edbc857057a97a0d4231ce9bf7eb1d3c5

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: Bug 1374847 - Remove nsIPrefBranch2 and nsIPrefBranchInternal.

Review commit: https://reviewboard.mozilla.org/r/165110/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/165110/

Comment 4

[Nicholas Nethercote [inactive]]

Comment on attachment 8894021 [details]
Bug 1374847 - Remove nsIPrefBranch2 and nsIPrefBranchInternal.

https://reviewboard.mozilla.org/r/165110/#review170632

::: modules/libpref/nsPrefBranch.h
(Diff revision 1)
>  {
>    friend class mozilla::PreferenceServiceReporter;
>  public:
>    NS_DECL_ISUPPORTS
>    NS_DECL_NSIPREFBRANCH
> -  NS_DECL_NSIPREFBRANCH2

LOL, this class was inheriting from nsIPrefBranchInternal, but using the NS_DECL\_\* macros from nsIPrefBranch and nsIPrefBranch2.

::: modules/libpref/nsPrefBranch.cpp
(Diff revision 1)
>  
>  NS_INTERFACE_MAP_BEGIN(nsPrefBranch)
>    NS_INTERFACE_MAP_ENTRY_AMBIGUOUS(nsISupports, nsIPrefBranch)
>    NS_INTERFACE_MAP_ENTRY(nsIPrefBranch)
> -  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIPrefBranch2, !mIsDefault)
> -  NS_INTERFACE_MAP_ENTRY_CONDITIONAL(nsIPrefBranchInternal, !mIsDefault)

This is the only non-obvious part of this patch. I don't understand why the `!mIsDefault` conditions are there; perhaps it dates from the time when the nsIPrefBranch{2,Internal} were not trivial subclasses of nsIPrefBranch.

But because nsIPrefBranch is a superclass of nsIPrefBranch2 and nsIPrefBranchInternal they must be dead code. So I guess it's ok.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e405ea7e3943
Remove nsIPrefBranch2 and nsIPrefBranchInternal. r=njn

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e405ea7e3943

Comment 7

[Ed Morley [:emorley]]

Just as an extra data point - with this change NoScript broke completely, preventing JS from loading on any page.

Hopefully they'll make the s/nsIPrefBranch2/nsIPrefBranch/ change soon [1], but for now I've had to roll back to the previous Nightly [2], since I'm not browsing without NoScript for security reasons.

I realise traditional addons are going away in Firefox 57, and so this is why we're making breaking changes, but in the meantime there are people who still rely in Nightly to do their work, so these changes do cause disruption.


[1] https://forums.informaction.com/viewtopic.php?f=10&t=23150
[2] https://download-installer.cdn.mozilla.net/pub/firefox/nightly/2017/08/2017-08-07-11-34-52-mozilla-central/

Comment 8

[u462496]

(In reply to Ed Morley [:emorley] from comment #7)
> Just as an extra data point - with this change NoScript broke completely,
> preventing JS from loading on any page.
> 
> Hopefully they'll make the s/nsIPrefBranch2/nsIPrefBranch/ change soon [1],
> but for now I've had to roll back to the previous Nightly [2], since I'm not
> browsing without NoScript for security reasons.
> 
> I realise traditional addons are going away in Firefox 57, and so this is
> why we're making breaking changes, but in the meantime there are people who
> still rely in Nightly to do their work, so these changes do cause disruption.
> 
> 
> [1] https://forums.informaction.com/viewtopic.php?f=10&t=23150
> [2]
> https://download-installer.cdn.mozilla.net/pub/firefox/nightly/2017/08/2017-
> 08-07-11-34-52-mozilla-central/

Yes, it seems it would be more graceful to wait until legacy addons are removed from Nightly.  Is there a reason for not waiting?

As much as I would like to be able to switch all my addons to WebExtensions, my WE versions are still not complete enough, and unfortunately my time is limited when I can work on them, so wringing all I can out of the legacy versions would be helpful.

Comment 9

[u462496]

Just an additional nit, there is nothing in the doc about nsIPrefBranch2 being deprecated.  But maybe there were other sources I should have been aware of.

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIPrefBranch2

Comment 10

[Dão Gottwald [:dao]]

(In reply to Kevin Jones from comment #8)
> Yes, it seems it would be more graceful to wait until legacy addons are
> removed from Nightly.  Is there a reason for not waiting?

AFAIK extensions.legacy.enabled was already supposed to be flipped and it's now expected to happen very soon.

Comment 11

[Dão Gottwald [:dao]]

(In reply to Dão Gottwald [::dao] from comment #10)
> (In reply to Kevin Jones from comment #8)
> > Yes, it seems it would be more graceful to wait until legacy addons are
> > removed from Nightly.  Is there a reason for not waiting?
> 
> AFAIK extensions.legacy.enabled was already supposed to be flipped and it's
> now expected to happen very soon.

It just landed. Bug 1388946.

Comment 12

[Jan Tojnar]

Duplicate of Bug 795718?