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

RESOLVED FIXED in Thunderbird 15.0

Status

MailNews Core
Account Manager
--
trivial
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

(Blocks: 3 bugs)

Trunk
Thunderbird 15.0
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

5 years ago
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);
(Assignee)

Comment 1

5 years ago
Created attachment 623430 [details] [diff] [review]
patch
Attachment #623430 - Flags: review?(mconley)

Comment 2

5 years ago
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).
(Assignee)

Comment 3

5 years ago
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

5 years ago
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>
Attachment #623430 - Flags: review-
(Assignee)

Comment 5

5 years ago
(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

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
(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.
(Assignee)

Comment 8

5 years ago
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?
(Assignee)

Comment 9

5 years ago
Created attachment 623442 [details] [diff] [review]
patch v2

What about this?
Attachment #623430 - Attachment is obsolete: true
Attachment #623430 - Flags: review?(mconley)
Attachment #623442 - Flags: review?(iann_bugzilla)

Comment 10

5 years ago
(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.
(Assignee)

Comment 11

5 years ago
Created attachment 623443 [details] [diff] [review]
patch v3

Those JS arrays sometimes deceive me :)
Attachment #623442 - Attachment is obsolete: true
Attachment #623442 - Flags: review?(iann_bugzilla)
Attachment #623443 - Flags: review?(iann_bugzilla)

Comment 12

5 years ago
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.
Attachment #623443 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Updated

5 years ago
Blocks: 755885
(Assignee)

Comment 13

5 years ago
Created attachment 624511 [details] [diff] [review]
patch v4

Ian, yes, I'll try to merge disableIfLocked in bug 755885.
Attachment #623443 - Attachment is obsolete: true
Attachment #624511 - Flags: review?(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.
Attachment #624511 - Flags: review?(mconley) → review+
(Assignee)

Comment 15

5 years ago
Created attachment 626185 [details] [diff] [review]
patch v5
Attachment #624511 - Attachment is obsolete: true
Attachment #626185 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/b7a65151271e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 15.0
You need to log in before you can comment on or make changes to this bug.