754579 - convert smtpEditOverlay.js and SmtpServerEdit.js to Services.jsm and MailServices.js

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[:aceman]

SmtpServerEdit.js:            Components.classes["@mozilla.org/messengercompose/smtp;1"].getService(Components.interfaces.nsISmtpService);
SmtpServerEdit.js:      var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService(Components.interfaces.nsIPromptService);
smtpEditOverlay.js:var gPrefBranch = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefBranch);
smtpEditOverlay.js:var gSmtpService = Components.classes["@mozilla.org/messengercompose/smtp;1"].getService(Components.interfaces.nsISmtpService);
smtpEditOverlay.js:    var prefService = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefService);

Comment 1

[:aceman]

Attachment: patch

Comment 2

[Ian Neal]

From what I can see the only consumer of smtpEditOverlay is SmtpServerEdit, so potentially worth merging the files (whether that is in the scope of this bug is another matter).

Comment 3

[:aceman]

I would not be able to do that, so let's keep it outside the bug.
But it seems I forgot to throw am-smtp.js into the patch. That could be in scope.

Comment 4

[Ian Neal]

Comment on attachment 623430 [details] [diff] [review]
patch

>+++ b/mailnews/base/prefs/content/SmtpServerEdit.js

> var smtpServer;
As this is a global it should probably be something like gSmtpServer
> 
> function onLoad(event)
> {
>+    let server = window.arguments[0].server;
>     initializeDialog(server);
As this is the only consumer, you might as well get rid of the initializeDialog function.
> }
> 
> function initializeDialog(server)
> {
>     smtpServer = server;
If you do:
gSmtpServer = window.arguments[0].server;
>     initSmtpSettings(server);
You can use gSmtpServer as the argument here.

>+++ b/mailnews/base/prefs/content/smtpEditOverlay.js

> // Disables xul elements that have associated preferences locked.
> function onLockPreference()
> {
>   try {
>+    let finalPrefString = "mail.smtpserver." +
>+      MailServices.smtp.defaultServer.key + ".";
> 
>+    let allPrefElements = {
>       hostname:     gSmtpHostname,
>       description:  gSmtpDescription,
>       port:         gSmtpPort,
>       authMethod:   gSmtpAuthMethod,
>       try_ssl:      gSmtpSocketType
>     };
> 
>+    gSmtpPrefBranch = Services.prefs.getBranch(finalPrefString);
>+    disableIfLocked(allPrefElements);
Rather than setting a global variable why not either pass it as an argument to the disableIfLocked function or just inline the disableIfLocked function.

> 
> // Does the work of disabling an element given the array which contains xul id/prefstring pairs.
> // Also saves the id/locked state in an array so that other areas of the code can avoid
> // stomping on the disabled state indiscriminately.
This comment is not correct.
>+function disableIfLocked(prefstrArray)
> {
>+  for (let prefstring in prefstrArray)
>     if (gSmtpPrefBranch.prefIsLocked(prefstring))
>       prefstrArray[prefstring].disabled = true;
> }

>+++ b/mailnews/base/prefs/content/smtpEditOverlay.xul
>@@ -42,54 +42,61 @@
> 
> <overlay xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
>          xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> 
>   <script type="application/javascript"
>           src="chrome://messenger/content/smtpEditOverlay.js"/>
> 
>   <vbox id="smtpServerEditor">
> 
>+    <separator class="thin"/>
> 
>       <groupbox>
The indentation is incorrect from here down.
>         <caption label="&security.caption;"/>
> 
>         <grid flex="1">
>           <columns>
>             <column/>
>             <column flex="1"/>
>@@ -137,11 +144,11 @@
>                      accesskey="&userName.accesskey;"
>                      control="smtp.username"/>
>               <textbox id="smtp.username" flex="1"
>                        preftype="string"
>                        prefstring="mail.smtpserver.%serverkey%.username"/>
>             </row>
>           </rows>
>         </grid>
>-     </groupbox>
>+      </groupbox>
This is not correctly indented either.
>   </vbox>
> </overlay>

Comment 5

[:aceman]

(In reply to Ian Neal from comment #4)
> >+    <separator class="thin"/>
> > 
> >       <groupbox>
> The indentation is incorrect from here down.
> >         <caption label="&security.caption;"/>
> > 
> >         <grid flex="1">
> >           <columns>
> >             <column/>
> >             <column flex="1"/>
> >@@ -137,11 +144,11 @@
> >                      accesskey="&userName.accesskey;"
> >                      control="smtp.username"/>
> >               <textbox id="smtp.username" flex="1"
> >                        preftype="string"
> >                        prefstring="mail.smtpserver.%serverkey%.username"/>
> >             </row>
> >           </rows>
> >         </grid>
> >-     </groupbox>
> >+      </groupbox>
> This is not correctly indented either.
> >   </vbox>
> > </overlay>

I didn't want to reindent the whole file, but I can do it :)

Comment 6

[Ian Neal]

(In reply to :aceman from comment #5)
> (In reply to Ian Neal from comment #4)
> > >+    <separator class="thin"/>
> > > 
> > >       <groupbox>
> > The indentation is incorrect from here down.
> > >         <caption label="&security.caption;"/>
> > > 
> > >         <grid flex="1">
> > >           <columns>
> > >             <column/>
> > >             <column flex="1"/>
> > >@@ -137,11 +144,11 @@
> > >                      accesskey="&userName.accesskey;"
> > >                      control="smtp.username"/>
> > >               <textbox id="smtp.username" flex="1"
> > >                        preftype="string"
> > >                        prefstring="mail.smtpserver.%serverkey%.username"/>
> > >             </row>
> > >           </rows>
> > >         </grid>
> > >-     </groupbox>
> > >+      </groupbox>
> > This is not correctly indented either.
> > >   </vbox>
> > > </overlay>
> 
> I didn't want to reindent the whole file, but I can do it :)
Well, if not the whole file, the last change needs to be correctly indented.

Comment 7

[:aceman]

(In reply to Ian Neal from comment #4)
> > // Does the work of disabling an element given the array which contains xul id/prefstring pairs.
> > // Also saves the id/locked state in an array so that other areas of the code can avoid
> > // stomping on the disabled state indiscriminately.
> This comment is not correct.
> >+function disableIfLocked(prefstrArray)
> > {
> >+  for (let prefstring in prefstrArray)
> >     if (gSmtpPrefBranch.prefIsLocked(prefstring))
> >       prefstrArray[prefstring].disabled = true;
> > }

There is the same comment in am-offline.js where the code does what it says.
I wonder if the code here shouldn't do the same. It actually never disables any elements, just sets the array values, that are never used.

Comment 8

[:aceman]

As said, there are similar functions in am-offline.js. It seems these features should be consolidated in am-prefs.js (that is commented as being the place for that). But I am not up to that now.

So, do we want to find out what disableIfLocked in smtpEditrOverlay.js is supposed to do? Or we just ignore it even if it is broken as fixing it may not be in scope here?

Comment 9

[:aceman]

Attachment: patch v2

What about this?

Comment 10

[Ian Neal]

(In reply to :aceman from comment #8)
> As said, there are similar functions in am-offline.js. It seems these
> features should be consolidated in am-prefs.js (that is commented as being
> the place for that). But I am not up to that now.
> 
> So, do we want to find out what disableIfLocked in smtpEditrOverlay.js is
> supposed to do? Or we just ignore it even if it is broken as fixing it may
> not be in scope here?
prefstrArray[prefstring] is the XUL element, so it is setting the XUL element to be disabled.

Comment 11

[:aceman]

Attachment: patch v3

Those JS arrays sometimes deceive me :)

Comment 12

[Ian Neal]

Comment on attachment 623443 [details] [diff] [review]
patch v3

>+++ b/mailnews/base/prefs/content/am-smtp.js

>   refreshServerList: function(aServerKeyToSelect, aFocusList)
>   {
>     // remove all children
>     while (this.mServerList.hasChildNodes())
>       this.mServerList.removeChild(this.mServerList.lastChild);
> 
>+    var defaultServer = MailServices.smtp.defaultServer;
>+    this.fillSmtpServers(this.mServerList, MailServices.smtp.smtpServers, defaultServer);
To be consistent either get rid of the defaultServer variable and inline it into the call, or add one for smtpServers.

>+++ b/mailnews/base/prefs/content/smtpEditOverlay.js

> // Disables xul elements that have associated preferences locked.
> function onLockPreference()
> {
>   try {
>+    let allPrefElements = {
>       hostname:     gSmtpHostname,
>       description:  gSmtpDescription,
>       port:         gSmtpPort,
>       authMethod:   gSmtpAuthMethod,
>       try_ssl:      gSmtpSocketType
>     };
>+    disableIfLocked(allPrefElements);
>+  } catch (e) { // non-fatal
>+    Components.utils.reportError("Error while getting locked prefs: " + e);
>+  }
>+}
> 
>+/**
>+ * Does the work of disabling an element given the array which contains xul id/prefstring pairs.
>+ *
>+ * @param prefstrArray  array of XUL elements to check
>+ */
>+function disableIfLocked(prefstrArray)
>+{
>+  let finalPrefString = "mail.smtpserver." +
>+    MailServices.smtp.defaultServer.key + ".";
>+  let smtpPrefBranch = Services.prefs.getBranch(finalPrefString);
> 
>+  for (let prefstring in prefstrArray)
>+    if (smtpPrefBranch.prefIsLocked(prefstring))
>       prefstrArray[prefstring].disabled = true;
> }
I still don't see why you need a separate disableIfLocked function, unless you are planning for some future shared function.
If not, then just merge disableIfLocked into the onLockPreference function.

r=me with those issues addressed.

Comment 13

[:aceman]

Attachment: patch v4

Ian, yes, I'll try to merge disableIfLocked in bug 755885.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 624511 [details] [diff] [review]
patch v4

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

I'm pretty happy with this, except for the nits I found.

With those fixed, r=me.

::: mailnews/base/prefs/content/SmtpServerEdit.js
@@ +41,2 @@
>  
> +var gSmtpServer;

We prefer let over var.

@@ +43,4 @@
>  
>  function onLoad(event)
>  {
> +    gSmtpServer = window.arguments[0].server;

While you're here, these lines should only be indented 2 spaces.

@@ +48,5 @@
>  }
>  
>  function onAccept()
>  {
>      if (hostnameIsIllegal(gSmtpHostname.value)) {

This function looks great, but each line needs to have two spaces dropped from its indentation.

Comment 15

[:aceman]

Attachment: patch v5

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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