732106 - Remove support for ispdata (and fix movemail account creation)

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[Joshua Cranmer [:jcranmer]]

The new account configuration setups mean that bundling ispdata is only necessary now for the movemail/rss account setup. Therefore, we ought to move those account setups out of using ispdata. Once that is done, we can remove ispdata (and yet another use of RDF!).

Comment 1

[Ben Bucksch (:BenB)]

Just for reference, the new account wizard supports configs in the isp/ dir in the install directory, in XML format
https://developer.mozilla.org/en/Thunderbird/Autoconfiguration#Mechanisms
https://developer.mozilla.org/en/Thunderbird/Autoconfiguration/FileFormat/HowTo

Comment 2

[Joshua Cranmer [:jcranmer]]

Attachment: Part 1: Move movemail to not use ISP data and remove ispdata in the tree

Comment 3

[Joshua Cranmer [:jcranmer]]

Attachment: Part 2: Remove support for ISP data in the account wizard (will go to bug 1467238)

Comment 4

[Joshua Cranmer [:jcranmer]]

Attachment: Part 3: Aggressively kill dead code in the account wizard (will go to bug 1467238)

I separated this out from part 2 because I'm not as sure about this code--I'm testing using Thunderbird, so I can't see very well if I'm breaking SeaMonkey's use of this.

Comment 5

[[e-mail]]

