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)
Core
Preferences: Backend
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-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/#review200332
This looks sane to me, but redirecting to Kris who knows more about the history here...
Updated•7 years ago
|
Attachment #8924053 -
Flags: review?(aswan) → review?(kmaglione+bmo)
Assignee | ||
Comment 4•7 years ago
|
||
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!
Assignee | ||
Comment 5•7 years ago
|
||
Ahem, if you *see* what I mean.
Comment 6•7 years ago
|
||
mozreview-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/#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 7•7 years ago
|
||
mozreview-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 8•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 9•7 years ago
|
||
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.
Comment 10•7 years ago
|
||
(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)
Comment 11•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/943f22d90aca
https://hg.mozilla.org/mozilla-central/rev/43c726ab7f71
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Comment 12•7 years ago
|
||
Now my legacy extensions don't load the default preferences. How can I do it manually?
Comment 13•7 years ago
|
||
(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.
Comment 14•7 years ago
|
||
(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)
Comment 15•7 years ago
|
||
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?
Comment 16•7 years ago
|
||
(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.
Comment 17•7 years ago
|
||
(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.
Comment 18•7 years ago
|
||
(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.
Comment 19•7 years ago
|
||
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)
Comment 20•7 years ago
|
||
Found bug 1413432.
Comment 21•7 years ago
|
||
(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.
Comment 22•7 years ago
|
||
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)
Comment 23•7 years ago
|
||
Also backed out on a branch:
https://hg.mozilla.org/releases/mozilla-beta/rev/267010f9177f
You need to log in
before you can comment on or make changes to this bug.
Description
•