1276482 - modernize mailnews/import/content/importDialog.js

Status: RESOLVED FIXED

(MailNews Core :: Import, defect)

Description

[:aceman]

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.

Comment 1

[:aceman]

Attachment: patch

Try run: https://hg.mozilla.org/try-comm-central/rev/7d2dff340754f10b9fde99ba3e8371455eee96d4 . But I doubt there are tests for this dialog.

Comment 2

[Magnus Melin [:mkmelin]]

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.

Comment 3

[:aceman]

(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?

Comment 4

[Magnus Melin [:mkmelin]]

Different apps would have different ids. The postbox id for instance seems to be [e-mail]

But probably no SeaMonkey clones exist, at least that I know.

Comment 5

[:aceman]

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.

Comment 6

[:aceman]

Attachment: patch v1.1

Comment 7

[Philip Chee]

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) {
  ....

Comment 8

[:aceman]

(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.

Comment 9

[:aceman]

Attachment: patch v1.2

Converted more occurrences to the instanceof pattern.

Comment 10

[:aceman]

https://hg.mozilla.org/comm-central/rev/b039b045d92731faf1c3d31dce2b98e13a8bbb49