Closed Bug 1538590 Opened 5 years ago Closed 4 years ago

Webextensions API to migrate legacy prefs

Categories

(Thunderbird :: Add-Ons: Extensions API, enhancement, P2)

enhancement

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: Fallen, Unassigned)

References

Details

Attachments

(1 file, 1 obsolete file)

Add-ons will want to migrate their prefs to WebExtensions. For Firefox they did this with those hybird legacy/wx add-ons which we no longer have.

I'm creating an API that will allow migrating prefs from extensions.<key>.* to the local storage of the add-on by specifying legacy_prefs: "<key>" in their manifest.

The only thing I am uncertain on is if it makes sense to add something to the permission prompt, along the lines of "Migrate legacy preferences belonging to '<key>'". This could show if <key> doesn't happen to be the extension id. This would mean strings which makes backporting difficult, and we probably want to backport this. We could also assert this in policy on ATN, but that wouldn't catch them all.

Attached patch Fix - v1 (obsolete) β€” β€” Splinter Review
Attachment #9053166 - Flags: review?(geoff)

I have a bad feeling about this that I can't quite put my finger on. Something about assuming everything went okay and deleting the pref branch doesn't feel right.

I had imagined a more transactional approach, where the extension requests the preferences, then decides what to do with the returned values. When everything is okay, it asks for the preferences to be removed.

I see your point, though I'm not sure how to best solve it. The issue with the transactional approach where deleting is optional is that it is an escape hatch to keep using legacy prefs. The developer could just keep requesting all their legacy prefs without ever deleting them.

If something goes wrong, I'm hoping that the code would throw along the way and then the deleteBranch never happens.

See Also: → 1529932
See Also: 1529932

(In reply to Philipp Kewisch [:Fallen] [:πŸ“†] from comment #3)

an escape hatch to keep using legacy prefs. The developer could just keep requesting all their legacy prefs without ever deleting them.

They'll have no method to set the legacy prefs so they can't present any kind of UI to do so.

If something goes wrong, I'm hoping that the code would throw along the way and then the deleteBranch never happens.

What if the something going wrong is another extension with overlapping pref names? What if the developer screws it all up and releases something broken that deletes all their users' prefs? I think this is riskier than it looks and it's the sort of thing people get very mad about if we mess it up.

Also, what if the extensions doesn't store their prefs where we expect them? That's hardly uncommon.

Comment on attachment 9053166 [details] [diff] [review]
Fix - v1

R- on that. Feel free to invite other opinions though.
Attachment #9053166 - Flags: review?(geoff) → review-

I'm bothered by the fact that with WebExtensions, that preferences are stored in local storage and not in prefs.js anymore. This also means that the user can't see all prefs in one go in the file and probably also not anymore in the config editor, as it is no longer possible for the WebExtension to initialize the prefs on the default preferences branch or to get/set the value or add observers to watch for changed preferences, either from the add-on itself or from the main application, or am I missing something here?

Another concern about local storage prefs: How is it possible to get a complete Backup including these local storage prefs?

WebExtensions shouldn't be observing preferences from the main application. If there is something that needs to be observed, then this should be incorporated into a different WebExtensions API that can be listened to.

If an add-on wants to show its preferences, it can do so on the options page. It has fine grained control over all the options and can decide on presentation themselves.

If you need to observe preference changes on your own extension, you can use the storage onChanged notifications. If you need to back up your preferences, the extension can provide an export option. If you want to back up without the add-on providing a such feature you can do so with the developer tools.

What I can say would not fit with the concept of WebExtensions is exposing about:config via an API.

Attached patch Fix - v2 β€” β€” Splinter Review

Here is your transactional approach. A note this won't allow extensions that store their prefs somewhere wild to access them, but those could still work with an experiment.

It would also still allow developers to screw things up by purging preferences from a branch they were not involved with. The manifest approach could have allowed us to show something in the permission dialog, I'm not sure how that would work out here or if it is worth the effort.

Attachment #9053166 - Attachment is obsolete: true
Attachment #9059707 - Flags: review?(geoff)

(In reply to Philipp Kewisch [:Fallen] [:πŸ“†] from comment #10)

It would also still allow developers to screw things up by purging preferences from a branch they were not involved with. The manifest approach could have allowed us to show something in the permission dialog, I'm not sure how that would work out here or if it is worth the effort.

You could still store the branch name in the manifest and remove the argument to the functions. We could use that to warn the user but I'm not sure there's any benefit. Not having the function arguments would prevent abuse as an extension would only be able to read/delete one pref branch.

No action here since April? Looks like you're waiting for Philipp. I'd just cancel the review to make it clear.

Why not just encourage developers to do this via the experiments approach and give them examples? Not all developers have been using extension.* (e.g. conversations), and migrating manually means they can get everything to the right places.

I was kind of hoping that this would be a way to avoid developers needing to use an experiments API at all if this is the only thing they need from the legacy code. Given we currently aren't disabling experiments we could postpone this and instead reconsider once we have somewhat complete APIs and developers haven't migrated their prefs.

Attachment #9059707 - Flags: review?(geoff)
Priority: -- → P2

Geoff, can you take on finishing this off.
I would not be too concerned about leaving the pref in.

Assignee: philipp → geoff

Talked to Geoff and we decided not to implement this.

Assignee: geoff → nobody
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: