Closed Bug 1276482 Opened 4 years ago Closed 4 years ago

modernize mailnews/import/content/importDialog.js

Categories

(MailNews Core :: Import, defect, trivial)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 50.0

People

(Reporter: aceman, Assigned: aceman)

Details

Attachments

(1 file, 2 obsolete files)

54.55 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
The file mailnews/import/content/importDialog.js uses strange syntax in some places, e.g.:
1. if ( expression )
2. return (expression)
3. uses possibly unneeded top. to reference global variables
4. depends on 'var'-declared variables to exist in parent scopes
5. uses listbox.setAttribute("selectedIndex", value) which is undocumented, and should be called as 'listbox.selectedIndex = value' property.

I'd like to clean up this file.
Attached patch patch (obsolete) — Splinter Review
Try run: https://hg.mozilla.org/try-comm-central/rev/7d2dff340754f10b9fde99ba3e8371455eee96d4 . But I doubt there are tests for this dialog.
Attachment #8762316 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8762316 [details] [diff] [review]
patch

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

::: mailnews/import/content/importDialog.js
@@ +1017,5 @@
>      if (radioGroup.value == "all")
>      {
>        let args = { closeMigration: true };
> +      if (Services.appinfo.ID == "{3550f703-e582-4d05-9a08-453d09bdfdc6}") {
> +        // Running as Thunderbird.

why not the name instead? but see below

@@ +1026,5 @@
> +        window.openDialog("chrome://communicator/content/migration/migration.xul",
> +                          "", "chrome,dialog,modal,centerscreen");
> +      } else {
> +        Components.utils.reportError("Running as an unsupported application.");
> +      }

I don't think you want to conver ifdef thunderbirds like this. If anything you probably want to special case the seamonkey part, but there might be Thunderbird derivatives, so you could just put that in the else clause.
Attachment #8762316 - Flags: review?(mkmelin+mozilla) → review+
(In reply to Magnus Melin from comment #2)
> >        let args = { closeMigration: true };
> > +      if (Services.appinfo.ID == "{3550f703-e582-4d05-9a08-453d09bdfdc6}") {
> > +        // Running as Thunderbird.
> 
> why not the name instead? but see below

Because I assumed TB clones would change the name, but not the ID (for addons). So ID seemed safer.

OK, if we turn it around and check for Seamonkey (by name or ID?) we could catch all the cloned by the else branch. So you assume there are no clones of Seamonkey, right?
Different apps would have different ids. The postbox id for instance seems to be postbox@postbox-inc.com

But probably no SeaMonkey clones exist, at least that I know.
OK, I'll reverse the condition.

If we wanted to have the same code for all, we could easily make the Seamonkey's migration.xul take the same args as TB, but I don't know to merge the different paths to the xul files.
Attached patch patch v1.1 (obsolete) — Splinter Review
Attachment #8762316 - Attachment is obsolete: true
Attachment #8762725 - Flags: review?(philip.chee)
Attachment #8762725 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8762725 [details] [diff] [review]
patch v1.1

r=me with the following issues fixed - or explained why not fixed.

> -  progressMeter.setAttribute( "value", val);
> +  progressMeter.setAttribute("value", val);
progressMeter.value = val;

> -    moduleArray[i] = {name:top.importService.GetModuleName(top.importType, i), index:i };
> +    moduleArray[i] = { name:gImportService.GetModuleName(gImportType, i), index:i };
{ name: gImportService.GetModuleName(gImportType, i), index: i }

> +    if (doesWantProgress)
> +    {
> +      var deck = document.getElementById("stateDeck");
> +      var header = document.getElementById("header");
> +      var progressStatusEl = document.getElementById("progressStatus");
> +      var progressTitleEl = document.getElementById("progressTitle");
....
> +      let meterText = gImportMsgsBundle.getFormattedString("AddrProgressMeterText", [ name ]);
If you are going to convert var's to let's you should do that consistently.

>    var strings = aString.split("\n");
> -  for (var i = 0; i < strings.length; i++) {
> +  for (let i = 0; i < strings.length; i++) {
>      if (strings[i]) {
how about:
for (let string of strings) {
  if (string)

> +            filePicker.init(window,
> +                            gImportMsgsBundle.getString("ImportSelectSettings"),
> +                            Ci.nsIFilePicker.modeOpen);
...............................filePicker.modeOpen
> +            filePicker.appendFilters(Ci.nsIFilePicker.filterAll);
...............filePicker.appendFilters(filePicker.filterAll);
(Since you already QI'd filePicker to nsIFilePicker)
... Several usages of nsIFilePicker ...

>    if (map != null) {
> -    map = map.QueryInterface( Components.interfaces.nsIImportFieldMap);
> +    map = map.QueryInterface(Ci.nsIImportFieldMap);

You can combine these two lines into:
if (map instance of Ci.nsIImportFieldMap) { ....
(instance of will QI for you behind the curtains)

>    var filtersInterface = module.GetImportInterface("filters");
>    if (filtersInterface != null)
> -    filtersInterface = filtersInterface.QueryInterface(Components.interfaces.nsIImportFilters);
> +    filtersInterface = filtersInterface.QueryInterface(Ci.nsIImportFilters);
>    if (filtersInterface == null) {
>      error.data = gImportMsgsBundle.getString('ImportFiltersBadModule');
> -    return( false);
> +    return false;
>    }

if (filtersInterface instance of Ci.nsIImportFilters) {
  return filtersInterface.Import(error);
}
error.data = gImportMsgsBundle.getString('ImportFiltersBadModule');
return false;

> -#endif
> +      if (Services.appinfo.ID == "{92650c4d-4b8e-4d2a-b7eb-24ecf4f6b63a}") {
> +        // Running as Seamonkey.
> +        window.openDialog("chrome://communicator/content/migration/migration.xul",
> +                          "", "chrome,dialog,modal,centerscreen");

Instead of adding a comment I suggest:

let SEAMONKEY_ID = "{92650c4d-4b8e-4d2a-b7eb-24ecf4f6b63a}";
if (Services.appinfo.ID == SEAMONKEY_ID) {
  ....
Attachment #8762725 - Flags: review?(philip.chee) → review+
(In reply to Philip Chee from comment #7)
> > +    if (doesWantProgress)
> > +    {
> > +      var deck = document.getElementById("stateDeck");
> > +      var header = document.getElementById("header");
> > +      var progressStatusEl = document.getElementById("progressStatus");
> > +      var progressTitleEl = document.getElementById("progressTitle");
> ....
> > +      let meterText = gImportMsgsBundle.getFormattedString("AddrProgressMeterText", [ name ]);
> If you are going to convert var's to let's you should do that consistently.

I can do this block of vars, but I am not converting all vars to let, only in lines that I touch anyway.

I do the other improvements, thanks.
Attached patch patch v1.2Splinter Review
Converted more occurrences to the instanceof pattern.
Attachment #8762725 - Attachment is obsolete: true
Attachment #8762725 - Flags: review?(mkmelin+mozilla)
Attachment #8762752 - Flags: review?(mkmelin+mozilla)
Attachment #8762752 - Flags: review?(mkmelin+mozilla) → review+
https://hg.mozilla.org/comm-central/rev/b039b045d92731faf1c3d31dce2b98e13a8bbb49
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 50.0
You need to log in before you can comment on or make changes to this bug.