1560485 - Remove use of DOM storage in TB account provisioner

Status: RESOLVED FIXED

(Thunderbird :: General, task)

Description

[Jorg K (CEST = GMT+2)]

Attachment: 1557233-dont-use-DOM-storge.patch

+++ This bug was initially created as a clone of Bug #1557233 +++
+++ This bug was initially created as a clone of Bug #1557018 +++

DOM storage doesn't work well in TB, in bug 1557233 I had to set a pref to get it working again. It's constantly breaking and a maintenance nightmare, see bug 1389673, bug 1515992 and bug 1551533. It's a giant hack and overkill to store a single string against a fake URL "https://accountprovisioner.thunderbird.invalid".

This patch uses a pref instead of DOM storage to store the entered user name in chase the user launches the panel again.

Comment 1

[Magnus Melin [:mkmelin]]

It doesn't work atm (apparently), but that's the point, we should make sure it does.

Comment 2

[Jorg K (CEST = GMT+2)]

Can you please give me one good reason for using DOM storage here.

Why jump through hoops with this code
https://searchfox.org/comm-central/rev/9f64b3c10baf73678700dbb7b82020ce0f54ae34/mail/test/mozmill/newmailaccount/test-newmailaccount.js#1203
https://searchfox.org/comm-central/search?q=.createStorage(&case=false&regexp=false&path=mail
creating fake URLs, principals, running through a heap of DOM security checks and now a disabled client ID check (no idea what that is)
https://hg.mozilla.org/comm-central/rev/fb22bd6b99a7#l1.12
for the sole purpose to move one string from one panel to the other at an absolutely GIGANTIC maintenance cost. Because we can?

I don't get it. The way forward is to reduce reliance on specialised Gecko features and remove their misuse.

Comment 3

[Ben Bucksch (:BenB)]

Comment on attachment 9073160 [details] [diff] [review]
1557233-dont-use-DOM-storge.patch

Good approach.

Please change "username" to "realname", because I think the field stores the user real world name (please double-check, because I can't be certain from the patch context), not the technical username. The term "username" is a fixed term referring to an technical account name, not the real world name. Please change the pref to "mail.provider.realname".

r+ with that change done.

Comment 4

[Ben Bucksch (:BenB)]

(Fixing title, because "account creation dialog" and "account provisioner dialog" are 2 completely different dialogs.)

Comment 5

[Ben Bucksch (:BenB)]

I concur that we shouldn't be using DOM storage here. The pref should work fine.

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1560485-dont-use-DOM-storge.patch (v2)

Renamed the pref. Also restored the saveName() function to reduce code duplication and code churn. Tests pass.

EDIT: Also removed the DOM storage pref.

Comment 7

[Ben Bucksch (:BenB)]

Comment on attachment 9073181 [details] [diff] [review]
1560485-dont-use-DOM-storge.patch (v2)

Good change. Thank you.
r+

Comment 8

[Ben Bucksch (:BenB)]

EDIT: Also removed the DOM storage pref.

I'm presuming you made sure that the tests pass and the problems from bug 1557233 do not re-appear. I trust you on that.

Comment 9

[Jorg K (CEST = GMT+2)]

See comment #6: "Tests pass" ;-)

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/705abdff6892
Use a pref instead of DOM storage for name typed into the new account provisioner. r=BenB

Comment 11

[Jorg K (CEST = GMT+2)]

BTW, that stuff was implemented here:
https://hg.mozilla.org/comm-central/rev/0eb34d6ba3f3#l7.82

The review comment in bug 686347 comment #6 said:
"I wonder if there's a better place to store these things than localstorage, like, say, prefs?"

The answer was:
"As I said above, no. I'm not sure the prefs are fit for storing big chunks of data like this, but we could indeed turn this into a JSON blob that's stored in the prefs."

Well, back in the days, they actually stored three strings, real name, username and domain:
https://hg.mozilla.org/comm-central/rev/0eb34d6ba3f3#l7.95

Somehow two got removed since. I don't think three strings are a "big chunk of data", but we notice that concern was raised before and ignored.

Comment 12

[Magnus Melin [:mkmelin]]

domstorage is not some internal feature, but very much a web feature. That we don't run from a web page makes it not work of course... :/ I still do think a pref is the wrong place to store this temporary data, though arguably it's not important to store it at all.

In the future, please wait for feedback and don't land stuff hours after you requested it, without the feedback arriving.