(In reply to Joshua Cranmer from comment #0)
> The new account configuration setups mean that bundling ispdata is only
> necessary now for the movemail/rss account setup.

Actually SeaMonkey still uses ispdata for RSS too. There's a bug for removing it but I got asked to review it and I'm in no hurry to remove RDF just because some people prefer the extra work of dedicated functions to create RSS and movemail accounts, so you'd better ask Mnyromyr to review it.

In fact, I'm surprised that Thunderbird is still using the old account wizard. Can't you just stop building it?

Comment 6

[Magnus Melin [:mkmelin]]

It's still used in thunderbird for newsgroups and movemail.

Comment 7

[Joshua Cranmer [:jcranmer]]

Pinging for review on these patches?

Comment 8

[Philip Chee]

In the interest of getting things moving, if I don't hear from Mnyromyr or NeilAway in the next seven days, I'll proactively rs all the patches - assuming you'll fix the bit-rot.

Comment 9

[Philip Chee]

Comment on attachment 815371 [details] [diff] [review]
Part 1: Move movemail to not use ISP data and remove ispdata in the tree

rs=me on condition you fix the bit-rot

Comment 10

[Philip Chee]

Comment on attachment 815372 [details] [diff] [review]
Part 2: Remove support for ISP data in the account wizard (will go to bug 1467238)

rs=me on condition you fix the bit-rot

Comment 11

[Philip Chee]

Comment on attachment 815373 [details] [diff] [review]
Part 3: Aggressively kill dead code in the account wizard (will go to bug 1467238)

rs=me on condition you fix the bit-rot

Comment 12

[:aceman]

Attachment: Part 1: Move movemail to not use ISP data and remove ispdata/ in the tree, v1.1

This takes the current version of the 'part 1' patch from https://hg.mozilla.org/users/Pidgeot18_gmail.com/patch-queues/file/tip/patches-derdf/notispdata and updates it for current trunk.
Aside from unbitrotting, the logical changes I had to do are:
-Remove changing Makefile.in failes, which no longer exist and I had to touch the corresponding moz.build files.
-The patch restored rss account creation in AccountWizard.xul. Thus for Thunderbird there were 2 ways to create RSS account: New -> RSS account and New -> Other account -> Feeds and Blogs (the restored one). With the patch the latter one didn't work later on in the wizard. So I removed this latter one in favour of the one that works (for TB only). Alta88, can you please check this and comment if that is right?

Comment 13

[alta88]

Comment on attachment 8972275 [details] [diff] [review]
Part 1: Move movemail to not use ISP data and remove ispdata/ in the tree, v1.1

Review of attachment 8972275 [details] [diff] [review]:
-----------------------------------------------------------------

I assume this doesn't change any existing Tb behavior for the feeds wizard, so fine.  The rdf file for the old account wizard rss part, used only in SM and only used for strings, wasn't ever updated (per Tb) since there was a former reviewer who really liked rdf despite the well known eventual deprecation.

Comment 14

[Richard Marti (:Paenglab)]

Comment on attachment 8972275 [details] [diff] [review]
Part 1: Move movemail to not use ISP data and remove ispdata/ in the tree, v1.1

(In reply to :aceman from comment #12)
> Created attachment 8972275 [details] [diff] [review]
> Part 1: Move movemail to not use ISP data and remove ispdata/ in the tree,
> v1.1
> 
> This takes the current version of the 'part 1' patch from
> https://hg.mozilla.org/users/Pidgeot18_gmail.com/patch-queues/file/tip/
> patches-derdf/notispdata and updates it for current trunk.
> Aside from unbitrotting, the logical changes I had to do are:
> -Remove changing Makefile.in failes, which no longer exist and I had to
> touch the corresponding moz.build files.
> -The patch restored rss account creation in AccountWizard.xul. Thus for
> Thunderbird there were 2 ways to create RSS account: New -> RSS account and
> New -> Other account -> Feeds and Blogs (the restored one). With the patch
> the latter one didn't work later on in the wizard. So I removed this latter
> one in favour of the one that works (for TB only). Alta88, can you please
> check this and comment if that is right?

I suppose you asked me for only going the through New --> Fedd account. This is the way we already go, so ui-r+.

Comment 15

[:aceman]

Comment on attachment 8972275 [details] [diff] [review]
Part 1: Move movemail to not use ISP data and remove ispdata/ in the tree, v1.1

This would be useful to get into TB60, unless we are OK with having this Movemail account creation broken and mentioned in Known issues in the release notes.
Ben, can you look at this?
Or does somebody else feel like reviewing it?

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 8972275 [details] [diff] [review]
Part 1: Move movemail to not use ISP data and remove ispdata/ in the tree, v1.1

rs=jorgk. Looks good. You tested this, right? What about part 2 and part 3? I'm sure those patches are rotten. If they are required, please refresh, otherwise, obsolete.

Comment 17

[:aceman]

(In reply to Richard Marti (:Paenglab) from comment #14)
> I suppose you asked me for only going the through New --> Fedd account. This
> is the way we already go, so ui-r+.

Can you please also try the Movemail account creation on Linux?

Comment 18

[:aceman]

(In reply to Jorg K (GMT+2) (bustage-fix and urgent reviews only) from comment #16)
> Comment on attachment 8972275 [details] [diff] [review]
> rs=jorgk. Looks good. You tested this, right? What about part 2 and part 3?
> I'm sure those patches are rotten. If they are required, please refresh,
> otherwise, obsolete.

It seems to me those are further cleanup not strictly required to make movemail creation work Thunderbird again. I will do those after part 1 is finished and those do not need to be landed in TB60 (only part 1 needs to), but of course they can in a later point release.

Comment 19

[Magnus Melin [:mkmelin]]

Comment on attachment 8972275 [details] [diff] [review]
Part 1: Move movemail to not use ISP data and remove ispdata/ in the tree, v1.1

Review of attachment 8972275 [details] [diff] [review]:
-----------------------------------------------------------------

Seems to set up the account alright. r=mkmelin

::: mailnews/base/prefs/content/aw-accounttype.js
@@ +76,5 @@
> +          setPageData(pageData, "server", "hostname", "localhost");
> +          setPageData(pageData, "server", "port", "");
> +        } else if (pages == "rss") {
> +          SetCurrentAccountData({
> +            wizardAccountName: "Feeds", // XXX: &feeds.accountName;

needs work?

@@ +92,5 @@
> +          setPageData(pageData, "server", "username", "nobody");
> +          setPageData(pageData, "identity", "email", null);
> +          setPageData(pageData, "identity", "fullName", null);
> +        } else {
> +          wizardPanels = button.value.split(/ *, */);

please keep the comment that this could be an account added by an add-on

Comment 20

[:aceman]

Attachment: Part 1: Move movemail to not use ISP data and remove ispdata/ in the tree, v1.2

Thanks, fixed the hard-coded string.

Comment 21

[Jorg K (CEST = GMT+2)]

Comment on attachment 8983611 [details] [diff] [review]
Part 1: Move movemail to not use ISP data and remove ispdata/ in the tree, v1.2

[Triage Comment]

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4ac7393cb694
restore movemail and rss account creation option in account wizard and remove ispdata rdf files. r=mkmelin, ui-r=Paenglab

Comment 23

[Jorg K (CEST = GMT+2)]

I'm closing this. Maybe we want to revisit this bug to land parts 2 and 3.

Comment 24

[:aceman]

Yes, we definitely want the other parts to land (it is RDF). If you want it to go in a new bug, you can please file it and I'll refresh the patches.

Comment 25

[Jorg K (CEST = GMT+2)]

I think you can file it since you'll know better what this is all about.

Comment 26

[Jorg K (CEST = GMT+2)]

TB 60 beta 7 (BETA_60_CONTINUATION branch):
https://hg.mozilla.org/releases/comm-beta/rev/efee84953cde487b3cbbaa7cb14b2e29ea220501

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/0e040d8fb364
Followup to adjust the suite package manifest. r=frg

Comment 28

[Frank-Rainer Grahl (:frg)]

Attachment: 732106-manifest.patch

Joerg, could you approve and land this when the base patch hits comm-esr60.

Comment 29

[Jorg K (CEST = GMT+2)]

Comment on attachment 8984107 [details] [diff] [review]
732106-manifest.patch

Sure.

Comment 30

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-esr60/rev/c6dd6ec3b660

Still need to uplift the patch from comment 29.

Comment 31

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-esr60/rev/b65a6bc69c12620739164fff934646fb4cdf53a8