Closed Bug 1374847 Opened 3 years ago Closed 3 years ago

Remove nsIPrefBranch2 and nsIPrefBranchInternal

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: njn, Assigned: emk)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

nsIPrefBranch2 and nsIPrefBranchInternal are both trivial subclasses of nsIPrefBranch which exist purely for add-on compatibility.
Note, there are still some references to nsIPrefBranchInternal in-tree that will need to be updated.

This is blocked until FF57 for addon compat.
Priority: -- → P3
Depends on: 1387017
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.
Attachment #8894021 - Flags: review?(n.nethercote) → review+
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/e405ea7e3943
Remove nsIPrefBranch2 and nsIPrefBranchInternal. r=njn
https://hg.mozilla.org/mozilla-central/rev/e405ea7e3943
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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/
(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.
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
(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.
(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.
Duplicate of Bug 795718?
Duplicate of this bug: 795718
Depends on: 1389449
You need to log in before you can comment on or make changes to this bug.