Closed Bug 1467238 Opened 6 years ago Closed 5 years ago

Remove support code for ispdata

Categories

(MailNews Core :: Account Manager, enhancement)

Product:
MailNews Core
Component:
Account Manager
Type:
enhancement
Priority:
Not set
Severity:
minor

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 67.0

People

(Reporter: aceman, Assigned: benc)

References

Details

Attachments

(2 files, 2 obsolete files)

rmallispdata-3.patch
(deleted), patch
benc
: review+
Details | Diff | Splinter Review
kill-ispdata-aggressive-3.patch
(deleted), patch
benc
: review+
Details | Diff | Splinter Review 
+++ 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:
https://hg.mozilla.org/users/Pidgeot18_gmail.com/patch-queues/file/tip/patches-derdf/

This bug needs to refresh the patches:
rmallispdata
kill-ispdata-aggressive
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.

https://hg.mozilla.org/users/Pidgeot18_gmail.com/patch-queues/file/tip/patches-derdf/rmallispdata

https://hg.mozilla.org/users/Pidgeot18_gmail.com/patch-queues/file/tip/patches-derdf/kill-ispdata-aggressive

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.

Great!

Attached patch rmallispdata-2.patch (obsolete) (deleted) — — 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]
rmallispdata-2.patch

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]
rmallispdata-2.patch

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

Thanks.

::: 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?)

Attached patch rmallispdata-3.patch (deleted) — — Splinter Review

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) (deleted) — — 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]
kill-ispdata-aggressive-2.patch

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]
kill-ispdata-aggressive-2.patch

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]
kill-ispdata-aggressive-2.patch

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+
Attached patch kill-ispdata-aggressive-3.patch (deleted) — — Splinter 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 mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/71a714fc1d04
Remove support for ISP data in the account wizard. r=mkmelin
https://hg.mozilla.org/comm-central/rev/5bf193df84a9
Aggressively kill dead code in the account wizard. r=mkmelin DONTBUILD

Status: NEW → RESOLVED
Closed: 5 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.

Attachment

General

Creator:
Created:
Updated:
Size: