Closed Bug 1413413 Opened 7 years ago Closed 7 years ago

Remove support for extensions having their own prefs file

Categories

(Core :: Preferences: Backend, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

Attachments

(2 files)

Now that legacy extensions are no longer supported, we don't need generic extension prefs support.
Comment on attachment 8924053 [details]
Bug 1413413 (part 2) - Remove support for extensions having their own prefs file. .

https://reviewboard.mozilla.org/r/195272/#review200332

This looks sane to me, but redirecting to Kris who knows more about the history here...
Attachment #8924053 - Flags: review?(aswan) → review?(kmaglione+bmo)
Thanks for redirecting, aswan,

kmag, what I'm after from you is a general "yes, this functionality is dead" rather than a fine-grained review, if you wee what I mean. Thanks!
Blocks: 1413432
Ahem, if you *see* what I mean.
Comment on attachment 8924053 [details]
Bug 1413413 (part 2) - Remove support for extensions having their own prefs file. .

https://reviewboard.mozilla.org/r/195272/#review200334

::: modules/libpref/Preferences.cpp:2572
(Diff revision 1)
>        // Debugging to see why we end up with very long strings here with
>        // some addons, see bug 836263.

This comment can be removed now.

::: toolkit/xre/nsXREDirProvider.cpp
(Diff revision 1)
> -    if (mProfileDir) {
> -      nsCOMPtr<nsIFile> overrideFile;
> -      mProfileDir->Clone(getter_AddRefs(overrideFile));
> -      overrideFile->AppendNative(NS_LITERAL_CSTRING(PREF_OVERRIDE_DIRNAME));
> -
> -      bool exists;
> -      if (NS_SUCCEEDED(overrideFile->Exists(&exists)) && exists)
> -        directories.AppendObject(overrideFile);
> -    }

Hmm... I wonder if anyone is still using this...

If I had to guess, I'd say it's probably being used by malware distributors, and by a few users who will complain. But those probably aren't good enough reasons to keep it.
Attachment #8924053 - Flags: review?(kmaglione+bmo) → review+
Comment on attachment 8924052 [details]
Bug 1413413 (part 1) - Remove unused "@mozilla.org/preferences;1" ID. .

https://reviewboard.mozilla.org/r/195270/#review200346
Attachment #8924052 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8924053 [details]
Bug 1413413 (part 2) - Remove support for extensions having their own prefs file. .

https://reviewboard.mozilla.org/r/195272/#review200348

::: commit-message-a7377:9
(Diff revision 1)
> +
> +Pieces removed include the following.
> +
> +- The "load-extension-default" observer notification.
> +
> +- The code for reading defaults/preferences/*.js.

from addons. We still read the one from the app.
Attachment #8924053 - Flags: review?(mh+mozilla) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/943f22d90aca3ac389b97c9ac3b19452ec130f4a
Bug 1413413 (part 1) - Remove unused "@mozilla.org/preferences;1" ID. r=glandium.

https://hg.mozilla.org/integration/mozilla-inbound/rev/43c726ab7f71353f4b8d0c14bca27d65edc6ad99
Bug 1413413 (part 2) - Remove support for extensions having their own prefs file. r=glandium,kmag.
(Looks like there wouldn't be any harm done by assigning this bug.)

Nicholas, could you advise us as to how to best adopt the removed functionality into TB:
https://hg.mozilla.org/integration/mozilla-inbound/rev/43c726ab7f71353f4b8d0c14bca27d65edc6ad99#l1.83
doesn't appear to be a lot of high-maintenance complicated code. The problems are 1) where to call this code and 2) how to gain access to the 'static void' functions like PREF_InitParseState.
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Flags: needinfo?(n.nethercote)
Now my legacy extensions don't load the default preferences. How can I do it manually?
(In reply to Oriol Brufau [:Oriol] from comment #12)
> Now my legacy extensions don't load the default preferences. How can I do it
> manually?

Honestly, if you're still using non-bootstrapped extensions, you're going to have much bigger problems soon.

(In reply to Jorg K (GMT+2) from comment #10)
> (Looks like there wouldn't be any harm done by assigning this bug.)
> 
> Nicholas, could you advise us as to how to best adopt the removed
> functionality into TB:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 43c726ab7f71353f4b8d0c14bca27d65edc6ad99#l1.83
> doesn't appear to be a lot of high-maintenance complicated code. The
> problems are 1) where to call this code and 2) how to gain access to the
> 'static void' functions like PREF_InitParseState.

The traditional route bootstrapped extensions have taken in the past is to just use the subscript loader to load the prefs.js files into a scope with pref and user_pref functions defined.
(In reply to Jorg K (GMT+2) from comment #10)
> Nicholas, could you advise us as to how to best adopt the removed
> functionality into TB:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 43c726ab7f71353f4b8d0c14bca27d65edc6ad99#l1.83
> doesn't appear to be a lot of high-maintenance complicated code. The
> problems are 1) where to call this code and 2) how to gain access to the
> 'static void' functions like PREF_InitParseState.

Also, I'd advise against trying to use any pref service internals here. This code was removed because the pref service is being massively refactored in preparation for a rewrite in Rust. If you write any code that relies on its implementation, it is pretty much guaranteed to break again soon.
Flags: needinfo?(n.nethercote)
Oh, you beat me to it, I was going to clear the NI. Philipp is in the process of preparing something in bug 1414398, please take a look.

> Honestly, if you're still using non-bootstrapped extensions, you're going to have much bigger problems soon.
I'd say most TB extension as non-bootstrapped since they overload part of the UI. I'm just a novice add-on author, I found that for simple things bootstrapped extensions are OK, but for heavy stuff it needs to be non-bootstrapped. Our calendar add-on Lightning is non-bootstrapped.

What's on the books?
(In reply to Jorg K (GMT+2) from comment #15)
> > Honestly, if you're still using non-bootstrapped extensions, you're going to have much bigger problems soon.
>
> I'd say most TB extension as non-bootstrapped since they overload part of
> the UI. I'm just a novice add-on author, I found that for simple things
> bootstrapped extensions are OK, but for heavy stuff it needs to be
> non-bootstrapped. Our calendar add-on Lightning is non-bootstrapped.

It's perfectly possible for bootstrapped add-ons to overload parts of the UI. We've had many heavyweight bootstrapped add-ons that do just that, in Firefox.

Regardless, support for non-bootstrapped legacy extensions is being removed from the add-on manager and other parts of the platform. It's a huge maintenance burden, and we can't continue to support it just for the sake of Thunderbird.

> What's on the books?

Not sure what you mean.
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #16)
> > What's on the books?
> Not sure what you mean.
Umm, "What's on the books?" was enquiring about your plans. You've already answered that, thank you.
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #13)
> The traditional route bootstrapped extensions have taken in the past is to
> just use the subscript loader to load the prefs.js files into a scope with
> pref and user_pref functions defined.

Thanks, I defined the following pref function and manually ran the prefs.js file in the browser console and now the extension works again:

  function pref(n, v) {
    var p = Components.classes["@mozilla.org/preferences-service;1"]
           .getService(Components.interfaces.nsIPrefService);
    switch (typeof v) {
      case "boolean": p.setBoolPref(n, v); return;
      case "number": p.setIntPref(n, v); return;
      case "string": p.setCharPref(n, v); return;
    }
  }

But this doesn't seem the right approach, I think I should define these values as the defaults instead of as user values. But I didn't find any method to define the default.
Kris, coming back to your comment #16:
Could you give us a summary of what is planned for legacy add-ons, maybe post to
  Thunderbird email developers <maildev@lists.thunderbird.net>

In comment #16 you're saying that support for non-bootstrapped add-ons will be removed. When will that happen? Is there a bug we can follow? What about bootstrapped add-ons? For how long will they be supported? What about devtools? They are going to be turned onto a system add-on (bug 1369801). And just an ignorant question: What's a system add-on. There is also mention of system add-ons in the "Proposal to remove some preferences override support" thread on dev-platform.
Flags: needinfo?(kmaglione+bmo)
(In reply to Oriol Brufau [:Oriol] from comment #18)
> But this doesn't seem the right approach, I think I should define these
> values as the defaults instead of as user values. But I didn't find any
> method to define the default.

Use nsIPrefService.getDefaultBranch and operate on the default branch.
Depends on: 1416279
For the record:
Thunderbird 58 beta building with this backed out via comm-beta patch:
https://hg.mozilla.org/releases/comm-beta/rev/c1f5d46aa9613ef790fa73dbfdea95fd0f9a7536
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.