Closed Bug 1467238 Opened 6 years ago Closed 6 years ago

Remove support code for ispdata


(MailNews Core :: Account Manager, enhancement)

Not set


(Not tracked)

Thunderbird 67.0


(Reporter: aceman, Assigned: benc)




(2 files, 2 obsolete files)

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

After removal of ispdata usage in bug 732106 we can remove some of the support code for ispdata.

jcranmer has patches for the removal at:

This bug needs to refresh the patches:
No longer blocks: 1425962
No longer depends on: 1425356
Keywords: regression

Ben: for the rdf usage in ispUtils which is obsolete since bug 732106, you can probably reuse this bug. Some pointers can be found from the patches below, but many things may already have been cleaned up.

Assignee: acelists → benc


Ben C :)
He's doing RDF removal, and one of the 2 last js usages of rdf-service is in dead code.


Attached patch rmallispdata-2.patch (obsolete) — Splinter Review

Rebased version of the first patch. I'm not really familiar with the code it affects so can't tell you how sensible it looks.
Not sure who's best to review it, so I put you down by default Magnus :-)

Attachment #9045105 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9045105 [details] [diff] [review]

Review of attachment 9045105 [details] [diff] [review]:

Looks reasonable. There's likely a lot of more clean-up to do here, but we do it piece by piece.
Attachment #9045105 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 9045105 [details] [diff] [review]

Review of attachment 9045105 [details] [diff] [review]:


::: mailnews/base/prefs/content/AccountWizard.js
@@ +740,1 @@
>        dump("Invalid account: Got " + accountData + "\n");

The dump()s are possibly unneeded now, we do not try to get any ISP data. And the accountData = null is bogus too, we overwrite it again a line below.

Ben C: Did you check that local mail spool (on Linux) and NNTP can still be set up after this patch?

Ah, nevermind, that was bug 732106.

Still, these cases need to be at least tested once.

(In reply to Ben Bucksch (:BenB) from comment #10)

Still, these cases need to be at least tested once.

I just had a quick go at creating NNTP and movemail account with that patch.
NNTP looked flawless, but movemail seemed to mis-parse a few of the emails. I'd just copied my day-to-day TB inbox into /var/spool/mail/ben. I assumed the TB mbox format is the same as expected for mailspools?
Either way, I think that:

  1. my mbox could have some odd formatting in there from my time fiddling with mbox<->maildir conversions...
  2. it probably could use someone else to try it out
  3. the actual account wizard seemed perfectly smooth and free of problems, so if there is any issue, it's outside the scope of this bug :-)

(I assume there's some mozmill/mochi test to cover the account wizard?)

minor tweaks as per Comment 7.

Attachment #9045105 - Attachment is obsolete: true
Attachment #9045458 - Flags: review+

benc, there might be some differences in the mbox format, it's not completely standardized, just a convention.
The fact that you could set up movemail and see messages means that the code relevant to this bug here worked.
Thanks for testing this!

Attached patch kill-ispdata-aggressive-2.patch (obsolete) — Splinter Review

And here's an update of the part-3 patch.
Again, it's just a straight update without any real understanding of the code, but it seems to work fine for me, YMMV :-)

I think there's to be some linting cleanup work to do on the files touched by these two patches, but I'll leave that until we're happy with the changes rather than obscuring these patches by peppering lots of extra modifications everywhere.

Yes, for readability of the hg/git history, it would be better to land linting changes in a separate patch. Esp. if they would obscure the real changes.

(In reply to Ben Campbell from comment #14)

Created attachment 9045531 [details] [diff] [review]

Ready for review?

(In reply to Magnus Melin [:mkmelin] from comment #16)

(In reply to Ben Campbell from comment #14)

Created attachment 9045531 [details] [diff] [review]

Ready for review?

Yes! just wanted to check up on the lint aspect first.

Attachment #9045531 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9045531 [details] [diff] [review]

Review of attachment 9045531 [details] [diff] [review]:

Looks ok, let's just fix the ugliest ones.
You can also hg qrefresh -U

::: mailnews/base/prefs/content/aw-accounttype.js
@@ +24,5 @@
>      return true;
>  }
>  function setupWizardPanels() {
> +  var pageData = parent.GetPageData();

can we make all the vars let instead, while were touching this

@@ +38,5 @@
> +  var wizardPanels, i;
> +  var isMailAccount = pageData.accounttype.mailaccount;
> +  var isNewsAccount = pageData.accounttype.newsaccount;
> +  if (isMailAccount && isMailAccount.value)
> +    wizardPanels = new Array("identitypage", "incomingpage", "outgoingpage", "accnamepage");

just use [ ]
nobody used new Array anymore

for the if-elses, add braces for them all

@@ +57,3 @@
> +  // Set up order of panels
> +  for (i = 0; i < (wizardPanels.length-1); i++)

decleare let i here, not earlier

::: mailnews/base/prefs/content/aw-identity.js
@@ +112,5 @@
>      emailFieldLabel.setAttribute("value", gPrefsBundle.getString("emailFieldText"));
> +    // Check for obtained values and set with default values if needed
> +    var username = gPrefsBundle.getString("exampleEmailUserName"); 
> +    var domain = gPrefsBundle.getString("exampleEmailDomain"); 

some trailing whitespace here
Attachment #9045531 - Flags: review?(mkmelin+mozilla) → review+

Updated to tidy up some of the touched code.

Attachment #9045531 - Attachment is obsolete: true
Attachment #9046256 - Flags: review+

I'm happy with these patches, but I'm not very confident on this part of the code. Are there any issues with landing them?

Let's get them landed.

Keywords: checkin-needed

Pushed by
Remove support for ISP data in the account wizard. r=mkmelin
Aggressively kill dead code in the account wizard. r=mkmelin DONTBUILD

Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 67.0
You need to log in before you can comment on or make changes to this bug.