Closed Bug 732106 Opened 8 years ago Closed Last year

Remove support for ispdata (and fix movemail account creation)

Categories

(MailNews Core :: Account Manager, defect)

defect
Not set

Tracking

(thunderbird_esr60 fixed, thunderbird60 fixed, thunderbird61 wontfix, thunderbird62 fixed)

RESOLVED FIXED
Thunderbird 62.0
Tracking Status
thunderbird_esr60 --- fixed
thunderbird60 --- fixed
thunderbird61 --- wontfix
thunderbird62 --- fixed

People

(Reporter: jcranmer, Assigned: jcranmer)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(4 files, 2 obsolete files)

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!).
See Also: → 732109
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
Assignee: nobody → Pidgeot18
Status: NEW → ASSIGNED
Attachment #815371 - Flags: review?(mnyromyr)
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.
Attachment #815373 - Flags: review?(mnyromyr)
Attachment #815371 - Flags: feedback+
(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?
It's still used in thunderbird for newsgroups and movemail.
Pinging for review on these patches?
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 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
Attachment #815371 - Flags: review+
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
Attachment #815372 - Flags: review+
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
Attachment #815373 - Flags: review+
Blocks: 1425962
Depends on: 1425356
Keywords: regression
Summary: Remove support for ispdata → Remove support for ispdata (and fix movemail account creation)
Version: unspecified → Trunk
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?
Attachment #815371 - Attachment is obsolete: true
Attachment #815371 - Flags: review?(mnyromyr)
Attachment #8972275 - Flags: ui-review?(richard.marti)
Attachment #8972275 - Flags: review?(ben.bucksch)
Attachment #8972275 - Flags: feedback?(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.
Attachment #8972275 - Flags: feedback?(alta88) → feedback+
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+.
Attachment #8972275 - Flags: ui-review?(richard.marti) → ui-review+
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?
Attachment #8972275 - Flags: review?(mkmelin+mozilla)
Attachment #8972275 - Flags: review?(jorgk)
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.
Flags: needinfo?(acelists)
Attachment #8972275 - Flags: review?(jorgk) → review+
(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?
Flags: needinfo?(acelists)
(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 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
Attachment #8972275 - Flags: review?(mkmelin+mozilla)
Attachment #8972275 - Flags: review?(ben.bucksch)
Attachment #8972275 - Flags: review+
Thanks, fixed the hard-coded string.
Attachment #8972275 - Attachment is obsolete: true
Attachment #8983611 - Flags: review+
Keywords: checkin-needed
Whiteboard: [leave open]
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]
Attachment #8983611 - Flags: approval-comm-esr60+
Attachment #8983611 - Flags: approval-comm-beta+
Pushed by mozilla@jorgk.com:
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
Status: ASSIGNED → RESOLVED
Closed: Last year
Keywords: checkin-needed
Resolution: --- → FIXED
I'm closing this. Maybe we want to revisit this bug to land parts 2 and 3.
Target Milestone: --- → Thunderbird 62.0
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.
I think you can file it since you'll know better what this is all about.
Blocks: 1467238
Whiteboard: [leave open]
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/0e040d8fb364
Followup to adjust the suite package manifest. r=frg
Joerg, could you approve and land this when the base patch hits comm-esr60.
Flags: needinfo?(jorgk)
Attachment #8984107 - Flags: review+
Attachment #8984107 - Flags: approval-comm-esr60?
Comment on attachment 8984107 [details] [diff] [review]
732106-manifest.patch

Sure.
Flags: needinfo?(jorgk)
Attachment #8984107 - Flags: approval-comm-esr60? → approval-comm-esr60+
Attachment #815372 - Attachment description: Part 2: Remove support for ISP data in the account wizard → Part 2: Remove support for ISP data in the account wizard (will go to bug 1467238)
Flags: needinfo?(jorgk)
Attachment #815372 - Flags: review?(mnyromyr)
Attachment #815373 - Attachment description: Part 3: Aggressively kill dead code in the account wizard → Part 3: Aggressively kill dead code in the account wizard (will go to bug 1467238)
Attachment #815373 - Flags: review?(mnyromyr)
You need to log in before you can comment on or make changes to this bug.